Merge branch 'fix/gb/exclude-persisted-variables-from-environment-name' into 'master'

Do not allow to use `CI_PIPELINE_ID` in environment name

Closes #46443

See merge request gitlab-org/gitlab-ce!19032

app/models/ci/build.rb

@@ -599,6 +599,7 @@ module Ci
         break variables unless persisted?
 
         variables
+          .concat(pipeline.persisted_variables)
           .append(key: 'CI_JOB_ID', value: id.to_s)
           .append(key: 'CI_JOB_TOKEN', value: token, public: false)
           .append(key: 'CI_BUILD_ID', value: id.to_s)

app/models/ci/pipeline.rb

@@ -523,9 +523,14 @@ module Ci
       strong_memoize(:legacy_trigger) { trigger_requests.first }
     end
 
+    def persisted_variables
+      Gitlab::Ci::Variables::Collection.new.tap do |variables|
+        variables.append(key: 'CI_PIPELINE_ID', value: id.to_s) if persisted?
+      end
+    end
+
     def predefined_variables
       Gitlab::Ci::Variables::Collection.new
-        .append(key: 'CI_PIPELINE_ID', value: id.to_s)
         .append(key: 'CI_CONFIG_PATH', value: ci_yaml_file_path)
         .append(key: 'CI_PIPELINE_SOURCE', value: source.to_s)
         .append(key: 'CI_COMMIT_MESSAGE', value: git_commit_message)

changelogs/unreleased/fix-gb-exclude-persisted-variables-from-environment-name.yml

+---
+title: Exclude CI_PIPELINE_ID from variables supported in dynamic environment name
+merge_request: 19032
+author:
+type: fixed

doc/ci/environments.md

@@ -252,6 +252,7 @@ including predefined, secure variables and `.gitlab-ci.yml`
 [`variables`](yaml/README.md#variables). You however cannot use variables
 defined under `script` or on the Runner's side. There are other variables that
 are unsupported in environment name context:
+- `CI_PIPELINE_ID`
 - `CI_JOB_ID`
 - `CI_JOB_TOKEN`
 - `CI_BUILD_ID`

doc/ci/variables/README.md

@@ -553,6 +553,7 @@ We do not support variables containing tokens because of security reasons.
 
 You can find a full list of unsupported variables below:
 
+- `CI_PIPELINE_ID`
 - `CI_JOB_ID`
 - `CI_JOB_TOKEN`
 - `CI_BUILD_ID`

spec/models/ci/build_spec.rb

@@ -629,6 +629,14 @@ describe Ci::Build do
 
         it { is_expected.to eq('review/host') }
       end
+
+      context 'when using persisted variables' do
+        let(:build) do
+          create(:ci_build, environment: 'review/x$CI_BUILD_ID')
+        end
+
+        it { is_expected.to eq('review/x') }
+      end
     end
 
     describe '#starts_environment?' do
@@ -1525,6 +1533,7 @@ describe Ci::Build do
     let(:container_registry_enabled) { false }
     let(:predefined_variables) do
       [
+        { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true },
         { key: 'CI_JOB_ID', value: build.id.to_s, public: true },
         { key: 'CI_JOB_TOKEN', value: build.token, public: false },
         { key: 'CI_BUILD_ID', value: build.id.to_s, public: true },
@@ -1556,7 +1565,6 @@ describe Ci::Build do
         { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true },
         { key: 'CI_PROJECT_URL', value: project.web_url, public: true },
         { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true },
-        { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true },
         { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true },
         { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true },
         { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true },

spec/models/ci/pipeline_spec.rb

@@ -167,13 +167,39 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '#persisted_variables' do
+    context 'when pipeline is not persisted yet' do
+      subject { build(:ci_pipeline).persisted_variables }
+
+      it 'does not contain some variables' do
+        keys = subject.map { |variable| variable[:key] }
+
+        expect(keys).not_to include 'CI_PIPELINE_ID'
+      end
+    end
+
+    context 'when pipeline is persisted' do
+      subject { build_stubbed(:ci_pipeline).persisted_variables }
+
+      it 'does contains persisted variables' do
+        keys = subject.map { |variable| variable[:key] }
+
+        expect(keys).to eq %w[CI_PIPELINE_ID]
+      end
+    end
+  end
+
   describe '#predefined_variables' do
     subject { pipeline.predefined_variables }
 
     it 'includes all predefined variables in a valid order' do
       keys = subject.map { |variable| variable[:key] }
 
-      expect(keys).to eq %w[CI_PIPELINE_ID CI_CONFIG_PATH CI_PIPELINE_SOURCE CI_COMMIT_MESSAGE CI_COMMIT_TITLE CI_COMMIT_DESCRIPTION]
+      expect(keys).to eq %w[CI_CONFIG_PATH
+                            CI_PIPELINE_SOURCE
+                            CI_COMMIT_MESSAGE
+                            CI_COMMIT_TITLE
+                            CI_COMMIT_DESCRIPTION]
     end
   end
 

spec/services/ci/create_pipeline_service_spec.rb

@@ -395,7 +395,27 @@ describe Ci::CreatePipelineService do
         result = execute_service
 
         expect(result).to be_persisted
-        expect(Environment.find_by(name: "review/master")).not_to be_nil
+        expect(Environment.find_by(name: "review/master")).to be_present
+      end
+    end
+
+    context 'with environment name including persisted variables' do
+      before do
+        config = YAML.dump(
+          deploy: {
+            environment: { name: "review/id1$CI_PIPELINE_ID/id2$CI_BUILD_ID" },
+            script: 'ls'
+          }
+        )
+
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      it 'skipps persisted variables in environment name' do
+        result = execute_service
+
+        expect(result).to be_persisted
+        expect(Environment.find_by(name: "review/id1/id2")).to be_present
       end
     end