xds: ensure node ID is populated in errors from the server (#8140)

* xds: ensure node ID is populated in errors from the server

* xds: cleanup server e2e tests

* don't wrap status errors

Author: easwars

internal/testutils/xds/e2e/clientresources.go

@@ -140,72 +140,6 @@ func marshalAny(m proto.Message) *anypb.Any {
 	return a
 }
 
-// filterChainWontMatch returns a filter chain that won't match if running the
-// test locally.
-func filterChainWontMatch(routeName string, addressPrefix string, srcPorts []uint32) *v3listenerpb.FilterChain {
-	hcm := &v3httppb.HttpConnectionManager{
-		RouteSpecifier: &v3httppb.HttpConnectionManager_Rds{
-			Rds: &v3httppb.Rds{
-				ConfigSource: &v3corepb.ConfigSource{
-					ConfigSourceSpecifier: &v3corepb.ConfigSource_Ads{Ads: &v3corepb.AggregatedConfigSource{}},
-				},
-				RouteConfigName: routeName,
-			},
-		},
-		HttpFilters: []*v3httppb.HttpFilter{RouterHTTPFilter},
-	}
-	return &v3listenerpb.FilterChain{
-		Name: routeName + "-wont-match",
-		FilterChainMatch: &v3listenerpb.FilterChainMatch{
-			PrefixRanges: []*v3corepb.CidrRange{
-				{
-					AddressPrefix: addressPrefix,
-					PrefixLen: &wrapperspb.UInt32Value{
-						Value: uint32(0),
-					},
-				},
-			},
-			SourceType:  v3listenerpb.FilterChainMatch_SAME_IP_OR_LOOPBACK,
-			SourcePorts: srcPorts,
-			SourcePrefixRanges: []*v3corepb.CidrRange{
-				{
-					AddressPrefix: addressPrefix,
-					PrefixLen: &wrapperspb.UInt32Value{
-						Value: uint32(0),
-					},
-				},
-			},
-		},
-		Filters: []*v3listenerpb.Filter{
-			{
-				Name:       "filter-1",
-				ConfigType: &v3listenerpb.Filter_TypedConfig{TypedConfig: marshalAny(hcm)},
-			},
-		},
-	}
-}
-
-// ListenerResourceThreeRouteResources returns a listener resource that points
-// to three route configurations. Only the filter chain that points to the first
-// route config can be matched to.
-func ListenerResourceThreeRouteResources(host string, port uint32, secLevel SecurityLevel, routeName string) *v3listenerpb.Listener {
-	lis := defaultServerListenerCommon(host, port, secLevel, routeName, false)
-	lis.FilterChains = append(lis.FilterChains, filterChainWontMatch("routeName2", "1.1.1.1", []uint32{1}))
-	lis.FilterChains = append(lis.FilterChains, filterChainWontMatch("routeName3", "2.2.2.2", []uint32{2}))
-	return lis
-}
-
-// ListenerResourceFallbackToDefault returns a listener resource that contains a
-// filter chain that will never get chosen to process traffic and a default
-// filter chain. The default filter chain points to routeName2.
-func ListenerResourceFallbackToDefault(host string, port uint32, secLevel SecurityLevel) *v3listenerpb.Listener {
-	lis := defaultServerListenerCommon(host, port, secLevel, "", false)
-	lis.FilterChains = nil
-	lis.FilterChains = append(lis.FilterChains, filterChainWontMatch("routeName", "1.1.1.1", []uint32{1}))
-	lis.DefaultFilterChain = filterChainWontMatch("routeName2", "2.2.2.2", []uint32{2})
-	return lis
-}
-
 // DefaultServerListener returns a basic xds Listener resource to be used on the
 // server side. The returned Listener resource contains an inline route
 // configuration with the name of routeName.

test/xds/xds_server_rbac_test.go

@@ -265,9 +265,13 @@ func (s) TestServerSideXDS_RouteConfiguration(t *testing.T) {
 	// This Unary Call should match to a route with an incorrect action. Thus,
 	// this RPC should not go through as per A36, and this call should receive
 	// an error with codes.Unavailable.
-	if _, err = client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != codes.Unavailable {
+	_, err = client.UnaryCall(ctx, &testpb.SimpleRequest{})
+	if status.Code(err) != codes.Unavailable {
 		t.Fatalf("client.UnaryCall() = _, %v, want _, error code %s", err, codes.Unavailable)
 	}
+	if !strings.Contains(err.Error(), nodeID) {
+		t.Fatalf("client.UnaryCall() = %v, want xDS node id %q", err, nodeID)
+	}
 
 	// This Streaming Call should match to a route with an incorrect action.
 	// Thus, this RPC should not go through as per A36, and this call should
@@ -276,8 +280,13 @@ func (s) TestServerSideXDS_RouteConfiguration(t *testing.T) {
 	if err != nil {
 		t.Fatalf("StreamingInputCall(_) = _, %v, want <nil>", err)
 	}
-	if _, err = stream.CloseAndRecv(); status.Code(err) != codes.Unavailable || !strings.Contains(err.Error(), "the incoming RPC matched to a route that was not of action type non forwarding") {
-		t.Fatalf("streaming RPC should have been denied")
+	_, err = stream.CloseAndRecv()
+	const wantStreamingErr = "the incoming RPC matched to a route that was not of action type non forwarding"
+	if status.Code(err) != codes.Unavailable || !strings.Contains(err.Error(), wantStreamingErr) {
+		t.Fatalf("client.StreamingInputCall() = %v, want error with code %s and message %q", err, codes.Unavailable, wantStreamingErr)
+	}
+	if !strings.Contains(err.Error(), nodeID) {
+		t.Fatalf("client.StreamingInputCall() = %v, want xDS node id %q", err, nodeID)
 	}
 
 	// This Full Duplex should not match to a route, and thus should return an
@@ -286,8 +295,13 @@ func (s) TestServerSideXDS_RouteConfiguration(t *testing.T) {
 	if err != nil {
 		t.Fatalf("FullDuplexCall(_) = _, %v, want <nil>", err)
 	}
-	if _, err = dStream.Recv(); status.Code(err) != codes.Unavailable || !strings.Contains(err.Error(), "the incoming RPC did not match a configured Route") {
-		t.Fatalf("streaming RPC should have been denied")
+	_, err = dStream.Recv()
+	const wantFullDuplexErr = "the incoming RPC did not match a configured Route"
+	if status.Code(err) != codes.Unavailable || !strings.Contains(err.Error(), wantFullDuplexErr) {
+		t.Fatalf("client.FullDuplexCall() = %v, want error with code %s and message %q", err, codes.Unavailable, wantFullDuplexErr)
+	}
+	if !strings.Contains(err.Error(), nodeID) {
+		t.Fatalf("client.FullDuplexCall() = %v, want xDS node id %q", err, nodeID)
 	}
 }
 
@@ -826,7 +840,7 @@ func serverListenerWithBadRouteConfiguration(t *testing.T, host string, port uin
 	}
 }
 
-func (s) TestRBACToggledOn_WithBadRouteConfiguration(t *testing.T) {
+func (s) TestRBAC_WithBadRouteConfiguration(t *testing.T) {
 	managementServer, nodeID, bootstrapContents, xdsResolver := setup.ManagementServerAndResolver(t)
 
 	lis, cleanup2 := setupGRPCServer(t, bootstrapContents)
@@ -867,11 +881,19 @@ func (s) TestRBACToggledOn_WithBadRouteConfiguration(t *testing.T) {
 	defer cc.Close()
 
 	client := testgrpc.NewTestServiceClient(cc)
-	if _, err := client.EmptyCall(ctx, &testpb.Empty{}); status.Code(err) != codes.Unavailable {
-		t.Fatalf("EmptyCall() returned err with status: %v, if RBAC is disabled all RPC's should proceed as normal", status.Code(err))
+	_, err = client.EmptyCall(ctx, &testpb.Empty{})
+	if status.Code(err) != codes.Unavailable {
+		t.Fatalf("EmptyCall() returned %v, want Unavailable", err)
+	}
+	if !strings.Contains(err.Error(), nodeID) {
+		t.Fatalf("EmptyCall() = %v, want xDS node id %q", err, nodeID)
+	}
+	_, err = client.UnaryCall(ctx, &testpb.SimpleRequest{})
+	if status.Code(err) != codes.Unavailable {
+		t.Fatalf("UnaryCall() returned %v, want Unavailable", err)
 	}
-	if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != codes.Unavailable {
-		t.Fatalf("UnaryCall() returned err with status: %v, if RBAC is disabled all RPC's should proceed as normal", status.Code(err))
+	if !strings.Contains(err.Error(), nodeID) {
+		t.Fatalf("UnaryCall() = %v, want xDS node id %q", err, nodeID)
 	}
 }