From d0ff854f0068aa3cc793ac583016764165aa7e07 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Mon, 17 Jan 2022 23:50:42 -0800
Subject: [PATCH] Optimize decryption of CI variables

In pipelines that have many environments and many CI variables, the
decryption in `Ci::HasVariable#to_runner_variable` is a
significant bottleneck due to repeated decryption via
`OpenSSL::PKCS5.pbkdf2_hmac`. We can optimize this by caching the
value in the request. In one customer's pipeline, this cuts down
`Ci::CreatePipelineService` from 80+ seconds to 30 seconds.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/326271

Changelog: performance
---
 app/models/concerns/ci/has_variable.rb        | 19 ++++++
 app/services/ci/create_pipeline_service.rb    |  2 +
 .../enable_ci_variable_caching.yml            |  8 +++
 spec/models/concerns/ci/has_variable_spec.rb  | 59 +++++++++++++++++++
 4 files changed, 88 insertions(+)
 create mode 100644 config/feature_flags/development/enable_ci_variable_caching.yml

diff --git a/app/models/concerns/ci/has_variable.rb b/app/models/concerns/ci/has_variable.rb
index 7309469c77ee3e..db0a8e64cad8d5 100644
--- a/app/models/concerns/ci/has_variable.rb
+++ b/app/models/concerns/ci/has_variable.rb
@@ -31,7 +31,26 @@ def key=(new_key)
     end
 
     def to_runner_variable
+      return uncached_runner_variable unless ::Gitlab::SafeRequestStore.read(:enable_ci_variable_caching)
+
+      var_cache_key = to_runner_variable_cache_key
+
+      return uncached_runner_variable unless var_cache_key
+
+      ::Gitlab::SafeRequestStore.fetch(var_cache_key) { uncached_runner_variable }
+    end
+
+    private
+
+    def uncached_runner_variable
       { key: key, value: value, public: false, file: file? }
     end
+
+    def to_runner_variable_cache_key
+      return unless persisted?
+
+      variable_id = read_attribute(self.class.primary_key)
+      "#{self.class}#to_runner_variable:#{variable_id}:#{key}"
+    end
   end
 end
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index d53e136effb2df..4bc1d28cfbeefd 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -63,6 +63,8 @@ def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request
       @logger = build_logger
       @pipeline = Ci::Pipeline.new
 
+      ::Gitlab::SafeRequestStore.write(:enable_ci_variable_caching, ::Feature.enabled?(:enable_ci_variable_caching, project, default_enabled: :yaml))
+
       command = Gitlab::Ci::Pipeline::Chain::Command.new(
         source: source,
         origin_ref: params[:ref],
diff --git a/config/feature_flags/development/enable_ci_variable_caching.yml b/config/feature_flags/development/enable_ci_variable_caching.yml
new file mode 100644
index 00000000000000..aefb1af32fc0b1
--- /dev/null
+++ b/config/feature_flags/development/enable_ci_variable_caching.yml
@@ -0,0 +1,8 @@
+---
+name: enable_ci_variable_caching
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78440
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350529
+milestone: '14.8'
+type: development
+group: group::pipeline execution
+default_enabled: false
diff --git a/spec/models/concerns/ci/has_variable_spec.rb b/spec/models/concerns/ci/has_variable_spec.rb
index e917ec6b802ec4..7f395a737fab6a 100644
--- a/spec/models/concerns/ci/has_variable_spec.rb
+++ b/spec/models/concerns/ci/has_variable_spec.rb
@@ -68,9 +68,68 @@
   end
 
   describe '#to_runner_variable' do
+    let_it_be(:ci_variable) { create(:ci_variable) }
+
+    subject { ci_variable }
+
     it 'returns a hash for the runner' do
       expect(subject.to_runner_variable)
         .to include(key: subject.key, value: subject.value, public: false)
     end
+
+    context 'with RequestStore enabled', :request_store do
+      let(:expected) do
+        {
+          file: false,
+          key: subject.key,
+          value: subject.value,
+          public: false,
+          masked: false
+        }
+      end
+
+      before do
+        # CreatePipelineService normally writes this because this feature flag
+        # cannot be checked in a tight loop
+        ::Gitlab::SafeRequestStore[:enable_ci_variable_caching] = true
+      end
+
+      it 'decrypts once' do
+        expect(OpenSSL::PKCS5).to receive(:pbkdf2_hmac).once.and_call_original
+
+        2.times { expect(subject.reload.to_runner_variable).to eq(expected) }
+      end
+
+      it 'does not cache similar keys', :aggregate_failures do
+        group_var = create(:ci_group_variable, key: subject.key, value: 'group')
+        project_var = create(:ci_variable, key: subject.key, value: 'project')
+
+        expect(subject.to_runner_variable).to include(key: subject.key, value: subject.value)
+        expect(group_var.to_runner_variable).to include(key: subject.key, value: 'group')
+        expect(project_var.to_runner_variable).to include(key: subject.key, value: 'project')
+      end
+
+      it 'does not cache unpersisted values' do
+        new_variable = Ci::Variable.new(key: SecureRandom.hex, value: "12345")
+        old_value = new_variable.to_runner_variable
+        new_variable.value = '98765'
+
+        expect(new_variable.to_runner_variable).not_to eq(old_value)
+      end
+
+      context 'with enable_ci_variable_caching feature flag disabled' do
+        before do
+          # CreatePipelineService normally writes this because this feature flag
+          # cannot be checked in a tight loop
+          ::Gitlab::SafeRequestStore[:enable_ci_variable_caching] = false
+        end
+
+        it 'decrypts twice' do
+          expect(OpenSSL::PKCS5).to receive(:pbkdf2_hmac).twice.and_call_original
+
+          2.times { expect(subject.reload.to_runner_variable).to eq(expected) }
+        end
+      end
+    end
   end
 end
-- 
GitLab