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

maelvls · 2024-07-19T09:52:25Z

In #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

inteon · 2024-07-19T10:03:40Z

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

maelvls · 2024-07-19T10:09:37Z

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.

maelvls · 2024-07-19T10:17:47Z

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

inteon · 2024-07-19T10:26:48Z

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.

maelvls · 2024-07-19T10:28:41Z

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: #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.

deploy/charts/openshift-routes/templates/_helpers.tpl

Signed-off-by: Maël Valais <[email protected]>

Signed-off-by: Tim Ramlot <[email protected]>

inteon · 2024-08-12T07:55:11Z

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

maelvls · 2024-08-12T10:11:53Z

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.

inteon · 2024-08-12T10:16:13Z

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: b36d537

inteon · 2024-08-12T14:46:43Z

deploy/charts/openshift-routes/templates/deployment.yaml

@@ -1,7 +1,7 @@
 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: {{ include "openshift-routes.fullname" . }}
+  name: {{ include "openshift-routes.fullname" . }}-controller


This change is necessary to circumvent the following issue:

$ 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 Error: UPGRADE FAILED: cannot patch "openshift-routes" with kind Deployment: Deployment.apps "openshift-routes" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/component":"controller", "app.kubernetes.io/instance":"openshift-routes", "app.kubernetes.io/name":"openshift-routes"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

Renaming may solve the Helm use case, but it also means the controller name changes in the generated static manifest, which if just blindly kubectl applied would create a second deployment

Indeed, seems like an annoying issue. Maybe we just leave the deployment name the same and ask users to use helm upgrade --force?

What exactly is the label diff? Could we generate a template function that outputs the same labels as before?

I guess that is the easiest solution: not changing the label selector. Will try to achieve that.

fixed in 80614e7

inteon · 2024-08-12T14:53:17Z

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

maelvls · 2024-08-12T15:12:31Z

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

cert-manager-prow · 2024-08-12T15:12:34Z

@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 · 2024-08-12T15:12:37Z

[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:

~~OWNERS~~ [maelvls]

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

maelvls · 2024-08-13T18:23:41Z

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

Signed-off-by: Tim Ramlot <[email protected]>

inteon · 2024-08-14T10:06:40Z

/lgtm

inteon · 2024-08-14T10:09:40Z

/unhold

cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2024

maelvls mentioned this pull request Jul 19, 2024

Future of Static Manifest? #73

Closed

maelvls mentioned this pull request Jul 19, 2024

Migrate the project to Makefile modules and release v0.6.0-alpha.0 #60

Merged

maelvls mentioned this pull request Jul 19, 2024

Starting with v0.6.0, image tags now start with v #75

Closed

maelvls force-pushed the static-manifest branch from fdb1e13 to 3d06c13 Compare July 19, 2024 11:27

inteon reviewed Jul 19, 2024

View reviewed changes

deploy/charts/openshift-routes/templates/_helpers.tpl Outdated Show resolved Hide resolved

maelvls and others added 2 commits August 12, 2024 09:36

helm: add omitHelmLabels so that people can generate static manifests

40d707f

Signed-off-by: Maël Valais <[email protected]>

don't use version label in Delployment label selector

b36d537

Signed-off-by: Tim Ramlot <[email protected]>

inteon force-pushed the static-manifest branch from 3d06c13 to b36d537 Compare August 12, 2024 07:44

inteon reviewed Aug 12, 2024

View reviewed changes

cert-manager-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2024

cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2024

make sure we use the same selectorLabels as used in 0.5.0

80614e7

Signed-off-by: Tim Ramlot <[email protected]>

inteon force-pushed the static-manifest branch from aa19208 to 80614e7 Compare August 14, 2024 08:36

cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024

cert-manager-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2024

cert-manager-prow bot merged commit cfdfe89 into cert-manager:main Aug 14, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maelvls commented Jul 19, 2024 •

edited

Loading

inteon commented Jul 19, 2024

maelvls commented Jul 19, 2024 •

edited

Loading

maelvls commented Jul 19, 2024 •

edited

Loading

inteon commented Jul 19, 2024 •

edited by maelvls

Loading

maelvls commented Jul 19, 2024 •

edited

Loading

inteon commented Aug 12, 2024

maelvls commented Aug 12, 2024 •

edited

Loading

inteon commented Aug 12, 2024

inteon Aug 12, 2024 •

edited

Loading

ThatsMrTalbot Aug 12, 2024

inteon Aug 12, 2024

ThatsMrTalbot Aug 12, 2024

inteon Aug 14, 2024

inteon Aug 14, 2024

inteon commented Aug 12, 2024

maelvls commented Aug 12, 2024

cert-manager-prow bot commented Aug 12, 2024

cert-manager-prow bot commented Aug 12, 2024

maelvls commented Aug 13, 2024

inteon commented Aug 14, 2024

inteon commented Aug 14, 2024

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

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

Conversation

maelvls commented Jul 19, 2024 • edited Loading

inteon commented Jul 19, 2024

maelvls commented Jul 19, 2024 • edited Loading

maelvls commented Jul 19, 2024 • edited Loading

inteon commented Jul 19, 2024 • edited by maelvls Loading

maelvls commented Jul 19, 2024 • edited Loading

inteon commented Aug 12, 2024

maelvls commented Aug 12, 2024 • edited Loading

inteon commented Aug 12, 2024

inteon Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

ThatsMrTalbot Aug 12, 2024

Choose a reason for hiding this comment

inteon Aug 12, 2024

Choose a reason for hiding this comment

ThatsMrTalbot Aug 12, 2024

Choose a reason for hiding this comment

inteon Aug 14, 2024

Choose a reason for hiding this comment

inteon Aug 14, 2024

Choose a reason for hiding this comment

inteon commented Aug 12, 2024

maelvls commented Aug 12, 2024

cert-manager-prow bot commented Aug 12, 2024

cert-manager-prow bot commented Aug 12, 2024

maelvls commented Aug 13, 2024

inteon commented Aug 14, 2024

inteon commented Aug 14, 2024

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

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

maelvls commented Jul 19, 2024 •

edited

Loading

maelvls commented Jul 19, 2024 •

edited

Loading

maelvls commented Jul 19, 2024 •

edited

Loading

inteon commented Jul 19, 2024 •

edited by maelvls

Loading

maelvls commented Jul 19, 2024 •

edited

Loading

maelvls commented Aug 12, 2024 •

edited

Loading

inteon Aug 12, 2024 •

edited

Loading