Merge branch '16610-gitlab-pages-storage-size-limitations-by-project-or-group-4' into 'master'

GitLab Pages - Storage size Limitations by Project or Group - backend

See merge request gitlab-org/gitlab!20154

app/controllers/projects/pages_controller.rb

@@ -34,7 +34,7 @@ class Projects::PagesController < Projects::ApplicationController
         if result[:status] == :success
           flash[:notice] = 'Your changes have been saved'
         else
-          flash[:alert] = 'Something went wrong on our end'
+          flash[:alert] = result[:message]
         end
 
         redirect_to project_pages_path(@project)
@@ -45,6 +45,12 @@ class Projects::PagesController < Projects::ApplicationController
   private
 
   def project_params
-    params.require(:project).permit(:pages_https_only)
+    params.require(:project).permit(project_params_attributes)
+  end
+
+  def project_params_attributes
+    %i[pages_https_only]
   end
 end
+
+Projects::PagesController.prepend_if_ee('EE::Projects::PagesController')

app/models/application_setting.rb

@@ -121,6 +121,11 @@ class ApplicationSetting < ApplicationRecord
             presence: true,
             numericality: { only_integer: true, greater_than: 0 }
 
+  validates :max_pages_size,
+            presence: true,
+            numericality: { only_integer: true, greater_than: 0,
+                            less_than: ::Gitlab::Pages::MAX_SIZE / 1.megabyte }
+
   validates :default_artifacts_expire_in, presence: true, duration: true
 
   validates :container_registry_token_expire_delay,

app/models/namespace.rb

@@ -46,6 +46,8 @@ class Namespace < ApplicationRecord
     length: { maximum: 255 },
     namespace_path: true
 
+  validates :max_artifacts_size, numericality: { only_integer: true, greater_than: 0, allow_nil: true }
+
   validate :nesting_level_allowed
 
   validates_associated :runners

app/models/project.rb

@@ -375,6 +375,7 @@ class Project < ApplicationRecord
     inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
   validates :variables, variable_duplicates: { scope: :environment_scope }
   validates :bfg_object_map, file_size: { maximum: :max_attachment_size }
+  validates :max_artifacts_size, numericality: { only_integer: true, greater_than: 0, allow_nil: true }
 
   # Scopes
   scope :pending_delete, -> { where(pending_delete: true) }

app/services/projects/update_pages_service.rb

@@ -6,7 +6,6 @@ module Projects
     FailedToExtractError = Class.new(StandardError)
 
     BLOCK_SIZE = 32.kilobytes
-    MAX_SIZE = 1.terabyte
     PUBLIC_DIR = 'public'
 
     # this has to be invalid group name,
@@ -130,12 +129,16 @@ module Projects
       1 + max_size / BLOCK_SIZE
     end
 
+    def max_size_from_settings
+      Gitlab::CurrentSettings.max_pages_size.megabytes
+    end
+
     def max_size
-      max_pages_size = Gitlab::CurrentSettings.max_pages_size.megabytes
+      max_pages_size = max_size_from_settings
 
-      return MAX_SIZE if max_pages_size.zero?
+      return ::Gitlab::Pages::MAX_SIZE if max_pages_size.zero?
 
-      [max_pages_size, MAX_SIZE].min
+      max_pages_size
     end
 
     def tmp_path
@@ -200,3 +203,5 @@ module Projects
     end
   end
 end
+
+Projects::UpdatePagesService.prepend_if_ee('EE::Projects::UpdatePagesService')

changelogs/unreleased/16610-gitlab-pages-storage-size-limitations-by-project-or-group-4.yml

+---
+title: Fix pages size limit setting in database if it is above the hard limit
+merge_request: 20154
+author:
+type: fixed

db/migrate/20191213120427_fix_max_pages_size.rb

+# frozen_string_literal: true
+
+class FixMaxPagesSize < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+  MAX_SIZE = 1.terabyte / 1.megabyte
+
+  class ApplicationSetting < ActiveRecord::Base
+    self.table_name = 'application_settings'
+    self.inheritance_column = :_type_disabled
+  end
+
+  def up
+    table = ApplicationSetting.arel_table
+    ApplicationSetting.where(table[:max_pages_size].gt(MAX_SIZE)).update_all(max_pages_size: MAX_SIZE)
+  end
+
+  def down
+    # no-op
+  end
+end

