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.

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

Run webhook validation rules inside NKG control plane #388

Merged
merged 9 commits into from
Jan 24, 2023

Conversation

pleshakov
Copy link
Contributor

Proposed changes

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 #362

Example of an event for an invalid HTTPRoute:

  Warning  Rejected  3m31s              nginx-kubernetes-gateway-nginx  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; validation error: spec.rules[0].matches[0].path.value: Invalid value: "//coffee": must not contain "//"

Example of a error log messages for the same HTTPRoute:

{"level":"error","ts":"2023-01-19T19:08:19Z","msg":"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","controller":"httproute","controllerGroup":"gateway.networking.k8s.io","controllerKind":"HTTPRoute","HTTPRoute":{"name":"coffee","namespace":"default"},"namespace":"default","name":"coffee","reconcileID":"3be665e7-3ced-40bc-9a89-91a41182fd50","error":"spec.rules[0].matches[0].path.value: Invalid value: \"//coffee\": must not contain \"//\"","errorCauses":[{"error":"spec.rules[0].matches[0].path.value: Invalid value: \"//coffee\": must not contain \"//\""}],"stacktrace":"github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler.(*Implementation).Reconcile\n\tgit.luolix.top/nginxinc/nginx-kubernetes-gateway/internal/reconciler/implementation.go:115\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile\n\tsigs.k8s.io/controller-runtime@v0.14.1/pkg/internal/controller/controller.go:122\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\tsigs.k8s.io/controller-runtime@v0.14.1/pkg/internal/controller/controller.go:323\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\tsigs.k8s.io/controller-runtime@v0.14.1/pkg/internal/controller/controller.go:274\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\tsigs.k8s.io/controller-runtime@v0.14.1/pkg/internal/controller/controller.go:235"}

@pleshakov pleshakov added the enhancement New feature or request label Jan 19, 2023
@pleshakov pleshakov requested a review from a team as a code owner January 19, 2023 19:19
@kate-osborn
Copy link
Contributor

kate-osborn commented Jan 19, 2023

Screen Shot 2023-01-19 at 2 07 46 PM

Is `nginx-kubernetes-gateway-nginx` a typo?

@pleshakov
Copy link
Contributor Author

@kate-osborn

That comes from

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

which is to generate a unique name in case multiple NKGs are in the cluster. But it looks for sure confusing.

Perhaps nginx-kubernetes-gateway-with-gatewayclass-nginx ?

internal/manager/manager.go Outdated Show resolved Hide resolved
internal/manager/manager.go Show resolved Hide resolved
internal/manager/validators_test.go Outdated Show resolved Hide resolved
internal/reconciler/implementation.go Show resolved Hide resolved
internal/reconciler/implementation.go Outdated Show resolved Hide resolved
internal/reconciler/implementation.go Outdated Show resolved Hide resolved
internal/reconciler/implementation.go Outdated Show resolved Hide resolved
internal/reconciler/implementation.go Outdated Show resolved Hide resolved
internal/reconciler/implementation.go Outdated Show resolved Hide resolved
@kate-osborn
Copy link
Contributor

@kate-osborn

That comes from

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

which is to generate a unique name in case multiple NKGs are in the cluster. But it looks for sure confusing.

Perhaps nginx-kubernetes-gateway-with-gatewayclass-nginx ?

Hmm. Let's just keep it as it is for now.

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 #362
@pleshakov pleshakov merged commit e360645 into main Jan 24, 2023
@pleshakov pleshakov deleted the feature/webhook-validation branch January 24, 2023 22:51
@pleshakov pleshakov changed the title Run webhook validation Run webhook validation rules inside NKG control plane Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Run webhook validation
2 participants