diff --git a/.rubocop_todo/rspec/file_path.yml b/.rubocop_todo/rspec/file_path.yml index 3cf0e969c345e316397c1f813ed2329ca54b0898..ebeb967841cd591a1e2eb359ac457d2e10775560 100644 --- a/.rubocop_todo/rspec/file_path.yml +++ b/.rubocop_todo/rspec/file_path.yml @@ -65,3 +65,4 @@ RSpec/FilePath: - 'spec/services/ci/create_pipeline_service/rate_limit_spec.rb' - 'spec/services/ci/create_pipeline_service/rules_spec.rb' - 'spec/services/ci/create_pipeline_service/tags_spec.rb' + - 'spec/services/ci/create_pipeline_service/variables_spec.rb' diff --git a/app/presenters/ci/build_runner_presenter.rb b/app/presenters/ci/build_runner_presenter.rb index e3b992ff49ed27ea93e3e331be1c1c5702bbd772..7242a80b924a0229676f5f68c266a4a5fb7e0715 100644 --- a/app/presenters/ci/build_runner_presenter.rb +++ b/app/presenters/ci/build_runner_presenter.rb @@ -33,8 +33,13 @@ def git_depth end def runner_variables + stop_expanding_raw_refs = ::Feature.enabled?(:ci_raw_variables_in_yaml_config, project) + variables - .sort_and_expand_all(keep_undefined: true, expand_file_vars: false, project: project) + .sort_and_expand_all(keep_undefined: true, + expand_file_refs: false, + expand_raw_refs: !stop_expanding_raw_refs, + project: project) .to_runner_variables end diff --git a/lib/gitlab/ci/variables/collection.rb b/lib/gitlab/ci/variables/collection.rb index 1dbe5481807d97bbf549ea538c06d8dd60bba55d..e976606107242a8d8cf038eb605f4eee811c66f3 100644 --- a/lib/gitlab/ci/variables/collection.rb +++ b/lib/gitlab/ci/variables/collection.rb @@ -72,7 +72,8 @@ def reject(&block) Collection.new(@variables.reject(&block)) end - def expand_value(value, keep_undefined: false, expand_file_vars: true, project: nil) + # `expand_raw_refs` will be deleted with the FF `ci_raw_variables_in_yaml_config`. + def expand_value(value, keep_undefined: false, expand_file_refs: true, expand_raw_refs: true, project: nil) value.gsub(Item::VARIABLES_REGEXP) do match = Regexp.last_match # it is either a valid variable definition or a ($$ / %%) full_match = match[0] @@ -86,20 +87,26 @@ def expand_value(value, keep_undefined: false, expand_file_vars: true, project: variable = self[variable_name] if variable # VARIABLE_NAME is an existing variable - next variable.value unless variable.file? - - # Will be cleaned up with https://gitlab.com/gitlab-org/gitlab/-/issues/378266 - if project - # We only log if `project` exists to make sure it is called from `Ci::BuildRunnerPresenter` - # when the variables are sent to Runner. - Gitlab::AppJsonLogger.info( - event: 'file_variable_is_referenced_in_another_variable', - project_id: project.id, - variable: variable_name - ) + if variable.file? + # Will be cleaned up with https://gitlab.com/gitlab-org/gitlab/-/issues/378266 + if project + # We only log if `project` exists to make sure it is called from `Ci::BuildRunnerPresenter` + # when the variables are sent to Runner. + Gitlab::AppJsonLogger.info(event: 'file_variable_is_referenced_in_another_variable', + project_id: project.id, + variable: variable_name) + end + + expand_file_refs ? variable.value : full_match + elsif variable.raw? + # With `full_match`, we defer the expansion of raw variables to the runner. If we expand them here, + # the runner will not know the expanded value is a raw variable and it tries to expand it again. + # Discussion: https://gitlab.com/gitlab-org/gitlab/-/issues/353991#note_1103274951 + expand_raw_refs ? variable.value : full_match + else + variable.value end - expand_file_vars ? variable.value : full_match elsif keep_undefined full_match # we do not touch the variable definition else @@ -108,7 +115,8 @@ def expand_value(value, keep_undefined: false, expand_file_vars: true, project: end end - def sort_and_expand_all(keep_undefined: false, expand_file_vars: true, project: nil) + # `expand_raw_refs` will be deleted with the FF `ci_raw_variables_in_yaml_config`. + def sort_and_expand_all(keep_undefined: false, expand_file_refs: true, expand_raw_refs: true, project: nil) sorted = Sort.new(self) return self.class.new(self, sorted.errors) unless sorted.valid? @@ -123,7 +131,8 @@ def sort_and_expand_all(keep_undefined: false, expand_file_vars: true, project: # expand variables as they are added variable = item.to_runner_variable variable[:value] = new_collection.expand_value(variable[:value], keep_undefined: keep_undefined, - expand_file_vars: expand_file_vars, + expand_file_refs: expand_file_refs, + expand_raw_refs: expand_raw_refs, project: project) new_collection.append(variable) end diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb index 45faf4f63ebb667537f6b0abc43ff63e330052f7..0fcf11121fa87382072367f1d3573a82ea67181a 100644 --- a/lib/gitlab/ci/variables/collection/item.rb +++ b/lib/gitlab/ci/variables/collection/item.rb @@ -21,9 +21,10 @@ def value @variable.fetch(:value) end - def raw + def raw? @variable.fetch(:raw) end + alias_method :raw, :raw? def file? @variable.fetch(:file) @@ -39,7 +40,7 @@ def ==(other) def depends_on strong_memoize(:depends_on) do - next if raw + next if raw? next unless self.class.possible_var_reference?(value) diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb index 9443bf6d6d57657050f7991cf4b65fc05aa40cc8..f7c6f7f51df4ede8c8d171a4dcd75ab622aab7fc 100644 --- a/spec/lib/gitlab/ci/variables/collection/item_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb @@ -197,11 +197,11 @@ end end - describe '#raw' do + describe '#raw?' do it 'returns false when :raw is not specified' do item = described_class.new(**variable) - expect(item.raw).to eq false + expect(item.raw?).to eq false end context 'when :raw is specified as true' do @@ -212,7 +212,7 @@ it 'returns true' do item = described_class.new(**variable) - expect(item.raw).to eq true + expect(item.raw?).to eq true end end end diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb index 4710698fd956afa9240ac403aa01b24c3d710beb..10b8f0065d99878cd0d1c7e20984554e05b4fdc4 100644 --- a/spec/lib/gitlab/ci/variables/collection_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection_spec.rb @@ -300,7 +300,6 @@ Gitlab::Ci::Variables::Collection.new .append(key: 'CI_JOB_NAME', value: 'test-1') .append(key: 'CI_BUILD_ID', value: '1') - .append(key: 'RAW_VAR', value: '$TEST1', raw: true) .append(key: 'TEST1', value: 'test-3') .append(key: 'FILEVAR1', value: 'file value 1', file: true) end @@ -322,10 +321,6 @@ value: 'key${TEST1}-${CI_JOB_NAME}', result: 'keytest-3-test-1' }, - "complex expansions with raw variable": { - value: 'key${RAW_VAR}-${CI_JOB_NAME}', - result: 'key$TEST1-test-1' - }, "missing variable not keeping original": { value: 'key${MISSING_VAR}-${CI_JOB_NAME}', result: 'key-test-1' @@ -339,22 +334,22 @@ value: 'key-$TEST1-%%HOME%%-$${HOME}', result: 'key-test-3-%%HOME%%-$${HOME}' }, - "file variable with expand_file_vars: true": { + "file variable with expand_file_refs: true": { value: 'key-$FILEVAR1-$TEST1', result: 'key-file value 1-test-3' }, - "file variable with expand_file_vars: false": { + "file variable with expand_file_refs: false": { value: 'key-$FILEVAR1-$TEST1', result: 'key-$FILEVAR1-test-3', - expand_file_vars: false + expand_file_refs: false } } end with_them do - let(:options) { { keep_undefined: keep_undefined, expand_file_vars: expand_file_vars }.compact } + let(:options) { { keep_undefined: keep_undefined, expand_file_refs: expand_file_refs }.compact } - subject(:result) { collection.expand_value(value, **options) } + subject(:expanded_result) { collection.expand_value(value, **options) } it 'matches expected expansion' do is_expected.to eq(result) @@ -509,17 +504,35 @@ { key: 'variable4', value: 'keyvalue${variable2}value3' } ] }, - "complex expansions with raw variable": { + "complex expansions with raw variable with expand_raw_refs: true (default)": { + variables: [ + { key: 'variable1', value: 'value1' }, + { key: 'raw_var', value: 'raw-$variable1', raw: true }, + { key: 'nonraw_var', value: 'nonraw-$variable1' }, + { key: 'variable2', value: '$raw_var and $nonraw_var' } + ], + keep_undefined: false, + result: [ + { key: 'variable1', value: 'value1' }, + { key: 'raw_var', value: 'raw-$variable1', raw: true }, + { key: 'nonraw_var', value: 'nonraw-value1' }, + { key: 'variable2', value: 'raw-$variable1 and nonraw-value1' } + ] + }, + "complex expansions with raw variable with expand_raw_refs: false": { variables: [ - { key: 'variable3', value: 'key_${variable}_${variable2}' }, - { key: 'variable', value: '$variable2', raw: true }, - { key: 'variable2', value: 'value2' } + { key: 'variable1', value: 'value1' }, + { key: 'raw_var', value: 'raw-$variable1', raw: true }, + { key: 'nonraw_var', value: 'nonraw-$variable1' }, + { key: 'variable2', value: '$raw_var and $nonraw_var' } ], keep_undefined: false, + expand_raw_refs: false, result: [ - { key: 'variable', value: '$variable2', raw: true }, - { key: 'variable2', value: 'value2' }, - { key: 'variable3', value: 'key_$variable2_value2' } + { key: 'variable1', value: 'value1' }, + { key: 'raw_var', value: 'raw-$variable1', raw: true }, + { key: 'nonraw_var', value: 'nonraw-value1' }, + { key: 'variable2', value: '$raw_var and nonraw-value1' } ] }, "variable value referencing password with special characters": { @@ -553,8 +566,9 @@ with_them do let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) } + let(:options) { { keep_undefined: keep_undefined, expand_raw_refs: expand_raw_refs }.compact } - subject { collection.sort_and_expand_all(keep_undefined: keep_undefined) } + subject(:expanded_result) { collection.sort_and_expand_all(**options) } it 'returns Collection' do is_expected.to be_an_instance_of(Gitlab::Ci::Variables::Collection) diff --git a/spec/presenters/ci/build_runner_presenter_spec.rb b/spec/presenters/ci/build_runner_presenter_spec.rb index 845ae79c4978f60ddb03f2fe8f9c812f453cfa5f..952de121cc4600d2bacb73e3b5317958fee835d3 100644 --- a/spec/presenters/ci/build_runner_presenter_spec.rb +++ b/spec/presenters/ci/build_runner_presenter_spec.rb @@ -325,7 +325,7 @@ is_expected.to eq(presenter.variables.to_runner_variables) end - context 'when there are variables to expand' do + context 'when there is a file variable to expand' do before_all do create(:ci_variable, project: project, key: 'regular_var', @@ -360,6 +360,49 @@ runner_variables end end + + context 'when there is a raw variable to expand' do + before_all do + create(:ci_variable, project: project, + key: 'regular_var', + value: 'value 1') + create(:ci_variable, project: project, + key: 'raw_var', + value: 'value 2', + raw: true) + create(:ci_variable, project: project, + key: 'var_with_variables', + value: 'value 3 and $regular_var and $raw_var and $undefined_var') + end + + it 'returns expanded variables without expanding raws' do + expect(runner_variables).to include( + { key: 'regular_var', value: 'value 1', + public: false, masked: false }, + { key: 'raw_var', value: 'value 2', + public: false, masked: false, raw: true }, + { key: 'var_with_variables', value: 'value 3 and value 1 and $raw_var and $undefined_var', + public: false, masked: false } + ) + end + + context 'when the FF ci_raw_variables_in_yaml_config is disabled' do + before do + stub_feature_flags(ci_raw_variables_in_yaml_config: false) + end + + it 'returns expanded variables' do + expect(runner_variables).to include( + { key: 'regular_var', value: 'value 1', + public: false, masked: false }, + { key: 'raw_var', value: 'value 2', + public: false, masked: false, raw: true }, + { key: 'var_with_variables', value: 'value 3 and value 1 and value 2 and $undefined_var', + public: false, masked: false } + ) + end + end + end end describe '#runner_variables subset' do diff --git a/spec/services/ci/create_pipeline_service/variables_spec.rb b/spec/services/ci/create_pipeline_service/variables_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c0ed8ad2a9e77ae179b066637b8dfa73f5788405 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/variables_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectness do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.first_owner } + + let(:service) { described_class.new(project, user, { ref: 'master' }) } + let(:pipeline) { service.execute(:push).payload } + + before do + stub_ci_pipeline_yaml_file(config) + end + + context 'when using variables' do + context 'when variables have expand: true/false' do + let(:config) do + <<-YAML + variables: + VAR7: + value: "value 7 $CI_PIPELINE_ID" + expand: false + VAR8: + value: "value 8 $CI_PIPELINE_ID" + expand: false + + rspec: + script: rspec + variables: + VAR1: "JOBID-$CI_JOB_ID" + VAR2: "PIPELINEID-$CI_PIPELINE_ID and $VAR1" + VAR3: + value: "PIPELINEID-$CI_PIPELINE_ID and $VAR1" + expand: false + VAR4: + value: "JOBID-$CI_JOB_ID" + expand: false + VAR5: "PIPELINEID-$CI_PIPELINE_ID and $VAR4" + VAR6: + value: "PIPELINEID-$CI_PIPELINE_ID and $VAR4" + expand: false + VAR7: "overridden value 7 $CI_PIPELINE_ID" + YAML + end + + let(:rspec) { find_job('rspec') } + + it 'creates the pipeline with a job that has variable expanded according to "expand"' do + expect(pipeline).to be_created_successfully + + expect(Ci::BuildRunnerPresenter.new(rspec).runner_variables).to include( + { key: 'VAR1', value: "JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR2', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR3', value: "PIPELINEID-$CI_PIPELINE_ID and $VAR1", public: true, masked: false, raw: true }, + { key: 'VAR4', value: "JOBID-$CI_JOB_ID", public: true, masked: false, raw: true }, + { key: 'VAR5', value: "PIPELINEID-#{pipeline.id} and $VAR4", public: true, masked: false }, + { key: 'VAR6', value: "PIPELINEID-$CI_PIPELINE_ID and $VAR4", public: true, masked: false, raw: true }, + { key: 'VAR7', value: "overridden value 7 #{pipeline.id}", public: true, masked: false }, + { key: 'VAR8', value: "value 8 $CI_PIPELINE_ID", public: true, masked: false, raw: true } + ) + end + + context 'when the FF ci_raw_variables_in_yaml_config is disabled' do + before do + stub_feature_flags(ci_raw_variables_in_yaml_config: false) + end + + it 'creates the pipeline with a job that has all variables expanded' do + expect(pipeline).to be_created_successfully + + expect(Ci::BuildRunnerPresenter.new(rspec).runner_variables).to include( + { key: 'VAR1', value: "JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR2', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR3', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR4', value: "JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR5', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR6', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR7', value: "overridden value 7 #{pipeline.id}", public: true, masked: false }, + { key: 'VAR8', value: "value 8 #{pipeline.id}", public: true, masked: false } + ) + end + end + end + end + + private + + def find_job(name) + pipeline.processables.find { |job| job.name == name } + end +end