Merge branch '254222-use-pages_deployments-in-the-zip-serving-instead-of-artifacts' into 'master'

Use pages_deployments in the zip serving instead of artifacts

See merge request gitlab-org/gitlab!46320

app/models/namespace.rb

@@ -362,7 +362,7 @@ class Namespace < ApplicationRecord
 
   def pages_virtual_domain
     Pages::VirtualDomain.new(
-      all_projects_with_pages.includes(:route, :project_feature),
+      all_projects_with_pages.includes(:route, :project_feature, pages_metadatum: :pages_deployment),
       trim_prefix: full_path
     )
   end

app/models/pages/lookup_path.rb

@@ -22,11 +22,7 @@ module Pages
     end
 
     def source
-      if artifacts_archive && !artifacts_archive.file_storage?
-        zip_source
-      else
-        file_source
-      end
+      zip_source || file_source
     end
 
     def prefix
@@ -42,19 +38,39 @@ module Pages
     attr_reader :project, :trim_prefix, :domain
 
     def artifacts_archive
-      return unless Feature.enabled?(:pages_artifacts_archive, project)
+      return unless Feature.enabled?(:pages_serve_from_artifacts_archive, project)
+
+      archive = project.pages_metadatum.artifacts_archive
+
+      archive&.file
+    end
+
+    def deployment
+      return unless Feature.enabled?(:pages_serve_from_deployments, project)
+
+      deployment = project.pages_metadatum.pages_deployment
 
-      # Using build artifacts is temporary solution for quick test
-      # in production environment, we'll replace this with proper
-      # `pages_deployments` later
-      project.pages_metadatum.artifacts_archive&.file
+      deployment&.file
     end
 
     def zip_source
-      {
-        type: 'zip',
-        path: artifacts_archive.url(expire_at: 1.day.from_now)
-      }
+      source = deployment || artifacts_archive
+
+      return unless source
+
+      if source.file_storage?
+        return unless Feature.enabled?(:pages_serve_with_zip_file_protocol, project)
+
+        {
+          type: 'zip',
+          path: 'file://' + source.path
+        }
+      else
+        {
+          type: 'zip',
+          path: source.url(expire_at: 1.day.from_now)
+        }
+      end
     end
 
     def file_source

app/models/pages_deployment.rb

@@ -21,6 +21,10 @@ class PagesDeployment < ApplicationRecord
 
   mount_file_store_uploader ::Pages::DeploymentUploader
 
+  def log_geo_deleted_event
+    # this is to be adressed in https://gitlab.com/groups/gitlab-org/-/epics/589
+  end
+
   private
 
   def set_size

config/feature_flags/development/pages_artifacts_archive.yml → config/feature_flags/development/pages_serve_from_artifacts_archive.yml

 ---
-name: pages_artifacts_archive
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40361
-rollout_issue_url: 
+name: pages_serve_from_artifacts_archive
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46320
+rollout_issue_url:
+group: group::release management
 milestone: '13.4'
 type: development
-group: group::release management
 default_enabled: false

config/feature_flags/development/pages_serve_from_deployments.yml

+---
+name: pages_serve_from_deployments
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46320
+rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/2932
+milestone: '13.6'
+type: development
+group: group::Release Management
+default_enabled: false

config/feature_flags/development/pages_serve_with_zip_file_protocol.yml

+---
+name: pages_serve_with_zip_file_protocol
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46320
+rollout_issue_url:
+milestone: '13.6'
+type: development
+group: group::Release Management
+default_enabled: false

spec/fixtures/api/schemas/internal/pages/lookup_path.json

