Fix webhook certname in helm template #7775

kvalliyurnatt · 2024-05-06T14:13:10Z

fixes: #7771 where the webhook secret name was not being set correctly in the _helpers file.
Also renaming webhook.secret to webhook.certsSecret to match what is there in our documentation.

kvalliyurnatt · 2024-05-06T14:15:33Z

Tested it locally
without the cert name

helm template -s templates/statefulset.yaml deploy/eck-operator                                                                         ✔  12587  10:07:38
---
# Source: eck-operator/templates/statefulset.yaml
apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: elastic-operator
  namespace: default
  labels:

    app.kubernetes.io/name: elastic-operator
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "2.14.0-SNAPSHOT"
    helm.sh/chart: eck-operator-2.14.0-SNAPSHOT
    app.kubernetes.io/managed-by: Helm
spec:
  selector:
    matchLabels:

      app.kubernetes.io/name: elastic-operator
      app.kubernetes.io/instance: release-name
  serviceName: elastic-operator
  replicas: 1
  template:
    metadata:
      annotations:
        # Rename the fields "error" to "error.message" and "source" to "event.source"
        # This is to avoid a conflict with the ECS "error" and "source" documents.
        "co.elastic.logs/raw": "[{\"type\":\"container\",\"json.keys_under_root\":true,\"paths\":[\"/var/log/containers/*${data.kubernetes.container.id}.log\"],\"processors\":[{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"error\",\"to\":\"_error\"}]}},{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"_error\",\"to\":\"error.message\"}]}},{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"source\",\"to\":\"_source\"}]}},{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"_source\",\"to\":\"event.source\"}]}}]}]"
        "checksum/config": 7dd16b2cd406beb01ef1c3eb51447ec68bb7869c6ba9c1f9a5a456766b528c8b
      labels:

        app.kubernetes.io/name: elastic-operator
        app.kubernetes.io/instance: release-name
    spec:
      terminationGracePeriodSeconds: 10
      serviceAccountName: elastic-operator
      automountServiceAccountToken: true
      securityContext:
        runAsNonRoot: true
      containers:
        - image: "docker.elastic.co/eck/eck-operator:2.14.0-SNAPSHOT"
          imagePullPolicy: IfNotPresent
          name: manager
          args:
            - "manager"
            - "--config=/conf/eck.yaml"
          securityContext:
            allowPrivilegeEscalation: false
            capabilities:
              drop:
              - ALL
            readOnlyRootFilesystem: true
            runAsNonRoot: true
          env:
            - name: OPERATOR_NAMESPACE
              valueFrom:
                fieldRef:
                  fieldPath: metadata.namespace
            - name: POD_IP
              valueFrom:
                fieldRef:
                  fieldPath: status.podIP
            - name: WEBHOOK_SECRET
              value: elastic-operator-webhook-cert
          resources:
            limits:
              cpu: 1
              memory: 1Gi
            requests:
              cpu: 100m
              memory: 150Mi
          ports:
            - containerPort: 9443
              name: https-webhook
              protocol: TCP
          volumeMounts:
            - mountPath: "/conf"
              name: conf
              readOnly: true
            - mountPath: /tmp/k8s-webhook-server/serving-certs
              name: cert
              readOnly: true
      volumes:
        - name: conf
          configMap:
            name: elastic-operator
        - name: cert
          secret:
            defaultMode: 420
            secretName: elastic-operator-webhook-cert

setting the secret name

helm template -s templates/statefulset.yaml deploy/eck-operator --set=webhook.certsSecret=test-kvalliy                                  ✔  12588  10:07:59
---
# Source: eck-operator/templates/statefulset.yaml
apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: elastic-operator
  namespace: default
  labels:

    app.kubernetes.io/name: elastic-operator
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "2.14.0-SNAPSHOT"
    helm.sh/chart: eck-operator-2.14.0-SNAPSHOT
    app.kubernetes.io/managed-by: Helm