ee/app/controllers/ee/groups_controller.rb

@@ -34,6 +34,7 @@ module EE
         params_ee << :custom_project_templates_group_id if current_group&.group_project_template_available?
         params_ee << :ip_restriction_ranges if current_group&.feature_available?(:group_ip_restriction)
         params_ee << { allowed_email_domain_attributes: [:id, :domain] } if current_group&.feature_available?(:group_allowed_email_domains)
+        params_ee << :max_pages_size if can?(current_user, :update_max_pages_size)
       end
     end
 

ee/app/controllers/ee/projects/pages_controller.rb

+# frozen_string_literal: true
+
+module EE
+  module Projects
+    module PagesController
+      extend ActiveSupport::Concern
+      extend ::Gitlab::Utils::Override
+
+      override :project_params_attributes
+      def project_params_attributes
+        super + project_params_ee
+      end
+
+      private
+
+      def project_params_ee
+        if can?(current_user, :update_max_pages_size)
+          %i[max_pages_size]
+        else
+          []
+        end
+      end
+    end
+  end
+end

ee/app/models/ee/namespace.rb

@@ -64,6 +64,10 @@ module EE
       validate :validate_plan_name
       validate :validate_shared_runner_minutes_support
 
+      validates :max_pages_size,
+                numericality: { only_integer: true, greater_than: 0, allow_nil: true,
+                                less_than: ::Gitlab::Pages::MAX_SIZE / 1.megabyte }
+
       delegate :trial?, :trial_ends_on, :trial_starts_on, :upgradable?, to: :gitlab_subscription, allow_nil: true
 
       before_create :sync_membership_lock_with_parent

ee/app/models/ee/project.rb

@@ -162,6 +162,9 @@ module EE
 
       validates :repository_size_limit,
         numericality: { only_integer: true, greater_than_or_equal_to: 0, allow_nil: true }
+      validates :max_pages_size,
+        numericality: { only_integer: true, greater_than: 0, allow_nil: true,
+                        less_than: ::Gitlab::Pages::MAX_SIZE / 1.megabyte }
 
       validates :approvals_before_merge, numericality: true, allow_blank: true
 

ee/app/models/license.rb

@@ -88,6 +88,7 @@ class License < ApplicationRecord
     object_storage
     operations_dashboard
     packages
+    pages_size_limit
     productivity_analytics
     project_aliases
     protected_environments
@@ -204,6 +205,7 @@ class License < ApplicationRecord
     ldap_group_sync_filter
     multiple_ldap_servers
     object_storage
+    pages_size_limit
     project_aliases
     repository_size_limit
     required_ci_templates

ee/app/policies/ee/global_policy.rb

@@ -13,6 +13,10 @@ module EE
         License.feature_available?(:security_dashboard)
       end
 
+      condition(:pages_size_limit_available) do
+        License.feature_available?(:pages_size_limit)
+      end
+
       rule { ~anonymous & operations_dashboard_available }.enable :read_operations_dashboard
       rule { ~anonymous & security_dashboard_available }.enable :read_security_dashboard
 
@@ -21,6 +25,8 @@ module EE
         enable :destroy_licenses
       end
 
+      rule { admin & pages_size_limit_available }.enable :update_max_pages_size
+
       rule { support_bot }.prevent :use_quick_actions
 
       rule { ~anonymous }.policy do

ee/app/services/ee/projects/update_pages_service.rb

+# frozen_string_literal: true
+
+module EE::Projects::UpdatePagesService
+  extend ::Gitlab::Utils::Override
+
+  override :max_size_from_settings
+  def max_size_from_settings
+    return super unless project.feature_available?(:pages_size_limit)
+
+    project.closest_setting(:max_pages_size).megabytes
+  end
+end

ee/spec/controllers/groups_controller_spec.rb

@@ -20,4 +20,42 @@ describe GroupsController do
       end
     end
   end
