fix unit tests to avoid data race in pipelines

Author: salonichf5

internal/mode/static/state/graph/policies_test.go

@@ -25,60 +25,20 @@ var testNs = "test"
 func TestAttachPolicies(t *testing.T) {
 	t.Parallel()
 	policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "Policy"}
-
-	gwPolicyKey := createTestPolicyKey(policyGVK, "gw-policy")
-	gwPolicy := &Policy{
-		Valid:  true,
-		Source: &policiesfakes.FakePolicy{},
-		TargetRefs: []PolicyTargetRef{
-			{
-				Kind:   kinds.Gateway,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway"},
-			},
-			{
-				Kind:   kinds.Gateway,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway2"}, // ignored
-			},
-		},
-	}
-
-	routePolicyKey := createTestPolicyKey(policyGVK, "route-policy")
-	routePolicy := &Policy{
-		Valid:  true,
-		Source: &policiesfakes.FakePolicy{},
-		TargetRefs: []PolicyTargetRef{
-			{
-				Kind:   kinds.HTTPRoute,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-route"},
-			},
-			{
-				Kind:   kinds.HTTPRoute,
-				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "hr2-route"},
-			},
-		},
-	}
-
-	grpcRoutePolicyKey := createTestPolicyKey(policyGVK, "grpc-route-policy")
-	grpcRoutePolicy := &Policy{
-		Valid:  true,
-		Source: &policiesfakes.FakePolicy{},
-		TargetRefs: []PolicyTargetRef{
-			{
-				Kind:   kinds.GRPCRoute,
+	createPolicy := func(targetRefsNames []string, refKind v1.Kind) *Policy {
+		targetRefs := make([]PolicyTargetRef, 0, len(targetRefsNames))
+		for _, name := range targetRefsNames {
+			targetRefs = append(targetRefs, PolicyTargetRef{
+				Kind:   refKind,
 				Group:  v1.GroupName,
-				Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc-route"},
-			},
-		},
-	}
-
-	ngfPolicies := map[PolicyKey]*Policy{
-		gwPolicyKey:        gwPolicy,
-		routePolicyKey:     routePolicy,
-		grpcRoutePolicyKey: grpcRoutePolicy,
+				Nsname: types.NamespacedName{Namespace: testNs, Name: name},
+			})
+		}
+		return &Policy{
+			Valid:      true,
+			Source:     &policiesfakes.FakePolicy{},
+			TargetRefs: targetRefs,
+		}
 	}
 
 	createRouteKey := func(name string, routeType RouteType) RouteKey {
@@ -88,77 +48,40 @@ func TestAttachPolicies(t *testing.T) {
 		}
 	}
 
-	newGraph := func() *Graph {
-		return &Graph{
-			Gateway: &Gateway{
-				Source: &v1.Gateway{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "gateway",
-						Namespace: testNs,
-					},
+	createGateway := func(name string) *Gateway {
+		return &Gateway{
+			Source: &v1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      name,
+					Namespace: testNs,
 				},
-				Valid: true,
 			},
-			Routes: map[RouteKey]*L7Route{
-				createRouteKey("hr-route", RouteTypeHTTP): {
-					Source: &v1.HTTPRoute{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:      "hr-route",
-							Namespace: testNs,
-						},
-					},
-					ParentRefs: []ParentRef{
-						{
-							Attachment: &ParentRefAttachmentStatus{
-								Attached: true,
-							},
-						},
-					},
-					Valid:      true,
-					Attachable: true,
-				},
-				createRouteKey("hr2-route", RouteTypeHTTP): {
-					Source: &v1.HTTPRoute{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:      "hr2-route",
-							Namespace: testNs,
-						},
-					},
-					ParentRefs: []ParentRef{
-						{
-							Attachment: &ParentRefAttachmentStatus{
-								Attached: true,
-							},
-						},
+			Valid: true,
+		}
+	}
+
+	createRoutesForGraph := func(routes map[string]RouteType) map[RouteKey]*L7Route {
+		routesMap := make(map[RouteKey]*L7Route, len(routes))
+		for routeName, routeType := range routes {
+			routesMap[createRouteKey(routeName, routeType)] = &L7Route{
+				Source: &v1.HTTPRoute{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      routeName,
+						Namespace: testNs,
 					},
-					Valid:      true,
-					Attachable: true,
 				},
-				createRouteKey("grpc-route", RouteTypeGRPC): {
-					Source: &v1alpha2.GRPCRoute{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:      "grpc-route",
-							Namespace: testNs,
-						},
-					},
-					ParentRefs: []ParentRef{
-						{
-							Attachment: &ParentRefAttachmentStatus{
-								Attached: true,
-							},
+				ParentRefs: []ParentRef{
+					{
+						Attachment: &ParentRefAttachmentStatus{
+							Attached: true,
 						},
 					},
-					Valid:      true,
-					Attachable: true,
 				},
-			},
-
-			NGFPolicies: ngfPolicies,
+				Valid:      true,
+				Attachable: true,
+			}
 		}
-	}
-
-	newModifiedGraph := func(mod func(g *Graph) *Graph) *Graph {
-		return mod(newGraph())
+		return routesMap
 	}
 
 	expectNoPolicyAttachment := func(g *WithT, graph *Graph) {
@@ -192,30 +115,65 @@ func TestAttachPolicies(t *testing.T) {
 	}
 
 	tests := []struct {
-		graph  *Graph
-		expect func(g *WithT, graph *Graph)
-		name   string
+		// graph          *Graph
+		gateway        *Gateway
+		routes         map[RouteKey]*L7Route
+		ngfPolicies    map[PolicyKey]*Policy
+		expect         func(g *WithT, graph *Graph)
+		controllerName string
+		name           string
 	}{
 		{
 			name: "nil Gateway",
-			graph: newModifiedGraph(func(g *Graph) *Graph {
-				g.Gateway = nil
-				return g
-			}),
+			routes: createRoutesForGraph(
+				map[string]RouteType{
+					"hr1-route":  RouteTypeHTTP,
+					"hr2-route":  RouteTypeHTTP,
+					"grpc-route": RouteTypeGRPC,
+				},
+			),
+			ngfPolicies: map[PolicyKey]*Policy{
+				createTestPolicyKey(policyGVK, "gw-policy"): createPolicy([]string{"gateway", "gateway1"}, kinds.Gateway),
+				createTestPolicyKey(policyGVK, "route-policy"): createPolicy(
+					[]string{"hr1-route", "hr2-route"},
+					kinds.HTTPRoute,
+				),
+				createTestPolicyKey(policyGVK, "grpc-route-policy"): createPolicy([]string{"grpc-route"}, kinds.GRPCRoute),
+			},
 			expect: expectNoPolicyAttachment,
 		},
 		{
-			name: "nil routes",
-			graph: newModifiedGraph(func(g *Graph) *Graph {
-				g.Routes = nil
-				return g
-			}),
+			name:    "nil routes",
+			gateway: createGateway("gateway"),
+			ngfPolicies: map[PolicyKey]*Policy{
+				createTestPolicyKey(policyGVK, "gw-policy1"): createPolicy([]string{"gateway", "gateway1"}, kinds.Gateway),
+				createTestPolicyKey(policyGVK, "route-policy1"): createPolicy(
+					[]string{"hr1-route", "hr2-route"},
+					kinds.HTTPRoute,
+				),
+				createTestPolicyKey(policyGVK, "grpc-route-policy1"): createPolicy([]string{"grpc-route"}, kinds.GRPCRoute),
+			},
 			expect: expectGatewayPolicyAttachment,
 		},
 		{
-			name:   "normal",
-			graph:  newGraph(),
-			expect: expectPolicyAttachment,
+			name: "normal",
+			routes: createRoutesForGraph(
+				map[string]RouteType{
+					"hr-1":   RouteTypeHTTP,
+					"hr-2":   RouteTypeHTTP,
+					"grpc-1": RouteTypeGRPC,
+				},
+			),
+			ngfPolicies: map[PolicyKey]*Policy{
+				createTestPolicyKey(policyGVK, "gw-policy2"): createPolicy([]string{"gateway2", "gateway3"}, kinds.Gateway),
+				createTestPolicyKey(policyGVK, "route-policy2"): createPolicy(
+					[]string{"hr-1", "hr-2"},
+					kinds.HTTPRoute,
+				),
+				createTestPolicyKey(policyGVK, "grpc-route-policy2"): createPolicy([]string{"grpc-1"}, kinds.GRPCRoute),
+			},
+			gateway: createGateway("gateway2"),
+			expect:  expectPolicyAttachment,
 		},
 	}
 
@@ -224,8 +182,14 @@ func TestAttachPolicies(t *testing.T) {
 			t.Parallel()
 			g := NewWithT(t)
 
-			test.graph.attachPolicies("nginx-gateway")
-			test.expect(g, test.graph)
+			graph := &Graph{
+				Gateway:     test.gateway,
+				Routes:      test.routes,
+				NGFPolicies: test.ngfPolicies,
+			}
+
+			graph.attachPolicies("nginx-gateway")
+			test.expect(g, graph)
 		})
 	}
 }

internal/mode/static/state/graph/reference_grant_test.go

@@ -345,63 +345,57 @@ func TestRefAllowedFrom(t *testing.T) {
 		},
 	}
 
