Add dnsendpoint CRD to Helm chart

[onedr0p]

Description

Taken from https://github.com/kubernetes-sigs/external-dns/tree/master/docs/contributing/crd-source

Fixes: #2640

Checklist

• Unit tests updated
• End user documentation updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: onedr0p / name: Devin Buhl (300591f, 0248ab0, c3a6440, b3bc78a, 162b5b6, 92e84ff)

[k8s-ci-robot]

Welcome @onedr0p!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @onedr0p. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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/test-infra repository.

[onedr0p]

@stevehipwell as discussed in #2640 I have put this in the crds folder un-templated.

1. Do we really need approval to include this? The CRD is usable with the application in its current state and is included in this Git repo...
2. If no one can or will answer this question, can we just include it in the Helm chart and move on?
3. Why do we need to wait for the CRD to be GA if it works as is? The whole project itself isn't even GA (still < 1.0.0)

If the maintainers or community decide to remove support or change the CRD, it can be updated as such from the Helm chart moving forward but I see no blockers for adding this right now.

[mloiseleur]

/ok-to-test

[mloiseleur]

It makes sense to me.
/lgtm

[onedr0p]

@mloiseleur should I update the chart version? If so to what version?

[mloiseleur]

Nope. Release of the chart are done in a dedicated PR. See for instance this one.

[stevehipwell]

@onedr0p I've added some review comments and you also need to add an entry to the CHANGELOG for each chart PR.

That said I'm not convinced that adding the CRD to the Helm chart is the right move. Firstly adding a CRD to a _Helm_chart means that by default everyone who installs the chart will also install the CRD; this makes sense where the CRD is required or likely to be required (given the constraints below), but in this case I suspect users of the CRD are in the minority. Secondly as Helm shouldn't be used for managing CRDs (it will only install them and never modify them) adding the CRD would only help the small subset of users who want to use the CRD and are putting together a quick POC (for a non-GA CRD this is even worse). On balance I think adding the CRD makes the chart experience worse for more users than better.

/hold

[onedr0p]

@stevehipwell changes made.

For me, adding the DNSEndpoint CR actually improves the experience since I am using Flux and it can upgrade the CRDs (I suspect this is the same for Argo users). Having the CRD in the chart means I don't need to apply it manually or install it out-of-band of the chart itself.

[stevehipwell]

For me, adding the DNSEndpoint CR actually improves the experience since I am using Flux and it can upgrade the CRDs (I suspect this is the same for Argo users). Having the CRD in the chart means I don't need to apply it manually or install it out-of-band of the chart itself.

@onedr0p in this case Helm isn't managing the CRD and you're just using it as a lookup for Flux (which should be setting the --skip-crds arg), and as you are using the CRD I can see how this does exactly what you want it to do. However anyone not using the CRD and/or not using a tool which can manage the CRDs out of band isn't going to be in the same situation.

[onedr0p]

@stevehipwell I wonder if it would be good to change the upgrade/install command in the charts README to include --skip-crds and/or better document the behavior here in the charts README?

Important

By default this chart will install the DNSEndpoint CRD which isn't GA and some people may wish to not have this installed with the chart. To avoid having the DNSEndpoint CRD installed with the Helm chart add --skip-crds to your Helm install/upgrade command.

Does that make sense?

[stevehipwell]

@onedr0p the more I think about this the less I think we should do this.

In your use case all you'd need to do is add the CRD to your Flux repo as a raw manifest, given that it hasn't changed in a long time it doesn't seem like there is a usability problem to solve here (you could add repo automation to either keep this updated if it ever changed or block if there is a drift).

If the CRD looks like it's going to start changing more regularly then the following options would be a better fit than adding the CRD to the chart.

• Add a seperate CRD chart
• Use a Job to create/update the CRD

[onedr0p]

In your use case all you'd need to do is add the CRD to your Flux repo as a raw manifest

This is what I already do, but it's not ideal. I and others would love for this to be handled by the chart since it's currently a "supported" feature of the application.

I get Helm doesn't make this easy... but it seems like you have some strong biases against adding CRDs to charts in general where a lot of projects include them (to name one of many node-feature-discovery).

