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

xdsclient: improve federation watchers test #5696

Merged
merged 7 commits into from
Oct 19, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Oct 6, 2022

Improve federation watchers test by moving to an e2e style test which verifies functionality without relying on any internal state.

This PR is similar to how the LDS watchers test was cleaned up in #5506.

#resource-agnostic-xdsclient-api

RELEASE NOTES: n/a

@easwars easwars requested a review from zasweq October 6, 2022 18:27
@easwars easwars added this to the 1.51 Release milestone Oct 6, 2022
@easwars easwars force-pushed the federation_watchers_test branch from f5cf418 to 2f2baf2 Compare October 12, 2022 03:35
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

Lot of comments on this one.

@zasweq zasweq assigned easwars and unassigned zasweq Oct 14, 2022
@easwars easwars assigned zasweq and unassigned easwars Oct 17, 2022
@easwars easwars force-pushed the federation_watchers_test branch from dda7d57 to 573eda7 Compare October 17, 2022 18:44
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM outside a few more minor comments.


// Create a bootstrap file in a temporary directory.
// Create the bootstrap
Copy link
Contributor

Choose a reason for hiding this comment

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

"the bootstrap"? Are we a western? bootstrap file please. Or just delete this comment "bootstrapContents, err := xdsinternal.BootstrapContents" is self explanatory but I don't feel strongly about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted.

// a single resource with canonicalized context parameters. The test verifies
// that both watchers are notified.
Copy link
Contributor

Choose a reason for hiding this comment

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

So sorry, but what does "canonicalized context parameters mean"? Here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/cncf/xds/blob/main/proposals/TP1-xds-transport-next.md#normalization is the spec. What canonicalized (or normalized) It isn't clear from reading that. But basically what it means for us is that we sort the context parameters. That is the reason why the following two resource names are considered the same:

xdstp:///envoy.config.listener.v3.Listener/xdsclient-test-lds-resource?a=1&b=2
xdstp:///envoy.config.listener.v3.Listener/xdsclient-test-lds-resource?b=2&a=1

Our federation spec does go into more detail here:

New-style resource names used as cache keys must be [normalized]
(https://github.com/cncf/xds/blob/main/proposals/TP1-xds-transport-next.md#normalization). In particular, this 
means sorting the context params in normal lexicographic order.

If a new-style name has duplicate values for the same context param, the behavior is undefined. The easiest thing
is probably to just use the last value seen, since this allows the implementation to just populate the map by iterating
over the query params.

https://github.com/grpc/proposal/blob/master/A47-xds-federation.md

@zasweq zasweq assigned easwars and unassigned zasweq Oct 18, 2022
@zasweq
Copy link
Contributor

zasweq commented Oct 18, 2022

This has a conflict btw.

@easwars easwars force-pushed the federation_watchers_test branch from d9076aa to 836c01e Compare October 18, 2022 19:47
@easwars easwars force-pushed the federation_watchers_test branch from 157cbf6 to 925f214 Compare October 19, 2022 00:00
@easwars easwars merged commit 28fae96 into grpc:master Oct 19, 2022
@easwars easwars deleted the federation_watchers_test branch October 19, 2022 00:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants