Commit d0ff854f authored by Stan Hu's avatar Stan Hu

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
parent 1d2df5a4
...@@ -31,7 +31,26 @@ module Ci ...@@ -31,7 +31,26 @@ module Ci
end end
def to_runner_variable 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? } { key: key, value: value, public: false, file: file? }
end 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
end end
...@@ -63,6 +63,8 @@ module Ci ...@@ -63,6 +63,8 @@ module Ci
@logger = build_logger @logger = build_logger
@pipeline = Ci::Pipeline.new @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( command = Gitlab::Ci::Pipeline::Chain::Command.new(
source: source, source: source,
origin_ref: params[:ref], origin_ref: params[:ref],
......
---
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
...@@ -68,9 +68,68 @@ RSpec.describe Ci::HasVariable do ...@@ -68,9 +68,68 @@ RSpec.describe Ci::HasVariable do
end end
describe '#to_runner_variable' do describe '#to_runner_variable' do
let_it_be(:ci_variable) { create(:ci_variable) }
subject { ci_variable }
it 'returns a hash for the runner' do it 'returns a hash for the runner' do
expect(subject.to_runner_variable) expect(subject.to_runner_variable)
.to include(key: subject.key, value: subject.value, public: false) .to include(key: subject.key, value: subject.value, public: false)
end 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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment