-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Proxy check default #4381
Proxy check default #4381
Conversation
… 0.0.0.0. This also provides a mechanism to configure custom address or disable the check entirely from managed proxy config.
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.
Just the one question
} | ||
|
||
// If we have a bind address and its diallable, use that | ||
if bindAddr, ok := proxyCfg["bind_address"].(string); ok && |
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.
Should we also make sure bindAddr is not a unix socket? Seems like it but I am not sure if we allow binding a proxy to a unix socket.
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 don't think it ever makes sense for the public listener on the proxy to be a unix socket. We may well add support for the private app => proxy listener being unix but this health check is always against the public socket so I think we are safe.
The TCP check is for the public port - i.e. is the service available to other instances in the cluster.
run a [TCP health check](/docs/agent/checks.html) against. By default this is | ||
the same as the proxy's [bind address](#bind_address) except if the | ||
bind_address is `0.0.0.0` or `[::]` in which case this defaults to `127.0.0.1` | ||
and assumes agent can dial proxy over loopback. For more complex |
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 agent"
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 great.
I'm fine merging as-is, and this is probably fine, but we may want to consider using go-sockaddr
in the future to check for the 0.0.0.0 and IPv6 equivalent rather than a string comparison. If we find edge cases or something.
Fixes #4301
If the managed proxy is bound to an unspecified address (0.0.0.0/[::]), default to 127.0.0.1.
Also adds a config mechanism to override that for more complex setups where loopback isn't the right option, and a way to disable the TCP check entirely should there be some reason it doesn't work for your topology.
Docs updated too (please proof read!).