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.

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 || {}
 

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 })

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
+}

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 = {

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

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/

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

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}

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 }}

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 -}}

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 }}

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 }}

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 }}

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: {}