Skip to content

Commit 6028324

Browse files
miledxzmiledxz
committed
Run webhook validation (nginx#388)
In case the webhook is not installed or not running validation properly, we still want NKG to ensure that the webhook validation is always performed and NKG rejects any invalid resource. Fixes nginx#362
1 parent 048fb32 commit 6028324

12 files changed

+436
-152
lines changed

‎deploy/manifests/nginx-gateway.yaml

+7
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,13 @@ rules:
1717
verbs:
1818
- list
1919
- watch
20+
- apiGroups:
21+
- ""
22+
resources:
23+
- events
24+
verbs:
25+
- create
26+
- patch
2027
- apiGroups:
2128
- discovery.k8s.io
2229
resources:

‎internal/manager/controllers.go

+11-1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ type controllerConfig struct {
2626
k8sPredicate predicate.Predicate
2727
fieldIndices index.FieldIndices
2828
newReconciler newReconcilerFunc
29+
webhookValidator reconciler.ValidatorFunc
2930
}
3031

3132
type controllerOption func(*controllerConfig)
@@ -55,6 +56,12 @@ func withNewReconciler(newReconciler newReconcilerFunc) controllerOption {
5556
}
5657
}
5758

59+
func withWebhookValidator(validator reconciler.ValidatorFunc) controllerOption {
60+
return func(cfg *controllerConfig) {
61+
cfg.webhookValidator = validator
62+
}
63+
}
64+
5865
func defaultControllerConfig() controllerConfig {
5966
return controllerConfig{
6067
newReconciler: reconciler.NewImplementation,
@@ -65,7 +72,8 @@ func registerController(
6572
ctx context.Context,
6673
objectType client.Object,
6774
mgr manager.Manager,
68-
eventCh chan interface{},
75+
eventCh chan<- interface{},
76+
recorder reconciler.EventRecorder,
6977
options ...controllerOption,
7078
) error {
7179
cfg := defaultControllerConfig()
@@ -92,6 +100,8 @@ func registerController(
92100
ObjectType: objectType,
93101
EventCh: eventCh,
94102
NamespacedNameFilter: cfg.namespacedNameFilter,
103+
WebhookValidator: cfg.webhookValidator,
104+
EventRecorder: recorder,
95105
}
96106

97107
err := builder.Complete(cfg.newReconciler(recCfg))

‎internal/manager/controllers_test.go

+57-97
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,12 @@ import (
66
"reflect"
77
"testing"
88

9+
. "github.com/onsi/gomega"
10+
"github.com/onsi/gomega/gcustom"
11+
"github.com/onsi/gomega/types"
912
"k8s.io/apimachinery/pkg/runtime"
1013
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
14+
"k8s.io/apimachinery/pkg/util/validation/field"
1115
"sigs.k8s.io/controller-runtime/pkg/client/fake"
1216
"sigs.k8s.io/controller-runtime/pkg/log/zap"
1317
"sigs.k8s.io/gateway-api/apis/v1beta1"
@@ -17,6 +21,7 @@ import (
1721
"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/managerfakes"
1822
"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/predicate"
1923
"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler"
24+
"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler/reconcilerfakes"
2025
)
2126

2227
func TestRegisterController(t *testing.T) {
@@ -81,114 +86,69 @@ func TestRegisterController(t *testing.T) {
8186
namespacedNameFilter := filter.CreateFilterForGatewayClass("test")
8287
fieldIndexes := index.CreateEndpointSliceFieldIndices()
8388

84-
eventCh := make(chan interface{})
89+
webhookValidator := createValidator(func(_ *v1beta1.HTTPRoute) field.ErrorList {
90+
return nil
91+
})
92+
93+
eventRecorder := &reconcilerfakes.FakeEventRecorder{}
94+
95+
eventCh := make(chan<- interface{})
96+
97+
beSameFunctionPointer := func(expected interface{}) types.GomegaMatcher {
98+
return gcustom.MakeMatcher(func(f interface{}) (bool, error) {
99+
// comparing functions is not allowed in Go, so we're comparing the pointers
100+
return reflect.ValueOf(expected).Pointer() == reflect.ValueOf(f).Pointer(), nil
101+
})
102+
}
85103

86104
for _, test := range tests {
87-
newReconciler := func(c reconciler.Config) *reconciler.Implementation {
88-
if c.Getter != test.fakes.mgr.GetClient() {
89-
t.Errorf(
90-
"regiterController() created a reconciler config with Getter %p but expected %p for case of %q",
91-
c.Getter,
92-
test.fakes.mgr.GetClient(),
93-
test.msg,
94-
)
95-
}
96-
if c.ObjectType != objectType {
97-
t.Errorf(
98-
"registerController() created a reconciler config with ObjectType %T but expected %T for case of %q",
99-
c.ObjectType,
100-
objectType,
101-
test.msg,
102-
)
105+
t.Run(test.msg, func(t *testing.T) {
106+
g := NewGomegaWithT(t)
107+
108+
newReconciler := func(c reconciler.Config) *reconciler.Implementation {
109+
g.Expect(c.Getter).To(BeIdenticalTo(test.fakes.mgr.GetClient()))
110+
g.Expect(c.ObjectType).To(BeIdenticalTo(objectType))
111+
g.Expect(c.EventCh).To(BeIdenticalTo(eventCh))
112+
g.Expect(c.EventRecorder).To(BeIdenticalTo(eventRecorder))
113+
g.Expect(c.WebhookValidator).Should(beSameFunctionPointer(webhookValidator))
114+
g.Expect(c.NamespacedNameFilter).Should(beSameFunctionPointer(namespacedNameFilter))
115+
116+
return reconciler.NewImplementation(c)
103117
}
104-
if c.EventCh != eventCh {
105-
t.Errorf(
106-
"registerController() created a reconciler config with EventCh %v but expected %v for case of %q",
107-
c.EventCh,
108-
eventCh,
109-
test.msg,
110-
)
111-
}
112-
// comparing functions is not allowed in Go, so we're comparing the pointers
113-
// nolint: lll
114-
if reflect.ValueOf(c.NamespacedNameFilter).Pointer() != reflect.ValueOf(namespacedNameFilter).Pointer() {
115-
t.Errorf(
116-
"registerController() created a reconciler config with NamespacedNameFilter %p but expected %p for case of %q",
117-
c.NamespacedNameFilter,
118-
namespacedNameFilter,
119-
test.msg,
120-
)
118+
119+
err := registerController(
120+
context.Background(),
121+
objectType,
122+
test.fakes.mgr,
123+
eventCh,
124+
eventRecorder,
125+
withNamespacedNameFilter(namespacedNameFilter),
126+
withK8sPredicate(predicate.ServicePortsChangedPredicate{}),
127+
withFieldIndices(fieldIndexes),
128+
withNewReconciler(newReconciler),
129+
withWebhookValidator(webhookValidator),
130+
)
131+
132+
if test.expectedErr == nil {
133+
g.Expect(err).To(BeNil())
134+
} else {
135+
g.Expect(err).To(MatchError(test.expectedErr))
121136
}
122137

123-
return reconciler.NewImplementation(c)
124-
}
138+
indexCallCount := test.fakes.indexer.IndexFieldCallCount()
125139

126-
err := registerController(
127-
context.Background(),
128-
objectType,
129-
test.fakes.mgr,
130-
eventCh,
131-
withNamespacedNameFilter(namespacedNameFilter),
132-
withK8sPredicate(predicate.ServicePortsChangedPredicate{}),
133-
withFieldIndices(fieldIndexes),
134-
withNewReconciler(newReconciler),
135-
)
136-
137-
if !errors.Is(err, test.expectedErr) {
138-
t.Errorf(
139-
"registerController() returned %q but expected %q for case of %q",
140-
err,
141-
test.expectedErr,
142-
test.msg,
143-
)
144-
}
140+
g.Expect(indexCallCount).To(Equal(1))
145141

146-
indexCallCount := test.fakes.indexer.IndexFieldCallCount()
147-
if indexCallCount != 1 {
148-
t.Errorf(
149-
"registerController() called indexer.IndexField() %d times but expected 1 for case of %q",
150-
indexCallCount,
151-
test.msg,
152-
)
153-
} else {
154142
_, objType, field, indexFunc := test.fakes.indexer.IndexFieldArgsForCall(0)
155143

156-
if objType != objectType {
157-
t.Errorf(
158-
"registerController() called indexer.IndexField() with object type %T but expected %T for case %q",
159-
objType,
160-
objectType,
161-
test.msg,
162-
)
163-
}
164-
if field != index.KubernetesServiceNameIndexField {
165-
t.Errorf("registerController() called indexer.IndexField() with field %q but expected %q for case %q",
166-
field,
167-
index.KubernetesServiceNameIndexField,
168-
test.msg,
169-
)
170-
}
144+
g.Expect(objType).To(BeIdenticalTo(objectType))
145+
g.Expect(field).To(BeIdenticalTo(index.KubernetesServiceNameIndexField))
171146

172147
expectedIndexFunc := fieldIndexes[index.KubernetesServiceNameIndexField]
173-
// comparing functions is not allowed in Go, so we're comparing the pointers
174-
// nolint:lll
175-
if reflect.ValueOf(indexFunc).Pointer() != reflect.ValueOf(expectedIndexFunc).Pointer() {
176-
t.Errorf("registerController() called indexer.IndexField() with indexFunc %p but expected %p for case %q",
177-
indexFunc,
178-
expectedIndexFunc,
179-
test.msg,
180-
)
181-
}
182-
}
148+
g.Expect(indexFunc).To(beSameFunctionPointer(expectedIndexFunc))
183149

184-
addCallCount := test.fakes.mgr.AddCallCount()
185-
if addCallCount != test.expectedMgrAddCallCount {
186-
t.Errorf(
187-
"registerController() called mgr.Add() %d times but expected %d times for case %q",
188-
addCallCount,
189-
test.expectedMgrAddCallCount,
190-
test.msg,
191-
)
192-
}
150+
addCallCount := test.fakes.mgr.AddCallCount()
151+
g.Expect(addCallCount).To(Equal(test.expectedMgrAddCallCount))
152+
})
193153
}
194154
}

‎internal/manager/manager.go

+13-1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"sigs.k8s.io/controller-runtime/pkg/manager"
1515
k8spredicate "sigs.k8s.io/controller-runtime/pkg/predicate"
1616
gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
17+
"sigs.k8s.io/gateway-api/apis/v1beta1/validation"
1718

1819
"github.com/nginxinc/nginx-kubernetes-gateway/internal/config"
1920
"github.com/nginxinc/nginx-kubernetes-gateway/internal/events"
@@ -71,13 +72,21 @@ func Start(cfg config.Config) error {
7172
objectType: &gatewayv1beta1.GatewayClass{},
7273
options: []controllerOption{
7374
withNamespacedNameFilter(filter.CreateFilterForGatewayClass(cfg.GatewayClassName)),
75+
// as of v0.6.0, the Gateway API Webhook doesn't include a validation function
76+
// for the GatewayClass resource
7477
},
7578
},
7679
{
7780
objectType: &gatewayv1beta1.Gateway{},
81+
options: []controllerOption{
82+
withWebhookValidator(createValidator(validation.ValidateGateway)),
83+
},
7884
},
7985
{
8086
objectType: &gatewayv1beta1.HTTPRoute{},
87+
options: []controllerOption{
88+
withWebhookValidator(createValidator(validation.ValidateHTTPRoute)),
89+
},
8190
},
8291
{
8392
objectType: &apiv1.Service{},
@@ -99,8 +108,11 @@ func Start(cfg config.Config) error {
99108

100109
ctx := ctlr.SetupSignalHandler()
101110

111+
recorderName := fmt.Sprintf("nginx-kubernetes-gateway-%s", cfg.GatewayClassName)
112+
recorder := mgr.GetEventRecorderFor(recorderName)
113+
102114
for _, regCfg := range controllerRegCfgs {
103-
err := registerController(ctx, regCfg.objectType, mgr, eventCh, regCfg.options...)
115+
err := registerController(ctx, regCfg.objectType, mgr, eventCh, recorder, regCfg.options...)
104116
if err != nil {
105117
return fmt.Errorf("cannot register controller for %T: %w", regCfg.objectType, err)
106118
}

‎internal/manager/validators.go

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package manager
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
7+
"k8s.io/apimachinery/pkg/util/validation/field"
8+
"sigs.k8s.io/controller-runtime/pkg/client"
9+
10+
"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler"
11+
)
12+
13+
// createValidator creates a reconciler.ValidatorFunc from a function that validates a resource of type R.
14+
func createValidator[R client.Object](validate func(R) field.ErrorList) reconciler.ValidatorFunc {
15+
return func(obj client.Object) error {
16+
if obj == nil {
17+
panic(errors.New("obj is nil"))
18+
}
19+
20+
r, ok := obj.(R)
21+
if !ok {
22+
panic(fmt.Errorf("obj type mismatch: got %T, expected %T", obj, r))
23+
}
24+
25+
return validate(r).ToAggregate()
26+
}
27+
}

‎internal/manager/validators_test.go

+77
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
package manager
2+
3+
import (
4+
"testing"
5+
6+
. "github.com/onsi/gomega"
7+
"k8s.io/apimachinery/pkg/util/validation/field"
8+
"sigs.k8s.io/controller-runtime/pkg/client"
9+
"sigs.k8s.io/gateway-api/apis/v1beta1"
10+
)
11+
12+
func TestCreateTypedValidator(t *testing.T) {
13+
tests := []struct {
14+
name string
15+
obj client.Object
16+
errorList field.ErrorList
17+
expectPanic bool
18+
expectErr bool
19+
}{
20+
{
21+
obj: &v1beta1.HTTPRoute{},
22+
errorList: field.ErrorList{},
23+
expectPanic: false,
24+
expectErr: false,
25+
name: "no errors",
26+
},
27+
{
28+
obj: &v1beta1.HTTPRoute{},
29+
errorList: []*field.Error{{Detail: "test"}},
30+
expectPanic: false,
31+
expectErr: true,
32+
name: "one error",
33+
},
34+
{
35+
obj: nil,
36+
errorList: field.ErrorList{},
37+
expectPanic: true,
38+
expectErr: false,
39+
name: "nil object",
40+
},
41+
{
42+
obj: &v1beta1.Gateway{},
43+
errorList: field.ErrorList{},
44+
expectPanic: true,
45+
expectErr: false,
46+
name: "wrong object type",
47+
},
48+
}
49+
50+
for _, test := range tests {
51+
t.Run(test.name, func(t *testing.T) {
52+
g := NewGomegaWithT(t)
53+
54+
v := createValidator(createValidateHTTPRouteThatReturns(test.errorList))
55+
56+
if test.expectPanic {
57+
g.Expect(func() { _ = v(test.obj) }).To(Panic())
58+
return
59+
}
60+
61+
result := v(test.obj)
62+
63+
if test.expectErr {
64+
g.Expect(result).ToNot(BeNil())
65+
return
66+
}
67+
68+
g.Expect(result).To(BeNil())
69+
})
70+
}
71+
}
72+
73+
func createValidateHTTPRouteThatReturns(errorList field.ErrorList) func(*v1beta1.HTTPRoute) field.ErrorList {
74+
return func(*v1beta1.HTTPRoute) field.ErrorList {
75+
return errorList
76+
}
77+
}

0 commit comments

Comments
 (0)