Apply suggestions from code review

Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
hashicorp · Mar 9, 2022 · 6c795dd · 6c795dd
1 parent a728869
commit 6c795dd
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 36 deletions.
diff --git a/charts/consul/templates/mesh-gateway-deployment.yaml b/charts/consul/templates/mesh-gateway-deployment.yaml
@@ -122,11 +122,16 @@ spec:
         {{- include "consul.getAutoEncryptClientCA" . | nindent 8 }}
         {{- end }}
         - name: mesh-gateway-init
+          image: {{ .Values.global.imageK8S }}
           env:
           - name: HOST_IP
             valueFrom:
               fieldRef:
                 fieldPath: status.hostIP
+          - name: POD_IP
+            valueFrom:
+              fieldRef:
+                fieldPath: status.podIP
           {{- if .Values.global.tls.enabled }}
           - name: CONSUL_CACERT
             value: /consul/tls/ca/tls.crt
@@ -137,27 +142,14 @@ spec:
             {{- else }}
             value: http://$(HOST_IP):8500
             {{- end }}
-          image: {{ .Values.global.imageK8S }}
-          volumeMounts:
-          - mountPath: /consul/login
-            name: consul-data
-            readOnly: false
-          {{- 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 }}
           command:
             - "/bin/sh"
             - "-ec"
             - |
                 {{- if .Values.global.acls.manageSystemACLs }}
                 consul-k8s-control-plane acl-init \
                   -component-name=mesh-gateway \
+                  -token-sink-file=/consul/service/acl-token \
                   {{- if and .Values.global.federation.enabled .Values.global.federation.primaryDatacenter }}
                   -acl-auth-method={{ template "consul.fullname" . }}-k8s-component-auth-method-{{ .Values.global.datacenter }} \
                   -primary-datacenter={{ .Values.global.federation.primaryDatacenter }} \
@@ -244,9 +236,23 @@ spec:
 
                 /consul-bin/consul services register \
                   {{- if .Values.global.acls.manageSystemACLs }}
-                  -token-file=/consul/login/acl-token \
+                  -token-file=/consul/service/acl-token \
                   {{- end }}
                   /consul/service/service.hcl
+          volumeMounts:
+            - name: consul-service
+              mountPath: /consul/service
+            - name: consul-bin
+              mountPath: /consul-bin
+            {{- 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 }}
           {{- if .Values.meshGateway.initServiceInitContainer.resources }}
           resources: {{ toYaml .Values.meshGateway.initServiceInitContainer.resources | nindent 12 }}
           {{- end }}
@@ -262,8 +268,8 @@ spec:
             {{- end }}
           {{- end }}
           volumeMounts:
-          - mountPath: /consul/login
-            name: consul-data
+          - mountPath: /consul/service
+            name: consul-service
             readOnly: true
           - name: consul-bin
             mountPath: /consul-bin
@@ -293,7 +299,7 @@ spec:
             {{- end }}
             {{- if .Values.global.acls.manageSystemACLs }}
             - name: CONSUL_HTTP_TOKEN_FILE
-              value: /consul/login/acl-token
+              value: /consul/service/acl-token
             {{- end }}
             {{- if .Values.global.tls.enabled }}
             - name: CONSUL_HTTP_ADDR
@@ -341,7 +347,7 @@ spec:
           lifecycle:
             preStop:
               exec:
-                command: ["/bin/sh", "-ec", "/consul-bin/consul logout -token-file=/consul/login/acl-token"]
+                command: ["/bin/sh", "-ec", "/consul-bin/consul services deregister -id=\"{{ .Values.meshGateway.consulServiceName }}\"", "&&", "/bin/sh", "-ec", "/consul-bin/consul logout"]
 
         # consul-sidecar ensures the mesh gateway is always registered with
         # the local Consul agent, even if it loses the initial registration.
@@ -393,7 +399,7 @@ spec:
             - -service-config=/consul/service/service.hcl
             - -consul-binary=/consul-bin/consul
             {{- if .Values.global.acls.manageSystemACLs }}
-            - -token-file=/consul/login/acl-token
+            - -token-file=/consul/service/acl-token
             {{- end }}
       {{- if .Values.meshGateway.priorityClassName }}
       priorityClassName: {{ .Values.meshGateway.priorityClassName | quote }}

diff --git a/charts/consul/test/unit/mesh-gateway-deployment.bats b/charts/consul/test/unit/mesh-gateway-deployment.bats
@@ -595,7 +595,7 @@ key2: value2' \
       --set 'connectInject.enabled=true' \
       --set 'global.acls.manageSystemACLs=true' \
       . | tee /dev/stderr |
-      yq '[.spec.template.spec.containers[0].lifecycle.preStop.exec.command[2]] | any(contains("logout"))' | tee /dev/stderr)
+      yq '[.spec.template.spec.containers[0].lifecycle.preStop.exec.command[6]] | any(contains("logout"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 }
 
@@ -641,11 +641,11 @@ key2: value2' \
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[1].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
+      yq '[.env[2].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[1].value] | any(contains("http://$(HOST_IP):8500"))' | tee /dev/stderr)
+      yq '[.env[2].value] | any(contains("http://$(HOST_IP):8500"))' | tee /dev/stderr)
       echo $actual
   [ "${actual}" = "true" ]
 }
@@ -666,20 +666,20 @@ key2: value2' \
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[1].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr)
+      yq '[.env[2].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[2].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
+      yq '[.env[3].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[2].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr)
+      yq '[.env[3].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr)
       echo $actual
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '.volumeMounts[1] | any(contains("consul-ca-cert"))' | tee /dev/stderr)
+      yq '.volumeMounts[2] | any(contains("consul-ca-cert"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 }
 
@@ -710,20 +710,20 @@ key2: value2' \
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[1].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr)
+      yq '[.env[2].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[2].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
+      yq '[.env[3].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[2].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr)
+      yq '[.env[3].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr)
       echo $actual
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '.volumeMounts[1] | any(contains("consul-ca-cert"))' | tee /dev/stderr)
+      yq '.volumeMounts[2] | any(contains("consul-ca-cert"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 }
 
@@ -744,20 +744,20 @@ key2: value2' \
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[1].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr)
+      yq '[.env[2].name] | any(contains("CONSUL_CACERT"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[2].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
+      yq '[.env[3].name] | any(contains("CONSUL_HTTP_ADDR"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '[.env[2].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr)
+      yq '[.env[3].value] | any(contains("https://$(HOST_IP):8501"))' | tee /dev/stderr)
       echo $actual
   [ "${actual}" = "true" ]
 
   local actual=$(echo $object |
-      yq '.volumeMounts[1] | any(contains("consul-auto-encrypt-ca-cert"))' | tee /dev/stderr)
+      yq '.volumeMounts[2] | any(contains("consul-auto-encrypt-ca-cert"))' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 }
 
@@ -1110,6 +1110,7 @@ EOF
 
   exp='consul-k8s-control-plane acl-init \
   -component-name=mesh-gateway \
+  -token-sink-file=/consul/service/acl-token \
   -acl-auth-method=RELEASE-NAME-consul-k8s-component-auth-method \
   -log-level=info \
   -log-json=false
@@ -1151,7 +1152,7 @@ service {
 EOF
 
 /consul-bin/consul services register \
-  -token-file=/consul/login/acl-token \
+  -token-file=/consul/service/acl-token \
   /consul/service/service.hcl'
 
   [ "${actual}" = "${exp}" ]