feat(helm): add switch for admission webhook namespaceSelector - #2091
Conversation
Add scheduler.admissionWebhook.manageNamespaceSelector (default true) to let the chart opt out of rendering and managing the webhook namespaceSelector field. On managed platforms such as AKS, the control plane mutates and owns namespaceSelector via admissionsenforcer, which conflicts with Helm server-side apply field ownership and breaks helm upgrade. Setting the value to false leaves the field fully managed by the platform while default behavior remains unchanged. Refs Project-HAMi#2039 Signed-off-by: spencercjh <jiahao.cai@dynamia.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe HAMi Helm chart adds ChangesAdmission webhook selector management
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)charts/hami/templates/scheduler/webhook.yamlTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Helm chart configuration option, manageNamespaceSelector, which allows users to toggle the rendering of the webhook's namespaceSelector field to prevent ownership conflicts on managed Kubernetes platforms like AKS. The review feedback points out a potential breaking change during upgrades: if an existing user upgrades without this new key in their custom values, it will evaluate to falsy and omit the namespace selector. The reviewer suggests a safer conditional check using hasKey to ensure it defaults to true when the key is absent.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/assign mesutoezdil |
/assign mesutoezdil |
With helm upgrade --reuse-values, the previous release's computed values are reused without merging the new chart's values.yaml, so scheduler.admissionWebhook.manageNamespaceSelector resolves to nil and the namespaceSelector block would be silently dropped. Guard with hasKey so the block renders unless the value is explicitly set to false. Refs Project-HAMi#2039 Signed-off-by: spencercjh <jiahao.cai@dynamia.ai>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, spencercjh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Most of this Pull Request was generated or revised with the assistance of AI tools(kimi cli with K3 max). I have reviewed the resulting content and take full responsibility for its accuracy, security, licensing compliance, and inclusion in this project.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds an explicit chart option
scheduler.admissionWebhook.manageNamespaceSelector(defaulttrue) to control whether the chart renders and manages the webhooknamespaceSelectorfield:true(default): keep current behavior and rendernamespaceSelectorexactly as todayfalse: do not rendernamespaceSelector, so the field can be fully managed by the platform / external controllerOn managed platforms such as AKS, the control plane mutates
MutatingWebhookConfigurationand takes ownership ofnamespaceSelectorviaadmissionsenforcer(Azure/AKS#4002). Since the chart currently always renders that field, Helm fights the platform for field ownership and subsequenthelm upgrade/helmfile syncmay fail with a server-side apply conflict. Setting this option tofalselets those environments opt out.Which issue(s) this PR fixes:
Fixes #2039
Special notes for your reviewer:
This change is about field ownership compatibility only; it does not change HAMi webhook semantics by default.
helm templatediff against master is empty).scheduler.admissionWebhook.manageNamespaceSelector=false, the entirenamespaceSelector:block is omitted whileobjectSelectorand all other webhook fields render unchanged.helm lintand thetrivy configchart check (CIlint_chartequivalent) both pass.Does this PR introduce a user-facing change?:
Yes. It introduces an optional chart value for managed Kubernetes environments where the webhook
namespaceSelectoris mutated / owned externally. Default behavior remains unchanged.Summary by CodeRabbit
New Features
Documentation
scheduler.admissionWebhook.manageNamespaceSelectorsetting and its default value.