-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add consul version compatibility check on startup #62
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.
looks good to me! very readable and lots of helpful comments. only a suggestion for a tiny typo
// to retry the request. | ||
return fmt.Errorf("unable to check version compatibility with Consul agent: %s", err) | ||
} | ||
agentVersionRaw, ok := agentInfo["Config"]["Version"] |
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.
cool - didn't know you could check a map within a map in one call!
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.
I honestly always have to sanity check this in https://play.golang.com/ 😂
if err != nil { | ||
if strings.Contains(err.Error(), "429") { | ||
// 429 is a warning that something is unhealthy. This may occur when ESM | ||
// races with Consul servers first starting up, so this is safe to retry. |
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.
helpful comment, thanks!
|
||
func TestAgent_VerifyConsulCompatibility(t *testing.T) { | ||
// Smoke test to test the compatibility with the current Consul version | ||
// pinned in go dependency. |
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.
🎉
The ESM Consul client uses endpoints that are not supported in Consul prior to 1.4.1. This checks for compatibility with all running Consul servers at startup and exits with an informative error rather than silently failing on updating health checks for external services.
Compatibility with agent is necessary because the ESM client makes health check update using txn request to the local agent first, which then is forwarded to the servers.
ACL for operator:read is necessary for Consul-ESM v0.4.0 due to a new feature that check’s ESM and Consul’s version compatibility on start up: #62. This feature makes a request to /operator/autopilot, which needs to be added to the list of necessary ACLs.
Recent versions of the ESM Consul client uses endpoints that are not supported in Consul
prior to 1.4.1. This checks for compatibility with all running Consul servers at startup and exits
with an informative error rather than silently failing on updating health checks for external
services.
This a follow-up PR to enforce the version constraint in the documentation
https://github.com/hashicorp/consul-esm#prerequisites
Example error message
New CLI version output
Resolves #33