xdsclient: use dataplane authority for virtualhost lookup #6997
Conversation
|
@ejona86 -- please take a pass at the behavior change. I will get someone from the team to also take a look. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6997 +/- ##
==========================================
+ Coverage 82.46% 82.48% +0.02%
==========================================
Files 296 296
Lines 31465 31461 -4
==========================================
+ Hits 25947 25952 +5
+ Misses 4463 4452 -11
- Partials 1055 1057 +2
|
| @@ -119,6 +119,7 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon | |||
| endpoint = target.URL.Opaque | |||
| } | |||
| endpoint = strings.TrimPrefix(endpoint, "/") | |||
| r.dataplaneAuthority = endpoint | |||
There was a problem hiding this comment.
This isn't the "last path component". If you have xds:///foo/bar then the string here is "foo/bar".
There was a problem hiding this comment.
Thanks for bringing this up. Just by intuition I assumed that the dataplane authority (or the virtual host domain name to lookup for) should be the endpoint from the dial target.
I brought this up with the gRPC xDS group and Mark has a patchset to the A47 gRFC to clarify this.
Given that I think this is the expected behavior.
There was a problem hiding this comment.
I agree they should be the same, but the docs you copied said it was different than you were using. (And the docs did say they were the same.)
There was a problem hiding this comment.
:) yes, comment needs updating.. I'll fix the comment once there is an LGTM on the doc update
There was a problem hiding this comment.
I've updated excerpt used in the docstring. PTAL :) @ejona86
There was a problem hiding this comment.
I don't see "any remaining '/' characters will be percent-encoded" implemented here.
There was a problem hiding this comment.
I did some investigation and it seems like we are percentage encoding everything in the URI path except for /s intentionally (see here). I have opened #7002 to track the investigation.
I've updated the PR to only change the virtualhost lookup logic and will track the % encoding question as a followup.
There was a problem hiding this comment.
Currently, there is an open question about whether or not to use the %-encoded authority to match the virtual host domain.
per @markdroth -
C-core currently uses the %-encoded authority to match the virtual host domain.
@ejona86 , @dfawley -- Do you have a strong preference or shall we be guided by the existing implementation?
There was a problem hiding this comment.
Per @ejona86 --
We will send the percent-encoded form on the wire, so it seems appropriate to match against the same here. That's what the server would match against.
In general, the fewer different representations of authority, the fewer chances for subtle bugs.
|
I've fixed the behavior as per the discussions in chat. Please take a look @ejona86 |
| server := stubserver.StartTestService(t, nil) | ||
| defer server.Stop() | ||
|
|
||
| const serviceName = "my-service-client-side-xds" |
There was a problem hiding this comment.
This string looks like it would have worked before your changes. Does this test fail without your changes to xds_resolver.go?
There was a problem hiding this comment.
Fixed. Now the serviceName has 2 special chars - , and /.
resolver/resolver.go
Outdated
| // Authority is the effective authority of the connection for which the | ||
| // resolver is built for. |
There was a problem hiding this comment.
client or ClientConn instead of connection.
And: for which the resolver is built.
| server := stubserver.StartTestService(t, nil) | ||
| defer server.Stop() | ||
|
|
||
| serviceName := "my-service-client-side-xds/2nd component" |
There was a problem hiding this comment.
Nit: please use const anywhere that it's possible to.
Resolve breaking change in grpc/grpc-go#6997. This is a low leverl interface so I don't think its a huge concern it broke
Resolve breaking change in grpc/grpc-go#6997. This is a low leverl interface so I don't think its a huge concern it broke
Resolve breaking change in grpc/grpc-go#6997. This is a low leverl interface so I don't think its a huge concern it broke
Resolve breaking change in grpc/grpc-go#6997. This is a low leverl interface so I don't think its a huge concern it broke
Resolve breaking change in grpc/grpc-go#6997. This is a low leverl interface so I don't think its a huge concern it broke (cherry picked from commit 48a1921)
Resolve breaking change in grpc/grpc-go#6997. This is a low leverl interface so I don't think its a huge concern it broke (cherry picked from commit 48a1921)
* Bump grpc-go and x/net (#50236) (cherry picked from commit e872055) * Bump gRPC deps (#50129) Resolve breaking change in grpc/grpc-go#6997. This is a low leverl interface so I don't think its a huge concern it broke (cherry picked from commit 48a1921) * Bump go-control-plane (#48841) * Bump go-control-plane * Bump and turn off by default * fix gen * bump (cherry picked from commit faf5bdd) --------- Co-authored-by: John Howard <howardjohn@google.com>
Fixes #6996
A new field
Authorityis added toresolver.BuildOptionsto pass the effective authority of clientconn for which the resolver was built.Testing:
I have verified that this diff works with the bootstrap generator @ GoogleCloudPlatform/traffic-director-grpc-bootstrap#57.
Test logs
RELEASE NOTES: none