From 4f2b8e1de147a2bcdde10e2e05df9da4d3caacbb Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Wed, 27 Nov 2024 12:54:03 +0100 Subject: [PATCH 01/24] Add worker architecture for scheduled PEP This adds the worker architecture to run scheduled pipelines based on security policies. Changelog: added # Conflicts: # db/structure.sql --- config/initializers/1_settings.rb | 3 + config/sidekiq_queues.yml | 2 + ...y_pipeline_execution_project_schedules.yml | 13 +++ ...e_security_pipeline_execution_schedules.rb | 35 +++++++ ...o_security_pipeline_execution_schedules.rb | 20 ++++ ...o_security_pipeline_execution_schedules.rb | 19 ++++ db/schema_migrations/20241210194442 | 1 + db/schema_migrations/20241210194457 | 1 + db/schema_migrations/20241210194529 | 1 + db/structure.sql | 47 ++++++++++ .../pipeline_execution_project_schedule.rb | 46 +++++++++ ee/app/models/security/policy.rb | 19 ++++ ee/app/workers/all_queues.yml | 18 ++++ .../run_schedule_worker.rb | 18 ++++ .../schedule_worker.rb | 34 +++++++ .../pipeline_execution_project_schedules.rb | 13 +++ ee/spec/factories/security/policies.rb | 10 ++ ...ipeline_execution_project_schedule_spec.rb | 93 +++++++++++++++++++ ee/spec/models/security/policy_spec.rb | 42 +++++++++ .../schedule_worker_spec.rb | 53 +++++++++++ locale/gitlab.pot | 3 + 21 files changed, 491 insertions(+) create mode 100644 db/docs/security_pipeline_execution_project_schedules.yml create mode 100644 db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb create mode 100644 db/migrate/20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb create mode 100644 db/migrate/20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb create mode 100644 db/schema_migrations/20241210194442 create mode 100644 db/schema_migrations/20241210194457 create mode 100644 db/schema_migrations/20241210194529 create mode 100644 ee/app/models/security/pipeline_execution_project_schedule.rb create mode 100644 ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb create mode 100644 ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb create mode 100644 ee/spec/factories/security/pipeline_execution_project_schedules.rb create mode 100644 ee/spec/models/security/pipeline_execution_project_schedule_spec.rb create mode 100644 ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 6995e801ba433f..8360f2e461fa1d 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -880,6 +880,9 @@ Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker'] ||= {} Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker']['cron'] ||= '*/15 * * * *' Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker']['job_class'] = 'Security::OrchestrationPolicyRuleScheduleWorker' + Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker'] ||= {} + Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['cron'] ||= '*/15 * * * *' + Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['job_class'] = 'Security::PipelineExecutionPolicies::ScheduleWorker' Settings.cron_jobs['security_scans_purge_worker'] ||= {} Settings.cron_jobs['security_scans_purge_worker']['cron'] ||= '0 */4 * * 6,0' Settings.cron_jobs['security_scans_purge_worker']['job_class'] = 'Security::Scans::PurgeWorker' diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 77253d070bf8ee..a5605da78f6ad2 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -827,6 +827,8 @@ - 1 - - security_persist_security_policies - 1 +- - security_pipeline_execution_policies_run_schedule + - 1 - - security_process_scan_result_policy - 1 - - security_refresh_compliance_framework_security_policies diff --git a/db/docs/security_pipeline_execution_project_schedules.yml b/db/docs/security_pipeline_execution_project_schedules.yml new file mode 100644 index 00000000000000..8daeb229986f9f --- /dev/null +++ b/db/docs/security_pipeline_execution_project_schedules.yml @@ -0,0 +1,13 @@ +--- +table_name: security_pipeline_execution_project_schedules +classes: +- Security::PipelineExecutionProjectSchedule +feature_categories: +- security_policy_management +description: Stores schedule executions for pipeline execution schedule policies +introduced_by_url: +milestone: '17.7' +gitlab_schema: gitlab_main_cell +sharding_key: + project_id: projects +table_size: small diff --git a/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb b/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb new file mode 100644 index 00000000000000..cfb3cf783c9db1 --- /dev/null +++ b/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class CreateSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] + milestone '17.7' + + def change + create_table :security_pipeline_execution_project_schedules do |t| # rubocop:disable Migration/EnsureFactoryForTable -- False positive, factory name is prefixed with security + t.timestamps_with_timezone null: false + t.datetime_with_timezone :next_run_at, null: false + t.bigint :security_policy_id, null: false + t.bigint :project_id, null: false + t.text :cron, null: false, limit: 255 + t.text :timezone, limit: 128 + end + + add_index( + :security_pipeline_execution_project_schedules, + :next_run_at, + name: 'idx_security_pipeline_execution_project_schedules_next_run_at' + ) + + add_index( + :security_pipeline_execution_project_schedules, + :security_policy_id, + name: 'idx_pipeline_execution_schedules_security_policy_id' + ) + + add_index( + :security_pipeline_execution_project_schedules, + [:project_id, :security_policy_id], + unique: true, + name: 'uniq_idx_pipeline_execution_schedules_projects_and_policies' + ) + end +end diff --git a/db/migrate/20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb b/db/migrate/20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb new file mode 100644 index 00000000000000..e321390eb73796 --- /dev/null +++ b/db/migrate/20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddSecurityPolicyIdToSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] + milestone '17.7' + disable_ddl_transaction! + + def up + add_concurrent_foreign_key( + :security_pipeline_execution_project_schedules, + :security_policies, + column: :security_policy_id, + on_delete: :cascade + ) + end + + def down + remove_foreign_key_if_exists :security_pipeline_execution_project_schedules, :security_policies, + column: :security_policy_id + end +end diff --git a/db/migrate/20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb b/db/migrate/20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb new file mode 100644 index 00000000000000..79ab6466f11960 --- /dev/null +++ b/db/migrate/20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddProjectsForeignKeyToSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] + milestone '17.7' + disable_ddl_transaction! + + def up + add_concurrent_foreign_key( + :security_pipeline_execution_project_schedules, + :projects, + column: :project_id, + on_delete: :cascade + ) + end + + def down + remove_foreign_key_if_exists :security_pipeline_execution_project_schedules, column: :project_id + end +end diff --git a/db/schema_migrations/20241210194442 b/db/schema_migrations/20241210194442 new file mode 100644 index 00000000000000..fcac81622651eb --- /dev/null +++ b/db/schema_migrations/20241210194442 @@ -0,0 +1 @@ +fe65dda0c86220ada8703a1095a4672ed08e0a55590e1b7a7837550ed6026889 \ No newline at end of file diff --git a/db/schema_migrations/20241210194457 b/db/schema_migrations/20241210194457 new file mode 100644 index 00000000000000..8f7b1f1c16c495 --- /dev/null +++ b/db/schema_migrations/20241210194457 @@ -0,0 +1 @@ +e619d3df566c5dd7a1c64f696e7287e2dcb6f5c6d67eb1ec6b45b2013563417a \ No newline at end of file diff --git a/db/schema_migrations/20241210194529 b/db/schema_migrations/20241210194529 new file mode 100644 index 00000000000000..38344d54928ce4 --- /dev/null +++ b/db/schema_migrations/20241210194529 @@ -0,0 +1 @@ +899ce3631eb5e5925ee043c4f79d211f35b84fe396505e1f095aff0b90daef77 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index ddb22289e7bd50..5f2e3fe18ad02c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20013,6 +20013,28 @@ CREATE SEQUENCE security_orchestration_policy_rule_schedules_id_seq ALTER SEQUENCE security_orchestration_policy_rule_schedules_id_seq OWNED BY security_orchestration_policy_rule_schedules.id; +CREATE TABLE security_pipeline_execution_project_schedules ( + id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + next_run_at timestamp with time zone NOT NULL, + security_policy_id bigint NOT NULL, + project_id bigint NOT NULL, + cron text NOT NULL, + timezone text, + CONSTRAINT check_bbbe4b1b8d CHECK ((char_length(cron) <= 255)), + CONSTRAINT check_bf13eb178a CHECK ((char_length(timezone) <= 128)) +); + +CREATE SEQUENCE security_pipeline_execution_project_schedules_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE security_pipeline_execution_project_schedules_id_seq OWNED BY security_pipeline_execution_project_schedules.id; + CREATE TABLE security_policies ( id bigint NOT NULL, security_orchestration_policy_configuration_id bigint NOT NULL, @@ -24545,6 +24567,8 @@ ALTER TABLE ONLY security_orchestration_policy_configurations ALTER COLUMN id SE ALTER TABLE ONLY security_orchestration_policy_rule_schedules ALTER COLUMN id SET DEFAULT nextval('security_orchestration_policy_rule_schedules_id_seq'::regclass); +ALTER TABLE ONLY security_pipeline_execution_project_schedules ALTER COLUMN id SET DEFAULT nextval('security_pipeline_execution_project_schedules_id_seq'::regclass); + ALTER TABLE ONLY security_policies ALTER COLUMN id SET DEFAULT nextval('security_policies_id_seq'::regclass); ALTER TABLE ONLY security_policy_project_links ALTER COLUMN id SET DEFAULT nextval('security_policy_project_links_id_seq'::regclass); @@ -27296,6 +27320,9 @@ ALTER TABLE ONLY security_orchestration_policy_configurations ALTER TABLE ONLY security_orchestration_policy_rule_schedules ADD CONSTRAINT security_orchestration_policy_rule_schedules_pkey PRIMARY KEY (id); +ALTER TABLE ONLY security_pipeline_execution_project_schedules + ADD CONSTRAINT security_pipeline_execution_project_schedules_pkey PRIMARY KEY (id); + ALTER TABLE ONLY security_policies ADD CONSTRAINT security_policies_pkey PRIMARY KEY (id); @@ -29331,6 +29358,8 @@ CREATE INDEX idx_packages_packages_on_project_id_name_version_package_type ON pa CREATE INDEX idx_personal_access_tokens_on_previous_personal_access_token_id ON personal_access_tokens USING btree (previous_personal_access_token_id); +CREATE INDEX idx_pipeline_execution_schedules_security_policy_id ON security_pipeline_execution_project_schedules USING btree (security_policy_id); + CREATE INDEX idx_pkgs_conan_file_metadata_on_pkg_file_id_when_recipe_file ON packages_conan_file_metadata USING btree (package_file_id) WHERE (conan_file_type = 1); CREATE INDEX idx_pkgs_debian_group_distribution_keys_on_distribution_id ON packages_debian_group_distribution_keys USING btree (distribution_id); @@ -29399,6 +29428,8 @@ CREATE INDEX idx_scan_result_policies_on_configuration_id_id_updated_at ON scan_ CREATE INDEX idx_scan_result_policy_violations_on_policy_id_and_id ON scan_result_policy_violations USING btree (scan_result_policy_id, id); +CREATE INDEX idx_security_pipeline_execution_project_schedules_next_run_at ON security_pipeline_execution_project_schedules USING btree (next_run_at); + CREATE INDEX idx_security_policies_config_id_policy_index ON security_policies USING btree (security_orchestration_policy_configuration_id, policy_index); CREATE UNIQUE INDEX idx_security_scans_on_build_and_scan_type ON security_scans USING btree (build_id, scan_type); @@ -32969,6 +33000,8 @@ CREATE INDEX index_security_orchestration_policy_rule_schedules_on_namespace ON CREATE INDEX index_security_orchestration_policy_rule_schedules_on_project_i ON security_orchestration_policy_rule_schedules USING btree (project_id); +CREATE INDEX index_security_pipeline_execution_schedules_on_next_run_at ON security_pipeline_execution_schedules USING btree (next_run_at); + CREATE INDEX index_security_policies_on_policy_management_project_id ON security_policies USING btree (security_policy_management_project_id); CREATE UNIQUE INDEX index_security_policies_on_unique_config_type_policy_index ON security_policies USING btree (security_orchestration_policy_configuration_id, type, policy_index); @@ -34061,6 +34094,8 @@ CREATE UNIQUE INDEX uniq_idx_on_packages_conan_package_revisions_revision ON pac CREATE UNIQUE INDEX uniq_idx_packages_packages_on_project_id_name_version_ml_model ON packages_packages USING btree (project_id, name, version) WHERE ((package_type = 14) AND (status <> 4)); +CREATE UNIQUE INDEX uniq_idx_pipeline_execution_schedules_projects_and_policies ON security_pipeline_execution_project_schedules USING btree (project_id, security_policy_id); + CREATE UNIQUE INDEX uniq_idx_project_compliance_framework_on_project_framework ON project_compliance_framework_settings USING btree (project_id, framework_id); CREATE UNIQUE INDEX uniq_idx_provision_syncs_on_namespace_id_and_sync_requested_at ON subscription_provision_syncs USING btree (namespace_id, sync_requested_at DESC); @@ -36604,6 +36639,9 @@ ALTER TABLE ONLY namespace_settings ALTER TABLE ONLY packages_nuget_metadata ADD CONSTRAINT fk_21569c0856 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY security_pipeline_execution_project_schedules + ADD CONSTRAINT fk_21a3dca413 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE p_ci_build_trace_metadata ADD CONSTRAINT fk_21d25cac1a_p FOREIGN KEY (partition_id, trace_artifact_id) REFERENCES p_ci_job_artifacts(partition_id, id) ON UPDATE CASCADE ON DELETE CASCADE; @@ -36784,6 +36822,9 @@ ALTER TABLE ONLY approval_group_rules_users ALTER TABLE ONLY bulk_import_exports ADD CONSTRAINT fk_39c726d3b5 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY security_pipeline_execution_schedules + ADD CONSTRAINT fk_39de1c09cd FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY ml_model_versions ADD CONSTRAINT fk_39f8aa0b8a FOREIGN KEY (package_id) REFERENCES packages_packages(id) ON DELETE SET NULL; @@ -37372,6 +37413,9 @@ ALTER TABLE ONLY dependency_proxy_blob_states ALTER TABLE ONLY subscription_provision_syncs ADD CONSTRAINT fk_964dc9cc6c FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE; +ALTER TABLE ONLY security_pipeline_execution_schedules + ADD CONSTRAINT fk_9625df5e82 FOREIGN KEY (security_policy_id) REFERENCES security_policies(id) ON DELETE CASCADE; + ALTER TABLE ONLY issues ADD CONSTRAINT fk_96b1dd429c FOREIGN KEY (milestone_id) REFERENCES milestones(id) ON DELETE SET NULL; @@ -37498,6 +37542,9 @@ ALTER TABLE ONLY merge_request_merge_schedules ALTER TABLE ONLY merge_requests ADD CONSTRAINT fk_a6963e8447 FOREIGN KEY (target_project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY security_pipeline_execution_project_schedules + ADD CONSTRAINT fk_a766128d99 FOREIGN KEY (security_policy_id) REFERENCES security_policies(id) ON DELETE CASCADE; + ALTER TABLE ONLY merge_requests_closing_issues ADD CONSTRAINT fk_a8703820ae FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb new file mode 100644 index 00000000000000..10e37fbe35008c --- /dev/null +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Security + class PipelineExecutionProjectSchedule < ApplicationRecord + include Schedulable + include CronSchedulable + + self.table_name = 'security_pipeline_execution_project_schedules' + + belongs_to :project + belongs_to :security_policy, class_name: 'Security::Policy' + + validates :security_policy, uniqueness: { scope: :project_id } + validates :security_policy, :project, presence: true + validates :cron, cron: true, presence: true + validate :security_policy_is_pipeline_execution_schedule_policy + + scope :for_project, ->(project) { where(project: project) } + scope :runnable, -> { where(next_run_at: ...Time.zone.now) } + + def cron_timezone + # ActiveSupport::TimeZone.new will return nil if timezone is invalid + # https://github.com/rails/rails/blob/dd8f7185faeca6ee968a6e9367f6d8601a83b8db/activesupport/lib/active_support/values/time_zone.rb#L309-L312 + + ActiveSupport::TimeZone.new(timezone)&.name || Time.zone.name + rescue ArgumentError + # In case self.timezone is nil + Time.zone.name + end + + private + + def worker_cron_expression + Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['cron'] + end + + def security_policy_is_pipeline_execution_schedule_policy + return unless security_policy + + return if security_policy&.type_pipeline_execution_schedule_policy? + + errors.add(:security_policy, + _("Security policy must be of type pipeline_execution_schedule_policy")) + end + end +end diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index f0ab672099e93e..9c642ca5519e96 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -31,6 +31,9 @@ class Policy < ApplicationRecord has_many :projects, through: :security_policy_project_links + has_many :security_pipeline_execution_project_schedules, class_name: 'Security::PipelineExecutionProjectSchedule', + foreign_key: :security_policy_id, inverse_of: :security_policy + enum type: { approval_policy: 0, scan_execution_policy: 1, @@ -104,12 +107,20 @@ def link_project!(project) transaction do security_policy_project_links.for_project(project).first_or_create! link_policy_rules_project!(project) + + if type_pipeline_execution_schedule_policy? + security_pipeline_execution_project_schedules.for_project(project).first_or_create!( + cron: schedule_cadence, + timezone: schedule_timezone + ) + end end end def unlink_project!(project) transaction do security_policy_project_links.for_project(project).delete_all + security_pipeline_execution_project_schedules.for_project(project).delete_all unlink_policy_rules_project!(project) end end @@ -230,6 +241,14 @@ def edit_path private + def schedule_cadence + content.dig('schedule', 'cadence') + end + + def schedule_timezone + content.dig('schedule', 'timezone') + end + def link_policy_rules_project!(project, policy_rules = approval_policy_rules.undeleted) return if !type_approval_policy? || policy_rules.empty? diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index db0cc3435a9362..e704f710654225 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -660,6 +660,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: cronjob:security_pipeline_execution_policies_schedule + :worker_name: Security::PipelineExecutionPolicies::ScheduleWorker + :feature_category: :security_policy_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: cronjob:security_policies_report_security_policies_metrics :worker_name: Security::Policies::ReportSecurityPoliciesMetricsWorker :feature_category: :security_policy_management @@ -2595,6 +2604,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: security_pipeline_execution_policies_run_schedule + :worker_name: Security::PipelineExecutionPolicies::RunScheduleWorker + :feature_category: :security_policy_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: security_process_scan_result_policy :worker_name: Security::ProcessScanResultPolicyWorker :feature_category: :security_policy_management diff --git a/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb new file mode 100644 index 00000000000000..0f561f6c79da0c --- /dev/null +++ b/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Security + module PipelineExecutionPolicies + class RunScheduleWorker + include ApplicationWorker + + idempotent! + + data_consistency :sticky + feature_category :security_policy_management + + def perform(schedule_id) + # no-op + end + end + end +end diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb new file mode 100644 index 00000000000000..d1f7e0d572f0fe --- /dev/null +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Security + module PipelineExecutionPolicies + class ScheduleWorker + include ApplicationWorker + include CronjobQueue + include Security::SecurityOrchestrationPolicies::CadenceChecker + + idempotent! + + data_consistency :sticky + feature_category :security_policy_management + + def perform + Security::PipelineExecutionProjectSchedule.runnable.find_in_batches do |schedules| + schedules.each do |schedule| + unless valid_cadence?(schedule.cron) + log_invalid_cadence_error(schedule.project_id, schedule.cron) + + break + end + + schedule.schedule_next_run! + + with_context(project: schedule.project) do + Security::PipelineExecutionPolicies::RunScheduleWorker.perform_async(schedule.id) + end + end + end + end + end + end +end diff --git a/ee/spec/factories/security/pipeline_execution_project_schedules.rb b/ee/spec/factories/security/pipeline_execution_project_schedules.rb new file mode 100644 index 00000000000000..176fb1f502bfb4 --- /dev/null +++ b/ee/spec/factories/security/pipeline_execution_project_schedules.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :security_pipeline_execution_project_schedule, class: 'Security::PipelineExecutionProjectSchedule' do + project + + security_policy do + association(:security_policy, :pipeline_execution_schedule_policy) + end + + cron { '0 0 * * *' } + end +end diff --git a/ee/spec/factories/security/policies.rb b/ee/spec/factories/security/policies.rb index d33249b048a6f2..4acd7d77a5bbb3 100644 --- a/ee/spec/factories/security/policies.rb +++ b/ee/spec/factories/security/policies.rb @@ -102,6 +102,16 @@ } end end + + trait :pipeline_execution_schedule_policy do + type { Security::Policy.types[:pipeline_execution_schedule_policy] } + content do + { + content: { include: [{ project: 'compliance-project', file: "compliance-pipeline.yml" }] }, + schedule: { cadence: '0 0 * * *' } + } + end + end end factory :scan_execution_policy, diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb new file mode 100644 index 00000000000000..9691d80774c6b5 --- /dev/null +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::PipelineExecutionProjectSchedule, feature_category: :security_policy_management do + describe 'validations' do + let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } + + subject { schedule } + + it { is_expected.to be_valid } + it { is_expected.to validate_presence_of(:security_policy) } + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_uniqueness_of(:security_policy).scoped_to(:project_id) } + it { is_expected.to validate_presence_of(:cron) } + + it 'does not allow invalid cron patterns' do + schedule.cron = '0 0 0 * *' + + expect(schedule).not_to be_valid + end + + context 'when security policy is not a pipeline_execution_schedule_policy' do + let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_policy) } + let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } + + before do + # Set a valid cron value so the cron syntax validation passes + schedule.cron = '0 0 * * *' + end + + it { is_expected.not_to be_valid } + end + end + + describe 'associations' do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:security_policy).class_name('Security::Policy') } + end + + describe 'scopes' do + describe '.for_project' do + let_it_be(:project) { create(:project) } + let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule, project: project) } + let_it_be(:other_schedule) { create(:security_pipeline_execution_project_schedule) } + + it 'returns schedules for the given project' do + expect(described_class.for_project(project)).to contain_exactly(schedule) + end + end + + describe '.runnable' do + let_it_be(:runnable_schedule) { create(:security_pipeline_execution_project_schedule) } + let_it_be(:future_schedule) { create(:security_pipeline_execution_project_schedule) } + + before do + # Use update_column to avoid callbacks overwriting next_run_at. + runnable_schedule.update_column(:next_run_at, 1.hour.ago) + future_schedule.update_column(:next_run_at, 1.hour.from_now) + end + + it 'returns schedules that are due to run' do + expect(described_class.runnable).to contain_exactly(runnable_schedule) + end + end + end + + describe '#cron_timezone' do + let(:schedule) { build(:security_pipeline_execution_project_schedule) } + + subject { schedule.cron_timezone } + + context 'when timezone is not set' do + it { is_expected.to eq(Time.zone.name) } + end + + context 'when timezone is valid' do + before do + schedule.timezone = 'America/New_York' + end + + it { is_expected.to eq('America/New_York') } + end + + context 'when timezone is invalid' do + before do + schedule.timezone = 'Invalid/Timezone' + end + + it { is_expected.to eq(Time.zone.name) } + end + end +end diff --git a/ee/spec/models/security/policy_spec.rb b/ee/spec/models/security/policy_spec.rb index 257d24a804ff8d..63bfabd6b5f12b 100644 --- a/ee/spec/models/security/policy_spec.rb +++ b/ee/spec/models/security/policy_spec.rb @@ -10,6 +10,7 @@ it { is_expected.to have_many(:approval_policy_rules) } it { is_expected.to have_many(:security_policy_project_links) } it { is_expected.to have_many(:projects).through(:security_policy_project_links) } + it { is_expected.to have_many(:security_pipeline_execution_project_schedules) } it do is_expected.to validate_uniqueness_of(:security_orchestration_policy_configuration_id).scoped_to(%i[type @@ -136,6 +137,33 @@ expect { policy.link_project!(project) }.to not_change { Security::PolicyProjectLink.count } .and not_change { Security::ApprovalPolicyRuleProjectLink.count } end + + context 'when policy is a pipeline execution schedule policy' do + let_it_be(:policy) do + create( + :security_policy, + :pipeline_execution_schedule_policy, + content: { + content: { include: [{ project: 'compliance-project', file: "compliance-pipeline.yml" }] }, + schedule: { + cadence: '0 * * * *', + timezone: 'Europe/Berlin' + } + } + ) + end + + it 'creates a new schedule with the right attributes' do + expect { policy.link_project!(project) }.to change { Security::PolicyProjectLink.count }.by(1) + .and change { Security::PipelineExecutionProjectSchedule.count }.by(1) + + schedule = policy.security_pipeline_execution_project_schedules.first + + expect(schedule.project).to eq(project) + expect(schedule.timezone).to eq('Europe/Berlin') + expect(schedule.cron).to eq('0 * * * *') + end + end end describe '#unlink_project!' do @@ -161,6 +189,20 @@ .to not_change { Security::PolicyProjectLink.count } .and not_change { Security::ApprovalPolicyRuleProjectLink.count } end + + context 'when policy is a pipeline execution schedule policy' do + let_it_be(:policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + + before do + create(:security_policy_project_link, project: project, security_policy: policy) + create(:security_pipeline_execution_project_schedule, project: project, security_policy: policy) + end + + it 'removes the schedule' do + expect { policy.unlink_project!(project) }.to change { Security::PolicyProjectLink.count }.by(-1) + .and change { Security::PipelineExecutionProjectSchedule.count }.by(-1) + end + end end describe '#update_project_approval_policy_rule_links' do diff --git a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb new file mode 100644 index 00000000000000..7ceed784d4044d --- /dev/null +++ b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::PipelineExecutionPolicies::ScheduleWorker, feature_category: :security_policy_management do + describe '#perform' do + let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } + + subject(:perform) { described_class.new.perform } + + before do + # Use update_column to avoid callbacks overwriting next_run_at. + schedule.update_column(:next_run_at, 1.hour.ago) + end + + it 'enqueues the run worker' do + expect(Security::PipelineExecutionPolicies::RunScheduleWorker).to receive(:perform_async).with(schedule.id) + + perform + end + + it 'updates next_run_at' do + expect { perform }.to change { schedule.reload.next_run_at } + end + + context 'if cadence is invalid' do + before do + schedule.update_column(:cron, '*/5 * * * *') + end + + it 'does not updates next_run_at' do + expect { perform }.not_to change { schedule.reload.next_run_at } + end + + it 'does not enqueues the run worker' do + expect(Security::PipelineExecutionPolicies::RunScheduleWorker).not_to receive(:perform_async).with(schedule.id) + + perform + end + + it 'logs the error' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + event: 'scheduled_scan_execution_policy_validation', + message: 'Invalid cadence', + project_id: schedule.project_id, + cadence: schedule.cron + ) + + perform + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e4ee0d59874f36..3758a0053b6834 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50598,6 +50598,9 @@ msgstr "" msgid "Security policy bot cannot be added as a group member" msgstr "" +msgid "Security policy must be of type pipeline_execution_schedule_policy" +msgstr "" + msgid "Security policy project is already assigned." msgstr "" -- GitLab From 1d75bcaa1594ea9f9ffe8e28fbcde4fbded9b876 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 13 Dec 2024 07:54:57 +0100 Subject: [PATCH 02/24] Fix structure.sql --- db/structure.sql | 8 -------- 1 file changed, 8 deletions(-) diff --git a/db/structure.sql b/db/structure.sql index 5f2e3fe18ad02c..4a4ad6be549a35 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -33000,8 +33000,6 @@ CREATE INDEX index_security_orchestration_policy_rule_schedules_on_namespace ON CREATE INDEX index_security_orchestration_policy_rule_schedules_on_project_i ON security_orchestration_policy_rule_schedules USING btree (project_id); -CREATE INDEX index_security_pipeline_execution_schedules_on_next_run_at ON security_pipeline_execution_schedules USING btree (next_run_at); - CREATE INDEX index_security_policies_on_policy_management_project_id ON security_policies USING btree (security_policy_management_project_id); CREATE UNIQUE INDEX index_security_policies_on_unique_config_type_policy_index ON security_policies USING btree (security_orchestration_policy_configuration_id, type, policy_index); @@ -36822,9 +36820,6 @@ ALTER TABLE ONLY approval_group_rules_users ALTER TABLE ONLY bulk_import_exports ADD CONSTRAINT fk_39c726d3b5 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; -ALTER TABLE ONLY security_pipeline_execution_schedules - ADD CONSTRAINT fk_39de1c09cd FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY ml_model_versions ADD CONSTRAINT fk_39f8aa0b8a FOREIGN KEY (package_id) REFERENCES packages_packages(id) ON DELETE SET NULL; @@ -37413,9 +37408,6 @@ ALTER TABLE ONLY dependency_proxy_blob_states ALTER TABLE ONLY subscription_provision_syncs ADD CONSTRAINT fk_964dc9cc6c FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE; -ALTER TABLE ONLY security_pipeline_execution_schedules - ADD CONSTRAINT fk_9625df5e82 FOREIGN KEY (security_policy_id) REFERENCES security_policies(id) ON DELETE CASCADE; - ALTER TABLE ONLY issues ADD CONSTRAINT fk_96b1dd429c FOREIGN KEY (milestone_id) REFERENCES milestones(id) ON DELETE SET NULL; -- GitLab From 850f0fc1f2812e89a274d97a69323b55e6e22dc5 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 13 Dec 2024 22:17:30 +0100 Subject: [PATCH 03/24] Apply review suggestions --- .../pipeline_execution_project_schedule.rb | 5 ++--- .../run_schedule_worker.rb | 2 +- .../schedule_worker.rb | 4 ++-- .../pipeline_execution_project_schedules.rb | 4 +--- ...ipeline_execution_project_schedule_spec.rb | 13 +++++++---- .../schedule_worker_spec.rb | 22 ++++++++++++++----- 6 files changed, 31 insertions(+), 19 deletions(-) diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index 10e37fbe35008c..c3ec63a7319436 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -2,7 +2,6 @@ module Security class PipelineExecutionProjectSchedule < ApplicationRecord - include Schedulable include CronSchedulable self.table_name = 'security_pipeline_execution_project_schedules' @@ -16,7 +15,7 @@ class PipelineExecutionProjectSchedule < ApplicationRecord validate :security_policy_is_pipeline_execution_schedule_policy scope :for_project, ->(project) { where(project: project) } - scope :runnable, -> { where(next_run_at: ...Time.zone.now) } + scope :runnable_schedules, -> { where(next_run_at: ...Time.zone.now) } def cron_timezone # ActiveSupport::TimeZone.new will return nil if timezone is invalid @@ -37,7 +36,7 @@ def worker_cron_expression def security_policy_is_pipeline_execution_schedule_policy return unless security_policy - return if security_policy&.type_pipeline_execution_schedule_policy? + return if security_policy.type_pipeline_execution_schedule_policy? errors.add(:security_policy, _("Security policy must be of type pipeline_execution_schedule_policy")) diff --git a/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb index 0f561f6c79da0c..7a6e175c012296 100644 --- a/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb @@ -11,7 +11,7 @@ class RunScheduleWorker feature_category :security_policy_management def perform(schedule_id) - # no-op + # no-op, will be implemented as part of https://gitlab.com/gitlab-org/gitlab/-/issues/504091 end end end diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb index d1f7e0d572f0fe..edca5f958f42ac 100644 --- a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -13,12 +13,12 @@ class ScheduleWorker feature_category :security_policy_management def perform - Security::PipelineExecutionProjectSchedule.runnable.find_in_batches do |schedules| + Security::PipelineExecutionProjectSchedule.runnable_schedules.find_in_batches do |schedules| schedules.each do |schedule| unless valid_cadence?(schedule.cron) log_invalid_cadence_error(schedule.project_id, schedule.cron) - break + next end schedule.schedule_next_run! diff --git a/ee/spec/factories/security/pipeline_execution_project_schedules.rb b/ee/spec/factories/security/pipeline_execution_project_schedules.rb index 176fb1f502bfb4..89fa1511bdbd35 100644 --- a/ee/spec/factories/security/pipeline_execution_project_schedules.rb +++ b/ee/spec/factories/security/pipeline_execution_project_schedules.rb @@ -4,9 +4,7 @@ factory :security_pipeline_execution_project_schedule, class: 'Security::PipelineExecutionProjectSchedule' do project - security_policy do - association(:security_policy, :pipeline_execution_schedule_policy) - end + association :security_policy, :pipeline_execution_schedule_policy cron { '0 0 * * *' } end diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb index 9691d80774c6b5..30cc1ac30f0f62 100644 --- a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -4,16 +4,21 @@ RSpec.describe Security::PipelineExecutionProjectSchedule, feature_category: :security_policy_management do describe 'validations' do - let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } + let(:schedule) { build(:security_pipeline_execution_project_schedule) } subject { schedule } it { is_expected.to be_valid } it { is_expected.to validate_presence_of(:security_policy) } it { is_expected.to validate_presence_of(:project) } - it { is_expected.to validate_uniqueness_of(:security_policy).scoped_to(:project_id) } it { is_expected.to validate_presence_of(:cron) } + describe 'uniqueness validation' do + let(:schedule) { create(:security_pipeline_execution_project_schedule) } + + it { is_expected.to validate_uniqueness_of(:security_policy).scoped_to(:project_id) } + end + it 'does not allow invalid cron patterns' do schedule.cron = '0 0 0 * *' @@ -49,7 +54,7 @@ end end - describe '.runnable' do + describe '.runnable_schedules' do let_it_be(:runnable_schedule) { create(:security_pipeline_execution_project_schedule) } let_it_be(:future_schedule) { create(:security_pipeline_execution_project_schedule) } @@ -60,7 +65,7 @@ end it 'returns schedules that are due to run' do - expect(described_class.runnable).to contain_exactly(runnable_schedule) + expect(described_class.runnable_schedules).to contain_exactly(runnable_schedule) end end end diff --git a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb index 7ceed784d4044d..2f145f4dae9734 100644 --- a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb +++ b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb @@ -24,16 +24,26 @@ end context 'if cadence is invalid' do + let_it_be(:invalid_schedule) { create(:security_pipeline_execution_project_schedule) } + before do - schedule.update_column(:cron, '*/5 * * * *') + invalid_schedule.update_column(:next_run_at, 1.hour.ago) + invalid_schedule.update_column(:cron, '*/5 * * * *') + end + + it 'does not updates next_run_at for invalid schedules' do + expect { perform }.not_to change { invalid_schedule.reload.next_run_at } end - it 'does not updates next_run_at' do - expect { perform }.not_to change { schedule.reload.next_run_at } + it 'still updates next_run_at for valid schedules' do + expect { perform }.to change { schedule.reload.next_run_at } end it 'does not enqueues the run worker' do - expect(Security::PipelineExecutionPolicies::RunScheduleWorker).not_to receive(:perform_async).with(schedule.id) + expect(Security::PipelineExecutionPolicies::RunScheduleWorker).not_to( + receive(:perform_async).with(invalid_schedule.id) + ) + expect(Security::PipelineExecutionPolicies::RunScheduleWorker).to receive(:perform_async).with(schedule.id) perform end @@ -42,8 +52,8 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( event: 'scheduled_scan_execution_policy_validation', message: 'Invalid cadence', - project_id: schedule.project_id, - cadence: schedule.cron + project_id: invalid_schedule.project_id, + cadence: invalid_schedule.cron ) perform -- GitLab From 7e110a9f0fc44fea790a9a333229679c29292197 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 13 Dec 2024 22:20:38 +0100 Subject: [PATCH 04/24] Remove rubocop disable --- ...241210194442_create_security_pipeline_execution_schedules.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb b/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb index cfb3cf783c9db1..0d9a7d839cdbd2 100644 --- a/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb +++ b/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb @@ -4,7 +4,7 @@ class CreateSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2 milestone '17.7' def change - create_table :security_pipeline_execution_project_schedules do |t| # rubocop:disable Migration/EnsureFactoryForTable -- False positive, factory name is prefixed with security + create_table :security_pipeline_execution_project_schedules do |t| t.timestamps_with_timezone null: false t.datetime_with_timezone :next_run_at, null: false t.bigint :security_policy_id, null: false -- GitLab From 70942ad87ec9dd1318de7cde27ee45ebab0b59fd Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Mon, 16 Dec 2024 22:07:22 +0100 Subject: [PATCH 05/24] Add migration file to rubocop todolist --- .rubocop_todo/migration/ensure_factory_for_table.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.rubocop_todo/migration/ensure_factory_for_table.yml b/.rubocop_todo/migration/ensure_factory_for_table.yml index aaba711d248213..61287354468dd8 100644 --- a/.rubocop_todo/migration/ensure_factory_for_table.yml +++ b/.rubocop_todo/migration/ensure_factory_for_table.yml @@ -26,3 +26,4 @@ Migration/EnsureFactoryForTable: - 'db/migrate/20240404192955_create_early_access_program_tracking_events.rb' - 'db/migrate/20241216070908_create_compliance_requirements_controls.rb' - 'db/migrate/20241227065733_create_project_control_compliance_statuses.rb' + - 'db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb' -- GitLab From 968830ac96d87c74c57e82c76f390d9acaff7dee Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Tue, 17 Dec 2024 15:56:30 +0100 Subject: [PATCH 06/24] Apply review suggestions --- config/initializers/1_settings.rb | 2 +- ...241210194442_create_security_pipeline_execution_schedules.rb | 2 +- db/structure.sql | 2 +- ee/app/models/security/pipeline_execution_project_schedule.rb | 1 + .../security/pipeline_execution_policies/schedule_worker.rb | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 8360f2e461fa1d..d9c2a5a2621413 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -881,7 +881,7 @@ Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker']['cron'] ||= '*/15 * * * *' Settings.cron_jobs['security_orchestration_policy_rule_schedule_worker']['job_class'] = 'Security::OrchestrationPolicyRuleScheduleWorker' Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker'] ||= {} - Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['cron'] ||= '*/15 * * * *' + Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['cron'] ||= '* * * * *' Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['job_class'] = 'Security::PipelineExecutionPolicies::ScheduleWorker' Settings.cron_jobs['security_scans_purge_worker'] ||= {} Settings.cron_jobs['security_scans_purge_worker']['cron'] ||= '0 */4 * * 6,0' diff --git a/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb b/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb index 0d9a7d839cdbd2..4b58e700ece48f 100644 --- a/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb +++ b/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb @@ -15,7 +15,7 @@ def change add_index( :security_pipeline_execution_project_schedules, - :next_run_at, + [:next_run_at, :id], name: 'idx_security_pipeline_execution_project_schedules_next_run_at' ) diff --git a/db/structure.sql b/db/structure.sql index 4a4ad6be549a35..7b1c6e3e88130d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -29428,7 +29428,7 @@ CREATE INDEX idx_scan_result_policies_on_configuration_id_id_updated_at ON scan_ CREATE INDEX idx_scan_result_policy_violations_on_policy_id_and_id ON scan_result_policy_violations USING btree (scan_result_policy_id, id); -CREATE INDEX idx_security_pipeline_execution_project_schedules_next_run_at ON security_pipeline_execution_project_schedules USING btree (next_run_at); +CREATE INDEX idx_security_pipeline_execution_project_schedules_next_run_at ON security_pipeline_execution_project_schedules USING btree (next_run_at, id); CREATE INDEX idx_security_policies_config_id_policy_index ON security_policies USING btree (security_orchestration_policy_configuration_id, policy_index); diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index c3ec63a7319436..9b91ec2dbb3c6c 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -3,6 +3,7 @@ module Security class PipelineExecutionProjectSchedule < ApplicationRecord include CronSchedulable + include EachBatch self.table_name = 'security_pipeline_execution_project_schedules' diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb index edca5f958f42ac..817016e91ed68f 100644 --- a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -13,7 +13,7 @@ class ScheduleWorker feature_category :security_policy_management def perform - Security::PipelineExecutionProjectSchedule.runnable_schedules.find_in_batches do |schedules| + Security::PipelineExecutionProjectSchedule.runnable_schedules.each_batch do |schedules| schedules.each do |schedule| unless valid_cadence?(schedule.cron) log_invalid_cadence_error(schedule.project_id, schedule.cron) -- GitLab From 4f4d389b0b5e25a80e3f9bc4d282b5443697c33b Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Wed, 18 Dec 2024 11:09:33 +0100 Subject: [PATCH 07/24] Lock worker if other worker is still processing --- .../schedule_worker.rb | 34 ++++++++++++++----- .../schedule_worker_spec.rb | 29 +++++++++++++--- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb index 817016e91ed68f..7feb8505e5df97 100644 --- a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -6,6 +6,10 @@ class ScheduleWorker include ApplicationWorker include CronjobQueue include Security::SecurityOrchestrationPolicies::CadenceChecker + include ExclusiveLeaseGuard + + LEASE_KEY = 'security_pipeline_execution_policies_schedule' + LEASE_TIMEOUT = 5.minutes idempotent! @@ -13,22 +17,34 @@ class ScheduleWorker feature_category :security_policy_management def perform - Security::PipelineExecutionProjectSchedule.runnable_schedules.each_batch do |schedules| - schedules.each do |schedule| - unless valid_cadence?(schedule.cron) - log_invalid_cadence_error(schedule.project_id, schedule.cron) + try_obtain_lease do + Security::PipelineExecutionProjectSchedule.runnable_schedules.each_batch do |schedules| + schedules.each do |schedule| + unless valid_cadence?(schedule.cron) + log_invalid_cadence_error(schedule.project_id, schedule.cron) - next - end + next + end - schedule.schedule_next_run! + with_context(project: schedule.project) do + Security::PipelineExecutionPolicies::RunScheduleWorker.perform_async(schedule.id) + end - with_context(project: schedule.project) do - Security::PipelineExecutionPolicies::RunScheduleWorker.perform_async(schedule.id) + schedule.schedule_next_run! end end end end + + private + + def lease_key + LEASE_KEY + end + + def lease_timeout + LEASE_TIMEOUT + end end end end diff --git a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb index 2f145f4dae9734..68617c02936d08 100644 --- a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb +++ b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Security::PipelineExecutionPolicies::ScheduleWorker, feature_category: :security_policy_management do + include ExclusiveLeaseHelpers + describe '#perform' do let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } @@ -13,14 +15,31 @@ schedule.update_column(:next_run_at, 1.hour.ago) end - it 'enqueues the run worker' do - expect(Security::PipelineExecutionPolicies::RunScheduleWorker).to receive(:perform_async).with(schedule.id) + it_behaves_like 'an idempotent worker' do + it 'enqueues the run worker' do + expect(Security::PipelineExecutionPolicies::RunScheduleWorker).to receive(:perform_async).with(schedule.id) + + perform + end - perform + it 'updates next_run_at' do + expect { perform }.to change { schedule.reload.next_run_at } + end end - it 'updates next_run_at' do - expect { perform }.to change { schedule.reload.next_run_at } + context 'when another worker is still running' do + let(:lease_key) { described_class::LEASE_KEY } + let(:timeout) { described_class::LEASE_TIMEOUT } + let(:lease) { Gitlab::ExclusiveLease.new(lease_key, timeout: timeout).try_obtain } + + it 'does not enqueue the run worker' do + expect(Security::PipelineExecutionPolicies::RunScheduleWorker).not_to receive(:perform_async) + expect(lease).not_to be_nil + + perform + + Gitlab::ExclusiveLease.cancel(lease_key, lease) + end end context 'if cadence is invalid' do -- GitLab From 36530b70f4c316a2a9182fd0ae11c580455fc90d Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Thu, 19 Dec 2024 10:47:03 +0100 Subject: [PATCH 08/24] Stop using CronSchedulable --- .../security/pipeline_execution_project_schedule.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index 9b91ec2dbb3c6c..dedaeefa54bd95 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -2,8 +2,7 @@ module Security class PipelineExecutionProjectSchedule < ApplicationRecord - include CronSchedulable - include EachBatch + include Schedulable self.table_name = 'security_pipeline_execution_project_schedules' @@ -30,6 +29,16 @@ def cron_timezone private + def set_next_run_at + self.next_run_at = calculate_next_run_at + end + + def calculate_next_run_at + Gitlab::Ci::CronParser + .new(cron, cron_timezone) + .next_time_from(Time.zone.now) + end + def worker_cron_expression Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['cron'] end -- GitLab From 5e05ffffde5a76b526e3e1cdf6b0506204b4040c Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Thu, 19 Dec 2024 11:14:14 +0100 Subject: [PATCH 09/24] Use keyset pagination to iterate schedules --- .../pipeline_execution_project_schedule.rb | 1 + .../schedule_worker.rb | 6 ++++- ...ipeline_execution_project_schedule_spec.rb | 24 +++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index dedaeefa54bd95..2f55a47ee0447f 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -16,6 +16,7 @@ class PipelineExecutionProjectSchedule < ApplicationRecord scope :for_project, ->(project) { where(project: project) } scope :runnable_schedules, -> { where(next_run_at: ...Time.zone.now) } + scope :ordered_by_next_run_at, -> { order(:next_run_at, :id) } def cron_timezone # ActiveSupport::TimeZone.new will return nil if timezone is invalid diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb index 7feb8505e5df97..a907fb8badad3e 100644 --- a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -18,7 +18,11 @@ class ScheduleWorker def perform try_obtain_lease do - Security::PipelineExecutionProjectSchedule.runnable_schedules.each_batch do |schedules| + scope = Security::PipelineExecutionProjectSchedule.runnable_schedules.ordered_by_next_run_at + + iterator = Gitlab::Pagination::Keyset::Iterator.new(scope: scope) + + iterator.each_batch(of: 100) do |schedules| schedules.each do |schedule| unless valid_cadence?(schedule.cron) log_invalid_cadence_error(schedule.project_id, schedule.cron) diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb index 30cc1ac30f0f62..0c7ca779db97ec 100644 --- a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -68,6 +68,30 @@ expect(described_class.runnable_schedules).to contain_exactly(runnable_schedule) end end + + describe '.ordered_by_next_run_at' do + let_it_be(:monthly_schedule) { create(:security_pipeline_execution_project_schedule) } + let_it_be(:weekly_schedule) { create(:security_pipeline_execution_project_schedule) } + let_it_be(:daily_schedule) { create(:security_pipeline_execution_project_schedule) } + let_it_be(:daily_schedule_2) { create(:security_pipeline_execution_project_schedule) } + + before do + # Use update_column to avoid callbacks overwriting next_run_at. + monthly_schedule.update_column(:next_run_at, Time.zone.now + 1.month) + weekly_schedule.update_column(:next_run_at, Time.zone.now + 1.week) + + # Use the same time for both records to ensure the order by id is correct. + daily_schedule_time = Time.zone.now + 1.day + daily_schedule.update_column(:next_run_at, daily_schedule_time) + daily_schedule_2.update_column(:next_run_at, daily_schedule_time) + end + + it 'returns schedules ordered by next_run_at and id' do + expect(described_class.ordered_by_next_run_at).to eq( + [daily_schedule, daily_schedule_2, weekly_schedule, monthly_schedule] + ) + end + end end describe '#cron_timezone' do -- GitLab From 33bf7136ee50ca0d16a62ef8d16a3a1f02749f12 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Thu, 19 Dec 2024 12:08:39 +0100 Subject: [PATCH 10/24] Update timestamps and milestone --- .rubocop_todo/migration/ensure_factory_for_table.yml | 6 ++++++ db/docs/security_pipeline_execution_project_schedules.yml | 2 +- ...9105929_create_security_pipeline_execution_schedules.rb} | 2 +- ...y_policy_id_to_security_pipeline_execution_schedules.rb} | 2 +- ...foreign_key_to_security_pipeline_execution_schedules.rb} | 2 +- db/schema_migrations/20241210194442 | 1 - db/schema_migrations/20241210194457 | 1 - db/schema_migrations/20241210194529 | 1 - db/schema_migrations/20241219105929 | 1 + db/schema_migrations/20241219105931 | 1 + db/schema_migrations/20241219105936 | 1 + 11 files changed, 13 insertions(+), 7 deletions(-) rename db/migrate/{20241210194442_create_security_pipeline_execution_schedules.rb => 20241219105929_create_security_pipeline_execution_schedules.rb} (98%) rename db/migrate/{20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb => 20241219105931_add_security_policy_id_to_security_pipeline_execution_schedules.rb} (96%) rename db/migrate/{20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb => 20241219105936_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb} (96%) delete mode 100644 db/schema_migrations/20241210194442 delete mode 100644 db/schema_migrations/20241210194457 delete mode 100644 db/schema_migrations/20241210194529 create mode 100644 db/schema_migrations/20241219105929 create mode 100644 db/schema_migrations/20241219105931 create mode 100644 db/schema_migrations/20241219105936 diff --git a/.rubocop_todo/migration/ensure_factory_for_table.yml b/.rubocop_todo/migration/ensure_factory_for_table.yml index 61287354468dd8..a8a1c358890f75 100644 --- a/.rubocop_todo/migration/ensure_factory_for_table.yml +++ b/.rubocop_todo/migration/ensure_factory_for_table.yml @@ -27,3 +27,9 @@ Migration/EnsureFactoryForTable: - 'db/migrate/20241216070908_create_compliance_requirements_controls.rb' - 'db/migrate/20241227065733_create_project_control_compliance_statuses.rb' - 'db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb' + - 'db/migrate/20240419082037_create_ai_self_hosted_models.rb' + - 'db/migrate/20240423064716_create_ci_build_execution_config.rb' + - 'db/migrate/20240430110033_create_ai_feature_settings.rb' + - 'db/migrate/20241127092714_create_container_registry_protection_tag_rules.rb' + - 'db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb' + diff --git a/db/docs/security_pipeline_execution_project_schedules.yml b/db/docs/security_pipeline_execution_project_schedules.yml index 8daeb229986f9f..e3020676e08ce7 100644 --- a/db/docs/security_pipeline_execution_project_schedules.yml +++ b/db/docs/security_pipeline_execution_project_schedules.yml @@ -6,7 +6,7 @@ feature_categories: - security_policy_management description: Stores schedule executions for pipeline execution schedule policies introduced_by_url: -milestone: '17.7' +milestone: '17.8' gitlab_schema: gitlab_main_cell sharding_key: project_id: projects diff --git a/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb b/db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb similarity index 98% rename from db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb rename to db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb index 4b58e700ece48f..03689bcee8da68 100644 --- a/db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb +++ b/db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class CreateSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] - milestone '17.7' + milestone '17.8' def change create_table :security_pipeline_execution_project_schedules do |t| diff --git a/db/migrate/20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb b/db/migrate/20241219105931_add_security_policy_id_to_security_pipeline_execution_schedules.rb similarity index 96% rename from db/migrate/20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb rename to db/migrate/20241219105931_add_security_policy_id_to_security_pipeline_execution_schedules.rb index e321390eb73796..615dbc940a4d1d 100644 --- a/db/migrate/20241210194457_add_security_policy_id_to_security_pipeline_execution_schedules.rb +++ b/db/migrate/20241219105931_add_security_policy_id_to_security_pipeline_execution_schedules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddSecurityPolicyIdToSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] - milestone '17.7' + milestone '17.8' disable_ddl_transaction! def up diff --git a/db/migrate/20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb b/db/migrate/20241219105936_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb similarity index 96% rename from db/migrate/20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb rename to db/migrate/20241219105936_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb index 79ab6466f11960..251778b25108a8 100644 --- a/db/migrate/20241210194529_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb +++ b/db/migrate/20241219105936_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddProjectsForeignKeyToSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] - milestone '17.7' + milestone '17.8' disable_ddl_transaction! def up diff --git a/db/schema_migrations/20241210194442 b/db/schema_migrations/20241210194442 deleted file mode 100644 index fcac81622651eb..00000000000000 --- a/db/schema_migrations/20241210194442 +++ /dev/null @@ -1 +0,0 @@ -fe65dda0c86220ada8703a1095a4672ed08e0a55590e1b7a7837550ed6026889 \ No newline at end of file diff --git a/db/schema_migrations/20241210194457 b/db/schema_migrations/20241210194457 deleted file mode 100644 index 8f7b1f1c16c495..00000000000000 --- a/db/schema_migrations/20241210194457 +++ /dev/null @@ -1 +0,0 @@ -e619d3df566c5dd7a1c64f696e7287e2dcb6f5c6d67eb1ec6b45b2013563417a \ No newline at end of file diff --git a/db/schema_migrations/20241210194529 b/db/schema_migrations/20241210194529 deleted file mode 100644 index 38344d54928ce4..00000000000000 --- a/db/schema_migrations/20241210194529 +++ /dev/null @@ -1 +0,0 @@ -899ce3631eb5e5925ee043c4f79d211f35b84fe396505e1f095aff0b90daef77 \ No newline at end of file diff --git a/db/schema_migrations/20241219105929 b/db/schema_migrations/20241219105929 new file mode 100644 index 00000000000000..cb010f38a22c74 --- /dev/null +++ b/db/schema_migrations/20241219105929 @@ -0,0 +1 @@ +f24035f53fcf6e75c53f17dbf089d614c50758f0ce43c4935018aeba3de03ebf \ No newline at end of file diff --git a/db/schema_migrations/20241219105931 b/db/schema_migrations/20241219105931 new file mode 100644 index 00000000000000..56aafbf12c7abf --- /dev/null +++ b/db/schema_migrations/20241219105931 @@ -0,0 +1 @@ +4481e32a4750c39000bf3dcce052104801fc181dbc2764a7457dc7dc0113ab66 \ No newline at end of file diff --git a/db/schema_migrations/20241219105936 b/db/schema_migrations/20241219105936 new file mode 100644 index 00000000000000..d615139411067b --- /dev/null +++ b/db/schema_migrations/20241219105936 @@ -0,0 +1 @@ +9b4cbf2f170f25d747c0706ad2e5293284456060e255e8dcbcf9a9cc96ab031c \ No newline at end of file -- GitLab From 34eff8d0ce49550ee72f10b56e4363a7ca65361b Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Thu, 19 Dec 2024 22:03:39 +0100 Subject: [PATCH 11/24] Remove cron and timezone from schedules --- ...e_security_pipeline_execution_schedules.rb | 2 -- db/structure.sql | 6 +--- .../pipeline_execution_project_schedule.rb | 9 ++++- ee/app/models/security/policy.rb | 5 +-- .../pipeline_execution_project_schedules.rb | 2 -- ...ipeline_execution_project_schedule_spec.rb | 36 +++++++++++-------- ee/spec/models/security/policy_spec.rb | 3 +- .../schedule_worker_spec.rb | 14 ++++++-- 8 files changed, 44 insertions(+), 33 deletions(-) diff --git a/db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb b/db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb index 03689bcee8da68..7ec26bffb8e795 100644 --- a/db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb +++ b/db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb @@ -9,8 +9,6 @@ def change t.datetime_with_timezone :next_run_at, null: false t.bigint :security_policy_id, null: false t.bigint :project_id, null: false - t.text :cron, null: false, limit: 255 - t.text :timezone, limit: 128 end add_index( diff --git a/db/structure.sql b/db/structure.sql index 7b1c6e3e88130d..13051f6f9b622c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20019,11 +20019,7 @@ CREATE TABLE security_pipeline_execution_project_schedules ( updated_at timestamp with time zone NOT NULL, next_run_at timestamp with time zone NOT NULL, security_policy_id bigint NOT NULL, - project_id bigint NOT NULL, - cron text NOT NULL, - timezone text, - CONSTRAINT check_bbbe4b1b8d CHECK ((char_length(cron) <= 255)), - CONSTRAINT check_bf13eb178a CHECK ((char_length(timezone) <= 128)) + project_id bigint NOT NULL ); CREATE SEQUENCE security_pipeline_execution_project_schedules_id_seq diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index 2f55a47ee0447f..4da949b11c1fd8 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -11,7 +11,6 @@ class PipelineExecutionProjectSchedule < ApplicationRecord validates :security_policy, uniqueness: { scope: :project_id } validates :security_policy, :project, presence: true - validates :cron, cron: true, presence: true validate :security_policy_is_pipeline_execution_schedule_policy scope :for_project, ->(project) { where(project: project) } @@ -28,8 +27,16 @@ def cron_timezone Time.zone.name end + def cron + security_policy.content.dig('schedule', 'cadence') + end + private + def timezone + security_policy.content.dig('schedule', 'timezone') + end + def set_next_run_at self.next_run_at = calculate_next_run_at end diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 9c642ca5519e96..9776570be46eff 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -109,10 +109,7 @@ def link_project!(project) link_policy_rules_project!(project) if type_pipeline_execution_schedule_policy? - security_pipeline_execution_project_schedules.for_project(project).first_or_create!( - cron: schedule_cadence, - timezone: schedule_timezone - ) + security_pipeline_execution_project_schedules.for_project(project).first_or_create! end end end diff --git a/ee/spec/factories/security/pipeline_execution_project_schedules.rb b/ee/spec/factories/security/pipeline_execution_project_schedules.rb index 89fa1511bdbd35..b2656b42eab30f 100644 --- a/ee/spec/factories/security/pipeline_execution_project_schedules.rb +++ b/ee/spec/factories/security/pipeline_execution_project_schedules.rb @@ -5,7 +5,5 @@ project association :security_policy, :pipeline_execution_schedule_policy - - cron { '0 0 * * *' } end end diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb index 0c7ca779db97ec..64a771f017cbec 100644 --- a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -11,7 +11,6 @@ it { is_expected.to be_valid } it { is_expected.to validate_presence_of(:security_policy) } it { is_expected.to validate_presence_of(:project) } - it { is_expected.to validate_presence_of(:cron) } describe 'uniqueness validation' do let(:schedule) { create(:security_pipeline_execution_project_schedule) } @@ -19,21 +18,10 @@ it { is_expected.to validate_uniqueness_of(:security_policy).scoped_to(:project_id) } end - it 'does not allow invalid cron patterns' do - schedule.cron = '0 0 0 * *' - - expect(schedule).not_to be_valid - end - context 'when security policy is not a pipeline_execution_schedule_policy' do let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_policy) } let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } - before do - # Set a valid cron value so the cron syntax validation passes - schedule.cron = '0 0 * * *' - end - it { is_expected.not_to be_valid } end end @@ -104,18 +92,36 @@ end context 'when timezone is valid' do + let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + + let_it_be(:policy_content) do + { + content: { include: [{ project: 'compliance-project', file: "compliance-pipeline.yml" }] }, + schedule: { cadence: '0 0 * * *', timezone: 'America/New_York' } + }.deep_stringify_keys + end + + let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } + before do - schedule.timezone = 'America/New_York' + security_policy.update!(content: policy_content) end it { is_expected.to eq('America/New_York') } end context 'when timezone is invalid' do - before do - schedule.timezone = 'Invalid/Timezone' + let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + + let(:policy_content) do + { + content: { include: [{ project: 'compliance-project', file: "compliance-pipeline.yml" }] }, + schedule: { cadence: '0 0 * * *', timezone: 'Invalid/Timezone' } + } end + let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } + it { is_expected.to eq(Time.zone.name) } end end diff --git a/ee/spec/models/security/policy_spec.rb b/ee/spec/models/security/policy_spec.rb index 63bfabd6b5f12b..6fc4c83f7e3237 100644 --- a/ee/spec/models/security/policy_spec.rb +++ b/ee/spec/models/security/policy_spec.rb @@ -160,8 +160,7 @@ schedule = policy.security_pipeline_execution_project_schedules.first expect(schedule.project).to eq(project) - expect(schedule.timezone).to eq('Europe/Berlin') - expect(schedule.cron).to eq('0 * * * *') + expect(schedule.security_policy).to eq(policy) end end end diff --git a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb index 68617c02936d08..274b4ba968faea 100644 --- a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb +++ b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb @@ -43,11 +43,21 @@ end context 'if cadence is invalid' do - let_it_be(:invalid_schedule) { create(:security_pipeline_execution_project_schedule) } + let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + let_it_be(:invalid_schedule) do + create(:security_pipeline_execution_project_schedule, security_policy: security_policy) + end + + let(:policy_content) do + { + content: { include: [{ project: 'compliance-project', file: "compliance-pipeline.yml" }] }, + schedule: { cadence: '*/5 * * * *' } + }.deep_stringify_keys + end before do invalid_schedule.update_column(:next_run_at, 1.hour.ago) - invalid_schedule.update_column(:cron, '*/5 * * * *') + security_policy.update_column(:content, policy_content) end it 'does not updates next_run_at for invalid schedules' do -- GitLab From 14761ecd31eb43d261aa09f0a617305e92833ef1 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Thu, 19 Dec 2024 22:45:29 +0100 Subject: [PATCH 12/24] Apply review suggestion --- .../models/security/orchestration_policy_configuration.rb | 1 + .../security/orchestration_policy_configuration_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/ee/app/models/security/orchestration_policy_configuration.rb b/ee/app/models/security/orchestration_policy_configuration.rb index c015d3e2da3ab6..ad264227db904e 100644 --- a/ee/app/models/security/orchestration_policy_configuration.rb +++ b/ee/app/models/security/orchestration_policy_configuration.rb @@ -152,6 +152,7 @@ def policies_changed? yaml_differs_from_db?(security_policies.type_approval_policy, scan_result_policies) || yaml_differs_from_db?(security_policies.type_scan_execution_policy, scan_execution_policy) || yaml_differs_from_db?(security_policies.type_pipeline_execution_policy, pipeline_execution_policy) || + yaml_differs_from_db?(security_policies.type_pipeline_execution_schedule_policy, pipeline_execution_schedule_policy) || yaml_differs_from_db?(security_policies.type_vulnerability_management_policy, vulnerability_management_policy) end diff --git a/ee/spec/models/security/orchestration_policy_configuration_spec.rb b/ee/spec/models/security/orchestration_policy_configuration_spec.rb index b0ed9f02ad6643..9a2288838badf9 100644 --- a/ee/spec/models/security/orchestration_policy_configuration_spec.rb +++ b/ee/spec/models/security/orchestration_policy_configuration_spec.rb @@ -3237,6 +3237,14 @@ it { is_expected.to be_truthy } end + context 'when pipeline execution schedule policy has changed' do + before do + create(:security_policy, :pipeline_execution_schedule_policy, security_orchestration_policy_configuration: configuration) + end + + it { is_expected.to be_truthy } + end + context 'when vulnerability management policy has changed' do before do create(:security_policy, -- GitLab From 4939f7b7a2feeb32736cd507d34e5d471d6a4c91 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Thu, 19 Dec 2024 22:49:05 +0100 Subject: [PATCH 13/24] increase batch count --- .../security/pipeline_execution_policies/schedule_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb index a907fb8badad3e..83279b56d61d2f 100644 --- a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -22,7 +22,7 @@ def perform iterator = Gitlab::Pagination::Keyset::Iterator.new(scope: scope) - iterator.each_batch(of: 100) do |schedules| + iterator.each_batch(of: 1000) do |schedules| schedules.each do |schedule| unless valid_cadence?(schedule.cron) log_invalid_cadence_error(schedule.project_id, schedule.cron) -- GitLab From 1884c3059c61d3913c0c79e59230c6e95218820b Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 20 Dec 2024 10:06:38 +0100 Subject: [PATCH 14/24] Avoid N+1 queries --- .../pipeline_execution_project_schedule.rb | 1 + .../schedule_worker.rb | 7 +++++-- ...ipeline_execution_project_schedule_spec.rb | 21 +++++++++++++++++++ .../schedule_worker_spec.rb | 13 ++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index 4da949b11c1fd8..845bd224c9d224 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -16,6 +16,7 @@ class PipelineExecutionProjectSchedule < ApplicationRecord scope :for_project, ->(project) { where(project: project) } scope :runnable_schedules, -> { where(next_run_at: ...Time.zone.now) } scope :ordered_by_next_run_at, -> { order(:next_run_at, :id) } + scope :including_security_policy_and_project, -> { includes(:security_policy, :project) } def cron_timezone # ActiveSupport::TimeZone.new will return nil if timezone is invalid diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb index 83279b56d61d2f..42bd6200c798ce 100644 --- a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -18,7 +18,10 @@ class ScheduleWorker def perform try_obtain_lease do - scope = Security::PipelineExecutionProjectSchedule.runnable_schedules.ordered_by_next_run_at + scope = Security::PipelineExecutionProjectSchedule + .runnable_schedules + .including_security_policy_and_project + .ordered_by_next_run_at iterator = Gitlab::Pagination::Keyset::Iterator.new(scope: scope) @@ -30,7 +33,7 @@ def perform next end - with_context(project: schedule.project) do + with_context(project: schedule.project_id) do Security::PipelineExecutionPolicies::RunScheduleWorker.perform_async(schedule.id) end diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb index 64a771f017cbec..8af8a73f57bf17 100644 --- a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -80,6 +80,27 @@ ) end end + + describe '.including_security_policy_and_project' do + let_it_be(:schedule_1) { create(:security_pipeline_execution_project_schedule) } + let_it_be(:schedule_2) { create(:security_pipeline_execution_project_schedule) } + + it 'preloads security_policy and project' do + recorder = ActiveRecord::QueryRecorder.new do + schedules = described_class.including_security_policy_and_project + + schedules.each do |schedule| + schedule.security_policy + schedule.project + end + end + + # 1. Load schedules + # 2. Load security_policy + # 3. Load project + expect(recorder.count).to eq(3) + end + end end describe '#cron_timezone' do diff --git a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb index 274b4ba968faea..58dc164fa879d6 100644 --- a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb +++ b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb @@ -27,6 +27,19 @@ end end + it 'avoids N+1 queries' do + schedule.update_column(:next_run_at, 1.hour.ago) + + control_count = ActiveRecord::QueryRecorder.new { described_class.new.perform }.count + + schedule.update_column(:next_run_at, 1.hour.ago) + schedule_2 = create(:security_pipeline_execution_project_schedule) + schedule_2.update_column(:next_run_at, 1.hour.ago) + + # +4 queries to update next_run_at for one additional schedule + expect { described_class.new.perform }.not_to exceed_query_limit(control_count + 4) + end + context 'when another worker is still running' do let(:lease_key) { described_class::LEASE_KEY } let(:timeout) { described_class::LEASE_TIMEOUT } -- GitLab From 51b1a2045cdce6764fda00adc0712435e6b94ac0 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 20 Dec 2024 10:45:30 +0100 Subject: [PATCH 15/24] Calculate next run based on last run --- .../pipeline_execution_project_schedule.rb | 13 +++--- ...ipeline_execution_project_schedule_spec.rb | 43 +++++++++++++++++++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index 845bd224c9d224..3ac68be0528aaf 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -39,17 +39,16 @@ def timezone end def set_next_run_at - self.next_run_at = calculate_next_run_at + new_next_run_at = calculate_next_run_at(next_run_at || Time.zone.now) + new_next_run_at = calculate_next_run_at(Time.zone.now) if new_next_run_at.past? + + self.next_run_at = new_next_run_at end - def calculate_next_run_at + def calculate_next_run_at(from_time) Gitlab::Ci::CronParser .new(cron, cron_timezone) - .next_time_from(Time.zone.now) - end - - def worker_cron_expression - Settings.cron_jobs['security_pipeline_execution_policies_schedule_worker']['cron'] + .next_time_from(from_time) end def security_policy_is_pipeline_execution_schedule_policy diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb index 8af8a73f57bf17..b5148152271039 100644 --- a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -103,6 +103,49 @@ end end + describe 'callbacks' do + describe 'update next_run_at on save', time_travel_to: '2024-12-20 00:00:00' do + subject(:save!) { schedule.save! } + + context 'when next_run_at is not set' do + let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + + let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } + + it 'sets next_run_at to the next cron run based on current time' do + save! + + expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) + end + end + + context 'when next_run_at is set' do + let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } + + it 'updates next_run_at to the next cron run based on last next_run_at' do + save! + + expect(schedule.next_run_at).to eq(Time.zone.now + 2.days) + end + end + + context 'when new next_run_at value would be a result in the past' do + let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } + + before do + # Use update_column to avoid callbacks overwriting next_run_at. + schedule.update_column(:next_run_at, 1.year.ago) + end + + it 'updates next_run_at to the next cron run based on current time' do + save! + + expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) + end + end + end + end + describe '#cron_timezone' do let(:schedule) { build(:security_pipeline_execution_project_schedule) } -- GitLab From 6ac86f9a9855603e929fcaf338b991f40c1faf85 Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 20 Dec 2024 11:42:47 +0100 Subject: [PATCH 16/24] Remove Schedulable module --- .../pipeline_execution_project_schedule.rb | 7 +- ...ipeline_execution_project_schedule_spec.rb | 112 +++++++++++------- .../schedule_worker_spec.rb | 9 +- 3 files changed, 80 insertions(+), 48 deletions(-) diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index 3ac68be0528aaf..f26f94e8979a74 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -2,7 +2,7 @@ module Security class PipelineExecutionProjectSchedule < ApplicationRecord - include Schedulable + before_create :set_next_run_at self.table_name = 'security_pipeline_execution_project_schedules' @@ -32,6 +32,11 @@ def cron security_policy.content.dig('schedule', 'cadence') end + def schedule_next_run! + set_next_run_at + save! + end + private def timezone diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb index b5148152271039..971ecc4dfa2d20 100644 --- a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -47,9 +47,8 @@ let_it_be(:future_schedule) { create(:security_pipeline_execution_project_schedule) } before do - # Use update_column to avoid callbacks overwriting next_run_at. - runnable_schedule.update_column(:next_run_at, 1.hour.ago) - future_schedule.update_column(:next_run_at, 1.hour.from_now) + runnable_schedule.update!(next_run_at: 1.hour.ago) + future_schedule.update!(next_run_at: 1.hour.from_now) end it 'returns schedules that are due to run' do @@ -64,14 +63,13 @@ let_it_be(:daily_schedule_2) { create(:security_pipeline_execution_project_schedule) } before do - # Use update_column to avoid callbacks overwriting next_run_at. - monthly_schedule.update_column(:next_run_at, Time.zone.now + 1.month) - weekly_schedule.update_column(:next_run_at, Time.zone.now + 1.week) + monthly_schedule.update!(next_run_at: Time.zone.now + 1.month) + weekly_schedule.update!(next_run_at: Time.zone.now + 1.week) # Use the same time for both records to ensure the order by id is correct. daily_schedule_time = Time.zone.now + 1.day - daily_schedule.update_column(:next_run_at, daily_schedule_time) - daily_schedule_2.update_column(:next_run_at, daily_schedule_time) + daily_schedule.update!(next_run_at: daily_schedule_time) + daily_schedule_2.update!(next_run_at: daily_schedule_time) end it 'returns schedules ordered by next_run_at and id' do @@ -104,44 +102,17 @@ end describe 'callbacks' do - describe 'update next_run_at on save', time_travel_to: '2024-12-20 00:00:00' do - subject(:save!) { schedule.save! } - - context 'when next_run_at is not set' do - let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } - - let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } - - it 'sets next_run_at to the next cron run based on current time' do - save! - - expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) - end - end - - context 'when next_run_at is set' do - let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } - - it 'updates next_run_at to the next cron run based on last next_run_at' do - save! - - expect(schedule.next_run_at).to eq(Time.zone.now + 2.days) - end - end + describe 'update next_run_at on create', time_travel_to: '2024-12-20 00:00:00' do + let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } - context 'when new next_run_at value would be a result in the past' do - let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule) } + let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } - before do - # Use update_column to avoid callbacks overwriting next_run_at. - schedule.update_column(:next_run_at, 1.year.ago) - end + subject(:save!) { schedule.save! } - it 'updates next_run_at to the next cron run based on current time' do - save! + it 'sets next_run_at to the next cron run based on current time' do + save! - expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) - end + expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) end end end @@ -189,4 +160,61 @@ it { is_expected.to eq(Time.zone.name) } end end + + describe '#cron' do + let_it_be(:security_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + + let(:policy_content) do + { + content: { include: [{ project: 'compliance-project', file: "compliance-pipeline.yml" }] }, + schedule: { cadence: '0 0 1 * *', timezone: 'Invalid/Timezone' } + } + end + + let(:schedule) { build(:security_pipeline_execution_project_schedule, security_policy: security_policy) } + + before do + security_policy.update!(content: policy_content) + end + + it 'returns the cadence value from the security policy' do + expect(schedule.cron).to eq('0 0 1 * *') + end + end + + describe 'schedule_next_run!', time_travel_to: '2024-12-20 00:00:00' do + let(:schedule) { create(:security_pipeline_execution_project_schedule) } + + subject(:schedule_next_run!) { schedule.schedule_next_run! } + + it 'updates next_run_at to the next cron run based on last next_run_at' do + schedule_next_run! + + expect(schedule.next_run_at).to eq(Time.zone.now + 2.days) + end + + context 'when new next_run_at value would result in a time in the past' do + before do + schedule.next_run_at = 1.year.ago + end + + it 'updates next_run_at to the next cron run based on current time' do + schedule_next_run! + + expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) + end + end + + context 'when next_run_at is nil' do + before do + schedule.next_run_at = nil + end + + it 'sets next_run_at to the next cron run based on current time' do + schedule_next_run! + + expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) + end + end + end end diff --git a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb index 58dc164fa879d6..233ded3b8ad465 100644 --- a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb +++ b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb @@ -11,8 +11,7 @@ subject(:perform) { described_class.new.perform } before do - # Use update_column to avoid callbacks overwriting next_run_at. - schedule.update_column(:next_run_at, 1.hour.ago) + schedule.update!(next_run_at: 1.hour.ago) end it_behaves_like 'an idempotent worker' do @@ -28,13 +27,13 @@ end it 'avoids N+1 queries' do - schedule.update_column(:next_run_at, 1.hour.ago) + schedule.update!(next_run_at: 1.hour.ago) control_count = ActiveRecord::QueryRecorder.new { described_class.new.perform }.count - schedule.update_column(:next_run_at, 1.hour.ago) + schedule.update!(next_run_at: 1.hour.ago) schedule_2 = create(:security_pipeline_execution_project_schedule) - schedule_2.update_column(:next_run_at, 1.hour.ago) + schedule_2.update!(next_run_at: 1.hour.ago) # +4 queries to update next_run_at for one additional schedule expect { described_class.new.perform }.not_to exceed_query_limit(control_count + 4) -- GitLab From edbe30d1f80c7bddeeb3c8702447c927a28aa50c Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 20 Dec 2024 12:38:55 +0100 Subject: [PATCH 17/24] Delete schedules in worker --- ee/app/models/security/policy.rb | 4 ++++ .../security/delete_security_policy_worker.rb | 1 + ee/spec/models/security/policy_spec.rb | 20 +++++++++++++++++++ .../delete_security_policy_worker_spec.rb | 15 ++++++++++++++ 4 files changed, 40 insertions(+) diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 9776570be46eff..48ad53125d4957 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -225,6 +225,10 @@ def delete_scan_execution_policy_rules scan_execution_policy_rules.delete_all(:delete_all) end + def delete_security_pipeline_execution_project_schedules + security_pipeline_execution_project_schedules.delete_all(:delete_all) + end + def edit_path return if name.blank? diff --git a/ee/app/workers/security/delete_security_policy_worker.rb b/ee/app/workers/security/delete_security_policy_worker.rb index 30968a3dfb55f8..6adbc7f450b0db 100644 --- a/ee/app/workers/security/delete_security_policy_worker.rb +++ b/ee/app/workers/security/delete_security_policy_worker.rb @@ -18,6 +18,7 @@ def perform(security_policy_id) Security::Policy.transaction do policy.delete_approval_policy_rules policy.delete_scan_execution_policy_rules + policy.delete_security_pipeline_execution_project_schedules policy.delete end diff --git a/ee/spec/models/security/policy_spec.rb b/ee/spec/models/security/policy_spec.rb index 6fc4c83f7e3237..804805f1978479 100644 --- a/ee/spec/models/security/policy_spec.rb +++ b/ee/spec/models/security/policy_spec.rb @@ -553,6 +553,26 @@ end end + describe '#delete_security_pipeline_execution_project_schedules' do + let_it_be(:policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + let_it_be(:other_policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + let_it_be(:other_schedule) { create(:security_pipeline_execution_project_schedule, security_policy: other_policy) } + + before do + create_list(:security_pipeline_execution_project_schedule, 3, security_policy: policy) + end + + it 'deletes all associated PipelineExecutionProjectSchedule' do + expect { policy.delete_security_pipeline_execution_project_schedules }.to change { + Security::PipelineExecutionProjectSchedule.count + }.by(-3) + end + + it 'does not delete PipelineExecutionProjectSchedule from other policies' do + expect { policy.delete_security_pipeline_execution_project_schedules }.not_to change { other_schedule.reload } + end + end + describe '#delete_approval_policy_rules_for_project' do let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration) } let_it_be(:policy) do diff --git a/ee/spec/workers/security/delete_security_policy_worker_spec.rb b/ee/spec/workers/security/delete_security_policy_worker_spec.rb index 609a5b563ae00d..cd1404ad994da8 100644 --- a/ee/spec/workers/security/delete_security_policy_worker_spec.rb +++ b/ee/spec/workers/security/delete_security_policy_worker_spec.rb @@ -43,6 +43,21 @@ end end + context 'when the policy type is pipeline_execution_schedule_policy' do + let_it_be(:policy) { create(:security_policy, :pipeline_execution_schedule_policy) } + let_it_be(:schedule) { create(:security_pipeline_execution_project_schedule, security_policy: policy) } + + before do + allow_next_found_instance_of(Security::Policy) do |security_policy| + allow(security_policy).to receive(:delete) + end + end + + it 'deletes the security policy and associated records' do + expect { perform }.to change { Security::PipelineExecutionProjectSchedule.count }.by(-1) + end + end + context 'when the security policy exists' do it 'deletes the security policy and associated records' do expect { perform }.to change { ApprovalProjectRule.count }.by(-1) -- GitLab From 6ab8b9763dd8127923a5d559c5510e833b2c0efa Mon Sep 17 00:00:00 2001 From: Andy Schoenen Date: Fri, 20 Dec 2024 21:34:08 +0100 Subject: [PATCH 18/24] Remove unused methods --- ee/app/models/security/policy.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 48ad53125d4957..eb0ce69f674de9 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -242,14 +242,6 @@ def edit_path private - def schedule_cadence - content.dig('schedule', 'cadence') - end - - def schedule_timezone - content.dig('schedule', 'timezone') - end - def link_policy_rules_project!(project, policy_rules = approval_policy_rules.undeleted) return if !type_approval_policy? || policy_rules.empty? -- GitLab From 2e014b04f56b9a387ea8b4d74a9687295fa40eee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= Date: Tue, 7 Jan 2025 09:31:56 +0100 Subject: [PATCH 19/24] Refresh migration timestamps --- .rubocop_todo/migration/ensure_factory_for_table.yml | 9 +-------- ...0737_create_security_pipeline_execution_schedules.rb} | 0 ...olicy_id_to_security_pipeline_execution_schedules.rb} | 0 ...eign_key_to_security_pipeline_execution_schedules.rb} | 0 db/schema_migrations/20241219105929 | 1 - db/schema_migrations/20241219105931 | 1 - db/schema_migrations/20241219105936 | 1 - db/schema_migrations/20250113110737 | 1 + db/schema_migrations/20250113110741 | 1 + db/schema_migrations/20250113110743 | 1 + 10 files changed, 4 insertions(+), 11 deletions(-) rename db/migrate/{20241219105929_create_security_pipeline_execution_schedules.rb => 20250113110737_create_security_pipeline_execution_schedules.rb} (100%) rename db/migrate/{20241219105931_add_security_policy_id_to_security_pipeline_execution_schedules.rb => 20250113110741_add_security_policy_id_to_security_pipeline_execution_schedules.rb} (100%) rename db/migrate/{20241219105936_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb => 20250113110743_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb} (100%) delete mode 100644 db/schema_migrations/20241219105929 delete mode 100644 db/schema_migrations/20241219105931 delete mode 100644 db/schema_migrations/20241219105936 create mode 100644 db/schema_migrations/20250113110737 create mode 100644 db/schema_migrations/20250113110741 create mode 100644 db/schema_migrations/20250113110743 diff --git a/.rubocop_todo/migration/ensure_factory_for_table.yml b/.rubocop_todo/migration/ensure_factory_for_table.yml index a8a1c358890f75..1075d929ed6875 100644 --- a/.rubocop_todo/migration/ensure_factory_for_table.yml +++ b/.rubocop_todo/migration/ensure_factory_for_table.yml @@ -25,11 +25,4 @@ Migration/EnsureFactoryForTable: - 'db/migrate/20240306121653_create_relation_import_tracker.rb' - 'db/migrate/20240404192955_create_early_access_program_tracking_events.rb' - 'db/migrate/20241216070908_create_compliance_requirements_controls.rb' - - 'db/migrate/20241227065733_create_project_control_compliance_statuses.rb' - - 'db/migrate/20241210194442_create_security_pipeline_execution_schedules.rb' - - 'db/migrate/20240419082037_create_ai_self_hosted_models.rb' - - 'db/migrate/20240423064716_create_ci_build_execution_config.rb' - - 'db/migrate/20240430110033_create_ai_feature_settings.rb' - - 'db/migrate/20241127092714_create_container_registry_protection_tag_rules.rb' - - 'db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb' - + - 'db/migrate/20250107082720_create_security_pipeline_execution_schedules.rb' diff --git a/db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb b/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb similarity index 100% rename from db/migrate/20241219105929_create_security_pipeline_execution_schedules.rb rename to db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb diff --git a/db/migrate/20241219105931_add_security_policy_id_to_security_pipeline_execution_schedules.rb b/db/migrate/20250113110741_add_security_policy_id_to_security_pipeline_execution_schedules.rb similarity index 100% rename from db/migrate/20241219105931_add_security_policy_id_to_security_pipeline_execution_schedules.rb rename to db/migrate/20250113110741_add_security_policy_id_to_security_pipeline_execution_schedules.rb diff --git a/db/migrate/20241219105936_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb b/db/migrate/20250113110743_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb similarity index 100% rename from db/migrate/20241219105936_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb rename to db/migrate/20250113110743_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb diff --git a/db/schema_migrations/20241219105929 b/db/schema_migrations/20241219105929 deleted file mode 100644 index cb010f38a22c74..00000000000000 --- a/db/schema_migrations/20241219105929 +++ /dev/null @@ -1 +0,0 @@ -f24035f53fcf6e75c53f17dbf089d614c50758f0ce43c4935018aeba3de03ebf \ No newline at end of file diff --git a/db/schema_migrations/20241219105931 b/db/schema_migrations/20241219105931 deleted file mode 100644 index 56aafbf12c7abf..00000000000000 --- a/db/schema_migrations/20241219105931 +++ /dev/null @@ -1 +0,0 @@ -4481e32a4750c39000bf3dcce052104801fc181dbc2764a7457dc7dc0113ab66 \ No newline at end of file diff --git a/db/schema_migrations/20241219105936 b/db/schema_migrations/20241219105936 deleted file mode 100644 index d615139411067b..00000000000000 --- a/db/schema_migrations/20241219105936 +++ /dev/null @@ -1 +0,0 @@ -9b4cbf2f170f25d747c0706ad2e5293284456060e255e8dcbcf9a9cc96ab031c \ No newline at end of file diff --git a/db/schema_migrations/20250113110737 b/db/schema_migrations/20250113110737 new file mode 100644 index 00000000000000..f5e207c13cb368 --- /dev/null +++ b/db/schema_migrations/20250113110737 @@ -0,0 +1 @@ +1a3e7b05abb29af280785bb9b9f5fc44eb39501eaf8dbe07679cf1b5555b2450 \ No newline at end of file diff --git a/db/schema_migrations/20250113110741 b/db/schema_migrations/20250113110741 new file mode 100644 index 00000000000000..1550bd7f6f546f --- /dev/null +++ b/db/schema_migrations/20250113110741 @@ -0,0 +1 @@ +72ca31fd8fdcb59f7995e357b6d9020dd00be252cf8b721dbe380c861ab8515d \ No newline at end of file diff --git a/db/schema_migrations/20250113110743 b/db/schema_migrations/20250113110743 new file mode 100644 index 00000000000000..0fba3e69e87e70 --- /dev/null +++ b/db/schema_migrations/20250113110743 @@ -0,0 +1 @@ +14e14f5f36843bdff5b0b3d532d6fdf8254dac66afdad1cab55d80dfca8a1daf \ No newline at end of file -- GitLab From 956cfbfd07e14bd25c4af4385afb288035d0f66b Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Mon, 13 Jan 2025 15:04:04 +0100 Subject: [PATCH 20/24] Delete project schedules in batches --- ...0113110737_create_security_pipeline_execution_schedules.rb | 4 ++-- db/structure.sql | 2 +- ee/app/models/security/pipeline_execution_project_schedule.rb | 2 ++ ee/app/models/security/policy.rb | 4 +++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb b/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb index 7ec26bffb8e795..a16179f6ab603a 100644 --- a/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb +++ b/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb @@ -19,8 +19,8 @@ def change add_index( :security_pipeline_execution_project_schedules, - :security_policy_id, - name: 'idx_pipeline_execution_schedules_security_policy_id' + [:security_policy_id, :id], + name: 'idx_pipeline_execution_schedules_security_policy_id_and_id' ) add_index( diff --git a/db/structure.sql b/db/structure.sql index 13051f6f9b622c..56fa0dc518276a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -29354,7 +29354,7 @@ CREATE INDEX idx_packages_packages_on_project_id_name_version_package_type ON pa CREATE INDEX idx_personal_access_tokens_on_previous_personal_access_token_id ON personal_access_tokens USING btree (previous_personal_access_token_id); -CREATE INDEX idx_pipeline_execution_schedules_security_policy_id ON security_pipeline_execution_project_schedules USING btree (security_policy_id); +CREATE INDEX idx_pipeline_execution_schedules_security_policy_id_and_id ON security_pipeline_execution_project_schedules USING btree (security_policy_id, id); CREATE INDEX idx_pkgs_conan_file_metadata_on_pkg_file_id_when_recipe_file ON packages_conan_file_metadata USING btree (package_file_id) WHERE (conan_file_type = 1); diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index f26f94e8979a74..8134f49ba1d8da 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -2,6 +2,8 @@ module Security class PipelineExecutionProjectSchedule < ApplicationRecord + include EachBatch + before_create :set_next_run_at self.table_name = 'security_pipeline_execution_project_schedules' diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index eb0ce69f674de9..42d26d1069a76e 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -226,7 +226,9 @@ def delete_scan_execution_policy_rules end def delete_security_pipeline_execution_project_schedules - security_pipeline_execution_project_schedules.delete_all(:delete_all) + security_pipeline_execution_project_schedules.each_batch do |batch| + batch.delete_all + end end def edit_path -- GitLab From bdffee56c017dd74a72f1fb840984a0ff68809e8 Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Mon, 13 Jan 2025 15:38:27 +0100 Subject: [PATCH 21/24] Always anchor `#schedule_next_run!` on current time --- .../models/security/pipeline_execution_project_schedule.rb | 5 +---- .../security/pipeline_execution_project_schedule_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/ee/app/models/security/pipeline_execution_project_schedule.rb b/ee/app/models/security/pipeline_execution_project_schedule.rb index 8134f49ba1d8da..0674190e6b8251 100644 --- a/ee/app/models/security/pipeline_execution_project_schedule.rb +++ b/ee/app/models/security/pipeline_execution_project_schedule.rb @@ -46,10 +46,7 @@ def timezone end def set_next_run_at - new_next_run_at = calculate_next_run_at(next_run_at || Time.zone.now) - new_next_run_at = calculate_next_run_at(Time.zone.now) if new_next_run_at.past? - - self.next_run_at = new_next_run_at + self.next_run_at = calculate_next_run_at(Time.zone.now) end def calculate_next_run_at(from_time) diff --git a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb index 971ecc4dfa2d20..b65352fc26f115 100644 --- a/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb +++ b/ee/spec/models/security/pipeline_execution_project_schedule_spec.rb @@ -187,10 +187,10 @@ subject(:schedule_next_run!) { schedule.schedule_next_run! } - it 'updates next_run_at to the next cron run based on last next_run_at' do + it 'updates next_run_at to the next cron run based on current time' do schedule_next_run! - expect(schedule.next_run_at).to eq(Time.zone.now + 2.days) + expect(schedule.next_run_at).to eq(Time.zone.now + 1.day) end context 'when new next_run_at value would result in a time in the past' do -- GitLab From 5daf0bcc003268da90426559e12d4e674ca17d9d Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Mon, 13 Jan 2025 15:44:43 +0100 Subject: [PATCH 22/24] Fix ensure_factory_for_table.yml --- .rubocop_todo/migration/ensure_factory_for_table.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.rubocop_todo/migration/ensure_factory_for_table.yml b/.rubocop_todo/migration/ensure_factory_for_table.yml index 1075d929ed6875..24f79229caa77a 100644 --- a/.rubocop_todo/migration/ensure_factory_for_table.yml +++ b/.rubocop_todo/migration/ensure_factory_for_table.yml @@ -25,4 +25,5 @@ Migration/EnsureFactoryForTable: - 'db/migrate/20240306121653_create_relation_import_tracker.rb' - 'db/migrate/20240404192955_create_early_access_program_tracking_events.rb' - 'db/migrate/20241216070908_create_compliance_requirements_controls.rb' + - 'db/migrate/20241227065733_create_project_control_compliance_statuses.rb' - 'db/migrate/20250107082720_create_security_pipeline_execution_schedules.rb' -- GitLab From 03d046eaf7f722077409f9bd13f1cf8dfdb9ba3e Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Tue, 14 Jan 2025 11:01:00 +0000 Subject: [PATCH 23/24] Move to %17.9 milestone --- db/docs/security_pipeline_execution_project_schedules.yml | 2 +- ...250113110737_create_security_pipeline_execution_schedules.rb | 2 +- ...curity_policy_id_to_security_pipeline_execution_schedules.rb | 2 +- ...ects_foreign_key_to_security_pipeline_execution_schedules.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/db/docs/security_pipeline_execution_project_schedules.yml b/db/docs/security_pipeline_execution_project_schedules.yml index e3020676e08ce7..efa3fa9cc706d3 100644 --- a/db/docs/security_pipeline_execution_project_schedules.yml +++ b/db/docs/security_pipeline_execution_project_schedules.yml @@ -6,7 +6,7 @@ feature_categories: - security_policy_management description: Stores schedule executions for pipeline execution schedule policies introduced_by_url: -milestone: '17.8' +milestone: '17.9' gitlab_schema: gitlab_main_cell sharding_key: project_id: projects diff --git a/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb b/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb index a16179f6ab603a..c5496500aa2997 100644 --- a/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb +++ b/db/migrate/20250113110737_create_security_pipeline_execution_schedules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class CreateSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] - milestone '17.8' + milestone '17.9' def change create_table :security_pipeline_execution_project_schedules do |t| diff --git a/db/migrate/20250113110741_add_security_policy_id_to_security_pipeline_execution_schedules.rb b/db/migrate/20250113110741_add_security_policy_id_to_security_pipeline_execution_schedules.rb index 615dbc940a4d1d..291b75bb8bc339 100644 --- a/db/migrate/20250113110741_add_security_policy_id_to_security_pipeline_execution_schedules.rb +++ b/db/migrate/20250113110741_add_security_policy_id_to_security_pipeline_execution_schedules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddSecurityPolicyIdToSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] - milestone '17.8' + milestone '17.9' disable_ddl_transaction! def up diff --git a/db/migrate/20250113110743_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb b/db/migrate/20250113110743_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb index 251778b25108a8..6553dce5f47515 100644 --- a/db/migrate/20250113110743_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb +++ b/db/migrate/20250113110743_add_projects_foreign_key_to_security_pipeline_execution_schedules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddProjectsForeignKeyToSecurityPipelineExecutionSchedules < Gitlab::Database::Migration[2.2] - milestone '17.8' + milestone '17.9' disable_ddl_transaction! def up -- GitLab From 32ceab7b0e4f7606bf306cf28ab27292dcbd526a Mon Sep 17 00:00:00 2001 From: Dominic Bauer Date: Wed, 15 Jan 2025 17:40:24 +0100 Subject: [PATCH 24/24] Add `scheduled_pipeline_execution_policies` feature flag --- ee/app/models/security/policy.rb | 2 +- .../pipeline_execution_policies/schedule_worker.rb | 2 ++ .../scheduled_pipeline_execution_policies.yml | 9 +++++++++ ee/spec/models/security/policy_spec.rb | 10 ++++++++++ .../schedule_worker_spec.rb | 12 ++++++++++++ 5 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 ee/config/feature_flags/gitlab_com_derisk/scheduled_pipeline_execution_policies.yml diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 42d26d1069a76e..35a007a5099c0d 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -108,7 +108,7 @@ def link_project!(project) security_policy_project_links.for_project(project).first_or_create! link_policy_rules_project!(project) - if type_pipeline_execution_schedule_policy? + if type_pipeline_execution_schedule_policy? && Feature.enabled?(:scheduled_pipeline_execution_policies, project) security_pipeline_execution_project_schedules.for_project(project).first_or_create! end end diff --git a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb index 42bd6200c798ce..2057a2a467a498 100644 --- a/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb +++ b/ee/app/workers/security/pipeline_execution_policies/schedule_worker.rb @@ -27,6 +27,8 @@ def perform iterator.each_batch(of: 1000) do |schedules| schedules.each do |schedule| + next unless Feature.enabled?(:scheduled_pipeline_execution_policies, schedule.project) + unless valid_cadence?(schedule.cron) log_invalid_cadence_error(schedule.project_id, schedule.cron) diff --git a/ee/config/feature_flags/gitlab_com_derisk/scheduled_pipeline_execution_policies.yml b/ee/config/feature_flags/gitlab_com_derisk/scheduled_pipeline_execution_policies.yml new file mode 100644 index 00000000000000..afdc56b696d3fc --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/scheduled_pipeline_execution_policies.yml @@ -0,0 +1,9 @@ +--- +name: scheduled_pipeline_execution_policies +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/504092 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/175246 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/513337 +milestone: '17.9' +group: group::security policies +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/spec/models/security/policy_spec.rb b/ee/spec/models/security/policy_spec.rb index 804805f1978479..b13bcdca4353c5 100644 --- a/ee/spec/models/security/policy_spec.rb +++ b/ee/spec/models/security/policy_spec.rb @@ -162,6 +162,16 @@ expect(schedule.project).to eq(project) expect(schedule.security_policy).to eq(policy) end + + context 'with feature disabled' do + before do + stub_feature_flags(scheduled_pipeline_execution_policies: false) + end + + it 'does not create a link' do + expect { policy.link_project!(project) }.not_to change { Security::PipelineExecutionProjectSchedule.count } + end + end end end diff --git a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb index 233ded3b8ad465..b0d740b0cc56ed 100644 --- a/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb +++ b/ee/spec/workers/security/pipeline_execution_policies/schedule_worker_spec.rb @@ -24,6 +24,18 @@ it 'updates next_run_at' do expect { perform }.to change { schedule.reload.next_run_at } end + + context 'with feature disabled' do + before do + stub_feature_flags(scheduled_pipeline_execution_policies: false) + end + + it 'does not enqueue the run worker' do + expect(Security::PipelineExecutionPolicies::RunScheduleWorker).not_to receive(:perform_async) + + perform + end + end end it 'avoids N+1 queries' do -- GitLab