spec:
  selector:
    matchLabels:

      app.kubernetes.io/name: elastic-operator
      app.kubernetes.io/instance: release-name
  serviceName: elastic-operator
  replicas: 1
  template:
    metadata:
      annotations:
        # Rename the fields "error" to "error.message" and "source" to "event.source"
        # This is to avoid a conflict with the ECS "error" and "source" documents.
        "co.elastic.logs/raw": "[{\"type\":\"container\",\"json.keys_under_root\":true,\"paths\":[\"/var/log/containers/*${data.kubernetes.container.id}.log\"],\"processors\":[{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"error\",\"to\":\"_error\"}]}},{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"_error\",\"to\":\"error.message\"}]}},{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"source\",\"to\":\"_source\"}]}},{\"convert\":{\"mode\":\"rename\",\"ignore_missing\":true,\"fields\":[{\"from\":\"_source\",\"to\":\"event.source\"}]}}]}]"
        "checksum/config": 7dd16b2cd406beb01ef1c3eb51447ec68bb7869c6ba9c1f9a5a456766b528c8b
      labels:

        app.kubernetes.io/name: elastic-operator
        app.kubernetes.io/instance: release-name
    spec:
      terminationGracePeriodSeconds: 10
      serviceAccountName: elastic-operator
      automountServiceAccountToken: true
      securityContext:
        runAsNonRoot: true
      containers:
        - image: "docker.elastic.co/eck/eck-operator:2.14.0-SNAPSHOT"
          imagePullPolicy: IfNotPresent
          name: manager
          args:
            - "manager"
            - "--config=/conf/eck.yaml"
          securityContext:
            allowPrivilegeEscalation: false
            capabilities:
              drop:
              - ALL
            readOnlyRootFilesystem: true
            runAsNonRoot: true
          env:
            - name: OPERATOR_NAMESPACE
              valueFrom:
                fieldRef:
                  fieldPath: metadata.namespace
            - name: POD_IP
              valueFrom:
                fieldRef:
                  fieldPath: status.podIP
            - name: WEBHOOK_SECRET
              value: test-kvalliy
          resources:
            limits:
              cpu: 1
              memory: 1Gi
            requests:
              cpu: 100m
              memory: 150Mi
          ports:
            - containerPort: 9443
              name: https-webhook
              protocol: TCP
          volumeMounts:
            - mountPath: "/conf"
              name: conf
              readOnly: true
            - mountPath: /tmp/k8s-webhook-server/serving-certs
              name: cert
              readOnly: true
      volumes:
        - name: conf
          configMap:
            name: elastic-operator
        - name: cert
          secret:
            defaultMode: 420
            secretName: test-kvalliy

kvalliyurnatt · 2024-05-06T14:25:01Z

deploy/eck-operator/templates/configmap.yaml

@@ -79,6 +79,6 @@ data:
    {{- if not .Values.config.containerSuffix }}
    ubi-only: {{ .Values.config.ubiOnly }}
    {{- end }}
-    {{- with .Values.webhook.secret }}
+    {{- with .Values.webhook.certsSecret }}


I am not sure we should keep this as webhook.secret and change the docs ? since the operator config itself is named webhook-secret

Even if we fix this I don't understand how this can be used today since the WEBHOOK_SECRET env. variable seems to take precedence over webhook-secret in the configmap :

cloud-on-k8s/deploy/eck-operator/templates/statefulset.yaml

Lines 73 to 76 in 233cbd0

{{- if .Values.webhook.enabled }}

- name: WEBHOOK_SECRET

value: {{ include "eck-operator.webhookSecretName" . }}

{{- end }}

barkbay

Let's maybe add a unit test?

--- a/deploy/eck-operator/templates/tests/statefulset_test.yaml
+++ b/deploy/eck-operator/templates/tests/statefulset_test.yaml
@@ -4,6 +4,20 @@ templates:
   - statefulset.yaml
   - configmap.yaml
 tests:
+  - it: should use the specified webhook secret name
+    set:
+      webhook:
+        manageCerts: false
+        certsSecret: "my-webhook-server-cert"
+    asserts:
+      - template: statefulset.yaml
+        equal:
+          path: spec.template.spec.volumes[1].name
+          value: cert
+      - template: statefulset.yaml
+        equal:
+          path: spec.template.spec.volumes[1].secret.secretName
+          value: my-webhook-server-cert