-	resolver := newReferenceGrantResolver(refGrants)
-	refAllowedFromGRPCRoute := resolver.refAllowedFrom(fromGRPCRoute(grNs))
-	refAllowedFromHTTPRoute := resolver.refAllowedFrom(fromHTTPRoute(hrNs))
-	refAllowedFromTLSRoute := resolver.refAllowedFrom(fromTLSRoute(trNs))
-	refAllowedFromGateway := resolver.refAllowedFrom(fromGateway(gwNs))
-
 	tests := []struct {
 		name           string
-		refAllowedFrom func(resource toResource) bool
+		refAllowedFrom fromResource
 		toResource     toResource
 		expAllowed     bool
 	}{
 		{
 			name:           "ref allowed from gateway to secret",
-			refAllowedFrom: refAllowedFromGateway,
+			refAllowedFrom: fromGateway(gwNs),
 			toResource:     toSecret(allowedGatewayNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from gateway to secret",
-			refAllowedFrom: refAllowedFromGateway,
+			refAllowedFrom: fromGateway(gwNs),
 			toResource:     toSecret(notAllowedNsName),
 			expAllowed:     false,
 		},
 		{
 			name:           "ref allowed from httproute to service",
-			refAllowedFrom: refAllowedFromHTTPRoute,
+			refAllowedFrom: fromHTTPRoute(hrNs),
 			toResource:     toService(allowedHTTPRouteNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from httproute to service",
-			refAllowedFrom: refAllowedFromHTTPRoute,
+			refAllowedFrom: fromHTTPRoute(hrNs),
 			toResource:     toService(notAllowedNsName),
 			expAllowed:     false,
 		},
 		{
 			name:           "ref allowed from grpcroute to service",
-			refAllowedFrom: refAllowedFromGRPCRoute,
+			refAllowedFrom: fromGRPCRoute(grNs),
 			toResource:     toService(allowedGRPCRouteNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from grpcroute to service",
-			refAllowedFrom: refAllowedFromGRPCRoute,
+			refAllowedFrom: fromGRPCRoute(grNs),
 			toResource:     toService(notAllowedNsName),
 			expAllowed:     false,
 		},
 		{
 			name:           "ref allowed from tlsroute to service",
-			refAllowedFrom: refAllowedFromTLSRoute,
+			refAllowedFrom: fromTLSRoute(trNs),
 			toResource:     toService(allowedTLSRouteNsName),
 			expAllowed:     true,
 		},
 		{
 			name:           "ref not allowed from tlsroute to service",
-			refAllowedFrom: refAllowedFromTLSRoute,
+			refAllowedFrom: fromTLSRoute(trNs),
 			toResource:     toService(notAllowedNsName),
 			expAllowed:     false,
 		},
@@ -411,8 +405,11 @@ func TestRefAllowedFrom(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
+			resolver := newReferenceGrantResolver(refGrants)
+			refAllowed := resolver.refAllowedFrom(test.refAllowedFrom)
+
 			g := NewWithT(t)
-			g.Expect(test.refAllowedFrom(test.toResource)).To(Equal(test.expAllowed))
+			g.Expect(refAllowed(test.toResource)).To(Equal(test.expAllowed))
 		})
 	}
 }