--three-way-merge produces useless diff on meta.helm.sh annotations

[mumoshu]

Running diff upgrade --three-way-merge added via #304 on an unchanged release produces diffs on meta.helm.sh/release-name and meta.helm.sh/release-namespace. I guess this should be omitted from the diff as it's added automatically by helm AND running helm upgrade won't actually remove those annotations.

$ HELM_NAMESPACE=default HELM_BIN=helm372 ./helm-diff upgrade envoy1 stable/envoy --three-way-merge
default, envoy, Service (v1) has changed:
  apiVersion: v1
  kind: Service
  metadata:
-   annotations:
-     meta.helm.sh/release-name: envoy1
-     meta.helm.sh/release-namespace: default
    creationTimestamp: "2022-01-08T11:00:49Z"
    labels:
      app: envoy
      app.kubernetes.io/managed-by: Helm
      chart: envoy-1.9.4
      heritage: Helm
      release: envoy1

@luxurine @Temikus Have you experienced this? Would you agree that this should be fixed somehow?

[TBBle]

It's not game-breaking, but it's a bit of a wart. I guess we could explicitly strip those two annotations from objects fetched from the cluster. Although then if someone has included them in the chart templates by-hand (unlikely, but not impossible, nor invalid), I guess they'd show up in the merge again, as 'drift'.

So stripping them from all three sets of resources (source, current, target) is probably feasible, and doesn't immediately jump out with any downsides.

Worth noting, the app.kubernetes.io/managed-by: Helm label is handled the same way, but because it predates the auto-added metadata, it's present in most people's Helm templates, which is why you're not seeing it in this test-case.

So handling all three of these Helm-added metadata items in one fix is a good way to observe and validate both cases.

[mumoshu]

@TBBle Thank you so much for your feedback! I've enhanced helm-diff to ignore diff on meta.helm.sh annotations #331.

I didn't actually see any unnecessary diff on app.kubernetes.io/managed-by: Helm perhaps due to the chart I used for testing didn't have app.kubernetes.io/managed-by templates in it.

I'd definitely do the same care on the label as soon as I or anyone have seen the real instance of the problem.

[TBBle]

You're very welcome.

I think you aren't seeing it in the envoy chart (assuming this is the same one you tested with) because it does include explicit app.kubernetes.io/managed-by: {{ .Release.Service }} in its envoy.labels named template, so ends up in all its generated manifests, and the code I linked becomes a no-op on that annotation.

If you removed that line from the template, the resulting objects from helm install or helm upgrade would be identical, but you'd see in helm-diff that it's no longer present in the generated manifests.

However, the documented Helm best practice is still to explicitly include that label in your template, which keeps back-compatiblity with charts that predate the newer meta.helm.sh annotations. That's also what helm create outputs, so any charts created from that flow will have this annotation in their templates.

This means you probably won't see charts in the wild that lack this annotation very often, but I have encountered a few, such as https://github.com/Addyvan/valheim-k8s/tree/main/chart. It's not incorrect to not have those standard annotations, just unusual.

[mumoshu]

@TBBle I appreciate your detailed response very much! Good to know- so it was the opposite of what I was thinking.

FWIW, I tried running diff with the three-way-merge option against valheim-k8s and surprisingly it didn't show diff on the managed-by label:

$ HELM_NAMESPACE=default HELM_BIN=helm372 ./helm-diff upgrade valheim-server valheim-k8s/valheim-k8s --three-way-merge --set image.tag=1
default, valheim-server, Deployment (apps) has changed:
  apiVersion: apps/v1
  kind: Deployment
  metadata:
    annotations:
      deployment.kubernetes.io/revision: "1"
    creationTimestamp: "2022-01-09T07:38:53Z"
-   generation: 1
+   generation: 2
    labels:
      app.kubernetes.io/managed-by: Helm
    name: valheim-server
    namespace: default
    resourceVersion: "7128693"
    uid: 9bbbfb7a-fd34-4488-8e88-f4867aacd785
  spec:
    progressDeadlineSeconds: 600
    replicas: 1
    revisionHistoryLimit: 10
    selector:
      matchLabels:
        app: valheim-server
    strategy:
      type: Recreate
    template:
      metadata:
        creationTimestamp: null
        labels:
          app: valheim-server
      spec:
        containers:
        - env:
          - name: SERVER_NAME
            value: example-server-name
          - name: WORLD_NAME
            value: example-world-name
          - name: SERVER_PASS
            value: password
          - name: SERVER_PORT
            value: "2456"
-         image: lloesche/valheim-server:latest
+         image: lloesche/valheim-server:1
          imagePullPolicy: Always
          name: valheim-server
          ports:
          - containerPort: 2456
            name: gameport
            protocol: TCP
          - containerPort: 2457
            name: queryport
            protocol: TCP
          resources: {}
          terminationMessagePath: /dev/termination-log
          terminationMessagePolicy: File
          volumeMounts:
          - mountPath: /config
            name: gamefiles
          - mountPath: /opt/valheim
            name: serverfiles
        dnsPolicy: ClusterFirst
        restartPolicy: Always
        schedulerName: default-scheduler
        securityContext: {}
        terminationGracePeriodSeconds: 30
        volumes:
        - hostPath:
            path: /data/valheim
            type: DirectoryOrCreate
          name: gamefiles
        - hostPath:
            path: /data/valheim-server
            type: DirectoryOrCreate
          name: serverfiles

I also ran helm template valheim-server valheim-k8s/valheim-k8s and helm get release valheim-server and saw there's no managed-by label in any resources in the outputs.

The diff was okay even if I reverted #331. This seems to imply that there's a slightly different edge-case than we've discussed so far.

[mumoshu]

FWIW, the envoy chart I used when testing is https://github.com/helm/charts/blob/master/stable/envoy.

With that and before #331, helm-diff + three-way-merge detected diff on the service only even though there are other resources like deployment and a configmap!

After rereading templates in the envoy chart, I realized that the service was the only resource that had non-empty annotations rendered by the template:

apiVersion: v1
kind: Service
metadata:
  name: {{ .Values.service.name }}
  labels:
    app: {{ template "envoy.name" . }}
    chart: {{ template "envoy.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
  annotations:
{{- with .Values.service.annotations }}
{{ toYaml . | indent 4 }}
{{- end }}

where templates for the other resources lacked the whole annotations:

apiVersion: v1
kind: ConfigMap
metadata:
  name: {{ template "envoy.fullname" . }}
  labels:
    app: {{ template "envoy.name" . }}
    chart: {{ template "envoy.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}

apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ template "envoy.fullname" . }}
  labels:
    app: {{ template "envoy.name" . }}
    chart: {{ template "envoy.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}

apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
  name: {{ template "envoy.fullname" . }}
  labels:
    app: {{ template "envoy.name" . }}
    chart: {{ template "envoy.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}

What I can guess from this example is that any template has non-empty annotations results in unintended diff on meta.helm.sh annotations only. On the other hand, non-empty labels in the template doesn't result in such diff, which is odd.

[TBBle]

Huh, that is interesting. Clearly it's not as simple as I'd assumed, and since you didn't see the behaviour I expected, I'm making a faulty assumption somewhere. Thank you for confirming that.

This might be an aspect of the three-way merge logic, where if the annotations section is completely missing in the template, the patch logic doesn't even look inside it, as it won't touch it anyway, and so it's not seen by helm-diff.

But I'm still guessing here, so if it's doing the right thing with #331 applied, that's the best outcome. ^_^

[mumoshu]

For posterity's sake, we're going to remove deployment.kubernetes.io/revision annotation and metadata.generation as well from the diff. Please see issue #349 and PR #359 for more information.