Reuse prometheus chart values when editing alerts

Alerts for the GitLab-managed app Prometheus are updated via helm.
Prior to this change, when alerts were updated, all helm values would
receive the current contents of `values.yaml`, with the `alerts` section
overridden, or falling back to the default chart values.

This meant that any time we made changes to `values.yaml` affecting
component versions (e.g. the versions of prometheus or alertmanager),
those would trigger a deployment of those component in the cluster,
potentially breaking the prometheus installation.

This was a very real problem, since we currently have hardcoded versions
of both prometheus server and alertmanager in `values.yaml`, and would
like to update the chart and remove the hardcoded versions in a single
change.

With this change, alert updates no longer take any values from
`values.yaml`; instead they only update alerts. We avoid reverting to
default chart values by using helm's `--reuse-values` flag instead of
the `--reset-values` flag.

See https://gitlab.com/gitlab-org/gitlab/issues/38294

app/models/clusters/applications/prometheus.rb

@@ -55,10 +55,10 @@ module Clusters
         )
       end
 
-      def upgrade_command(values)
-        ::Gitlab::Kubernetes::Helm::InstallCommand.new(
+      def patch_command(values)
+        ::Gitlab::Kubernetes::Helm::PatchCommand.new(
           name: name,
-          version: VERSION,
+          version: version,
           rbac: cluster.platform_kubernetes_rbac?,
           chart: chart,
           files: files_with_replaced_values(values)

app/services/clusters/applications/base_helm_service.rb

@@ -61,8 +61,8 @@ module Clusters
         @update_command ||= app.update_command
       end
 
-      def upgrade_command(new_values = "")
-        app.upgrade_command(new_values)
+      def patch_command(new_values = "")
+        app.patch_command(new_values)
       end
     end
   end

ee/app/services/clusters/applications/check_upgrade_progress_service.rb

@@ -50,17 +50,21 @@ module Clusters
       end
 
       def remove_pod
-        helm_api.delete_pod!(upgrade_command.pod_name)
+        helm_api.delete_pod!(pod_name)
       rescue
         # no-op
       end
 
       def phase
-        helm_api.status(upgrade_command.pod_name)
+        helm_api.status(pod_name)
       end
 
       def errors
-        helm_api.log(upgrade_command.pod_name)
+        helm_api.log(pod_name)
+      end
+
+      def pod_name
+        @pod_name ||= patch_command.pod_name
       end
     end
   end

ee/app/services/clusters/applications/prometheus_config_service.rb

@@ -9,7 +9,7 @@ module Clusters
         @app = app
       end
 
-      def execute(config)
+      def execute(config = {})
         if has_alerts?
           generate_alert_manager(config)
         else
@@ -24,6 +24,7 @@ module Clusters
       def reset_alert_manager(config)
         config = set_alert_manager_enabled(config, false)
         config.delete('alertmanagerFiles')
+        config['serverFiles'] ||= {}
         config['serverFiles']['alerts'] = {}
 
         config
@@ -37,6 +38,7 @@ module Clusters
       end
 
       def set_alert_manager_enabled(config, enabled)
+        config['alertmanager'] ||= {}
         config['alertmanager']['enabled'] = enabled
 
         config
@@ -54,6 +56,8 @@ module Clusters
       end
 
       def set_alert_manager_groups(config)
+        config['serverFiles'] ||= {}
+        config['serverFiles']['alerts'] ||= {}
         config['serverFiles']['alerts']['groups'] ||= []
 
         environments_with_alerts.each do |env_name, alerts|

ee/app/services/clusters/applications/prometheus_update_service.rb

@@ -13,11 +13,7 @@ module Clusters
       def execute
         app.make_updating!
 
-        values = load_config(app)
-          .yield_self { |config| update_config(config) }
-          .yield_self { |config| config.to_yaml }
-
-        helm_api.update(upgrade_command(values))
+        helm_api.update(patch_command(values))
 
         ::ClusterWaitForAppUpdateWorker.perform_in(::ClusterWaitForAppUpdateWorker::INTERVAL, app.name, app.id)
       rescue ::Kubeclient::HttpError => ke
@@ -28,14 +24,11 @@ module Clusters
 
       private
 
-      def load_config(app)
-        YAML.safe_load(app.values)
-      end
-
-      def update_config(config)
+      def values
         PrometheusConfigService
           .new(project, cluster, app)
-          .execute(config)
+          .execute
+          .to_yaml
       end
     end
   end

ee/spec/services/clusters/applications/prometheus_update_service_spec.rb

@@ -8,20 +8,20 @@ describe Clusters::Applications::PrometheusUpdateService do
     let(:environment) { create(:environment, project: project) }
     let(:cluster) { create(:cluster, :provided_by_user, :with_installed_helm, projects: [project]) }
     let(:application) { create(:clusters_applications_prometheus, :installed, cluster: cluster) }
-    let(:values_yaml) { application.values }
-    let!(:upgrade_command) { application.upgrade_command('') }
+    let(:empty_alerts_values_update_yaml) { "---\nalertmanager:\n  enabled: false\nserverFiles:\n  alerts: {}\n" }
+    let!(:patch_command) { application.patch_command(empty_alerts_values_update_yaml) }
     let(:helm_client) { instance_double(::Gitlab::Kubernetes::Helm::Api) }
 
     subject(:service) { described_class.new(application, project) }
 
     before do
-      allow(service).to receive(:upgrade_command).and_return(upgrade_command)
+      allow(service).to receive(:patch_command).with(empty_alerts_values_update_yaml).and_return(patch_command)
       allow(service).to receive(:helm_api).and_return(helm_client)
     end
 
     context 'when there are no errors' do
       before do
-        expect(helm_client).to receive(:update).with(upgrade_command)
+        expect(helm_client).to receive(:update).with(patch_command)
 
         allow(::ClusterWaitForAppUpdateWorker)
           .to receive(:perform_in)
@@ -38,7 +38,6 @@ describe Clusters::Applications::PrometheusUpdateService do
 
       it 'updates current config' do
         prometheus_config_service = spy(:prometheus_config_service)
-        values = YAML.safe_load(values_yaml)
 
         expect(Clusters::Applications::PrometheusConfigService)
           .to receive(:new)
@@ -47,7 +46,7 @@ describe Clusters::Applications::PrometheusUpdateService do
 
         expect(prometheus_config_service)
           .to receive(:execute)
-          .with(values)
+          .and_return(YAML.safe_load(empty_alerts_values_update_yaml))
 
         service.execute
       end

lib/gitlab/kubernetes/helm/client_command.rb

@@ -43,6 +43,10 @@ module Gitlab
           optional_tls_flags
         end
 
+        def repository_update_command
+          'helm repo update'
+        end
+
         def optional_tls_flags
           return [] unless files.key?(:'ca.pem')
 

lib/gitlab/kubernetes/helm/install_command.rb

@@ -39,10 +39,6 @@ module Gitlab
 
         private
 
-        def repository_update_command
-          'helm repo update'
-        end
-
         # Uses `helm upgrade --install` which means we can use this for both
         # installation and uprade of applications
         def install_command

lib/gitlab/kubernetes/helm/patch_command.rb

+# frozen_string_literal: true
+
+# PatchCommand is for updating values in installed charts without overwriting
+# existing values.
+module Gitlab
+  module Kubernetes
+    module Helm
+      class PatchCommand
+        include BaseCommand
+        include ClientCommand
+
+        attr_reader :name, :files, :chart, :repository
+        attr_accessor :version
+
+        def initialize(name:, chart:, files:, rbac:, version:, repository: nil)
+          # version is mandatory to prevent chart mismatches
+          # we do not want our values interpreted in the context of the wrong version
+          raise ArgumentError, 'version is required' if version.blank?
+
+          @name = name
+          @chart = chart
+          @version = version
+          @rbac = rbac
+          @files = files
+          @repository = repository
+        end
+
+        def generate_script
+          super + [
+            init_command,
+            wait_for_tiller_command,
+            repository_command,
+            repository_update_command,
+            upgrade_command
+          ].compact.join("\n")
+        end
+
+        def rbac?
+          @rbac
+        end
+
+        private
+
+        def upgrade_command
+          command = ['helm', 'upgrade', name, chart] +
+            reuse_values_flag +
+            tls_flags_if_remote_tiller +
+            version_flag +
+            namespace_flag +
+            value_flag
+
+          command.shelljoin
+        end
+
+        def reuse_values_flag
+          ['--reuse-values']
+        end
+
+        def value_flag
+          ['-f', "/data/helm/#{name}/config/values.yaml"]
+        end
+
+        def namespace_flag
+          ['--namespace', Gitlab::Kubernetes::Helm::NAMESPACE]
+        end
+
+        def version_flag
+          ['--version', version]
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/kubernetes/helm/patch_command_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Kubernetes::Helm::PatchCommand do
+  let(:files) { { 'ca.pem': 'some file content' } }
+  let(:repository) { 'https://repository.example.com' }
+  let(:rbac) { false }
+  let(:version) { '1.2.3' }
+
+  subject(:patch_command) do
+    described_class.new(
+      name: 'app-name',
+      chart: 'chart-name',
+      rbac: rbac,
+      files: files,
+      version: version,
+      repository: repository
+    )
+  end
+
+  context 'when local tiller feature is disabled' do
+    before do
+      stub_feature_flags(managed_apps_local_tiller: false)
+    end
+
+    let(:tls_flags) do
+      <<~EOS.squish
+      --tls
+      --tls-ca-cert /data/helm/app-name/config/ca.pem
+      --tls-cert /data/helm/app-name/config/cert.pem
+      --tls-key /data/helm/app-name/config/key.pem
+      EOS
+    end
+
+    it_behaves_like 'helm commands' do
+      let(:commands) do
+        <<~EOS
+        helm init --upgrade
+        for i in $(seq 1 30); do helm version #{tls_flags} && s=0 && break || s=$?; sleep 1s; echo \"Retrying ($i)...\"; done; (exit $s)
+        helm repo add app-name https://repository.example.com
+        helm repo update
+        #{helm_upgrade_comand}
+        EOS
+      end
+
+      let(:helm_upgrade_comand) do
+        <<~EOS.squish
+        helm upgrade app-name chart-name
+          --reuse-values
+          #{tls_flags}
+          --version 1.2.3
+          --namespace gitlab-managed-apps
+          -f /data/helm/app-name/config/values.yaml
+        EOS
+      end
+    end
+  end
+
+  it_behaves_like 'helm commands' do
+    let(:commands) do
+      <<~EOS
+      export HELM_HOST="localhost:44134"
+      tiller -listen ${HELM_HOST} -alsologtostderr &
+      helm init --client-only
+      helm repo add app-name https://repository.example.com
+      helm repo update
+      #{helm_upgrade_comand}
+      EOS
+    end
+
+    let(:helm_upgrade_comand) do
+      <<~EOS.squish
+      helm upgrade app-name chart-name
+        --reuse-values
+        --version 1.2.3
+        --namespace gitlab-managed-apps
+        -f /data/helm/app-name/config/values.yaml
+      EOS
+    end
+  end
+
+  context 'when rbac is true' do
+    let(:rbac) { true }
+
+    it_behaves_like 'helm commands' do
+      let(:commands) do
+        <<~EOS
+        export HELM_HOST="localhost:44134"
+        tiller -listen ${HELM_HOST} -alsologtostderr &
+        helm init --client-only
+        helm repo add app-name https://repository.example.com
+        helm repo update
+        #{helm_upgrade_command}
+        EOS
+      end
+
+      let(:helm_upgrade_command) do
+        <<~EOS.squish
+        helm upgrade app-name chart-name
+          --reuse-values
+          --version 1.2.3
+          --namespace gitlab-managed-apps
+          -f /data/helm/app-name/config/values.yaml
+        EOS
+      end
+    end
+  end
+
+  context 'when there is no ca.pem file' do
+    let(:files) { { 'file.txt': 'some content' } }
+
+    it_behaves_like 'helm commands' do
+      let(:commands) do
+        <<~EOS
+        export HELM_HOST="localhost:44134"
+        tiller -listen ${HELM_HOST} -alsologtostderr &
+        helm init --client-only
+        helm repo add app-name https://repository.example.com
+        helm repo update
+        #{helm_upgrade_command}
+        EOS
+      end
+
+      let(:helm_upgrade_command) do
+        <<~EOS.squish
+        helm upgrade app-name chart-name
+           --reuse-values
+           --version 1.2.3
+           --namespace gitlab-managed-apps
+           -f /data/helm/app-name/config/values.yaml
+        EOS
+      end
+    end
+  end
+
+  describe '#pod_name' do
+    subject { patch_command.pod_name }
+
+    it { is_expected.to eq 'install-app-name' }
+  end
+
+  context 'when there is no version' do
+    let(:version) { nil }
+
+    it { expect { patch_command }.to raise_error(ArgumentError, 'version is required') }
+  end
+
+  describe '#rbac?' do
+    subject { patch_command.rbac? }
+
+    context 'rbac is enabled' do
+      let(:rbac) { true }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'rbac is not enabled' do
+      let(:rbac) { false }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#pod_resource' do
+    subject { patch_command.pod_resource }
+
+    context 'rbac is enabled' do
+      let(:rbac) { true }
+
+      it 'generates a pod that uses the tiller serviceAccountName' do
+        expect(subject.spec.serviceAccountName).to eq('tiller')
+      end
+    end
+
+    context 'rbac is not enabled' do
+      let(:rbac) { false }
+
+      it 'generates a pod that uses the default serviceAccountName' do
+        expect(subject.spec.serviceAcccountName).to be_nil
+      end
+    end
+  end
+
+  describe '#config_map_resource' do
+    let(:metadata) do
+      {
+        name: "values-content-configuration-app-name",
+        namespace: 'gitlab-managed-apps',
+        labels: { name: "values-content-configuration-app-name" }
+      }
+    end
+
+    let(:resource) { ::Kubeclient::Resource.new(metadata: metadata, data: files) }
+
+    subject { patch_command.config_map_resource }
+
+    it 'returns a KubeClient resource with config map content for the application' do
+      is_expected.to eq(resource)
+    end
+  end
+
+  describe '#service_account_resource' do
+    subject { patch_command.service_account_resource }
+
+    it 'returns nothing' do
+      is_expected.to be_nil
+    end
+  end
+
+  describe '#cluster_role_binding_resource' do
+    subject { patch_command.cluster_role_binding_resource }
+
+    it 'returns nothing' do
+      is_expected.to be_nil
+    end
+  end
+end

spec/models/clusters/applications/prometheus_spec.rb

@@ -206,21 +206,19 @@ describe Clusters::Applications::Prometheus do
     end
   end
 
-  describe '#upgrade_command' do
+  describe '#patch_command' do
+    subject(:patch_command) { prometheus.patch_command(values) }
+
     let(:prometheus) { build(:clusters_applications_prometheus) }
     let(:values) { prometheus.values }
 
-    it 'returns an instance of Gitlab::Kubernetes::Helm::InstallCommand' do
-      expect(prometheus.upgrade_command(values)).to be_an_instance_of(::Gitlab::Kubernetes::Helm::InstallCommand)
-    end
+    it { is_expected.to be_an_instance_of(::Gitlab::Kubernetes::Helm::PatchCommand) }
 
     it 'is initialized with 3 arguments' do
-      command = prometheus.upgrade_command(values)
-
-      expect(command.name).to eq('prometheus')
-      expect(command.chart).to eq('stable/prometheus')
-      expect(command.version).to eq('6.7.3')
-      expect(command.files).to eq(prometheus.files)
+      expect(patch_command.name).to eq('prometheus')
+      expect(patch_command.chart).to eq('stable/prometheus')
+      expect(patch_command.version).to eq('6.7.3')
+      expect(patch_command.files).to eq(prometheus.files)
     end
   end