Enable custom pod labels for helm chart #326

MarkDPierce · 2023-10-30T11:57:22Z

Hey, we leverage tooling that requires certain labels to be present. While deploying into our ecosystem I noticed the pods don't have the capability to have custom labels. This PR is meant to include that annotation and feature for others that require specific pods labels

CLAassistant · 2023-10-30T11:57:29Z

All committers have signed the CLA.

yorugac · 2023-10-30T12:38:02Z

Hi @MarkDPierce, why customLabels cannot be used? They should be applied to Deployment as well, here.

MarkDPierce · 2023-10-30T12:50:10Z

When I use a custom label of environment: test and generate the manifest I get the following.

# Source: k6-operator/charts/k6-operator/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: release-name-k6-operator-controller-manager
  namespace: release-name-system
  labels:
    control-plane: "controller-manager"
    app.kubernetes.io/component: controller
    helm.sh/chart: k6-operator-1.2.0
    app.kubernetes.io/name: k6-operator
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "0.0.11"
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/part-of: k6-operator
    environment: test
  annotations:

spec:
  replicas: 1
  selector:
    matchLabels:
      app.kubernetes.io/name: k6-operator
      app.kubernetes.io/instance: release-name
  template:
    metadata:
      labels:
        app.kubernetes.io/name: k6-operator
        app.kubernetes.io/instance: release-name
    spec:
      containers:
        - name: kube-rbac-proxy
          image: IMAGE LOCATION
          imagePullPolicy: IfNotPresent
          args:
            - --secure-listen-address=0.0.0.0:8443
            - --upstream=http://127.0.0.1:8080/
            - --logtostderr=true
            - --v=10
          ports:
            - containerPort: 8443
              name: https
        - name: manager
          image: IMAGE LOCATION
          imagePullPolicy: Always
          resources:
            limits:
              cpu: 50m
              memory: 50Mi
            requests:
              cpu: 50m
              memory: 50Mi
          command:
            - /manager
          args:
            - --enable-leader-election
            - --metrics-addr=127.0.0.1:8080
      serviceAccount: k6-operator-controller
      terminationGracePeriodSeconds: 10

These customLabels are not included in the template metadata. Due to security reasons we need labels at the pod level. So every templated pod will get these custom labels.

Thanks for actually asking because I totally forgot to add {{- include "k6-operator.podLabels" . | nindent 8 }} to the deployment.yaml template.

My change should have it looking like

# Source: k6-operator/charts/k6-operator/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: release-name-k6-operator-controller-manager
  namespace: release-name-system
  labels:
    control-plane: "controller-manager"
    app.kubernetes.io/component: controller
    helm.sh/chart: k6-operator-1.2.0
    app.kubernetes.io/name: k6-operator
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "0.0.11"
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/part-of: k6-operator
    environment: test
  annotations:

spec:
  replicas: 1
  selector:
    matchLabels:
      app.kubernetes.io/name: k6-operator
      app.kubernetes.io/instance: release-name
  template:
    metadata:
      labels:
        app.kubernetes.io/name: k6-operator
        app.kubernetes.io/instance: release-name
        environment: test
    spec:
      containers:
        - name: kube-rbac-proxy
          image: IMAGE LOCATION
          imagePullPolicy: IfNotPresent
          args:
            - --secure-listen-address=0.0.0.0:8443
            - --upstream=http://127.0.0.1:8080/
            - --logtostderr=true
            - --v=10
          ports:
            - containerPort: 8443
              name: https
        - name: manager
          image: IMAGE LOCATION
          imagePullPolicy: Always
          resources:
            limits:
              cpu: 50m
              memory: 50Mi
            requests:
              cpu: 50m
              memory: 50Mi
          command:
            - /manager
          args:
            - --enable-leader-election
            - --metrics-addr=127.0.0.1:8080
      serviceAccount: k6-operator-controller
      terminationGracePeriodSeconds: 10

MarkDPierce · 2023-10-30T13:15:45Z

Sorry, that was a long winded explanation to just say deployment.yaml only has

spec:
  replicas: 1
  selector:
    matchLabels:
      {{- include "k6-operator.selectorLabels" . | nindent 6 }}
  template:
    metadata:
      labels:
        {{- include "k6-operator.selectorLabels" . | nindent 8 }}
    spec:

So customLabels wouldnt work in this scenario.

yorugac · 2023-10-31T10:14:46Z

Thanks for full explanation. Now after some looking around, I'm not fully certain we should have customLabels at all 😕 E.g. these charts don't seem to propagate user-defined labels at the level of Deployments or StatefulSets:
https://github.com/grafana/mimir/blob/main/operations/helm/charts/mimir-distributed/values.yaml
https://github.com/grafana/loki/blob/main/production/helm/loki/values.yaml

In k6-operator case, there's only one Deployment which makes it easier; though I suspect in the future we might need to make further breaking changes here.

I'd appreciate some feedback from community here, about how everyone uses labels in their charts 🙂

yorugac · 2023-10-31T10:35:04Z

charts/k6-operator/Chart.yaml

@@ -2,7 +2,7 @@ apiVersion: v1
 appVersion: "0.0.11"
 description: A Helm chart to install the k6-operator
 name: k6-operator
-version: 1.2.0
+version: 2.0.0


Why do you think it should be a breaking change? This seems like a "pure" addition: old deployments should continue to work as is. I.e. why not 1.3.0?

FYI, I'll need to test it a bit and probably return to this PR ~ tomorrow.

Thanks for sharing your experience with versioning of other Helm charts. The chart in k6-operator was added very recently and we're figuring out the "right way" still 😅
Let's go with major bump this time indeed 👍

MarkDPierce · 2023-10-31T12:08:12Z

It's a feature change and other charts we've worked on within the open source community prefer a major bump. I also attempted to find any docs you might have had towards the process. So went with best guess, I'm happy to change or conform to what ever you deem appropriate.

…

On Tue, Oct 31, 2023, 11:35 Olha Yevtushenko ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In charts/k6-operator/Chart.yaml <#326 (comment)>: > @@ -2,7 +2,7 @@ apiVersion: v1 appVersion: "0.0.11" description: A Helm chart to install the k6-operator name: k6-operator -version: 1.2.0 +version: 2.0.0 Why do you think it should be a breaking change? This seems like a "pure" addition: old deployments should continue to work as is. I.e. why not 1.3.0? FYI, I'll need to test it a bit and probably return to this PR ~ tomorrow. — Reply to this email directly, view it on GitHub <#326 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AK56WNSNSJ57K32AQ5HWDVLYCDH6JAVCNFSM6AAAAAA6V4LZRCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBVHE3TANJUGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

yorugac

Sorry for the delay!
The chart generated looks fine and working 🙌 I have a small change request: could you please add podLabels to example in charts/k6-operator/samples/customAnnotationsAndLabels.yaml? It could be an additional sample (file) too, btw.

And another thing. I've noticed that there is an additional empty line in manifest when one omits podLabels like this:

  ...
    metadata:
      labels:
        app.kubernetes.io/name: k6-operator
        app.kubernetes.io/instance: release-name
        
    spec:
      ...

😕 The same empty line is inserted with custom labels which use similar definition and default {} so it's not just change in this PR. This is a minor thing but if you or someone knows how to fix it, I'd appreciate a PR here 😄

yorugac · 2023-11-04T11:00:50Z

charts/k6-operator/Chart.yaml

@@ -2,7 +2,7 @@ apiVersion: v1
 appVersion: "0.0.11"
 description: A Helm chart to install the k6-operator
 name: k6-operator
-version: 1.2.0
+version: 2.0.0


Thanks for sharing your experience with versioning of other Helm charts. The chart in k6-operator was added very recently and we're figuring out the "right way" still 😅
Let's go with major bump this time indeed 👍

MarkDPierce · 2023-11-07T14:57:23Z

Hey sorry for the delay. Work had prio.

I took a stab at the line breaks in your labels and annotations but it might require a bit of refactoring. At least with both you have | default "" | which means it's by design. With a bit of refactoring I managed to achieve the following

---
# Source: k6-operator/templates/namespace.yaml
apiVersion: v1
kind: Namespace
metadata:
  name: release-name-system
  labels:
    app.kubernetes.io/name: release-name-k6-operator
    control-plane: "controller-manager"
    foo: bar
  annotations:
    bar: foo
---
# Source: k6-operator/templates/serviceAccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: k6-operator-controller
  namespace: release-name-system
  labels:
    app.kubernetes.io/component: controller
    helm.sh/chart: k6-operator-2.0.0
    app.kubernetes.io/name: k6-operator
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "0.0.11"
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/part-of: k6-operator
    foo: bar
  annotations:
    bar: foo

But I do think the changes would be out of scope for this change.

yorugac · 2023-11-08T17:59:19Z

@MarkDPierce yes, the line break would be out of scope, but thanks for taking a look. If you see the solution, please feel free to open a PR 🙂 ATM, I don't think default "" is a must so long as the values are passed correctly or are omitted correctly 🤔

Either way, the podLabels change looks good, thank you for this!

MarkDPierce added 3 commits October 30, 2023 12:54

Update template to include podLabels

c0412c2

Include podLabels annotation in values file

559c32e

Bump chart version

dbe2b64

Include helm templating for pod labels

7e02a6d

yorugac reviewed Oct 31, 2023

View reviewed changes

yorugac added this to the 0.12 milestone Oct 31, 2023

yorugac requested changes Nov 4, 2023

View reviewed changes

Requested Feedback

23e42a0

yorugac approved these changes Nov 8, 2023

View reviewed changes

yorugac merged commit 696fead into grafana:main Nov 8, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable custom pod labels for helm chart #326

Enable custom pod labels for helm chart #326

MarkDPierce commented Oct 30, 2023

CLAassistant commented Oct 30, 2023 •

edited

Loading

yorugac commented Oct 30, 2023

MarkDPierce commented Oct 30, 2023 •

edited

Loading

MarkDPierce commented Oct 30, 2023

yorugac commented Oct 31, 2023

yorugac Oct 31, 2023

yorugac Nov 4, 2023

MarkDPierce commented Oct 31, 2023 via email

yorugac left a comment

yorugac Nov 4, 2023

MarkDPierce commented Nov 7, 2023 •

edited

Loading

yorugac commented Nov 8, 2023

Enable custom pod labels for helm chart #326

Enable custom pod labels for helm chart #326

Conversation

MarkDPierce commented Oct 30, 2023

CLAassistant commented Oct 30, 2023 • edited Loading

yorugac commented Oct 30, 2023

MarkDPierce commented Oct 30, 2023 • edited Loading

MarkDPierce commented Oct 30, 2023

yorugac commented Oct 31, 2023

yorugac Oct 31, 2023

Choose a reason for hiding this comment

yorugac Nov 4, 2023

Choose a reason for hiding this comment

MarkDPierce commented Oct 31, 2023 via email

yorugac left a comment

Choose a reason for hiding this comment

yorugac Nov 4, 2023

Choose a reason for hiding this comment

MarkDPierce commented Nov 7, 2023 • edited Loading

yorugac commented Nov 8, 2023

CLAassistant commented Oct 30, 2023 •

edited

Loading

MarkDPierce commented Oct 30, 2023 •

edited

Loading

MarkDPierce commented Nov 7, 2023 •

edited

Loading