Skip to content

Commit b8faab1

Browse files
ciarams87ciarams87
committed
Align with BackendTLSPolicy validation
1 parent 2993d57 commit b8faab1

File tree

6 files changed

+178
-21
lines changed

6 files changed

+178
-21
lines changed

‎internal/controller/state/conditions/conditions.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,18 @@ const (
147147
// when a policy cannot be applied due to the ancestor limit being reached.
148148
PolicyMessageAncestorLimitReached = "Policies cannot be applied because the ancestor status list " +
149149
"has reached the maximum size. The following policies have been ignored:"
150+
151+
// BackendTLSPolicyReasonInvalidCACertificateRef is used with the "ResolvedRefs" condition when a
152+
// CACertificateRef refers to a resource that cannot be resolved or is misconfigured.
153+
BackendTLSPolicyReasonInvalidCACertificateRef v1alpha2.PolicyConditionReason = "InvalidCACertificateRef"
154+
155+
// BackendTLSPolicyReasonInvalidKind is used with the "ResolvedRefs" condition when a
156+
// CACertificateRef refers to an unknown or unsupported kind of resource.
157+
BackendTLSPolicyReasonInvalidKind v1alpha2.PolicyConditionReason = "InvalidKind"
158+
159+
// BackendTLSPolicyReasonNoValidCACertificate is used with the "Accepted" condition when all
160+
// CACertificateRefs are invalid.
161+
BackendTLSPolicyReasonNoValidCACertificate v1alpha2.PolicyConditionReason = "NoValidCACertificate"
150162
)
151163

152164
// Condition defines a condition to be reported in the status of resources.
@@ -1053,3 +1065,47 @@ func NewClientSettingsPolicyAffected() Condition {
10531065
Message: "ClientSettingsPolicy is applied to the resource",
10541066
}
10551067
}
1068+
1069+
// NewBackendTLSPolicyResolvedRefs returns a Condition that indicates that all CACertificateRefs
1070+
// in the BackendTLSPolicy are resolved.
1071+
func NewBackendTLSPolicyResolvedRefs() Condition {
1072+
return Condition{
1073+
Type: string(GatewayResolvedRefs),
1074+
Status: metav1.ConditionTrue,
1075+
Reason: string(GatewayResolvedRefs),
1076+
Message: "All CACertificateRefs are resolved",
1077+
}
1078+
}
1079+
1080+
// NewBackendTLSPolicyInvalidCACertificateRef returns a Condition that indicates that a
1081+
// CACertificateRef in the BackendTLSPolicy refers to a resource that cannot be resolved or is misconfigured.
1082+
func NewBackendTLSPolicyInvalidCACertificateRef(message string) Condition {
1083+
return Condition{
1084+
Type: string(GatewayResolvedRefs),
1085+
Status: metav1.ConditionFalse,
1086+
Reason: string(BackendTLSPolicyReasonInvalidCACertificateRef),
1087+
Message: message,
1088+
}
1089+
}
1090+
1091+
// NewBackendTLSPolicyInvalidKind returns a Condition that indicates that a CACertificateRef
1092+
// in the BackendTLSPolicy refers to an unknown or unsupported kind of resource.
1093+
func NewBackendTLSPolicyInvalidKind(message string) Condition {
1094+
return Condition{
1095+
Type: string(GatewayResolvedRefs),
1096+
Status: metav1.ConditionFalse,
1097+
Reason: string(BackendTLSPolicyReasonInvalidKind),
1098+
Message: message,
1099+
}
1100+
}
1101+
1102+
// NewBackendTLSPolicyNoValidCACertificate returns a Condition that indicates that all
1103+
// CACertificateRefs in the BackendTLSPolicy are invalid.
1104+
func NewBackendTLSPolicyNoValidCACertificate(message string) Condition {
1105+
return Condition{
1106+
Type: string(v1alpha2.PolicyConditionAccepted),
1107+
Status: metav1.ConditionFalse,
1108+
Reason: string(BackendTLSPolicyReasonNoValidCACertificate),
1109+
Message: message,
1110+
}
1111+
}

