Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

tls_support #24

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

tls_support #24

wants to merge 8 commits into from

Conversation

Liron24
Copy link

@Liron24 Liron24 commented Nov 30, 2018

Added tls support
Changed arguments to support it and used NewSecureConnection

@alicebob
Copy link
Owner

alicebob commented Dec 4, 2018

Hi, sorry for the delay.
Thanks for the PR! I have a few questions, but I'll start with the most important one:
would it be possible to keep all the current flags as-is, and have the TLS options be new flags only?
I'm thinking something along these lines:

enableTLS = flag.Bool("tls", false, "enable TLS")
tlsName   = flag.String("tlsName", "myServer", "TLS name")
tlsKey    = flag.String("tlsKey", "./tls.key", "TLS certificate - key")
tlsCert   = flag.String("tlsCert", "./tls.cert", "TLS certificate - cert")

@Liron24
Copy link
Author

Liron24 commented Dec 4, 2018

Hi,
Yes it's definitely possible.
I wanted to seperate the port from the node address since usually secured connection to aerospike is done via port 3010 and not 3000.
I will refactor and update the PR.

@Liron24
Copy link
Author

Liron24 commented Dec 4, 2018

Done.

@alicebob
Copy link
Owner

alicebob commented Dec 4, 2018

Thanks!

  • Please run all code through go fmt
  • The default for all flags should stay no TLS, I think (also the port should stay :3000 by default?)
  • It looks like it always makes a secure connection now (with NewSecureConnection()). Are you sure this still works with the default (non-secure) connection?

@Liron24
Copy link
Author

Liron24 commented Dec 4, 2018

You were correct I was missing an if to choose between secure connection and non secure connection.
Fixed and performed other requests as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants