Fix policy attachment when ancestors slice is full (#3698)

Problem: Multiple bugs when ancestor slice was full:
BackendTLSPolicy became invalid
NGF Policies would not partially attach policies to services when multiple ancestor refs were created from a single service but attach to the whole service
No condition was added to ancestor
No logs on controller

Solution: Added conditions to ancestor and logs to controller. I fixed BackendTLSPolicy to add attachments until ancestors became full and fixed NGF policies as well

Author: sarthyparty

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

@@ -138,6 +138,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 ancestor limit being reached.
+	PolicyMessageAncestorLimitReached = "Policies cannot be applied because the ancestor status list " +
+		"has reached the maximum size. The following policies have been ignored:"
 )
 
 // Condition defines a condition to be reported in the status of resources.
@@ -968,6 +977,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(policyType string, policyName string) Condition {
+	return Condition{
+		Type:    string(v1alpha2.PolicyConditionAccepted),
+		Status:  metav1.ConditionFalse,
+		Reason:  string(PolicyReasonAncestorLimitReached),
+		Message: fmt.Sprintf("%s %s %s", PolicyMessageAncestorLimitReached, policyType, policyName),
+	}
+}
+
 // 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/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

@@ -3,7 +3,9 @@ package graph
 import (
 	"fmt"
 	"slices"
+	"strings"
 
+	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	v1 "sigs.k8s.io/gateway-api/apis/v1"
@@ -18,7 +20,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
@@ -34,7 +37,6 @@ func processBackendTLSPolicies(
 	backendTLSPolicies map[types.NamespacedName]*v1alpha3.BackendTLSPolicy,
 	configMapResolver *configMapResolver,
 	secretResolver *secretResolver,
-	ctlrName string,
 	gateways map[types.NamespacedName]*Gateway,
 ) map[types.NamespacedName]*BackendTLSPolicy {
 	if len(backendTLSPolicies) == 0 || len(gateways) == 0 {
@@ -45,7 +47,7 @@ func processBackendTLSPolicies(
 	for nsname, backendTLSPolicy := range backendTLSPolicies {
 		var caCertRef types.NamespacedName
 
-		valid, ignored, conds := validateBackendTLSPolicy(backendTLSPolicy, configMapResolver, secretResolver, ctlrName)
+		valid, ignored, conds := validateBackendTLSPolicy(backendTLSPolicy, configMapResolver, secretResolver)
 
 		if valid && !ignored && backendTLSPolicy.Spec.Validation.CACertificateRefs != nil {
 			caCertRef = types.NamespacedName{
@@ -68,17 +70,10 @@ func validateBackendTLSPolicy(
 	backendTLSPolicy *v1alpha3.BackendTLSPolicy,
 	configMapResolver *configMapResolver,
 	secretResolver *secretResolver,
-	ctlrName 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
-	}
-
 	if err := validateBackendTLSHostname(backendTLSPolicy); err != nil {
 		valid = false
 		conds = append(conds, conditions.NewPolicyInvalid(fmt.Sprintf("invalid hostname: %s", err.Error())))
@@ -183,31 +178,148 @@ func validateBackendTLSWellKnownCACerts(btp *v1alpha3.BackendTLSPolicy) error {
 	return nil
 }
 
+// countNonNGFAncestors counts the number of non-NGF ancestors in policy status.
+func countNonNGFAncestors(policy *v1alpha3.BackendTLSPolicy, ctlrName string) int {
+	nonNGFCount := 0
+	for _, ancestor := range policy.Status.Ancestors {
+		if string(ancestor.ControllerName) != ctlrName {
+			nonNGFCount++
+		}
+	}
+	return nonNGFCount
+}
+
+// addPolicyAncestorLimitCondition adds or updates a PolicyAncestorLimitReached condition.
+func addPolicyAncestorLimitCondition(
+	conds []conditions.Condition,
+	policyName string,
+	policyType string,
+) []conditions.Condition {
+	for i, condition := range conds {
+		if condition.Reason == string(conditions.PolicyReasonAncestorLimitReached) {
+			if !strings.Contains(condition.Message, policyName) {
+				conds[i].Message = fmt.Sprintf("%s, %s %s", condition.Message, policyType, policyName)
+			}
+			return conds
+		}
+	}
+
+	newCondition := conditions.NewPolicyAncestorLimitReached(policyType, policyName)
+	return append(conds, newCondition)
+}
+
+// collectOrderedGateways collects gateways in spec order (services) then creation time order (gateways within service).
+func collectOrderedGateways(
+	policy *v1alpha3.BackendTLSPolicy,
+	services map[types.NamespacedName]*ReferencedService,
+	gateways map[types.NamespacedName]*Gateway,
+	existingNGFGatewayAncestors map[types.NamespacedName]struct{},
+) []types.NamespacedName {
+	seenGateways := make(map[types.NamespacedName]struct{})
+	existingGateways := make([]types.NamespacedName, 0)
+	newGateways := make([]types.NamespacedName, 0)
+
+	// Process services in spec order to maintain deterministic gateway ordering
+	for _, refs := range policy.Spec.TargetRefs {
+		if refs.Kind != kinds.Service {
+			continue
+		}
+
+		svcNsName := types.NamespacedName{
+			Namespace: policy.Namespace,
+			Name:      string(refs.Name),
+		}
+
+		referencedService, exists := services[svcNsName]
+		if !exists {
+			continue
+		}
+
+		// Add to ordered lists, categorizing existing vs new, skipping duplicates
+		for gateway := range referencedService.GatewayNsNames {
+			if _, seen := seenGateways[gateway]; seen {
+				continue
+			}
+			seenGateways[gateway] = struct{}{}
+			if _, exists := existingNGFGatewayAncestors[gateway]; exists {
+				existingGateways = append(existingGateways, gateway)
+			} else {
+				newGateways = append(newGateways, gateway)
+			}
+		}
+	}
+
+	sortGatewaysByCreationTime(existingGateways, gateways)
+	sortGatewaysByCreationTime(newGateways, gateways)
+
+	return append(existingGateways, newGateways...)
+}
+
+func extractExistingNGFGatewayAncestors(
+	backendTLSPolicy *v1alpha3.BackendTLSPolicy,
+	ctlrName string,
+) map[types.NamespacedName]struct{} {
+	existingNGFGatewayAncestors := make(map[types.NamespacedName]struct{})
+
+	for _, ancestor := range backendTLSPolicy.Status.Ancestors {
+		if string(ancestor.ControllerName) != ctlrName {
+			continue
+		}
+
+		if ancestor.AncestorRef.Kind != nil && *ancestor.AncestorRef.Kind == v1.Kind(kinds.Gateway) &&
+			ancestor.AncestorRef.Namespace != nil {
+			gatewayNsName := types.NamespacedName{
+				Namespace: string(*ancestor.AncestorRef.Namespace),
+				Name:      string(ancestor.AncestorRef.Name),
+			}
+			existingNGFGatewayAncestors[gatewayNsName] = struct{}{}
+		}
+	}
+
+	return existingNGFGatewayAncestors
+}
+
 func addGatewaysForBackendTLSPolicies(
 	backendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy,
 	services map[types.NamespacedName]*ReferencedService,
+	ctlrName string,
+	gateways map[types.NamespacedName]*Gateway,
+	logger logr.Logger,
 ) {
 	for _, backendTLSPolicy := range backendTLSPolicies {
-		gateways := make(map[types.NamespacedName]struct{})
+		existingNGFGatewayAncestors := extractExistingNGFGatewayAncestors(backendTLSPolicy.Source, ctlrName)
+		orderedGateways := collectOrderedGateways(
+			backendTLSPolicy.Source,
+			services,
+			gateways,
+			existingNGFGatewayAncestors,
+		)
 
-		for _, refs := range backendTLSPolicy.Source.Spec.TargetRefs {
-			if refs.Kind != kinds.Service {
-				continue
-			}
+		ancestorCount := countNonNGFAncestors(backendTLSPolicy.Source, ctlrName)
 
-			for svcNsName, referencedServices := range services {
-				if svcNsName.Name != string(refs.Name) {
-					continue
-				}
+		// Process each gateway, respecting ancestor limits
+		for _, gatewayNsName := range orderedGateways {
+			// Check if adding this gateway would exceed the ancestor limit
+			if ancestorCount >= maxAncestors {
+				policyName := backendTLSPolicy.Source.Namespace + "/" + backendTLSPolicy.Source.Name
+				proposedAncestor := createParentReference(v1.GroupName, kinds.Gateway, gatewayNsName)
+				gatewayName := getAncestorName(proposedAncestor)
 
-				for gateway := range referencedServices.GatewayNsNames {
-					gateways[gateway] = struct{}{}
+				if gateway, ok := gateways[gatewayNsName]; ok {
+					gateway.Conditions = addPolicyAncestorLimitCondition(gateway.Conditions, policyName, kinds.BackendTLSPolicy)
+				} else {
+					// This should never happen, but we'll log it if it does
+					logger.Error(fmt.Errorf("gateway not found in the graph"),
+						"Gateway not found in the graph", "policy", policyName, "ancestor", gatewayName)
 				}
+
+				logAncestorLimitReached(logger, policyName, "BackendTLSPolicy", gatewayName)
+				continue
 			}
-		}
 
-		for gateway := range gateways {
-			backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gateway)
+			ancestorCount++
+
+			backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gatewayNsName)
 		}
 	}
 }