-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose option to enable TLS when sniffing an Elasticsearch Cluster #2263
Conversation
Jaeger uses the default scheme set by the olivere client (which is http) when sniffing an Elasticsearch cluster without the option to change it. This makes it impossible to use sniffing with a TLS Elasticsearch cluster. The scheme can be set using SetScheme client option https://pkg.go.dev/github.com/olivere/elatic/v7\?tab\=doc\#SetScheme This change exposes that client option as a boolean command line option: --es.sniffer-tls-enabled Signed-off-by: nilsenj <jennynilsen@rentalcars.com>
What |
The operator does not use sniffing. It connects using the service address which only works from within the cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please run make lint
make go-lint
make[1]: Entering directory `/home/travis/gopath/src/github.com/jaegertracing/jaeger'
Running go lint...
Lint Failures
pkg/es/config/config.go:49:2: struct field SnifferTlsEnabled should be SnifferTLSEnabled
plugin/storage/es/options.go:34:2: const suffixSnifferTlsEnabled should be suffixSnifferTLSEnabled
make[1]: *** [go-lint] Error 1
@@ -288,6 +292,11 @@ func (c *Configuration) getConfigOptions(logger *zap.Logger) ([]elastic.ClientOp | |||
// we don' have a valid token to do the check ad if we don't disable the check the service that | |||
// uses this won't start. | |||
elastic.SetHealthcheck(!c.AllowTokenFromContext)} | |||
if c.SnifferTlsEnabled { | |||
options = append(options, elastic.SetScheme("https")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we derive the scheme from es.server-urls
? That way we don't have to add new flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the scheme from es-server-urls would work for us because both ingress and the elasticsearch nodes are using https. So, I'm happy to make this change. However, strictly speaking the two things are independent of each other. The point of using sniffing is that we can use ingress/loadbalancer for the initial node discovery then connect to the actual elasticsearch nodes for sending traffic. Are you concerned about somebody that has an https enabled ingress/loadbalancer but uses http for the elasticsearch nodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that people using TLS would normally protect all endpoints. If that is not the case we can merge your patch that adds an additional flag to control TLS just for sniffing.
The PR looks good I have suggested two edits. Once that is done we can merge it.
plugin/storage/es/options.go
Outdated
@@ -95,6 +96,7 @@ func NewOptions(primaryNamespace string, otherNamespaces ...string) *Options { | |||
CreateIndexTemplates: true, | |||
Version: 0, | |||
Servers: []string{defaultServerURL}, | |||
SnifferTlsEnabled: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed false
is default
pkg/es/config/config.go
Outdated
@@ -288,6 +292,11 @@ func (c *Configuration) getConfigOptions(logger *zap.Logger) ([]elastic.ClientOp | |||
// we don' have a valid token to do the check ad if we don't disable the check the service that | |||
// uses this won't start. | |||
elastic.SetHealthcheck(!c.AllowTokenFromContext)} | |||
if c.SnifferTlsEnabled { | |||
options = append(options, elastic.SetScheme("https")) | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http
is the default the else
branch can be removed.
@@ -288,6 +292,11 @@ func (c *Configuration) getConfigOptions(logger *zap.Logger) ([]elastic.ClientOp | |||
// we don' have a valid token to do the check ad if we don't disable the check the service that | |||
// uses this won't start. | |||
elastic.SetHealthcheck(!c.AllowTokenFromContext)} | |||
if c.SnifferTlsEnabled { | |||
options = append(options, elastic.SetScheme("https")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that people using TLS would normally protect all endpoints. If that is not the case we can merge your patch that adds an additional flag to control TLS just for sniffing.
The PR looks good I have suggested two edits. Once that is done we can merge it.
Signed-off-by: nilsenj <jennynilsen@rentalcars.com>
Codecov Report
@@ Coverage Diff @@
## master #2263 +/- ##
==========================================
+ Coverage 96.15% 96.19% +0.04%
==========================================
Files 219 218 -1
Lines 10652 10692 +40
==========================================
+ Hits 10242 10285 +43
+ Misses 354 351 -3
Partials 56 56
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@jennynilsen would you like to add your company to https://github.com/jaegertracing/jaeger/blob/master/ADOPTERS.md or comment on #207? |
Jaeger uses the default scheme set by the olivere client (which is http) when sniffing an Elasticsearch cluster without the option to change it. This makes it impossible to use sniffing with a TLS Elasticsearch cluster.
The scheme can be set using the SetScheme client option
https://pkg.go.dev/github.com/olivere/elatic/v7\?tab\=doc\#SetScheme
This change exposes that client option as a boolean command line option: --es.sniffer-tls-enabled
Signed-off-by: nilsenj jennynilsen@rentalcars.com
Which problem is this PR solving?
In our use case, jaeger-collector needs to connect to Elasticsearch running in Kubernetes from outside the cluster. By using sniffing, we can use the ingress for the initial node discovery and then send traffic directly to the elasticsearch nodes without it needing to go through the ingress.
In our case, when using ingress only (without sniffing) jaeger represents a substantial proportion of the ingress traffic. When we switched to using sniffing we were able to reduce the load on our ingress 10x.
If Elasticsearch is running with TLS enabled, then we need a way to tell jaeger-collector to use https when connecting to the discovered nodes.
Short description of the changes