-
Notifications
You must be signed in to change notification settings - Fork 17
initial scaffolding for consul-server-connection-manager integration #407
Conversation
Tests seem to mostly be hanging currently, with errors like below when cancelling:
If the actual integration is halfway-correct, I'm guessing some mocks might need to be updated. |
consulClient, err := api.NewClient(cfg) | ||
if err != nil { | ||
logger.Error("error creating consul client", "error", err) | ||
return 1 | ||
} | ||
|
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.
There didn't seem to be any reason to initialize an API client here when it always gets re-initialized in RunExec
anyway.
return api.NewClient(&config.ConsulConfig) | ||
} | ||
|
||
func login(config ExecConfig, s discovery.State) (*api.Client, string, error) { |
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.
TODO: login/logout functionality appears to be supported directly within the consul-server-connection-manager library, refs hashicorp/consul-server-connection-manager#9
move logout to ServiceRegistry, which will maintain an updated Consul API client
…-server-connection-manager library
This reverts commit e8115f9.
c8f8bf0
to
6243108
Compare
@@ -236,34 +291,16 @@ func (c *CertManager) Manage(ctx context.Context) error { | |||
c.leafWatch.HybridHandler = c.handleLeafWatch |
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.
To reimplement this without relying on the Consul agent, we'd basically need to rewrite all of the logic from https://github.com/hashicorp/consul/blob/main/agent/cache-types/connect_ca_leaf.go#L514-L685 to generate a new leaf certificate (which currently depends on internal methods of Consul's connect
package) and as well as tie it into the CA root watch handler to regenerate certificates on rotation, as the gRPC API only exposes the lower-level Sign
method.
Changes proposed in this PR:
Integrate the consul-server-connnection-manager library to replace the reliance on a Consul client when interacting with the Consul API.
Removes a workaround introduced in #73 for a bug in Consul 1.11 due to a dependency on the Consul agent for the
ConnectCALeaf
API call - this appears to have been fixed in hashicorp/consul#12820 and was included in the Consul v1.11.6 patch and newer releases.How I've tested this PR:
TODO
How I expect reviewers to test this PR:
TBD
Checklist: