Repository: nginx/nginx-gateway-fabric

Issue:
Align with Gateway API invalid BackendTLSPolicy conventions
The spec was updated to be explicit about how implementations handle validation cases for certs

We need to ensure we align with the spec and update our implementation where required

https://gateway-api.sigs.k8s.io/reference/spec/#backendtlspolicyvalidation


Acceptance Criteria:
- Investigate and implement the required validation for the BackendTLSPolicy. Error messaging should be consistent with the Gateway API conventions spec.
- Ensure the conformance tests pass: https://github.com/kubernetes-sigs/gateway-api/pull/4010 (note: the tests are currently only available in main, they will be released as part of 1.4). See https://github.com/nginx/nginx-gateway-fabric/issues/1412#issuecomment-3108978162 for details on how to run these tests


Discussion:

Solution patch:
diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go
index 9dcf8290..c9909ed2 100644
--- a/internal/controller/state/conditions/conditions.go
+++ b/internal/controller/state/conditions/conditions.go
@@ -147,6 +147,18 @@ const (
 	// 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:"
+
+	// BackendTLSPolicyReasonInvalidCACertificateRef is used with the "ResolvedRefs" condition when a
+	// CACertificateRef refers to a resource that cannot be resolved or is misconfigured.
+	BackendTLSPolicyReasonInvalidCACertificateRef v1alpha2.PolicyConditionReason = "InvalidCACertificateRef"
+
+	// BackendTLSPolicyReasonInvalidKind is used with the "ResolvedRefs" condition when a
+	// CACertificateRef refers to an unknown or unsupported kind of resource.
+	BackendTLSPolicyReasonInvalidKind v1alpha2.PolicyConditionReason = "InvalidKind"
+
+	// BackendTLSPolicyReasonNoValidCACertificate is used with the "Accepted" condition when all
+	// CACertificateRefs are invalid.
+	BackendTLSPolicyReasonNoValidCACertificate v1alpha2.PolicyConditionReason = "NoValidCACertificate"
 )
 
 // Condition defines a condition to be reported in the status of resources.
@@ -1053,3 +1065,47 @@ func NewClientSettingsPolicyAffected() Condition {
 		Message: "ClientSettingsPolicy is applied to the resource",
 	}
 }
