From 1235fc71552c6d08c64c88892da40147ea027c22 Mon Sep 17 00:00:00 2001
From: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
Date: Mon, 13 Jan 2020 12:29:43 +0100
Subject: [PATCH] fix(helm): only hot reload serviceResource

Before this fix, the sync volume would be attached to all containers in
the target resource when using hot reloading with Helm modules, which is
not desirable e.g. when using user-specified sidecar containers.

Now, the sync volume is only attached to the container identified by
serviceResource.containerName or the Helm module's name.
---
 .../src/plugins/kubernetes/helm/common.ts     |  2 +-
 .../src/plugins/kubernetes/helm/deployment.ts |  6 +--
 .../src/plugins/kubernetes/helm/hot-reload.ts |  8 ++++
 .../src/plugins/kubernetes/hot-reload.ts      | 46 ++++++++----------
 .../data/test-projects/helm/api/Chart.yaml    |  4 ++
 .../helm/two-containers/.helmignore           | 22 +++++++++
 .../helm/two-containers/Chart.yaml            | 14 ++++++
 .../helm/two-containers/garden.yml            | 11 +++++
 .../helm/two-containers/templates/NOTES.txt   | 21 ++++++++
 .../two-containers/templates/_helpers.tpl     | 32 +++++++++++++
 .../two-containers/templates/deployment.yaml  | 46 ++++++++++++++++++
 .../two-containers/templates/ingress.yaml     | 40 ++++++++++++++++
 .../two-containers/templates/service.yaml     | 19 ++++++++
 .../helm/two-containers/values.yaml           | 48 +++++++++++++++++++
 .../src/plugins/kubernetes/helm/hot-reload.ts | 48 ++++++++++++++++++-
 15 files changed, 336 insertions(+), 31 deletions(-)
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/.helmignore
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/Chart.yaml
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/garden.yml
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/templates/NOTES.txt
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/templates/_helpers.tpl
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/templates/deployment.yaml
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/templates/ingress.yaml
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/templates/service.yaml
 create mode 100644 garden-service/test/data/test-projects/helm/two-containers/values.yaml

