Putting chart version in metadata.labels in a Deployment v1beta1 without selector prevents upgrade

[desaintmartin]

BUG REPORT

Any Chart containing Deployment/StatefulSet/DaemonSet apps/v1beta1 or extensions/v1beta1 without spec.selector using a chart version in metadata.labels can't be upgraded.

Note:
This is the first bug among 3 regarding "I can't upgrade my Release".
For the apps/v1beta2 problem, see #7680.
For the immutable volumeClaimTemplate in StatefulSet (any version), see #7803

From the k8s documentation

• in Deployment/StatefulSet/DaemonSet apps/v1beta1 or extensions/v1beta1, spec.selector is not mandatory.

• If it is not specified, it is generated from spec.metadata.labels

• Source: Official documentation: https://v1-7.docs.kubernetes.io/docs/concepts/workloads/controllers/deployment/

  If .spec.selector is unspecified, .spec.selector.matchLabels defaults to .spec.template.metadata.labels

Impacted charts

Probably all of https://github.com/helm/charts/pulls?utf8=%E2%9C%93&q=is%3Apr+%22Add+chart+and+release+labels+in+pods%22+

How to reproduce (using stable/dokuwiki as an example)

Let's install the stable/dokuwiki chart using git, change the version in chart.yaml (example: 1.2.3 -> 1.2.4) without changing anything else, and try to upgrade. See it fail:

cd stable/dokuwiki
helm install --name test .
vim Chart.yaml
helm upgrade test .

Because of metadata.labels in templates/deployment.yaml containing:
chart: {{ template "dokuwiki" . }}
It will fail with:

Error: UPGRADE FAILED: Deployment.apps "test-dokuwiki" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"release":"test", "app":"dokuwiki", "chart":"dokuwiki-2.0.7"}: `selector` does not match template `labels`

I think it comes from the fact that during the initial installation, k8s generated the selector, but during upgrade, both helm and k8s ignore it.

But putting an explicit selector, even if it contains a chart label, will make it upgradable:

  selector:
    matchLabels:
      app: {{ template "dokuwiki.name" . }}
      chart: {{ template "dokuwiki.chart" . }}
      release: {{ .Release.Name | quote }}

helm upgrade test .

It works!

Conclusion

• We must always set selector in Deployments/StatefulSets/DaemonSets.
• Selectors can technically contain a chart label although it would be not wise as it would break compatibility for apps/v1beta2 and later (see Putting chart version in spec.selector.matchLabels in apps/v1beta2 and later prevents ANY future upgrade of existing Release #7680 )
• Ideally, Helm should be aware of selector (explicit or generated)
• Note that charts using v1beta1, not having explicit selector, and don't have mutable fields (i.e no chart or version) in spec.template.metadata.labels are not impacted.

Thankfully, this can be solved using a minor version change.

[desaintmartin]

cc @mattfarina @davidkarlsen @scottrigby @javsalgar @carrodher

[scottrigby]

Hi @desaintmartin Thanks again for working to unpack these related issues.

Just to disambiguate these two issues, are you essentially saying this?

• Fixing upgrades for resources in apps/v1beta1 or extensions/v1beta1 require explicitly adding match labels - including the mutable chart label (this issue, Putting chart version in metadata.labels in a Deployment v1beta1 without selector prevents upgrade #7726).
• Fixing upgrades for resources >=apps/v1 require removing any mutable labels from spec.selector, specifically chart (Putting chart version in spec.selector.matchLabels in apps/v1beta2 and later prevents ANY future upgrade of existing Release #7680).
  • As far as we know, this is a breaking change requiring a manual upgrade, so the chart should get a MAJOR version bump and the same PR should document the manual steps for this release in an "Upgrade" section of chart's README ([stable/*] Create and document charts policy for breaking change upgrades #5657).

---

If so, should we clarify this in #7692?

And can this issue should clarify this line in the "Conclusion":

set selector everywhere and don't put mutable fields in it (for forward compatibility)

For this issue (resources in apps/v1beta1 or extensions/v1beta1), it sounds like "set selector everywhere" is correct, but adding a mutable fields is exactly what we want them to do here?

[desaintmartin]

Hey @scottrigby, just helping!
You are correct, except that, in my understanding, the immutability comes with apps/v1beta2 and later, and not only apps/v1. Any confirmation or invalidation would be appreciated! :)

Let me clarify the conclusion by clearly separating the two problems, i.e what should be done now (apps/v1beta1 or extensions/v1beta1) and what is nice for the future (>=v1beta2).

Regarding #7692, I didn't want to get into the details of "why" but let's try, let's continue there for this part of the discussion.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

[desaintmartin]

Have all charts been updated? Can we close this?

[carrodher]

Charts owned by Bitnami were updated

[jkirkham-ratehub]

I've also encountered this issue in the stable/mongodb chart. It doesn't appear to be fixed yet (https://github.com/helm/charts/tree/master/stable/mongodb).

[desaintmartin]

It is related to #7680 to be exact, thanks for reporting.

[juan131]

MongoDB chart was fixed on this PR: #10121

[scottrigby]

We can close this issue because we have the correct way to handle this documented in the Names and Labels section of the REVIEW_GUIDELINES.

If we want to automate checking for this, we should add to https://github.com/helm/chart-testing 😄