Fix policy attachment when ancestors slice is full

Author: Sarthak Agrawal

internal/controller/state/change_processor.go

@@ -268,6 +268,7 @@ func (c *ChangeProcessorImpl) Process() *graph.Graph {
 		c.cfg.GatewayClassName,
 		c.cfg.PlusSecrets,
 		c.cfg.Validators,
+		c.cfg.Logger,
 	)
 
 	return c.latestGraph

internal/controller/state/conditions/conditions.go

@@ -133,6 +133,15 @@ const (
 	// GatewayReasonParamsRefInvalid is used with the "GatewayResolvedRefs" condition when the
 	// parametersRef resource is invalid.
 	GatewayReasonParamsRefInvalid v1.GatewayConditionReason = "ParametersRefInvalid"
+
+	// PolicyReasonAncestorLimitReached is used with the "PolicyAccepted" condition when a policy
+	// cannot be applied because the ancestor status list has reached the maximum size of 16.
+	PolicyReasonAncestorLimitReached v1alpha2.PolicyConditionReason = "AncestorLimitReached"
+
+	// PolicyMessageAncestorLimitReached is a message used with PolicyReasonAncestorLimitReached
+	// when a policy cannot be applied due to the 16 ancestor limit being reached.
+	PolicyMessageAncestorLimitReached = "Policy cannot be applied because the ancestor status list " +
+		"has reached the maximum size of 16"
 )
 
 // Condition defines a condition to be reported in the status of resources.
@@ -933,6 +942,17 @@ func NewPolicyTargetNotFound(msg string) Condition {
 	}
 }
 
+// NewPolicyAncestorLimitReached returns a Condition that indicates that the Policy is not accepted because
+// the ancestor status list has reached the maximum size of 16.
+func NewPolicyAncestorLimitReached(_ string) Condition {
+	return Condition{
+		Type:    string(v1alpha2.PolicyConditionAccepted),
+		Status:  metav1.ConditionFalse,
+		Reason:  string(PolicyReasonAncestorLimitReached),
+		Message: PolicyMessageAncestorLimitReached,
+	}
+}
+
 // NewPolicyNotAcceptedTargetConflict returns a Condition that indicates that the Policy is not accepted
 // because the target resource has a conflict with another resource when attempting to apply this policy.
 func NewPolicyNotAcceptedTargetConflict(msg string) Condition {

internal/controller/state/conditions/conditions_test.go

@@ -5,6 +5,7 @@ import (
 
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/gateway-api/apis/v1alpha2"
 )
 
 func TestDeduplicateConditions(t *testing.T) {
@@ -145,3 +146,16 @@ func TestHasMatchingCondition(t *testing.T) {
 		})
 	}
 }
+
+func TestNewPolicyAncestorLimitReached(t *testing.T) {
+	t.Parallel()
+	g := NewWithT(t)
+
+	policyName := "test-policy"
+	condition := NewPolicyAncestorLimitReached(policyName)
+
+	g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
+	g.Expect(condition.Status).To(Equal(metav1.ConditionFalse))
+	g.Expect(condition.Reason).To(Equal(string(PolicyReasonAncestorLimitReached)))
+	g.Expect(condition.Message).To(Equal(PolicyMessageAncestorLimitReached))
+}

internal/controller/state/dataplane/configuration.go

@@ -390,7 +390,7 @@ func newBackendGroup(
 			UpstreamName: ref.ServicePortReference(),
 			Weight:       ref.Weight,
 			Valid:        valid,
-			VerifyTLS:    convertBackendTLS(ref.BackendTLSPolicy),
+			VerifyTLS:    convertBackendTLS(ref.BackendTLSPolicy, gatewayName),
 		})
 	}
 
@@ -401,10 +401,15 @@ func newBackendGroup(
 	}
 }
 