diff --git a/garden-service/src/plugins/kubernetes/helm/common.ts b/garden-service/src/plugins/kubernetes/helm/common.ts
index 7ad4fe871c..3ac665b2df 100644
--- a/garden-service/src/plugins/kubernetes/helm/common.ts
+++ b/garden-service/src/plugins/kubernetes/helm/common.ts
@@ -183,7 +183,7 @@ export function getReleaseName(config: HelmModuleConfig) {
  *
  * Throws error if no resource spec is configured, or it is empty.
  */
-export function getServiceResourceSpec(module: HelmModule) {
+export function getServiceResourceSpec(module: HelmModule): HelmResourceSpec {
   const baseModule = getBaseModule(module)
   let resourceSpec = module.spec.serviceResource || {}
 
diff --git a/garden-service/src/plugins/kubernetes/helm/deployment.ts b/garden-service/src/plugins/kubernetes/helm/deployment.ts
index 6e819e6a91..dfdd07c14d 100644
--- a/garden-service/src/plugins/kubernetes/helm/deployment.ts
+++ b/garden-service/src/plugins/kubernetes/helm/deployment.ts
@@ -23,7 +23,7 @@ import { configureHotReload, HotReloadableResource } from "../hot-reload"
 import { apply, deleteResources } from "../kubectl"
 import { KubernetesPluginContext } from "../config"
 import { ContainerHotReloadSpec } from "../../container/config"
-import { getHotReloadSpec } from "./hot-reload"
+import { getHotReloadSpec, getHotReloadContainerName } from "./hot-reload"
 import { DeployServiceParams } from "../../../types/plugin/service/deployService"
 import { DeleteServiceParams } from "../../../types/plugin/service/deleteService"
 import { getForwardablePorts, killPortForwards } from "../port-forward"
@@ -91,8 +91,8 @@ export async function deployHelmService({
     configureHotReload({
       target: hotReloadTarget,
       hotReloadSpec,
-      hotReloadArgs: resourceSpec && resourceSpec.hotReloadArgs,
-      containerName: resourceSpec && resourceSpec.containerName,
+      hotReloadArgs: resourceSpec.hotReloadArgs,
+      containerName: getHotReloadContainerName(module),
     })
 
     await apply({ log, provider, manifests: [hotReloadTarget], namespace })
diff --git a/garden-service/src/plugins/kubernetes/helm/hot-reload.ts b/garden-service/src/plugins/kubernetes/helm/hot-reload.ts
index 753099ae1b..a38df27cf9 100644
--- a/garden-service/src/plugins/kubernetes/helm/hot-reload.ts
+++ b/garden-service/src/plugins/kubernetes/helm/hot-reload.ts
@@ -89,3 +89,11 @@ export function getHotReloadSpec(service: HelmService) {
 
   return hotReloadSpec
 }
+
+/**
+ * Used to determine which container in the target resource to attach the hot reload sync volume to.
+ */
+export function getHotReloadContainerName(module: HelmModule) {
+  const resourceSpec = getServiceResourceSpec(module)
+  return resourceSpec.containerName || module.name
+}
diff --git a/garden-service/src/plugins/kubernetes/hot-reload.ts b/garden-service/src/plugins/kubernetes/hot-reload.ts
index 394254f003..ff190af269 100644
--- a/garden-service/src/plugins/kubernetes/hot-reload.ts
+++ b/garden-service/src/plugins/kubernetes/hot-reload.ts
@@ -58,14 +58,10 @@ export function configureHotReload({
   hotReloadCommand,
   hotReloadArgs,
   containerName,
-}: ConfigureHotReloadParams) {
+}: ConfigureHotReloadParams): void {
   const kind = <HotReloadableKind>target.kind
-
   set(target, ["metadata", "annotations", gardenAnnotationKey("hot-reload")], "true")
-
-  const containers = target.spec.template.spec.containers || []
   const mainContainer = getResourceContainer(target, containerName)
-
   const syncVolumeName = `garden-sync`
 
   // We're copying the target folder, not just its contents
@@ -96,31 +92,29 @@ export function configureHotReload({
     }
   })
 
-  for (const container of containers) {
-    if (!container.volumeMounts) {
-      container.volumeMounts = []
-    }
-    // This any cast (and a couple below) are necessary because of flaws in the TS definitions in the client library.
-    container.volumeMounts.push(...(<any>syncMounts))
+  if (!mainContainer.volumeMounts) {
+    mainContainer.volumeMounts = []
+  }
+  // This any cast (and a couple below) are necessary because of flaws in the TS definitions in the client library.
+  mainContainer.volumeMounts.push(...(<any>syncMounts))
 
-    if (!container.ports) {
-      container.ports = []
-    }
+  if (!mainContainer.ports) {
+    mainContainer.ports = []
+  }
 
-    if (container.ports.find((p) => p.containerPort === RSYNC_PORT)) {
-      throw new Error(deline`
-        ${kind} ${target.metadata.name} is configured for hot reload, but one of its containers uses
-        port ${RSYNC_PORT}, which is reserved for internal use while hot reload is active. Please remove
-        ${RSYNC_PORT} from your services' port config.`)
-    }
+  if (mainContainer.ports.find((p) => p.containerPort === RSYNC_PORT)) {
+    throw new Error(deline`
+      ${kind} ${target.metadata.name} is configured for hot reload, but one of its containers uses
+      port ${RSYNC_PORT}, which is reserved for internal use while hot reload is active. Please remove
+      ${RSYNC_PORT} from your services' port config.`)
+  }
 
-    if (hotReloadCommand) {
-      container.command = hotReloadCommand
-    }
+  if (hotReloadCommand) {
+    mainContainer.command = hotReloadCommand
+  }
 
-    if (hotReloadArgs) {
-      container.args = hotReloadArgs
-    }
+  if (hotReloadArgs) {
+    mainContainer.args = hotReloadArgs
   }
 
   const rsyncContainer = {
diff --git a/garden-service/test/data/test-projects/helm/api/Chart.yaml b/garden-service/test/data/test-projects/helm/api/Chart.yaml
index 6b53945ade..12285e770f 100644
--- a/garden-service/test/data/test-projects/helm/api/Chart.yaml
+++ b/garden-service/test/data/test-projects/helm/api/Chart.yaml
@@ -3,3 +3,7 @@ appVersion: "1.0"
 description: A Helm chart for Kubernetes
 name: api
 version: 0.1.0
+image:
+  repository: busybox
+  tag: latest
+  pullPolicy: IfNotPresent
diff --git a/garden-service/test/data/test-projects/helm/two-containers/.helmignore b/garden-service/test/data/test-projects/helm/two-containers/.helmignore
new file mode 100644
index 0000000000..50af031725
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/.helmignore
@@ -0,0 +1,22 @@
+# Patterns to ignore when building packages.
+# This supports shell glob matching, relative path matching, and
+# negation (prefixed with !). Only one pattern per line.
+.DS_Store
+# Common VCS dirs
+.git/
+.gitignore
+.bzr/
+.bzrignore
+.hg/
+.hgignore
+.svn/
+# Common backup files
+*.swp
+*.bak
+*.tmp
+*~
+# Various IDEs
+.project
+.idea/
+*.tmproj
+.vscode/
diff --git a/garden-service/test/data/test-projects/helm/two-containers/Chart.yaml b/garden-service/test/data/test-projects/helm/two-containers/Chart.yaml
new file mode 100644
index 0000000000..71f966fd78
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/Chart.yaml
@@ -0,0 +1,14 @@
+apiVersion: v1
+appVersion: "1.0"
+description: Helm chart with two containers
+name: two-containers
+version: 0.1.0
+image:
+  repository: busybox
+  tag: latest
+  pullPolicy: IfNotPresent
+service:
+  name: busybox
+  type: ClusterIP
+  externalPort: 80
+  internalPort: 80
diff --git a/garden-service/test/data/test-projects/helm/two-containers/garden.yml b/garden-service/test/data/test-projects/helm/two-containers/garden.yml
new file mode 100644
index 0000000000..395eb1988c
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/garden.yml
@@ -0,0 +1,11 @@
+kind: Module
+description: Helm module with two containers
+type: helm
+name: two-containers
+releaseName: two-containers-release
+serviceResource:
+  kind: Deployment
+  containerModule: api-image
+values:
+  image:
+    tag: ${modules.api-image.version}
diff --git a/garden-service/test/data/test-projects/helm/two-containers/templates/NOTES.txt b/garden-service/test/data/test-projects/helm/two-containers/templates/NOTES.txt
new file mode 100644
index 0000000000..0abc04b517
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/templates/NOTES.txt
@@ -0,0 +1,21 @@
+1. Get the application URL by running these commands:
+{{- if .Values.ingress.enabled }}
+{{- range $host := .Values.ingress.hosts }}
+  {{- range $.Values.ingress.paths }}
+  http{{ if $.Values.ingress.tls }}s{{ end }}://{{ $host }}{{ . }}
+  {{- end }}
+{{- end }}
+{{- else if contains "NodePort" .Values.service.type }}
+  export NODE_PORT=$(kubectl get --namespace {{ .Release.Namespace }} -o jsonpath="{.spec.ports[0].nodePort}" services {{ include "two-containers.fullname" . }})
+  export NODE_IP=$(kubectl get nodes --namespace {{ .Release.Namespace }} -o jsonpath="{.items[0].status.addresses[0].address}")
+  echo http://$NODE_IP:$NODE_PORT
+{{- else if contains "LoadBalancer" .Values.service.type }}
+     NOTE: It may take a few minutes for the LoadBalancer IP to be available.
+           You can watch the status of by running 'kubectl get svc -w {{ include "two-containers.fullname" . }}'
+  export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "two-containers.fullname" . }} -o jsonpath='{.status.loadBalancer.ingress[0].ip}')
+  echo http://$SERVICE_IP:{{ .Values.service.port }}
+{{- else if contains "ClusterIP" .Values.service.type }}
+  export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "two-containers.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}")
+  echo "Visit http://127.0.0.1:8080 to use your application"
+  kubectl port-forward $POD_NAME 8080:80
+{{- end }}
diff --git a/garden-service/test/data/test-projects/helm/two-containers/templates/_helpers.tpl b/garden-service/test/data/test-projects/helm/two-containers/templates/_helpers.tpl
new file mode 100644
index 0000000000..9a3ba6f737
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/templates/_helpers.tpl
@@ -0,0 +1,32 @@
+{{/* vim: set filetype=mustache: */}}
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "two-containers.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
+{{- end -}}
+
+{{/*
+Create a default fully qualified app name.
+We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "two-containers.fullname" -}}
+{{- if .Values.fullnameOverride -}}
+{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
+{{- else -}}
+{{- $name := default .Chart.Name .Values.nameOverride -}}
+{{- if contains $name .Release.Name -}}
+{{- .Release.Name | trunc 63 | trimSuffix "-" -}}
+{{- else -}}
+{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
+{{- end -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Create chart name and version as used by the chart label.
+*/}}
+{{- define "two-containers.chart" -}}
+{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}}
+{{- end -}}
diff --git a/garden-service/test/data/test-projects/helm/two-containers/templates/deployment.yaml b/garden-service/test/data/test-projects/helm/two-containers/templates/deployment.yaml
new file mode 100644
index 0000000000..6ed2892eaa
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/templates/deployment.yaml
@@ -0,0 +1,46 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: {{ include "two-containers.fullname" . }}
+  labels:
+    app.kubernetes.io/name: {{ include "two-containers.name" . }}
+    helm.sh/chart: {{ include "two-containers.chart" . }}
+    app.kubernetes.io/instance: {{ .Release.Name }}
+    app.kubernetes.io/managed-by: {{ .Release.Service }}
+spec:
+  replicas: {{ .Values.replicaCount }}
+  selector:
+    matchLabels:
+      app.kubernetes.io/name: {{ include "two-containers.name" . }}
+      app.kubernetes.io/instance: {{ .Release.Name }}
+  template:
+    metadata:
+      labels:
+        app.kubernetes.io/name: {{ include "two-containers.name" . }}
+        app.kubernetes.io/instance: {{ .Release.Name }}
+    spec:
+      containers:
+        - name: second-container
+          image: busybox
+          command: ['sh', '-c', 'echo The container is running! && sleep 3600']
+        - name: {{ .Chart.Name }}
+          image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
+          imagePullPolicy: {{ .Values.image.pullPolicy }}
+          ports:
+            - name: http
+              containerPort: 80
+              protocol: TCP
+          resources:
+            {{- toYaml .Values.resources | nindent 12 }}
+      {{- with .Values.nodeSelector }}
+      nodeSelector:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+    {{- with .Values.affinity }}
+      affinity:
+        {{- toYaml . | nindent 8 }}
+    {{- end }}
+    {{- with .Values.tolerations }}
+      tolerations:
+        {{- toYaml . | nindent 8 }}
+    {{- end }}
diff --git a/garden-service/test/data/test-projects/helm/two-containers/templates/ingress.yaml b/garden-service/test/data/test-projects/helm/two-containers/templates/ingress.yaml
new file mode 100644
index 0000000000..787262345a
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/templates/ingress.yaml
@@ -0,0 +1,40 @@
+{{- if .Values.ingress.enabled -}}
+{{- $fullName := include "two-containers.fullname" . -}}
+{{- $ingressPaths := .Values.ingress.paths -}}
+apiVersion: extensions/v1beta1
+kind: Ingress
+metadata:
+  name: {{ $fullName }}
+  labels:
+    app.kubernetes.io/name: {{ include "two-containers.name" . }}
+    helm.sh/chart: {{ include "two-containers.chart" . }}
+    app.kubernetes.io/instance: {{ .Release.Name }}
+    app.kubernetes.io/managed-by: {{ .Release.Service }}
+  {{- with .Values.ingress.annotations }}
+  annotations:
+    {{- toYaml . | nindent 4 }}
+  {{- end }}
+spec:
+{{- if .Values.ingress.tls }}
+  tls:
+  {{- range .Values.ingress.tls }}
+    - hosts:
+      {{- range .hosts }}
+        - {{ . | quote }}
+      {{- end }}
+      secretName: {{ .secretName }}
+  {{- end }}
+{{- end }}
+  rules:
+  {{- range .Values.ingress.hosts }}
+    - host: {{ . | quote }}
+      http:
+        paths:
+	{{- range $ingressPaths }}
+          - path: {{ . }}
+            backend:
+              serviceName: {{ $fullName }}
+              servicePort: http
+	{{- end }}
+  {{- end }}
+{{- end }}
diff --git a/garden-service/test/data/test-projects/helm/two-containers/templates/service.yaml b/garden-service/test/data/test-projects/helm/two-containers/templates/service.yaml
new file mode 100644
index 0000000000..4afc42761a
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/templates/service.yaml
@@ -0,0 +1,19 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: {{ include "two-containers.fullname" . }}
+  labels:
+    app.kubernetes.io/name: {{ include "two-containers.name" . }}
+    helm.sh/chart: {{ include "two-containers.chart" . }}
+    app.kubernetes.io/instance: {{ .Release.Name }}
+    app.kubernetes.io/managed-by: {{ .Release.Service }}
+spec:
+  type: {{ .Values.service.type }}
+  ports:
+    - port: {{ .Values.service.port }}
+      targetPort: http
+      protocol: TCP
+      name: http
+  selector:
+    app.kubernetes.io/name: {{ include "two-containers.name" . }}
+    app.kubernetes.io/instance: {{ .Release.Name }}
diff --git a/garden-service/test/data/test-projects/helm/two-containers/values.yaml b/garden-service/test/data/test-projects/helm/two-containers/values.yaml
new file mode 100644
index 0000000000..7bbb6c9188
--- /dev/null
+++ b/garden-service/test/data/test-projects/helm/two-containers/values.yaml
@@ -0,0 +1,48 @@
+# Default values for api.
+# This is a YAML-formatted file.
+# Declare variables to be passed into your templates.
+
+replicaCount: 1
+
+image:
+  repository: api-image
+  tag: stable
+  pullPolicy: IfNotPresent
+
+nameOverride: ""
+fullnameOverride: ""
+
+service:
+  type: ClusterIP
+  port: 80
+
+ingress:
+  enabled: false
+  annotations: {}
+    # kubernetes.io/ingress.class: nginx
+    # kubernetes.io/tls-acme: "true"
+  paths: []
+  hosts:
+    - chart-example.local
+  tls: []
+  #  - secretName: chart-example-tls
+  #    hosts:
+  #      - chart-example.local
+
+resources: {}
+  # We usually recommend not to specify default resources and to leave this as a conscious
+  # choice for the user. This also increases chances charts run on environments with little
+  # resources, such as Minikube. If you do want to specify resources, uncomment the following
+  # lines, adjust them as necessary, and remove the curly braces after 'resources:'.
+  # limits:
+  #  cpu: 100m
+  #  memory: 128Mi
+  # requests:
+  #  cpu: 100m
+  #  memory: 128Mi
+
+nodeSelector: {}
+
+tolerations: []
+
+affinity: {}
diff --git a/garden-service/test/integ/src/plugins/kubernetes/helm/hot-reload.ts b/garden-service/test/integ/src/plugins/kubernetes/helm/hot-reload.ts
index 4d9f6c1719..9f25a03bf7 100644
--- a/garden-service/test/integ/src/plugins/kubernetes/helm/hot-reload.ts
+++ b/garden-service/test/integ/src/plugins/kubernetes/helm/hot-reload.ts
@@ -1,10 +1,18 @@
 import { expect } from "chai"
 
 import { TestGarden, expectError } from "../../../../../helpers"
-import { getHotReloadSpec } from "../../../../../../src/plugins/kubernetes/helm/hot-reload"
+import { getHotReloadSpec, getHotReloadContainerName } from "../../../../../../src/plugins/kubernetes/helm/hot-reload"
 import { deline } from "../../../../../../src/util/string"
 import { ConfigGraph } from "../../../../../../src/config-graph"
 import { getHelmTestGarden } from "./common"
+import {
+  findServiceResource,
+  getChartResources,
+  getServiceResourceSpec,
+} from "../../../../../../src/plugins/kubernetes/helm/common"
+import { PluginContext } from "../../../../../../src/plugin-context"
+import { KubernetesProvider } from "../../../../../../src/plugins/kubernetes/config"
+import { configureHotReload } from "../../../../../../src/plugins/kubernetes/hot-reload"
 
 describe("getHotReloadSpec", () => {
   let garden: TestGarden
@@ -71,3 +79,41 @@ describe("getHotReloadSpec", () => {
     )
   })
 })
