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: Add new fields to resolver.BuildOption struct to support dialing a remote name resolver #3098

Merged
merged 3 commits into from
Nov 4, 2019

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Oct 14, 2019

These fields will be used by resolver implementations which need to talk
to a remote name resolver.

@easwars easwars added the Type: Feature New features or improvements in behavior label Oct 14, 2019
@easwars easwars added this to the 1.25 Release milestone Oct 14, 2019
@easwars easwars requested a review from dfawley October 14, 2019 23:26
@easwars easwars changed the title Add a couple of fields to resolver.BuildOption struct. Add new fields to resolver.BuildOption struct. Oct 15, 2019
@easwars
Copy link
Contributor Author

easwars commented Oct 15, 2019

@dfawley
Based on our offline discussion, I also added the CredsBundle field to the BuildOption struct.

@dfawley
Copy link
Member

dfawley commented Oct 15, 2019

Can you update this PR so that it also sets all the fields? Or was this PR intended to be a proposal / strawman for discussion purposes?

@easwars
Copy link
Contributor Author

easwars commented Oct 15, 2019

Can you update this PR so that it also sets all the fields? Or was this PR intended to be a proposal / strawman for discussion purposes?

I was initially being lazy and thought I'd do that with the xdsResolver changes. But makes sense to do it here. Done.

These fields will be used by resolver implementations which need to talk
to a remote name resolver.
@easwars easwars changed the title Add new fields to resolver.BuildOption struct. resolver: Add new fields to resolver.BuildOption struct to support dialing a remote name resolver Oct 17, 2019
@dfawley dfawley self-assigned this Oct 24, 2019
@easwars
Copy link
Contributor Author

easwars commented Oct 28, 2019

Ping ...

@easwars easwars mentioned this pull request Oct 29, 2019
DisableServiceConfig bool
// DialCreds is the transport credentials that a resolver implementation
// can use to dial a remote name resolution server. Resolver
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should say simply that it's the DialCreds for the ClientConn? It may not be appropriate to use these credentials for your name resolver in many (practically all?) cases. Same for the other two fields, except that a Bundle is a little more likely to be OK to use. Possibly include a warning about using 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.

Does this sound OK? If so, I can make similar modifications to the other two fields as well.

DialCreds is the transport credentials used by the ClientConn (set as a DialOption
using a call to WithTransportCredentials) while communicating with the gRPC
server. In some cases, it may be appropriate for a resolver implementation to use 
these credentials to talk to a remote name resolution server. In most cases though, 
it might not be appropriate and should be used with caution.

@dfawley dfawley assigned easwars and unassigned dfawley Oct 29, 2019
@easwars easwars assigned dfawley and unassigned easwars Oct 29, 2019
@dfawley
Copy link
Member

dfawley commented Oct 30, 2019

How about:

DialCreds is the transport credentials used by the ClientConn for communicating with the target gRPC service (set via WithTransportCredentials). In cases where a name resolution service requires the same credentials, the resolver may use this field. In most cases though, it is not appropriate, and this field may be ignored.

@dfawley dfawley assigned easwars and unassigned dfawley Oct 31, 2019
@easwars
Copy link
Contributor Author

easwars commented Nov 1, 2019

How about:

DialCreds is the transport credentials used by the ClientConn for communicating with the target gRPC service (set via WithTransportCredentials). In cases where a name resolution service requires the same credentials, the resolver may use this field. In most cases though, it is not appropriate, and this field may be ignored.

Done. Thanks.

@easwars easwars assigned dfawley and unassigned easwars Nov 1, 2019
@easwars easwars merged commit 88bf070 into grpc:master Nov 4, 2019
@easwars easwars deleted the xds_resolver branch December 18, 2019 17:02
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants