feat(chart): add crds.install flag to gate DNSEndpoint CRD installation - #6595
feat(chart): add crds.install flag to gate DNSEndpoint CRD installation#6595ausias-armesto wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 @ausias-armesto! |
|
Hi @ausias-armesto. 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 updates the charts/external-dns Helm chart to support multi-release-per-cluster deployments by introducing a value gate for installing the cluster-scoped DNSEndpoint CRD.
Changes:
- Adds
.crds.install(defaulttrue) to control whether theDNSEndpointCustomResourceDefinitionis rendered/installed. - Moves the
DNSEndpointCRD manifest intotemplates/and wraps it with{{- if .Values.crds.install -}}. - Adds Helm unittest coverage and updates generated chart documentation/schema and the chart changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| charts/external-dns/values.yaml | Introduces the new .crds.install value with default true. |
| charts/external-dns/values.schema.json | Adds schema for the new crds.install value. |
| charts/external-dns/tests/crds_test.yaml | Adds unit tests ensuring the CRD is present by default and omitted when disabled. |
| charts/external-dns/templates/dnsendpoint-crd.yaml | Renders the CRD conditionally based on .Values.crds.install and adds labels. |
| charts/external-dns/README.md | Documents the new crds.install value in the values table. |
| charts/external-dns/CHANGELOG.md | Adds an UNRELEASED entry for the new .crds.install value. |
Suppressed comments (2)
charts/external-dns/templates/dnsendpoint-crd.yaml:7
- Moving the CRD into
templates/means Helm will treat it like a normal manifest and (by default) delete it onhelm uninstall. Since deleting a CRD also deletes all its CRs, this is a high-risk operational change compared to the priorcrds/behavior. Consider addinghelm.sh/resource-policy: keepso uninstalling the release does not remove the CRD (users can still delete it manually if desired).
This issue also appears on line 7 of the same file.
charts/external-dns/templates/dnsendpoint-crd.yaml:10
- The CRD is cluster-scoped and shared across releases. Using
external-dns.labelshere injects release-specific and version-specific labels (e.g.app.kubernetes.io/instanceandhelm.sh/chart), which can cause label "flapping" if multiple releases ever render this CRD and also forces a CRD patch on every chart version bump. Consider removing these labels (or using only stable, non-release-specific labels).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
Clusters running multiple external-dns releases from this chart (e.g. one release per DNS provider) get the DNSEndpoint CRD applied by every release, since it's unconditionally bundled under crds/. This makes ArgoCD (and similar GitOps tools) see the same CRD resource emitted more than once within a single Application, which errors instead of just being a warning. Move the CRD out of the crds/ directory into templates/, gated by a new .crds.install value (default true, preserving current behavior). Set it to false on all but one release to install the CRD exactly once. Note: because the CRD is now rendered from templates/, Helm may attempt to update it on upgrade and will delete it on uninstall unless it is marked with helm.sh/resource-policy: keep.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
charts/external-dns/CHANGELOG.md:23
- This changelog entry says the CRD is "now rendered from templates/", but in this PR the CRD is already in
templates/dnsendpoint-crd.yamland the change is to gate rendering behind.Values.crds.install. Consider rewording to avoid implying the CRD was moved intotemplates/by this change (and to avoid overstating a behavioral change that may have existed previously).
- Add value `.crds.install` to control whether the `DNSEndpoint` `CustomResourceDefinition` is installed. Defaults to `true`; set to `false` on all but one release when running multiple `external-dns` releases (e.g. one per provider) in the same cluster to avoid the CRD being installed more than once. Note: because the CRD is now rendered from `templates/`, Helm may attempt to update it on upgrade and will delete it on uninstall unless it is marked with `helm.sh/resource-policy: keep`.
8f182da to
5cf2b81
Compare
|
Previous attempts
When it was added #4322 Supported with --skip-crd flag. At its core, Helm is a templating engine. Helm's CRD handling is deliberately minimal, and it's a known, longstanding limitation (not a bug) rooted in a design decision:
Helm treats CRDs as "install-time, hands-off-after-that" objects, favoring predictability and avoiding destructive cascades over convenience - which is why most projects (including this chart) ship CRDs in the special crds/ folder and document that upgrades/removals are a manual, out-of-band step for the operator. If someone from maintainer team thinks we should switch to CRD templating, no objection to unhold. /hold |
What does it do?
Adds a
.crds.installvalue (defaulttrue) to theexternal-dnschart, gating installation of theDNSEndpointCustomResourceDefinition. The CRD is moved fromcrds/intotemplates/dnsendpoint-crd.yaml, wrapped in{{- if .Values.crds.install -}}.Motivation
When running multiple
external-dnsreleases of this chart in the same cluster (e.g. one release per DNS provider — a common pattern when different Ingress/Service subsets are delegated to different providers), every release installs its own copy of theDNSEndpointCRD via the chart's bundledcrds/directory. Since CRDs are cluster-scoped, this means the identical CRD object is emitted by more than one Helm release.GitOps tools that aggregate multiple releases under one logical application (e.g. ArgoCD via Helmfile) then see the same resource (
CustomResourceDefinition/dnsendpoints.externaldns.k8s.io) appear more than once within a single application's manifest set, which is treated as a comparison error rather than a warning, blocking sync/diff for the whole application.Helm's
crds/directory has no templating support by design, so there's no way to conditionally skip it today. Moving the CRD intotemplates/with a value-gated toggle lets users installing multiple releases setcrds.install: falseon all but one release, so the CRD is applied exactly once, while leaving default single-release behavior unchanged.This follows the same pattern used by several other charts that support multi-release-per-cluster installs (e.g. cert-manager's
crds.enabled), trading Helm's CRD-protection-on-uninstall guarantee (nativecrds/are never deleted/upgraded by Helm) for the ability to toggle installation — a tradeoff that only affects users who explicitly opt in by setting the value.More
helm-docs, values.schema.json regenerated viahelm schema)## [UNRELEASED]perdocs/contributing/chart.md