Skip to content
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

Use proxy address for default check #14433

Merged
merged 1 commit into from Sep 1, 2022
Merged

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Aug 31, 2022

When a sidecar proxy is registered, a check is automatically added.
Previously, the address this check used was the underlying service's
address instead of the proxy's address, even though the check is testing
if the proxy is up.

This worked in most cases because the proxy ran on the same IP as the
underlying service but it's not guaranteed and so the proper default
address should be the proxy's address.

image

The docs say:

By default we add a TCP check on the local address and port for the proxy, and a service alias check for the parent service. If either check or checks fields are set, only the provided checks are registered.

Which is now correct but previously was incorrect.

Testing & Reproduction steps

  • Tested via unit tests
services {
  name = "s1"
  port = 8181
  connect {
    sidecar_service {
      address = "255.255.255.255"
      proxy {
      }
    }
  }
}
consul agent -dev -config-file service.hcl
curl localhost:8500/v1/agent/checks                                                                                                           ⎈ kind-kind
{
    "service:s1-sidecar-proxy:1": {
        "Node": "Lukes-MacBook-Pro.local",
        "CheckID": "service:s1-sidecar-proxy:1",
        "Name": "Connect Sidecar Listening",
        "Status": "critical",
        "Notes": "",
        "Output": "dial tcp 255.255.255.255:21000: connect: address family not supported by protocol family",
...

This proves it's using the correct address. Previously it would use 127.0.0.1, the address of the service instance.

PR Checklist

  • updated test coverage
  • not a security concern

When a sidecar proxy is registered, a check is automatically added.
Previously, the address this check used was the underlying service's
address instead of the proxy's address, even though the check is testing
if the proxy is up.

This worked in most cases because the proxy ran on the same IP as the
underlying service but it's not guaranteed and so the proper default
address should be the proxy's address.
@@ -202,14 +213,11 @@ func (a *Agent) sidecarPortFromServiceID(sidecarCompoundServiceID structs.Servic
return sidecarPort, nil
}

func sidecarDefaultChecks(serviceID string, localServiceAddress string, port int) []*structs.CheckType {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the comments here because the first one:

// Setup default check if none given

Is already commented where this function is called and the second one is now commented where we pass in the address.

@lkysow lkysow requested a review from a team September 1, 2022 18:41
@lkysow lkysow added type/bug Feature does not function as expected theme/health-checks Health Check functionality theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies labels Sep 1, 2022
@lkysow lkysow merged commit 81d7cc4 into main Sep 1, 2022
@lkysow lkysow deleted the lkysow/sidecar-proxy-check branch September 1, 2022 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/health-checks Health Check functionality type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants