From 7617e26a7b35d2082b92f54e7dd932e952ad44f3 Mon Sep 17 00:00:00 2001 From: Kate Osborn <50597707+kate-osborn@users.noreply.github.com> Date: Fri, 20 Dec 2024 08:48:40 -0700 Subject: [PATCH] Fix duplicate ancestors on UpstreamSettingsPolicy status (#2937) Problem: When an UpstreamSettingsPolicy targeted multiple Services, NGF would write duplicate ancestors to its status. Solution: Only add unique ancestors to policy ancestors list. --- internal/mode/static/state/graph/policies.go | 9 +- .../mode/static/state/graph/policies_test.go | 42 +++++++ .../static/state/graph/policy_ancestor.go | 34 ++++++ .../state/graph/policy_ancestor_test.go | 110 ++++++++++++++++++ 4 files changed, 194 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/state/graph/policies.go b/internal/mode/static/state/graph/policies.go index 9ce3e7eb23..7634c2d099 100644 --- a/internal/mode/static/state/graph/policies.go +++ b/internal/mode/static/state/graph/policies.go @@ -112,11 +112,18 @@ func attachPolicyToService( if !gw.Valid { ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("Parent Gateway is invalid")} + if ancestorsContainsAncestorRef(policy.Ancestors, ancestor.Ancestor) { + return + } + policy.Ancestors = append(policy.Ancestors, ancestor) return } - policy.Ancestors = append(policy.Ancestors, ancestor) + if !ancestorsContainsAncestorRef(policy.Ancestors, ancestor.Ancestor) { + policy.Ancestors = append(policy.Ancestors, ancestor) + } + svc.Policies = append(svc.Policies, policy) } diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index 2c11ac2229..6a840230fb 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/internal/mode/static/state/graph/policies_test.go @@ -539,6 +539,7 @@ func TestAttachPolicyToService(t *testing.T) { t.Parallel() gwNsname := types.NamespacedName{Namespace: testNs, Name: "gateway"} + gw2Nsname := types.NamespacedName{Namespace: testNs, Name: "gateway2"} getGateway := func(valid bool) *Gateway { return &Gateway{ @@ -572,6 +573,47 @@ func TestAttachPolicyToService(t *testing.T) { }, }, }, + { + name: "attachment; ancestor already exists so don't duplicate", + policy: &Policy{ + Source: &policiesfakes.FakePolicy{}, + Ancestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gwNsname), + }, + }, + }, + svc: &ReferencedService{}, + gw: getGateway(true /*valid*/), + expAttached: true, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gwNsname), // only one ancestor per Gateway + }, + }, + }, + { + name: "attachment; ancestor doesn't exists so add it", + policy: &Policy{ + Source: &policiesfakes.FakePolicy{}, + Ancestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gw2Nsname), + }, + }, + }, + svc: &ReferencedService{}, + gw: getGateway(true /*valid*/), + expAttached: true, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gw2Nsname), + }, + { + Ancestor: getGatewayParentRef(gwNsname), + }, + }, + }, { name: "no attachment; gateway is invalid", policy: &Policy{Source: &policiesfakes.FakePolicy{}}, diff --git a/internal/mode/static/state/graph/policy_ancestor.go b/internal/mode/static/state/graph/policy_ancestor.go index 7120f94afa..b55990d6d3 100644 --- a/internal/mode/static/state/graph/policy_ancestor.go +++ b/internal/mode/static/state/graph/policy_ancestor.go @@ -4,6 +4,8 @@ import ( "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" ) const maxAncestors = 16 @@ -61,3 +63,35 @@ func createParentReference( Name: v1.ObjectName(nsname.Name), } } + +func ancestorsContainsAncestorRef(ancestors []PolicyAncestor, ref v1.ParentReference) bool { + for _, an := range ancestors { + if parentRefEqual(an.Ancestor, ref) { + return true + } + } + + return false +} + +func parentRefEqual(ref1, ref2 v1.ParentReference) bool { + if !helpers.EqualPointers(ref1.Kind, ref2.Kind) { + return false + } + + if !helpers.EqualPointers(ref1.Group, ref2.Group) { + return false + } + + if !helpers.EqualPointers(ref1.Namespace, ref2.Namespace) { + return false + } + + // we don't check the other fields in ParentRef because we don't set them + + if ref1.Name != ref2.Name { + return false + } + + return true +} diff --git a/internal/mode/static/state/graph/policy_ancestor_test.go b/internal/mode/static/state/graph/policy_ancestor_test.go index 794caf6c2f..0b34f8e1ef 100644 --- a/internal/mode/static/state/graph/policy_ancestor_test.go +++ b/internal/mode/static/state/graph/policy_ancestor_test.go @@ -4,10 +4,12 @@ import ( "testing" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" ) func TestBackendTLSPolicyAncestorsFull(t *testing.T) { @@ -169,3 +171,111 @@ func TestNGFPolicyAncestorsFull(t *testing.T) { }) } } + +func TestAncestorContainsAncestorRef(t *testing.T) { + t.Parallel() + + gw1 := types.NamespacedName{Namespace: testNs, Name: "gw1"} + gw2 := types.NamespacedName{Namespace: testNs, Name: "gw2"} + route := types.NamespacedName{Namespace: testNs, Name: "route"} + newRoute := types.NamespacedName{Namespace: testNs, Name: "new-route"} + + ancestors := []PolicyAncestor{ + { + Ancestor: createParentReference(v1.GroupName, kinds.Gateway, gw1), + }, + { + Ancestor: createParentReference(v1.GroupName, kinds.Gateway, gw2), + }, + { + Ancestor: createParentReference(v1.GroupName, kinds.HTTPRoute, route), + }, + } + + tests := []struct { + ref v1.ParentReference + name string + contains bool + }{ + { + name: "contains Gateway ref", + ref: createParentReference(v1.GroupName, kinds.Gateway, gw1), + contains: true, + }, + { + name: "contains Route ref", + ref: createParentReference(v1.GroupName, kinds.HTTPRoute, route), + contains: true, + }, + { + name: "does not contain ref", + ref: createParentReference(v1.GroupName, kinds.HTTPRoute, newRoute), + contains: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + g.Expect(ancestorsContainsAncestorRef(ancestors, test.ref)).To(Equal(test.contains)) + }) + } +} + +func TestParentRefEqual(t *testing.T) { + t.Parallel() + ref1NsName := types.NamespacedName{Namespace: testNs, Name: "ref1"} + + ref1 := createParentReference(v1.GroupName, kinds.HTTPRoute, ref1NsName) + + tests := []struct { + ref v1.ParentReference + name string + equal bool + }{ + { + name: "kinds different", + ref: createParentReference(v1.GroupName, kinds.Gateway, ref1NsName), + equal: false, + }, + { + name: "groups different", + ref: createParentReference("diff-group", kinds.HTTPRoute, ref1NsName), + equal: false, + }, + { + name: "namespace different", + ref: createParentReference( + v1.GroupName, + kinds.HTTPRoute, + types.NamespacedName{Namespace: "diff-ns", Name: "ref1"}, + ), + equal: false, + }, + { + name: "name different", + ref: createParentReference( + v1.GroupName, + kinds.HTTPRoute, + types.NamespacedName{Namespace: testNs, Name: "diff-name"}, + ), + equal: false, + }, + { + name: "equal", + ref: createParentReference(v1.GroupName, kinds.HTTPRoute, ref1NsName), + equal: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + g.Expect(parentRefEqual(ref1, test.ref)).To(Equal(test.equal)) + }) + } +}