Implement config reload support for TLS #506
Conversation
Allows reloading TLS config. This includes enabling/disabling TLS, rotating keys/certs, enabling/disabling client verification, etc.
|
I think in general we need to make sure that the file that is pointed to has not changed. I think production may simply replace existing file with updated cert/creds and then want reload to do the right thing. But internally the TLS info we have may look the same. |
|
@derekcollison I don't follow. This already accounts for file changes because when we parse the new config we also construct a new |
server/reload.go
Outdated
| func (t *tlsOption) Apply(server *Server) { | ||
| tlsRequired := t.newValue != nil | ||
| server.info.TLSRequired = tlsRequired | ||
| server.info.TLSVerify = tlsRequired && t.newValue.ClientAuth == tls.RequireAndVerifyClientCert |
There was a problem hiding this comment.
Guard against nil pointers when the reload is disabling TLS (t.newValue == nil). Same as here.
There was a problem hiding this comment.
Not clear (although maybe correct) that an assignment in Go does not always return true.. We could just do an easy "if" statement for the ClientAuth assignment and it would read much clearer.
There was a problem hiding this comment.
We are not returning the value of an assignment. This is purely a boolean expression being assigned to server.info.TLSVerify. Maybe the parens used here make that more clear?
There was a problem hiding this comment.
The comparison makes sense, but having assignment first here is confusing, and I used to be a C guy ;)
|
ok, was not clear from PR or the test that this would do the right thing in that case. |
|
Yeah, I intend to add some more robust testing of config reload soon. |
|
TLS will be a main usage of config reload. I think we should get the test for when the file the config points to changes earlier rather than later. |
|
Will add a test to this PR. |
|
Thanks for change to readability. Add a test and we should be good. |
|
@derekcollison added pretty comprehensive tests around TLS reload. LMK what you think... |
|
Changes Unknown when pulling 4c33177 on tls_reload into ** on master**. |
|
LGTM |
| server.info.TLSVerify = (t.newValue.ClientAuth == tls.RequireAndVerifyClientCert) | ||
| message = "enabled" | ||
| } | ||
| server.generateServerInfoJSON() |
There was a problem hiding this comment.
I was about to ask to surround this with server lock/unlock but I realize that this is done higher in the caller stack. So we are good here.
Allows reloading TLS config. This includes enabling/disabling TLS,
rotating keys/certs, enabling/disabling client verification, etc.
@nats-io/core