+
+// NewBackendTLSPolicyResolvedRefs returns a Condition that indicates that all CACertificateRefs
+// in the BackendTLSPolicy are resolved.
+func NewBackendTLSPolicyResolvedRefs() Condition {
+	return Condition{
+		Type:    string(GatewayResolvedRefs),
+		Status:  metav1.ConditionTrue,
+		Reason:  string(GatewayResolvedRefs),
+		Message: "All CACertificateRefs are resolved",
+	}
+}
+
+// NewBackendTLSPolicyInvalidCACertificateRef returns a Condition that indicates that a
+// CACertificateRef in the BackendTLSPolicy refers to a resource that cannot be resolved or is misconfigured.
+func NewBackendTLSPolicyInvalidCACertificateRef(message string) Condition {
+	return Condition{
+		Type:    string(GatewayResolvedRefs),
+		Status:  metav1.ConditionFalse,
+		Reason:  string(BackendTLSPolicyReasonInvalidCACertificateRef),
+		Message: message,
+	}
+}
+
+// NewBackendTLSPolicyInvalidKind returns a Condition that indicates that a CACertificateRef
+// in the BackendTLSPolicy refers to an unknown or unsupported kind of resource.
+func NewBackendTLSPolicyInvalidKind(message string) Condition {
+	return Condition{
+		Type:    string(GatewayResolvedRefs),
+		Status:  metav1.ConditionFalse,
+		Reason:  string(BackendTLSPolicyReasonInvalidKind),
+		Message: message,
+	}
+}
+
+// NewBackendTLSPolicyNoValidCACertificate returns a Condition that indicates that all
+// CACertificateRefs in the BackendTLSPolicy are invalid.
+func NewBackendTLSPolicyNoValidCACertificate(message string) Condition {
+	return Condition{
+		Type:    string(v1alpha2.PolicyConditionAccepted),
+		Status:  metav1.ConditionFalse,
+		Reason:  string(BackendTLSPolicyReasonNoValidCACertificate),
+		Message: message,
+	}
+}
diff --git a/internal/controller/state/graph/backend_refs.go b/internal/controller/state/graph/backend_refs.go
index 997df80c..d18a81cc 100644
--- a/internal/controller/state/graph/backend_refs.go
+++ b/internal/controller/state/graph/backend_refs.go
@@ -216,6 +216,7 @@ func createBackendRef(
 		ref.Namespace,
 		string(ref.Name),
 		route.Source.GetNamespace(),
+		svcPort,
 	)
 	if err != nil {
 		backendRef := BackendRef{
@@ -307,8 +308,10 @@ func findBackendTLSPolicyForService(
 	refNamespace *gatewayv1.Namespace,
 	refName,
 	routeNamespace string,
+	servicePort v1.ServicePort,
 ) (*BackendTLSPolicy, error) {
 	var beTLSPolicy *BackendTLSPolicy
+	var conflictingPolicies []*BackendTLSPolicy
 	var err error
 
 	refNs := routeNamespace
@@ -316,21 +319,45 @@ func findBackendTLSPolicyForService(
 		refNs = string(*refNamespace)
 	}
 
+	// First pass: find all policies targeting this service and port
 	for _, btp := range backendTLSPolicies {
 		btpNs := btp.Source.Namespace
 		for _, targetRef := range btp.Source.Spec.TargetRefs {
 			if string(targetRef.Name) == refName && btpNs == refNs {
-				if beTLSPolicy != nil {
+				// Check if this policy applies to the specific port we're interested in
+				if targetRef.SectionName != nil {
+					// Policy targets a specific port by name
+					if servicePort.Name != string(*targetRef.SectionName) {
+						// This policy targets a different port, skip it
+						continue
+					}
+				}
+				// Policy applies to all ports (no sectionName) or matches our port
+
+				if beTLSPolicy == nil {
+					beTLSPolicy = btp
+				} else {
+					// Found a conflict - determine which policy wins
 					if sort.LessClientObject(btp.Source, beTLSPolicy.Source) {
+						// btp wins, beTLSPolicy loses
+						conflictingPolicies = append(conflictingPolicies, beTLSPolicy)
 						beTLSPolicy = btp
+					} else {
+						// beTLSPolicy wins, btp loses
+						conflictingPolicies = append(conflictingPolicies, btp)
 					}
-				} else {
-					beTLSPolicy = btp
 				}
 			}
 		}
 	}
 
+	// Set conflicted conditions on losing policies
+	for _, conflictedPolicy := range conflictingPolicies {
+		conflictedPolicy.IsReferenced = true
+		conflictedPolicy.Conditions = append(conflictedPolicy.Conditions,
+			conditions.NewPolicyConflicted("Conflicts with another BackendTLSPolicy targeting the same Service"))
+	}
+
 	if beTLSPolicy != nil {
 		beTLSPolicy.IsReferenced = true
 		if !beTLSPolicy.Valid {
diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go
index 11a77c87..591f83d6 100644
--- a/internal/controller/state/graph/backend_tls_policy.go
+++ b/internal/controller/state/graph/backend_tls_policy.go
@@ -81,6 +81,8 @@ func validateBackendTLSPolicy(
 
 	caCertRefs := backendTLSPolicy.Spec.Validation.CACertificateRefs
 	wellKnownCerts := backendTLSPolicy.Spec.Validation.WellKnownCACertificates
+
+	// Check mutual exclusivity
 	switch {
 	case len(caCertRefs) > 0 && wellKnownCerts != nil:
 		valid = false
@@ -88,10 +90,13 @@ func validateBackendTLSPolicy(
 		conds = append(conds, conditions.NewPolicyInvalid(msg))
 
 	case len(caCertRefs) > 0:
-		if err := validateBackendTLSCACertRef(backendTLSPolicy, configMapResolver, secretResolver); err != nil {
+		certConds := validateBackendTLSCACertRef(backendTLSPolicy, configMapResolver, secretResolver)
+		if len(certConds) > 0 {
 			valid = false
-			conds = append(conds, conditions.NewPolicyInvalid(
-				fmt.Sprintf("invalid CACertificateRef: %s", err.Error())))
+			conds = append(conds, certConds...)
+		} else if valid {
+			// Only set ResolvedRefs to true if CACertificateRefs are valid AND overall policy is valid
+			conds = append(conds, conditions.NewBackendTLSPolicyResolvedRefs())
 		}
 
 	case wellKnownCerts != nil:
@@ -103,8 +108,12 @@ func validateBackendTLSPolicy(
 
 	default:
 		valid = false
-		conds = append(conds, conditions.NewPolicyInvalid("CACertRefs and WellKnownCACerts are both nil"))
+		conds = append(
+			conds,
+			conditions.NewPolicyInvalid("either CACertificateRefs or WellKnownCACertificates must be specified"),
+		)
 	}
+
 	return valid, ignored, conds
 }
 
@@ -123,11 +132,11 @@ func validateBackendTLSCACertRef(
 	btp *v1alpha3.BackendTLSPolicy,
 	configMapResolver *configMapResolver,
 	secretResolver *secretResolver,
-) error {
+) []conditions.Condition {
 	if len(btp.Spec.Validation.CACertificateRefs) != 1 {
 		path := field.NewPath("validation.caCertificateRefs")
 		valErr := field.TooMany(path, len(btp.Spec.Validation.CACertificateRefs), 1)
-		return valErr
+		return []conditions.Condition{conditions.NewPolicyInvalid(valErr.Error())}
 	}
 
 	selectedCertRef := btp.Spec.Validation.CACertificateRefs[0]
@@ -136,13 +145,19 @@ func validateBackendTLSCACertRef(
 	if !slices.Contains(allowedCaCertKinds, selectedCertRef.Kind) {
 		path := field.NewPath("validation.caCertificateRefs[0].kind")
 		valErr := field.NotSupported(path, btp.Spec.Validation.CACertificateRefs[0].Kind, allowedCaCertKinds)
-		return valErr
+		return []conditions.Condition{
+			conditions.NewBackendTLSPolicyInvalidKind(valErr.Error()),
+			conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
+		}
 	}
 	if selectedCertRef.Group != "" &&
 		selectedCertRef.Group != "core" {
 		path := field.NewPath("validation.caCertificateRefs[0].group")
 		valErr := field.NotSupported(path, selectedCertRef.Group, []string{"", "core"})
-		return valErr
+		return []conditions.Condition{
+			conditions.NewBackendTLSPolicyInvalidKind(valErr.Error()),
+			conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
+		}
 	}
 	nsName := types.NamespacedName{
 		Namespace: btp.Namespace,
@@ -153,15 +168,21 @@ func validateBackendTLSCACertRef(
 	case "ConfigMap":
 		if err := configMapResolver.resolve(nsName); err != nil {
 			path := field.NewPath("validation.caCertificateRefs[0]")
-			return field.Invalid(path, selectedCertRef, err.Error())
+			valErr := field.Invalid(path, selectedCertRef, err.Error())
+			return []conditions.Condition{
+				conditions.NewBackendTLSPolicyInvalidCACertificateRef(valErr.Error()),
+				conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
+			}
 		}
 	case "Secret":
 		if err := secretResolver.resolve(nsName); err != nil {
 			path := field.NewPath("validation.caCertificateRefs[0]")
-			return field.Invalid(path, selectedCertRef, err.Error())
+			valErr := field.Invalid(path, selectedCertRef, err.Error())
+			return []conditions.Condition{
+				conditions.NewBackendTLSPolicyInvalidCACertificateRef(valErr.Error()),
+				conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
+			}
 		}
-	default:
-		return fmt.Errorf("invalid certificate reference kind %q", selectedCertRef.Kind)
 	}
 	return nil
 }



Test patch:
diff --git a/internal/controller/state/graph/backend_refs_test.go b/internal/controller/state/graph/backend_refs_test.go
index 0d3e74a1..2af666b6 100644
--- a/internal/controller/state/graph/backend_refs_test.go
+++ b/internal/controller/state/graph/backend_refs_test.go
@@ -1707,7 +1707,13 @@ func TestFindBackendTLSPolicyForService(t *testing.T) {
 			t.Parallel()
 			g := NewWithT(t)
 
-			btp, err := findBackendTLSPolicyForService(test.backendTLSPolicies, ref.Namespace, string(ref.Name), "test")
+			btp, err := findBackendTLSPolicyForService(
+				test.backendTLSPolicies,
+				ref.Namespace,
+				string(ref.Name),
+				"test",
+				v1.ServicePort{Name: "https", Port: 443},
+			)
 
 			g.Expect(btp.Source.Name).To(Equal(test.expectedBtpName))
 			g.Expect(err).ToNot(HaveOccurred())
diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go
index 20567860..53e9011e 100644
--- a/internal/controller/state/graph/backend_tls_policy_test.go
+++ b/internal/controller/state/graph/backend_tls_policy_test.go
@@ -479,13 +479,59 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
 
 			valid, ignored, conds := validateBackendTLSPolicy(test.tlsPolicy, configMapResolver, secretMapResolver)
 
-			if !test.isValid && !test.ignored {
-				g.Expect(conds).To(HaveLen(1))
-			} else {
-				g.Expect(conds).To(BeEmpty())
-			}
 			g.Expect(valid).To(Equal(test.isValid))
 			g.Expect(ignored).To(Equal(test.ignored))
+
+			if test.isValid && !test.ignored {
+				// Valid cases should have ResolvedRefs condition set to true
+				if len(test.tlsPolicy.Spec.Validation.CACertificateRefs) > 0 {
+					g.Expect(conds).To(HaveLen(1))
+					g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
+					g.Expect(conds[0].Status).To(Equal(metav1.ConditionTrue))
+				} else {
+					// WellKnownCACertificates case - no specific condition expected
+					g.Expect(conds).To(BeEmpty())
+				}
+			} else if !test.isValid && !test.ignored {
+				// Invalid cases should have at least one condition
+				g.Expect(conds).ToNot(BeEmpty())
+
+				// Check specific condition types based on the test case
+				switch test.name {
+				case "invalid ca cert ref kind":
+					// Should have InvalidKind condition and NoValidCACertificate condition
+					g.Expect(conds).To(HaveLen(2))
+					g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
+					g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
+					g.Expect(conds[0].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonInvalidKind)))
+					g.Expect(conds[1].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
+					g.Expect(conds[1].Status).To(Equal(metav1.ConditionFalse))
+					g.Expect(conds[1].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonNoValidCACertificate)))
+				case "invalid ca cert ref name":
+					// Should have InvalidCACertificateRef condition and NoValidCACertificate condition
+					g.Expect(conds).To(HaveLen(2))
+					g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
+					g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
+					g.Expect(conds[0].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonInvalidCACertificateRef)))
+					g.Expect(conds[1].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
+					g.Expect(conds[1].Status).To(Equal(metav1.ConditionFalse))
+					g.Expect(conds[1].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonNoValidCACertificate)))
+				case "invalid ca cert ref group":
+					// Should have InvalidKind condition and NoValidCACertificate condition
+					g.Expect(conds).To(HaveLen(2))
+					g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
+					g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
+					g.Expect(conds[0].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonInvalidKind)))
+					g.Expect(conds[1].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
+					g.Expect(conds[1].Status).To(Equal(metav1.ConditionFalse))
+					g.Expect(conds[1].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonNoValidCACertificate)))
+				default:
+					// Other invalid cases should have generic PolicyInvalid condition
+					g.Expect(conds).To(HaveLen(1))
+					g.Expect(conds[0].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
+					g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
+				}
+			}
 		})
 	}
 }
diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go
index 46802f33..ac5cfff3 100644
--- a/internal/controller/state/graph/graph_test.go
+++ b/internal/controller/state/graph/graph_test.go
@@ -50,6 +50,7 @@ func TestBuildGraph(t *testing.T) {
 	}
 
 	btpAcceptedConds := []conditions.Condition{
+		conditions.NewBackendTLSPolicyResolvedRefs(),
 		conditions.NewPolicyAccepted(),
 		conditions.NewPolicyAccepted(),
 		conditions.NewPolicyAccepted(),