Fix: webhook deadlock (jaegertracing#1850)

* introduce recommended common label for app name Signed-off-by: Benedikt Bongartz <[email protected]> * exclude deployments named jaeger-operator from the injection webhook Signed-off-by: Benedikt Bongartz <[email protected]> * change deployment webhook failurePolicy to ignore Signed-off-by: Benedikt Bongartz <[email protected]> * regenerate manifest and bundle Signed-off-by: Benedikt Bongartz <[email protected]> * test webhook deployment exception on app name Signed-off-by: Benedikt Bongartz <[email protected]> Co-authored-by: Ruben Vargas <[email protected]>
andreasgerstmayr · Apr 13, 2022 · d60fada · d60fada
1 parent 3e456b7
commit d60fada
Show file tree

Hide file tree

Showing 19 changed files with 149 additions and 3 deletions.
diff --git a/...le/manifests/jaeger-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml b/...le/manifests/jaeger-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
@@ -3,6 +3,7 @@ kind: ClusterRole
 metadata:
   creationTimestamp: null
   labels:
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
   name: jaeger-operator-metrics-reader
 rules:

diff --git a/bundle/manifests/jaeger-operator-metrics_v1_service.yaml b/bundle/manifests/jaeger-operator-metrics_v1_service.yaml
@@ -4,6 +4,7 @@ metadata:
   creationTimestamp: null
   labels:
     app.kubernetes.io/component: metrics
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
   name: jaeger-operator-metrics
 spec:
@@ -13,6 +14,7 @@ spec:
     protocol: TCP
     targetPort: https
   selector:
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
 status:
   loadBalancer: {}
diff --git a/bundle/manifests/jaeger-operator-webhook-service_v1_service.yaml b/bundle/manifests/jaeger-operator-webhook-service_v1_service.yaml
@@ -3,6 +3,7 @@ kind: Service
 metadata:
   creationTimestamp: null
   labels:
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
   name: jaeger-operator-webhook-service
 spec:
@@ -11,6 +12,7 @@ spec:
     protocol: TCP
     targetPort: 9443
   selector:
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
 status:
   loadBalancer: {}
diff --git a/bundle/manifests/jaeger-operator.clusterserviceversion.yaml b/bundle/manifests/jaeger-operator.clusterserviceversion.yaml
@@ -376,11 +376,13 @@ spec:
           replicas: 1
           selector:
             matchLabels:
+              app.kubernetes.io/name: jaeger-operator
               name: jaeger-operator
           strategy: {}
           template:
             metadata:
               labels:
+                app.kubernetes.io/name: jaeger-operator
                 name: jaeger-operator
             spec:
               containers:
@@ -526,8 +528,14 @@ spec:
     - v1
     containerPort: 443
     deploymentName: jaeger-operator
-    failurePolicy: Fail
+    failurePolicy: Ignore
     generateName: deployment.sidecar-injector.jaegertracing.io
+    objectSelector:
+      matchExpressions:
+      - key: app.kubernetes.io/name
+        operator: NotIn
+        values:
+        - jaeger-operator
     rules:
     - apiGroups:
       - apps

diff --git a/bundle/manifests/jaegertracing.io_jaegers.yaml b/bundle/manifests/jaegertracing.io_jaegers.yaml
@@ -6,6 +6,7 @@ metadata:
     controller-gen.kubebuilder.io/version: v0.8.0
   creationTimestamp: null
   labels:
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
   name: jaegers.jaegertracing.io
 spec:

diff --git a/bundle/manifests/prometheus_rbac.authorization.k8s.io_v1_role.yaml b/bundle/manifests/prometheus_rbac.authorization.k8s.io_v1_role.yaml
@@ -6,6 +6,7 @@ metadata:
     include.release.openshift.io/single-node-developer: "true"
   creationTimestamp: null
   labels:
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
   name: prometheus
 rules:

diff --git a/bundle/manifests/prometheus_rbac.authorization.k8s.io_v1_rolebinding.yaml b/bundle/manifests/prometheus_rbac.authorization.k8s.io_v1_rolebinding.yaml
@@ -6,6 +6,7 @@ metadata:
     include.release.openshift.io/single-node-developer: "true"
   creationTimestamp: null
   labels:
+    app.kubernetes.io/name: jaeger-operator
     name: jaeger-operator
   name: prometheus
 roleRef:

diff --git a/config/default/kustomization.yaml b/config/default/kustomization.yaml
@@ -12,8 +12,10 @@ namespace: observability
 #namePrefix: jaeger-operator-
 
 # Labels to add to all resources and selectors.
+# https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels
 commonLabels:
   name: jaeger-operator
+  app.kubernetes.io/name: jaeger-operator
 
 bases:
 - ../crd

diff --git a/config/webhook/deployment_inject_patch.yaml b/config/webhook/deployment_inject_patch.yaml
@@ -0,0 +1,12 @@
+apiVersion: admissionregistration.k8s.io/v1
+kind: MutatingWebhookConfiguration
+metadata:
+  name: mutating-webhook-configuration
+webhooks:
+  - name: deployment.sidecar-injector.jaegertracing.io
+    objectSelector: # Skip resources with the name jaeger-operator
+      matchExpressions:
+        - key: app.kubernetes.io/name
+          operator: NotIn
+          values:
+            - "jaeger-operator"
diff --git a/config/webhook/kustomization.yaml b/config/webhook/kustomization.yaml
@@ -6,3 +6,6 @@ namePrefix: jaeger-operator-
 
 configurations:
 - kustomizeconfig.yaml
+
+patchesStrategicMerge:
+  - deployment_inject_patch.yaml
diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml
@@ -32,7 +32,7 @@ webhooks:
       name: webhook-service
       namespace: system
       path: /mutate-v1-deployment
-  failurePolicy: Fail
+  failurePolicy: Ignore
   name: deployment.sidecar-injector.jaegertracing.io
   rules:
   - apiGroups:

diff --git a/controllers/appsv1/deployment_webhook.go b/controllers/appsv1/deployment_webhook.go
@@ -37,7 +37,7 @@ func NewDeploymentInterceptorWebhook(c client.Client) webhook.AdmissionHandler {
 }
 
 // You need to ensure the path here match the path in the marker.
-// +kubebuilder:webhook:path=/mutate-v1-deployment,mutating=true,failurePolicy=fail,groups="apps",resources=deployments,sideEffects=None,verbs=create;update,versions=v1,name=deployment.sidecar-injector.jaegertracing.io,admissionReviewVersions=v1
+// +kubebuilder:webhook:path=/mutate-v1-deployment,mutating=true,failurePolicy=ignore,groups="apps",resources=deployments,sideEffects=None,verbs=create;update,versions=v1,name=deployment.sidecar-injector.jaegertracing.io,admissionReviewVersions=v1
 
 // +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=core,resources=namespaces/status,verbs=get;update;patch

diff --git a/tests/e2e/sidecar/render.sh b/tests/e2e/sidecar/render.sh
@@ -19,3 +19,6 @@ jaeger_service_name="order"
 render_install_vertx "01"
 render_find_service "agent-as-sidecar" "$jaeger_service_name" "01" "02"
 render_find_service "agent-as-sidecar2" "$jaeger_service_name" "02" "05"
+
+start_test "sidecar-skip-webhook"
+render_install_vertx "01"
diff --git a/tests/e2e/sidecar/sidecar-skip-webhook/00-assert.yaml b/tests/e2e/sidecar/sidecar-skip-webhook/00-assert.yaml
@@ -0,0 +1,8 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: agent-as-sidecar
+spec:
+  replicas: 1
+status:
+  readyReplicas: 1
diff --git a/tests/e2e/sidecar/sidecar-skip-webhook/00-install.yaml b/tests/e2e/sidecar/sidecar-skip-webhook/00-install.yaml
@@ -0,0 +1,19 @@
+apiVersion: jaegertracing.io/v1
+kind: Jaeger
+metadata:
+  name: agent-as-sidecar
+spec:
+  strategy: allinone
+  allInOne:
+    options:
+      log-level:  "debug"
+      memory.max-traces: 10000
+  ingress:
+    enabled: true
+    security: "none"
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - command: ./annotate-ns.sh
+    namespaced: true
diff --git a/tests/e2e/sidecar/sidecar-skip-webhook/01-assert.yaml b/tests/e2e/sidecar/sidecar-skip-webhook/01-assert.yaml
@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  labels:
+    app: vertx-create-span-sidecar
+spec:
+  containers:
+    - name: vertx-create-span-sidecar
+status:
+  phase: Running
diff --git a/tests/e2e/sidecar/sidecar-skip-webhook/01-errors.yaml b/tests/e2e/sidecar/sidecar-skip-webhook/01-errors.yaml
@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  labels:
+    app: vertx-create-span-sidecar
+spec:
+  containers:
+    - name: vertx-create-span-sidecar
+status:
+  phase: Running
diff --git a/tests/e2e/sidecar/sidecar-skip-webhook/01-install.yaml b/tests/e2e/sidecar/sidecar-skip-webhook/01-install.yaml
@@ -0,0 +1,36 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: vertx-create-span-sidecar
+  annotations:
+    "sidecar.jaegertracing.io/inject": "true"
+  labels:
+    # deployment should not pass the webhook - no sidecar gets injected
+    "app.kubernetes.io/name": "jaeger-operator"
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: vertx-create-span-sidecar
+  template:
+    metadata:
+      labels:
+        app: vertx-create-span-sidecar
+    spec:
+      containers:
+        - name: vertx-create-span-sidecar
+          image: "jaegertracing/vertx-create-span:operator-e2e-tests"
+          ports:
+            - containerPort: 8080
+          readinessProbe:
+            httpGet:
+              path: "/"
+              port: 8080
+            initialDelaySeconds: 1
+            periodSeconds: 1
+          livenessProbe:
+            httpGet:
+              path: "/"
+              port: 8080
+            initialDelaySeconds: 1
+            periodSeconds: 1
diff --git a/tests/e2e/sidecar/sidecar-skip-webhook/annotate-ns.sh b/tests/e2e/sidecar/sidecar-skip-webhook/annotate-ns.sh
@@ -0,0 +1,27 @@
+#!/usr/bin/env bash
+
+PARAMS=""
+NAMESPACE=""
+while (( "$#" )); do
+  case "$1" in
+    -n|--namespace)
+      if [ -n "$2" ] && [ ${2:0:1} != "-" ]; then
+        NAMESPACE=$2
+        shift 2
+      else
+        echo "Error: Argument for $1 is missing" >&2
+        exit 1
+      fi
+      ;;
+    -*|--*=) # unsupported flags
+      echo "Error: Unsupported flag $1" >&2
+      exit 1
+      ;;
+    *) # preserve positional arguments
+      PARAMS="$PARAMS $1"
+      shift
+      ;;
+  esac
+done
+
+kubectl annotate --overwrite namespaces ${NAMESPACE} "sidecar.jaegertracing.io/inject"="true"