Reload TLS certificates on change #2389
Conversation
pavolloffay
force-pushed
the
reload-certs
branch
from
1adb1e6
to
cbd3917
Compare
| Config: &tls.Config{ | ||
| ServerName: c.TLS.ServerName, | ||
| }, | ||
| CertPath: c.TLS.CertPath, |
There was a problem hiding this comment.
@yurishkuro instead of relying on the driver to load the certs I have changed it to use our TLS loading.
pkg/config/tlscfg/reload.go
Outdated
| var err error | ||
| switch event.Name { | ||
| case w.opts.CAPath: | ||
| err = addCertToPool(w.opts.CAPath, rootCAs) |
There was a problem hiding this comment.
Does this mean that if the contents of a CAPath has changed, the new CA is added to the pool, but the old isn't removed?
There was a problem hiding this comment.
Correct, the pool does not expose API to remove old certificates
pkg/config/tlscfg/reload.go
Outdated
| if !ok { | ||
| return | ||
| } | ||
| w.logger.Error("Watcher got error", zap.Error(err)) |
There was a problem hiding this comment.
Also not sure what kind of errors it would get. If those are high-frequency as well, it would probably be better to have them at debug level.
There was a problem hiding this comment.
I would keep this at error level for now until we find out that these are high-freq (I haven't seen this being logged yet).
Codecov Report
@@ Coverage Diff @@
## master #2389 +/- ##
==========================================
- Coverage 95.60% 95.58% -0.02%
==========================================
Files 206 208 +2
Lines 10549 10676 +127
==========================================
+ Hits 10085 10205 +120
- Misses 396 398 +2
- Partials 68 73 +5
Continue to review full report at Codecov.
|
pkg/config/tlscfg/options.go
Outdated
| ServerName string `mapstructure:"server_name"` // only for client-side TLS config | ||
| ClientCAPath string `mapstructure:"client_ca"` // only for server-side TLS config for client auth | ||
| SkipHostVerify bool `mapstructure:"skip_host_verify"` | ||
| watcher *certWatcher `mapstructure:"-"` |
| @@ -76,6 +76,9 @@ func main() { | |||
| consumer.Start() | |||
|
|
|||
| svc.RunAndThen(func() { | |||
| if err := options.TLS.Close(); err != nil { | |||
There was a problem hiding this comment.
will this not be already included in storageFactory.Close()?
There was a problem hiding this comment.
The storage factory closes the TLS config for the producer. This closes the consumer.
| @@ -70,5 +74,6 @@ func (b ProxyBuilder) GetManager() configmanager.ClientConfigManager { | |||
| // Close closes connections used by proxy. | |||
| func (b ProxyBuilder) Close() error { | |||
| b.reporter.Close() | |||
| b.tlsCloser.Close() | |||
| return b.conn.Close() | |||
There was a problem hiding this comment.
could use multiclose.Wrap(a, b, c).Close()
There was a problem hiding this comment.
done, the reporter didn't implement io.Closer I have added return error to it.
pavolloffay
force-pushed
the
reload-certs
branch
from
845aab6
to
5b95e48
Compare
| if err := c.tlsCloser.Close(); err != nil { | ||
| c.logger.Error("failed to close TLS certificate watcher", zap.Error(err)) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
not sure why this function keeps logging, would be cleaner to return them via multierr.Wrap, but doesn't need to be in this PR.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
force-pushed
the
reload-certs
branch
from
4dff444
to
88e5c65
Compare
Resolves jaegertracing/jaeger-operator#1099
This PR enables reloading of TLS certs in Elasticsearch client. The same approach can be used for other clients using our
tlscfgor we can enable it by default.I have tested this on OCP 4.4 and Jaeger Operator with self-provisioned ES. To trigger the cert change I have removed master certs and wiped out the tmp dir in the operator pod.