From 8c0db94845552a4ff536c16484999b510b73a01d Mon Sep 17 00:00:00 2001 From: Clemens Beck Date: Tue, 12 Dec 2023 13:29:04 +0100 Subject: [PATCH 1/2] Use timestamp instead of helm revision in job names Improve support for `helm template` based tools such as Helmfile and ArgoCD by replacing `.Release.Revision` with timestamp values in Job names. The release revision is always evaluated to 1 and can not be overridden when `helm template` is invoked. Changelog: changed --- .../certmanager-issuer/templates/_helpers.tpl | 5 ++-- .../charts/migrations/templates/_helpers.tpl | 9 +++---- charts/minio/templates/_helpers.tpl | 9 +++---- charts/registry/templates/_helpers.tpl | 7 ++--- spec/configuration/global_spec.rb | 9 ++++--- spec/configuration/migrations_spec.rb | 26 ++++++++++--------- spec/configuration/pages_spec.rb | 3 ++- spec/configuration/securitycontext_spec.rb | 3 ++- spec/helm_template_helper.rb | 7 +++++ templates/_helpers.tpl | 17 +++++++----- 10 files changed, 57 insertions(+), 38 deletions(-) diff --git a/charts/certmanager-issuer/templates/_helpers.tpl b/charts/certmanager-issuer/templates/_helpers.tpl index 6f28a068a7..94f32c4792 100644 --- a/charts/certmanager-issuer/templates/_helpers.tpl +++ b/charts/certmanager-issuer/templates/_helpers.tpl @@ -23,8 +23,9 @@ Due to the helm delete not cleaning up these jobs, we add a random value to reduce collision */}} {{- define "certmanager-issuer.jobname" -}} -{{- $name := printf "%s-issuer" .Release.Name | trunc 55 | trimSuffix "-" -}} -{{- printf "%s-%d" $name .Release.Revision | trunc 63 | trimSuffix "-" -}} +{{- $name := printf "%s-issuer" .Release.Name | trunc 41 | trimSuffix "-" -}} +{{- $timestamp := include "gitlab.timestamp" . }} +{{- printf "%s-%s" $name $timestamp | trunc 63 | trimSuffix "-" -}} {{- end -}} {{/* diff --git a/charts/gitlab/charts/migrations/templates/_helpers.tpl b/charts/gitlab/charts/migrations/templates/_helpers.tpl index c7ff8dccdb..c1a8eafb8f 100644 --- a/charts/gitlab/charts/migrations/templates/_helpers.tpl +++ b/charts/gitlab/charts/migrations/templates/_helpers.tpl @@ -2,12 +2,11 @@ {{/* Create a default fully qualified job name. -Due to the job only being allowed to run once, we add the chart revision so helm +Due to the job only being allowed to run once, we add a timestamp so helm upgrades don't cause errors trying to create the already ran job. -Due to the helm delete not cleaning up these jobs, we add a randome value to -reduce collision */}} {{- define "migrations.jobname" -}} -{{- $name := include "fullname" . | trunc 55 | trimSuffix "-" -}} -{{- printf "%s-%d" $name .Release.Revision | trunc 63 | trimSuffix "-" -}} +{{- $name := include "fullname" . | trunc 41 | trimSuffix "-" -}} +{{- $timestamp := include "gitlab.timestamp" . }} +{{- printf "%s-%s" $name $timestamp | trunc 63 | trimSuffix "-" -}} {{- end -}} diff --git a/charts/minio/templates/_helpers.tpl b/charts/minio/templates/_helpers.tpl index 83dc6b11e5..497af2b5e6 100755 --- a/charts/minio/templates/_helpers.tpl +++ b/charts/minio/templates/_helpers.tpl @@ -28,14 +28,13 @@ Return the appropriate apiVersion for networkpolicy. {{/* Create a default fully qualified job name for creating default buckets. -Due to the job only being allowed to run once, we add the chart revision so helm +Due to the job only being allowed to run once, we add a timestamp so helm upgrades don't cause errors trying to create the already ran job. -Due to the helm delete not cleaning up these jobs, we add a random value to -reduce collision */}} {{- define "minio.createBucketsJobName" -}} -{{- $name := include "minio.fullname" . | trunc 40 | trimSuffix "-" -}} -{{- printf "%s-create-buckets-%d" $name .Release.Revision -}} +{{- $name := include "minio.fullname" . | trunc 41 | trimSuffix "-" -}} +{{- $timestamp := include "gitlab.timestamp" . }} +{{- printf "%s-%s" $name $timestamp | trunc 63 | trimSuffix "-" -}} {{- end -}} {{/* diff --git a/charts/registry/templates/_helpers.tpl b/charts/registry/templates/_helpers.tpl index 58cf86114b..ee9fdccdb9 100644 --- a/charts/registry/templates/_helpers.tpl +++ b/charts/registry/templates/_helpers.tpl @@ -159,10 +159,11 @@ Failing that a serviceAccount will be generated automatically {{/* Create a default fully qualified job name. -Due to the job only being allowed to run once, we add the chart revision so Helm +Due to the job only being allowed to run once, we add a timestamp so helm upgrades don't cause errors trying to create the already ran job. */}} {{- define "registry.migrations.jobname" -}} -{{- $name := include "registry.fullname" . | trunc 55 | trimSuffix "-" -}} -{{- printf "%s-migrations-%d" $name .Release.Revision | trunc 63 | trimSuffix "-" -}} +{{- $name := include "registry.fullname" . | trunc 41 | trimSuffix "-" -}} +{{- $timestamp := include "gitlab.timestamp" . }} +{{- printf "%s-%s" $name $timestamp | trunc 63 | trimSuffix "-" -}} {{- end -}} diff --git a/spec/configuration/global_spec.rb b/spec/configuration/global_spec.rb index 68cdc64f7f..1af7f18ece 100644 --- a/spec/configuration/global_spec.rb +++ b/spec/configuration/global_spec.rb @@ -173,6 +173,10 @@ describe 'global configuration' do )).deep_merge(default_values) end + let(:t) do + HelmTemplate.new(shell_values) + end + let(:ignored_deployments) do [ 'Deployment/test-gitlab-runner', @@ -190,14 +194,13 @@ describe 'global configuration' do let(:ignored_jobs) do [ - 'Job/test-minio-create-buckets-1', - 'Job/test-shared-secrets-1', + t.resources_by_kind_and_labels("Job", { "app" => "minio" }).keys.first, + t.resources_by_kind_and_labels("Job", { "app" => "gitlab" }).keys.first, # shared-secrets 'Job/test-gitlab-upgrade-check' ] end it 'adds the provided suffix to all image tags' do - t = HelmTemplate.new(shell_values) expect(t.exit_code).to eq(0), "Unexpected error code #{t.exit_code} -- #{t.stderr}" objects = t.resources_by_kind('Deployment').reject { |key, _| ignored_deployments.include? key } diff --git a/spec/configuration/migrations_spec.rb b/spec/configuration/migrations_spec.rb index 550b15dec8..996abb5b21 100644 --- a/spec/configuration/migrations_spec.rb +++ b/spec/configuration/migrations_spec.rb @@ -42,13 +42,14 @@ describe 'migrations configuration' do it 'Populates the additional labels in the expected manner' do t = HelmTemplate.new(values) expect(t.exit_code).to eq(0), "Unexpected error code #{t.exit_code} -- #{t.stderr}" + migrations_job = t.resources_by_kind_and_labels("Job", { "app" => "migrations" }).values.first expect(t.dig('ConfigMap/test-migrations', 'metadata', 'labels')).to include('global' => 'migrations') - expect(t.dig('Job/test-migrations-1', 'metadata', 'labels')).to include('foo' => 'global') - expect(t.dig('Job/test-migrations-1', 'metadata', 'labels')).to include('global' => 'migrations') - expect(t.dig('Job/test-migrations-1', 'metadata', 'labels')).not_to include('global' => 'global') - expect(t.dig('Job/test-migrations-1', 'spec', 'template', 'metadata', 'labels')).to include('global' => 'pod') - expect(t.dig('Job/test-migrations-1', 'spec', 'template', 'metadata', 'labels')).to include('pod' => 'true') - expect(t.dig('Job/test-migrations-1', 'spec', 'template', 'metadata', 'labels')).to include('global_pod' => 'true') + expect(migrations_job.dig('metadata', 'labels')).to include('foo' => 'global') + expect(migrations_job.dig('metadata', 'labels')).to include('global' => 'migrations') + expect(migrations_job.dig('metadata', 'labels')).not_to include('global' => 'global') + expect(migrations_job.dig('spec', 'template', 'metadata', 'labels')).to include('global' => 'pod') + expect(migrations_job.dig('spec', 'template', 'metadata', 'labels')).to include('pod' => 'true') + expect(migrations_job.dig('spec', 'template', 'metadata', 'labels')).to include('global_pod' => 'true') expect(t.dig('ServiceAccount/test-migrations', 'metadata', 'labels')).to include('global' => 'migrations') end end @@ -69,12 +70,13 @@ describe 'migrations configuration' do it 'Populates the additional annotations in the expected manner' do t = HelmTemplate.new(values) expect(t.exit_code).to eq(0), "Unexpected error code #{t.exit_code} -- #{t.stderr}" - expect(t.dig('Job/test-migrations-1', 'metadata', 'annotations')).to include('foo' => 'bar') - expect(t.dig('Job/test-migrations-1', 'metadata', 'annotations')).to include('bar' => 'foo') - expect(t.dig('Job/test-migrations-1', 'metadata', 'annotations')).not_to include('baz' => 'baz') - expect(t.dig('Job/test-migrations-1', 'spec', 'template', 'metadata', 'annotations')).to include('foo' => 'foo') - expect(t.dig('Job/test-migrations-1', 'spec', 'template', 'metadata', 'annotations')).to include('bar' => 'foo') - expect(t.dig('Job/test-migrations-1', 'spec', 'template', 'metadata', 'annotations')).to include('baz' => 'baz') + migrations_job = t.resources_by_kind_and_labels("Job", { "app" => "migrations" }).values.first + expect(migrations_job.dig('metadata', 'annotations')).to include('foo' => 'bar') + expect(migrations_job.dig('metadata', 'annotations')).to include('bar' => 'foo') + expect(migrations_job.dig('metadata', 'annotations')).not_to include('baz' => 'baz') + expect(migrations_job.dig('spec', 'template', 'metadata', 'annotations')).to include('foo' => 'foo') + expect(migrations_job.dig('spec', 'template', 'metadata', 'annotations')).to include('bar' => 'foo') + expect(migrations_job.dig('spec', 'template', 'metadata', 'annotations')).to include('baz' => 'baz') end end end diff --git a/spec/configuration/pages_spec.rb b/spec/configuration/pages_spec.rb index 7458a1bd2f..c75e0f4069 100644 --- a/spec/configuration/pages_spec.rb +++ b/spec/configuration/pages_spec.rb @@ -281,8 +281,9 @@ describe 'GitLab Pages' do describe 'access control' do it 'creates necessary secrets and configmaps and mounts them on migration job' do + migrations_job = pages_enabled_template.resources_by_kind_and_labels("Job", { "app" => "migrations" }).keys.first migrations_secret_mounts = pages_enabled_template.projected_volume_sources( - 'Job/test-migrations-1', + migrations_job, 'init-migrations-secrets' ) diff --git a/spec/configuration/securitycontext_spec.rb b/spec/configuration/securitycontext_spec.rb index 3a7cdd8454..67909c8b0b 100644 --- a/spec/configuration/securitycontext_spec.rb +++ b/spec/configuration/securitycontext_spec.rb @@ -99,7 +99,8 @@ describe 'security context' do end it 'applied fsGroupChangePolicy to the migrations job' do - policy = template.dig("Job/test-migrations-1", 'spec', 'template', 'spec', 'securityContext', 'fsGroupChangePolicy') + migrations_job = template.resources_by_kind_and_labels("Job", { "app" => "migrations" }).values.first + policy = migrations_job.dig('spec', 'template', 'spec', 'securityContext', 'fsGroupChangePolicy') expect(policy).to eq("OnRootMismatch"), "Unexpected fsGroupChangePolicy #{policy}" end diff --git a/spec/helm_template_helper.rb b/spec/helm_template_helper.rb index 893204095d..1baa8cc8f5 100644 --- a/spec/helm_template_helper.rb +++ b/spec/helm_template_helper.rb @@ -199,6 +199,13 @@ class HelmTemplate @mapped.select{ |_, hash| hash['kind'] == kind } end + def resources_by_kind_and_labels(kind, labels) + @mapped.select{ |_, hash| + hash['kind'] == kind && + labels.all? { |k,v| hash['metadata']['labels'][k] == v } + } + end + def exit_code() @exit_code.to_i end diff --git a/templates/_helpers.tpl b/templates/_helpers.tpl index cb211230c2..068c792c54 100644 --- a/templates/_helpers.tpl +++ b/templates/_helpers.tpl @@ -468,15 +468,13 @@ Return the name template for shared-secrets job. {{/* Create a default fully qualified job name for shared-secrets. -Due to the job only being allowed to run once, we add the chart revision so helm +Due to the job only being allowed to run once, we add a timestamp so helm upgrades don't cause errors trying to create the already ran job. -Due to the helm delete not cleaning up these jobs, we add a randome value to -reduce collision */}} {{- define "shared-secrets.jobname" -}} -{{- $name := include "shared-secrets.fullname" . | trunc 55 | trimSuffix "-" -}} -{{- $rand := randAlphaNum 3 | lower }} -{{- printf "%s-%d-%s" $name .Release.Revision $rand | trunc 63 | trimSuffix "-" -}} +{{- $name := include "shared-secrets.fullname" . | trunc 41 | trimSuffix "-" -}} +{{- $timestamp := include "gitlab.timestamp" . }} +{{- printf "%s-%s" $name $timestamp | trunc 63 | trimSuffix "-" -}} {{- end -}} {{/* @@ -548,3 +546,10 @@ securityContext: {{- end }} {{- end }} {{- end -}} + +{{/* +Return the current UTC time formatted as: yyyy-MM-dd-HH-mm-ss. +*/}} +{{- define "gitlab.timestamp" -}} +{{ dateInZone "2006-01-02-15-04-05" (now) "UTC" }} +{{- end -}} -- GitLab From e17834a03d7293fe06a80445ca6beb2b4f815a01 Mon Sep 17 00:00:00 2001 From: Clemens Beck Date: Wed, 13 Dec 2023 12:51:25 +0100 Subject: [PATCH 2/2] Add Helm revision label to jobs Add the "gitlab.com/helm-revision" label to all jobs and their pod templates to help with troubleshooting. --- .../templates/issuer-job.yaml | 2 ++ .../charts/migrations/templates/_jobspec.yaml | 2 ++ .../minio/templates/create-buckets-job.yaml | 6 +++-- spec/configuration/labels_spec.rb | 26 +++++++++++++++++++ templates/_application.tpl | 3 +++ templates/_helpers.tpl | 1 + templates/shared-secrets/job.yaml | 2 ++ 7 files changed, 40 insertions(+), 2 deletions(-) diff --git a/charts/certmanager-issuer/templates/issuer-job.yaml b/charts/certmanager-issuer/templates/issuer-job.yaml index 7878755db3..daca7719ff 100644 --- a/charts/certmanager-issuer/templates/issuer-job.yaml +++ b/charts/certmanager-issuer/templates/issuer-job.yaml @@ -8,6 +8,7 @@ metadata: labels: {{- include "gitlab.standardLabels" . | nindent 4 }} {{- include "gitlab.commonLabels" . | nindent 4 }} + {{- include "gitlab.revisionLabel" . | nindent 4 }} spec: activeDeadlineSeconds: 300 template: @@ -15,6 +16,7 @@ spec: labels: app: {{ template "name" . }} release: {{ .Release.Name }} + {{- include "gitlab.revisionLabel" . | nindent 8 }} spec: {{- include "gitlab.nodeSelector" . | nindent 6 }} securityContext: diff --git a/charts/gitlab/charts/migrations/templates/_jobspec.yaml b/charts/gitlab/charts/migrations/templates/_jobspec.yaml index 127ab296e2..7296646792 100644 --- a/charts/gitlab/charts/migrations/templates/_jobspec.yaml +++ b/charts/gitlab/charts/migrations/templates/_jobspec.yaml @@ -10,6 +10,7 @@ metadata: labels: {{- include "gitlab.standardLabels" . | nindent 4 }} {{- include "gitlab.commonLabels" . | nindent 4 }} + {{- include "gitlab.revisionLabel" . | nindent 4 }} {{- if .Values.annotations }} annotations: {{- range $key, $value := .Values.annotations }} @@ -31,6 +32,7 @@ spec: {{- include "gitlab.standardLabels" . | nindent 8 }} {{- include "gitlab.commonLabels" . | nindent 8 }} {{- include "gitlab.podLabels" . | nindent 8 }} + {{- include "gitlab.revisionLabel" . | nindent 8 }} spec: {{- if .Values.tolerations }} tolerations: diff --git a/charts/minio/templates/create-buckets-job.yaml b/charts/minio/templates/create-buckets-job.yaml index d960488526..f4f1c1591d 100755 --- a/charts/minio/templates/create-buckets-job.yaml +++ b/charts/minio/templates/create-buckets-job.yaml @@ -8,8 +8,9 @@ metadata: name: {{ template "minio.createBucketsJobName" . }} namespace: {{ $.Release.Namespace }} labels: -{{- include "gitlab.standardLabels" . | nindent 4 }} -{{- include "gitlab.commonLabels" . | nindent 4 }} + {{- include "gitlab.standardLabels" . | nindent 4 }} + {{- include "gitlab.commonLabels" . | nindent 4 }} + {{- include "gitlab.revisionLabel" . | nindent 4 }} spec: activeDeadlineSeconds: 600 template: @@ -19,6 +20,7 @@ spec: {{- include "gitlab.standardLabels" . | nindent 8 }} {{- include "gitlab.commonLabels" . | nindent 8 }} {{- include "gitlab.podLabels" . | nindent 8 }} + {{- include "gitlab.revisionLabel" . | nindent 8 }} {{- if .Values.jobAnnotations }} annotations: {{- range $key, $value := .Values.jobAnnotations }} diff --git a/spec/configuration/labels_spec.rb b/spec/configuration/labels_spec.rb index 1900337e85..ef4acaf5eb 100644 --- a/spec/configuration/labels_spec.rb +++ b/spec/configuration/labels_spec.rb @@ -100,4 +100,30 @@ describe 'Labels configuration' do expect(local_template.dig(target_chart, 'spec', 'template', 'metadata', 'labels')).to include(chart_values['gitlab']['webservice']['podLabels']) end end + + context "Job labels" do + let(:local_template) do + HelmTemplate.new(default_values.deep_merge(chart_values)) + end + + let(:revision_label) { "gitlab.com/helm-revision" } + + let(:ignored_jobs) do + [ + 'Job/test-certmanager-startupapicheck', + 'Job/test-gitlab-upgrade-check' + ] + end + + it "Renders the helm revision label" do + expect(local_template.exit_code).to eq(0), "Unexpected error code #{local_template.exit_code} -- #{local_template.stderr}" + + jobs = local_template.resources_by_kind("Job").reject { |job_key, _| ignored_jobs.include?(job_key) } + + jobs.each do |_, job| + expect(job.dig("metadata", "labels", revision_label)).to eql('1') + expect(job.dig("spec", "template", "metadata", "labels", revision_label)).to eql('1') + end + end + end end diff --git a/templates/_application.tpl b/templates/_application.tpl index 95639daf23..c85fe79260 100644 --- a/templates/_application.tpl +++ b/templates/_application.tpl @@ -43,6 +43,9 @@ heritage: {{ .Release.Service }} {{- end -}} {{- end -}} +{{- define "gitlab.revisionLabel" -}} +gitlab.com/helm-revision: {{ .Release.Revision | quote }} +{{- end -}} {{- define "gitlab.nodeSelector" -}} {{- $nodeSelector := default .Values.global.nodeSelector .Values.nodeSelector -}} diff --git a/templates/_helpers.tpl b/templates/_helpers.tpl index 068c792c54..2c3badff39 100644 --- a/templates/_helpers.tpl +++ b/templates/_helpers.tpl @@ -553,3 +553,4 @@ Return the current UTC time formatted as: yyyy-MM-dd-HH-mm-ss. {{- define "gitlab.timestamp" -}} {{ dateInZone "2006-01-02-15-04-05" (now) "UTC" }} {{- end -}} + diff --git a/templates/shared-secrets/job.yaml b/templates/shared-secrets/job.yaml index 4cf5d898ab..d0209c2f04 100644 --- a/templates/shared-secrets/job.yaml +++ b/templates/shared-secrets/job.yaml @@ -9,6 +9,7 @@ metadata: labels: {{- include "gitlab.standardLabels" . | nindent 4 }} {{- include "gitlab.commonLabels" . | nindent 4 }} + {{- include "gitlab.revisionLabel" . | nindent 4 }} annotations: "helm.sh/hook": pre-install,pre-upgrade "helm.sh/hook-delete-policy": hook-succeeded,before-hook-creation @@ -19,6 +20,7 @@ spec: {{- include "gitlab.standardLabels" . | nindent 8 }} {{- include "gitlab.commonLabels" . | nindent 8 }} {{- include "gitlab.podLabels" . | nindent 8 }} + {{- include "gitlab.revisionLabel" . | nindent 8 }} annotations: {{- range $key, $value := $sharedSecretValues.annotations }} {{ $key }}: {{ $value | quote }} -- GitLab