Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: flyzstu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @flyzstu. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
This PR extends the helm/v2-alpha plugin鈥檚 chart generation so admission webhook configuration (namespace/object selectors, CEL matchConditions, and timeoutSeconds) can be driven via Helm values, while attempting to keep any webhook fields already present in the Kustomize/controller-tools output authoritative. It also updates the Helm plugin documentation and regenerates the chart fixtures used in testdata/docs.
Changes:
- Add a new Helm templater applier to inject webhook admission settings templates into
ValidatingWebhookConfiguration/MutatingWebhookConfigurationmanifests. - Extend generated
values.yamlwithwebhook.namespaceSelector,webhook.objectSelector,webhook.matchConditions, andwebhook.timeoutSeconds. - Document the new values and update regenerated chart testdata in
testdata/anddocs/book/tutorial fixtures.
Reviewed changes
Copilot reviewed 6 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/project-v4-with-plugins/dist/chart/values.yaml | Adds new webhook.* values for selectors, match conditions, and timeout. |
| testdata/project-v4-with-plugins/dist/chart/templates/webhook/validating-webhook-configuration.yaml | Renders the new webhook admission fields from Helm values. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/templates/values.go | Updates the values.yaml generator to emit the new webhook values. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/templater.go | Invokes the new webhook templating applier for webhook configuration resources. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/templater_test.go | Adds a templater-level test asserting the new values are referenced. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/appliers/webhook.go | New applier that injects templated admission settings into webhook configs. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/appliers/webhook_test.go | New unit test for the webhook applier behavior. |
| docs/book/src/plugins/available/helm-v2-alpha.md | Documents the new webhook admission configuration values and usage. |
| docs/book/src/multiversion-tutorial/testdata/project/dist/chart/values.yaml | Regenerated chart values fixture reflecting new webhook values. |
| docs/book/src/multiversion-tutorial/testdata/project/dist/chart/templates/webhook/validating-webhook-configuration.yaml | Regenerated validating webhook template fixture with new fields. |
| docs/book/src/multiversion-tutorial/testdata/project/dist/chart/templates/webhook/mutating-webhook-configuration.yaml | Regenerated mutating webhook template fixture with new fields. |
| docs/book/src/cronjob-tutorial/testdata/project/dist/chart/values.yaml | Regenerated chart values fixture reflecting new webhook values. |
| docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/webhook/validating-webhook-configuration.yaml | Regenerated validating webhook template fixture with new fields. |
| docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/webhook/mutating-webhook-configuration.yaml | Regenerated mutating webhook template fixture with new fields. |
| var additions strings.Builder | ||
| if !strings.Contains(yamlContent, " namespaceSelector:") { | ||
| additions.WriteString(" {{- with .Values.webhook.namespaceSelector }}\n") | ||
| additions.WriteString(" namespaceSelector:\n") | ||
| additions.WriteString(" {{- toYaml . | nindent 4 }}\n") | ||
| additions.WriteString(" {{- end }}\n") | ||
| } | ||
| if !strings.Contains(yamlContent, " objectSelector:") { | ||
| additions.WriteString(" {{- with .Values.webhook.objectSelector }}\n") |
There was a problem hiding this comment.
Fixed in 44e7cc2. TemplateWebhookConfiguration now processes each top-level webhook entry independently instead of using whole-document field checks. A field configured on one webhook no longer suppresses Helm templating for the other entries.
| if !strings.Contains(yamlContent, " timeoutSeconds:") { | ||
| additions.WriteString(" timeoutSeconds: {{ .Values.webhook.timeoutSeconds }}\n") | ||
| } |
There was a problem hiding this comment.
Fixed in 44e7cc2. timeoutSeconds is now rendered inside an optional with block. If the value is removed from values.yaml, the field is omitted and Kubernetes can apply its default instead of receiving an invalid value.
| import ( | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestTemplateWebhookConfiguration(t *testing.T) { |
There was a problem hiding this comment.
Fixed in 44e7cc2. Replaced the standalone testing.T test with a Ginkgo spec to match the existing appliers test suite style.
| configured := `apiVersion: admissionregistration.k8s.io/v1 | ||
| kind: ValidatingWebhookConfiguration | ||
| webhooks: | ||
| - name: validation.example.com | ||
| namespaceSelector: | ||
| matchLabels: | ||
| webhook: enabled | ||
| rules: | ||
| - apiGroups: | ||
| - example.com | ||
| ` | ||
| configuredResult := TemplateWebhookConfiguration(configured) | ||
| if strings.Count(configuredResult, " namespaceSelector:") != 1 { | ||
| t.Fatalf("expected an existing namespaceSelector to be preserved without duplication:\n%s", configuredResult) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 44e7cc2. Added a regression case with two webhook entries: one already configured by a patch marker and one unconfigured. The test verifies that the unconfigured entry still receives Helm templates while the existing field is not duplicated.
| @@ -17,6 +17,21 @@ webhooks: | |||
| path: /mutate-batch-tutorial-kubebuilder-io-v1-cronjob | |||
There was a problem hiding this comment.
We need to wait for the release of the new version of controller-tools
Then note that the changes are in the makers and the markers will produce the files with those
We will need to use those on tests and mocks
We will need to update the docs
We will need to test it indeed with e2e tests.
And we will need to check how to expose in the helm.
So, I think we will need to wait here
There was a problem hiding this comment.
Lets hold for now and wait the changes be released then we can see how we can do that
/hold
|
Hi @flyzstu We need this one but could you please get the latest changes and rebase it since that will be impacted by. |
Summary
namespaceSelector,objectSelector,matchConditions, andtimeoutSecondsthrough Helm values.patchmarkers.Fixes #5758
Implementation
controller-tools v0.21.0 includes the webhook patch marker support from controller-tools#1339. This change keeps those Kustomize-generated fields authoritative while adding Helm value templates for fields that are not already present in the source webhook configuration.
Validation
make install && make generatemake lint-fixmake lintmake test-unitmake verify-helmhelm templatewith custom selectors and timeout values