feat!: update Helm chart values and schema

[xakraz]

• Update version from 1.15.2 to 2.0.0 in Chart.yaml
• Add externalDns section and move external-dns specific properties in values.schema.json
• Add externalDns section in values.yaml

[linux-foundation-easycla]

• ✅login: xakraz / (95a1474)

The committers listed above are authorized under a signed CLA.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign stevehipwell for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• charts/OWNERS

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

[k8s-ci-robot]

Welcome @xakraz!

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 @xakraz. 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-sigs/prow repository.

[stevehipwell]

@xakraz sorry I'm unsure as to the context of this PR as I can't see an issue or discussion about why the chart needs a breaking change?

[xakraz]

Hi @stevehipwell ,

We use Helmper to cache public Helm charts and related container images.

Helmper is able to "patch" the default values file before pushing the chart to the private registry. Among the properties it updates is the one named registry, which turns out to also be an external-dns configuration property.

We thought it would be easier to submit a PR for updating this chart and make it like many other popular charts rather than updating Helmper 😇

The change suggested is to introduce a top-level key for external-dns specific configuration and move every existing ones in the values.yaml under that key. I thought it will be a breaking one but I am open to any suggestions 😁

P.S. we suggested such kinds of changes in other projects like stakater/Reloader#849 😇

[stevehipwell]

@xakraz I don't think that heavily modifying an extensively used community chart that is based on an idiomatic Helm pattern is an appropriate solution to a problem with a niche consumer that is still in beta.

The fact that the Helmer implementation collides with existing chart values sounds like something that needs fixing on their side, not in every Helm chart. It sounds like simple case of name spacing would solve this (e.g. helmer.registry).

[xakraz]

@xakraz I don't think that heavily modifying an extensively used community chart that is based on an idiomatic Helm pattern is an appropriate solution to a problem with a niche consumer that is still in beta.

The fact that the Helmper implementation collides with existing chart values sounds like something that needs fixing on their side, not in every Helm chart. It sounds like simple case of name spacing would solve this (e.g. helmper.registry).

I do understand your consideration about making a breaking change on a popular Helm chart.

However, I would like to highlight some things:

1. The more I use Helm charts, the more I think it is very confusing to have that many properties at the root level of the values.yaml document. Mixing Helm-specific variables, Kubernetes manifests specs properties and the software's own configuration is a mess. You mentioned name-spacing, that is exactly what I am suggesting 😇
2. Helm values Yaml document schema is un-speced since its beginning, and therefore idiomatic. Nevertheless you might admit that the registry property name makes sense in the area of Helm charts or container image artifact storage.
3. Moreover, Helmper might still be in beta, nonetheless, on its documentation page (that I shared previously), it is mentioned that it has been tested against other popular Helm charts: Prometheus, Promtail, Loki, Mimir-Distributed, Grafana, Cilium, Cert-Manager, Ingress-Nginx, Reflector, Velero, Kured, Keda, Trivy-Operator, Kubescape-Operator, ArgoCD, ... It seems that for these charts, the registry property being related to artifacts storage is an "ok" idiomatic convention.
4. And finally, introducing a Helmper-specific namespace for this usage seems less generic than the other way around 🙃

Considering these 4 items in total, I think it is a shame to avoid such a change, especially on a Helm chart, because it is "breaking". Otherwise, from then on, we won't make any breaking changes. And making changes to the packaging system
that is a few template files seems less "impactful" than on the upstream software 😁

[ivankatliarchuk]

The external-dns Helm chart is designed to work flawlessly with standard deployment tools like Helm, ArgoCD, Flux, and testing frameworks like helm-unittest, without any modifications. If you're finding modifications necessary, it suggests a potential incompatibility or misconfiguration with the tool you're referencing. In such cases, exploring alternative Helm charts, like the one from Bitnami, or creating a custom chart might be what you are looking for.

If you're referring to 'idiomatic conventions' that make sense, please propose them as additions to the Helm best practices documentation first, at
https://helm.sh/docs/chart_best_practices/.

At the moment, how this pr looks like;

• removed extra spaces. yeah something we missed 100%
• The remaining changes appear to be more aligned with an App-of-Apps pattern. And not clear what is the benefit in this chart

[stevehipwell]

@xakraz Helm has zero constraints on what can be used as a value key as the values are accessed via the .Values parent that makes sure these are namespaced away from other implementation details such as the keys under .Release. A tool that wants to operate in the wider Helm ecosystem cannot add arbitrary constraints as that will make it incompatible with any charts not modified to support it. Helm and it's idioms aren't perfect, but they're in wide use.

Have you engaged with the Helmer maintainers on your issue? You're going to have far more chance of success fixing the singular source of the incompatibility rather than attempting to modify the many unrelated projects that you want to consume.

[k8s-ci-robot]

PR needs rebase.

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-sigs/prow repository.

[xakraz]

I agree with both of you regarding your statements about this Helm chart design and Helm's pros and flaws.
But I still don't get why these prevent such a change.

@ivankatliarchuk you say

If you're finding modifications necessary, it suggests a potential incompatibility or misconfiguration with the tool you're referencing.
"incompatibility", sure that is why I am suggesting a change 😇

Regarding the alternative you suggest, the long-term impact of having a "forked" version or using a chart other than the one from the upstream project seems far greater. Still, thanks for the suggestions 😄

The remaining changes appear to be more aligned with an App-of-Apps pattern

I don't get this one ^^

@stevehipwell

Have you engaged with the Helmer maintainers on your issue? You're going to have far more chance of success fixing the singular source of the incompatibility rather than attempting to modify the many unrelated projects that you want to consume.

Yes, I have 😁

The thing is, Helmper tries to extract container image references from Helm Charts values.yaml file based on conventions, which are also used by many other Helm charts.

And as you said, Helm has zero constraints on what can be used as a value key, so it will always be a story of conventions 🤷🏻

Regarding us attempting to modify many unrelated projects that we want to consume wiht Helmper, mots of the charts we use don't have any issues. Here, the issue comes from external-dns having a configuration property named registry as it is commonly used in ohter charts to refer to the container images' one.

Beside being a breaking change, that semver helps us to deal with, and not providing direct value to the chart itself, I am wondering what makes it a blocker to "group" external-dns's configuration properties under one key in the values.yaml document.

As I said, at least for new commers, it will be far easier to understand. It is already done that way in many charts

• https://github.com/cert-manager/cert-manager/blob/master/deploy/charts/cert-manager/values.yaml#L259
• https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/values.yaml#L56
• https://github.com/grafana/helm-charts/blob/main/charts/grafana/values.yaml#L857
• ...

In the end, I understand that you are not fond of this idea, so I will try to find another way.
Bad luck 😕

Feel free to close the issue if that is your final thought.

[ivankatliarchuk]

/close

[k8s-ci-robot]

@ivankatliarchuk: Closed this PR.

In response to this:

/close

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-sigs/prow repository.