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

Provide a way for NameResolvers to dynamically provide authority #4469

Closed
ejona86 opened this issue May 16, 2018 · 7 comments · Fixed by #6126
Closed

Provide a way for NameResolvers to dynamically provide authority #4469

ejona86 opened this issue May 16, 2018 · 7 comments · Fixed by #6126
Milestone

Comments

@ejona86
Copy link
Member

ejona86 commented May 16, 2018

Today, getServiceAuthority() must not do any I/O to determine the authority. That is, it must be computable from the target string.

It's come up before to allow the NameResolver to retrieve the authority from the name resolution system. However, in the past those systems couldn't securely retrieve the authority so it would allow MITM attacks. We've now come across a resolution system that can provide the necessary security guarantee, as it uses TLS to communicate with the resolution system.

Since this is a niche use-case, I think it is fair to leave getServiceAuthority() in place. Also, Channel.authority() is implemented via getServiceAuthority(), so the current restrictions are still probably appropriate. We'll just need to add documentation to Channel.authority() that it may be partially wrong (since each backend may have a different authority). CallCredentials shouldn't have an issue with dynamic authority, although that should be verified.

I think we can use an authority Attribute on the EquivalentAddressGroup or PairSocketAddress. This does require us to make a decision for #4302 since it's basically the same problem. The design decision has been made; we can safely use EAG here.

CC @zhangkun83

@enguerrand
Copy link
Contributor

Could you please advise what the current state of this is? Currently, when I try to provide an authority through EAG I am running into this policy exception. Do I need to provide additional Attributes with LBConfigs or is this use case simply not yet supported?

@ejona86
Copy link
Member Author

ejona86 commented Sep 3, 2019

@enguerrand, this is not implemented. There is no API to instruct gRPC what authority to use. It seems like you may be specifying the grpclb authority, which is something unrelated (it is the grpclb server to contact).

There should be few users of this feature, as it requires your name resolver to be fully authenticated (like over HTTPS) and makes your name resolver the equivalent of root.

@enguerrand
Copy link
Contributor

@ejona86: Thanks a lot for the quick feedback.

as it requires your name resolver to be fully authenticated (like over HTTPS)

In my case the name resolver is trusted because it is an implementation that works locally:
I'd like to do load balancing on a static list of ip addresses that are supplied to my application through configuration.

The servers have ssl certificates with their respective ip address in the subject alternative names.

Do I get it right that my only options are

  1. unified subject alternative names
  2. manual load balancing outside of grpc

@ejona86
Copy link
Member Author

ejona86 commented Sep 4, 2019

@enguerrand, your use-case would be solved by this issue. Although you can probably understand how rare cases that need this are; having a fixed list of IPs for the client isn't very common and doesn't scale. Yes, (1) and (2) are your primary options, although fixing this issue would be another option.

The core of the change needed here is to change the authority passed here:

new ClientTransportFactory.ClientTransportOptions()
.setAuthority(authority)
.setEagAttributes(addressIndex.getCurrentEagAttributes())

The design calls for creating a new Attribute.Key that would be set in the EquivalentAddressGroup. So addressIndex.getCurrentEagAttributes() would be checked whether it contains that authority key. If the key is not present then it would continue using the current authority.

@enguerrand
Copy link
Contributor

@ejona86, Thanks again for those helpful pointers. I tried to implement this as per your recommendation and opened PR 6126

Note that I found that the Attributes did not propagate all the way through to the InternalSubchannel. I found they were intentionally stripped in the RoundRobinLoadBalancer

I modified this to keep the newly added authority attribute to illustrate the issue. I am sure this change is conflicting with other design requirements. (It also leads to failing tests.)

I am happy to revise this further if you could provide some background info on why the stripping is needed. Thanks again!

@ejona86
Copy link
Member Author

ejona86 commented Sep 5, 2019

Hmm... RR is an interesting case, since it is stripping the attributes just for the diff comparison. I'll need to talk it over with @zhangkun83

We do want EAG Attributes to propagate through LBs, so we'll most likely want to change RR to avoid stripping when creating the subchannel.

@ejona86
Copy link
Member Author

ejona86 commented Sep 5, 2019

Talked offline with @zhangkun83. We think RR should propagate the attributes. But we purposefully don't use the Attributes when diffing because we want to avoid equals() on Attributes, as it is slow and probably not worth a reconnect when it changes.

So this means two things for round robin: 1) we need to preserve the attributes when doing the diffing (or restore after the diffing), and 2) when we choose to reuse a subchannel we need to call subchannel.updateAddresses() to provide the possibly-updated EAG. (2) means that the diff will result in added, removed, and (now) kept/maintained sets.

For the diffing it would seem easiest to wrap the EquivalentAddressGroups (EAGs) to override equals()/hashCode(). Alternatively, we could keep a Map<stripped EAG, original EAG>.

This approach will mean two things: 1) RR will only create one subchannel when the attributes are different and 2) if the authority changes the subchannel will not immediately apply the authority (it will be applied next natural reconnect). That seems okay for now, and seems it could be changed in the future if they became a problem.

ejona86 pushed a commit that referenced this issue Sep 12, 2019
This enables NameResolvers to dynamically provide authority for each
Subchannel

Fixes #4469
@ejona86 ejona86 modified the milestones: Next, 1.25 Sep 12, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 11, 2019
fixmebot bot referenced this issue in aomsw13/develop_test Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants