-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xdsclient/xdsresource: add AutoHostRewrite and Endpoint Hostname support #8728
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8728 +/- ##
==========================================
+ Coverage 83.21% 83.22% +0.01%
==========================================
Files 419 419
Lines 32443 32454 +11
==========================================
+ Hits 26997 27011 +14
- Misses 4056 4058 +2
+ Partials 1390 1385 -5
🚀 New features to boost your workflow:
|
easwars
left a comment
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.
The PR description says:
- This string value is included as a per-endpoint resolver attribute (hostname) in the parsed resource struct.
But the PR only stores it as a string, and not as a resolver attribute. The per-endpoint resolver attribute is defined here:
Line 191 in 69769dc
| Attributes *attributes.Attributes |
This means that the parsed Endpoint struct defined here:
| type Endpoint struct { |
resolver.Endpoint instead of a slice of address strings, and the newly added HostName field and some of the other existing things like Weight and HashKey also need to move into the endpoint attributes. This is probably big enough to be made as a separate PR though. Let's discuss this offline.
But for now, maybe the PR description need to be modified though to state that we are only storing it as a string for now and that in a follow-up PR we will change it to be an endpoint attribute.
internal/envconfig/envconfig.go
Outdated
| // XDSAuthorityRewrite is set if xDS authority rewriting is enabled, | ||
| // according to gRFC A81. It can be enabled by setting | ||
| // GRPC_EXPERIMENTAL_XDS_AUTHORITY_REWRITE to true. |
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.
Nit: Maybe something like:
// XDSAuthorityRewrite indicates whether xDS authority rewriting is enabled.
// This feature is defined in gRFC A81 and is enabled by setting the
// environment variable GRPC_EXPERIMENTAL_XDS_AUTHORITY_REWRITE to "true".
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.
Done.
| update.RouteConfigName = name | ||
| case *v3httppb.HttpConnectionManager_RouteConfig: | ||
| routeU, err := generateRDSUpdateFromRouteConfiguration(apiLis.GetRouteConfig()) | ||
| routeU, err := generateRDSUpdateFromRouteConfiguration(nil, apiLis.GetRouteConfig()) |
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.
Why are we passing nil here? This is an inline route configuration in the listener resource and needs to be handled exactly the same way as the non-inlined version is. So, you would have to plumb the DecodeOptions all the way from listenerResourceDecoder.Decode to unmarshalListenerResource to here (just like you are currently doing for the RDS decoding pipeline).
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.
Made the changes.
| // Can specify v3 here, as will never get to this function | ||
| // if v2. | ||
| routeU, err := generateRDSUpdateFromRouteConfiguration(hcm.GetRouteConfig()) | ||
| routeU, err := generateRDSUpdateFromRouteConfiguration(nil, hcm.GetRouteConfig()) |
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.
It is fine to pass nil here since this is server side listener resource processing and here we don't have to worry about authority rewriting. We might have to plump the decode options to this at some point in the future if they are required for something else on the server-side, but for now, we are good.
| func (d *routeConfigResourceDecoder) Decode(resource *xdsclient.AnyProto, _ xdsclient.DecodeOptions) (*xdsclient.DecodeResult, error) { | ||
| name, rc, err := unmarshalRouteConfigResource(resource.ToAny()) | ||
| func (d *routeConfigResourceDecoder) Decode(resource *xdsclient.AnyProto, opts xdsclient.DecodeOptions) (*xdsclient.DecodeResult, error) { | ||
| name, rc, err := unmarshalRouteConfigResource(&opts, resource.ToAny()) |
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.
Nit: Why is the order of arguments being reversed here? It would be nicer if we can retain the same order of arguments for Decode and unmarshalRouteConfigResource and the whole chain down from there. For here, and for the LDS side as well (which you are going to have to add, see another comment for more details).
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.
Done.
| } | ||
|
|
||
| got := update.VirtualHosts[0].Routes[0].AutoHostRewrite | ||
|
|
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.
Nit: delete newline
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.
done.
| ldsTarget = "lds.target.good:1111" | ||
| ) | ||
|
|
||
| buildRouteConfig := func() *v3routepb.RouteConfiguration { |
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.
Why is this a function? Can this be declared inside the test body instead?
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.
Done.
| Action: &v3routepb.Route_Route{ | ||
| Route: &v3routepb.RouteAction{ | ||
| ClusterSpecifier: &v3routepb.RouteAction_Cluster{Cluster: clusterName}, | ||
| HostRewriteSpecifier: &v3routepb.RouteAction_AutoHostRewrite{AutoHostRewrite: &wrapperspb.BoolValue{Value: true}}, |
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.
We probably also need test cases for when the AutoHostRewrite field in the proto is false.
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.
Added a testcase for that.
| m: func() *v3endpointpb.ClusterLoadAssignment { | ||
| clab0 := newClaBuilder("test", nil) | ||
| clab0.addLocality("locality-1", 1, 1, []endpointOpts{{addrWithPort: "addr1:314"}}, &addLocalityOptions{ | ||
| clab0.addLocality("locality-1", 1, 1, []endpointOpts{{addrWithPort: "addr1:314", hostname: "addr1"}}, &addLocalityOptions{ |
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.
I know these callsites are existing art, but they are really hard to read. While you are here, could you please follow guidelines from here to make them more readable. See: go/go-style/decisions#func-formatting
Thanks.
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.
Made the changes.
This PR implements the validation logic and extracting per endpoint Hostname attributes from xDS resources for gRFC A81
Key Changes:
RDS Resource Validation :
RouteAction.auto_host_rewriteis extracted from the RDS resource and stored in route structtruein the parsed route struct if thetrusted_xds_serveroption is present in theServerConfigand the global environment variable for authority overriding is enabled.EDS Resource Validation:
Endpoint.hostnamefield is extracted from the EDS resource and will be stored as ahostnamestring in parsed endpoint struct. It will be changed to be an per-endpoint resolver attribute in a follow-up PR.RELEASE NOTES: None