diff --git a/xds/internal/client/v2client_rds.go b/xds/internal/client/v2client_rds.go index 14ae6cdf..82140c9c 100644 --- a/xds/internal/client/v2client_rds.go +++ b/xds/internal/client/v2client_rds.go @@ -48,9 +48,9 @@ func (v2c *v2Client) handleRDSResponse(resp *xdspb.DiscoveryResponse) error { v2c.logger.Infof("Resource with name: %v, type: %T, contains: %v. Picking routes for current watching hostname %v", rc.GetName(), rc, rc, v2c.hostname) // Use the hostname (resourceName for LDS) to find the routes. - cluster := getClusterFromRouteConfiguration(rc, hostname) + cluster, err := getClusterFromRouteConfiguration(rc, hostname) if cluster == "" { - return fmt.Errorf("xds: received invalid RouteConfiguration in RDS response: %+v", rc) + return fmt.Errorf("xds: received invalid RouteConfiguration in RDS response: %+v with err: %v", rc, err) } // If we get here, it means that this resource was a good one. @@ -62,7 +62,8 @@ func (v2c *v2Client) handleRDSResponse(resp *xdspb.DiscoveryResponse) error { } // getClusterFromRouteConfiguration checks if the provided RouteConfiguration -// meets the expected criteria. If so, it returns a non-empty clusterName. +// meets the expected criteria. If so, it returns a non-empty clusterName with +// nil error. // // A RouteConfiguration resource is considered valid when only if it contains a // VirtualHost whose domain field matches the server name from the URI passed @@ -75,8 +76,7 @@ func (v2c *v2Client) handleRDSResponse(resp *xdspb.DiscoveryResponse) error { // only look at the last route in the list (the default route), whose match // field must be empty and whose route field must be set. Inside that route // message, the cluster field will contain the clusterName we are looking for. -func getClusterFromRouteConfiguration(rc *xdspb.RouteConfiguration, host string) string { - // TODO: return error for better error logging and nack. +func getClusterFromRouteConfiguration(rc *xdspb.RouteConfiguration, host string) (string, error) { // // Currently this returns "" on error, and the caller will return an error. // But the error doesn't contain details of why the response is invalid @@ -87,22 +87,22 @@ func getClusterFromRouteConfiguration(rc *xdspb.RouteConfiguration, host string) vh := findBestMatchingVirtualHost(host, rc.GetVirtualHosts()) if vh == nil { // No matching virtual host found. - return "" + return "", fmt.Errorf("no matching virtual host found") } if len(vh.Routes) == 0 { // The matched virtual host has no routes, this is invalid because there // should be at least one default route. - return "" + return "", fmt.Errorf("matched virtual host has no routes") } dr := vh.Routes[len(vh.Routes)-1] if match := dr.GetMatch(); match == nil || (match.GetPrefix() != "" && match.GetPrefix() != "/") { // The matched virtual host is invalid. Match is not "" or "/". - return "" + return "", fmt.Errorf("matched virtual host is invalid") } if route := dr.GetRoute(); route != nil { - return route.GetCluster() + return route.GetCluster(), nil } - return "" + return "", fmt.Errorf("matched route is nil") } type domainMatchType int diff --git a/xds/internal/client/v2client_rds_test.go b/xds/internal/client/v2client_rds_test.go index 8bbabb98..f9936d79 100644 --- a/xds/internal/client/v2client_rds_test.go +++ b/xds/internal/client/v2client_rds_test.go @@ -34,16 +34,19 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { name string rc *xdspb.RouteConfiguration wantCluster string + wantError bool }{ { name: "no-virtual-hosts-in-rc", rc: emptyRouteConfig, wantCluster: "", + wantError: true, }, { name: "no-domains-in-rc", rc: noDomainsInRouteConfig, wantCluster: "", + wantError: true, }, { name: "non-matching-domain-in-rc", @@ -53,6 +56,7 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { }, }, wantCluster: "", + wantError: true, }, { name: "no-routes-in-rc", @@ -62,6 +66,7 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { }, }, wantCluster: "", + wantError: true, }, { name: "default-route-match-field-is-nil", @@ -82,6 +87,7 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { }, }, wantCluster: "", + wantError: true, }, { name: "default-route-match-field-is-non-nil", @@ -99,6 +105,7 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { }, }, wantCluster: "", + wantError: true, }, { name: "default-route-routeaction-field-is-nil", @@ -111,6 +118,7 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { }, }, wantCluster: "", + wantError: true, }, { name: "default-route-cluster-field-is-empty", @@ -131,11 +139,13 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { }, }, wantCluster: "", + wantError: true, }, { name: "good-route-config-with-empty-string-route", rc: goodRouteConfig1, wantCluster: goodClusterName1, + wantError: false, }, { // default route's match is not empty string, but "/". @@ -156,7 +166,8 @@ func (s) TestRDSGetClusterFromRouteConfiguration(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if gotCluster := getClusterFromRouteConfiguration(test.rc, goodLDSTarget1); gotCluster != test.wantCluster { + gotCluster, gotError := getClusterFromRouteConfiguration(test.rc, goodLDSTarget1) + if gotCluster != test.wantCluster || (gotError != nil) != test.wantError { t.Errorf("getClusterFromRouteConfiguration(%+v, %v) = %v, want %v", test.rc, goodLDSTarget1, gotCluster, test.wantCluster) } })