From 32dd2feff934dd4484fb888cadac2d891a404c67 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Thu, 12 Aug 2021 12:59:12 +0530 Subject: [PATCH 01/13] Add cadence, timezone and starts_at columns to dast_profile_schedules Add following columns to dast_profile_schedules cadence:jsonb, timezone:text starts_at:datetime_with_timezone Changelog: changed --- ...1446_add_cadence_to_dast_profile_schedules.rb | 7 +++++++ ...621_add_timezone_to_dast_profile_schedules.rb | 16 ++++++++++++++++ ...04_add_starts_at_to_dast_profile_schedules.rb | 7 +++++++ db/schema_migrations/20210807101446 | 1 + db/schema_migrations/20210807101621 | 1 + db/schema_migrations/20210807102004 | 1 + db/structure.sql | 6 +++++- 7 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20210807101446_add_cadence_to_dast_profile_schedules.rb create mode 100644 db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb create mode 100644 db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb create mode 100644 db/schema_migrations/20210807101446 create mode 100644 db/schema_migrations/20210807101621 create mode 100644 db/schema_migrations/20210807102004 diff --git a/db/migrate/20210807101446_add_cadence_to_dast_profile_schedules.rb b/db/migrate/20210807101446_add_cadence_to_dast_profile_schedules.rb new file mode 100644 index 00000000000000..c9b17e3d5c59d8 --- /dev/null +++ b/db/migrate/20210807101446_add_cadence_to_dast_profile_schedules.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddCadenceToDastProfileSchedules < ActiveRecord::Migration[6.1] + def change + add_column :dast_profile_schedules, :cadence, :jsonb, null: false, default: {} + end +end diff --git a/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb b/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb new file mode 100644 index 00000000000000..3c0668d27b648a --- /dev/null +++ b/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddTimezoneToDastProfileSchedules < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + add_column :dast_profile_schedules, :timezone, :text, null: false, default: '' + add_text_limit :dast_profile_schedules, :timezone, 255 + end + + def down + remove_column :dast_profile_schedules, :timezone + end +end diff --git a/db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb b/db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb new file mode 100644 index 00000000000000..32c8f506fe2144 --- /dev/null +++ b/db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddStartsAtToDastProfileSchedules < ActiveRecord::Migration[6.1] + def change + add_column :dast_profile_schedules, :starts_at, :datetime_with_timezone, null: false, default: '' + end +end diff --git a/db/schema_migrations/20210807101446 b/db/schema_migrations/20210807101446 new file mode 100644 index 00000000000000..0b6d526429fd8b --- /dev/null +++ b/db/schema_migrations/20210807101446 @@ -0,0 +1 @@ +30e1463616c60b92afb28bbb76e3c55830a385af6df0e60e16ed96d9e75943b9 \ No newline at end of file diff --git a/db/schema_migrations/20210807101621 b/db/schema_migrations/20210807101621 new file mode 100644 index 00000000000000..ab053cf4cbc427 --- /dev/null +++ b/db/schema_migrations/20210807101621 @@ -0,0 +1 @@ +7e9b39914ade766357751953a4981225dbae7e5d371d4824af61b01af70f46ae \ No newline at end of file diff --git a/db/schema_migrations/20210807102004 b/db/schema_migrations/20210807102004 new file mode 100644 index 00000000000000..e63485435f822a --- /dev/null +++ b/db/schema_migrations/20210807102004 @@ -0,0 +1 @@ +a2454f9fca3b1cedf7a0f2288b69abe799fe1f9ff4e2fe26d2cadfdddea73a83 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 870277556bb34f..db576022d667d7 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12064,7 +12064,11 @@ CREATE TABLE dast_profile_schedules ( updated_at timestamp with time zone NOT NULL, active boolean DEFAULT true NOT NULL, cron text NOT NULL, - CONSTRAINT check_86531ea73f CHECK ((char_length(cron) <= 255)) + cadence jsonb DEFAULT '{}'::jsonb NOT NULL, + timezone text DEFAULT ''::text NOT NULL, + starts_at timestamp with time zone NOT NULL, + CONSTRAINT check_86531ea73f CHECK ((char_length(cron) <= 255)), + CONSTRAINT check_be4d1c3af1 CHECK ((char_length(timezone) <= 255)) ); COMMENT ON TABLE dast_profile_schedules IS '{"owner":"group::dynamic analysis","description":"Scheduling for scans using DAST Profiles"}'; -- GitLab From 91a53fee6697801672dc6150d8a304dc8fee9ed1 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Thu, 12 Aug 2021 13:57:06 +0530 Subject: [PATCH 02/13] Add Validations and next run at scheduling logic Add model validations. Add scheduling logic. Add cron expression generator --- app/models/concerns/cron_schedulable.rb | 8 +- .../dast_profile_schedule_cadence.json | 31 +++++ ee/app/models/dast/profile.rb | 2 +- ee/app/models/dast/profile_schedule.rb | 55 ++++++++- ee/spec/factories/dast/profile_schedules.rb | 5 +- ee/spec/models/dast/profile_schedule_spec.rb | 108 +++++++++++++++--- ee/spec/models/dast/profile_spec.rb | 2 +- .../dast/profile_schedule_worker_spec.rb | 12 ++ lib/gitlab/ci/cron_parser.rb | 32 +++++- spec/lib/gitlab/ci/cron_parser_spec.rb | 45 ++++++++ 10 files changed, 271 insertions(+), 29 deletions(-) create mode 100644 app/validators/json_schemas/dast_profile_schedule_cadence.json diff --git a/app/models/concerns/cron_schedulable.rb b/app/models/concerns/cron_schedulable.rb index 48605ecc3d7231..d5b86db2640da6 100644 --- a/app/models/concerns/cron_schedulable.rb +++ b/app/models/concerns/cron_schedulable.rb @@ -14,12 +14,10 @@ def set_next_run_at # The `next_run_at` column is set to the actual execution date of worker that # triggers the schedule. This way, a schedule like `*/1 * * * *` won't be triggered # in a short interval when the worker runs irregularly by Sidekiq Memory Killer. - def calculate_next_run_at - now = Time.zone.now + def calculate_next_run_at(start_time = Time.zone.now) + ideal_next_run = ideal_next_run_from(start_time) - ideal_next_run = ideal_next_run_from(now) - - if ideal_next_run == cron_worker_next_run_from(now) + if ideal_next_run == cron_worker_next_run_from(start_time) ideal_next_run else cron_worker_next_run_from(ideal_next_run) diff --git a/app/validators/json_schemas/dast_profile_schedule_cadence.json b/app/validators/json_schemas/dast_profile_schedule_cadence.json new file mode 100644 index 00000000000000..5583acfa5af5f7 --- /dev/null +++ b/app/validators/json_schemas/dast_profile_schedule_cadence.json @@ -0,0 +1,31 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "Dast profile schedule cadence schema", + "type": "object", + "anyOf": [ + { + "properties": { + "unit": { "enum": ["day"] }, + "duration": { "enum": [1] } + } + }, + { + "properties": { + "unit": { "enum": ["week"] }, + "duration": { "enum": [1] } + } + }, + { + "properties": { + "unit": { "enum": ["month"] }, + "duration": { "enum": [1, 3 ,6] } + } + }, + { + "properties": { + "unit": { "enum": ["year"] }, + "duration": { "enum": [1] } + } + } + ] +} diff --git a/ee/app/models/dast/profile.rb b/ee/app/models/dast/profile.rb index d79854c655424d..542f71bcc164b9 100644 --- a/ee/app/models/dast/profile.rb +++ b/ee/app/models/dast/profile.rb @@ -10,7 +10,7 @@ class Profile < ApplicationRecord has_many :secret_variables, through: :dast_site_profile, class_name: 'Dast::SiteProfileSecretVariable' - has_many :dast_profile_schedules, class_name: 'Dast::ProfileSchedule', foreign_key: :dast_profile_id, inverse_of: :dast_profile + has_one :dast_profile_schedule, class_name: 'Dast::ProfileSchedule', foreign_key: :dast_profile_id, inverse_of: :dast_profile validates :description, length: { maximum: 255 } validates :name, length: { maximum: 255 }, uniqueness: { scope: :project_id }, presence: true diff --git a/ee/app/models/dast/profile_schedule.rb b/ee/app/models/dast/profile_schedule.rb index debe43ed2bcdc5..c784546ef6119f 100644 --- a/ee/app/models/dast/profile_schedule.rb +++ b/ee/app/models/dast/profile_schedule.rb @@ -3,24 +3,71 @@ class Dast::ProfileSchedule < ApplicationRecord include CronSchedulable + CRON_DEFAULT = '* * * * *' + self.table_name = 'dast_profile_schedules' belongs_to :project - belongs_to :dast_profile, class_name: 'Dast::Profile', optional: false, inverse_of: :dast_profile_schedules + 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 - validates :cron, presence: true - validates :next_run_at, presence: true + validates :timezone, presence: true, length: { maximum: 255 } + validates :starts_at, presence: true + validates :cadence, json_schema: { filename: 'dast_profile_schedule_cadence', draft: 7 } + + serialize :cadence, Serializers::Json # rubocop:disable Cop/ActiveRecordSerialize scope :with_project, -> { includes(:project) } scope :with_profile, -> { includes(dast_profile: [:dast_site_profile, :dast_scanner_profile]) } scope :with_owner, -> { includes(:owner) } scope :active, -> { where(active: true) } + attribute :set_offset, :boolean, default: false + + before_save :set_offset_true!, if: :will_save_change_to_starts_at? + before_save :set_cron, :set_next_run_at + + def repeat? + self.cadence != {} + end + + def schedule_next_run! + return deactivate! unless repeat? + + super + end + + def audit_details + owner&.name + end + private + def deactivate! + update!(active: false) + end + def cron_timezone - next_run_at.zone + Time.zone.name + end + + def set_cron + self.cron = + if repeat? + Gitlab::Ci::CronParser.parse_natural_with_timestamp(starts_at, cadence) + else + CRON_DEFAULT + end + end + + def set_offset_true! + self.set_offset = true + end + + def set_next_run_at + return super unless set_offset + + self.next_run_at = cron_worker_next_run_from(starts_at) end def worker_cron_expression diff --git a/ee/spec/factories/dast/profile_schedules.rb b/ee/spec/factories/dast/profile_schedules.rb index 6098f733285192..d00f03fc308263 100644 --- a/ee/spec/factories/dast/profile_schedules.rb +++ b/ee/spec/factories/dast/profile_schedules.rb @@ -5,7 +5,8 @@ project dast_profile owner { association(:user) } - cron { '*/10 * * * *' } - next_run_at { Time.now } + timezone { FFaker::Address.time_zone } + starts_at { Time.now } + cadence { { unit: %w(day month year week).sample, duration: 1 } } end end diff --git a/ee/spec/models/dast/profile_schedule_spec.rb b/ee/spec/models/dast/profile_schedule_spec.rb index 0f7107136ca81e..1345845fc3c053 100644 --- a/ee/spec/models/dast/profile_schedule_spec.rb +++ b/ee/spec/models/dast/profile_schedule_spec.rb @@ -7,14 +7,48 @@ describe 'associations' do it { is_expected.to belong_to(:project) } - it { is_expected.to belong_to(:dast_profile).class_name('Dast::Profile').required.inverse_of(:dast_profile_schedules) } + it { is_expected.to belong_to(:dast_profile).class_name('Dast::Profile').required.inverse_of(:dast_profile_schedule) } it { is_expected.to belong_to(:owner).class_name('User').with_foreign_key(:user_id) } end describe 'validations' do it { is_expected.to be_valid } - it { is_expected.to validate_presence_of(:cron) } - it { is_expected.to validate_presence_of(:next_run_at) } + it { is_expected.to validate_presence_of(:timezone) } + it { is_expected.to validate_length_of(:timezone).is_at_most(255) } + + it { is_expected.to validate_presence_of(:starts_at) } + + describe 'cadence' do + context 'when valid values' do + [ + { unit: 'day', duration: 1 }, + { unit: 'week', duration: 1 }, + { unit: 'month', duration: 1 }, + { unit: 'month', duration: 3 }, + { unit: 'month', duration: 6 }, + { unit: 'year', duration: 1 }, + {} + ].each do |cadence| + it "allows #{cadence[:unit]} values" do + schedule = build(:dast_profile_schedule, cadence: cadence) + expect(schedule.cadence).to eq(cadence.stringify_keys) + end + end + end + + context 'when invalid values' do + [ + { unit: 'day', duration: 3 }, + { unit: 'month_foo', duration: 100 } + ].each do |cadence| + it "disallow #{cadence[:unit]} values" do + expect { build(:dast_profile_schedule, cadence: cadence).validate! }.to raise_error(ActiveRecord::RecordInvalid) do |err| + expect(err.record.errors.full_messages).to include('Cadence must be a valid json schema') + end + end + end + end + end end describe 'scopes' do @@ -31,13 +65,13 @@ end end - describe 'runnable_schedules' do + describe '.runnable_schedules' do subject { described_class.runnable_schedules } context 'when there are runnable schedules' do let!(:profile_schedule) do - travel_to(1.day.ago) do - create(:dast_profile_schedule) + travel_to(2.days.ago) do + create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }) end end @@ -80,15 +114,61 @@ end end + describe 'before_save' do + describe '#set_cron' do + it 'sets the cron value' do + expect(subject.cron).to be_present + end + end + end + + describe 'attributes' do + it { is_expected.to have_attributes(set_offset: true) } + end + describe '#set_next_run_at' do - it_behaves_like 'handles set_next_run_at' do - let(:schedule) { create(:dast_profile_schedule, cron: '*/1 * * * *') } - let(:schedule_1) { create(:dast_profile_schedule) } - let(:schedule_2) { create(:dast_profile_schedule) } - let(:new_cron) { '0 0 1 1 *' } - - let(:ideal_next_run_at) { schedule.send(:ideal_next_run_from, Time.zone.now) } - let(:cron_worker_next_run_at) { schedule.send(:cron_worker_next_run_from, Time.zone.now) } + let(:schedule) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }, starts_at: Time.zone.now + 1.minute) } + let(:schedule_1) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }) } + let(:schedule_2) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }) } + + let(:cron_worker_next_run_at) { schedule.send(:cron_worker_next_run_from, Time.zone.now) } + + context 'when schedule runs every minute' do + it "updates next_run_at to the worker's execution time" do + travel_to(1.day.ago) do + expect(schedule.next_run_at).to eq(cron_worker_next_run_at) + end + end + end + + context 'when there are two different schedules in the same time zones' do + it 'sets the sames next_run_at' do + expect(schedule_1.next_run_at).to eq(schedule_2.next_run_at) + end + end + + context 'when starts_at is updated for existing schedules' do + it 'updates next_run_at automatically' do + expect { schedule.update!(starts_at: Time.zone.now + 2.days + 1.minute) }.to change { schedule.next_run_at } + expect(schedule.set_offset).to be true + end + end + end + + describe '#schedule_next_run!' do + context 'when repeat? is true' do + it 'does not set the active to false' do + subject.schedule_next_run! + expect(subject.active).to be true + end + end + + context 'when repeat? is false' do + it 'does not set the active to false' do + subject.update_column(:cadence, {}) + subject.schedule_next_run! + expect(subject.active).to be false + end end end end diff --git a/ee/spec/models/dast/profile_spec.rb b/ee/spec/models/dast/profile_spec.rb index 12358f55d9caf2..a76216f431f549 100644 --- a/ee/spec/models/dast/profile_spec.rb +++ b/ee/spec/models/dast/profile_spec.rb @@ -12,7 +12,7 @@ it { is_expected.to belong_to(:dast_site_profile) } it { is_expected.to belong_to(:dast_scanner_profile) } it { is_expected.to have_many(:secret_variables).through(:dast_site_profile).class_name('Dast::SiteProfileSecretVariable') } - it { is_expected.to have_many(:dast_profile_schedules).class_name('Dast::ProfileSchedule').with_foreign_key(:dast_profile_id).inverse_of(:dast_profile) } + it { is_expected.to have_one(:dast_profile_schedule).class_name('Dast::ProfileSchedule').with_foreign_key(:dast_profile_id).inverse_of(:dast_profile) } end describe 'validations' do diff --git a/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb b/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb index 4b0e999d558b2b..5ca7e8cd77786e 100644 --- a/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb +++ b/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb @@ -101,5 +101,17 @@ def record_preloaded_queries subject end end + + context 'when single run schedule exists' do + before do + schedule.update_columns(next_run_at: 1.minute.ago, cadence: {}) + end + + it 'executes the rule schedule service and deactivate the schedule' do + expect(schedule.repeat?).to be(false) + subject + expect(schedule.reload.active).to be(false) + end + end end end diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index bc03658aab8614..a54f239ba1ef08 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -6,8 +6,36 @@ class CronParser VALID_SYNTAX_SAMPLE_TIME_ZONE = 'UTC' VALID_SYNTAX_SAMPLE_CRON = '* * * * *' - def self.parse_natural(expression, cron_timezone = 'UTC') - new(Fugit::Nat.parse(expression)&.original, cron_timezone) + class << self + def parse_natural(expression, cron_timezone = 'UTC') + new(Fugit::Nat.parse(expression)&.original, cron_timezone) + end + + # This method generates compatible expressions that can be + # parsed by Fugit::Nat.parse to generate a cron line. + # It takes start date of the cron and cadence in the following format: + # cadence = { + # unit: 'day/week/month/year' + # duration: 1 + # } + def parse_natural_with_timestamp(starts_at, cadence) + case cadence[:unit] + when 'day' # Currently supports only 'every 1 day'. + "#{starts_at.min} #{starts_at.hour} * * *" + when 'week' # Currently supports only 'every 1 week'. + "#{starts_at.min} #{starts_at.hour} * * #{starts_at.wday}" + when 'month' + "#{starts_at.min} #{starts_at.hour} #{starts_at.mday} #{fallin_months(cadence[:duration], starts_at)} *" + when 'year' # Currently supports only 'every 1 year'. + "#{starts_at.min} #{starts_at.hour} #{starts_at.mday} #{starts_at.month} *" + else + raise NotImplementedError, "The cadence unit #{cadence[:unit]} is not implemented" + end + end + + def fallin_months(offset, start_date) + (1..(12 / offset)).map { |i| start_date.next_month(offset * i).month }.join(',') + end end def initialize(cron, cron_timezone = 'UTC') diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 15293429354c0e..0b5254932acfc7 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -297,4 +297,49 @@ it { is_expected.to eq(true) } end end + + describe '.parse_natural', :aggregate_failures do + let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'day', duration: 1 }) } + let(:time) { Time.zone.now } + let(:hours) { Fugit::Cron.parse(cron_line).hours } + let(:minutes) { Fugit::Cron.parse(cron_line).minutes } + let(:weekdays) { Fugit::Cron.parse(cron_line).weekdays.first } + let(:months) { Fugit::Cron.parse(cron_line).months } + + context 'when repeat cycle is day' do + it 'generates daily cron expression' do + expect(hours).to include time.hour + expect(minutes).to include time.min + end + end + + context 'when repeat cycle is week' do + let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'week', duration: 1 }) } + + it 'generates weekly cron expression' do + expect(hours).to include time.hour + expect(minutes).to include time.min + expect(weekdays).to include time.wday + end + end + + context 'when repeat cycle is month' do + let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'month', duration: 3 }) } + + it 'generates monthly cron expression' do + expect(minutes).to include time.min + expect(months).to include time.month + end + end + + context 'when repeat cycle is year' do + let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'year', duration: 1 }) } + + it 'generates yearly cron expression' do + expect(hours).to include time.hour + expect(minutes).to include time.min + expect(months).to include time.month + end + end + end end -- GitLab From 4b986872305847690f389d6cf8478153a48b03a0 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Fri, 13 Aug 2021 13:17:39 +0530 Subject: [PATCH 03/13] Acknowledge review comments Changes default for starts_at Add validation for timezone remove redundant method --- ...4_add_starts_at_to_dast_profile_schedules.rb | 2 +- db/structure.sql | 15 +++++++-------- ee/app/models/dast/profile_schedule.rb | 16 +++++++--------- ee/spec/models/dast/profile_schedule_spec.rb | 17 +++++++---------- .../dast/profile_schedule_worker_spec.rb | 2 +- spec/lib/gitlab/ci/cron_parser_spec.rb | 8 ++++---- 6 files changed, 27 insertions(+), 33 deletions(-) diff --git a/db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb b/db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb index 32c8f506fe2144..4eea5fd7e8cc5a 100644 --- a/db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb +++ b/db/migrate/20210807102004_add_starts_at_to_dast_profile_schedules.rb @@ -2,6 +2,6 @@ class AddStartsAtToDastProfileSchedules < ActiveRecord::Migration[6.1] def change - add_column :dast_profile_schedules, :starts_at, :datetime_with_timezone, null: false, default: '' + add_column :dast_profile_schedules, :starts_at, :datetime_with_timezone, null: false, default: -> { 'NOW()' } end end diff --git a/db/structure.sql b/db/structure.sql index db576022d667d7..7e34079040ea5d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12066,7 +12066,7 @@ CREATE TABLE dast_profile_schedules ( cron text NOT NULL, cadence jsonb DEFAULT '{}'::jsonb NOT NULL, timezone text DEFAULT ''::text NOT NULL, - starts_at timestamp with time zone NOT NULL, + starts_at timestamp with time zone DEFAULT now() NOT NULL, CONSTRAINT check_86531ea73f CHECK ((char_length(cron) <= 255)), CONSTRAINT check_be4d1c3af1 CHECK ((char_length(timezone) <= 255)) ); @@ -12288,7 +12288,6 @@ CREATE TABLE dep_ci_build_trace_section_names ( ); CREATE SEQUENCE dep_ci_build_trace_section_names_id_seq - AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE @@ -26524,9 +26523,6 @@ ALTER TABLE ONLY alert_management_alerts ALTER TABLE ONLY identities ADD CONSTRAINT fk_aade90f0fc FOREIGN KEY (saml_provider_id) REFERENCES saml_providers(id) ON DELETE CASCADE; -ALTER TABLE ONLY dep_ci_build_trace_sections - ADD CONSTRAINT fk_ab7c104e26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY ci_sources_pipelines ADD CONSTRAINT fk_acd9737679 FOREIGN KEY (source_project_id) REFERENCES projects(id) ON DELETE CASCADE; @@ -26797,9 +26793,6 @@ ALTER TABLE ONLY cluster_agents ALTER TABLE ONLY protected_tag_create_access_levels ADD CONSTRAINT fk_f7dfda8c51 FOREIGN KEY (protected_tag_id) REFERENCES protected_tags(id) ON DELETE CASCADE; -ALTER TABLE ONLY dep_ci_build_trace_section_names - ADD CONSTRAINT fk_f8cd72cd26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY ci_stages ADD CONSTRAINT fk_fb57e6cc56 FOREIGN KEY (pipeline_id) REFERENCES ci_pipelines(id) ON DELETE CASCADE; @@ -27916,6 +27909,9 @@ ALTER TABLE ONLY merge_request_user_mentions ALTER TABLE ONLY x509_commit_signatures ADD CONSTRAINT fk_rails_ab07452314 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY dep_ci_build_trace_sections + ADD CONSTRAINT fk_rails_ab7c104e26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY resource_iteration_events ADD CONSTRAINT fk_rails_abf5d4affa FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE; @@ -28375,6 +28371,9 @@ ALTER TABLE ONLY issues_self_managed_prometheus_alert_events ALTER TABLE ONLY merge_requests_closing_issues ADD CONSTRAINT fk_rails_f8540692be FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE; +ALTER TABLE ONLY dep_ci_build_trace_section_names + ADD CONSTRAINT fk_rails_f8cd72cd26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY merge_trains ADD CONSTRAINT fk_rails_f90820cb08 FOREIGN KEY (pipeline_id) REFERENCES ci_pipelines(id) ON DELETE SET NULL; diff --git a/ee/app/models/dast/profile_schedule.rb b/ee/app/models/dast/profile_schedule.rb index c784546ef6119f..0ae0ff3bfe4b04 100644 --- a/ee/app/models/dast/profile_schedule.rb +++ b/ee/app/models/dast/profile_schedule.rb @@ -11,9 +11,10 @@ class Dast::ProfileSchedule < ApplicationRecord 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 - validates :timezone, presence: true, length: { maximum: 255 } + validates :timezone, presence: true, inclusion: { in: :timezones } validates :starts_at, presence: true validates :cadence, json_schema: { filename: 'dast_profile_schedule_cadence', draft: 7 } + validates :dast_profile_id, uniqueness: true serialize :cadence, Serializers::Json # rubocop:disable Cop/ActiveRecordSerialize @@ -22,9 +23,6 @@ class Dast::ProfileSchedule < ApplicationRecord scope :with_owner, -> { includes(:owner) } scope :active, -> { where(active: true) } - attribute :set_offset, :boolean, default: false - - before_save :set_offset_true!, if: :will_save_change_to_starts_at? before_save :set_cron, :set_next_run_at def repeat? @@ -60,12 +58,8 @@ def set_cron end end - def set_offset_true! - self.set_offset = true - end - def set_next_run_at - return super unless set_offset + return super unless will_save_change_to_starts_at? self.next_run_at = cron_worker_next_run_from(starts_at) end @@ -73,4 +67,8 @@ def set_next_run_at def worker_cron_expression Settings.cron_jobs['app_sec_dast_profile_schedule_worker']['cron'] end + + def timezones + @timezones ||= ActiveSupport::TimeZone.all.map { |tz| tz.tzinfo.identifier } + end end diff --git a/ee/spec/models/dast/profile_schedule_spec.rb b/ee/spec/models/dast/profile_schedule_spec.rb index 1345845fc3c053..91166efd21f948 100644 --- a/ee/spec/models/dast/profile_schedule_spec.rb +++ b/ee/spec/models/dast/profile_schedule_spec.rb @@ -12,11 +12,13 @@ end describe 'validations' do + let(:timezones) { ActiveSupport::TimeZone.all.map { |tz| tz.tzinfo.identifier } } + it { is_expected.to be_valid } it { is_expected.to validate_presence_of(:timezone) } - it { is_expected.to validate_length_of(:timezone).is_at_most(255) } - + it { is_expected.to validate_inclusion_of(:timezone).in_array(timezones) } it { is_expected.to validate_presence_of(:starts_at) } + it { is_expected.to validate_uniqueness_of(:dast_profile_id) } describe 'cadence' do context 'when valid values' do @@ -122,10 +124,6 @@ end end - describe 'attributes' do - it { is_expected.to have_attributes(set_offset: true) } - end - describe '#set_next_run_at' do let(:schedule) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }, starts_at: Time.zone.now + 1.minute) } let(:schedule_1) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }) } @@ -149,22 +147,21 @@ context 'when starts_at is updated for existing schedules' do it 'updates next_run_at automatically' do - expect { schedule.update!(starts_at: Time.zone.now + 2.days + 1.minute) }.to change { schedule.next_run_at } - expect(schedule.set_offset).to be true + expect { schedule.update!(starts_at: Time.zone.now + 2.days) }.to change { schedule.next_run_at } end end end describe '#schedule_next_run!' do context 'when repeat? is true' do - it 'does not set the active to false' do + it 'sets active to true' do subject.schedule_next_run! expect(subject.active).to be true end end context 'when repeat? is false' do - it 'does not set the active to false' do + it 'sets active to false' do subject.update_column(:cadence, {}) subject.schedule_next_run! expect(subject.active).to be false diff --git a/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb b/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb index 5ca7e8cd77786e..5051ca3e8a61f2 100644 --- a/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb +++ b/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb @@ -107,7 +107,7 @@ def record_preloaded_queries schedule.update_columns(next_run_at: 1.minute.ago, cadence: {}) end - it 'executes the rule schedule service and deactivate the schedule' do + it 'executes the rule schedule service and deactivate the schedule', :aggregate_failures do expect(schedule.repeat?).to be(false) subject expect(schedule.reload.active).to be(false) diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 0b5254932acfc7..26786bd9b7de83 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -307,7 +307,7 @@ let(:months) { Fugit::Cron.parse(cron_line).months } context 'when repeat cycle is day' do - it 'generates daily cron expression' do + it 'generates daily cron expression', :aggregate_failures do expect(hours).to include time.hour expect(minutes).to include time.min end @@ -316,7 +316,7 @@ context 'when repeat cycle is week' do let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'week', duration: 1 }) } - it 'generates weekly cron expression' do + it 'generates weekly cron expression', :aggregate_failures do expect(hours).to include time.hour expect(minutes).to include time.min expect(weekdays).to include time.wday @@ -326,7 +326,7 @@ context 'when repeat cycle is month' do let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'month', duration: 3 }) } - it 'generates monthly cron expression' do + it 'generates monthly cron expression', :aggregate_failures do expect(minutes).to include time.min expect(months).to include time.month end @@ -335,7 +335,7 @@ context 'when repeat cycle is year' do let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'year', duration: 1 }) } - it 'generates yearly cron expression' do + it 'generates yearly cron expression', :aggregate_failures do expect(hours).to include time.hour expect(minutes).to include time.min expect(months).to include time.month -- GitLab From bc6cd449c0d910c5fac39b55ac90b0de72aca6e7 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Mon, 16 Aug 2021 16:03:01 +0530 Subject: [PATCH 04/13] Add Unique index on dast_profile for schedule --- ...index_on_dast_profile_to_dast_profile_schedules.rb | 11 +++++++++++ db/schema_migrations/20210816095826 | 1 + db/structure.sql | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb create mode 100644 db/schema_migrations/20210816095826 diff --git a/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb b/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb new file mode 100644 index 00000000000000..d270f03e9b4bfd --- /dev/null +++ b/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddUniqueIndexOnDastProfileToDastProfileSchedules < ActiveRecord::Migration[6.1] + def change + remove_index :dast_profile_schedules, :dast_profile_id + add_index :dast_profile_schedules, :dast_profile_id, unique: true + end +end diff --git a/db/schema_migrations/20210816095826 b/db/schema_migrations/20210816095826 new file mode 100644 index 00000000000000..079a83fae8f538 --- /dev/null +++ b/db/schema_migrations/20210816095826 @@ -0,0 +1 @@ +d1ad234656f49861d2ca7694d23116e930bba597fca32b1015db698cc23bdc1c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 7e34079040ea5d..31fc300396b7cd 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -23615,7 +23615,7 @@ CREATE UNIQUE INDEX index_daily_build_group_report_results_unique_columns ON ci_ CREATE INDEX index_dast_profile_schedules_active_next_run_at ON dast_profile_schedules USING btree (active, next_run_at); -CREATE INDEX index_dast_profile_schedules_on_dast_profile_id ON dast_profile_schedules USING btree (dast_profile_id); +CREATE UNIQUE INDEX index_dast_profile_schedules_on_dast_profile_id ON dast_profile_schedules USING btree (dast_profile_id); CREATE UNIQUE INDEX index_dast_profile_schedules_on_project_id_and_dast_profile_id ON dast_profile_schedules USING btree (project_id, dast_profile_id); -- GitLab From b0df554eb969b1b691b33d7dcf202d2d9bdd8f80 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Mon, 16 Aug 2021 22:39:10 +0530 Subject: [PATCH 05/13] Add uniq index --- ...nique_index_on_dast_profile_to_dast_profile_schedules.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb b/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb index d270f03e9b4bfd..8f9f4df0609149 100644 --- a/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb +++ b/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb @@ -4,8 +4,12 @@ # for more information on how to write migrations for GitLab. class AddUniqueIndexOnDastProfileToDastProfileSchedules < ActiveRecord::Migration[6.1] + # We disable these cops here because changing this index is safe. The table does not + # have any data in it as it's behind a feature flag. + # rubocop: disable Migration/AddIndex + # rubocop: disable Migration/RemoveIndex def change - remove_index :dast_profile_schedules, :dast_profile_id + remove_index :dast_profile_schedules, :dast_profile_id, name: 'index_dast_profile_schedules_on_dast_profile_id' add_index :dast_profile_schedules, :dast_profile_id, unique: true end end -- GitLab From ceaba16319e772365762d9fc7765c45f52f42061 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Tue, 17 Aug 2021 12:18:46 +0530 Subject: [PATCH 06/13] Fixes spec and add check in migration --- .../20210807101621_add_timezone_to_dast_profile_schedules.rb | 5 ++++- ee/spec/models/dast/profile_schedule_spec.rb | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb b/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb index 3c0668d27b648a..f14a37925391ac 100644 --- a/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb +++ b/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb @@ -6,7 +6,10 @@ class AddTimezoneToDastProfileSchedules < ActiveRecord::Migration[6.1] disable_ddl_transaction! def up - add_column :dast_profile_schedules, :timezone, :text, null: false, default: '' + unless column_exists?(:dast_profile_schedules, :timezone) + add_column :dast_profile_schedules, :timezone, :text, null: false, default: '' + end + add_text_limit :dast_profile_schedules, :timezone, 255 end diff --git a/ee/spec/models/dast/profile_schedule_spec.rb b/ee/spec/models/dast/profile_schedule_spec.rb index 91166efd21f948..c7ff1f8d20688c 100644 --- a/ee/spec/models/dast/profile_schedule_spec.rb +++ b/ee/spec/models/dast/profile_schedule_spec.rb @@ -134,14 +134,14 @@ context 'when schedule runs every minute' do it "updates next_run_at to the worker's execution time" do travel_to(1.day.ago) do - expect(schedule.next_run_at).to eq(cron_worker_next_run_at) + expect(schedule.next_run_at.to_i).to eq(cron_worker_next_run_at.to_i) end end end context 'when there are two different schedules in the same time zones' do it 'sets the sames next_run_at' do - expect(schedule_1.next_run_at).to eq(schedule_2.next_run_at) + expect(schedule_1.next_run_at.to_i).to eq(schedule_2.next_run_at.to_i) end end -- GitLab From 876660d2db19c515fcd1d1a0bf085e433a3f9c6d Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 18 Aug 2021 15:38:18 +0530 Subject: [PATCH 07/13] Update migration and fixes spec --- ..._add_timezone_to_dast_profile_schedules.rb | 9 +++++- ..._dast_profile_to_dast_profile_schedules.rb | 29 ++++++++++++++++-- ...pound_index_from_dast_profile_schedules.rb | 30 +++++++++++++++++++ db/schema_migrations/20210818061156 | 1 + db/structure.sql | 4 +-- ee/spec/models/dast/profile_schedule_spec.rb | 2 +- lib/gitlab/ci/cron_parser.rb | 4 +++ spec/lib/gitlab/ci/cron_parser_spec.rb | 16 ++++++++++ 8 files changed, 87 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb create mode 100644 db/schema_migrations/20210818061156 diff --git a/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb b/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb index f14a37925391ac..3c3eb50743244d 100644 --- a/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb +++ b/db/migrate/20210807101621_add_timezone_to_dast_profile_schedules.rb @@ -5,15 +5,22 @@ class AddTimezoneToDastProfileSchedules < ActiveRecord::Migration[6.1] disable_ddl_transaction! + # We disable these cops here because adding the column is safe. The table does not + # have any data in it as it's behind a feature flag. + # rubocop: disable Rails/NotNullColumn def up + execute('DELETE FROM dast_profile_schedules') + unless column_exists?(:dast_profile_schedules, :timezone) - add_column :dast_profile_schedules, :timezone, :text, null: false, default: '' + add_column :dast_profile_schedules, :timezone, :text, null: false end add_text_limit :dast_profile_schedules, :timezone, 255 end def down + return unless column_exists?(:dast_profile_schedules, :timezone) + remove_column :dast_profile_schedules, :timezone end end diff --git a/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb b/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb index 8f9f4df0609149..b7ea8545df1847 100644 --- a/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb +++ b/db/migrate/20210816095826_add_unique_index_on_dast_profile_to_dast_profile_schedules.rb @@ -4,12 +4,35 @@ # for more information on how to write migrations for GitLab. class AddUniqueIndexOnDastProfileToDastProfileSchedules < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + INDEX_NAME = 'index_dast_profile_schedules_on_dast_profile_id' + TABLE = :dast_profile_schedules # We disable these cops here because changing this index is safe. The table does not # have any data in it as it's behind a feature flag. # rubocop: disable Migration/AddIndex # rubocop: disable Migration/RemoveIndex - def change - remove_index :dast_profile_schedules, :dast_profile_id, name: 'index_dast_profile_schedules_on_dast_profile_id' - add_index :dast_profile_schedules, :dast_profile_id, unique: true + def up + execute('DELETE FROM dast_profile_schedules') + + if index_exists_by_name?(TABLE, INDEX_NAME) + remove_index TABLE, :dast_profile_id, name: INDEX_NAME + end + + unless index_exists_by_name?(TABLE, INDEX_NAME) + add_index TABLE, :dast_profile_id, unique: true, name: INDEX_NAME + end + end + + def down + execute('DELETE FROM dast_profile_schedules') + + if index_exists_by_name?(TABLE, INDEX_NAME) + remove_index TABLE, :dast_profile_id, name: INDEX_NAME + end + + unless index_exists_by_name?(TABLE, INDEX_NAME) + add_index TABLE, :dast_profile_id + end end end diff --git a/db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb b/db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb new file mode 100644 index 00000000000000..8c07271a854167 --- /dev/null +++ b/db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveProjectProfileCompoundIndexFromDastProfileSchedules < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + TABLE = :dast_profile_schedules + INDEX_NAME = 'index_dast_profile_schedules_on_project_id_and_dast_profile_id' + # We disable these cops here because changing this index is safe. The table does not + # have any data in it as it's behind a feature flag. + # rubocop: disable Migration/AddIndex + # rubocop: disable Migration/RemoveIndex + def up + execute('DELETE FROM dast_profile_schedules') + + if index_exists_by_name?(TABLE, INDEX_NAME) + remove_index TABLE, %i[project_id dast_profile_id], name: INDEX_NAME + end + end + + def down + execute('DELETE FROM dast_profile_schedules') + + unless index_exists_by_name?(TABLE, INDEX_NAME) + add_index TABLE, %i[project_id dast_profile_id], name: INDEX_NAME + end + end +end diff --git a/db/schema_migrations/20210818061156 b/db/schema_migrations/20210818061156 new file mode 100644 index 00000000000000..fba5486b2a8ac9 --- /dev/null +++ b/db/schema_migrations/20210818061156 @@ -0,0 +1 @@ +23becdc9ad558882f4ce42e76391cdc2f760322a09c998082465fcb6d29dfeb5 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 31fc300396b7cd..5503ce33932dfd 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12065,7 +12065,7 @@ CREATE TABLE dast_profile_schedules ( active boolean DEFAULT true NOT NULL, cron text NOT NULL, cadence jsonb DEFAULT '{}'::jsonb NOT NULL, - timezone text DEFAULT ''::text NOT NULL, + timezone text NOT NULL, starts_at timestamp with time zone DEFAULT now() NOT NULL, CONSTRAINT check_86531ea73f CHECK ((char_length(cron) <= 255)), CONSTRAINT check_be4d1c3af1 CHECK ((char_length(timezone) <= 255)) @@ -23617,8 +23617,6 @@ CREATE INDEX index_dast_profile_schedules_active_next_run_at ON dast_profile_sch CREATE UNIQUE INDEX index_dast_profile_schedules_on_dast_profile_id ON dast_profile_schedules USING btree (dast_profile_id); -CREATE UNIQUE INDEX index_dast_profile_schedules_on_project_id_and_dast_profile_id ON dast_profile_schedules USING btree (project_id, dast_profile_id); - CREATE INDEX index_dast_profile_schedules_on_user_id ON dast_profile_schedules USING btree (user_id); CREATE INDEX index_dast_profiles_on_dast_scanner_profile_id ON dast_profiles USING btree (dast_scanner_profile_id); diff --git a/ee/spec/models/dast/profile_schedule_spec.rb b/ee/spec/models/dast/profile_schedule_spec.rb index c7ff1f8d20688c..582b0d0e8ac04f 100644 --- a/ee/spec/models/dast/profile_schedule_spec.rb +++ b/ee/spec/models/dast/profile_schedule_spec.rb @@ -125,7 +125,7 @@ end describe '#set_next_run_at' do - let(:schedule) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }, starts_at: Time.zone.now + 1.minute) } + let(:schedule) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }, starts_at: Time.zone.now) } let(:schedule_1) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }) } let(:schedule_2) { create(:dast_profile_schedule, cadence: { unit: 'day', duration: 1 }) } diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index a54f239ba1ef08..56c837d40b9ecc 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -25,6 +25,10 @@ def parse_natural_with_timestamp(starts_at, cadence) when 'week' # Currently supports only 'every 1 week'. "#{starts_at.min} #{starts_at.hour} * * #{starts_at.wday}" when 'month' + unless [3, 6, 12].include?(cadence[:duration]) + raise NotImplementedError, "The cadence #{cadence} is not supported" + end + "#{starts_at.min} #{starts_at.hour} #{starts_at.mday} #{fallin_months(cadence[:duration], starts_at)} *" when 'year' # Currently supports only 'every 1 year'. "#{starts_at.min} #{starts_at.hour} #{starts_at.mday} #{starts_at.month} *" diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 26786bd9b7de83..cef01e93542056 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -330,6 +330,14 @@ expect(minutes).to include time.min expect(months).to include time.month end + + context 'when an unsupported duration is specified' do + subject { described_class.parse_natural_with_timestamp(time, { unit: 'month', duration: 7 }) } + + it 'raises an exception' do + expect { subject }.to raise_error(NotImplementedError, 'The cadence {:unit=>"month", :duration=>7} is not supported') + end + end end context 'when repeat cycle is year' do @@ -341,5 +349,13 @@ expect(months).to include time.month end end + + context 'when the repeat cycle is not implemented' do + subject { described_class.parse_natural_with_timestamp(time, { unit: 'quarterly', duration: 1 }) } + + it 'raises an exception' do + expect { subject }.to raise_error(NotImplementedError, 'The cadence unit quarterly is not implemented') + end + end end end -- GitLab From 89a271bad91f27d56e79bf726bccfa7f39cef1cb Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 18 Aug 2021 17:32:18 +0530 Subject: [PATCH 08/13] Add project_id index --- ...add_index_project_id_on_dast_profile_schedule.rb | 13 +++++++++++++ db/schema_migrations/20210818115613 | 1 + db/structure.sql | 2 ++ 3 files changed, 16 insertions(+) create mode 100644 db/migrate/20210818115613_add_index_project_id_on_dast_profile_schedule.rb create mode 100644 db/schema_migrations/20210818115613 diff --git a/db/migrate/20210818115613_add_index_project_id_on_dast_profile_schedule.rb b/db/migrate/20210818115613_add_index_project_id_on_dast_profile_schedule.rb new file mode 100644 index 00000000000000..392b335ab451c5 --- /dev/null +++ b/db/migrate/20210818115613_add_index_project_id_on_dast_profile_schedule.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexProjectIdOnDastProfileSchedule < ActiveRecord::Migration[6.1] + # We disable these cops here because changing this index is safe. The table does not + # have any data in it as it's behind a feature flag. + # rubocop: disable Migration/AddIndex + def change + add_index :dast_profile_schedules, :project_id + end +end diff --git a/db/schema_migrations/20210818115613 b/db/schema_migrations/20210818115613 new file mode 100644 index 00000000000000..efe76d3a46a2c7 --- /dev/null +++ b/db/schema_migrations/20210818115613 @@ -0,0 +1 @@ +9c5114dac05e90c15567bb3274f20f03a82f9e4d73d5c72d89c26bc9d742cc35 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 5503ce33932dfd..bb8648bdb330e4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -23617,6 +23617,8 @@ CREATE INDEX index_dast_profile_schedules_active_next_run_at ON dast_profile_sch CREATE UNIQUE INDEX index_dast_profile_schedules_on_dast_profile_id ON dast_profile_schedules USING btree (dast_profile_id); +CREATE INDEX index_dast_profile_schedules_on_project_id ON dast_profile_schedules USING btree (project_id); + CREATE INDEX index_dast_profile_schedules_on_user_id ON dast_profile_schedules USING btree (user_id); CREATE INDEX index_dast_profiles_on_dast_scanner_profile_id ON dast_profiles USING btree (dast_scanner_profile_id); -- GitLab From 5da3100d60db45a33d617a0cb3156aeeb4552849 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Thu, 19 Aug 2021 10:36:19 +0530 Subject: [PATCH 09/13] Add unique true in down migration --- ...roject_profile_compound_index_from_dast_profile_schedules.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb b/db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb index 8c07271a854167..b50947a0a99942 100644 --- a/db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb +++ b/db/migrate/20210818061156_remove_project_profile_compound_index_from_dast_profile_schedules.rb @@ -24,7 +24,7 @@ def down execute('DELETE FROM dast_profile_schedules') unless index_exists_by_name?(TABLE, INDEX_NAME) - add_index TABLE, %i[project_id dast_profile_id], name: INDEX_NAME + add_index TABLE, %i[project_id dast_profile_id], unique: true, name: INDEX_NAME end end end -- GitLab From a8c58c7a05263b1db3c4eb503c7a317f17c780cc Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Thu, 19 Aug 2021 11:54:19 +0530 Subject: [PATCH 10/13] Add month in parser check --- lib/gitlab/ci/cron_parser.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index 56c837d40b9ecc..a2ac9d723469fe 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -25,7 +25,7 @@ def parse_natural_with_timestamp(starts_at, cadence) when 'week' # Currently supports only 'every 1 week'. "#{starts_at.min} #{starts_at.hour} * * #{starts_at.wday}" when 'month' - unless [3, 6, 12].include?(cadence[:duration]) + unless [1, 3, 6, 12].include?(cadence[:duration]) raise NotImplementedError, "The cadence #{cadence} is not supported" end -- GitLab From df447fcade3ef207010f257fe403529a2c8e4515 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Thu, 19 Aug 2021 13:58:11 +0530 Subject: [PATCH 11/13] Fixes structure.sql --- db/structure.sql | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/db/structure.sql b/db/structure.sql index bb8648bdb330e4..0b1a0c1b33e9c8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12288,6 +12288,7 @@ CREATE TABLE dep_ci_build_trace_section_names ( ); CREATE SEQUENCE dep_ci_build_trace_section_names_id_seq + AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE @@ -26523,6 +26524,9 @@ ALTER TABLE ONLY alert_management_alerts ALTER TABLE ONLY identities ADD CONSTRAINT fk_aade90f0fc FOREIGN KEY (saml_provider_id) REFERENCES saml_providers(id) ON DELETE CASCADE; +ALTER TABLE ONLY dep_ci_build_trace_sections + ADD CONSTRAINT fk_ab7c104e26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY ci_sources_pipelines ADD CONSTRAINT fk_acd9737679 FOREIGN KEY (source_project_id) REFERENCES projects(id) ON DELETE CASCADE; @@ -26793,6 +26797,9 @@ ALTER TABLE ONLY cluster_agents ALTER TABLE ONLY protected_tag_create_access_levels ADD CONSTRAINT fk_f7dfda8c51 FOREIGN KEY (protected_tag_id) REFERENCES protected_tags(id) ON DELETE CASCADE; +ALTER TABLE ONLY dep_ci_build_trace_section_names + ADD CONSTRAINT fk_f8cd72cd26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY ci_stages ADD CONSTRAINT fk_fb57e6cc56 FOREIGN KEY (pipeline_id) REFERENCES ci_pipelines(id) ON DELETE CASCADE; @@ -27909,9 +27916,6 @@ ALTER TABLE ONLY merge_request_user_mentions ALTER TABLE ONLY x509_commit_signatures ADD CONSTRAINT fk_rails_ab07452314 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; -ALTER TABLE ONLY dep_ci_build_trace_sections - ADD CONSTRAINT fk_rails_ab7c104e26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY resource_iteration_events ADD CONSTRAINT fk_rails_abf5d4affa FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE; @@ -28371,9 +28375,6 @@ ALTER TABLE ONLY issues_self_managed_prometheus_alert_events ALTER TABLE ONLY merge_requests_closing_issues ADD CONSTRAINT fk_rails_f8540692be FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE; -ALTER TABLE ONLY dep_ci_build_trace_section_names - ADD CONSTRAINT fk_rails_f8cd72cd26 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY merge_trains ADD CONSTRAINT fk_rails_f90820cb08 FOREIGN KEY (pipeline_id) REFERENCES ci_pipelines(id) ON DELETE SET NULL; -- GitLab From c2eff9c21c6b09ee9c74de7a9c27736a7da70db5 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Mon, 30 Aug 2021 12:13:28 +0530 Subject: [PATCH 12/13] Fixes review comments --- ee/spec/models/dast/profile_schedule_spec.rb | 22 +++++++++++++++++-- .../dast/profile_schedule_worker_spec.rb | 2 ++ lib/gitlab/ci/cron_parser.rb | 4 ++-- spec/lib/gitlab/ci/cron_parser_spec.rb | 2 +- 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/ee/spec/models/dast/profile_schedule_spec.rb b/ee/spec/models/dast/profile_schedule_spec.rb index 582b0d0e8ac04f..90513cb2509495 100644 --- a/ee/spec/models/dast/profile_schedule_spec.rb +++ b/ee/spec/models/dast/profile_schedule_spec.rb @@ -33,6 +33,7 @@ ].each do |cadence| it "allows #{cadence[:unit]} values" do schedule = build(:dast_profile_schedule, cadence: cadence) + expect(schedule.cadence).to eq(cadence.stringify_keys) end end @@ -118,8 +119,22 @@ describe 'before_save' do describe '#set_cron' do - it 'sets the cron value' do - expect(subject.cron).to be_present + context 'when repeat? is true' do + it 'sets the cron value' do + freeze_time do + cron_statement = Gitlab::Ci::CronParser.parse_natural_with_timestamp(subject.starts_at, subject.cadence) + + expect(subject.cron).to eq cron_statement + end + end + end + + context 'when repeat? is false' do + subject { create(:dast_profile_schedule, cadence: {}) } + + it 'sets the cron value to default when non repeating' do + expect(subject.cron).to eq Dast::ProfileSchedule::CRON_DEFAULT + end end end end @@ -156,6 +171,7 @@ context 'when repeat? is true' do it 'sets active to true' do subject.schedule_next_run! + expect(subject.active).to be true end end @@ -163,7 +179,9 @@ context 'when repeat? is false' do it 'sets active to false' do subject.update_column(:cadence, {}) + subject.schedule_next_run! + expect(subject.active).to be false end end diff --git a/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb b/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb index 5051ca3e8a61f2..84fb16c477f644 100644 --- a/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb +++ b/ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb @@ -109,7 +109,9 @@ def record_preloaded_queries it 'executes the rule schedule service and deactivate the schedule', :aggregate_failures do expect(schedule.repeat?).to be(false) + subject + expect(schedule.reload.active).to be(false) end end diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index a2ac9d723469fe..7334a112ccf235 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -29,7 +29,7 @@ def parse_natural_with_timestamp(starts_at, cadence) raise NotImplementedError, "The cadence #{cadence} is not supported" end - "#{starts_at.min} #{starts_at.hour} #{starts_at.mday} #{fallin_months(cadence[:duration], starts_at)} *" + "#{starts_at.min} #{starts_at.hour} #{starts_at.mday} #{fall_in_months(cadence[:duration], starts_at)} *" when 'year' # Currently supports only 'every 1 year'. "#{starts_at.min} #{starts_at.hour} #{starts_at.mday} #{starts_at.month} *" else @@ -37,7 +37,7 @@ def parse_natural_with_timestamp(starts_at, cadence) end end - def fallin_months(offset, start_date) + def fall_in_months(offset, start_date) (1..(12 / offset)).map { |i| start_date.next_month(offset * i).month }.join(',') end end diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index cef01e93542056..4017accb462069 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -300,7 +300,7 @@ describe '.parse_natural', :aggregate_failures do let(:cron_line) { described_class.parse_natural_with_timestamp(time, { unit: 'day', duration: 1 }) } - let(:time) { Time.zone.now } + let(:time) { Time.parse('Mon, 30 Aug 2021 06:29:44.067132000 UTC +00:00') } let(:hours) { Fugit::Cron.parse(cron_line).hours } let(:minutes) { Fugit::Cron.parse(cron_line).minutes } let(:weekdays) { Fugit::Cron.parse(cron_line).weekdays.first } -- GitLab From 23b674b59e50da340c30b14a8c11b936d3b62e8e Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Mon, 30 Aug 2021 15:01:32 +0530 Subject: [PATCH 13/13] Fixes spec --- ee/app/models/dast/profile_schedule.rb | 2 +- ee/spec/models/dast/profile_schedule_spec.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/ee/app/models/dast/profile_schedule.rb b/ee/app/models/dast/profile_schedule.rb index 0ae0ff3bfe4b04..25b23ae5ead7db 100644 --- a/ee/app/models/dast/profile_schedule.rb +++ b/ee/app/models/dast/profile_schedule.rb @@ -26,7 +26,7 @@ class Dast::ProfileSchedule < ApplicationRecord before_save :set_cron, :set_next_run_at def repeat? - self.cadence != {} + cadence.present? end def schedule_next_run! diff --git a/ee/spec/models/dast/profile_schedule_spec.rb b/ee/spec/models/dast/profile_schedule_spec.rb index 90513cb2509495..a0562c3810c110 100644 --- a/ee/spec/models/dast/profile_schedule_spec.rb +++ b/ee/spec/models/dast/profile_schedule_spec.rb @@ -34,6 +34,7 @@ it "allows #{cadence[:unit]} values" do schedule = build(:dast_profile_schedule, cadence: cadence) + expect(schedule).to be_valid expect(schedule.cadence).to eq(cadence.stringify_keys) end end -- GitLab