Resolve conflicts in helm templates/values (#504)

Fixes: aws-controllers-k8s/community#2014

Prior to this patch, we observed some weird conflicts when installing multiple
ACK helm charts (as sub charts), e.g `ack-chart`, where the last controller
installation would override values/templates from the previous ones.
This issue stemmed from the asumption that helm "template" names are
unique across installations, which was a miss from the ACK team.

This patch addresses the conflict by prefixing all the helm template
variables with a `ack-$SERVICE-controller` string.

This solution comes with the cost of adding complexity to the already
complex helm "templates templates generator"... However the least we
could do is to leverage Go template "Funcs" to reduce the complexity of
writing those templates. For example instead of writing something like
`{{ "{{ \"define \"ack- }}" }}{{ .ServicePackageName }}{{
"-controller.rbac-rules\"" }}`,
we could "simply" write a `{{ DefineTemplate "rbac-rules" }}`

Signed-off-by: Amine Hilaly <[email protected]>

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pkg/generate/ack/release.go

@@ -14,6 +14,7 @@
 package ack
 
 import (
+	"fmt"
 	"strings"
 	ttpl "text/template"
 
@@ -27,6 +28,7 @@ var (
 		"config/controller/kustomization.yaml.tpl",
 		"helm/templates/cluster-role-binding.yaml.tpl",
 		"helm/templates/cluster-role-controller.yaml.tpl",
+		"helm/templates/_helpers.tpl.tpl",
 		"helm/Chart.yaml.tpl",
 		"helm/values.yaml.tpl",
 		"helm/values.schema.json",
@@ -37,24 +39,48 @@ var (
 		"helm/templates/caches-role-binding.yaml.tpl",
 		"helm/templates/leader-election-role.yaml.tpl",
 		"helm/templates/leader-election-role-binding.yaml.tpl",
+		"helm/templates/deployment.yaml.tpl",
+		"helm/templates/metrics-service.yaml.tpl",
+		"helm/templates/service-account.yaml.tpl",
 	}
 	releaseIncludePaths = []string{
 		"config/controller/kustomization_def.yaml.tpl",
 	}
-	releaseCopyPaths = []string{
-		"helm/templates/_helpers.tpl",
-		"helm/templates/deployment.yaml",
-		"helm/templates/metrics-service.yaml",
-		"helm/templates/service-account.yaml",
-	}
-	releaseFuncMap = ttpl.FuncMap{
-		"ToLower": strings.ToLower,
-		"Empty": func(subject string) bool {
-			return strings.TrimSpace(subject) == ""
-		},
+	releaseCopyPaths = []string{}
+	releaseFuncMap   = func(serviceName string) ttpl.FuncMap {
+		return ttpl.FuncMap{
+			"ToLower": strings.ToLower,
+			"Empty": func(subject string) bool {
+				return strings.TrimSpace(subject) == ""
+			},
+			"DefineTemplate": func(templateName string) string {
+				// Returnes a statement that defines a new template name with unique
+				// prefix for the ACK controller.
+				// For example, if serviceName is "s3" and templateName is "app.name"
+				// it will return {{- define "ack-s3-controller.app.name" -}}
+				return fmt.Sprintf("{{- define \"%s\" -}}", prefixServiceTemplateName(serviceName, templateName))
+			},
+			"IncludeTemplate": func(templateName string) string {
+				// Returns a statement that includes a template defined with DefineTemplate.
+				// For example, if serviceName is "s3" and templateName is "app.name"
+				// it will return {{- include "ack-s3-controller.app.name" . -}}
+				return fmt.Sprintf("{{ include \"%s\" . }}", prefixServiceTemplateName(serviceName, templateName))
+			},
+			"VarIncludeTemplate": func(variableName, templateName string) string {
+				// Returns a statement that declares a variable and includes a template defined with
+				// DefineTemplate.
+				// For example, if variableName is appName, serviceName is "s3", and templateName is "app.name"
+				// it will return {{- $variable := include "ack-s3-controller.app.name" .app.name -}}
+				return fmt.Sprintf("{{ $%s := include \"%s\" . }}", variableName, prefixServiceTemplateName(serviceName, templateName))
+			},
+		}
 	}
 )
 
+func prefixServiceTemplateName(serviceName, templateName string) string {
+	return fmt.Sprintf("ack-%s-controller.%s", serviceName, templateName)
+}
+
 // Release returns a pointer to a TemplateSet containing all the templates for
 // generating an ACK service controller release (Helm artifacts, etc)
 func Release(
@@ -74,10 +100,10 @@ func Release(
 		templateBasePaths,
 		releaseIncludePaths,
 		releaseCopyPaths,
-		releaseFuncMap,
+		releaseFuncMap(m.MetaVars().ServicePackageName),
 	)
-
 	metaVars := m.MetaVars()
+
 	releaseVars := &templateReleaseVars{
 		metaVars,
 		ImageReleaseVars{

templates/helm/templates/_helpers.tpl.tpl

@@ -0,0 +1,53 @@
+{{ "{{/* The name of the application this chart installs */}}" }}
+{{ DefineTemplate "app.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." }}
+{{ "*/}}" }}
+{{ DefineTemplate "app.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 -}}" }}
+
+{{ "{{/* The name and version as used by the chart label */}}" }}
+{{ DefineTemplate "chart.name-version" }}
+{{ "{{- printf \"%s-%s\" .Chart.Name .Chart.Version | replace \"+\" \"_\" | trunc 63 | trimSuffix \"-\" -}}" }}
+{{ "{{- end -}}" }}
+
+{{ "{{/* The name of the service account to use */}}" }}
+{{ DefineTemplate "service-account.name" }}
+    {{ "{{ default \"default\" .Values.serviceAccount.name }}" }}
+{{ "{{- end -}}" }}
+
+{{ DefineTemplate "watch-namespace" }}
+{{ "{{- if eq .Values.installScope \"namespace\" -}}" }}
+{{ "{{ .Values.watchNamespace | default .Release.Namespace }}" }}
+{{ "{{- end -}}" }}
+{{ "{{- end -}}" }}
+
+{{ "{{/* The mount path for the shared credentials file */}}" }}
+{{ DefineTemplate "aws.credentials.secret_mount_path" }}
+{{ "{{- \"/var/run/secrets/aws\" -}}" }}
+{{ "{{- end -}}" }}
+
+{{ "{{/* The path the shared credentials file is mounted */}}" }}
+{{ DefineTemplate "aws.credentials.path" }}
+{{ "{{- printf \"%s/%s\" (include \"aws.credentials.secret_mount_path\" .) .Values.aws.credentials.secretKey -}}" }}
+{{ "{{- end -}}" }}
+
+{{ "{{/* The rules a of ClusterRole or Role */}}" }}
+{{ DefineTemplate "rbac-rules" }}
+SEDREPLACERULES
+{{ "{{- end }}" }}

templates/helm/templates/cluster-role-binding.yaml.tpl

@@ -2,21 +2,21 @@
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
 metadata:
-  name: {{ "{{ include \"app.fullname\" . }}" }}
+  name: {{ IncludeTemplate "app.fullname" }}
 roleRef:
   kind: ClusterRole
   apiGroup: rbac.authorization.k8s.io
   name: ack-{{ .ServicePackageName }}-controller
 subjects:
 - kind: ServiceAccount
-  name: {{ "{{ include \"service-account.name\" . }}" }}
+  name: {{ IncludeTemplate "service-account.name" }}
   namespace: {{ "{{ .Release.Namespace }}" }}
 {{ "{{ else if eq .Values.installScope \"namespace\" }}" }}
-{{ "{{ $wn := include \"watch-namespace\" . }}" }}
+{{ VarIncludeTemplate "wn" "watch-namespace" }}
 {{ "{{ $namespaces := split \",\" $wn }}" }}
-{{ "{{ $fullname := include \"app.fullname\" .  }}" }}
+{{ VarIncludeTemplate "fullname" "app.fullname" }}
 {{ "{{ $releaseNamespace := .Release.Namespace }}" }}
-{{ "{{ $serviceAccountName := include \"service-account.name\" .  }}" }}
+{{ VarIncludeTemplate "serviceAccountName" "service-account.name" }}
 {{ "{{ range $namespaces }}" }}
 ---
 apiVersion: rbac.authorization.k8s.io/v1

templates/helm/templates/cluster-role-controller.yaml.tpl

@@ -1,5 +1,5 @@
 {{ "{{ $labels := .Values.role.labels }}" }}
-{{ "{{ $rules := include \"controller-role-rules\" . }}" }}
+{{ VarIncludeTemplate "rbacRules" "rbac-rules" }}
 {{ "{{ if eq .Values.installScope \"cluster\" }}" }}
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
@@ -9,9 +9,9 @@ metadata:
   {{ "{{- range $key, $value := $labels }}" }}
     {{ "{{ $key }}: {{ $value | quote }}" }}
   {{ "{{- end }}" }}
-{{ "{{- $rules }}" }}
+{{ "{{$rbacRules }}" }}
 {{ "{{ else if eq .Values.installScope \"namespace\" }}" }}
-{{ "{{ $wn := include \"watch-namespace\" . }}" }}
+{{ VarIncludeTemplate "wn" "watch-namespace" }}
 {{ "{{ $namespaces := split \",\" $wn }}" }}
 {{ "{{ range $namespaces }}" }}
 ---
@@ -24,6 +24,6 @@ metadata:
   {{ "{{- range $key, $value := $labels }}" }}
     {{ "{{ $key }}: {{ $value | quote }}" }}
   {{ "{{- end }}" }}
-{{ "{{- $rules }}" }}
+{{ "{{ $rbacRules }}" }}
 {{ "{{ end }}" }}
 {{ "{{ end }}" }}