Skip to content

Commit f1290da

Browse files
author
Kate Osborn
committed
Simplify ancestorsFull
1 parent 64e1f99 commit f1290da

File tree

5 files changed

+17
-219
lines changed

5 files changed

+17
-219
lines changed

‎internal/mode/static/state/graph/backend_tls_policy.go

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,13 @@
11
package graph
22

33
import (
4-
"errors"
54
"fmt"
65

76
"k8s.io/apimachinery/pkg/types"
87
"k8s.io/apimachinery/pkg/util/validation/field"
9-
"sigs.k8s.io/controller-runtime/pkg/client"
10-
v1 "sigs.k8s.io/gateway-api/apis/v1"
118
"sigs.k8s.io/gateway-api/apis/v1alpha3"
129

1310
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
14-
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
1511
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
1612
)
1713

@@ -45,12 +41,8 @@ func processBackendTLSPolicies(
4541
processedBackendTLSPolicies := make(map[types.NamespacedName]*BackendTLSPolicy, len(backendTLSPolicies))
4642
for nsname, backendTLSPolicy := range backendTLSPolicies {
4743
var caCertRef types.NamespacedName
48-
valid, ignored, conds := validateBackendTLSPolicy(
49-
backendTLSPolicy,
50-
configMapResolver,
51-
ctlrName,
52-
gateway,
53-
)
44+
45+
valid, ignored, conds := validateBackendTLSPolicy(backendTLSPolicy, configMapResolver, ctlrName)
5446

5547
if valid && !ignored && backendTLSPolicy.Spec.Validation.CACertificateRefs != nil {
5648
caCertRef = types.NamespacedName{
@@ -77,14 +69,16 @@ func validateBackendTLSPolicy(
7769
backendTLSPolicy *v1alpha3.BackendTLSPolicy,
7870
configMapResolver *configMapResolver,
7971
ctlrName string,
80-
gateway *Gateway,
8172
) (valid, ignored bool, conds []conditions.Condition) {
8273
valid = true
8374
ignored = false
84-
if err := validateAncestorMaxCount(backendTLSPolicy, ctlrName, gateway); err != nil {
75+
76+
// FIXME (kate-osborn): https://github.com/nginxinc/nginx-gateway-fabric/issues/1987
77+
if ancestorsFull(backendTLSPolicy.Status.Ancestors, ctlrName) {
8578
valid = false
8679
ignored = true
8780
}
81+
8882
if err := validateBackendTLSHostname(backendTLSPolicy); err != nil {
8983
valid = false
9084
conds = append(conds, staticConds.NewPolicyInvalid(fmt.Sprintf("invalid hostname: %s", err.Error())))
@@ -115,17 +109,6 @@ func validateBackendTLSPolicy(
115109
return valid, ignored, conds
116110
}
117111

118-
func validateAncestorMaxCount(backendTLSPolicy *v1alpha3.BackendTLSPolicy, ctlrName string, gateway *Gateway) error {
119-
ancestorRef := createParentReference(v1.GroupName, kinds.Gateway, client.ObjectKeyFromObject(gateway.Source))
120-
121-
if ancestorsFull(backendTLSPolicy.Status.Ancestors, ancestorRef, ctlrName) {
122-
// FIXME (kate-osborn): https://github.com/nginxinc/nginx-gateway-fabric/issues/1987
123-
return errors.New("too many ancestors, cannot attach a new Gateway")
124-
}
125-
126-
return nil
127-
}
128-
129112
func validateBackendTLSHostname(btp *v1alpha3.BackendTLSPolicy) error {
130113
h := string(btp.Spec.Validation.Hostname)
131114

‎internal/mode/static/state/graph/backend_tls_policy_test.go

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -397,16 +397,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
397397
t.Run(test.name, func(t *testing.T) {
398398
g := NewWithT(t)
399399

400-
gateway := &Gateway{
401-
Source: &gatewayv1.Gateway{ObjectMeta: metav1.ObjectMeta{Name: "gateway", Namespace: "test"}},
402-
}
403-
404-
valid, ignored, conds := validateBackendTLSPolicy(
405-
test.tlsPolicy,
406-
configMapResolver,
407-
"test",
408-
gateway,
409-
)
400+
valid, ignored, conds := validateBackendTLSPolicy(test.tlsPolicy, configMapResolver, "test")
410401

411402
g.Expect(valid).To(Equal(test.isValid))
412403
g.Expect(ignored).To(Equal(test.ignored))

‎internal/mode/static/state/graph/policies.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ func attachPolicyToRoute(policy *Policy, route *L7Route, ctlrName string) {
101101
}
102102

103103
curAncestorStatus := policy.Source.GetPolicyStatus().Ancestors
104-
if ancestorsFull(curAncestorStatus, ancestor.Ancestor, ctlrName) {
104+
if ancestorsFull(curAncestorStatus, ctlrName) {
105105
// FIXME (kate-osborn): https://github.com/nginxinc/nginx-gateway-fabric/issues/1987
106106
return
107107
}
@@ -136,7 +136,7 @@ func attachPolicyToGateway(
136136
}
137137

138138
curAncestorStatus := policy.Source.GetPolicyStatus().Ancestors
139-
if ancestorsFull(curAncestorStatus, ancestor.Ancestor, ctlrName) {
139+
if ancestorsFull(curAncestorStatus, ctlrName) {
140140
// FIXME (kate-osborn): https://github.com/nginxinc/nginx-gateway-fabric/issues/1987
141141
return
142142
}

‎internal/mode/static/state/graph/policy_ancestor.go

Lines changed: 1 addition & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,56 +4,27 @@ import (
44
"k8s.io/apimachinery/pkg/types"
55
v1 "sigs.k8s.io/gateway-api/apis/v1"
66
"sigs.k8s.io/gateway-api/apis/v1alpha2"
7-
8-
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
97
)
108

119
const maxAncestors = 16
1210

1311
func ancestorsFull(
1412
ancestors []v1alpha2.PolicyAncestorStatus,
15-
newAncestor v1.ParentReference,
1613
ctlrName string,
1714
) bool {
1815
if len(ancestors) < maxAncestors {
1916
return false
2017
}
2118

22-
status := v1alpha2.PolicyAncestorStatus{AncestorRef: newAncestor, ControllerName: v1.GatewayController(ctlrName)}
23-
2419
for _, ancestor := range ancestors {
25-
if ancestorStatusEqual(ancestor, status) {
20+
if string(ancestor.ControllerName) == ctlrName {
2621
return false
2722
}
2823
}
2924

3025
return true
3126
}
3227

33-
func ancestorStatusEqual(curStatus v1alpha2.PolicyAncestorStatus, newStatus v1alpha2.PolicyAncestorStatus) bool {
34-
if curStatus.ControllerName != newStatus.ControllerName {
35-
return false
36-
}
37-
38-
if !helpers.EqualPointers(curStatus.AncestorRef.Group, newStatus.AncestorRef.Group) {
39-
return false
40-
}
41-
42-
if !helpers.EqualPointers(curStatus.AncestorRef.Kind, newStatus.AncestorRef.Kind) {
43-
return false
44-
}
45-
46-
if curStatus.AncestorRef.Name != newStatus.AncestorRef.Name {
47-
return false
48-
}
49-
50-
if !helpers.EqualPointers(curStatus.AncestorRef.Namespace, newStatus.AncestorRef.Namespace) {
51-
return false
52-
}
53-
54-
return true
55-
}
56-
5728
func createParentReference(
5829
group v1.Group,
5930
kind v1.Kind,

‎internal/mode/static/state/graph/policy_ancestor_test.go

Lines changed: 7 additions & 154 deletions
Original file line numberDiff line numberDiff line change
@@ -6,25 +6,14 @@ import (
66
. "github.com/onsi/gomega"
77
v1 "sigs.k8s.io/gateway-api/apis/v1"
88
"sigs.k8s.io/gateway-api/apis/v1alpha2"
9-
10-
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
11-
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
129
)
1310

1411
func TestAncestorsFull(t *testing.T) {
15-
ns := "test"
16-
1712
createCurStatus := func(numAncestors int, ctlrName string) []v1alpha2.PolicyAncestorStatus {
1813
statuses := make([]v1alpha2.PolicyAncestorStatus, 0, numAncestors)
1914

2015
for i := 0; i < numAncestors; i++ {
2116
statuses = append(statuses, v1alpha2.PolicyAncestorStatus{
22-
AncestorRef: v1.ParentReference{
23-
Group: helpers.GetPointer[v1.Group](v1.GroupName),
24-
Kind: helpers.GetPointer[v1.Kind](kinds.Gateway),
25-
Namespace: (*v1.Namespace)(&ns),
26-
Name: "name",
27-
},
2817
ControllerName: v1.GatewayController(ctlrName),
2918
})
3019
}
@@ -33,169 +22,33 @@ func TestAncestorsFull(t *testing.T) {
3322
}
3423

3524
tests := []struct {
36-
newAncestor v1.ParentReference
37-
name string
38-
curStatus []v1alpha2.PolicyAncestorStatus
39-
expFull bool
25+
name string
26+
curStatus []v1alpha2.PolicyAncestorStatus
27+
expFull bool
4028
}{
4129
{
4230
name: "not full",
4331
curStatus: createCurStatus(15, "controller"),
44-
newAncestor: v1.ParentReference{
45-
Group: helpers.GetPointer[v1.Group](v1.GroupName),
46-
Kind: helpers.GetPointer[v1.Kind](kinds.Gateway),
47-
Namespace: (*v1.Namespace)(&ns),
48-
Name: "name",
49-
},
50-
expFull: false,
32+
expFull: false,
5133
},
5234
{
5335
name: "full; ancestor does not exist in current status",
5436
curStatus: createCurStatus(16, "controller"),
55-
newAncestor: v1.ParentReference{
56-
Group: helpers.GetPointer[v1.Group](v1.GroupName),
57-
Kind: helpers.GetPointer[v1.Kind](kinds.Gateway),
58-
Namespace: (*v1.Namespace)(&ns),
59-
Name: "name",
60-
},
61-
expFull: true,
37+
expFull: true,
6238
},
6339
{
6440
name: "full, but ancestor does exist in current status",
6541
curStatus: createCurStatus(16, "nginx-gateway"),
66-
newAncestor: v1.ParentReference{
67-
Group: helpers.GetPointer[v1.Group](v1.GroupName),
68-
Kind: helpers.GetPointer[v1.Kind](kinds.Gateway),
69-
Namespace: (*v1.Namespace)(&ns),
70-
Name: "name",
71-
},
72-
expFull: false,
42+
expFull: false,
7343
},
7444
}
7545

7646
for _, test := range tests {
7747
t.Run(test.name, func(t *testing.T) {
7848
g := NewWithT(t)
7949

80-
full := ancestorsFull(test.curStatus, test.newAncestor, "nginx-gateway")
50+
full := ancestorsFull(test.curStatus, "nginx-gateway")
8151
g.Expect(full).To(Equal(test.expFull))
8252
})
8353
}
8454
}
85-
86-
func TestAncestorStatusExists(t *testing.T) {
87-
getStatus := func() v1alpha2.PolicyAncestorStatus {
88-
ns := "test"
89-
90-
return v1alpha2.PolicyAncestorStatus{
91-
AncestorRef: v1.ParentReference{
92-
Group: helpers.GetPointer[v1.Group](v1.GroupName),
93-
Kind: helpers.GetPointer[v1.Kind](kinds.Gateway),
94-
Namespace: (*v1.Namespace)(&ns),
95-
Name: "name",
96-
},
97-
ControllerName: "nginx-gateway",
98-
}
99-
}
100-
101-
type modFunc func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus
102-
103-
getModifiedStatus := func(mod modFunc) v1alpha2.PolicyAncestorStatus {
104-
return mod(getStatus())
105-
}
106-
107-
tests := []struct {
108-
name string
109-
curStatus v1alpha2.PolicyAncestorStatus
110-
newStatus v1alpha2.PolicyAncestorStatus
111-
expEqual bool
112-
}{
113-
{
114-
name: "equal",
115-
curStatus: getStatus(),
116-
newStatus: getStatus(),
117-
expEqual: true,
118-
},
119-
{
120-
name: "different controller name",
121-
curStatus: getStatus(),
122-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
123-
s.ControllerName = "not-ours"
124-
return s
125-
}),
126-
expEqual: false,
127-
},
128-
{
129-
name: "different groups; one nil",
130-
curStatus: getStatus(),
131-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
132-
s.AncestorRef.Group = nil
133-
return s
134-
}),
135-
expEqual: false,
136-
},
137-
{
138-
name: "different groups",
139-
curStatus: getStatus(),
140-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
141-
s.AncestorRef.Group = helpers.GetPointer[v1.Group]("DiffGroup")
142-
return s
143-
}),
144-
expEqual: false,
145-
},
146-
{
147-
name: "different kinds; one nil",
148-
curStatus: getStatus(),
149-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
150-
s.AncestorRef.Kind = nil
151-
return s
152-
}),
153-
expEqual: false,
154-
},
155-
{
156-
name: "different kinds",
157-
curStatus: getStatus(),
158-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
159-
s.AncestorRef.Kind = helpers.GetPointer[v1.Kind](kinds.HTTPRoute)
160-
return s
161-
}),
162-
expEqual: false,
163-
},
164-
{
165-
name: "different names",
166-
curStatus: getStatus(),
167-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
168-
s.AncestorRef.Name = "diff-name"
169-
return s
170-
}),
171-
expEqual: false,
172-
},
173-
{
174-
name: "different namespaces; one nil",
175-
curStatus: getStatus(),
176-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
177-
s.AncestorRef.Namespace = nil
178-
return s
179-
}),
180-
expEqual: false,
181-
},
182-
{
183-
name: "different namespaces",
184-
newStatus: getModifiedStatus(func(s v1alpha2.PolicyAncestorStatus) v1alpha2.PolicyAncestorStatus {
185-
diffNs := "diff"
186-
s.AncestorRef.Namespace = (*v1.Namespace)(&diffNs)
187-
return s
188-
}),
189-
expEqual: false,
190-
},
191-
}
192-
193-
for _, test := range tests {
194-
t.Run(test.name, func(t *testing.T) {
195-
g := NewWithT(t)
196-
197-
equal := ancestorStatusEqual(test.curStatus, test.newStatus)
198-
g.Expect(equal).To(Equal(test.expEqual))
199-
})
200-
}
201-
}

0 commit comments

Comments
 (0)