feat: label the CRD with its managing installation and guard duplicat… - #440
feat: label the CRD with its managing installation and guard duplicat…#440yindia wants to merge 1 commit into
Conversation
✅ Deploy Preview for node-readiness-controller ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yindia 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 |
|
Welcome @yindia! |
|
Hi @yindia. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
|
@ajaysundarkI'm not sure it's the best approach, it false-positives on reinstall since Helm keeps crds/ CRDs after uninstall. Open to a better ideas if anyone has one |
|
On the uninstall/reinstall false positive: The reason Two ways to resolve this without dropping the guard:
|
Summary
Stamps the
NodeReadinessRuleCRD with amanaged-bylabel naming the installation that owns it, and refuses a freshhelm installwhen the CRD already exists.Design notes
Why the CRD carries the label. It's cluster scoped and singleton, so it's the one artifact every install flow contends over. It's set with a controller-gen marker, so
make manifestsregenerates it inconfig/crd/bases, andverify-chart-drift.shdiffs that against the chart'scrds/copy to catch the two drifting apart.Why a template guard, not a pre-install hook. The Helm guard uses
lookup+failin a template — same outcome without a ServiceAccount, ClusterRole or image, and it aborts at render time so the guidance lands in the terminal rather than Job logs.Why it's keyed on
.Release.IsInstallplus CRD existence, not the label value. Both installs may be carrying the default value, and that's precisely the accident being guarded against, so a value comparison would pass straight through it. Upgrades skip the check naturally.Two things worth a reviewer's attention
Label is CRD-only. Helm 3 forces
managed-by: Helmon resources it owns, so stamping the Deployment/RBAC/Services doesn't stick. The CRD works because Helm neither templates nor trackscrds/.Guard only covers
helm install.kubectl apply, kustomize and the static-pod path have none. A controller startup check would close the gap, but it's ruled out by alternative (1) in the issue — left out and documented. Happy to reconsider.Also included
Chart-rendered
NodeReadinessRuleCRs now get the standard chart labels, which they were missing. Unrelated tidy-up, happy to split it out.Related Issue
Fixes #438
Type of Change
/kind feature
Testing
helm installinto a clean cluster: CRD is created with app.kubernetes.io/managed-by=node-readiness-controller.helm installwith a second release name while the CRD exists: render aborts with the guidance message, nothing is applied.helm upgradeon the existing release: check is skipped, upgrade succeeds.helm templateoutput diffed againstdist/crds.yaml;verify-chart-drift.shpasses.Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?
Yes