+
+describe("configureHotReload", () => {
+  let garden: TestGarden
+  let graph: ConfigGraph
+  let provider: KubernetesProvider
+  let ctx: PluginContext
+
+  before(async () => {
+    garden = await getHelmTestGarden()
+    provider = <KubernetesProvider>await garden.resolveProvider("local-kubernetes")
+    ctx = garden.getPluginContext(provider)
+  })
+
+  beforeEach(async () => {
+    graph = await garden.getConfigGraph(garden.log)
+  })
+
+  it("should only mount the sync volume on the main/resource container", async () => {
+    const log = garden.log
+    const service = await graph.getService("two-containers")
+    const module = service.module
+    const chartResources = await getChartResources(ctx, module, true, log)
+    const resourceSpec = getServiceResourceSpec(module)
+    const hotReloadSpec = getHotReloadSpec(service)
+    const hotReloadTarget = await findServiceResource({ ctx, log, module, chartResources, resourceSpec })
+    const containerName = getHotReloadContainerName(module)
+    configureHotReload({
+      containerName,
+      hotReloadSpec,
+      target: hotReloadTarget,
+    })
+    const containers: any[] = hotReloadTarget.spec.template.spec.containers
+    // This is a second, non-main/resource container included by the Helm chart, which should not mount the sync volume.
+    const secondContainer = containers.find((c) => c.name === "second-container")
+
+    expect(secondContainer.volumeMounts || []).to.be.empty
+  })
+})