Surface invalid envirionment name errorsurface-invalid-envirionment-name-error

4 files changed, 106 insertions, 11 deletions

diff --git a/app/models/concerns/deployable.rb b/app/models/concerns/deployable.rb
index 85db01af18d..6e79e050294 100644
--- a/app/models/concerns/deployable.rb
+++ b/app/models/concerns/deployable.rb
@@ -6,21 +6,47 @@ module Deployable
   included do
     after_create :create_deployment
 
+    FailedToCreateEnvironmentError = Class.new(StandardError)
+
     def create_deployment
       return unless starts_environment? && !has_deployment?
 
-      environment = project.environments.find_or_create_by(
-        name: expanded_environment_name
-      )
-
       create_deployment!(
-        project_id: environment.project_id,
-        environment: environment,
+        project_id: project.id,
+        environment: ensure_environment,
         ref: ref,
         tag: tag,
         sha: sha,
         user: user,
         on_stop: on_stop)
     end
+
+    private
+
+    def ensure_environment
+      strong_memoize(:persisted_environment) do
+        project.environments.find_by_name(expanded_environment_name) ||
+          create_environment!
+      end
+    end
+
+    # Environment name could have invalid character, such as `:` (colon).
+    # If it's the case, we have to rollback parent's transaction, which is `pipeline.save!` or `build.save!`
+    def create_environment!
+      error_message = nil
+
+      Environment.transaction(requires_new: true) do
+        begin
+          project.environments.create!(
+            name: expanded_environment_name
+          )
+        rescue ActiveRecord::RecordInvalid => e
+          error_message = e.message
+          raise ActiveRecord::Rollback
+        end
+      end.tap do |ret|
+        raise FailedToCreateEnvironmentError, error_message unless ret
+      end
+    end
   end
 end
diff --git a/lib/gitlab/ci/pipeline/chain/create.rb b/lib/gitlab/ci/pipeline/chain/create.rb
index aa627bdb009..d17f54d8a43 100644
--- a/lib/gitlab/ci/pipeline/chain/create.rb
+++ b/lib/gitlab/ci/pipeline/chain/create.rb
@@ -9,7 +9,7 @@ module Gitlab
 
           def perform!
             pipeline.save!
-          rescue ActiveRecord::RecordInvalid => e
+          rescue ActiveRecord::RecordInvalid, Deployable::FailedToCreateEnvironmentError => e
             error("Failed to persist the pipeline: #{e}")
           end
 
diff --git a/spec/models/concerns/deployable_spec.rb b/spec/models/concerns/deployable_spec.rb
index ac79c75a55e..cf41ff16517 100644
--- a/spec/models/concerns/deployable_spec.rb
+++ b/spec/models/concerns/deployable_spec.rb
@@ -5,10 +5,6 @@ describe Deployable do
     let(:deployment) { job.deployment }
     let(:environment) { deployment&.environment }
 
-    before do
-      job.reload
-    end
-
     context 'when the deployable object will deploy to production' do
       let!(:job) { create(:ci_build, :start_review_app) }
 
@@ -49,5 +45,28 @@ describe Deployable do
         expect(environment).to be_nil
       end
     end
+
+    context 'when environment scope contains invalid character' do
+      let(:job) do
+        create(
+          :ci_build,
+          name: 'job:deploy-to-test-site',
+          environment: '$CI_JOB_NAME',
+          options: {
+            environment: {
+              name: '$CI_JOB_NAME',
+              url: 'http://staging.example.com/$CI_JOB_NAME',
+              on_stop: 'stop_review_app'
+            }
+          })
+      end
+
+      it 'fails to create build' do
+        expect { job }.to raise_error(
+          Deployable::FailedToCreateEnvironmentError,
+          "Validation failed: Name can contain only letters, digits, '-', '_', '/', '$', '{', '}', '.', and spaces, but it cannot start or end with '/'"
+        )
+      end
+    end
   end
 end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 193148d403a..f3e3b0fc0ff 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -608,5 +608,55 @@ describe Ci::CreatePipelineService do
           .to eq variables_attributes.map(&:with_indifferent_access)
       end
     end
+
+    context 'when pipeline has a job with environment' do
+      let(:pipeline) { execute_service }
+
+      before do
+        stub_ci_pipeline_yaml_file(YAML.dump(config))
+      end
+
+      context 'when environment name is valid' do
+        let(:config) do
+          {
+            review_app: {
+              script: 'deploy',
+              environment: {
+                name: 'review/${CI_COMMIT_REF_NAME}',
+                url: 'http://${CI_COMMIT_REF_SLUG}-staging.example.com'
+              }
+            }
+          }
+        end
+
+        it 'has a job with environment' do
+          expect(pipeline.builds.count).to eq(1)
+          expect(pipeline.builds.first.persisted_environment.name).to eq('review/master')
+          expect(pipeline.builds.first.deployment).to be_created
+        end
+      end
+
+      context 'when environment name is invalid' do
+        let(:config) do
+          {
+            'job:deploy-to-test-site': {
+              script: 'deploy',
+              environment: {
+                name: '${CI_JOB_NAME}',
+                url: 'https://$APP_URL'
+              }
+            }
+          }
+        end
+
+        it 'fails to create a pipeline' do
+          expect(pipeline).not_to be_persisted
+          expect(pipeline.errors[:base])
+            .to eq(["Failed to persist the pipeline: Validation failed: Name can" \
+                    " contain only letters, digits, '-', '_', '/', '$', '{', '}'," \
+                    " '.', and spaces, but it cannot start or end with '/'"])
+        end
+      end
+    end
   end
 end