diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index 9dcf829020..c9909ed271 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 997df80c75..d18a81cc43 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_refs_test.go b/internal/controller/state/graph/backend_refs_test.go index 0d3e74a1dd..2af666b6e4 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.go b/internal/controller/state/graph/backend_tls_policy.go index 11a77c87c2..591f83d6d3 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 } diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index 20567860b7..53e9011edb 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 46802f3391..ac5cfff3a2 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(),