Refactor Connect Inject Webhook to use webhook-cert-manager (#861)

* Enable webhook-cert-manager whenever either controller or connectInject is enabled * Remove connectInject.certs values. This behavior was already broken, and we don't want to support it going forward with webhook-cert-manager. Co-authored-by: Iryna Shustava <[email protected]>
hashicorp · Apr 6, 2021 · 3346897 · 3346897
1 parent e224fa4
commit 3346897
Show file tree

Hide file tree

Showing 28 changed files with 274 additions and 306 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -133,18 +133,17 @@ jobs:
             # exit early if any test fails (-failfast only works within a single
             # package).
             exit_code=0
-            pkgs=$(go list ./... | circleci tests split)
+            pkgs=$(go list ./... | grep -v -E 'metrics'| circleci tests split)
             echo "Running $pkgs"
             for pkg in $pkgs
             do
               if ! gotestsum --no-summary=all --jsonfile=jsonfile-${pkg////-} -- $pkg -p 1 -timeout 30m -failfast \
                     -use-kind \
                     -enable-multi-cluster \
-                    -enable-enterprise \
                     -kubecontext="kind-dc1" \
                     -secondary-kubecontext="kind-dc2" \
                     -debug-directory="$TEST_RESULTS/debug" \
-                    -consul-k8s-image=docker.mirror.hashicorp.services/hashicorpdev/consul-k8s:latest
+                    -consul-k8s-image=ishustava/consul-k8s-dev:04-06-2021-8a9a841 # TODO: change once feature-tproxy consul-k8s branch is merged
               then
                 echo "Tests in ${pkg} failed, aborting early"
                 exit_code=1

diff --git a/templates/connect-inject-authmethod-clusterrole.yaml b/templates/connect-inject-authmethod-clusterrole.yaml
@@ -1,4 +1,4 @@
-{{- if and (not .Values.connectInject.certs.secretName) (or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }}
 {{- if .Values.global.acls.manageSystemACLs }}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole

diff --git a/templates/connect-inject-authmethod-clusterrolebinding.yaml b/templates/connect-inject-authmethod-clusterrolebinding.yaml
@@ -1,4 +1,4 @@
-{{- if and (not .Values.connectInject.certs.secretName) (or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }}
 {{- if .Values.global.acls.manageSystemACLs }}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding

diff --git a/templates/connect-inject-authmethod-serviceaccount.yaml b/templates/connect-inject-authmethod-serviceaccount.yaml
@@ -1,4 +1,4 @@
-{{- if and (not .Values.connectInject.certs.secretName) (or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }}
 {{- if .Values.global.acls.manageSystemACLs }}
 apiVersion: v1
 kind: ServiceAccount

diff --git a/templates/connect-inject-clusterrole.yaml b/templates/connect-inject-clusterrole.yaml
@@ -1,4 +1,4 @@
-{{- if and (not .Values.connectInject.certs.secretName) (or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }}
 # The ClusterRole to enable the Connect injector to get, list, watch and patch MutatingWebhookConfiguration.
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
@@ -10,19 +10,12 @@ metadata:
     heritage: {{ .Release.Service }}
     release: {{ .Release.Name }}
 rules:
-- apiGroups: ["admissionregistration.k8s.io"]
-  resources: ["mutatingwebhookconfigurations"]
-  verbs: 
-    - "get"
-    - "list"
-    - "watch"
-    - "patch"
 - apiGroups: [""]
-  resources: ["pods", "nodes"]
+  resources: ["pods", "nodes", "endpoints"]
   verbs:
-    - "get"
-    - "list"
-    - "watch"
+  - "get"
+  - "list"
+  - "watch"
 {{- if .Values.global.enablePodSecurityPolicies }}
 - apiGroups: ["policy"]
   resources: ["podsecuritypolicies"]
@@ -34,10 +27,10 @@ rules:
 {{- if .Values.global.acls.manageSystemACLs }}
 - apiGroups: [""]
   resources:
-    - secrets
+  - secrets
   resourceNames:
-    - {{ template "consul.fullname" . }}-connect-inject-acl-token
+  - {{ template "consul.fullname" . }}-connect-inject-acl-token
   verbs:
-    - get
+  - get
 {{- end }}
 {{- end }}
diff --git a/templates/connect-inject-clusterrolebinding.yaml b/templates/connect-inject-clusterrolebinding.yaml
@@ -1,4 +1,4 @@
-{{- if and (not .Values.connectInject.certs.secretName) (or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
 metadata:
@@ -13,7 +13,7 @@ roleRef:
   kind: ClusterRole
   name: {{ template "consul.fullname" . }}-connect-injector-webhook
 subjects:
-  - kind: ServiceAccount
-    name: {{ template "consul.fullname" . }}-connect-injector-webhook-svc-account
-    namespace: {{ .Release.Namespace }}
+- kind: ServiceAccount
+  name: {{ template "consul.fullname" . }}-connect-injector-webhook-svc-account
+  namespace: {{ .Release.Namespace }}
 {{- end }}
diff --git a/templates/connect-inject-deployment.yaml b/templates/connect-inject-deployment.yaml
@@ -36,12 +36,14 @@ spec:
       annotations:
         "consul.hashicorp.com/connect-inject": "false"
     spec:
-  {{- if not .Values.connectInject.certs.secretName }}
       serviceAccountName: {{ template "consul.fullname" . }}-connect-injector-webhook-svc-account
-  {{- end }}
       containers:
         - name: sidecar-injector
           image: "{{ default .Values.global.imageK8S .Values.connectInject.image }}"
+          ports:
+          - containerPort: 8080
+            name: webhook-server
+            protocol: TCP
           env:
             - name: NAMESPACE
               valueFrom:
@@ -132,13 +134,7 @@ spec:
                 -consul-cross-namespace-acl-policy=cross-namespace-policy \
                 {{- end }}
                 {{- end }}
-                {{- if .Values.connectInject.certs.secretName }}
-                -tls-cert-file=/etc/connect-injector/certs/{{ .Values.connectInject.certs.certName }} \
-                -tls-key-file=/etc/connect-injector/certs/{{ .Values.connectInject.certs.keyName }} \
-                {{- else }}
-                -tls-auto=${CONSUL_FULLNAME}-connect-injector-cfg \
-                -tls-auto-hosts=${CONSUL_FULLNAME}-connect-injector-svc,${CONSUL_FULLNAME}-connect-injector-svc.${NAMESPACE},${CONSUL_FULLNAME}-connect-injector-svc.${NAMESPACE}.svc \
-                {{- end }}
+                -tls-cert-dir=/etc/connect-injector/certs \
                 {{- $resources := .Values.connectInject.sidecarProxy.resources }}
                 {{- /* kindIs is used here to differentiate between null and 0 */}}
                 {{- if not (kindIs "invalid" $resources.limits.memory) }}
@@ -185,81 +181,46 @@ spec:
                 -consul-sidecar-cpu-request={{ $consulSidecarResources.requests.cpu }} \
                 {{- end }}
                 {{- end }}
-          livenessProbe:
-            httpGet:
-              path: /health/ready
-              port: 8080
-              scheme: HTTPS
-            failureThreshold: 2
-            initialDelaySeconds: 1
-            periodSeconds: 2
-            successThreshold: 1
-            timeoutSeconds: 5
-          readinessProbe:
-            httpGet:
-              path: /health/ready
-              port: 8080
-              scheme: HTTPS
-            failureThreshold: 2
-            initialDelaySeconds: 2
-            periodSeconds: 2
-            successThreshold: 1
-            timeoutSeconds: 5
-          startupProbe:
-            httpGet:
-              path: /health/ready
-              port: 8080
-              scheme: HTTPS
-            failureThreshold: 15
-            periodSeconds: 2
-            timeoutSeconds: 5
-          {{- if (or .Values.connectInject.certs.secretName .Values.global.tls.enabled) }}
           volumeMounts:
-            {{- if .Values.connectInject.certs.secretName }}
-            - name: certs
-              mountPath: /etc/connect-injector/certs
-              readOnly: true
-            {{- end }}
-            {{- if .Values.global.tls.enabled }}
-            {{- if .Values.global.tls.enableAutoEncrypt }}
-            - name: consul-auto-encrypt-ca-cert
-            {{- else }}
-            - name: consul-ca-cert
-            {{- end }}
-              mountPath: /consul/tls/ca
-              readOnly: true
-            {{- end }}
-            {{- end }}
+          - name: certs
+            mountPath: /etc/connect-injector/certs
+            readOnly: true
+          {{- if .Values.global.tls.enabled }}
+          {{- if .Values.global.tls.enableAutoEncrypt }}
+          - name: consul-auto-encrypt-ca-cert
+          {{- else }}
+          - name: consul-ca-cert
+          {{- end }}
+            mountPath: /consul/tls/ca
+            readOnly: true
+          {{- end }}
           {{- with .Values.connectInject.resources }}
           resources:
             {{- toYaml . | nindent 12 }}
           {{- end }}
-      {{- if (or .Values.connectInject.certs.secretName .Values.global.tls.enabled) }}
       volumes:
-        {{- if .Values.connectInject.certs.secretName }}
-        - name: certs
-          secret:
-            secretName: {{ .Values.connectInject.certs.secretName }}
-        {{- end }}
-        {{- if .Values.global.tls.enabled }}
-        {{- if not (and .Values.externalServers.enabled .Values.externalServers.useSystemRoots) }}
-        - name: consul-ca-cert
-          secret:
-            {{- if .Values.global.tls.caCert.secretName }}
-            secretName: {{ .Values.global.tls.caCert.secretName }}
-            {{- else }}
-            secretName: {{ template "consul.fullname" . }}-ca-cert
-            {{- end }}
-            items:
-            - key: {{ default "tls.crt" .Values.global.tls.caCert.secretKey }}
-              path: tls.crt
-        {{- end }}
-        {{- if .Values.global.tls.enableAutoEncrypt }}
-        - name: consul-auto-encrypt-ca-cert
-          emptyDir:
-            medium: "Memory"
-        {{- end }}
-        {{- end }}
+      - name: certs
+        secret:
+          defaultMode: 420
+          secretName: {{ template "consul.fullname" . }}-connect-inject-webhook-cert
+      {{- if .Values.global.tls.enabled }}
+      {{- if not (and .Values.externalServers.enabled .Values.externalServers.useSystemRoots) }}
+      - name: consul-ca-cert
+        secret:
+          {{- if .Values.global.tls.caCert.secretName }}
+          secretName: {{ .Values.global.tls.caCert.secretName }}
+          {{- else }}
+          secretName: {{ template "consul.fullname" . }}-ca-cert
+          {{- end }}
+          items:
+          - key: {{ default "tls.crt" .Values.global.tls.caCert.secretKey }}
+            path: tls.crt
+      {{- end }}
+      {{- if .Values.global.tls.enableAutoEncrypt }}
+      - name: consul-auto-encrypt-ca-cert
+        emptyDir:
+          medium: "Memory"
+      {{- end }}
       {{- end }}
       {{- if or (and .Values.global.acls.manageSystemACLs) (and .Values.global.tls.enabled .Values.global.tls.enableAutoEncrypt) }}
       initContainers:

diff --git a/templates/connect-inject-mutatingwebhook.yaml b/templates/connect-inject-mutatingwebhook.yaml
@@ -26,7 +26,6 @@ webhooks:
         name: {{ template "consul.fullname" . }}-connect-injector-svc
         namespace: {{ .Release.Namespace }}
         path: "/mutate"
-      caBundle: {{ .Values.connectInject.certs.caBundle | quote }}
     rules:
       - operations: [ "CREATE" ]
         apiGroups: [""]

diff --git a/templates/connect-inject-serviceaccount.yaml b/templates/connect-inject-serviceaccount.yaml
@@ -1,4 +1,4 @@
-{{- if and (not .Values.connectInject.certs.secretName) (or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if or (and (ne (.Values.connectInject.enabled | toString) "-") .Values.connectInject.enabled) (and (eq (.Values.connectInject.enabled | toString) "-") .Values.global.enabled) }}
 apiVersion: v1
 kind: ServiceAccount
 metadata:

diff --git a/templates/webhook-cert-manager-clusterrole.yaml b/templates/webhook-cert-manager-clusterrole.yaml
@@ -1,4 +1,4 @@
-{{- if .Values.controller.enabled }}
+{{- if or .Values.connectInject.enabled .Values.controller.enabled}}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:

diff --git a/templates/webhook-cert-manager-clusterrolebinding.yaml b/templates/webhook-cert-manager-clusterrolebinding.yaml
@@ -1,4 +1,4 @@
-{{- if .Values.controller.enabled }}
+{{- if or .Values.connectInject.enabled .Values.controller.enabled}}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
 metadata:

diff --git a/templates/webhook-cert-manager-configmap.yaml b/templates/webhook-cert-manager-configmap.yaml
@@ -1,4 +1,4 @@
-{{- if .Values.controller.enabled }}
+{{- if or .Values.connectInject.enabled .Values.controller.enabled}}
 apiVersion: v1
 kind: ConfigMap
 metadata:
@@ -13,16 +13,30 @@ metadata:
 data:
   webhook-config.json: |-
     [
+    {{- if .Values.connectInject.enabled }}
+      {
+        "name": "{{ template "consul.fullname" . }}-connect-injector-cfg",
+        "tlsAutoHosts": [
+          "{{ template "consul.fullname" . }}-connect-injector-svc",
+          "{{ template "consul.fullname" . }}-connect-injector-svc.{{ .Release.Namespace }}",
+          "{{ template "consul.fullname" . }}-connect-injector-svc.{{ .Release.Namespace }}.svc",
+          "{{ template "consul.fullname" . }}-connect-injector-svc.{{ .Release.Namespace }}.svc.cluster.local"
+        ],
+        "secretName": "{{ template "consul.fullname" . }}-connect-inject-webhook-cert",
+        "secretNamespace": "{{ .Release.Namespace }}"
+      }{{- if and .Values.controller.enabled }},{{- end }}{{- end }}
+    {{- if and .Values.controller.enabled }}
       {
         "name": "{{ template "consul.fullname" . }}-controller-mutating-webhook-configuration",
         "tlsAutoHosts": [
           "{{ template "consul.fullname" . }}-controller-webhook",
+          "{{ template "consul.fullname" . }}-controller-webhook.{{ .Release.Namespace }}",
           "{{ template "consul.fullname" . }}-controller-webhook.{{ .Release.Namespace }}.svc",
           "{{ template "consul.fullname" . }}-controller-webhook.{{ .Release.Namespace }}.svc.cluster.local"
         ],
         "secretName": "{{ template "consul.fullname" . }}-controller-webhook-cert",
         "secretNamespace": "{{ .Release.Namespace }}"
       }
+    {{- end }}
     ]
-
-{{- end }}
+  {{- end }}
diff --git a/templates/webhook-cert-manager-deployment.yaml b/templates/webhook-cert-manager-deployment.yaml
@@ -1,4 +1,4 @@
-{{- if .Values.controller.enabled }}
+{{- if or .Values.connectInject.enabled .Values.controller.enabled}}
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -55,5 +55,4 @@ spec:
       - name: config
         configMap:
           name: {{ template "consul.fullname" . }}-webhook-cert-manager-config
-
 {{- end }}
diff --git a/templates/webhook-cert-manager-podsecuritypolicy.yaml b/templates/webhook-cert-manager-podsecuritypolicy.yaml
@@ -1,4 +1,4 @@
-{{- if and .Values.controller.enabled .Values.global.enablePodSecurityPolicies }}
+{{- if and (or .Values.controller.enabled .Values.connectInject.enabled) .Values.global.enablePodSecurityPolicies }}
 apiVersion: policy/v1beta1
 kind: PodSecurityPolicy
 metadata:

diff --git a/templates/webhook-cert-manager-serviceaccount.yaml b/templates/webhook-cert-manager-serviceaccount.yaml
@@ -1,4 +1,4 @@
-{{- if .Values.controller.enabled }}
+{{- if or .Values.connectInject.enabled .Values.controller.enabled}}
 apiVersion: v1
 kind: ServiceAccount
 metadata:

diff --git a/test/acceptance/tests/fixtures/bases/static-client/kustomization.yaml b/test/acceptance/tests/fixtures/bases/static-client/kustomization.yaml
@@ -1,4 +1,5 @@
 resources:
   - deployment.yaml
+  - service.yaml
   - serviceaccount.yaml
   - rolebinding.yaml
diff --git a/test/acceptance/tests/fixtures/bases/static-client/service.yaml b/test/acceptance/tests/fixtures/bases/static-client/service.yaml
@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: static-client
+spec:
+  selector:
+    app: static-client
+  ports:
+    - port: 80
diff --git a/test/unit/connect-inject-clusterrole.bats b/test/unit/connect-inject-clusterrole.bats
@@ -29,25 +29,6 @@ load _helpers
       .
 }
 
-@test "connectInject/ClusterRole: disabled with connectInject.certs.secretName set" {
-  cd `chart_dir`
-  assert_empty helm template \
-      -s templates/connect-inject-clusterrole.yaml  \
-      --set 'connectInject.enabled=true' \
-      --set 'connectInject.certs.secretName=foo' \
-      .
-}
-
-@test "connectInject/ClusterRole: enabled with connectInject.certs.secretName not set" {
-  cd `chart_dir`
-  local actual=$(helm template \
-      -s templates/connect-inject-clusterrole.yaml  \
-      --set 'connectInject.enabled=true' \
-      . | tee /dev/stderr |
-      yq 'length > 0' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-}
-
 #--------------------------------------------------------------------
 # global.enablePodSecurityPolicies