From 5a690c9a3666b91c763edab53e71dd6f1ec59223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= Date: Tue, 15 Apr 2025 16:45:20 +0200 Subject: [PATCH] Add experiment to allow `.pre` to be referenced by policy custom stages The only way to support injection of custom policy stages at the beginning of the pipeline is to use `.pre` as a reference. However, `.pre` stage is automatically moved to the beginning, making it impossible to inject custom stages at the beginning of the pipeline. This change adds an experiment flag in the policy to support this behavior. Changelog: added EE: true --- ee/lib/ee/gitlab/ci/config/stages.rb | 37 +++++--- .../pipeline_context.rb | 7 +- .../lib/ee/gitlab/ci/config/stages_spec.rb | 95 +++++++++++++++---- .../pipeline_context_spec.rb | 26 +++++ .../pipeline_execution_policy_spec.rb | 53 ++++++++++- lib/gitlab/ci/config.rb | 5 +- lib/gitlab/ci/config/stages.rb | 5 +- spec/lib/gitlab/ci/config/stages_spec.rb | 2 +- 8 files changed, 194 insertions(+), 36 deletions(-) diff --git a/ee/lib/ee/gitlab/ci/config/stages.rb b/ee/lib/ee/gitlab/ci/config/stages.rb index 38e6d0fcd712b9..6547c12de28de9 100644 --- a/ee/lib/ee/gitlab/ci/config/stages.rb +++ b/ee/lib/ee/gitlab/ci/config/stages.rb @@ -6,22 +6,13 @@ module Ci module Config module Stages extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override include ::Gitlab::Utils::StrongMemoize RESERVED_POLICY_PRE = '.pipeline-policy-pre' RESERVED_POLICY_POST = '.pipeline-policy-post' RESERVED = [RESERVED_POLICY_PRE, RESERVED_POLICY_POST].freeze - class_methods do - def wrap_with_reserved_stages(stages) - stages = stages.to_a - RESERVED - stages.unshift RESERVED_POLICY_PRE - stages.push RESERVED_POLICY_POST - - stages - end - end - def inject_reserved_stages! # If stages are not declared in config, we use the default stages to inject the reserved stages into. stages_to_wrap = stages.presence || ::Gitlab::Ci::Config::Entry::Stages.default @@ -30,7 +21,31 @@ def inject_reserved_stages! config end - delegate :wrap_with_reserved_stages, to: :class + override :inject_edge_stages! + def inject_edge_stages! + return super unless stages.present? && pipeline_policy_context&.inject_stages_before_pre? + + config[:stages] = wrap_with_edge_stages_preserving_pre(stages) + config + end + + private + + def wrap_with_reserved_stages(stages) + stages = stages.to_a - RESERVED + stages.unshift RESERVED_POLICY_PRE + stages.push RESERVED_POLICY_POST + + stages + end + + def wrap_with_edge_stages_preserving_pre(stages) + stages = stages.to_a - [self.class::EDGE_POST] + stages.unshift self.class::EDGE_PRE unless stages.include?(self.class::EDGE_PRE) + stages.push self.class::EDGE_POST + + stages + end end end end diff --git a/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb b/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb index 17cce3ce5a3215..b8fd44738fd20d 100644 --- a/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb +++ b/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb @@ -69,6 +69,11 @@ def scheduled_execution_policy_pipeline? command&.source == ::Security::PipelineExecutionPolicies::RunScheduleWorker::PIPELINE_SOURCE end + def inject_stages_before_pre? + creating_policy_pipeline? && + current_policy.experiment_enabled?(:pipeline_execution_policy_stages_before_pre) + end + def skip_ci_allowed? return true unless has_execution_policy_pipelines? @@ -123,7 +128,7 @@ def valid_stage?(stage) def enforce_stages!(config:) return config unless inject_policy_stages? - config = ::Gitlab::Ci::Config::Stages.new(config).inject_reserved_stages! + config = ::Gitlab::Ci::Config::Stages.new(config, pipeline_policy_context: self).inject_reserved_stages! return config if creating_policy_pipeline? config[:stages] = override_policy_stages if has_override_stages? diff --git a/ee/spec/lib/ee/gitlab/ci/config/stages_spec.rb b/ee/spec/lib/ee/gitlab/ci/config/stages_spec.rb index d25e7f8f89860c..260785c0e68ffb 100644 --- a/ee/spec/lib/ee/gitlab/ci/config/stages_spec.rb +++ b/ee/spec/lib/ee/gitlab/ci/config/stages_spec.rb @@ -1,30 +1,22 @@ # frozen_string_literal: true -require 'fast_spec_helper' +require 'spec_helper' RSpec.describe Gitlab::Ci::Config::Stages, feature_category: :pipeline_composition do - describe '.wrap_with_reserved_stages' do - subject { described_class.wrap_with_reserved_stages(stages) } - - context 'with nil value' do - let(:stages) { nil } - - it { is_expected.to eq %w[.pipeline-policy-pre .pipeline-policy-post] } - end - - context 'with values' do - let(:stages) { %w[s1 .pipeline-policy-pre] } - - it { is_expected.to eq %w[.pipeline-policy-pre s1 .pipeline-policy-post] } - end - end + include_context 'with pipeline policy context' describe '#inject_reserved_stages!' do - subject(:injected_config) { described_class.new(config).inject_reserved_stages! } + subject(:injected_config) do + described_class.new( + config, pipeline_policy_context: pipeline_policy_context.pipeline_execution_context + ).inject_reserved_stages! + end it 'does not mutate the original config' do config = { stages: ['test'] } - new_config = described_class.new(config).inject_reserved_stages! + new_config = described_class.new( + config, pipeline_policy_context: pipeline_policy_context.pipeline_execution_context + ).inject_reserved_stages! expect(new_config).not_to eq(config) end @@ -71,4 +63,71 @@ end end end + + describe '#inject_edge_stages!' do + subject(:injected_config) do + described_class.new( + config, + pipeline_policy_context: pipeline_policy_context.pipeline_execution_context + ).inject_edge_stages! + end + + let(:inject_before_pre) { true } + + before do + allow(pipeline_policy_context.pipeline_execution_context).to receive(:inject_stages_before_pre?) + .and_return(inject_before_pre) + end + + context 'without stages' do + let(:config) { { job: { script: 'test' } } } + + it { is_expected.to eq(config) } + end + + context 'without stages injected before .pre' do + let(:config) { { stages: %w[test .pre] } } + let(:inject_before_pre) { false } + + it 'wraps with edge stages' do + expect(injected_config[:stages]).to eq(%w[.pre test .post]) + end + end + + context 'without pipeline_policy_context' do + subject(:injected_config) { described_class.new(config, pipeline_policy_context: nil).inject_edge_stages! } + + let(:config) { { stages: %w[test .pre] } } + + it 'wraps with edge stages' do + expect(injected_config[:stages]).to eq(%w[.pre test .post]) + end + end + + context 'with stages injected before .pre' do + context 'when there are no edge stages' do + let(:config) { { stages: %w[test] } } + + it 'wraps with edge stages' do + expect(injected_config[:stages]).to eq(%w[.pre test .post]) + end + end + + context 'when there is .pre stage already' do + let(:config) { { stages: %w[test .pre] } } + + it 'wraps with .post and preserves .pre' do + expect(injected_config[:stages]).to eq(%w[test .pre .post]) + end + end + + context 'when there is a .post stage' do + let(:config) { { stages: %w[.post test] } } + + it 'ignores the declared .post stage' do + expect(injected_config[:stages]).to eq(%w[.pre test .post]) + end + end + end + end end diff --git a/ee/spec/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context_spec.rb b/ee/spec/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context_spec.rb index 48c2bd2c0db2dc..fbbe47c619941c 100644 --- a/ee/spec/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context_spec.rb +++ b/ee/spec/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context_spec.rb @@ -754,4 +754,30 @@ end end end + + describe '#inject_stages_before_pre?' do + subject { context.inject_stages_before_pre? } + + context 'with current_policy' do + include_context 'with mocked current_policy' + + let(:current_policy) { build(:pipeline_execution_policy_config, policy_config: policy_configuration) } + let(:policy_configuration) { build_stubbed(:security_orchestration_policy_configuration) } + + it { is_expected.to eq(false) } + + context 'when experiment is enabled' do + let(:policy_configuration) do + build_stubbed(:security_orchestration_policy_configuration, + experiments: { pipeline_execution_policy_stages_before_pre: { enabled: true } }) + end + + it { is_expected.to eq(true) } + end + end + + context 'without current_policy' do + it { is_expected.to eq(false) } + end + end end diff --git a/ee/spec/services/ci/create_pipeline_service/pipeline_execution_policy_spec.rb b/ee/spec/services/ci/create_pipeline_service/pipeline_execution_policy_spec.rb index c660af9321b130..9f66bd1646c5e2 100644 --- a/ee/spec/services/ci/create_pipeline_service/pipeline_execution_policy_spec.rb +++ b/ee/spec/services/ci/create_pipeline_service/pipeline_execution_policy_spec.rb @@ -31,7 +31,7 @@ let_it_be_with_reload(:namespace_policies_project) { create(:project, :empty_repo, group: group) } - let_it_be(:namespace_configuration) do + let_it_be_with_reload(:namespace_configuration) do create(:security_orchestration_policy_configuration, project: nil, namespace: group, security_policy_management_project: namespace_policies_project) end @@ -53,7 +53,7 @@ let_it_be_with_reload(:project_policies_project) { create(:project, :empty_repo, group: group) } - let_it_be(:project_configuration) do + let_it_be_with_reload(:project_configuration) do create(:security_orchestration_policy_configuration, project: project, security_policy_management_project: project_policies_project) end @@ -700,6 +700,55 @@ end end + context 'when policy stages are injected at the beginning of the pipeline using .pre' do + before do + config_params = { pipeline_execution_policy_stages_before_pre: { enabled: experiment_enabled } } + [namespace_configuration, project_configuration].each do |configuration| + configuration.update!(experiments: config_params) + end + end + + let(:experiment_enabled) { true } + let(:namespace_policy_content) do + { stages: %w[policy-build .pre], + namespace_build_job: { stage: 'policy-build', script: 'policy build script' } } + end + + let(:project_policy_content) do + { stages: %w[policy-build policy-test .pre], + project_test_job: { stage: 'policy-test', script: 'policy test script' } } + end + + it 'responds with success and injects the stages before .pre as reference', :aggregate_failures do + expect { execute }.to change { Ci::Build.count }.from(0).to(4) + + expect(execute).to be_success + expect(execute.payload).to be_persisted + + stages = execute.payload.stages + expect(stages.sort_by(&:position).map(&:name)).to eq(%w[policy-build policy-test build test]) + + expect(stages.find_by(name: 'policy-build').builds.map(&:name)).to contain_exactly('namespace_build_job') + expect(stages.find_by(name: 'policy-test').builds.map(&:name)).to contain_exactly('project_test_job') + expect(stages.find_by(name: 'build').builds.map(&:name)).to contain_exactly('build') + expect(stages.find_by(name: 'test').builds.map(&:name)).to contain_exactly('rspec') + end + + context 'when the experiment is disabled' do + let(:experiment_enabled) { false } + + it 'injects policy stages after project stages', :aggregate_failures do + expect { execute }.to change { Ci::Build.count }.from(0).to(4) + + expect(execute).to be_success + expect(execute.payload).to be_persisted + + stages = execute.payload.stages + expect(stages.sort_by(&:position).map(&:name)).to eq(%w[build test policy-build policy-test]) + end + end + end + context 'when there are cyclic dependencies' do let(:project_ci_yaml) do <<~YAML diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 56cfaf7c241585..43c2c8ee7aeb37 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -188,7 +188,10 @@ def build_config(config, inputs) return initial_config unless inject_edge_stages logger.instrument(:config_stages_inject, once: true) do - Config::Stages.new(initial_config).inject_edge_stages! + Config::Stages.new( + initial_config, + pipeline_policy_context: pipeline_policy_context&.pipeline_execution_context + ).inject_edge_stages! end end diff --git a/lib/gitlab/ci/config/stages.rb b/lib/gitlab/ci/config/stages.rb index b15b8c020ceae8..d6f9a8155643c4 100644 --- a/lib/gitlab/ci/config/stages.rb +++ b/lib/gitlab/ci/config/stages.rb @@ -16,9 +16,10 @@ def self.wrap_with_edge_stages(stages) stages end - def initialize(config) + def initialize(config, pipeline_policy_context:) @config = config.to_h.deep_dup @stages = extract_stages + @pipeline_policy_context = pipeline_policy_context end def inject_edge_stages! @@ -30,7 +31,7 @@ def inject_edge_stages! private - attr_reader :config, :stages + attr_reader :config, :stages, :pipeline_policy_context delegate :wrap_with_edge_stages, to: :class diff --git a/spec/lib/gitlab/ci/config/stages_spec.rb b/spec/lib/gitlab/ci/config/stages_spec.rb index 4d276b9c253919..73c0667157c851 100644 --- a/spec/lib/gitlab/ci/config/stages_spec.rb +++ b/spec/lib/gitlab/ci/config/stages_spec.rb @@ -20,7 +20,7 @@ end describe '#inject_edge_stages!' do - subject { described_class.new(config).inject_edge_stages! } + subject { described_class.new(config, pipeline_policy_context: nil).inject_edge_stages! } context 'without stages' do let(:config) do -- GitLab