-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
xdsclient: use dataplane authority for virtualhost lookup #6997
Changes from 4 commits
e308543
9a35ee6
c66cf72
960d384
7d919f7
bb71708
b9a7650
c882c16
01375d4
5eb5211
4eca127
ea83d59
485ea4c
6265227
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -119,6 +119,7 @@ | |
endpoint = target.URL.Opaque | ||
} | ||
endpoint = strings.TrimPrefix(endpoint, "/") | ||
r.dataplaneAuthority = endpoint | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :) 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated excerpt used in the docstring. PTAL :) @ejona86 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see "any remaining '/' characters will be percent-encoded" implemented here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did some investigation and it seems like we are percentage encoding everything in the URI path except for 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, there is an open question about whether or not to use the %-encoded authority to match the virtual host domain. per @markdroth - @ejona86 , @dfawley -- Do you have a strong preference or shall we be guided by the existing implementation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per @ejona86 -- In general, the fewer different representations of authority, the fewer chances for subtle bugs. |
||
r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) | ||
r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) | ||
return r, nil | ||
|
@@ -190,6 +191,15 @@ | |
serializer *grpcsync.CallbackSerializer | ||
serializerCancel context.CancelFunc | ||
|
||
// Per [A47] the authority used for the data plane connections (which is | ||
// also used to select the VirtualHost within the xDS RouteConfiguration) | ||
// will continue to be the last path component of the xds URI used to create | ||
// the gRPC channel (i.e., the part following the last / character, or the | ||
// entire path if the path contains no / character). | ||
// | ||
// [A47]: https://github.com/grpc/proposal/blob/master/A47-xds-federation.md | ||
dataplaneAuthority string | ||
|
||
ldsResourceName string | ||
listenerWatcher *listenerWatcher | ||
listenerUpdateRecvd bool | ||
|
@@ -413,9 +423,9 @@ | |
} | ||
|
||
func (r *xdsResolver) applyRouteConfigUpdate(update xdsresource.RouteConfigUpdate) { | ||
matchVh := xdsresource.FindBestMatchingVirtualHost(r.ldsResourceName, update.VirtualHosts) | ||
matchVh := xdsresource.FindBestMatchingVirtualHost(r.dataplaneAuthority, update.VirtualHosts) | ||
if matchVh == nil { | ||
r.onError(fmt.Errorf("no matching virtual host found for %q", r.ldsResourceName)) | ||
r.onError(fmt.Errorf("no matching virtual host found for %q", r.dataplaneAuthority)) | ||
return | ||
} | ||
r.currentRouteConfig = update | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Now the serviceName has 2 special chars -
, and
/
.