Align with BackendTLSPolicy validation (#3871)

Author: ciarams87

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,
+	}
+}

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,30 +308,56 @@ func findBackendTLSPolicyForService(
 	refNamespace *gatewayv1.Namespace,
 	refName,
 	routeNamespace string,
+	servicePort v1.ServicePort,
 ) (*BackendTLSPolicy, error) {
 	var beTLSPolicy *BackendTLSPolicy
+	var conflictingPolicies []*BackendTLSPolicy
 	var err error
 
 	refNs := routeNamespace
 	if refNamespace != nil {
 		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 {

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())

internal/controller/state/graph/backend_tls_policy.go

@@ -81,17 +81,22 @@ func validateBackendTLSPolicy(
 
 	caCertRefs := backendTLSPolicy.Spec.Validation.CACertificateRefs
 	wellKnownCerts := backendTLSPolicy.Spec.Validation.WellKnownCACertificates
+
+	// Check mutual exclusivity
 	switch {
 	case len(caCertRefs) > 0 && wellKnownCerts != nil:
 		valid = false
 		msg := "CACertificateRefs and WellKnownCACertificates are mutually exclusive"
 		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
 }

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))
+				}
+			}
 		})
 	}
 }

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(),