Enable TLS client connections to Cassandra #555
Conversation
rbtcollins
requested review from
black-adder,
pavolloffay,
vprithvi and
yurishkuro
as code owners
|
This is one of those things that are difficult to integration-test. We're not testing basic auth either. Could you please add some instructions to the docs section https://github.com/jaegertracing/jaeger/blob/master/docs/deployment.md#cassandra, e.g. links to Cassandra documentation or blogs about how these settings are meant to be used. |
|
@yurishkuro I've put some words in. |
|
@yurishkuro I think the failure is a flaky test? |
| flagSet.String( | ||
| nsConfig.namespace+suffixServerName, | ||
| nsConfig.TLS.ServerName, | ||
| "Override the TLS server name") |
There was a problem hiding this comment.
is this a part of https://en.wikipedia.org/wiki/Server_Name_Indication ?
There was a problem hiding this comment.
No. The main reason is to allow connecting to a host where the server name in the cert, or one of the SANs, differs.
e.g. you have a cert for foo.svc.cluster.local but you're connecting to just 'foo', you could set ServerName=foo.svc.cluster.local.
There is something odd in the dialer setup for jaeger at the moment which is leading to the TLS layer missing the hostname, and ServerName can be used to work around that. I'm going to poke around and see if I can make it more JustWorks in future - but there are still cases where ServerName is needed.
cmd/flags/cassandra/options.go
Outdated
| @@ -34,6 +34,12 @@ const ( | |||
| suffixSocketKeepAlive = ".socket-keep-alive" | |||
| suffixUsername = ".username" | |||
| suffixPassword = ".password" | |||
| suffixTLS = ".tls" | |||
| suffixCert = ".cert" | |||
There was a problem hiding this comment.
I think it would better to prefix all params with tls-, e.g. .tls-cert, .tls-key, etc.
cmd/flags/cassandra/options.go
Outdated
| suffixCert = ".cert" | ||
| suffixKey = ".key" | ||
| suffixCA = ".ca" | ||
| suffixServerName = ".servername" |
docs/deployment.md
Outdated
| configure the collector and query like so: | ||
|
|
||
| ``` | ||
| collector-linux --cassandra.servers=cassandra --cassandra.tls \ |
There was a problem hiding this comment.
I would prefer not to direct people to command line arguments, but instead use env variables. So this could be the standard Docker command for the collector with env vars defined. Also, one param per line to make it more readable.
docker run \
-e SPAN_STORAGE_TYPE=cassandra \
-e CASSANDRA_SERVERS=<...> \
-e CASSANDRA_TLS=true \
. . . etc . . .
jaegertracing/jaeger-collector
There was a problem hiding this comment.
ok, theres some automatic conversion to env variables from the command line args right?
There was a problem hiding this comment.
yes, s/[.-]/_/g + toUpperCase
Signed-off-by: Robert Collins <robertc@vmware.com>
|
Thanks! |
I'm not entirely sure how to test this in the context of Jaeger's test suite; it does work for me...