-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Allow TLS configurations for HTTP and RPC connections to be reloaded … #4025
Conversation
command/agent/agent.go
Outdated
@@ -777,14 +777,34 @@ func (a *Agent) Stats() map[string]map[string]string { | |||
|
|||
// ShouldReload determines if we should reload the configuration and agent | |||
// connections. If the TLS Configuration has not changed, we shouldn't reload. | |||
func (a *Agent) ShouldReload(newConfig *Config) (bool, bool) { | |||
func (a *Agent) ShouldReload(newConfig *Config) (bool, bool, bool) { |
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 we add named returns? Will help readability:
func (a *Agent) ShouldReload(newConfig *Config) (agent, http, rpc bool)
command/agent/agent.go
Outdated
|
||
var tlsInfoChanged bool | ||
if !a.config.TLSConfig.CertificateInfoIsEqual(newConfig.TLSConfig) { | ||
tlsInfoChanged = true |
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.
Would it be easier to return true, true, true
here and then you can remove the tlsInfoChanged
variable
command/agent/agent_test.go
Outdated
|
||
func TestServer_ShouldReload_ReturnTrueForOnlyHTTPChanges(t *testing.T) { | ||
t.Parallel() | ||
assert := assert.New(t) |
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.
use require
nomad/server_test.go
Outdated
assert.True(s1.config.TLSConfig.Equals(newTLSConfig)) | ||
assert.True(s1.config.TLSConfig.CertificateInfoIsEqual(newTLSConfig)) | ||
|
||
codec := rpcClient(t, s1) |
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.
This won't actually use TLS:
Lines 27 to 36 in 40db0af
func rpcClient(t *testing.T, s *Server) rpc.ClientCodec { | |
addr := s.config.RPCAddr | |
conn, err := net.DialTimeout("tcp", addr.String(), time.Second) | |
if err != nil { | |
t.Fatalf("err: %v", err) | |
} | |
// Write the Nomad RPC byte to set the mode | |
conn.Write([]byte{byte(pool.RpcNomad)}) | |
return pool.NewClientCodec(conn) | |
} |
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.
You may want to add a new helper that takes a tls config and then returns a client codec. You would have to write the TLS mode first:
conn.Write([]byte{byte(pool.RpcTLS)})
conn.Write([]byte{byte(pool.RpcNomad)})
You can remove the new code and put a TODO for yourself, and create a follow up PR
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
…separately