@@ -14,7 +14,7 @@
     "source": { "type": "object",
       "required": ["type", "path"],
       "properties" : {
-        "type": { "type": "string", "enum": ["file"] },
+        "type": { "type": "string", "enum": ["file", "zip", "zip_local"] },
         "path": { "type": "string" }
       },
       "additionalProperties": false

spec/models/namespace_spec.rb

@@ -1271,24 +1271,6 @@ RSpec.describe Namespace do
           expect(virtual_domain.lookup_paths).not_to be_empty
         end
       end
-
-      it 'preloads project_feature and route' do
-        project2 = create(:project, namespace: namespace)
-        project3 = create(:project, namespace: namespace)
-
-        project.mark_pages_as_deployed
-        project2.mark_pages_as_deployed
-        project3.mark_pages_as_deployed
-
-        virtual_domain = namespace.pages_virtual_domain
-
-        queries = ActiveRecord::QueryRecorder.new { virtual_domain.lookup_paths }
-
-        # 1 to load projects
-        # 1 to preload project features
-        # 1 to load routes
-        expect(queries.count).to eq(3)
-      end
     end
   end
 

spec/models/pages/lookup_path_spec.rb

@@ -3,15 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe Pages::LookupPath do
-  let_it_be(:project) do
-    create(:project, :pages_private, pages_https_only: true)
-  end
+  let(:project) { create(:project, :pages_private, pages_https_only: true) }
 
   subject(:lookup_path) { described_class.new(project) }
 
   before do
     stub_pages_setting(access_control: true, external_https: ["1.1.1.1:443"])
     stub_artifacts_object_storage
+    stub_pages_object_storage(::Pages::DeploymentUploader)
   end
 
   describe '#project_id' do
@@ -47,18 +46,63 @@ RSpec.describe Pages::LookupPath do
   end
 
   describe '#source' do
-    shared_examples 'uses disk storage' do
-      it 'sets the source type to "file"' do
-        expect(lookup_path.source[:type]).to eq('file')
-      end
+    let(:source) { lookup_path.source }
 
-      it 'sets the source path to the project full path suffixed with "public/' do
-        expect(lookup_path.source[:path]).to eq(project.full_path + "/public/")
+    shared_examples 'uses disk storage' do
+      it 'uses disk storage', :aggregate_failures do
+        expect(source[:type]).to eq('file')
+        expect(source[:path]).to eq(project.full_path + "/public/")
       end
     end
 
     include_examples 'uses disk storage'
 
+    context 'when there is pages deployment' do
+      let(:deployment) { create(:pages_deployment, project: project) }
+
+      before do
+        project.mark_pages_as_deployed
+        project.pages_metadatum.update!(pages_deployment: deployment)
+      end
+
+      it 'uses deployment from object storage', :aggregate_failures do
+        Timecop.freeze do
+          expect(source[:type]).to eq('zip')
+          expect(source[:path]).to eq(deployment.file.url(expire_at: 1.day.from_now))
+          expect(source[:path]).to include("Expires=86400")
+        end
+      end
+
+      context 'when deployment is in the local storage' do
+        before do
+          deployment.file.migrate!(::ObjectStorage::Store::LOCAL)
+        end
+
+        it 'uses file protocol', :aggregate_failures do
+          Timecop.freeze do
+            expect(source[:type]).to eq('zip')
+            expect(source[:path]).to eq('file://' + deployment.file.path)
+          end
+        end
+
+        context 'when pages_serve_with_zip_file_protocol feature flag is disabled' do
+          before do
+            stub_feature_flags(pages_serve_with_zip_file_protocol: false)
+          end
+
+          include_examples 'uses disk storage'
+        end
+      end
+
+      context 'when pages_serve_from_deployments feature flag is disabled' do
+        before do
+          stub_feature_flags(pages_serve_from_deployments: false)
+        end
+
+        include_examples 'uses disk storage'
+      end
+    end
+
     context 'when artifact_id from build job is present in pages metadata' do
       let(:artifacts_archive) { create(:ci_job_artifact, :zip, :remote_store, project: project) }
 
@@ -66,26 +110,36 @@ RSpec.describe Pages::LookupPath do
         project.mark_pages_as_deployed(artifacts_archive: artifacts_archive)
       end
 
-      it 'sets the source type to "zip"' do
-        expect(lookup_path.source[:type]).to eq('zip')
-      end
-
-      it 'sets the source path to the artifacts archive URL' do
+      it 'uses artifacts object storage', :aggregate_failures do
         Timecop.freeze do
-          expect(lookup_path.source[:path]).to eq(artifacts_archive.file.url(expire_at: 1.day.from_now))
-          expect(lookup_path.source[:path]).to include("Expires=86400")
+          expect(source[:type]).to eq('zip')
+          expect(source[:path]).to eq(artifacts_archive.file.url(expire_at: 1.day.from_now))
+          expect(source[:path]).to include("Expires=86400")
         end
       end
 
       context 'when artifact is not uploaded to object storage' do
         let(:artifacts_archive) { create(:ci_job_artifact, :zip) }
 
-        include_examples 'uses disk storage'
+        it 'uses file protocol', :aggregate_failures do
+          Timecop.freeze do
+            expect(source[:type]).to eq('zip')
+            expect(source[:path]).to eq('file://' + artifacts_archive.file.path)
+          end
+        end
+
+        context 'when pages_serve_with_zip_file_protocol feature flag is disabled' do
+          before do
+            stub_feature_flags(pages_serve_with_zip_file_protocol: false)
+          end
+
+          include_examples 'uses disk storage'
+        end
       end
 
       context 'when feature flag is disabled' do
         before do
-          stub_feature_flags(pages_artifacts_archive: false)
+          stub_feature_flags(pages_serve_from_artifacts_archive: false)
         end
 
         include_examples 'uses disk storage'

spec/requests/api/internal/pages_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe API::Internal::Pages do
 
   before do
     allow(Gitlab::Pages).to receive(:secret).and_return(pages_secret)
+    stub_pages_object_storage(::Pages::DeploymentUploader)
   end
 
   describe "GET /internal/pages/status" do
@@ -38,6 +39,12 @@ RSpec.describe API::Internal::Pages do
       get api("/internal/pages"), headers: headers, params: { host: host }
     end
 
+    around do |example|
+      freeze_time do
+        example.run
+      end
+    end
+
     context 'not authenticated' do
       it 'responds with 401 Unauthorized' do
         query_host('pages.gitlab.io')
@@ -55,7 +62,9 @@ RSpec.describe API::Internal::Pages do
       end
 
       def deploy_pages(project)
+        deployment = create(:pages_deployment, project: project)
         project.mark_pages_as_deployed
+        project.update_pages_deployment!(deployment)
       end
 
       context 'domain does not exist' do
@@ -190,8 +199,8 @@ RSpec.describe API::Internal::Pages do
                   'https_only' => false,
                   'prefix' => '/',
                   'source' => {
-                    'type' => 'file',
-                    'path' => 'gitlab-org/gitlab-ce/public/'
+                    'type' => 'zip',
+                    'path' => project.pages_metadatum.pages_deployment.file.url(expire_at: 1.day.from_now)
                   }
                 }
               ]