‎internal/controller/state/graph/backend_refs.go‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,7 @@ func createBackendRef(
216216
ref.Namespace,
217217
string(ref.Name),
218218
route.Source.GetNamespace(),
219+
svcPort,
219220
)
220221
if err != nil {
221222
backendRef := BackendRef{
@@ -307,30 +308,56 @@ func findBackendTLSPolicyForService(
307308
refNamespace *gatewayv1.Namespace,
308309
refName,
309310
routeNamespace string,
311+
servicePort v1.ServicePort,
310312
) (*BackendTLSPolicy, error) {
311313
var beTLSPolicy *BackendTLSPolicy
314+
var conflictingPolicies []*BackendTLSPolicy
312315
var err error
313316

314317
refNs := routeNamespace
315318
if refNamespace != nil {
316319
refNs = string(*refNamespace)
317320
}
318321

322+
// First pass: find all policies targeting this service and port
319323
for _, btp := range backendTLSPolicies {
320324
btpNs := btp.Source.Namespace
321325
for _, targetRef := range btp.Source.Spec.TargetRefs {
322326
if string(targetRef.Name) == refName && btpNs == refNs {
323-
if beTLSPolicy != nil {
327+
// Check if this policy applies to the specific port we're interested in
328+
if targetRef.SectionName != nil {
329+
// Policy targets a specific port by name
330+
if servicePort.Name != string(*targetRef.SectionName) {
331+
// This policy targets a different port, skip it
332+
continue
333+
}
334+
}
335+
// Policy applies to all ports (no sectionName) or matches our port
336+
337+
if beTLSPolicy == nil {
338+
beTLSPolicy = btp
339+
} else {
340+
// Found a conflict - determine which policy wins
324341
if sort.LessClientObject(btp.Source, beTLSPolicy.Source) {
342+
// btp wins, beTLSPolicy loses
343+
conflictingPolicies = append(conflictingPolicies, beTLSPolicy)
325344
beTLSPolicy = btp
345+
} else {
346+
// beTLSPolicy wins, btp loses
347+
conflictingPolicies = append(conflictingPolicies, btp)
326348
}
327-
} else {
328-
beTLSPolicy = btp
329349
}
330350
}
331351
}
332352
}
333353

354+
// Set conflicted conditions on losing policies
355+
for _, conflictedPolicy := range conflictingPolicies {
356+
conflictedPolicy.IsReferenced = true
357+
conflictedPolicy.Conditions = append(conflictedPolicy.Conditions,
358+
conditions.NewPolicyConflicted("Conflicts with another BackendTLSPolicy targeting the same Service"))
359+
}
360+
334361
if beTLSPolicy != nil {
335362
beTLSPolicy.IsReferenced = true
336363
if !beTLSPolicy.Valid {

‎internal/controller/state/graph/backend_refs_test.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1707,7 +1707,13 @@ func TestFindBackendTLSPolicyForService(t *testing.T) {
17071707
t.Parallel()
17081708
g := NewWithT(t)
17091709

1710-
btp, err := findBackendTLSPolicyForService(test.backendTLSPolicies, ref.Namespace, string(ref.Name), "test")
1710+
btp, err := findBackendTLSPolicyForService(
1711+
test.backendTLSPolicies,
1712+
ref.Namespace,
1713+
string(ref.Name),
1714+
"test",
1715+
v1.ServicePort{Name: "https", Port: 443},
1716+
)
17111717

17121718
g.Expect(btp.Source.Name).To(Equal(test.expectedBtpName))
17131719
g.Expect(err).ToNot(HaveOccurred())

‎internal/controller/state/graph/backend_tls_policy.go‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,17 +81,22 @@ func validateBackendTLSPolicy(
8181

8282
caCertRefs := backendTLSPolicy.Spec.Validation.CACertificateRefs
8383
wellKnownCerts := backendTLSPolicy.Spec.Validation.WellKnownCACertificates
84+
85+
// Check mutual exclusivity
8486
switch {
8587
case len(caCertRefs) > 0 && wellKnownCerts != nil:
8688
valid = false
8789
msg := "CACertificateRefs and WellKnownCACertificates are mutually exclusive"
8890
conds = append(conds, conditions.NewPolicyInvalid(msg))
8991

9092
case len(caCertRefs) > 0:
91-
if err := validateBackendTLSCACertRef(backendTLSPolicy, configMapResolver, secretResolver); err != nil {
93+
certConds := validateBackendTLSCACertRef(backendTLSPolicy, configMapResolver, secretResolver)
94+
if len(certConds) > 0 {
9295
valid = false
93-
conds = append(conds, conditions.NewPolicyInvalid(
94-
fmt.Sprintf("invalid CACertificateRef: %s", err.Error())))
96+
conds = append(conds, certConds...)
97+
} else if valid {
98+
// Only set ResolvedRefs to true if CACertificateRefs are valid AND overall policy is valid
99+
conds = append(conds, conditions.NewBackendTLSPolicyResolvedRefs())
95100
}
96101

97102
case wellKnownCerts != nil:
@@ -103,8 +108,12 @@ func validateBackendTLSPolicy(
103108

104109
default:
105110
valid = false
106-
conds = append(conds, conditions.NewPolicyInvalid("CACertRefs and WellKnownCACerts are both nil"))
111+
conds = append(
112+
conds,
113+
conditions.NewPolicyInvalid("either CACertificateRefs or WellKnownCACertificates must be specified"),
114+
)
107115
}
116+
108117
return valid, ignored, conds
109118
}
110119

@@ -123,11 +132,11 @@ func validateBackendTLSCACertRef(
123132
btp *v1alpha3.BackendTLSPolicy,
124133
configMapResolver *configMapResolver,
125134
secretResolver *secretResolver,
126-
) error {
135+
) []conditions.Condition {
127136
if len(btp.Spec.Validation.CACertificateRefs) != 1 {
128137
path := field.NewPath("validation.caCertificateRefs")
129138
valErr := field.TooMany(path, len(btp.Spec.Validation.CACertificateRefs), 1)
130-
return valErr
139+
return []conditions.Condition{conditions.NewPolicyInvalid(valErr.Error())}
131140
}
132141

133142
selectedCertRef := btp.Spec.Validation.CACertificateRefs[0]
@@ -136,13 +145,19 @@ func validateBackendTLSCACertRef(
136145
if !slices.Contains(allowedCaCertKinds, selectedCertRef.Kind) {
137146
path := field.NewPath("validation.caCertificateRefs[0].kind")
138147
valErr := field.NotSupported(path, btp.Spec.Validation.CACertificateRefs[0].Kind, allowedCaCertKinds)
139-
return valErr
148+
return []conditions.Condition{
149+
conditions.NewBackendTLSPolicyInvalidKind(valErr.Error()),
150+
conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
151+
}
140152
}
141153
if selectedCertRef.Group != "" &&
142154
selectedCertRef.Group != "core" {
143155
path := field.NewPath("validation.caCertificateRefs[0].group")
144156
valErr := field.NotSupported(path, selectedCertRef.Group, []string{"", "core"})
145-
return valErr
157+
return []conditions.Condition{
158+
conditions.NewBackendTLSPolicyInvalidKind(valErr.Error()),
159+
conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
160+
}
146161
}
147162
nsName := types.NamespacedName{
148163
Namespace: btp.Namespace,
@@ -153,15 +168,21 @@ func validateBackendTLSCACertRef(
153168
case "ConfigMap":
154169
if err := configMapResolver.resolve(nsName); err != nil {
155170
path := field.NewPath("validation.caCertificateRefs[0]")
156-
return field.Invalid(path, selectedCertRef, err.Error())
171+
valErr := field.Invalid(path, selectedCertRef, err.Error())
172+
return []conditions.Condition{
173+
conditions.NewBackendTLSPolicyInvalidCACertificateRef(valErr.Error()),
174+
conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
175+
}
157176
}
158177
case "Secret":
159178
if err := secretResolver.resolve(nsName); err != nil {
160179
path := field.NewPath("validation.caCertificateRefs[0]")
161-
return field.Invalid(path, selectedCertRef, err.Error())
180+
valErr := field.Invalid(path, selectedCertRef, err.Error())
181+
return []conditions.Condition{
182+
conditions.NewBackendTLSPolicyInvalidCACertificateRef(valErr.Error()),
183+
conditions.NewBackendTLSPolicyNoValidCACertificate("No valid CACertificateRef found"),
184+
}
162185
}
163-
default:
164-
return fmt.Errorf("invalid certificate reference kind %q", selectedCertRef.Kind)
165186
}
166187
return nil
167188
}

‎internal/controller/state/graph/backend_tls_policy_test.go‎

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -479,13 +479,59 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
479479

480480
valid, ignored, conds := validateBackendTLSPolicy(test.tlsPolicy, configMapResolver, secretMapResolver)
481481

482-
if !test.isValid && !test.ignored {
483-
g.Expect(conds).To(HaveLen(1))
484-
} else {
485-
g.Expect(conds).To(BeEmpty())
486-
}
487482
g.Expect(valid).To(Equal(test.isValid))
488483
g.Expect(ignored).To(Equal(test.ignored))
484+
485+
if test.isValid && !test.ignored {
486+
// Valid cases should have ResolvedRefs condition set to true
487+
if len(test.tlsPolicy.Spec.Validation.CACertificateRefs) > 0 {
488+
g.Expect(conds).To(HaveLen(1))
489+
g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
490+
g.Expect(conds[0].Status).To(Equal(metav1.ConditionTrue))
491+
} else {
492+
// WellKnownCACertificates case - no specific condition expected
493+
g.Expect(conds).To(BeEmpty())
494+
}
495+
} else if !test.isValid && !test.ignored {
496+
// Invalid cases should have at least one condition
497+
g.Expect(conds).ToNot(BeEmpty())
498+
499+
// Check specific condition types based on the test case
500+
switch test.name {
501+
case "invalid ca cert ref kind":
502+
// Should have InvalidKind condition and NoValidCACertificate condition
503+
g.Expect(conds).To(HaveLen(2))
504+
g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
505+
g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
506+
g.Expect(conds[0].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonInvalidKind)))
507+
g.Expect(conds[1].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
508+
g.Expect(conds[1].Status).To(Equal(metav1.ConditionFalse))
509+
g.Expect(conds[1].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonNoValidCACertificate)))
510+
case "invalid ca cert ref name":
511+
// Should have InvalidCACertificateRef condition and NoValidCACertificate condition
512+
g.Expect(conds).To(HaveLen(2))
513+
g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
514+
g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
515+
g.Expect(conds[0].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonInvalidCACertificateRef)))
516+
g.Expect(conds[1].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
517+
g.Expect(conds[1].Status).To(Equal(metav1.ConditionFalse))
518+
g.Expect(conds[1].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonNoValidCACertificate)))
519+
case "invalid ca cert ref group":
520+
// Should have InvalidKind condition and NoValidCACertificate condition
521+
g.Expect(conds).To(HaveLen(2))
522+
g.Expect(conds[0].Type).To(Equal(string(conditions.GatewayResolvedRefs)))
523+
g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
524+
g.Expect(conds[0].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonInvalidKind)))
525+
g.Expect(conds[1].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
526+
g.Expect(conds[1].Status).To(Equal(metav1.ConditionFalse))
527+
g.Expect(conds[1].Reason).To(Equal(string(conditions.BackendTLSPolicyReasonNoValidCACertificate)))
528+
default:
529+
// Other invalid cases should have generic PolicyInvalid condition
530+
g.Expect(conds).To(HaveLen(1))
531+
g.Expect(conds[0].Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
532+
g.Expect(conds[0].Status).To(Equal(metav1.ConditionFalse))
533+
}
534+
}
489535
})
490536
}
491537
}

‎internal/controller/state/graph/graph_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ func TestBuildGraph(t *testing.T) {
5050
}
5151

5252
btpAcceptedConds := []conditions.Condition{
53+
conditions.NewBackendTLSPolicyResolvedRefs(),
5354
conditions.NewPolicyAccepted(),
5455
conditions.NewPolicyAccepted(),
5556
conditions.NewPolicyAccepted(),

0 commit comments

Comments
 (0)