-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
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.
thanks for hopping on his andrew. Some stuff we can probably clean up later, but when the tests pass I'm good to merge this.
adding the conformance pr label just to see if it will pass, but we can remove if its blocking |
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.
Minor notes, and could use a changelog entry.
@@ -468,6 +468,7 @@ func runMockConsulServer(t *testing.T, opts mockConsulOptions) *mockConsulServer | |||
}) | |||
|
|||
server.clientConfig = consul.ClientConfig{ | |||
UseDynamic: 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.
Do we want to add another test where this is false
?
if consulCfg.Scheme != "https" { | ||
// override only if it needs to be explicitly marked | ||
consulCfg.Scheme = consulScheme | ||
} |
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.
Not sure I understand this comment?
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.
sorry, this is to say, if we already have the scheme set to https
(via things like ENV variables, which DefaultConfig
will source), then don't override it -- otherwise set it based off of what we got from parsing it out of the CONSUL_ADDR
(in 1.0.0 we don't set the scheme and use CONSUL_USE_SSL
, in <= 0.49 we use the scheme as part of the address, this handles both cases)
@@ -81,6 +83,46 @@ func (c *client) Wait(until time.Duration) error { | |||
} | |||
|
|||
func (c *client) WatchServers(ctx context.Context) error { | |||
if !c.config.UseDynamic { | |||
cfg := c.config.ApiClientConfig | |||
cfg.Address = fmt.Sprintf("%s:%d", c.config.Addresses, c.config.HTTPPort) |
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.
Not entirely specific to this line, but do we have unit test coverage for checking that both http://
prefixed and plain IPs are handled for agentful and agentless paths?
|
||
c.mutex.Lock() | ||
c.client = client | ||
c.token = cfg.Token |
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.
token
(and set token = c.config.Credentials.Static.Token; cfg.Token = token
in the non-login path) unless login
is mutating cfg.Token
?
Edit: reading the login
impl it looks like it does mutate cfg.Token
so this should be okay, but it is a bit unintuitive.
Merging, will take a pass at cleaning some of this up in a few days once we decide how long we need to support this auth flow code. |
Changes proposed in this PR:
Add an opt-in value for trying to use the new dynamic server discovery components. This is needed because agent (non-server) nodes don't have the requisite gRPC endpoints to actually use the connection manager library. When the
CONSUL_DYNAMIC_SERVER_DISCOVERY
is not set, then we do things in the "old way" without gRPC, otherwise if it's set we know we're talking to a server and can use gRPC.How I've tested this PR:
Manually w/ Consul 1.12 + 1.13 and consul-k8s 0.49.1 and consul-k8s main branch in both agent and agentless mode pending my PR that I'll link here.