Add fallback to no gRPC #454
Conversation
andrewstucki
added
the
pr/no-changelog
|
adding the conformance pr label just to see if it will pass, but we can remove if its blocking |
sarahalsmiller
added
the
pr/conformance
| @@ -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.
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.
Not sure I understand this comment?
There was a problem hiding this comment.
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.
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.
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_DISCOVERYis 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.