@@ -226,8 +235,8 @@ RSpec.describe API::Internal::Pages do
                   'https_only' => false,
                   'prefix' => '/myproject/',
                   'source' => {
-                    'type' => 'file',
-                    'path' => 'mygroup/myproject/public/'
+                    'type' => 'zip',
+                    'path' => project.pages_metadatum.pages_deployment.file.url(expire_at: 1.day.from_now)
                   }
                 }
               ]
@@ -235,6 +244,20 @@ RSpec.describe API::Internal::Pages do
           end
         end
 
+        it 'avoids N+1 queries' do
+          project = create(:project, group: group)
+          deploy_pages(project)
+
+          control = ActiveRecord::QueryRecorder.new { query_host('mygroup.gitlab-pages.io') }
+
+          3.times do
+            project = create(:project, group: group)
+            deploy_pages(project)
+          end
+
+          expect { query_host('mygroup.gitlab-pages.io') }.not_to exceed_query_limit(control)
+        end
+
         context 'group root project' do
           it 'responds with the correct domain configuration' do
             project = create(:project, group: group, name: 'mygroup.gitlab-pages.io')
@@ -253,8 +276,8 @@ RSpec.describe API::Internal::Pages do
                   'https_only' => false,
                   'prefix' => '/',
                   'source' => {
-                    'type' => 'file',
-                    'path' => 'mygroup/mygroup.gitlab-pages.io/public/'
+                    'type' => 'zip',
+                    'path' => project.pages_metadatum.pages_deployment.file.url(expire_at: 1.day.from_now)
                   }
                 }
               ]