-func convertBackendTLS(btp *graph.BackendTLSPolicy) *VerifyTLS {
+func convertBackendTLS(btp *graph.BackendTLSPolicy, gwNsName types.NamespacedName) *VerifyTLS {
 	if btp == nil || !btp.Valid {
 		return nil
 	}
+
+	if !slices.Contains(btp.Gateways, gwNsName) {
+		return nil
+	}
+
 	verify := &VerifyTLS{}
 	if btp.CaCertRef.Name != "" {
 		verify.CertBundleID = generateCertBundleID(btp.CaCertRef)

internal/controller/state/dataplane/configuration_test.go

@@ -607,6 +607,7 @@ func TestBuildConfiguration(t *testing.T) {
 		},
 		CaCertRef: types.NamespacedName{Namespace: "test", Name: "configmap-1"},
 		Valid:     true,
+		Gateways:  []types.NamespacedName{gatewayNsName},
 	}
 
 	expHTTPSHR8Groups[0].Backends[0].VerifyTLS = &VerifyTLS{
@@ -661,6 +662,7 @@ func TestBuildConfiguration(t *testing.T) {
 		},
 		CaCertRef: types.NamespacedName{Namespace: "test", Name: "configmap-2"},
 		Valid:     true,
+		Gateways:  []types.NamespacedName{gatewayNsName},
 	}
 
 	expHTTPSHR9Groups[0].Backends[0].VerifyTLS = &VerifyTLS{
@@ -3569,6 +3571,9 @@ func TestHostnameMoreSpecific(t *testing.T) {
 
 func TestConvertBackendTLS(t *testing.T) {
 	t.Parallel()
+
+	testGateway := types.NamespacedName{Namespace: "test", Name: "gateway"}
+
 	btpCaCertRefs := &graph.BackendTLSPolicy{
 		Source: &v1alpha3.BackendTLSPolicy{
 			ObjectMeta: metav1.ObjectMeta{
@@ -3588,6 +3593,7 @@ func TestConvertBackendTLS(t *testing.T) {
 		},
 		Valid:     true,
 		CaCertRef: types.NamespacedName{Namespace: "test", Name: "ca-cert"},
+		Gateways:  []types.NamespacedName{testGateway},
 	}
 
 	btpWellKnownCerts := &graph.BackendTLSPolicy{
@@ -3598,7 +3604,8 @@ func TestConvertBackendTLS(t *testing.T) {
 				},
 			},
 		},
-		Valid: true,
+		Valid:    true,
+		Gateways: []types.NamespacedName{testGateway},
 	}
 
 	expectedWithCertPath := &VerifyTLS{
@@ -3615,31 +3622,41 @@ func TestConvertBackendTLS(t *testing.T) {
 
 	tests := []struct {
 		btp      *graph.BackendTLSPolicy
+		gwNsName types.NamespacedName
 		expected *VerifyTLS
 		msg      string
 	}{
 		{
 			btp:      nil,
+			gwNsName: testGateway,
 			expected: nil,
 			msg:      "nil backend tls policy",
 		},
 		{
 			btp:      btpCaCertRefs,
+			gwNsName: testGateway,
 			expected: expectedWithCertPath,
 			msg:      "normal case with cert path",
 		},
 		{
 			btp:      btpWellKnownCerts,
+			gwNsName: testGateway,
 			expected: expectedWithWellKnownCerts,
 			msg:      "normal case no cert path",
 		},
+		{
+			btp:      btpCaCertRefs,
+			gwNsName: types.NamespacedName{Namespace: "test", Name: "unsupported-gateway"},
+			expected: nil,
+			msg:      "gateway not supported by policy",
+		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.msg, func(t *testing.T) {
 			t.Parallel()
 			g := NewWithT(t)
-			g.Expect(convertBackendTLS(tc.btp)).To(Equal(tc.expected))
+			g.Expect(convertBackendTLS(tc.btp, tc.gwNsName)).To(Equal(tc.expected))
 		})
 	}
 }

internal/controller/state/graph/backend_tls_policy.go

@@ -18,7 +18,8 @@ type BackendTLSPolicy struct {
 	Source *v1alpha3.BackendTLSPolicy
 	// CaCertRef is the name of the ConfigMap that contains the CA certificate.
 	CaCertRef types.NamespacedName
-	// Gateways are the names of the Gateways that are being checked for this BackendTLSPolicy.
+	// Gateways are the names of the Gateways for which this BackendTLSPolicy is effectively applied.
+	// Only contains gateways where the policy can be applied (not limited by ancestor status).
 	Gateways []types.NamespacedName
 	// Conditions include Conditions for the BackendTLSPolicy.
 	Conditions []conditions.Condition
@@ -68,16 +69,13 @@ func validateBackendTLSPolicy(
 	backendTLSPolicy *v1alpha3.BackendTLSPolicy,
 	configMapResolver *configMapResolver,
 	secretResolver *secretResolver,
-	ctlrName string,
+	_ string,
 ) (valid, ignored bool, conds []conditions.Condition) {
 	valid = true
 	ignored = false
 
-	// FIXME (kate-osborn): https://github.com/nginx/nginx-gateway-fabric/issues/1987
-	if backendTLSPolicyAncestorsFull(backendTLSPolicy.Status.Ancestors, ctlrName) {
-		valid = false
-		ignored = true
-	}
+	// Note: Ancestor limit checking moved to addGatewaysForBackendTLSPolicies for per-gateway effectiveness tracking
+	// The policy may be partially effective (work for some gateways but not others due to ancestor limits)
 
 	if err := validateBackendTLSHostname(backendTLSPolicy); err != nil {
 		valid = false
@@ -186,10 +184,12 @@ func validateBackendTLSWellKnownCACerts(btp *v1alpha3.BackendTLSPolicy) error {
 func addGatewaysForBackendTLSPolicies(
 	backendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy,
 	services map[types.NamespacedName]*ReferencedService,
+	ctlrName string,
 ) {
 	for _, backendTLSPolicy := range backendTLSPolicies {
-		gateways := make(map[types.NamespacedName]struct{})
+		potentialGateways := make(map[types.NamespacedName]struct{})
 
+		// First, collect all potential gateways for this policy
 		for _, refs := range backendTLSPolicy.Source.Spec.TargetRefs {
 			if refs.Kind != kinds.Service {
 				continue
@@ -201,13 +201,32 @@ func addGatewaysForBackendTLSPolicies(
 				}
 
 				for gateway := range referencedServices.GatewayNsNames {
-					gateways[gateway] = struct{}{}
+					potentialGateways[gateway] = struct{}{}
 				}
 			}
 		}
 
-		for gateway := range gateways {
-			backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gateway)
+		// Now check each potential gateway against ancestor limits
+		for gatewayNsName := range potentialGateways {
+			// Create a proposed ancestor reference for this gateway
+			proposedAncestor := createParentReference(v1.GroupName, kinds.Gateway, gatewayNsName)
+
+			// Check ancestor limit for BackendTLS policy
+			isFull := backendTLSPolicyAncestorsFull(
+				backendTLSPolicy.Source.Status.Ancestors,
+				ctlrName,
+			)
+
+			if isFull {
+				policyName := backendTLSPolicy.Source.Namespace + "/" + backendTLSPolicy.Source.Name
+				gatewayName := getAncestorName(proposedAncestor)
+				LogAncestorLimitReached(policyName, "BackendTLSPolicy", gatewayName)
+
+				continue
+			}
+
+			// Gateway can be effectively used by this policy
+			backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gatewayNsName)
 		}
 	}
 }

internal/controller/state/graph/backend_tls_policy_test.go

@@ -399,7 +399,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
 			},
 		},
 		{
-			name: "invalid case with too many ancestors",
+			name: "valid case with many ancestors (ancestor limit now handled during gateway assignment)",
 			tlsPolicy: &v1alpha3.BackendTLSPolicy{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "tls-policy",
@@ -416,7 +416,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
 					Ancestors: ancestors,
 				},
 			},
-			ignored: true,
+			isValid: true,
 		},
 	}
 
@@ -660,7 +660,7 @@ func TestAddGatewaysForBackendTLSPolicies(t *testing.T) {
 		g := NewWithT(t)
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
-			addGatewaysForBackendTLSPolicies(test.backendTLSPolicies, test.services)
+			addGatewaysForBackendTLSPolicies(test.backendTLSPolicies, test.services, "nginx-gateway")
 			g.Expect(helpers.Diff(test.backendTLSPolicies, test.expected)).To(BeEmpty())
 		})
 	}

internal/controller/state/graph/graph.go

@@ -13,6 +13,8 @@ import (
 	"sigs.k8s.io/gateway-api/apis/v1alpha3"
 	"sigs.k8s.io/gateway-api/apis/v1beta1"
 
+	"github.com/go-logr/logr"
+
 	ngfAPIv1alpha1 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1"
 	ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2"
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies"
@@ -198,7 +200,10 @@ func BuildGraph(
 	gcName string,
 	plusSecrets map[types.NamespacedName][]PlusSecretFile,
 	validators validation.Validators,
+	logger logr.Logger,
 ) *Graph {
+	// Set the logger for the graph package
+	SetLogger(logger)
 	processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName)
 	if gcExists && processedGwClasses.Winner == nil {
 		// configured GatewayClass does not reference this controller
@@ -269,7 +274,7 @@ func BuildGraph(
 
 	referencedServices := buildReferencedServices(routes, l4routes, gws)
 
-	addGatewaysForBackendTLSPolicies(processedBackendTLSPolicies, referencedServices)
+	addGatewaysForBackendTLSPolicies(processedBackendTLSPolicies, referencedServices, controllerName)
 
 	// policies must be processed last because they rely on the state of the other resources in the graph
 	processedPolicies := processPolicies(

internal/controller/state/graph/graph_test.go

@@ -3,6 +3,7 @@ package graph
 import (
 	"testing"
 
+	"github.com/go-logr/logr"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/format"
 	v1 "k8s.io/api/core/v1"
@@ -1309,6 +1310,7 @@ func TestBuildGraph(t *testing.T) {
 					GenericValidator:    &validationfakes.FakeGenericValidator{},
 					PolicyValidator:     fakePolicyValidator,
 				},
+				logr.Discard(),
 			)
 
 			g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty())

internal/controller/state/graph/multiple_gateways_test.go

@@ -3,6 +3,7 @@ package graph
 import (
 	"testing"
 
+	"github.com/go-logr/logr"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/format"
 	v1 "k8s.io/api/core/v1"
@@ -398,6 +399,7 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) {
 					GenericValidator:    &validationfakes.FakeGenericValidator{},
 					PolicyValidator:     fakePolicyValidator,
 				},
+				logr.Discard(),
 			)
 
 			g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty())
@@ -886,6 +888,7 @@ func Test_MultipleGateways_WithListeners(t *testing.T) {
 					GenericValidator:    &validationfakes.FakeGenericValidator{},
 					PolicyValidator:     fakePolicyValidator,
 				},
+				logr.Discard(),
 			)
 
 			g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty())