xds/resolver: cleanup tests to use real xDS client 5/n #5955
Conversation
easwars
force-pushed
the
xds_resolver_test_cleanup_5
branch
from
774440f
to
def3983
Compare
| // Configure the management server with a listener resource that specifies a | ||
| // max stream duration as part of its HTTP connection manager. Also | ||
| // configure a route configuration resource, which has multiple routes with | ||
| // different values of max stream duration. |
There was a problem hiding this comment.
Can you also explain the logic of taking these multiple values and mapping to the rpc timeout? I.e. longest of all? How do the values in LDS and RDS interact?
There was a problem hiding this comment.
I don't understand what you mean by longest of all.
This is how I see it:
- there is a max_stream_duration setting in the HTTP connection manager in the listener
- there is also a max_stream_duration in the route action
At RPC time, we find a matching route in the config selector, and if the route contains a non-zero value for the max_stream_duration, we use it. Else, we use it from the listener resource.
There was a problem hiding this comment.
I didn't think longest of all was correct, just an example comment of explanation. Can you add this explanation that you just wrote to this comment string (can be as verbose or shorter)?
| RouteConfigName: rdsName, | ||
| }}, | ||
| HttpFilters: []*v3httppb.HttpFilter{e2e.RouterHTTPFilter}, | ||
| CommonHttpProtocolOptions: &v3corepb.HttpProtocolOptions{ | ||
| MaxStreamDuration: durationpb.New(1 * time.Second), | ||
| }, | ||
| }, nil) | ||
| }) | ||
| resources := e2e.UpdateOptions{ | ||
| NodeID: nodeID, | ||
| Listeners: []*v3listenerpb.Listener{{ | ||
| Name: ldsName, | ||
| ApiListener: &v3listenerpb.ApiListener{ApiListener: hcm}, | ||
| FilterChains: []*v3listenerpb.FilterChain{{ | ||
| Name: "filter-chain-name", | ||
| Filters: []*v3listenerpb.Filter{{ | ||
| Name: wellknown.HTTPConnectionManager, | ||
| ConfigType: &v3listenerpb.Filter_TypedConfig{TypedConfig: hcm}, | ||
| }}, | ||
| }}, | ||
| }}, | ||
| Routes: []*v3routepb.RouteConfiguration{{ | ||
| Name: rdsName, | ||
| VirtualHosts: []*v3routepb.VirtualHost{{ | ||
| Domains: []string{ldsName}, | ||
| Routes: []*v3routepb.Route{ | ||
| { | ||
| Match: &v3routepb.RouteMatch{PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/foo"}}, | ||
| Action: &v3routepb.Route_Route{Route: &v3routepb.RouteAction{ | ||
| ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{WeightedClusters: &v3routepb.WeightedCluster{ | ||
| Clusters: []*v3routepb.WeightedCluster_ClusterWeight{ | ||
| { | ||
| Name: "A", | ||
| Weight: &wrapperspb.UInt32Value{Value: 100}, | ||
| }, | ||
| }}, | ||
| }, | ||
| MaxStreamDuration: &v3routepb.RouteAction_MaxStreamDuration{ | ||
| MaxStreamDuration: durationpb.New(5 * time.Second), | ||
| }, | ||
| }}, | ||
| }, | ||
| { | ||
| Match: &v3routepb.RouteMatch{PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/bar"}}, | ||
| Action: &v3routepb.Route_Route{Route: &v3routepb.RouteAction{ | ||
| ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{WeightedClusters: &v3routepb.WeightedCluster{ | ||
| Clusters: []*v3routepb.WeightedCluster_ClusterWeight{ | ||
| { | ||
| Name: "B", | ||
| Weight: &wrapperspb.UInt32Value{Value: 100}, | ||
| }, | ||
| }}, | ||
| }, | ||
| MaxStreamDuration: &v3routepb.RouteAction_MaxStreamDuration{ | ||
| MaxStreamDuration: durationpb.New(0 * time.Second), | ||
| }, | ||
| }}, | ||
| }, | ||
| { | ||
| Match: &v3routepb.RouteMatch{PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/"}}, | ||
| Action: &v3routepb.Route_Route{Route: &v3routepb.RouteAction{ | ||
| ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{WeightedClusters: &v3routepb.WeightedCluster{ | ||
| Clusters: []*v3routepb.WeightedCluster_ClusterWeight{ | ||
| { | ||
| Name: "C", | ||
| Weight: &wrapperspb.UInt32Value{Value: 100}, | ||
| }, | ||
| }}, | ||
| }, | ||
| }}, | ||
| }, | ||
| }, | ||
| }}, | ||
| }}, | ||
| SkipValidation: true, | ||
| } |
There was a problem hiding this comment.
This is a really long inline update sitting in the test body. Is there any reusability of this with other inline updates elsewhere in this file? Can we pull this into a helper (I'm thinking of e2e_tests and DefaultXXXResource() with some wrappers for knobs)?
There was a problem hiding this comment.
I understand that this is long. But you don't have to read through this proto to understand this test. There is one comment where the proto is defined where it says:
// Configure the management server with a listener resource that specifies a
// max stream duration as part of its HTTP connection manager. Also
// configure a route configuration resource, which has multiple routes with
// different values of max stream duration.
There is also another comment at the top of the test which says:
// TestResolverMaxStreamDuration tests the case where the resolver receives max
// stream duration as part of the listener and route configuration resources.
// The test verifies that the RPC timeout returned by the config selector
// matches the value specified in the config.
Now, we have had convenience functions which build protos in the past (not sure if it still exists), and over time, it exploded to have way too many options and even though the test is terse when we take that approach, it is not very readable. And once the convenience functions become complex enough, they start deserving tests of their own.
With this approach, once you read the two comments, you know that the only value of interest in the protos in the max_stream_duration fields and then it becomes very straightforward to understand what is happening in the test.
If you feel otherwise, please let me know what you think.
There was a problem hiding this comment.
Thanks for the explanation. If there's a bunch of default boilerplate as part of the proto (that gets normal e2e flow), I feel like it saves a ton of lines to have a general helper and either a. another helper that wraps that helper which sets a field (in this case max stream duration) or call the helper and modify the resource in the test. I wasn't reading it regardless, the comment was in relation to a lot of extra lines in the test.
Done. Thanks. |
#resource-agnostic-xdsclient-api
RELEASE NOTES: none