barkbay

LGTM

Updated unit test proposal to cover the env. variable:

diff --git a/deploy/eck-operator/templates/tests/statefulset_test.yaml b/deploy/eck-operator/templates/tests/statefulset_test.yaml
index 99425f476..d82a72621 100644
--- a/deploy/eck-operator/templates/tests/statefulset_test.yaml
+++ b/deploy/eck-operator/templates/tests/statefulset_test.yaml
@@ -4,6 +4,28 @@ templates:
   - statefulset.yaml
   - configmap.yaml
 tests:
+  - it: should use the specified webhook secret name
+    set:
+      webhook:
+        manageCerts: false
+        certsSecret: "my-webhook-server-cert"
+    asserts:
+      - template: statefulset.yaml
+        equal:
+          path: spec.template.spec.volumes[1].name
+          value: cert
+      - template: statefulset.yaml
+        equal:
+          path: spec.template.spec.volumes[1].secret.secretName
+          value: my-webhook-server-cert
+      - template: statefulset.yaml
+        equal:
+          path: spec.template.spec.containers[0].env[2].name
+          value: WEBHOOK_SECRET
+      - template: statefulset.yaml
+        equal:
+          path: spec.template.spec.containers[0].env[2].value
+          value: my-webhook-server-cert

pebrc

LGTM +1 for adding a unit test though

* fix webhook secret name in the helm charts and add a unit test as well. (cherry picked from commit 15d3ca2)

fix webhook secret name

ba0c04e

kvalliyurnatt requested review from thbkrkr, pebrc, barkbay, naemono and rhr323 May 6, 2024 14:13

botelastic bot added the triage label May 6, 2024

kvalliyurnatt added >bug Something isn't working v2.13.0 labels May 6, 2024

botelastic bot removed the triage label May 6, 2024

fix the configmap as well

6e02fea

kvalliyurnatt commented May 6, 2024

View reviewed changes

barkbay reviewed May 7, 2024

View reviewed changes

barkbay approved these changes May 7, 2024

View reviewed changes

pebrc approved these changes May 7, 2024

View reviewed changes

add unit test for helm chart

76cfe22

kvalliyurnatt merged commit 15d3ca2 into elastic:main May 7, 2024
5 checks passed

kvalliyurnatt added a commit to kvalliyurnatt/cloud-on-k8s that referenced this pull request May 7, 2024

Fix webhook certname in helm template (elastic#7775)

0a2e326

* fix webhook secret name in the helm charts and add a unit test as well. (cherry picked from commit 15d3ca2)

kvalliyurnatt mentioned this pull request May 7, 2024

[2.13] Fix webhook certname in helm template (#7775) #7783

Merged

kvalliyurnatt added a commit that referenced this pull request May 7, 2024

Fix webhook certname in helm template (#7775) (#7783)

a2eec9d

* fix webhook secret name in the helm charts and add a unit test as well. (cherry picked from commit 15d3ca2)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix webhook certname in helm template #7775

Fix webhook certname in helm template #7775

kvalliyurnatt commented May 6, 2024

kvalliyurnatt commented May 6, 2024

kvalliyurnatt May 6, 2024

barkbay May 7, 2024

barkbay left a comment

barkbay left a comment

pebrc left a comment

	{{- if .Values.webhook.enabled }}
	- name: WEBHOOK_SECRET
	value: {{ include "eck-operator.webhookSecretName" . }}
	{{- end }}

Fix webhook certname in helm template #7775

Fix webhook certname in helm template #7775

Conversation

kvalliyurnatt commented May 6, 2024

kvalliyurnatt commented May 6, 2024

kvalliyurnatt May 6, 2024

Choose a reason for hiding this comment

barkbay May 7, 2024

Choose a reason for hiding this comment

barkbay left a comment

Choose a reason for hiding this comment

barkbay left a comment

Choose a reason for hiding this comment

pebrc left a comment

Choose a reason for hiding this comment