Add new metrics-server.podLabels with default version label and set on pods

[jhulick]

What this PR does / why we need it:

• It adds common labels to metadata labels in the pod spec. The main reasoning is to persist the app.kubernetes.io/version and app.kubernetes.io/name labels to the pod for standardization. Per docs, (https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels) "In order to take full advantage of using these labels, they should be applied on every resource object.".

• Improves traceability throughout the application. Ex. via Kiali dashboard.

NOTE: this is an updated version of a previous version label PR I'd created, and hopefully improved, based on comments from @stevehipwell.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
N/A

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jhulick
Once this PR has been reviewed and has the lgtm label, please assign stevehipwell for approval. For more information see the Kubernetes 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]

This issue is currently awaiting triage.

If metrics-server contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[k8s-ci-robot]

Hi @jhulick. 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]

@jhulick I think you misunderstood my reply in #1596. As a convention/idiom Helm metadata shouldn't be pushed down into the pods; Helm is managing the deployment not the pods (or replica sets). This PR doesn't even add support for this is a passive way, it would force this metadata onto all chart consumers pods. If you want to add additional pod metadata the chart already allows you to set podLabels & podAnnotations.

/close

[k8s-ci-robot]

@stevehipwell: Closed this PR.

In response to this:

@jhulick I think you misunderstood my reply in #1596. As a convention/idiom Helm metadata shouldn't be pushed down into the pods; Helm is managing the deployment not the pods (or replica sets). This PR doesn't even add support for this is a passive way, it would force this metadata onto all chart consumers pods. If you want to add additional pod metadata the chart already allows you to set podLabels & podAnnotations.

/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.

[jhulick]

@stevehipwell, this PR removes the helm.sh/chart metadata label from the pod labels. This is the only difference from the previous PR. See the helm template output below. Are you saying that the version label itself is Helm metadata since it's pulling the version from the Chart.appVersion?

Thanks for you input!

[stevehipwell]

@jhulick I just went back and double checked and it looks like the idiom changed since I last checked. I'm still not sold on the value of doing this as it will add overhead for telemetry where pod labels are captured as part of the payload. Could you please explain your use case here? Given a good use case I'd accept a PR with a value defaulting to the existing behaviour to switch between labeling the pod with the current selector-labels and the labels template.

[jhulick]

@stevehipwell , yes, currently Istio Kiali isn't able to capture metrics-server network traffic because the pods do not have the version label. Kiali requires both common name and version labels. Here are the docs for Istio labels needed for telemetry: https://kiali.io/docs/configuration/istio/#labels-and-resource-names and https://istio.io/latest/docs/ops/deployment/application-requirements/#pod-requirements

On another note, the app.kubernetes.io/name label that is already on the pods (coming from the selectorLabels) is also derived from Chart.name, so I fail to see how this PR is changing any convention/idiom for Helm metadata.

[stevehipwell]

The point is that the original Helm idiom was to only use the selector labels for the pod labels. None of the labels besides app.kubernetes.io/name & app.kubernetes.io/instance (the selector labels) are required for the pods to function correctly.

Out of interest why are you looking to see Metrics Server in Kiali? AFAIK the only thing that's meant to connect to Metrics Server is the control plane which shouldn't be part of an Istio mesh (maybe Prometheus too but not over the Istio mesh).

[jhulick]

It's in support of a client request. I do agree that control plane services should not be part of the mesh... thanks again for your input. We'll go with your suggestion of using podLabels & podAnnotations.

[stevehipwell]

@jhulick sorry I was planning on speaking to some of the Helm maintainers to try and understand why they allowed the change, but I have just been too busy. I think a compromise here, and in the default chart template, would be to support adding this metadata as an opt-in argument.

[jhulick]

Thanks @stevehipwell, we will follow the opt-in approach.