Merge branch '335816-add-limit-counter-on-schedule' into 'master'

Add DastSchedules to PlanLimits

See merge request gitlab-org/gitlab!68551

db/migrate/20210908060951_add_dast_schedules_to_plan_limits.rb

+# frozen_string_literal: true
+
+class AddDastSchedulesToPlanLimits < Gitlab::Database::Migration[1.0]
+  def change
+    add_column(:plan_limits, :dast_profile_schedules, :integer, default: 1, null: false)
+  end
+end

db/migrate/20210908061132_insert_dast_profile_schedules_plan_limits.rb

+# frozen_string_literal: true
+
+class InsertDastProfileSchedulesPlanLimits < Gitlab::Database::Migration[1.0]
+  def up
+    create_or_update_plan_limit('dast_profile_schedules', 'default', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'free', 1)
+    create_or_update_plan_limit('dast_profile_schedules', 'bronze', 1)
+    create_or_update_plan_limit('dast_profile_schedules', 'silver', 1)
+    create_or_update_plan_limit('dast_profile_schedules', 'premium', 1)
+    create_or_update_plan_limit('dast_profile_schedules', 'premium_trial', 1)
+    create_or_update_plan_limit('dast_profile_schedules', 'gold', 20)
+    create_or_update_plan_limit('dast_profile_schedules', 'ultimate', 20)
+    create_or_update_plan_limit('dast_profile_schedules', 'ultimate_trial', 20)
+  end
+
+  def down
+    create_or_update_plan_limit('dast_profile_schedules', 'default', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'free', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'bronze', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'silver', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'premium', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'premium_trial', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'gold', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'ultimate', 0)
+    create_or_update_plan_limit('dast_profile_schedules', 'ultimate_trial', 0)
+  end
+end

db/schema_migrations/20210908060951

+716fad7f9005a40d0c6a3acb8e348c7e9459a6a619fc0bf085f231f328f19fd1
\ No newline at end of file

db/schema_migrations/20210908061132

+f9e14410f22c94d0500102bb0fd0a3a6bd27343b74810b852ceb1349a04be72d
\ No newline at end of file

db/structure.sql

@@ -17367,7 +17367,8 @@ CREATE TABLE plan_limits (
     ci_max_artifact_size_running_container_scanning integer DEFAULT 0 NOT NULL,
     ci_max_artifact_size_cluster_image_scanning integer DEFAULT 0 NOT NULL,
     ci_jobs_trace_size_limit integer DEFAULT 100 NOT NULL,
-    pages_file_entries integer DEFAULT 200000 NOT NULL
+    pages_file_entries integer DEFAULT 200000 NOT NULL,
+    dast_profile_schedules integer DEFAULT 1 NOT NULL
 );
 
 CREATE SEQUENCE plan_limits_id_seq

doc/administration/instance_limits.md

