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.
nginxinc · Dec 20, 2024 · 7617e26 · 7617e26
1 parent ac9c937
commit 7617e26
Show file tree

Hide file tree

Showing 4 changed files with 194 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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
@@ -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))
+		})
+	}
+}