Pass address attributes from balancer to creds handshaker. #3548
Conversation
balancer/balancer.go
Outdated
| @@ -136,6 +138,9 @@ type ClientConn interface { | |||
| // NewSubConn is called by balancer to create a new SubConn. | |||
| // It doesn't block and wait for the connections to be established. | |||
| // Behaviors of the SubConn can be controlled by options. | |||
| // | |||
| // Attributes field in resolver.Address can be used to pass arbitrary data | |||
There was a problem hiding this comment.
I don't believe this needs to be documented specifically. The Attributes should be expected to find their way anywhere the Address is plumbed.
clientconn.go
Outdated
| // Balancer implementations can specify arbitrary data in the Attributes | ||
| // field of resolver.Address, which is shoved into connectCtx that is passed |
There was a problem hiding this comment.
The resolver also produces attributes.
clientconn.go
Outdated
| // to the transport layer. The transport layer passes the same context to | ||
| // the credential handshaker. This makes is possible for address specific | ||
| // arbitrary data from balancers to reach the credential handshaker. | ||
| connectCtx = credentials.WithAddressInfo(connectCtx, credentials.AddressInfo{Attr: addr.Attributes}) |
There was a problem hiding this comment.
I can't decide whether this should happen here or in the transport. We give the transport the address already, so this is somewhat redundant here. Credentials are partially a grpc concept but also are transport dependent. I.e. with custom transports, an in-process implementation won't support transport credentials (it's not a byte transport so doesn't make sense).
There was a problem hiding this comment.
If we have the transport layer do it, then every transport implementation would have to do it, right. And for transports that don't support transportCredentials, this should still work (although it would be wasted effort here). But it should not affect correctness, right. Am I missing something?
There was a problem hiding this comment.
Yeah this is not about "functional correctness", but "design correctness" - making the right component responsible for doing the work.
If the API for ClientHandshake was to pass resolver.Address, which is how we'd design it now if we could, then we wouldn't do this at all. So then the question is, which component "owns" the ClientHandshake API? gRPC or Transport(s)? Another consideration: we already pass some information to the per-RPC credentials via context, and that is done in the transport. So putting this here and that there is a little inconsistent as well.
There was a problem hiding this comment.
Moved to the transport layer. So, there are no diffs now in clientconn.go
credentials/credentials.go
Outdated
| @@ -193,6 +194,31 @@ func RequestInfoFromContext(ctx context.Context) (ri RequestInfo, ok bool) { | |||
| return | |||
| } | |||
|
|
|||
| // AddressInfo contains address related data attached to the context passed to | |||
| // ClientHandshake. This makes it possible for balancer implementations to pass | |||
There was a problem hiding this comment.
As before, "balancer, resolver, grpc, etc". There's a flow of data here that the balancer and handshaker are only parts of.
|
Ping ... |
credentials/credentials.go
Outdated
| // ClientHandshakeInfo in a context. | ||
| type clientHandshakeInfoKey struct{} | ||
|
|
||
| // WithClientHandshakeInfo returns a copy of parent with chi stored as a value. |
There was a problem hiding this comment.
//
// This API is EXPERIMENTAL.
And below please.
Actually, this one is for internal use only, correct? Let's follow the same pattern as RequestInfo if so:
https://github.com/grpc/grpc-go/blob/master/internal/internal.go#L42:1
test/balancer_test.go
Outdated
|
|
||
| const attrBalancerName = "attribute-balancer" | ||
|
|
||
| // attrBalancerBuilder builds a balancer and passes the attribute key and value |
There was a problem hiding this comment.
Do you think you would be able to use the stub balancer instead:
https://github.com/grpc/grpc-go/pull/3431/files#diff-ef2472da4a075e04e3580d038b3e14aa
If so, please use it (and add it to this PR, since the other PR will take another week or so to submit).
There was a problem hiding this comment.
Done.
I had to embed the balancer.Balancer interface though to get it to compile. But I added a TODO there to remove it once the above mentioned PR goes in.
There was a problem hiding this comment.
Cool, I'm glad it worked out, thanks.
easwars
force-pushed
the
creds_in_attributes
branch
from
cfbd49a
to
5b3a0d1
Compare
| } | ||
| return nil | ||
| } | ||
| func (b *bal) ResolverError(e error) { |
There was a problem hiding this comment.
Please add a blank line between these functions, and below.
test/balancer_test.go
Outdated
|
|
||
| const attrBalancerName = "attribute-balancer" | ||
|
|
||
| // attrBalancerBuilder builds a balancer and passes the attribute key and value |
There was a problem hiding this comment.
Cool, I'm glad it worked out, thanks.
easwars
force-pushed
the
creds_in_attributes
branch
from
cf43f93
to
018203d
Compare
#effectively-use-attributes