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

Revert "xds/googlec2p: use xdstp names for LDS (#6949)" #6964

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented Feb 5, 2024

This reverts commit 3aafa84.

As described in https://github.com/grpc/proposal/blob/master/A47-xds-federation.md#bootstrap-config-changes, client_listener_resource_name_template inside the authority map should default to xdstp://<authority_name>/envoy.config.listener.v3.Listener/%s, so #6949 shouldn't be necessary.

cc @arvindbr8

RELEASE NOTES: none

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Merging #6964 (f802b97) into master (03e76b3) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6964      +/-   ##
==========================================
+ Coverage   83.70%   83.82%   +0.11%     
==========================================
  Files         288      288              
  Lines       30921    30920       -1     
==========================================
+ Hits        25883    25919      +36     
+ Misses       3976     3950      -26     
+ Partials     1062     1051      -11     
Files Coverage Δ
xds/googledirectpath/googlec2p.go 84.52% <100.00%> (-0.19%) ⬇️

... and 23 files with indirect coverage changes

@arvindbr8
Copy link
Member

I'm inclined to leaving it as is. To me it is evident that the resource name template uses xdstp://<authority_name>/envoy.config.listener.v3.Listener/%s only if I remember the gRFC percept or I follow the code to see that there is a default applied.
WDYT?

@apolcyn
Copy link
Contributor Author

apolcyn commented Feb 5, 2024

My concern is that this is inconsistent with other languages.

In the worst case, it could potentially mask other inconsistencies with the other langs.

If nothing else, this would cause some confusion when reading the google-c2p resolver code - since the Go c2p resolver would be generating a unique bootstrap config relative to the other languages.

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

SGTM

@arvindbr8 arvindbr8 added the Type: Internal Cleanup Refactors, etc label Feb 5, 2024
@arvindbr8 arvindbr8 added this to the 1.62 Release milestone Feb 5, 2024
@arvindbr8 arvindbr8 merged commit 05b4a8b into grpc:master Feb 5, 2024
14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants