Removal of linkerd.io/helm-release-version annotation

[naseemkullah]

Hello,

I am looking for any objections to the following before opening a PR.

I'd like to propose removing of what I suspect to be clutter, unless a reason for having this annotation can be provided. The annotation in question is linkerd.io/helm-release-version whose value is the helm revision (run helm ls to compare).

The reason I would like to remove it is that it pollutes helm diff outputs as follows in the various places where it exists.

      matchLabels:
        linkerd.io/control-plane-component: proxy-injector
    template:
      metadata:
        annotations:
-         linkerd.io/helm-release-version: "29"
+         linkerd.io/helm-release-version: "1"
          linkerd.io/created-by: linkerd/helm stable-2.7.1
          linkerd.io/identity-mode: default
          linkerd.io/proxy-version: stable-2.7.1
        labels:
          linkerd.io/control-plane-component: proxy-injector
          linkerd.io/control-plane-n

Thanks! ✌️

[grampelberg]

Sounds reasonable to me.

[naseemkullah]

Thanks for confirming @grampelberg!

[alpeb]

Those annotations were introduced in #3673 to effectively have those templates always change upon each helm update operation, and force those services to be restarted. We need them to restart so that they pick their associated certificates that get generated anew upon each redeployment.

[muffl0n]

This is unnecessary when using cert-manager, right? Is there a way to disable it? Gives us a "release has changed" warning on every diff run we make. Effectively "breaking" our CI.

[naseemkullah]

ah thanks @alpeb, they do serve a purpose then and we cannot simply remove them

[rootik]

In a CI/CD for k8s components deployment scenario with helmfile it leads to unwanted behavior when linkerd pods got restarted on each pipeline run even nothing in the state has changed. What workaround would you propose?

[philomory]

I have no idea why it works for you with helm diff in the first place, I'm absolutely unable to reproduce the behavior you're getting:

$ # without HELM_DIFF_USE_UPGRADE_DRY_RUN
$ helm get values -n linkerd linkerd-control-plane | helm diff upgrade -n linkerd linkerd-control-plane linkerd/linkerd-control-plane -f - | grep helm-release-version
-         linkerd.io/helm-release-version: "5"
+         linkerd.io/helm-release-version: "1"
-         linkerd.io/helm-release-version: "5"
+         linkerd.io/helm-release-version: "1"
$ # with HELM_DIFF_USE_UPGRADE_DRY_RUN
$ export HELM_DIFF_USE_UPGRADE_DRY_RUN=true
$ helm get values -n linkerd linkerd-control-plane | helm diff upgrade -n linkerd linkerd-control-plane linkerd/linkerd-control-plane -f - | grep helm-release-version
-         linkerd.io/helm-release-version: "5"
+         linkerd.io/helm-release-version: "6"
-         linkerd.io/helm-release-version: "5"
+         linkerd.io/helm-release-version: "6"

[philomory]

@Fantaztig The only way I can I see this actually working for you is if you're actually supplying a value for the cliVersion key even though you're using deploying via the helm chart; since if that key is set, then .Release.Revision doesn't get included in the the rendered template. But as far as I can tell, that's an undocumented implementation detail that exists only to support deploying with the linkerd cli tool (which uses the helm chart "under the hood").

Edit: Note that helm template (which helm diff uses by default) always assumes .Release.Revision will be 1, so if you've only just installed the linkerd-control-plane chart, and you run helm diff, then the actual in-cluster .Release.Revision will be 1, and the one produced by helm template will be 1, and so there will be no diff... until you change the configuration or upgrade to a newer linkerd version, at which point the in-cluster .Release.Revision will be 2, but the one produced by helm template will still be 1, so helm diff will from that point forward always show a diff.

[Fantaztig]

Ah yes, nevermind. I wasn't aware that this only applies to the latest version of linkerd. So yes, this affects us as well and it doesn't seem to make any proper sense to use a custom annotation to store the helm revision.

[philomory]

The ostensible reason is to ensure that components restart to pick up their new certificates, since if you don't supply your own certificates, and don't use something like cert-manager to manage them out-of-band, then, the Helm Chart just generates them for you, and because of the way that process works, they get regenerated each time you run Helm.

Which... fine. But that logic should only apply if you are letting the chart generate the certificates for you. If you're supplying them explicitly, or managing them with a solution like cert-manager, then there is absolutely no need for a restart or a constant diff.

[rootik]

It turned out that the root cause was these lines in the helm chart linkerd.io/helm-release-version: {{.Release.Revision | quote}}
If anything has changed in any helm object of the chart, helmfile apply runs helm install under the hood which resets the revision number to 1 so all chart objects become changed.