+
+  describe 'PUT #update' do
+    let(:group) { create(:group) }
+
+    context 'when max_pages_size param is specified' do
+      let(:params) { { max_pages_size: 100 } }
+
+      let(:request) do
+        post :update, params: { id: group.to_param, group: params }
+      end
+
+      let(:user) { create(:user) }
+
+      before do
+        stub_licensed_features(pages_size_limit: true)
+        group.add_owner(user)
+        sign_in(user)
+      end
+
+      context 'when user is an admin' do
+        let(:user) { create(:admin) }
+
+        it 'updates max_pages_size' do
+          request
+
+          expect(group.reload.max_pages_size).to eq(100)
+        end
+      end
+
+      context 'when user is not an admin' do
+        it 'does not update max_pages_size' do
+          request
+
+          expect(group.reload.max_pages_size).to eq(nil)
+        end
+      end
+    end
+  end
 end

ee/spec/controllers/projects/pages_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::PagesController do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  before do
+    allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
+    project.add_maintainer(user)
+    sign_in(user)
+  end
+
+  context 'when max_pages_size param is specified' do
+    let(:params) { { max_pages_size: 100 } }
+
+    let(:request) do
+      put :update, params: { namespace_id: project.namespace, project_id: project, project: params }
+    end
+
+    before do
+      stub_licensed_features(pages_size_limit: true)
+    end
+
+    context 'when user is an admin' do
+      let(:admin) { create(:admin) }
+
+      before do
+        sign_in(admin)
+      end
+
+      it 'updates max_pages_size' do
+        request
+
+        expect(project.reload.max_pages_size).to eq(100)
+      end
+    end
+
+    context 'when user is not an admin' do
+      it 'does not update max_pages_size' do
+        request
+
+        expect(project.reload.max_pages_size).to eq(nil)
+      end
+    end
+  end
+end

ee/spec/models/namespace_spec.rb

@@ -79,6 +79,13 @@ describe Namespace do
     end
   end
 
+  context 'validation' do
+    it do
+      is_expected.to validate_numericality_of(:max_pages_size).only_integer.is_greater_than(0)
+                       .is_less_than(::Gitlab::Pages::MAX_SIZE / 1.megabyte)
+    end
+  end
+
   describe 'custom validations' do
     describe '#validate_plan_name' do
       let(:group) { build(:group) }

ee/spec/models/project_spec.rb

@@ -195,6 +195,11 @@ describe Project do
       context 'with same variable keys and different environment scope' do
         it { expect(project).to be_valid }
       end
+
+      it do
+        is_expected.to validate_numericality_of(:max_pages_size).only_integer.is_greater_than(0)
+                         .is_less_than(::Gitlab::Pages::MAX_SIZE / 1.megabyte)
+      end
     end
 
     context 'mirror' do

ee/spec/policies/global_policy_spec.rb

@@ -89,4 +89,17 @@ describe GlobalPolicy do
       it { is_expected.not_to be_allowed(:read_security_dashboard) }
     end
   end
+
+  describe 'update_max_pages_size' do
+    context 'when feature is enabled' do
+      before do
+        stub_licensed_features(pages_size_limit: true)
+      end
+
+      it { is_expected.to be_disallowed(:update_max_pages_size) }
+      it { expect(described_class.new(create(:admin), [user])).to be_allowed(:update_max_pages_size) }
+    end
+
+    it { expect(described_class.new(create(:admin), [user])).to be_disallowed(:update_max_pages_size) }
+  end
 end

