[stable/kube-state-metrics] Apply best practices for label naming

[mrueg]

See: https://helm.sh/docs/chart_best_practices/#standard-labels

What this PR does / why we need it:

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

• fixes #

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• DCO signed
• Chart Version bumped
• Variables are documented in the README.md
• Title of the PR starts with chart name (e.g. [stable/chart])

[k8s-ci-robot]

Hi @mrueg. Thanks for your PR.

I'm waiting for a helm 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.

[tariq1890]

These are breaking changes. I would increment the major here. Let's go to 2.0.0 from here-on.

[tariq1890]

Thanks a lot @mrueg . These changes are awesome :). I've provided one review comment.

[mrueg]

Thanks for the quick review! I applied the requested change.

[tariq1890]

/ok-to-test

[tariq1890]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrueg, tariq1890

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:

• stable/kube-state-metrics/OWNERS [tariq1890]

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

[adamcroissant]

This change broke stable/newrelic-infrastructure, which relies on labels for auto-discovery of kube-state-metrics. From what I can tell, the newrelic k8s infrastructure agent (at least up to current version used by the chart 1.9.0) looks for the labels with expected keys app and k8s-app.

Relatively easy to fix manually by patching the labels on kube-state-metrics, but it would be preferable to avoid breaking changes if possible.

[tariq1890]

I understand the inconvenience.

But this was a major version bump. So breaking changes are to be anticipated.

[adamcroissant]

Yep I understand that. This change could have been made in a non-breaking manner though, and thereby given more time for downstream systems to adjust to the change. By applying both the new label and the old label, it would have enabled adherence to the best practice, avoided incompatibility with other systems, and still allowed for the dropping of the old label in the future when a major version bump happens.

Not the end of the world either way, but its an option that might be worth considering. I recognize there is the tradeoff of floating tech debt for some period of time until a major version bump allows dropping the old/deprecated value, but in my opinion that's worth the tradeoff.

[tariq1890]

I completely understand and the approach of an additive change you have suggested would have been ideal. But the problem is that even additive changes would cause a problem as deployment label selectors are immutable. So it just made more sense to go forward with the new labels, let go of the old ones, and bump the major version.

I could probably be missing something, but do you see a way around that?

[adamcroissant]

I might also be missing something 😆

My thought was that you could've essentially duplicated the labels. So the final result would've been:

  labels:
    app.kubernetes.io/name: {{ template "kube-state-metrics.name" . }}
    helm.sh/chart: {{ .Chart.Name }}-{{ .Chart.Version }}
    app.kubernetes.io/managed-by: {{ .Release.Service }}
    app.kubernetes.io/instance: {{ .Release.Name }}
    # Deprecated labels provided for backwards-compatibility. Subject to removal in subsequent major version.
    app: {{ template "kube-state-metrics.name" . }}
    chart: {{ .Chart.Name }}-{{ .Chart.Version }}
    heritage: {{ .Release.Service }}
    release: {{ .Release.Name }}

And the version would be 1.6.6 as originally specified in the pull request. If I'm not mistaken, this would mean that anyone who ran a helm install --upgrade to version 1.6.6 would have the (admittedly extensive and redundant) set of labels {app.kubernetes.io/name, helm.sh/chart, app.kubernetes.io/managed-by, app.kubernetes.io/instance, app, chart, heritage, release} on their deployment. And then once you eventually released 2.0.0, you would reduce that to only the new/non-deprecated labels. Both of those would result in an update to the deployment, same as the update done in this PR.

Again, not a huge deal. I mostly made the original comment to make it easier to link back to an issue for stable/newrelic-infrastructure (which I also passed on to NewRelic) to try and get that chart and/or the underlying docker image up to date with stable/kube-state-metrics. You're of course always entitled to release a breaking change with a major version, and thank you for spotting the breaking change in review and requiring a major bump.

The real takeaway is definitely that I should stop being lazy and pin the versions of the helm charts I'm installing so that interdependencies between them can't cause this kind of issue unexpectedly 😄

[tariq1890]

@adamcroissant No worries at all and I really appreciate you bringing this up. Feedback is always welcome :). I am happy to continue the thread to discuss a better way around this. Let's create an issue and tag the core maintainers of helm/charts as well.

I understand your suggestion of keeping the old labels, but I thought that adding new labels and not changing the label selectors (which are immutable) didn't make sense. This was my rationale. and this certainly may not have been the best approach.