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

connect: bootstrap envoy using -proxy-id #12011

Merged
merged 1 commit into from
Feb 18, 2022
Merged

connect: bootstrap envoy using -proxy-id #12011

merged 1 commit into from
Feb 18, 2022

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Feb 4, 2022

This PR modifies the Consul CLI arguments used to bootstrap envoy for
Connect sidecars to make use of '-proxy-id' instead of '-sidecar-for'.

Nomad registers the sidecar service, so we know what ID it has. The
'-sidecar-for' was intended for use when you only know the name of the
service for which the sidecar is being created.

The improvement here is that using '-proxy-id' does not require an underlying
request for listing Consul services. This will make make the interaction
between Nomad and Consul more efficient.

Closes #10452

Copy link
Contributor

@jazzyfresh jazzyfresh left a comment

Choose a reason for hiding this comment

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

Looks good. I'm still figuring out how to enable acls in the nightly tho

@shoenig
Copy link
Member Author

shoenig commented Feb 7, 2022

@jazzyfresh like the comment suggests, the NOMAD_TEST_CONSUL_ACLS flag exists and defaults off because it's just too flaky to be part of the regular e2e. It also adds like half an hour to the runtime.

  // Connect tests with Consul ACLs enabled. These are now gated behind the
  // NOMAD_TEST_CONSUL_ACLS environment variable, because they cause lots of
  // problems for e2e test flakiness (due to restarting consul, nomad, etc.).
  //
  // Run these tests locally when working on Connect.

@jazzyfresh
Copy link
Contributor

Right. I thought that's what you were asking for, and we were going to eat the cost. Did you just mean to just run the e2e manually then? Or create a custom test with ACLs not re-enable it for the whole suite?

This PR modifies the Consul CLI arguments used to bootstrap envoy for
Connect sidecars to make use of '-proxy-id' instead of '-sidecar-for'.

Nomad registers the sidecar service, so we know what ID it has. The
'-sidecar-for' was intended for use when you only know the name of the
service for which the sidecar is being created.

The improvement here is that using '-proxy-id' does not require an underlying
request for listing Consul services. This will make make the interaction
between Nomad and Consul more efficient.

Closes #10452
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connect: set proxy-id for sidecar proxies
2 participants