ee/spec/services/projects/update_pages_service_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe Projects::UpdatePagesService do
+  let(:root_namespace) { create(:namespace, max_pages_size: 300) }
+  let(:namespace) { create(:namespace, parent: root_namespace, max_pages_size: 200) }
+  let(:project) { create(:project, :repository, namespace: namespace, max_pages_size: 250) }
+  let(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) }
+  let(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') }
+
+  subject { described_class.new(project, build) }
+
+  describe 'maximum pages artifacts size' do
+    let(:metadata) { spy('metadata') }
+
+    before do
+      file = fixture_file_upload('spec/fixtures/pages.zip')
+      metafile = fixture_file_upload('spec/fixtures/pages.zip.meta')
+
+      create(:ci_job_artifact, :archive, file: file, job: build)
+      create(:ci_job_artifact, :metadata, file: metafile, job: build)
+
+      allow(build).to receive(:artifacts_metadata_entry)
+                        .and_return(metadata)
+
+      stub_licensed_features(pages_size_limit: true)
+    end
+
+    it_behaves_like 'pages size limit is', 250.megabytes
+
+    it 'uses closest setting for max_pages_size' do
+      allow(metadata).to receive(:total_size).and_return(1.megabyte)
+
+      expect(project).to receive(:closest_setting).with(:max_pages_size).and_call_original
+
+      subject.execute
+    end
+
+    context 'when pages_size_limit feature is not available' do
+      before do
+        stub_licensed_features(pages_size_limit: false)
+      end
+
+      it_behaves_like 'pages size limit is', 100.megabytes
+    end
+  end
+
+  def deploy_status
+    GenericCommitStatus.find_by(name: 'pages:deploy')
+  end
+end

lib/gitlab/pages.rb

@@ -4,6 +4,7 @@ module Gitlab
   class Pages
     VERSION = File.read(Rails.root.join("GITLAB_PAGES_VERSION")).strip.freeze
     INTERNAL_API_REQUEST_HEADER = 'Gitlab-Pages-Api-Request'.freeze
+    MAX_SIZE = 1.terabyte
 
     include JwtAuthenticatable
 

spec/controllers/projects/pages_controller_spec.rb

@@ -115,5 +115,16 @@ describe Projects::PagesController do
 
       patch :update, params: request_params
     end
+
+    context 'when update_service returns an error message' do
+      let(:update_service) { double(execute: { status: :error, message: 'some error happened' }) }
+
+      it 'adds an error message' do
+        patch :update, params: request_params
+
+        expect(response).to redirect_to(project_pages_path(project))
+        expect(flash[:alert]).to eq('some error happened')
+      end
+    end
   end
 end

spec/features/projects/pages_spec.rb

@@ -322,7 +322,7 @@ shared_examples 'pages settings editing' do
 
       before do
         allow(Projects::UpdateService).to receive(:new).and_return(service)
-        allow(service).to receive(:execute).and_return(status: :error)
+        allow(service).to receive(:execute).and_return(status: :error, message: 'Some error has occured')
       end
 
       it 'tries to change the setting' do
@@ -332,7 +332,7 @@ shared_examples 'pages settings editing' do
 
         click_button 'Save'
 
-        expect(page).to have_text('Something went wrong on our end')
+        expect(page).to have_text('Some error has occured')
       end
     end
 

spec/migrations/fix_max_pages_size_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20191213120427_fix_max_pages_size.rb')
+
+describe FixMaxPagesSize, :migration do
+  let(:application_settings) { table(:application_settings) }
+  let!(:default_setting) { application_settings.create! }
+  let!(:max_possible_setting) { application_settings.create!(max_pages_size: described_class::MAX_SIZE) }
+  let!(:higher_than_maximum_setting) { application_settings.create!(max_pages_size: described_class::MAX_SIZE + 1) }
+
+  it 'correctly updates settings only if needed' do
+    migrate!
+
+    expect(default_setting.reload.max_pages_size).to eq(100)
+    expect(max_possible_setting.reload.max_pages_size).to eq(described_class::MAX_SIZE)
+    expect(higher_than_maximum_setting.reload.max_pages_size).to eq(described_class::MAX_SIZE)
+  end
+end

spec/models/application_setting_spec.rb

@@ -67,6 +67,13 @@ describe ApplicationSetting do
     it { is_expected.not_to allow_value(nil).for(:push_event_activities_limit) }
 
     it { is_expected.to validate_numericality_of(:snippet_size_limit).only_integer.is_greater_than(0) }
+    it { is_expected.to validate_presence_of(:max_artifacts_size) }
+    it do
+      is_expected.to validate_numericality_of(:max_pages_size).only_integer.is_greater_than(0)
+                       .is_less_than(::Gitlab::Pages::MAX_SIZE / 1.megabyte)
+    end
+    it { is_expected.to validate_numericality_of(:max_artifacts_size).only_integer.is_greater_than(0) }
+    it { is_expected.to validate_numericality_of(:max_pages_size).only_integer.is_greater_than(0) }
 
     it { is_expected.not_to allow_value(7).for(:minimum_password_length) }
     it { is_expected.not_to allow_value(129).for(:minimum_password_length) }

spec/models/namespace_spec.rb

@@ -26,6 +26,7 @@ describe Namespace do
     it { is_expected.to validate_presence_of(:path) }
     it { is_expected.to validate_length_of(:path).is_at_most(255) }
     it { is_expected.to validate_presence_of(:owner) }
+    it { is_expected.to validate_numericality_of(:max_artifacts_size).only_integer.is_greater_than(0) }
 
     it 'does not allow too deep nesting' do
       ancestors = (1..21).to_a

spec/models/project_spec.rb

@@ -211,6 +211,7 @@ describe Project do
     it { is_expected.to validate_presence_of(:creator) }
     it { is_expected.to validate_presence_of(:namespace) }
     it { is_expected.to validate_presence_of(:repository_storage) }
+    it { is_expected.to validate_numericality_of(:max_artifacts_size).only_integer.is_greater_than(0) }
 
     it 'validates build timeout constraints' do
       is_expected.to validate_numericality_of(:build_timeout)

spec/services/projects/update_pages_service_spec.rb

@@ -185,60 +185,20 @@ describe Projects::UpdatePagesService do
         .and_return(metadata)
     end
 
-    shared_examples 'pages size limit exceeded' do
-      it 'limits the maximum size of gitlab pages' do
-        subject.execute
-
-        expect(deploy_status.description)
-          .to match(/artifacts for pages are too large/)
-        expect(deploy_status).to be_script_failure
-        expect(project.pages_metadatum).not_to be_deployed
-      end
-    end
-
     context 'when maximum pages size is set to zero' do
       before do
         stub_application_setting(max_pages_size: 0)
       end
 
-      context 'when page size does not exceed internal maximum' do
-        before do
-          allow(metadata).to receive(:total_size).and_return(200.megabytes)
-        end
-
-        it 'updates pages correctly' do
-          subject.execute
-
-          expect(deploy_status.description).not_to be_present
-          expect(project.pages_metadatum).to be_deployed
-        end
-      end
-
-      context 'when pages size does exceed internal maximum' do
-        before do
-          allow(metadata).to receive(:total_size).and_return(2.terabytes)
-        end
-
-        it_behaves_like 'pages size limit exceeded'
-      end
-    end
-
-    context 'when pages size is greater than max size setting' do
-      before do
-        stub_application_setting(max_pages_size: 200)
-        allow(metadata).to receive(:total_size).and_return(201.megabytes)
-      end
-
-      it_behaves_like 'pages size limit exceeded'
+      it_behaves_like 'pages size limit is', ::Gitlab::Pages::MAX_SIZE
     end
 
-    context 'when max size setting is greater than internal max size' do
+    context 'when size is limited on the instance level' do
       before do
-        stub_application_setting(max_pages_size: 3.terabytes / 1.megabyte)
-        allow(metadata).to receive(:total_size).and_return(2.terabytes)
+        stub_application_setting(max_pages_size: 100)
       end
 
-      it_behaves_like 'pages size limit exceeded'
+      it_behaves_like 'pages size limit is', 100.megabytes
     end
   end
 

spec/support/shared_examples/pages_size_limit_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'pages size limit is' do |size_limit|
+  context "when size is below the limit" do
+    before do
+      allow(metadata).to receive(:total_size).and_return(size_limit - 1.megabyte)
+    end
+
+    it 'updates pages correctly' do
+      subject.execute
+
+      expect(deploy_status.description).not_to be_present
+      expect(project.pages_metadatum).to be_deployed
+    end
+  end
+
+  context "when size is above the limit" do
+    before do
+      allow(metadata).to receive(:total_size).and_return(size_limit + 1.megabyte)
+    end
+
+    it 'limits the maximum size of gitlab pages' do
+      subject.execute
+
+      expect(deploy_status.description)
+        .to match(/artifacts for pages are too large/)
+      expect(deploy_status).to be_script_failure
+    end
+  end
+end