Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor webhook validation #510

Merged
merged 6 commits into from
Apr 4, 2023
Merged

Refactor webhook validation #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
183ed5d
Refactor Webhook validation code
pleshakov Mar 27, 2023
c6fae80
Add a note here about the GatewayClass not being covered
pleshakov Apr 4, 2023
073dd26
Improve descriptions in the unit tests
pleshakov Apr 4, 2023
a15a246
store -> state
pleshakov Apr 4, 2023
33765c3
Simplify validatingUpsertUpdater
pleshakov Apr 4, 2023
7bd7c2b
Merge branch 'main' into chore/refactor-webhook-validation
pleshakov Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions internal/manager/controllers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ type controllerConfig struct {
k8sPredicate predicate.Predicate
fieldIndices index.FieldIndices
newReconciler newReconcilerFunc
webhookValidator reconciler.ValidatorFunc
}

type controllerOption func(*controllerConfig)
Expand Down Expand Up @@ -56,12 +55,6 @@ func withNewReconciler(newReconciler newReconcilerFunc) controllerOption {
}
}

func withWebhookValidator(validator reconciler.ValidatorFunc) controllerOption {
return func(cfg *controllerConfig) {
cfg.webhookValidator = validator
}
}

func defaultControllerConfig() controllerConfig {
return controllerConfig{
newReconciler: reconciler.NewImplementation,
Expand All @@ -73,7 +66,6 @@ func registerController(
objectType client.Object,
mgr manager.Manager,
eventCh chan<- interface{},
recorder reconciler.EventRecorder,
options ...controllerOption,
) error {
cfg := defaultControllerConfig()
Expand All @@ -100,8 +92,6 @@ func registerController(
ObjectType: objectType,
EventCh: eventCh,
NamespacedNameFilter: cfg.namespacedNameFilter,
WebhookValidator: cfg.webhookValidator,
EventRecorder: recorder,
}

err := builder.Complete(cfg.newReconciler(recCfg))
Expand Down
12 changes: 0 additions & 12 deletions internal/manager/controllers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"github.com/onsi/gomega/types"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
"sigs.k8s.io/gateway-api/apis/v1beta1"
Expand All @@ -21,7 +20,6 @@ import (
"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/managerfakes"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/predicate"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler/reconcilerfakes"
)

func TestRegisterController(t *testing.T) {
Expand Down Expand Up @@ -86,12 +84,6 @@ func TestRegisterController(t *testing.T) {
namespacedNameFilter := filter.CreateFilterForGatewayClass("test")
fieldIndexes := index.CreateEndpointSliceFieldIndices()

webhookValidator := createValidator(func(_ *v1beta1.HTTPRoute) field.ErrorList {
return nil
})

eventRecorder := &reconcilerfakes.FakeEventRecorder{}

eventCh := make(chan<- interface{})

beSameFunctionPointer := func(expected interface{}) types.GomegaMatcher {
Expand All @@ -109,8 +101,6 @@ func TestRegisterController(t *testing.T) {
g.Expect(c.Getter).To(BeIdenticalTo(test.fakes.mgr.GetClient()))
g.Expect(c.ObjectType).To(BeIdenticalTo(objectType))
g.Expect(c.EventCh).To(BeIdenticalTo(eventCh))
g.Expect(c.EventRecorder).To(BeIdenticalTo(eventRecorder))
g.Expect(c.WebhookValidator).Should(beSameFunctionPointer(webhookValidator))
g.Expect(c.NamespacedNameFilter).Should(beSameFunctionPointer(namespacedNameFilter))

return reconciler.NewImplementation(c)
Expand All @@ -121,12 +111,10 @@ func TestRegisterController(t *testing.T) {
objectType,
test.fakes.mgr,
eventCh,
eventRecorder,
withNamespacedNameFilter(namespacedNameFilter),
withK8sPredicate(predicate.ServicePortsChangedPredicate{}),
withFieldIndices(fieldIndexes),
withNewReconciler(newReconciler),
withWebhookValidator(webhookValidator),
)

if test.expectedErr == nil {
Expand Down
19 changes: 6 additions & 13 deletions internal/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/manager"
k8spredicate "sigs.k8s.io/controller-runtime/pkg/predicate"
gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
gwapivalidation "sigs.k8s.io/gateway-api/apis/v1beta1/validation"

"github.com/nginxinc/nginx-kubernetes-gateway/internal/config"
"github.com/nginxinc/nginx-kubernetes-gateway/internal/events"
Expand Down Expand Up @@ -75,21 +74,13 @@ func Start(cfg config.Config) error {
objectType: &gatewayv1beta1.GatewayClass{},
options: []controllerOption{
withNamespacedNameFilter(filter.CreateFilterForGatewayClass(cfg.GatewayClassName)),
// as of v0.6.2, the Gateway API Webhook doesn't include a validation function
// for the GatewayClass resource
},
},
{
objectType: &gatewayv1beta1.Gateway{},
options: []controllerOption{
withWebhookValidator(createValidator(gwapivalidation.ValidateGateway)),
},
},
{
objectType: &gatewayv1beta1.HTTPRoute{},
options: []controllerOption{
withWebhookValidator(createValidator(gwapivalidation.ValidateHTTPRoute)),
},
},
{
objectType: &apiv1.Service{},
Expand All @@ -111,11 +102,8 @@ func Start(cfg config.Config) error {

ctx := ctlr.SetupSignalHandler()

recorderName := fmt.Sprintf("nginx-kubernetes-gateway-%s", cfg.GatewayClassName)
recorder := mgr.GetEventRecorderFor(recorderName)

for _, regCfg := range controllerRegCfgs {
err := registerController(ctx, regCfg.objectType, mgr, eventCh, recorder, regCfg.options...)
err := registerController(ctx, regCfg.objectType, mgr, eventCh, regCfg.options...)
if err != nil {
return fmt.Errorf("cannot register controller for %T: %w", regCfg.objectType, err)
}
Expand All @@ -124,6 +112,9 @@ func Start(cfg config.Config) error {
secretStore := secrets.NewSecretStore()
secretMemoryMgr := secrets.NewSecretDiskMemoryManager(secretsFolder, secretStore)

recorderName := fmt.Sprintf("nginx-kubernetes-gateway-%s", cfg.GatewayClassName)
recorder := mgr.GetEventRecorderFor(recorderName)

processor := state.NewChangeProcessorImpl(state.ChangeProcessorConfig{
GatewayCtlrName: cfg.GatewayCtlrName,
GatewayClassName: cfg.GatewayClassName,
Expand All @@ -134,6 +125,8 @@ func Start(cfg config.Config) error {
Validators: validation.Validators{
HTTPFieldsValidator: ngxvalidation.HTTPValidator{},
},
EventRecorder: recorder,
Scheme: scheme,
})

configGenerator := ngxcfg.NewGeneratorImpl()
Expand Down
27 changes: 0 additions & 27 deletions internal/manager/validators.go
View file
Open in desktop

This file was deleted.

77 changes: 0 additions & 77 deletions internal/manager/validators_test.go
View file
Open in desktop

This file was deleted.

28 changes: 1 addition & 27 deletions internal/reconciler/implementation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"reflect"

apiv1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -19,9 +18,6 @@ import (
// If the function returns false, the reconciler will log the returned string.
type NamespacedNameFilterFunc func(nsname types.NamespacedName) (bool, string)

// ValidatorFunc validates a Kubernetes resource.
type ValidatorFunc func(object client.Object) error

// Config contains the configuration for the Implementation.
type Config struct {
// Getter gets a resource from the k8s API.
Expand All @@ -32,10 +28,6 @@ type Config struct {
EventCh chan<- interface{}
// NamespacedNameFilter filters resources the controller will process. Can be nil.
NamespacedNameFilter NamespacedNameFilterFunc
// WebhookValidator validates a resource using the same rules as in the Gateway API Webhook. Can be nil.
WebhookValidator ValidatorFunc
// EventRecorder records event about resources.
EventRecorder EventRecorder
}

// Implementation is a reconciler for Kubernetes resources.
Expand Down Expand Up @@ -66,12 +58,6 @@ func newObject(objectType client.Object) client.Object {
return reflect.New(t).Interface().(client.Object)
}

const (
webhookValidationErrorLogMsg = "Rejected the resource because the Gateway API webhook failed to reject it with " +
"a validation error; make sure the webhook is installed and running correctly; " +
"NKG will delete any existing NGINX configuration that corresponds to the resource"
)

// Reconcile implements the reconcile.Reconciler Reconcile method.
func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
logger := log.FromContext(ctx)
Expand All @@ -98,22 +84,10 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) (
obj = nil
}

var validationError error
if obj != nil && r.cfg.WebhookValidator != nil {
validationError = r.cfg.WebhookValidator(obj)
}

if validationError != nil {
logger.Error(validationError, webhookValidationErrorLogMsg)
r.cfg.EventRecorder.Eventf(obj, apiv1.EventTypeWarning, "Rejected",
webhookValidationErrorLogMsg+"; validation error: %v", validationError)
}

var e interface{}
var op string

if obj == nil || validationError != nil {
// In case of a validation error, we handle the resource as if it was deleted.
if obj == nil {
e = &events.DeleteEvent{
Type: r.cfg.ObjectType,
NamespacedName: req.NamespacedName,
Expand Down
Loading