Skip to content

Commit 59c296c

Browse files
pleshakovpleshakov
authored
Implement NKG-specific validation for GatewayClass (#495)
Fixes #474
1 parent 9ee8131 commit 59c296c

12 files changed

+284
-250
lines changed

‎internal/state/change_processor_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -403,8 +403,8 @@ var _ = Describe("ChangeProcessor", func() {
403403

404404
expectedStatuses := state.Statuses{
405405
GatewayClassStatus: &state.GatewayClassStatus{
406-
Valid: true,
407406
ObservedGeneration: gc.Generation,
407+
Conditions: conditions.NewDefaultGatewayClassConditions(),
408408
},
409409
GatewayStatus: &state.GatewayStatus{
410410
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
@@ -512,8 +512,8 @@ var _ = Describe("ChangeProcessor", func() {
512512
}
513513
expectedStatuses := state.Statuses{
514514
GatewayClassStatus: &state.GatewayClassStatus{
515-
Valid: true,
516515
ObservedGeneration: gc.Generation,
516+
Conditions: conditions.NewDefaultGatewayClassConditions(),
517517
},
518518
GatewayStatus: &state.GatewayStatus{
519519
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
@@ -622,8 +622,8 @@ var _ = Describe("ChangeProcessor", func() {
622622
}
623623
expectedStatuses := state.Statuses{
624624
GatewayClassStatus: &state.GatewayClassStatus{
625-
Valid: true,
626625
ObservedGeneration: gc.Generation,
626+
Conditions: conditions.NewDefaultGatewayClassConditions(),
627627
},
628628
GatewayStatus: &state.GatewayStatus{
629629
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
@@ -731,8 +731,8 @@ var _ = Describe("ChangeProcessor", func() {
731731
}
732732
expectedStatuses := state.Statuses{
733733
GatewayClassStatus: &state.GatewayClassStatus{
734-
Valid: true,
735734
ObservedGeneration: gcUpdated.Generation,
735+
Conditions: conditions.NewDefaultGatewayClassConditions(),
736736
},
737737
GatewayStatus: &state.GatewayStatus{
738738
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
@@ -839,8 +839,8 @@ var _ = Describe("ChangeProcessor", func() {
839839
}
840840
expectedStatuses := state.Statuses{
841841
GatewayClassStatus: &state.GatewayClassStatus{
842-
Valid: true,
843842
ObservedGeneration: gcUpdated.Generation,
843+
Conditions: conditions.NewDefaultGatewayClassConditions(),
844844
},
845845
GatewayStatus: &state.GatewayStatus{
846846
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
@@ -940,8 +940,8 @@ var _ = Describe("ChangeProcessor", func() {
940940
}
941941
expectedStatuses := state.Statuses{
942942
GatewayClassStatus: &state.GatewayClassStatus{
943-
Valid: true,
944943
ObservedGeneration: gcUpdated.Generation,
944+
Conditions: conditions.NewDefaultGatewayClassConditions(),
945945
},
946946
GatewayStatus: &state.GatewayStatus{
947947
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
@@ -1061,8 +1061,8 @@ var _ = Describe("ChangeProcessor", func() {
10611061
}
10621062
expectedStatuses := state.Statuses{
10631063
GatewayClassStatus: &state.GatewayClassStatus{
1064-
Valid: true,
10651064
ObservedGeneration: gcUpdated.Generation,
1065+
Conditions: conditions.NewDefaultGatewayClassConditions(),
10661066
},
10671067
GatewayStatus: &state.GatewayStatus{
10681068
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-2"},
@@ -1125,8 +1125,8 @@ var _ = Describe("ChangeProcessor", func() {
11251125
}
11261126
expectedStatuses := state.Statuses{
11271127
GatewayClassStatus: &state.GatewayClassStatus{
1128-
Valid: true,
11291128
ObservedGeneration: gcUpdated.Generation,
1129+
Conditions: conditions.NewDefaultGatewayClassConditions(),
11301130
},
11311131
GatewayStatus: &state.GatewayStatus{
11321132
NsName: types.NamespacedName{Namespace: "test", Name: "gateway-2"},

‎internal/state/conditions/conditions.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,3 +263,25 @@ func NewRouteBackendRefUnsupportedValue(msg string) Condition {
263263
Message: msg,
264264
}
265265
}
266+
267+
// NewGatewayClassInvalidParameters returns a Condition that indicates that the GatewayClass has invalid parameters.
268+
func NewGatewayClassInvalidParameters(msg string) Condition {
269+
return Condition{
270+
Type: string(v1beta1.GatewayClassConditionStatusAccepted),
271+
Status: metav1.ConditionFalse,
272+
Reason: string(v1beta1.GatewayClassReasonInvalidParameters),
273+
Message: msg,
274+
}
275+
}
276+
277+
// NewDefaultGatewayClassConditions returns the default Conditions that must be present in the status of a GatewayClass.
278+
func NewDefaultGatewayClassConditions() []Condition {
279+
return []Condition{
280+
{
281+
Type: string(v1beta1.GatewayClassConditionStatusAccepted),
282+
Status: metav1.ConditionTrue,
283+
Reason: string(v1beta1.GatewayClassReasonAccepted),
284+
Message: "GatewayClass is accepted",
285+
},
286+
}
287+
}

‎internal/state/dataplane/configuration_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -748,9 +748,8 @@ func TestBuildConfiguration(t *testing.T) {
748748
{
749749
graph: &graph.Graph{
750750
GatewayClass: &graph.GatewayClass{
751-
Source: &v1beta1.GatewayClass{},
752-
Valid: false,
753-
ErrorMsg: "error",
751+
Source: &v1beta1.GatewayClass{},
752+
Valid: false,
754753
},
755754
Gateway: &graph.Gateway{
756755
Source: &v1beta1.Gateway{},

‎internal/state/graph/gatewayclass.go

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,54 @@
11
package graph
22

33
import (
4-
"fmt"
5-
4+
"k8s.io/apimachinery/pkg/util/validation/field"
65
"sigs.k8s.io/gateway-api/apis/v1beta1"
6+
7+
"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions"
78
)
89

910
// GatewayClass represents the GatewayClass resource.
1011
type GatewayClass struct {
1112
// Source is the source resource.
1213
Source *v1beta1.GatewayClass
13-
// ErrorMsg explains the error when the resource is invalid.
14-
ErrorMsg string
14+
// Conditions include Conditions for the GatewayClass.
15+
Conditions []conditions.Condition
1516
// Valid shows whether the GatewayClass is valid.
1617
Valid bool
1718
}
1819

19-
func buildGatewayClass(gc *v1beta1.GatewayClass, controllerName string) *GatewayClass {
20+
func gatewayClassBelongsToController(gc *v1beta1.GatewayClass, controllerName string) bool {
21+
// if GatewayClass doesn't exist, we assume it belongs to the controller
22+
if gc == nil {
23+
return true
24+
}
25+
26+
return string(gc.Spec.ControllerName) == controllerName
27+
}
28+
29+
func buildGatewayClass(gc *v1beta1.GatewayClass) *GatewayClass {
2030
if gc == nil {
2131
return nil
2232
}
2333

24-
var errorMsg string
34+
var conds []conditions.Condition
2535

26-
err := validateGatewayClass(gc, controllerName)
27-
if err != nil {
28-
errorMsg = err.Error()
36+
valErr := validateGatewayClass(gc)
37+
if valErr != nil {
38+
conds = append(conds, conditions.NewGatewayClassInvalidParameters(valErr.Error()))
2939
}
3040

3141
return &GatewayClass{
32-
Source: gc,
33-
Valid: err == nil,
34-
ErrorMsg: errorMsg,
42+
Source: gc,
43+
Valid: valErr == nil,
44+
Conditions: conds,
3545
}
3646
}
3747

38-
func validateGatewayClass(gc *v1beta1.GatewayClass, controllerName string) error {
39-
if string(gc.Spec.ControllerName) != controllerName {
40-
return fmt.Errorf("Spec.ControllerName must be %s got %s", controllerName, gc.Spec.ControllerName)
48+
func validateGatewayClass(gc *v1beta1.GatewayClass) error {
49+
if gc.Spec.ParametersRef != nil {
50+
path := field.NewPath("spec").Child("parametersRef")
51+
return field.Forbidden(path, "parametersRef is not supported")
4152
}
4253

4354
return nil

‎internal/state/graph/gatewayclass_test.go

Lines changed: 72 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,76 +3,111 @@ package graph
33
import (
44
"testing"
55

6-
"github.com/google/go-cmp/cmp"
6+
. "github.com/onsi/gomega"
77
"sigs.k8s.io/gateway-api/apis/v1beta1"
8+
9+
"github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers"
10+
"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions"
811
)
912

1013
func TestBuildGatewayClass(t *testing.T) {
11-
const controllerName = "my.controller"
14+
validGC := &v1beta1.GatewayClass{}
1215

13-
validGC := &v1beta1.GatewayClass{
14-
Spec: v1beta1.GatewayClassSpec{
15-
ControllerName: "my.controller",
16-
},
17-
}
1816
invalidGC := &v1beta1.GatewayClass{
1917
Spec: v1beta1.GatewayClassSpec{
20-
ControllerName: "wrong.controller",
18+
ParametersRef: &v1beta1.ParametersReference{},
2119
},
2220
}
2321

2422
tests := []struct {
2523
gc *v1beta1.GatewayClass
2624
expected *GatewayClass
27-
msg string
25+
name string
2826
}{
29-
{
30-
gc: nil,
31-
expected: nil,
32-
msg: "no gatewayclass",
33-
},
3427
{
3528
gc: validGC,
3629
expected: &GatewayClass{
37-
Source: validGC,
38-
Valid: true,
39-
ErrorMsg: "",
30+
Source: validGC,
31+
Valid: true,
4032
},
41-
msg: "valid gatewayclass",
33+
name: "valid gatewayclass",
34+
},
35+
{
36+
gc: nil,
37+
expected: nil,
38+
name: "no gatewayclass",
4239
},
4340
{
4441
gc: invalidGC,
4542
expected: &GatewayClass{
46-
Source: invalidGC,
47-
Valid: false,
48-
ErrorMsg: "Spec.ControllerName must be my.controller got wrong.controller",
43+
Source: invalidGC,
44+
Valid: false,
45+
Conditions: []conditions.Condition{
46+
conditions.NewGatewayClassInvalidParameters("spec.parametersRef: Forbidden: parametersRef is not supported"),
47+
},
4948
},
50-
msg: "invalid gatewayclass",
49+
name: "invalid gatewayclass",
5150
},
5251
}
5352

5453
for _, test := range tests {
55-
result := buildGatewayClass(test.gc, controllerName)
56-
if diff := cmp.Diff(test.expected, result); diff != "" {
57-
t.Errorf("buildGatewayClass() '%s' mismatch (-want +got):\n%s", test.msg, diff)
58-
}
54+
t.Run(test.name, func(t *testing.T) {
55+
g := NewGomegaWithT(t)
56+
57+
result := buildGatewayClass(test.gc)
58+
g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty())
59+
})
5960
}
6061
}
6162

62-
func TestValidateGatewayClass(t *testing.T) {
63-
gc := &v1beta1.GatewayClass{
64-
Spec: v1beta1.GatewayClassSpec{
65-
ControllerName: "test.controller",
63+
func TestGatewayClassBelongsToController(t *testing.T) {
64+
const controllerName = "my.controller"
65+
66+
tests := []struct {
67+
gc *v1beta1.GatewayClass
68+
name string
69+
expected bool
70+
}{
71+
{
72+
gc: &v1beta1.GatewayClass{
73+
Spec: v1beta1.GatewayClassSpec{
74+
ControllerName: controllerName,
75+
},
76+
},
77+
expected: true,
78+
name: "normal gatewayclass",
79+
},
80+
{
81+
gc: nil,
82+
expected: true,
83+
name: "no gatewayclass",
84+
},
85+
{
86+
gc: &v1beta1.GatewayClass{
87+
Spec: v1beta1.GatewayClassSpec{
88+
ControllerName: "some.controller",
89+
},
90+
},
91+
expected: false,
92+
name: "wrong controller name",
93+
},
94+
{
95+
gc: &v1beta1.GatewayClass{
96+
Spec: v1beta1.GatewayClassSpec{
97+
ControllerName: "",
98+
},
99+
},
100+
expected: false,
101+
name: "empty controller name",
66102
},
67103
}
68104

69-
err := validateGatewayClass(gc, "test.controller")
70-
if err != nil {
71-
t.Errorf("validateGatewayClass() returned unexpected error %v", err)
72-
}
105+
for _, test := range tests {
106+
t.Run(test.name, func(t *testing.T) {
107+
g := NewGomegaWithT(t)
73108

74-
err = validateGatewayClass(gc, "unmatched.controller")
75-
if err == nil {
76-
t.Errorf("validateGatewayClass() didn't return an error")
109+
result := gatewayClassBelongsToController(test.gc, controllerName)
110+
g.Expect(result).To(Equal(test.expected))
111+
})
77112
}
78113
}

‎internal/state/graph/graph.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,11 @@ func BuildGraph(
3939
secretMemoryMgr secrets.SecretDiskMemoryManager,
4040
validators validation.Validators,
4141
) *Graph {
42-
gc := buildGatewayClass(store.GatewayClass, controllerName)
42+
if !gatewayClassBelongsToController(store.GatewayClass, controllerName) {
43+
return &Graph{}
44+
}
45+
46+
gc := buildGatewayClass(store.GatewayClass)
4347

4448
processedGws := processGateways(store.Gateways, gcName)
4549

0 commit comments

Comments
 (0)