From bcbb59a5d575137e8c92dbff4af96ff7a68531bd Mon Sep 17 00:00:00 2001 From: Saloni Date: Tue, 24 Sep 2024 18:14:57 -0700 Subject: [PATCH 1/3] fix unit tests to avoid data race in pipelines --- .../mode/static/state/graph/policies_test.go | 228 ++++++++---------- .../state/graph/reference_grant_test.go | 29 +-- 2 files changed, 108 insertions(+), 149 deletions(-) diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index 499b7e8386..340d3f10f0 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/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,63 @@ func TestAttachPolicies(t *testing.T) { } tests := []struct { - graph *Graph - expect func(g *WithT, graph *Graph) - name string + gateway *Gateway + routes map[RouteKey]*L7Route + ngfPolicies map[PolicyKey]*Policy + expect func(g *WithT, graph *Graph) + 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 +180,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) }) } } diff --git a/internal/mode/static/state/graph/reference_grant_test.go b/internal/mode/static/state/graph/reference_grant_test.go index 618cba181d..cb9f780a1d 100644 --- a/internal/mode/static/state/graph/reference_grant_test.go +++ b/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)) }) } } From b60945e7b7983ca5a2b78296048d10dd9842698d Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 25 Sep 2024 10:51:22 -0700 Subject: [PATCH 2/3] update docs for parallel tests --- docs/developer/testing.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/developer/testing.md b/docs/developer/testing.md index 205ae8f37b..48b7e73dcc 100644 --- a/docs/developer/testing.md +++ b/docs/developer/testing.md @@ -36,6 +36,8 @@ the [Counterfeiter](https://github.com/maxbrunsfeld/counterfeiter) tool. Counter implementations of internal and public interfaces, allowing us to isolate and control dependencies during testing. It simplifies the process of mocking and stubbing, making our tests more robust and flexible. +**Parallelize unit tests**: We use `t.Parallel()` to parallelize all unit tests and subtests, allowing us faster execution. To avoid race conditions, each test and subtest is designed to be independent. If a test cannot be parallelized due to sequential dependencies, a comment stating the reason must be provided. + By combining BDD style tests, unit tests, and mock generation, we aim to achieve a comprehensive and maintainable testing strategy. This approach enables us to ensure the correctness, reliability, and flexibility of our codebase while promoting efficient refactoring and continuous development. From 063446fb820076dc64673de5ac071db3ec6788d7 Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 25 Sep 2024 13:48:48 -0700 Subject: [PATCH 3/3] update doc based on reviews --- docs/developer/testing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developer/testing.md b/docs/developer/testing.md index 48b7e73dcc..7804930ff3 100644 --- a/docs/developer/testing.md +++ b/docs/developer/testing.md @@ -36,7 +36,7 @@ the [Counterfeiter](https://github.com/maxbrunsfeld/counterfeiter) tool. Counter implementations of internal and public interfaces, allowing us to isolate and control dependencies during testing. It simplifies the process of mocking and stubbing, making our tests more robust and flexible. -**Parallelize unit tests**: We use `t.Parallel()` to parallelize all unit tests and subtests, allowing us faster execution. To avoid race conditions, each test and subtest is designed to be independent. If a test cannot be parallelized due to sequential dependencies, a comment stating the reason must be provided. +**Parallelize unit tests**: In general, all tests should be designed to run in parallel for faster execution and help uncover bugs. For standard Go tests, this requires adding `t.Parallel()` to every test and subtest. Ginkgo tests, on the other hand, automatically run in parallel without the need for additional configuration. If a component under test requires sequential execution, you can run tests sequentially by using an [ordered container](https://onsi.github.io/ginkgo/#ordered-containers) for Ginkgo tests or by omitting `t.Parallel()` from the go test or subtest. In such cases, it’s essential to include a comment explaining why parallel execution is not possible. By combining BDD style tests, unit tests, and mock generation, we aim to achieve a comprehensive and maintainable testing strategy. This approach enables us to ensure the correctness, reliability, and flexibility of our codebase while