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

resolver: State: add Endpoints and deprecate Addresses #6471

Merged
merged 2 commits into from Jul 31, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Jul 24, 2023

Also:

  • Removed resolver.UnregisterForTesting, which we didn't need.
  • Created internal.GRPCResolverSchemeExtraMetadata to control the behavior of setting additional metadata in the context for the user agent string, which is needed by xDS routing code.

Even though this is backward compatible and removes nothing, we should probably wait until several LB policies have been converted over to read Endpoints instead of Addresses before merging this PR. I'll be working on that in the meantime.

RELEASE NOTES:

  • resolver: State: add Endpoints and deprecate Addresses

@dfawley dfawley added the Type: API Change Breaking API changes (experimental APIs only!) label Jul 24, 2023
@dfawley dfawley added this to the 1.58 Release milestone Jul 24, 2023
@dfawley dfawley requested a review from easwars July 24, 2023 18:22
@easwars
Copy link
Contributor

easwars commented Jul 24, 2023

Removed resolver.UnregisterForTesting, which we didn't need

I see that this is being used in google3.

@dfawley
Copy link
Member Author

dfawley commented Jul 24, 2023

I see that this is being used in google3.

Argh, thanks for checking. I think we can just remove this usage as well. There doesn't seem to be any harm to leaving a test resolver registered in the case there was no other resolver with that name registered before. Also that test imports "xds" so that resolver is guaranteed to be registered already, and we will always replace it with resolver.Register instead.

balancer/balancer.go Show resolved Hide resolved
Comment on lines +168 to +170
// GRPCResolverSchemeExtraMetadata determines when gRPC will add extra
// metadata to RPCs.
GRPCResolverSchemeExtraMetadata string = "xds"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be only for the xds scheme or do we plan to add this for the xdstp scheme as well?

Also, could you help me figure out how this is used. The code in stream.go adds the metadata, but I don't see how or where this is used. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we supporting xdstp for the target string? If so, this needs to be extended.

This is read here when doing metadata matching for routes:

if extraMD, ok := grpcutil.ExtraMetadata(info.Context); ok {

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we supporting xdstp for the target string?

Hmm ... A47 section on target-uri syntax says we will continue to support the xds scheme for convenience. It does not say that we will start supporting the xdstp scheme. Nor does it say we will not support the xdstp scheme and instead the translations will be handled by the bootstrap configuration.

Do you think we should improve the language in this gRFC section or is it clear to you?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not super air-tight, but by not saying "we're supporting xdstp as a gRPC name resolver scheme and here's how", then I wouldn't assume we are adding it. We'd need a spec for how to handle it as a channel-level target. The text that follows does describe how we'll support this new stuff in the existing xds resolver instead of adding a top-level resolver scheme for xdstp:

we want the deployment to be able to decide to use new-style resource names without requiring users to explicitly specify them. To that end, we will add support for an optional authority in the xds URI. The procedure for handling an xds URI is as follows:

resolver/resolver.go Show resolved Hide resolved
resolver_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned dfawley and unassigned easwars Jul 24, 2023
Comment on lines +168 to +170
// GRPCResolverSchemeExtraMetadata determines when gRPC will add extra
// metadata to RPCs.
GRPCResolverSchemeExtraMetadata string = "xds"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we supporting xdstp for the target string?

Hmm ... A47 section on target-uri syntax says we will continue to support the xds scheme for convenience. It does not say that we will start supporting the xdstp scheme. Nor does it say we will not support the xdstp scheme and instead the translations will be handled by the bootstrap configuration.

Do you think we should improve the language in this gRFC section or is it clear to you?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: API Change Breaking API changes (experimental APIs only!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants