Merge branch 'environment-status-cross-join-fix' into 'master'

Fix cross-DB query in EnvironmentStatus

See merge request gitlab-org/gitlab!71894

app/models/ci/pipeline.rb

@@ -925,9 +925,22 @@ module Ci
     end
 
     def environments_in_self_and_descendants
-      environment_ids = self_and_descendants.joins(:deployments).select(:'deployments.environment_id')
+      if ::Feature.enabled?(:avoid_cross_joins_environments_in_self_and_descendants, default_enabled: :yaml)
+        # We limit to 100 unique environments for application safety.
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/340781#note_699114700
+        expanded_environment_names =
+          builds_in_self_and_descendants.joins(:metadata)
+                                        .where.not('ci_builds_metadata.expanded_environment_name' => nil)
+                                        .distinct('ci_builds_metadata.expanded_environment_name')
+                                        .limit(100)
+                                        .pluck(:expanded_environment_name)
+
+        Environment.where(project: project, name: expanded_environment_names)
+      else
+        environment_ids = self_and_descendants.joins(:deployments).select(:'deployments.environment_id')
 
-      Environment.where(id: environment_ids)
+        Environment.where(id: environment_ids)
+      end
     end
 
     # With multi-project and parent-child pipelines

app/models/environment_status.rb

@@ -100,13 +100,11 @@ class EnvironmentStatus
   def self.build_environments_status(mr, user, pipeline)
     return [] unless pipeline
 
-    ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/340781') do
-      pipeline.environments_in_self_and_descendants.includes(:project).available.map do |environment|
-        next unless Ability.allowed?(user, :read_environment, environment)
+    pipeline.environments_in_self_and_descendants.includes(:project).available.map do |environment|
+      next unless Ability.allowed?(user, :read_environment, environment)
 
-        EnvironmentStatus.new(pipeline.project, environment, mr, pipeline.sha)
-      end.compact
-    end
+      EnvironmentStatus.new(pipeline.project, environment, mr, pipeline.sha)
+    end.compact
   end
   private_class_method :build_environments_status
 end

config/feature_flags/development/avoid_cross_joins_environments_in_self_and_descendants.yml

+---
+name: avoid_cross_joins_environments_in_self_and_descendants
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71894
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/342991
+milestone: '14.4'
+type: development
+group: group::release
+default_enabled: false

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -1876,8 +1876,7 @@ RSpec.describe Projects::MergeRequestsController do
       let(:sha)         { forked.commit.sha }
       let(:environment) { create(:environment, project: forked) }
       let(:pipeline)    { create(:ci_pipeline, sha: sha, project: forked) }
-      let(:build)       { create(:ci_build, pipeline: pipeline) }
-      let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: 'master', deployable: build) }
+      let!(:build) { create(:ci_build, :with_deployment, environment: environment.name, pipeline: pipeline) }
 
       let(:merge_request) do
         create(:merge_request, source_project: forked, target_project: project, target_branch: 'master', head_pipeline: pipeline)
@@ -1901,8 +1900,7 @@ RSpec.describe Projects::MergeRequestsController do
           let(:source_environment)  { create(:environment, project: project) }
           let(:merge_commit_sha)    { project.repository.merge(user, forked.commit.id, merge_request, "merged in test") }
           let(:post_merge_pipeline) { create(:ci_pipeline, sha: merge_commit_sha, project: project) }
-          let(:post_merge_build)    { create(:ci_build, pipeline: post_merge_pipeline) }
-          let!(:source_deployment)  { create(:deployment, :succeed, environment: source_environment, sha: merge_commit_sha, ref: 'master', deployable: post_merge_build) }
+          let!(:post_merge_build)   { create(:ci_build, :with_deployment, environment: source_environment.name, pipeline: post_merge_pipeline) }
 
           before do
             merge_request.update!(merge_commit_sha: merge_commit_sha)
@@ -1944,9 +1942,6 @@ RSpec.describe Projects::MergeRequestsController do
 
     context 'when a merge request has multiple environments with deployments' do
       let(:sha) { merge_request.diff_head_sha }
-      let(:ref) { merge_request.source_branch }
-
-      let!(:build) { create(:ci_build, pipeline: pipeline) }
       let!(:pipeline) { create(:ci_pipeline, sha: sha, project: project) }
       let!(:environment) { create(:environment, name: 'env_a', project: project) }
       let!(:another_environment) { create(:environment, name: 'env_b', project: project) }
@@ -1954,8 +1949,8 @@ RSpec.describe Projects::MergeRequestsController do
       before do
         merge_request.update_head_pipeline
 
-        create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build)
-        create(:deployment, :succeed, environment: another_environment, sha: sha, ref: ref, deployable: build)
+        create(:ci_build, :with_deployment, environment: environment.name, pipeline: pipeline)
+        create(:ci_build, :with_deployment, environment: another_environment.name, pipeline: pipeline)
       end
 
       it 'exposes multiple environment statuses' do

spec/features/merge_request/user_sees_deployment_widget_spec.rb

@@ -13,6 +13,8 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
     let(:sha) { project.commit(ref).id }
     let(:pipeline) { create(:ci_pipeline, sha: sha, project: project, ref: ref) }
     let!(:manual) { }
+    let(:build) { create(:ci_build, :with_deployment, environment: environment.name, pipeline: pipeline) }
+    let!(:deployment) { build.deployment }
 
     before do
       merge_request.update!(merge_commit_sha: sha)
@@ -21,8 +23,9 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
     end
 
     context 'when deployment succeeded' do
-      let(:build) { create(:ci_build, :success, pipeline: pipeline) }
-      let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) }
+      before do
+        build.success!
+      end
 
       it 'displays that the environment is deployed' do
         visit project_merge_request_path(project, merge_request)
@@ -34,9 +37,8 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
 
       context 'when a user created a new merge request with the same SHA' do
         let(:pipeline2) { create(:ci_pipeline, sha: sha, project: project, ref: 'video') }
-        let(:build2) { create(:ci_build, :success, pipeline: pipeline2) }
         let(:environment2) { create(:environment, project: project) }
-        let!(:deployment2) { create(:deployment, environment: environment2, sha: sha, ref: 'video', deployable: build2) }
+        let!(:build2) { create(:ci_build, :with_deployment, :success, environment: environment2.name, pipeline: pipeline2) }
 
         it 'displays one environment which is related to the pipeline' do
           visit project_merge_request_path(project, merge_request)
@@ -50,8 +52,9 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
     end
 
     context 'when deployment failed' do
-      let(:build) { create(:ci_build, :failed, pipeline: pipeline) }
-      let!(:deployment) { create(:deployment, :failed, environment: environment, sha: sha, ref: ref, deployable: build) }
+      before do
+        build.drop!
+      end
 
       it 'displays that the deployment failed' do
         visit project_merge_request_path(project, merge_request)
@@ -63,8 +66,9 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
     end
 
     context 'when deployment running' do
-      let(:build) { create(:ci_build, :running, pipeline: pipeline) }
-      let!(:deployment) { create(:deployment, :running, environment: environment, sha: sha, ref: ref, deployable: build) }
+      before do
+        build.run!
+      end
 
       it 'displays that the running deployment' do
         visit project_merge_request_path(project, merge_request)
@@ -76,8 +80,8 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
     end
 
     context 'when deployment will happen' do
-      let(:build) { create(:ci_build, :created, pipeline: pipeline) }
-      let!(:deployment) { create(:deployment, environment: environment, sha: sha, ref: ref, deployable: build) }
+      let(:build) { create(:ci_build, :with_deployment, environment: environment.name, pipeline: pipeline) }
+      let!(:deployment) { build.deployment }
 
       it 'displays that the environment name' do
         visit project_merge_request_path(project, merge_request)
@@ -89,8 +93,9 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
     end
 
     context 'when deployment was cancelled' do
-      let(:build) { create(:ci_build, :canceled, pipeline: pipeline) }
-      let!(:deployment) { create(:deployment, :canceled, environment: environment, sha: sha, ref: ref, deployable: build) }
+      before do
+        build.cancel!
+      end
 
       it 'displays that the environment name' do
         visit project_merge_request_path(project, merge_request)
@@ -102,11 +107,10 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do
     end
 
     context 'with stop action' do
-      let(:build) { create(:ci_build, :success, pipeline: pipeline) }
-      let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) }
       let(:manual) { create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') }
 
       before do
+        build.success!
         deployment.update!(on_stop: manual.name)
         visit project_merge_request_path(project, merge_request)
         wait_for_requests

spec/features/merge_request/user_sees_merge_widget_spec.rb

@@ -45,18 +45,12 @@ RSpec.describe 'Merge request > User sees merge widget', :js do
     let!(:environment) { create(:environment, project: project) }
     let(:sha)          { project.commit(merge_request.source_branch).sha }
     let(:pipeline)     { create(:ci_pipeline, status: 'success', sha: sha, project: project, ref: merge_request.source_branch) }
-    let(:build)        { create(:ci_build, :success, pipeline: pipeline) }
-
-    let!(:deployment) do
-      create(:deployment, :succeed,
-                          environment: environment,
-                          ref: merge_request.source_branch,
-                          deployable: build,
-                          sha: sha)
-    end
+    let!(:build)       { create(:ci_build, :with_deployment, :success, environment: environment.name, pipeline: pipeline) }
+    let!(:deployment)  { build.deployment }
 
     before do
       merge_request.update!(head_pipeline: pipeline)
+      deployment.update!(status: :success)
       visit project_merge_request_path(project, merge_request)
     end