I really don't see how adding them here will degrade UX for people using the chart especially if documented but I digress 🤷🏼 It's unfortunate but I can close this PR and never bring it up again if you won't budge. At least the conversation on trying to add the CRDs exists for anyone in the future trying to contribute the same change.

[stevehipwell]

@onedr0p I'm not saying I wont budge; what I'm saying is that adding a CRD to each cluster with ExternalDNS installed where the end user isn't aware of the --skip-crds flag or even the need for it (as this is an existing chart without any CRDs or mention of them in the primary service documentation) doesn't sound great and this is even less defensible given the assumed limited number of people using the CRD (I'm one of these although we manage all of our CRDs external to Helm).

Given the current state of play I think the next step would be to figure out if there is a requirement from enough people to have a Helm style CRD installation method (e.g. CRD chart or Job).

For me the following scenarios would warrant a reevaluation.

• Change of Helm default CRD behaviour
• Explicit recommendation from the core ExternalDNS maintainers to prefer the CRD method
• End user feedback showing a significant number of users using the CRD method

[joryirving]

It feels like your reason not to include the CRD, are assuming that:

1. Users won't read the docs, and therefore you must cater to them
2. A limited amount of people are using the CRD.

I personally find DNSEndpoint pretty indispensable, and would be surprised it isn't commonly used, and trying to cater to the lowest common denominator on something this technical feels like the wrong way to go.

As @onedr0p already stated, other kubernetes-sigs projects have opted to include them, and IMO there's no downside if they're accidentally installed by helm anyways (if you're not going to use DNSEndpoint anyways).

If a chart installs a CRD, and they try to manually manage it externally anyways, they'll find out pretty quickly.

Either way, count me as an End user using the method that would implemented by this PR

[onedr0p]

@stevehipwell

I've read you concerns but I still really don't know what we're trying to protect against here by not merging this PR. It seems we're at a conflict at how managing Charts that contain CRDs should be handled. I and others would like the chart to handle CRDs either by the crds folder or the templated .Values.crds.enabled knob and you have stated that you do not manage any CRDs with Helm even if they are included in the chart. It seems like there is a bit of a bias in your decision here on how CRDs are handled in Helm charts which isn't great for people who want the option to have the Chart include CRDs by either of those "standard" methods.

That other idea to template the CRDs and have a .Values.crds.enabled knob like cert-manager, rook-ceph and many other charts do as well was shot down. At least using the template method (if it was defaulted to false) would get over the "unexpected CRD installed" problem you're concerned with.

I really feel like having a Job or a separate helm chart for just the CRDs is not the right way to go and this is evident by how many application and Helm Chart developers do not actually handle the CRDs this way.

Here's a list of some kubernetes-sigs applications that have Helm charts with crds included by either templated or crds directory and could not find any using the methods you purposed. Are you saying these all need to be refactored to handle CRDs differently or that they are doing it wrong? It seems to work for them, I really don't understand your concerns why it won't work here either.

• https://github.com/kubernetes-sigs/kueue/tree/main/charts/kueue/templates/crd
• https://github.com/kubernetes-sigs/node-feature-discovery/tree/master/deployment/helm/node-feature-discovery/crds
• https://github.com/kubernetes-sigs/secrets-store-csi-driver/tree/main/charts/secrets-store-csi-driver/crds
• https://github.com/kubernetes-sigs/security-profiles-operator/tree/main/deploy/helm/crds
• https://github.com/kubernetes-sigs/node-feature-discovery-operator/tree/master/deploy/helm/nfd-operator/crds
• https://github.com/kubernetes-sigs/karpenter/tree/main/kwok/charts/crds
• https://github.com/kubernetes-sigs/cluster-api-addon-provider-helm/blob/main/helm/cluster-api-addon-provider-helm/templates/helmchartproxy.yaml

From my experience most applications that have CRDs and implement a Helm chart are doing it one of those two ways and not the ways you have described. Please reconsider ❤️

[Ornias1993]

@stevehipwell

• On one hand we've many projects including crds in some way. Which makes users expect projects to include their CRDs
• On the other hand we've your recommendation not to.

I think in this case, one should have a precise and non-general reason to deviate from the standards set by nearly every other project catering a helm-chart.
Most of your arguments, seemingly, are just a rehash of previous general discussions on including crds or not and/or how Helm should handle them. This does not seem like the place, to do this all-over-again.

TLDR, unless there is a good reason against it, +1 on including the CRD as-well.

[mloiseleur]

Explicit recommendation from the core ExternalDNS maintainers to prefer the CRD method

I agree that current status of CRD with Helm CLI is not convenient. CLI of Helm v3 is out there since a long time, and so, CRDs limitations are known by many Helm CLI users. At least, (other) IaC tools handle CRDs of a Chart in a more convenient way.

=> As an external-dns maintainer, I recommend to include this CRD in the Chart.

Considering that this CRD has not been modified in the last 3 years, it makes sense to me to put it in crds/ folder. I don't recommend the Job way, it can quickly become complicated for enduser with this approach.

[buroa]

I agree; it makes sense to include the CRD in the chart. So many others do it this way, and it seems it's the preferred way going forward.

[stevehipwell]

Thanks for your feedback @mloiseleur, if the CRD is effectively a core part of ExternalDNS I see no reason not to include it. Let's get this merged so it can be included in #4357.

@onedr0p @joryirving @Ornias1993 @buroa; I'd like to point out that the argument that "other charts do something, so we should too" isn't a valid one. A lot of Helm charts lack curation and as such often implement functionality before considering the wider context and then are unable to revert the changes. Functionality should be evaluated by merit within the specific context where it's being applied.

[stevehipwell]

Thanks for the PR @onedr0p.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevehipwell

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• charts/OWNERS [stevehipwell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[stevehipwell]

/unhold

[stevehipwell]

/label tide/merge-method-squash

[Skaronator]

It's great to finally see this in the helm chart. I'm finally able to use this "new" CR now.

Maybe it would make sense to enable the Custom Resource service by default?

external-dns/charts/external-dns/values.yaml

Lines 199 to 203 in f5545b1

	# -- _Kubernetes_ resources to monitor for DNS entries.
	sources:
	- service
	- ingress

Currently, the ClusterRole is not allowed to monitor the Custom Resource:

external-dns/charts/external-dns/templates/clusterrole.yaml

Lines 50 to 57 in f5545b1

	{{- if has "crd" .Values.sources }}
	- apiGroups: ["externaldns.k8s.io"]
	resources: ["dnsendpoints"]
	verbs: ["get","watch","list"]
	- apiGroups: ["externaldns.k8s.io"]
	resources: ["dnsendpoints/status"]
	verbs: ["*"]
	{{- end }}

(thanks for pushing it though @onedr0p)

[onedr0p]

@Skaronator I am OK with it not being a default source in the chart as that is easy enough to override.

[Ornias1993]

I'd like to point out that the argument that "other charts do something, so we should too" isn't a valid one. A lot of Helm charts lack curation and as such often implement functionality before considering the wider context and then are unable to revert the changes.

I would like to point out thats essentially a strawman argument.

There is a difference between informal standards set by the industry, by big names as cert-manager, traefik, bitnami, velero etcetcetc and random other helm charts.

No one here pointed out to follow random other helm charts, but the standards set by big names including in the very same repo you're responding on.

[stevehipwell]

@Ornias1993 you realise I maintain this chart? The informal standards you refer to sound and awful lot like cargo culting. None of the charts you referenced would be in my list of "idiomatic" charts to base out of context decisions on.

[lucasfcnunes]

Thanks for adding the CRD to the chart!

--

That other idea to template the CRDs and have a .Values.crds.enabled knob like cert-manager, rook-ceph and many other charts do as well was shot down. At least using the template method (if it was defaulted to false) would get over the "unexpected CRD installed" problem you're concerned with.

@onedr0p
I really think the .Values.crds.enabled approach is more elegant (+ it's updatable).

The informal standards you refer to sound and awful lot like cargo culting. None of the charts you referenced would be in my list of "idiomatic" charts to base out of context decisions on.

cilium/cilium does their CRDs on the chart too and think it's pretty neat and well thought out.