From 2d6758ebc53c2f7929a928847d253a00de645009 Mon Sep 17 00:00:00 2001 From: Olivier Michaelis <38879457+oliviermichaelis@users.noreply.github.com> Date: Sun, 3 Jan 2021 17:52:16 +0100 Subject: [PATCH] fix(pkg/chartutil) Remove disabled dependencies Remove aliased dependencies that are disabled with a condition. Before, if a chart had multiple aliased dependencies that reference the same chart, the metadata was shared between all dependencies. This led to dependencies not being processed correctly and resulted in a bug where disabled dependencies were not removed and therefore rendered. The metadatas dependencies are now deep-copied to prevent metadata corruption. Signed-off-by: Olivier Michaelis <38879457+oliviermichaelis@users.noreply.github.com> --- pkg/chartutil/dependencies.go | 9 +++ pkg/chartutil/dependencies_test.go | 22 ++++++ .../.helmignore | 23 ++++++ .../Chart.yaml | 14 ++++ .../charts/child/.helmignore | 23 ++++++ .../charts/child/Chart.yaml | 12 +++ .../child/charts/grandchild/.helmignore | 23 ++++++ .../charts/child/charts/grandchild/Chart.yaml | 23 ++++++ .../charts/grandchild/templates/_helpers.tpl | 62 +++++++++++++++ .../charts/grandchild/templates/service.yaml | 15 ++++ .../child/charts/grandchild/values.yaml | 7 ++ .../charts/child/values.yaml | 6 ++ .../values.yaml | 79 +++++++++++++++++++ 13 files changed, 318 insertions(+) create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/.helmignore create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/Chart.yaml create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/.helmignore create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/Chart.yaml create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/.helmignore create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/Chart.yaml create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/_helpers.tpl create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/service.yaml create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/values.yaml create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/values.yaml create mode 100644 pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/values.yaml diff --git a/pkg/chartutil/dependencies.go b/pkg/chartutil/dependencies.go index f0ba166d9..cab445adb 100644 --- a/pkg/chartutil/dependencies.go +++ b/pkg/chartutil/dependencies.go @@ -173,6 +173,15 @@ Loop: cd := []*chart.Chart{} copy(cd, c.Dependencies()[:0]) for _, n := range c.Dependencies() { + // The metadata dependencies need to be deep copied. + // Otherwise we risk metadata corruption in case of multiple dependencies sharing the same sub-dependency chart + var metadataDependencies []*chart.Dependency + for _, d := range n.Metadata.Dependencies { + dep := *d + metadataDependencies = append(metadataDependencies, &dep) + } + n.Metadata.Dependencies = metadataDependencies + if _, ok := rm[n.Metadata.Name]; !ok { cd = append(cd, n) } diff --git a/pkg/chartutil/dependencies_test.go b/pkg/chartutil/dependencies_test.go index ac8e4d76e..c717d2aa5 100644 --- a/pkg/chartutil/dependencies_test.go +++ b/pkg/chartutil/dependencies_test.go @@ -521,3 +521,25 @@ func TestDependentChartsWithSomeSubchartsSpecifiedInDependency(t *testing.T) { t.Fatalf("expected 1 dependency specified in Chart.yaml, got %d", len(c.Metadata.Dependencies)) } } + +func TestChartWithDependencyAliasedTwiceWithCondition(t *testing.T) { + c := loadChart(t, "testdata/chart-with-dependency-aliased-twice-with-condition") + + if len(c.Dependencies()) != 1 { + t.Fatalf("expected 1 dependencies for this chart, but got %d", len(c.Dependencies())) + } + + if err := processDependencyEnabled(c, c.Values, ""); err != nil { + t.Fatalf("expected no errors but got %q", err) + } + + if len(c.Dependencies()) != 2 { + t.Fatalf("expected 2 dependencies for this chart, but got %d", len(c.Dependencies())) + } + + for _, d := range c.Dependencies() { + if d.Dependencies() != nil { + t.Fatalf("expected no dependency in chart %s as the condition should disable it", d.Metadata.Name) + } + } +} diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/.helmignore b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/.helmignore new file mode 100644 index 000000000..0e8a0eb36 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/.helmignore @@ -0,0 +1,23 @@ +# 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 +*.orig +*~ +# Various IDEs +.project +.idea/ +*.tmproj +.vscode/ diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/Chart.yaml b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/Chart.yaml new file mode 100644 index 000000000..c8fef83a4 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/Chart.yaml @@ -0,0 +1,14 @@ +apiVersion: v2 +name: chart-with-dependency-aliased-twice-with-condition +description: A chart with a dependency that is referenced twice with different aliases. The aliased charts have a sub-dependency with a condition. +type: application +version: 0.1.0 +appVersion: 1.16.0 + +dependencies: + - name: child + alias: childFoo + version: "0.1.0" + - name: child + alias: childBar + version: "0.1.0" diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/.helmignore b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/.helmignore new file mode 100644 index 000000000..0e8a0eb36 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/.helmignore @@ -0,0 +1,23 @@ +# 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 +*.orig +*~ +# Various IDEs +.project +.idea/ +*.tmproj +.vscode/ diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/Chart.yaml b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/Chart.yaml new file mode 100644 index 000000000..890602456 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/Chart.yaml @@ -0,0 +1,12 @@ +apiVersion: v2 +name: child +description: A Helm chart for Kubernetes +type: application +version: 0.1.0 +appVersion: 1.16.0 + +dependencies: + - name: grandchild + alias: grandchildAlias + version: "0.1.0" + condition: grandchildAlias.enabled diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/.helmignore b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/.helmignore new file mode 100644 index 000000000..0e8a0eb36 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/.helmignore @@ -0,0 +1,23 @@ +# 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 +*.orig +*~ +# Various IDEs +.project +.idea/ +*.tmproj +.vscode/ diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/Chart.yaml b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/Chart.yaml new file mode 100644 index 000000000..b81606d7c --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/Chart.yaml @@ -0,0 +1,23 @@ +apiVersion: v2 +name: grandchild +description: A Helm chart for Kubernetes + +# A chart can be either an 'application' or a 'library' chart. +# +# Application charts are a collection of templates that can be packaged into versioned archives +# to be deployed. +# +# Library charts provide useful utilities or functions for the chart developer. They're included as +# a dependency of application charts to inject those utilities and functions into the rendering +# pipeline. Library charts do not define any templates and therefore cannot be deployed. +type: application + +# This is the chart version. This version number should be incremented each time you make changes +# to the chart and its templates, including the app version. +# Versions are expected to follow Semantic Versioning (https://semver.org/) +version: 0.1.0 + +# This is the version number of the application being deployed. This version number should be +# incremented each time you make changes to the application. Versions are not expected to +# follow Semantic Versioning. They should reflect the version the application is using. +appVersion: 1.16.0 diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/_helpers.tpl b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/_helpers.tpl new file mode 100644 index 000000000..736d3f3f7 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/_helpers.tpl @@ -0,0 +1,62 @@ +{{/* +Expand the name of the chart. +*/}} +{{- define "grandchild.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 "grandchild.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 "grandchild.chart" -}} +{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }} +{{- end }} + +{{/* +Common labels +*/}} +{{- define "grandchild.labels" -}} +helm.sh/chart: {{ include "grandchild.chart" . }} +{{ include "grandchild.selectorLabels" . }} +{{- if .Chart.AppVersion }} +app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +{{- end }} +app.kubernetes.io/managed-by: {{ .Release.Service }} +{{- end }} + +{{/* +Selector labels +*/}} +{{- define "grandchild.selectorLabels" -}} +app.kubernetes.io/name: {{ include "grandchild.name" . }} +app.kubernetes.io/instance: {{ .Release.Name }} +{{- end }} + +{{/* +Create the name of the service account to use +*/}} +{{- define "grandchild.serviceAccountName" -}} +{{- if .Values.serviceAccount.create }} +{{- default (include "grandchild.fullname" .) .Values.serviceAccount.name }} +{{- else }} +{{- default "default" .Values.serviceAccount.name }} +{{- end }} +{{- end }} diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/service.yaml b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/service.yaml new file mode 100644 index 000000000..d0ea4ed34 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/templates/service.yaml @@ -0,0 +1,15 @@ +apiVersion: v1 +kind: Service +metadata: + name: {{ include "grandchild.fullname" . }} + labels: + {{- include "grandchild.labels" . | nindent 4 }} +spec: + type: {{ .Values.service.type }} + ports: + - port: {{ .Values.service.port }} + targetPort: http + protocol: TCP + name: http + selector: + {{- include "grandchild.selectorLabels" . | nindent 4 }} diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/values.yaml b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/values.yaml new file mode 100644 index 000000000..30b8225c8 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/charts/grandchild/values.yaml @@ -0,0 +1,7 @@ +# Default values for grandchild. +# This is a YAML-formatted file. +# Declare variables to be passed into your templates. + +service: + type: ClusterIP + port: 80 diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/values.yaml b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/values.yaml new file mode 100644 index 000000000..e0496dd24 --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/charts/child/values.yaml @@ -0,0 +1,6 @@ +# Default values for child. +# This is a YAML-formatted file. +# Declare variables to be passed into your templates. + +grandchildAlias: + enabled: false diff --git a/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/values.yaml b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/values.yaml new file mode 100644 index 000000000..16befb85b --- /dev/null +++ b/pkg/chartutil/testdata/chart-with-dependency-aliased-twice-with-condition/values.yaml @@ -0,0 +1,79 @@ +# Default values for chart-with-dependency-aliased-twice-with-condition. +# This is a YAML-formatted file. +# Declare variables to be passed into your templates. + +replicaCount: 1 + +image: + repository: nginx + pullPolicy: IfNotPresent + # Overrides the image tag whose default is the chart appVersion. + tag: "" + +imagePullSecrets: [] +nameOverride: "" +fullnameOverride: "" + +serviceAccount: + # Specifies whether a service account should be created + create: true + # Annotations to add to the service account + annotations: {} + # The name of the service account to use. + # If not set and create is true, a name is generated using the fullname template + name: "" + +podAnnotations: {} + +podSecurityContext: {} + # fsGroup: 2000 + +securityContext: {} + # capabilities: + # drop: + # - ALL + # readOnlyRootFilesystem: true + # runAsNonRoot: true + # runAsUser: 1000 + +service: + type: ClusterIP + port: 80 + +ingress: + enabled: false + annotations: {} + # kubernetes.io/ingress.class: nginx + # kubernetes.io/tls-acme: "true" + hosts: + - host: chart-example.local + paths: [] + 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 + +autoscaling: + enabled: false + minReplicas: 1 + maxReplicas: 100 + targetCPUUtilizationPercentage: 80 + # targetMemoryUtilizationPercentage: 80 + +nodeSelector: {} + +tolerations: [] + +affinity: {}