-
Notifications
You must be signed in to change notification settings - Fork 4
feat: support webhook server for ingress #213
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]>
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 server support for the ingress controller Helm chart, enabling admission webhook functionality to validate Kubernetes resources. The changes introduce comprehensive webhook configuration options, certificate management, and integration with the deployment.
- Adds webhook configuration section to values.yaml with options for enabling, port, failure policy, timeout, and certificate management
- Creates webhook template with ValidatingWebhookConfiguration for multiple resource types (routes, consumers, TLS, gateway proxies, ingresses, etc.)
- Updates deployment to mount webhook certificates and expose webhook port when enabled
- Modifies RBAC permissions to support webhook operations
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/ingress-controller/values.yaml | Adds webhook configuration section with enable flag, port, policies, and certificate options |
| charts/ingress-controller/templates/webhook.yaml | Creates new webhook template with ValidatingWebhookConfiguration, Service, and Secret resources |
| charts/ingress-controller/templates/deployment.yaml | Updates deployment to mount webhook certificates and expose webhook port conditionally |
| charts/ingress-controller/templates/configmap.yaml | Adds webhook configuration to the config map when webhook is enabled |
| charts/ingress-controller/templates/cluster_role.yaml | Reorganizes RBAC permissions and adds new resource access patterns |
| charts/ingress-controller/README.md | Documents new webhook configuration parameters |
Comments suppressed due to low confidence (1)
charts/ingress-controller/templates/webhook.yaml:1
- When webhook.certificate.provided is true, the code references a hardcoded secret name, but the values.yaml suggests users should provide their own secretName. The template should use .Values.webhook.certificate.secretName when provided is true.
{{- if .Values.webhook.enabled }}
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
| - name: webhook-certs | ||
| mountPath: /certs | ||
| readOnly: true | ||
| ports: |
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.
I suggest moving the ports field outside the conditional block.
If ports is wrapped within the webhook.enabled condition, future contributors adding new container ports might overlook the conditional logic, which could lead to missing or inconsistent port configurations.
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.
ok, i will update it later.
Signed-off-by: Ashing Zheng <[email protected]>
Since this version has not been officially released, I can only test it locally first.
Specific generated ingress-related configurations: