openshift-routes icon indicating copy to clipboard operation
openshift-routes copied to clipboard

Helm: add `omitHelmLabels` so that people can generate static manifests without the Helm-specific labels

Open maelvls opened this issue 1 year ago • 6 comments

In https://github.com/cert-manager/openshift-routes/issues/73, I wondered how users could continue using a static manifests (e.g., for ArgoCD using the "static manifests" pattern). In this PR, I propose to add a Helm value to help with generating a static manifest free of Helm-specific labels.

To generate a static manifest free of Helm-specific labels, you will now be able to run:

helm template deploy/charts/openshift-routes/ --set omitHelmLabels=true 

The only two labels and labelSelectors left are the three non-Helm-specific labels:

labels:
  app.kubernetes.io/name: openshift-routes
  app.kubernetes.io/component: controller
  app.kubernetes.io/version: v0.6.0-alpha.0

Here are the labels that get removed when using --set omitHelmLabels=true:

make helm-chart VERSION=v0.6.0-alpha.0
diff -u \
  <(helm template --show-only templates/deployment.yaml _bin/scratch/image/openshift-routes-0.6.0-alpha.0.tgz) \
  <(helm template --show-only templates/deployment.yaml _bin/scratch/image/openshift-routes-0.6.0-alpha.0.tgz --set omitHelmLabels=true)
--- helm template --show-only templates/deployment.yaml
+++ helm template --show-only templates/deployment.yaml --set omitHelmLabels=true
@@ -9,9 +9,6 @@
     app.kubernetes.io/name: openshift-routes
     app.kubernetes.io/component: controller
     app.kubernetes.io/version: 0.6.0-alpha.0
-    app.kubernetes.io/instance: release-name
-    app.kubernetes.io/managed-by: Helm
-    helm.sh/chart: openshift-routes-0.6.0-alpha.0
 spec:
   replicas: 1
   selector:
@@ -19,14 +16,12 @@
       app.kubernetes.io/name: openshift-routes
       app.kubernetes.io/component: controller
       app.kubernetes.io/version: 0.6.0-alpha.0
-      app.kubernetes.io/instance: release-name
   template:
     metadata:
       labels:
         app.kubernetes.io/name: openshift-routes
         app.kubernetes.io/component: controller
         app.kubernetes.io/version: 0.6.0-alpha.0
-        app.kubernetes.io/instance: release-name
     spec:
       automountServiceAccountToken: true
       serviceAccountName: release-name-openshift-routes

Difference between the old v0.5.0 static manifest and the static manifest you would get using --set omitHelmLabels=true:

make helm-chart VERSION=v0.6.0-alpha.0
diff -u \
  <(curl -sSL https://github.com/cert-manager/openshift-routes/releases/download/v0.5.0/cert-manager-openshift-routes.yaml) \
  <(helm template openshift-routes -n cert-manager _bin/scratch/image/openshift-routes-0.6.0-alpha.0.tgz --set omitHelmLabels=true)
--- curl -sSL https://github.com/cert-manager/openshift-routes/releases/download/v0.5.0/cert-manager-openshift-routes.yaml
+++ helm template openshift-routes -n cert-manager _bin/scratch/image/openshift-routes-0.6.0-alpha.0.tgz --set omitHelmLabels=true
@@ -1,8 +1,25 @@
 ---
+# Source: openshift-routes/templates/serviceaccount.yaml
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: openshift-routes
+  namespace: cert-manager
+  labels:
+    app.kubernetes.io/name: openshift-routes
+    app.kubernetes.io/component: controller
+    app.kubernetes.io/version: 0.6.0-alpha.0
+automountServiceAccountToken: false
+---
+# Source: openshift-routes/templates/rbac.yaml
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
-  name: cert-manager-openshift-routes
+  name: openshift-routes
+  labels:
+    app.kubernetes.io/name: openshift-routes
+    app.kubernetes.io/component: controller
+    app.kubernetes.io/version: 0.6.0-alpha.0
 rules:
 - apiGroups:
   - route.openshift.io
@@ -61,66 +78,71 @@
   - list
   - update
 ---
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: cert-manager-openshift-routes
-  namespace: cert-manager
-automountServiceAccountToken: false
----
+# Source: openshift-routes/templates/rbac.yaml
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
 metadata:
-  name: cert-manager-openshift-routes
+  name: openshift-routes
+  labels:
+    app.kubernetes.io/name: openshift-routes
+    app.kubernetes.io/component: controller
+    app.kubernetes.io/version: 0.6.0-alpha.0
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole
-  name: cert-manager-openshift-routes
+  name: openshift-routes
 subjects:
 - kind: ServiceAccount
-  name: cert-manager-openshift-routes
+  name: openshift-routes
   namespace: cert-manager
 ---
+# Source: openshift-routes/templates/deployment.yaml
 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: cert-manager-openshift-routes
+  name: openshift-routes
   namespace: cert-manager
   labels:
-    app.kubernetes.io/name: cert-manager-openshift-routes
-    app.kubernetes.io/version: "0.5.0"
+    app.kubernetes.io/name: openshift-routes
     app.kubernetes.io/component: controller
-    app.kubernetes.io/part-of: cert-manager
+    app.kubernetes.io/version: 0.6.0-alpha.0
 spec:
   replicas: 1
   selector:
     matchLabels:
-      app.kubernetes.io/name: cert-manager-openshift-routes
-      app.kubernetes.io/version: "0.5.0"
+      app.kubernetes.io/name: openshift-routes
       app.kubernetes.io/component: controller
-      app.kubernetes.io/part-of: cert-manager
+      app.kubernetes.io/version: 0.6.0-alpha.0
   template:
     metadata:
       labels:
-        app.kubernetes.io/name: cert-manager-openshift-routes
-        app.kubernetes.io/version: "0.5.0"
+        app.kubernetes.io/name: openshift-routes
         app.kubernetes.io/component: controller
-        app.kubernetes.io/part-of: cert-manager
+        app.kubernetes.io/version: 0.6.0-alpha.0
     spec:
-      serviceAccountName: cert-manager-openshift-routes
       automountServiceAccountToken: true
+      serviceAccountName: openshift-routes
+      securityContext:
+        runAsNonRoot: true
+        seccompProfile:
+          type: RuntimeDefault
       containers:
-        - name: cert-manager-openshift-routes
-          image: "ghcr.io/cert-manager/cert-manager-openshift-routes:0.5.0"
+        - name: openshift-routes
+          securityContext:
+            allowPrivilegeEscalation: false
+            capabilities:
+              drop:
+              - ALL
+            readOnlyRootFilesystem: true
+          image: "ghcr.io/cert-manager/cert-manager-openshift-routes:v0.6.0-alpha.0"
+          imagePullPolicy: IfNotPresent
           args:
-          - -v=5
+            - "-v=5"
+            - "--leader-election-namespace=cert-manager"
           ports:
           - containerPort: 6060
             name: readiness
             protocol: TCP
-          - containerPort: 9402
-            name: metrics
-            protocol: TCP
           readinessProbe:
             httpGet:
               port: readiness
@@ -128,3 +150,7 @@
             initialDelaySeconds: 3
             periodSeconds: 5
             timeoutSeconds: 3
+          resources:
+            {}
+      nodeSelector:
+        kubernetes.io/os: linux

maelvls avatar Jul 19 '24 09:07 maelvls

cert-manager has a similar value: https://github.com/cert-manager/cert-manager/blob/master/deploy/charts/cert-manager/values.yaml#L1380-L1384

inteon avatar Jul 19 '24 10:07 inteon

Talked during the open standup:

I noticed that the label

app.kubernetes.io/version: "0.5.0"

got removed. I will add it back using appVersion since it seems like a useful label.

Note that I noticed that some users prefer when the label selectors don't have a variable version: https://github.com/quarkusio/quarkus/issues/11070. But that seems to be only when a Pod is used directly since pod specs are immutable. In Openshift-routes, we use deployments which means this isn't a problem.

maelvls avatar Jul 19 '24 10:07 maelvls

I noticed some discrepancies with the image tag pattern in v0.5.0 and in v0.6.0:

-image: "ghcr.io/cert-manager/cert-manager-openshift-routes:0.5.0"
+image: "ghcr.io/cert-manager/cert-manager-openshift-routes:v0.6.0"

It looks like the v was added to the image tags after #60 got merged:

Is that intended? I guess I'm OK with this change, but isn't this going to break anyone? @inteon

maelvls avatar Jul 19 '24 10:07 maelvls

I noticed some discrepancies with the image tag pattern in v0.5.0 and in v0.6.0.

Thanks for noticing this. I did not notice that this was how the current images were versioned and that it differs now. I do believe it makes sense to continue using this new pattern. It only makes sense to use the non-v prefix for Helm charts, not for the OCI image.

inteon avatar Jul 19 '24 10:07 inteon

I agree. I'll give notice to the users of openshift-routes just to make sure we are giving them a chance to complain before the change takes place. To that effect:

→ I created a GitHub issue and pinned it: https://github.com/cert-manager/openshift-routes/issues/75 → I published a message on Slack and pinged Vinny (only person I know who uses openshift-routes in the Kubernetes Slack). → I emailed Jack Henschel to ask what he thinks.

maelvls avatar Jul 19 '24 10:07 maelvls

Talked during the open standup:

I noticed that the label

app.kubernetes.io/version: "0.5.0"

got removed. I will add it back using appVersion since it seems like a useful label.

Note that I noticed that some users prefer when the label selectors don't have a variable version: quarkusio/quarkus#11070. But that seems to be only when a Pod is used directly since pod specs are immutable. In Openshift-routes, we use deployments which means this isn't a problem.

The labelselector in a Deployment is also immutable:

# deployments.apps "openshift-routes" was not valid:
# * spec.template.metadata.labels: Invalid value: map[string]string{"app.kubernetes.io/component":"controller", "app.kubernetes.io/instance":"openshift-routes", "app.kubernetes.io/managed-by":"Helm", "app.kubernetes.io/name":"openshift-routes", "app.kubernetes.io/version":"0.6.0-alpha.0-20-gb36d5372ac760e", "helm.sh/chart":"openshift-routes-0.6.0-alpha.0-20-gb36d5372ac760e"}: `selector` does not match template `labels`
# * spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/component":"controller88", "app.kubernetes.io/instance":"openshift-routes", "app.kubernetes.io/name":"openshift-routes"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

inteon avatar Aug 12 '24 07:08 inteon

The labelselector in a Deployment is also immutable

You are right, I forgot about selector.matchLabels not being mutable on Deployment resources. I propose that we keep the version label anywhere else, and only skip it for the selector.matchLabels field.

maelvls avatar Aug 12 '24 10:08 maelvls

The labelselector in a Deployment is also immutable

You are right, I forgot about selector.matchLabels not being mutable on Deployment resources. I propose that we keep the version label anywhere else, and only skip it for the selector.matchLabels field.

Great, that is what I added here: https://github.com/cert-manager/openshift-routes/pull/74/commits/b36d5372ac760ee3a70414e02ce7abdcc3b0d0d1

inteon avatar Aug 12 '24 10:08 inteon

Updated diff after applying the latest changes included in this PR:

make helm-chart VERSION=v0.6.0-alpha.0
diff -u \
  <(helm template --show-only templates/deployment.yaml _bin/scratch/image/openshift-routes-0.6.0-alpha.0.tgz) \
  <(helm template --show-only templates/deployment.yaml _bin/scratch/image/openshift-routes-0.6.0-alpha.0.tgz --set omitHelmLabels=true)
--- helm template --show-only templates/deployment.yaml
+++ helm template --show-only templates/deployment.yaml --set omitHelmLabels=true
@@ -8,26 +8,19 @@
   labels:
     app.kubernetes.io/name: openshift-routes
     app.kubernetes.io/component: controller
-    app.kubernetes.io/instance: release-name
     app.kubernetes.io/version: 0.6.0-alpha.0
-    app.kubernetes.io/managed-by: Helm
-    helm.sh/chart: openshift-routes-0.6.0-alpha.0
 spec:
   replicas: 1
   selector:
     matchLabels:
       app.kubernetes.io/name: openshift-routes
       app.kubernetes.io/component: controller
-      app.kubernetes.io/instance: release-name
   template:
     metadata:
       labels:
         app.kubernetes.io/name: openshift-routes
         app.kubernetes.io/component: controller
-        app.kubernetes.io/instance: release-name
         app.kubernetes.io/version: 0.6.0-alpha.0
-        app.kubernetes.io/managed-by: Helm
-        helm.sh/chart: openshift-routes-0.6.0-alpha.0
     spec:
       automountServiceAccountToken: true
       serviceAccountName: release-name-openshift-routes

Proof that we can upgrade using Helm from v0.5.0 to v0.6.0-...

$ helm install openshift-routes -n cert-manager oci://ghcr.io/cert-manager/charts/openshift-routes --version=0.5.0
$ make helm-chart VERSION=v0.6.0-alpha.1
$ helm upgrade openshift-routes -n cert-manager _bin/scratch/image/openshift-routes-0.6.0-alpha.1.tgz
Release "openshift-routes" has been upgraded. Happy Helming!
NAME: openshift-routes
LAST DEPLOYED: Mon Aug 12 16:53:08 2024
NAMESPACE: cert-manager
STATUS: deployed
REVISION: 5
TEST SUITE: None

inteon avatar Aug 12 '24 14:08 inteon

Thanks for showing the diff output. I think this is good to go now.

/lgtm /approve /hold since there seems to be a discussion with Adam

maelvls avatar Aug 12 '24 15:08 maelvls

@maelvls: you cannot LGTM your own PR.

In response to this:

Thanks for showing the diff output. I think this is good to go now.

/lgtm /approve /hold since there seems to be a discussion with Adam

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.

cert-manager-prow[bot] avatar Aug 12 '24 15:08 cert-manager-prow[bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

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:

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

cert-manager-prow[bot] avatar Aug 12 '24 15:08 cert-manager-prow[bot]

I can't approve my own PR 😅 Can someone else LGTM, @ThatsMrTalbot maybe? Thanks.

maelvls avatar Aug 13 '24 18:08 maelvls

/lgtm

inteon avatar Aug 14 '24 10:08 inteon

/unhold

inteon avatar Aug 14 '24 10:08 inteon