@@ -523,6 +523,19 @@ Update `ci_jobs_trace_size_limit` with the new value in megabytes:
 Plan.default.actual_limits.update!(ci_jobs_trace_size_limit: 125)
 ```
 
+### Maximum number of active DAST profile schedules per project
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68551) in GitLab 14.3.
+
+Limit the number of active DAST profile schedules per project. A DAST profile schedule can be active or inactive.
+
+You can change the limit in the [GitLab Rails console](operations/rails_console.md#starting-a-rails-console-session).
+Update `dast_profile_schedules` with the new value:
+
+```ruby
+Plan.default.actual_limits.update!(dast_profile_schedules: 50)
+```
+
 ### Maximum size and depth of CI/CD configuration YAML files
 
 The default maximum size of a CI/CD configuration YAML file is 1 megabyte and the default depth is 100.

ee/app/models/dast/profile_schedule.rb

@@ -2,11 +2,16 @@
 
 class Dast::ProfileSchedule < ApplicationRecord
   include CronSchedulable
+  include Limitable
 
   CRON_DEFAULT = '* * * * *'
 
   self.table_name = 'dast_profile_schedules'
 
+  self.limit_name = table_name
+  self.limit_scope = :project
+  self.limit_relation = :with_active_schedules
+
   belongs_to :project
   belongs_to :dast_profile, class_name: 'Dast::Profile', optional: false, inverse_of: :dast_profile_schedule
   belongs_to :owner, class_name: 'User', optional: true, foreign_key: :user_id
@@ -18,9 +23,12 @@ class Dast::ProfileSchedule < ApplicationRecord
 
   serialize :cadence, Serializers::Json # rubocop:disable Cop/ActiveRecordSerialize
 
+  validate :validate_plan_limit_not_exceeded_while_activating, if: :will_save_change_to_active?
+
   scope :with_project, -> { includes(:project) }
   scope :with_profile, -> { includes(dast_profile: [:dast_site_profile, :dast_scanner_profile]) }
   scope :with_owner, -> { includes(:owner) }
+  scope :active_for_project, -> (project_id) { where(project_id: project_id).active }
   scope :active, -> { where(active: true) }
 
   before_save :set_cron, :set_next_run_at
@@ -39,6 +47,10 @@ class Dast::ProfileSchedule < ApplicationRecord
     owner&.name
   end
 
+  def with_active_schedules
+    self.class.active_for_project(project_id)
+  end
+
   private
 
   def deactivate!
@@ -71,4 +83,11 @@ class Dast::ProfileSchedule < ApplicationRecord
   def timezones
     @timezones ||= ActiveSupport::TimeZone.all.map { |tz| tz.tzinfo.identifier }
   end
+
+  # Plan limit is checked on: :create and if: :will_save_change_to_active?
+  # The will_save_change_to_active? returns false when the new object is created
+  # with the same value as the db defaults. That's why we need to this indirect to call validate_plan_limit_not_exceeded.
+  def validate_plan_limit_not_exceeded_while_activating
+    validate_plan_limit_not_exceeded
+  end
 end

ee/spec/models/dast/profile_schedule_spec.rb

@@ -3,7 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Dast::ProfileSchedule, type: :model do
-  subject { create(:dast_profile_schedule) }
+  let(:dast_profile_schedule) { create(:dast_profile_schedule, project: project) }
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:inactive_dast_profile_schedule) { create(:dast_profile_schedule, active: false, project: project) }
+
+  subject { dast_profile_schedule }
 
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
@@ -55,11 +60,50 @@ RSpec.describe Dast::ProfileSchedule, type: :model do
     end
   end
 
+  describe 'validate' do
+    describe 'validate_plan_limit_not_exceeded_while_activating' do
+      subject { build(:dast_profile_schedule, project: project, dast_profile: create(:dast_profile)) }
+
+      let_it_be(:plan_limits) { create(:plan_limits, :default_plan) }
+
+      context 'when the plan limit has not been exceeded' do
+        before do
+          create(:dast_profile_schedule, project: project, dast_profile: create(:dast_profile))
+        end
+
+        it 'can activate the schedule' do
+          expect { subject.save! }.to change { described_class.count }
+
+          subject.active = true
+
+          expect(subject.save).to be true
+        end
+      end
+
+      context 'when the plan limit has been exceeded' do
+        let_it_be(:inactive_schedule) { create(:dast_profile_schedule, project: project, dast_profile: create(:dast_profile), active: false) }
+
+        before do
+          plan_limits.update!(subject.class.limit_name => 1)
+
+          create(:dast_profile_schedule, project: create(:project), dast_profile: create(:dast_profile), active: true)
+        end
+
+        it 'prevents the schedule from being activated', :aggregate_failures do
+          expect { subject.dup.save! }.to change { described_class.count }
+
+          inactive_schedule.active = true
+
+          expect { inactive_schedule.save! }.to raise_error(ActiveRecord::RecordInvalid).and not_change { described_class.count }
+          expect(inactive_schedule.errors[:base]).to contain_exactly("Maximum number of #{inactive_schedule.class.limit_name.humanize(capitalize: false)} (1) exceeded")
+        end
+      end
+    end
+  end
+
   describe 'scopes' do
     describe 'active' do
       it 'includes the correct records' do
-        inactive_dast_profile_schedule = create(:dast_profile_schedule, active: false)
-
         result = described_class.active
 
         aggregate_failures do
@@ -169,6 +213,8 @@ RSpec.describe Dast::ProfileSchedule, type: :model do
   end
 
   describe '#schedule_next_run!' do
+    let_it_be(:plan_limits) { create(:plan_limits, :default_plan) }
+
     context 'when repeat? is true' do
       it 'sets active to true' do
         subject.schedule_next_run!
@@ -187,4 +233,18 @@ RSpec.describe Dast::ProfileSchedule, type: :model do
       end
     end
   end
+
+  describe 'limitable' do
+    it_behaves_like 'includes Limitable concern' do
+      subject { build(:dast_profile_schedule, project: project, dast_profile: create(:dast_profile)) }
+    end
+  end
+
+  describe '#with_active_schedules' do
+    subject { dast_profile_schedule.with_active_schedules }
+
+    it 'returns only active schedules' do
+      is_expected.not_to include(inactive_dast_profile_schedule)
+    end
+  end
 end

ee/spec/services/app_sec/dast/profiles/update_service_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe AppSec::Dast::Profiles::UpdateService do
   let_it_be(:dast_profile, reload: true) { create(:dast_profile, project: project, branch_name: 'orphaned-branch') }
   let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
   let_it_be(:dast_scanner_profile) { create(:dast_scanner_profile, project: project) }
+  let_it_be(:plan_limits) { create(:plan_limits, :default_plan) }
 
   let(:default_params) do
     {

ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 RSpec.describe AppSec::Dast::ProfileScheduleWorker do
   include ExclusiveLeaseHelpers
 
+  let_it_be(:plan_limits) { create(:plan_limits, :default_plan) }
   let_it_be(:schedule) { create(:dast_profile_schedule) }
 
   let(:worker) { described_class.new }

spec/factories/plan_limits.rb

@@ -4,6 +4,8 @@ FactoryBot.define do
   factory :plan_limits do
     plan
 
+    dast_profile_schedules { 50 }
+
     trait :default_plan do
       plan factory: :default_plan
     end