-
Notifications
You must be signed in to change notification settings - Fork 368
feat: add webhook for ingressclass and gateway #2572
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
Conversation
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds webhook validation for IngressClass and Gateway resources to warn users when referenced GatewayProxy resources do not exist, improving user experience by providing early feedback on configuration issues.
- Implements admission webhooks for IngressClass and Gateway resources that validate GatewayProxy references
- Adds comprehensive end-to-end tests to verify webhook functionality
- Updates webhook configuration manifests to register the new validation endpoints
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/webhook/v1/ingressclass_webhook.go | Implements IngressClass admission webhook with GatewayProxy reference validation |
| internal/webhook/v1/gateway_webhook.go | Implements Gateway admission webhook with GatewayProxy reference validation |
| internal/manager/webhooks.go | Registers the new webhooks with the manager |
| test/e2e/ingress/ingressclass_webhook.go | End-to-end tests for IngressClass webhook validation |
| test/e2e/gatewayapi/webhook.go | End-to-end tests for Gateway webhook validation |
| test/e2e/framework/manifests/webhook.yaml | Test webhook configuration for e2e tests |
| config/webhook/manifests.yaml | Production webhook configuration manifest |
| PROJECT | Kubebuilder project configuration updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type Gateway. | ||
| func (v *GatewayCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { | ||
| gateway, ok := obj.(*gatewaynetworkingk8siov1.Gateway) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we only need to issue warnings for the resources managed by this ingress.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll handle it later
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
| ) | ||
|
|
||
| var _ = Describe("Test Ingress Webhook", Label("networking.k8s.io", "ingress"), func() { | ||
| var _ = Describe("Test Ingress Webhook", Label("webhook"), func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about creating a separate directory for webhooks?
There are different resources inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I think we can handle this matter separately, in the future PR.
Type of change:
What this PR does / why we need it:
A warning is issued when the GatewayProxy resource referenced by Gateway or IngressClass does not exist
Because ValidatingWebhookConfiguration is a cluster resource, running test cases concurrently may lead to other test cases failing (for example, accessing a non-existent webhook server causing errors), so the test cases related to the webhook are tested separately.
Pre-submission checklist: