Skip to content

xds: don't allow multiple instances of a locality in the same priority#20574

Merged
adisuissa merged 2 commits intoenvoyproxy:mainfrom
markdroth:eds_duplicate_localities
Apr 12, 2022
Merged

xds: don't allow multiple instances of a locality in the same priority#20574
adisuissa merged 2 commits intoenvoyproxy:mainfrom
markdroth:eds_duplicate_localities

Conversation

@markdroth
Copy link
Copy Markdown
Contributor

Signed-off-by: Mark D. Roth roth@google.com

Commit Message: xds: don't allow multiple instances of a locality in the same priority
Additional Description: This is a documentation-only change. gRPC is a bit more restrictive here than Envoy is. This changes the documented requirement to match gRPC's restriction but does not change Envoy's implementation, with the goal being to converge on the more restrictive behavior in the long term but not break anything in the short term.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Mark D. Roth <roth@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20574 was opened by markdroth.

see: more, trace.

@alyssawilk
Copy link
Copy Markdown
Contributor

@adisuissa this has been waiting for 9 days. If your concern is CI, please comment as such and tag it as waiting.

@wrowe
Copy link
Copy Markdown
Contributor

wrowe commented Apr 7, 2022

/retest

If this does not resolve, @markdroth please merge main (even an empty commit should trigger a new build which will cause a CI run against the current main.)

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20574 (comment) was created by @wrowe.

see: more, trace.

…ities

Signed-off-by: Mark D. Roth <roth@google.com>
@adisuissa
Copy link
Copy Markdown
Contributor

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20574 (comment) was created by @adisuissa.

see: more, trace.

@adisuissa
Copy link
Copy Markdown
Contributor

@markdroth is there an issue tracking the convergence to the restrictive behavior?

@markdroth
Copy link
Copy Markdown
Contributor Author

No, there isn't, but feel free to create one.

The main thing I'm after here is defining the API semantics such that the same locality does not repeat in the same priority. If Envoy's implementation is a bit more permissive than the API spec, that's not the end of the world. I just don't want new people implementing their xDS servers in a way that doesn't work for gRPC clients.

@adisuissa
Copy link
Copy Markdown
Contributor

No, there isn't, but feel free to create one.

The main thing I'm after here is defining the API semantics such that the same locality does not repeat in the same priority. If Envoy's implementation is a bit more permissive than the API spec, that's not the end of the world. I just don't want new people implementing their xDS servers in a way that doesn't work for gRPC clients.

Opened #20768 to track this.

Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

/lgtm api

Should be merged once CI is green.

@adisuissa adisuissa merged commit 9d2148e into envoyproxy:main Apr 12, 2022
@markdroth markdroth deleted the eds_duplicate_localities branch April 12, 2022 14:11
vehre-x41 pushed a commit to vehre-x41/envoy that referenced this pull request Apr 19, 2022
envoyproxy#20574)

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants