fix: Add Missing Flags as Helm Values #3787
Conversation
Signed-off-by: Ian Stanton <[email protected]>
Signed-off-by: Ian Stanton <[email protected]>
Signed-off-by: Ian Stanton <[email protected]>
Signed-off-by: Ian Stanton <[email protected]>
Signed-off-by: Ian Stanton <[email protected]>
| volumeMounts: | ||
| - mountPath: /certs | ||
| name: cert | ||
| readOnly: true | ||
| $patch: delete |
There was a problem hiding this comment.
I added these here to avoid duplicate entries when generating manifests. I renamed the file as it is no longer only deleting ports.
| patchesStrategicMerge: | ||
| - kustomize-for-helm.yaml | ||
| - delete-ports.yaml | ||
| - delete-merge-values.yaml |
There was a problem hiding this comment.
Renamed file mentioned above.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #3787 +/- ##
===========================================
- Coverage 54.49% 40.71% -13.79%
===========================================
Files 134 251 +117
Lines 12329 17720 +5391
===========================================
+ Hits 6719 7215 +496
- Misses 5116 9883 +4767
- Partials 494 622 +128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| - --disable-cert-rotation={{ .Values.controllerManager.disableCertRotation }} | ||
| - --max-serving-threads={{ .Values.maxServingThreads }} | ||
| - --tls-min-version={{ .Values.controllerManager.tlsMinVersion }} | ||
| - --client-ca-name={{ .Values.controllerManager.clientCAName }} |
There was a problem hiding this comment.
All of these flags should be set conditionally only when the helm variable is set. We do not want to set new flags to user deployments during upgrades. Additionally, please add the helm variable information in https://github.com/open-policy-agent/gatekeeper/blob/master/cmd/build/helmify/static/README.md as well to make it visible.
There was a problem hiding this comment.
@ianstanton any update on this? all these flags should be only set conditionally.
|
@ianstanton, hello friend! Can you add some extra missing flag, it is |
@npoxopob Hey there! Sure, I can add that to this PR. |
|
@ianstanton any updates on this? |
I can come back to this over the weekend |
There was a problem hiding this comment.
Pull Request Overview
This PR adds missing command-line flags as configurable Helm values for the Gatekeeper controller, addressing issue #3784. The changes enhance the Helm chart's flexibility by exposing previously hard-coded flags through values.yaml configuration.
- Adds 11 new configurable flags including logging, certificate, caching, and telemetry options
- Updates both controller-manager and audit deployment templates to use these new values
- Implements proper Helm templating patterns with fallback logic for component-specific overrides
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| manifest_staging/charts/gatekeeper/values.yaml | Adds new Helm values for the missing flags with sensible defaults |
| manifest_staging/charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml | Updates controller-manager deployment to use new Helm values for flags and cert directory |
| manifest_staging/charts/gatekeeper/templates/gatekeeper-audit-deployment.yaml | Updates audit deployment to use new Helm values for flags and cert directory |
| cmd/build/helmify/static/values.yaml | Mirror of main values.yaml for Helm chart generation |
| cmd/build/helmify/replacements.go | Adds replacement pattern for cert volume mount path |
| cmd/build/helmify/kustomize-for-helm.yaml | Updates kustomization template with new flags and volume mounts |
| cmd/build/helmify/delete-merge-values.yaml | Removes hard-coded volume mounts to allow Helm templating |
Comments suppressed due to low confidence (1)
| tlsMinVersion: 1.3 | ||
| clientCertName: "" | ||
| clientCAName: "" | ||
| clientCNName: kube-apiserver |
There was a problem hiding this comment.
The clientCNName value should be a string but is missing quotes. This could cause YAML parsing issues. Change to: clientCNName: \"kube-apiserver\"
| clientCNName: kube-apiserver | |
| clientCNName: "kube-apiserver" |
Signed-off-by: Ian Stanton <[email protected]>
4 participants
What this PR does / why we need it:
Add the following flags as helm values and pass down to
controller-managerandauditdeployment templates:--log-level-key--log-level-encoder--cert-dir--api-cache-dir--otlp-endpoint--otlp-metric-interval--stackdriver-only-when-available--stackdriver-metric-interval--disable-enforcementaction-validation--client-ca-name--client-cn-nameWhich issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #3784
Special notes for your reviewer:
I left some of the missing flags in #3784 out as I'm still determining whether they belong as chart values. I also want to be sure the right flags are passed to the appropriate deployments (some are applied to both, others are unique to
controller-manageroraudit), so feedback is welcome there.