diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index 13aabc0d94c8e56de129af43c901d830e0a5fb25..c30ab828d68eb6c31c6f241fb90f88368949520d 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -38368,6 +38368,7 @@ Security policy state synchronization update. Returns `null` if the `security_po | `mergeRequestsTotal` | [`Int`](#int) | Total number of merge requests synced. | | `projectsProgress` | [`Float`](#float) | Percentage of projects synced. | | `projectsTotal` | [`Int`](#int) | Total number of projects synced. | +| `startedAt` | [`Time`](#time) | Policy synchronization start time. | ### `PolicyAnyMergeRequestViolation` diff --git a/ee/app/graphql/ee/graphql_triggers.rb b/ee/app/graphql/ee/graphql_triggers.rb index 4543b8aa6c7968b606cae2fb1ef8f1f36c6f0341..dba96685467798ea02e5d76785932f19cd807626 100644 --- a/ee/app/graphql/ee/graphql_triggers.rb +++ b/ee/app/graphql/ee/graphql_triggers.rb @@ -76,14 +76,15 @@ def self.security_policies_sync_updated( failed_projects, merge_requests_progress, merge_requests_total, - in_progress + in_progress, + started_at ) ::GitlabSchema.subscriptions.trigger( :security_policies_sync_updated, { policy_configuration_id: policy_configuration.to_global_id }, { projects_progress: projects_progress, projects_total: projects_total, failed_projects: failed_projects, merge_requests_progress: merge_requests_progress, merge_requests_total: merge_requests_total, - in_progress: in_progress } + in_progress: in_progress, started_at: started_at } ) end end diff --git a/ee/app/graphql/subscriptions/security/policies_sync_updated.rb b/ee/app/graphql/subscriptions/security/policies_sync_updated.rb index ff571addd417a040466ebfe467a4f3f59567e8c0..bf9f43cb314761058a8d9486c8b9118af91f3507 100644 --- a/ee/app/graphql/subscriptions/security/policies_sync_updated.rb +++ b/ee/app/graphql/subscriptions/security/policies_sync_updated.rb @@ -29,7 +29,8 @@ def update(_) failed_projects: object[:failed_projects], merge_requests_progress: object[:merge_requests_progress], merge_requests_total: object[:merge_requests_total], - in_progress: object[:in_progress] + in_progress: object[:in_progress], + started_at: Time.at(object[:started_at]) } end diff --git a/ee/app/graphql/types/gitlab_subscriptions/security/policies_sync_updated.rb b/ee/app/graphql/types/gitlab_subscriptions/security/policies_sync_updated.rb index 4bf53f7a143630e6a921221e3538aa7016376eb8..629a497506e84c11782967c94f721e3f74c20dba 100644 --- a/ee/app/graphql/types/gitlab_subscriptions/security/policies_sync_updated.rb +++ b/ee/app/graphql/types/gitlab_subscriptions/security/policies_sync_updated.rb @@ -34,6 +34,10 @@ class PoliciesSyncUpdated < ::Types::BaseObject field :in_progress, GraphQL::Types::Boolean, null: true, description: 'Whether security policies are currently being synchronized.' + + field :started_at, Types::TimeType, + null: true, + description: 'Policy synchronization start time.' end # rubocop:enable GraphQL/ExtractType # rubocop:enable Graphql/AuthorizeTypes diff --git a/ee/lib/security/security_orchestration_policies/policy_sync_state.rb b/ee/lib/security/security_orchestration_policies/policy_sync_state.rb index ea2795d78b2c7b4ba325ab4267e06b013531af63..f4391ef258e66a85d18295091287ec26b69598a6 100644 --- a/ee/lib/security/security_orchestration_policies/policy_sync_state.rb +++ b/ee/lib/security/security_orchestration_policies/policy_sync_state.rb @@ -30,13 +30,17 @@ def to_h merge_requests_pending = redis.scard(merge_requests_sync_key) merge_requests_total = redis.get(total_merge_requests_key).to_i + in_progress = sync_in_progress?(redis) + started_at = redis.get(sync_started_at_key).to_i + { projects_progress: get_progress(projects_pending, projects_total), projects_total: projects_total, failed_projects: redis.smembers(failed_projects_sync_key), merge_requests_progress: get_progress(merge_requests_pending, merge_requests_total), merge_requests_total: merge_requests_total, - in_progress: sync_in_progress?(redis) + in_progress: in_progress, + started_at: in_progress ? Time.at(started_at).utc : nil } end end @@ -46,7 +50,10 @@ def start_sync return if feature_disabled? with_redis do |redis| - redis.set(sync_in_progress_key, PROGRESS_MARKER, ex: POLICY_SYNC_TTL) + redis.multi do |multi| + multi.set(sync_in_progress_key, PROGRESS_MARKER, ex: POLICY_SYNC_TTL) + multi.set(sync_started_at_key, Time.now.utc.to_i, ex: POLICY_SYNC_TTL, nx: true) + end end end @@ -55,7 +62,7 @@ def finish_sync return if feature_disabled? with_redis do |redis| - redis.del(sync_in_progress_key) + redis.del(sync_in_progress_key, sync_started_at_key) end end @@ -66,6 +73,7 @@ def append_projects(project_ids) with_redis do |redis| redis.multi do |multi| multi.set(sync_in_progress_key, PROGRESS_MARKER, ex: POLICY_SYNC_TTL) + multi.set(sync_started_at_key, Time.now.utc.to_i, ex: POLICY_SYNC_TTL, nx: true) multi.sadd(projects_sync_key, project_ids) multi.incrby(total_projects_key, project_ids.size) @@ -111,6 +119,7 @@ def start_merge_request(merge_request_id) with_redis do |redis| redis.multi do |multi| multi.set(sync_in_progress_key, PROGRESS_MARKER, ex: POLICY_SYNC_TTL) + multi.set(sync_started_at_key, Time.now.utc.to_i, ex: POLICY_SYNC_TTL, nx: true) multi.sadd(merge_requests_sync_key, merge_request_id) multi.incr(total_merge_requests_key) @@ -219,6 +228,11 @@ def sync_in_progress_key "#{redis_key_tag}:in_progress" end + # String: UTC timestamp of when the sync started + def sync_started_at_key + "#{redis_key_tag}:started_at" + end + # Set: project IDs pending synchronization def projects_sync_key "#{redis_key_tag}:projects" @@ -271,7 +285,8 @@ def trigger_subscription all_failed_projects, merge_requests_pending, merge_requests_total, - in_progress = + in_progress, + started_at = with_redis do |redis| [ redis.scard(projects_sync_key), @@ -279,7 +294,8 @@ def trigger_subscription redis.smembers(failed_projects_sync_key), redis.scard(merge_requests_sync_key), redis.get(total_merge_requests_key).to_i, - sync_in_progress?(redis) + sync_in_progress?(redis), + redis.get(sync_started_at_key).to_i ] end @@ -296,7 +312,8 @@ def trigger_subscription all_failed_projects, get_progress(merge_requests_pending, merge_requests_total), merge_requests_total, - in_progress + in_progress, + in_progress ? Time.at(started_at).utc : nil ) end diff --git a/ee/spec/graphql/graphql_triggers_spec.rb b/ee/spec/graphql/graphql_triggers_spec.rb index 4810d0257f0df500f600d70221f49f5841a1a16c..d8460a5e04ccf81c692bb580b6ee1a190f8e844f 100644 --- a/ee/spec/graphql/graphql_triggers_spec.rb +++ b/ee/spec/graphql/graphql_triggers_spec.rb @@ -198,7 +198,8 @@ failed_projects, merge_requests, merge_requests_total, - in_progress + in_progress, + started_at ) end @@ -209,6 +210,7 @@ let(:merge_requests) { 50.0 } let(:merge_requests_total) { 200 } let(:in_progress) { true } + let(:started_at) { Time.at(42) } specify do expect(GitlabSchema.subscriptions).to receive(:trigger).with( @@ -220,7 +222,8 @@ failed_projects: failed_projects, merge_requests_progress: merge_requests, merge_requests_total: merge_requests_total, - in_progress: in_progress + in_progress: in_progress, + started_at: started_at } ).and_call_original diff --git a/ee/spec/graphql/subscriptions/security/policies_sync_updated_spec.rb b/ee/spec/graphql/subscriptions/security/policies_sync_updated_spec.rb index df93db35d0695a00419b68ca7f87373b4ecc36f4..26e4c6773971c24c22e0eeb8c5ec7c00d5ae1681 100644 --- a/ee/spec/graphql/subscriptions/security/policies_sync_updated_spec.rb +++ b/ee/spec/graphql/subscriptions/security/policies_sync_updated_spec.rb @@ -16,6 +16,7 @@ let(:merge_requests_progress) { 50 } let(:merge_requests_total) { 200 } let(:in_progress) { true } + let(:started_at) { 42 } let(:subscribe) { security_policies_sync_updated_subscription(policy_configuration, current_user) } @@ -35,7 +36,8 @@ failed_projects, merge_requests_progress, merge_requests_total, - in_progress) + in_progress, + started_at) end end @@ -51,7 +53,8 @@ "failedProjects" => failed_projects, "mergeRequestsProgress" => merge_requests_progress, "mergeRequestsTotal" => merge_requests_total, - "inProgress" => in_progress + "inProgress" => in_progress, + "startedAt" => Time.at(started_at).iso8601 }) end end diff --git a/ee/spec/lib/security/security_orchestration_policies/policy_sync_state/state_spec.rb b/ee/spec/lib/security/security_orchestration_policies/policy_sync_state/state_spec.rb index daee19cab5938296e941720c45499c97fc305fb6..ead7b87842e29b93c1f7cfcb42f5f8b405784285 100644 --- a/ee/spec/lib/security/security_orchestration_policies/policy_sync_state/state_spec.rb +++ b/ee/spec/lib/security/security_orchestration_policies/policy_sync_state/state_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Security::SecurityOrchestrationPolicies::PolicySyncState::State, :clean_gitlab_redis_shared_state, feature_category: :security_policy_management do + include ActiveSupport::Testing::TimeHelpers + let(:redis) { Gitlab::Redis::SharedState.pool.checkout } let(:merge_request_id) { 1 } @@ -36,11 +38,67 @@ end end + describe '#to_h' do + context 'when sync is not in progress' do + it 'excludes start time' do + expect(state.to_h).to include({ + in_progress: false, + started_at: nil + }) + end + end + + context 'when sync is in progress' do + it 'includes start time' do + freeze_time do + start_time = Time.now.utc + + state.append_projects([project_id]) + + expect(state.to_h).to include({ + in_progress: true, + started_at: start_time + }) + end + end + + context 'when sync is finished' do + it 'excludes start time' do + freeze_time do + state.append_projects([project_id]) + state.finish_project(project_id) + + expect(state.to_h).to include({ + in_progress: false, + started_at: nil + }) + end + end + end + end + end + describe '#start_sync' do it 'marks pending' do expect { state.start_sync }.to change { state.sync_in_progress?(redis) }.from(false).to(true) end + it 'sets start timestamp' do + freeze_time do + expect { state.start_sync }.to change { state.to_h[:started_at] }.from(nil).to(Time.now.utc) + end + end + + it 'does not overwrite existing start timestamp' do + travel_to(1.hour.ago) do + state.start_sync + end + + freeze_time do + expect { state.start_sync }.not_to change { state.to_h[:started_at] } + end + end + context 'with feature disabled' do before do stub_feature_flags(security_policy_sync_propagation_tracking: false) @@ -54,12 +112,20 @@ describe '#finish_sync' do before do - state.start_sync + freeze_time { state.start_sync } + end + + after do + travel_back end it 'removes pending' do expect { state.finish_sync }.to change { state.sync_in_progress?(redis) }.from(true).to(false) end + + it 'removes start timestamp' do + expect { state.finish_sync }.to change { state.to_h[:started_at] }.from(be_a(Time)).to(nil) + end end describe '#append_projects' do @@ -95,6 +161,28 @@ }.from(be_empty) end end + + context 'when starting a new sync' do + it 'sets start timestamp' do + freeze_time do + expect { state.append_projects([project_id]) }.to change { + state.to_h[:started_at] + }.from(nil).to(Time.now.utc) + end + end + end + + context 'when a sync is already in progress' do + it 'does not overwrite existing start timestamp' do + travel_to(1.hour.ago) do + state.start_sync + end + + freeze_time do + expect { state.append_projects([project_id]) }.not_to change { state.to_h[:started_at] } + end + end + end end describe '#finish_project' do @@ -249,6 +337,28 @@ }.from(nil) end end + + context 'when starting a new sync' do + it 'sets the start timestamp' do + freeze_time do + expect { state.start_merge_request(merge_request_id) }.to change { + state.to_h[:started_at] + }.from(nil).to(Time.now.utc) + end + end + end + + context 'when a sync is already in progress' do + it 'does not overwrite the existing timestamp' do + travel_to(1.hour.ago) do + state.start_sync + end + + freeze_time do + expect { state.start_merge_request(merge_request_id) }.not_to change { state.to_h[:started_at] } + end + end + end end describe '#start_merge_request_worker' do @@ -431,13 +541,19 @@ end describe '#clear' do - subject(:clear) { state.clear } - before do - state.append_projects([project_id]) - state.start_merge_request(merge_request_id) + freeze_time do + state.append_projects([project_id]) + state.start_merge_request(merge_request_id) + end end + after do + travel_back + end + + subject(:clear) { state.clear } + it 'resets pending project IDs' do expect { clear }.to change { state.pending_projects }.from(contain_exactly(project_id.to_s)).to(be_empty) end @@ -448,6 +564,10 @@ }.from(contain_exactly(merge_request_id.to_s)).to(be_empty) end + it 'clears start timestamp' do + expect { clear }.to change { state.to_h[:started_at] }.from(be_a(Time)).to(nil) + end + context 'with feature disabled' do before do stub_feature_flags(security_policy_sync_propagation_tracking: false) @@ -473,6 +593,8 @@ subject(:state) { described_class.new(policy_configuration.id) } before do + freeze_time + 10.times do |i| state.append_projects([i]) state.start_merge_request(i) @@ -483,23 +605,25 @@ it 'publishes with correct values at each step' do expect(GraphqlTriggers).to receive(:security_policies_sync_updated).with( policy_configuration, - 10, # project progress: (10 total - 9 pending) / 10 * 100 = 10% - 10, # projects total - [], # no failed projects yet - 0, # merge request progress: (10 total - 10 pending) / 10 * 100 = 0% - 10, # merge requests total, - true # in progress + 10, # project progress: (10 total - 9 pending) / 10 * 100 = 10% + 10, # projects total + [], # no failed projects yet + 0, # merge request progress: (10 total - 10 pending) / 10 * 100 = 0% + 10, # merge requests total, + true, # in progress + Time.now.utc ).ordered state.finish_project(1) expect(GraphqlTriggers).to receive(:security_policies_sync_updated).with( policy_configuration, - 10, # project progress: still 10% - 10, # projects total - [], # no failed projects yet - 10, # merge request progress: (10 total - 9 pending) / 10 * 100 = 10% - 10, # merge requests total, - true # in progress + 10, # project progress: still 10% + 10, # projects total + [], # no failed projects yet + 10, # merge request progress: (10 total - 9 pending) / 10 * 100 = 10% + 10, # merge requests total, + true, # in progress + Time.now.utc ).ordered state.finish_merge_request_worker(1) @@ -510,7 +634,8 @@ ["2"], # failed project 2 10, # merge request progress: still 10% 10, # merge requests total, - true # in progress + true, # in progress + Time.now.utc ).ordered state.fail_project(2) end @@ -523,12 +648,13 @@ expect(GraphqlTriggers).to have_received(:security_policies_sync_updated).with( policy_configuration, - 50, # project progress: (10 - 5) / 10 * 100 = 50% - 10, # projects total - [], # no failed projects - 0, # merge request progress: still 0% - 10, # merge requests total, - true # in progress + 50, # project progress: (10 - 5) / 10 * 100 = 50% + 10, # projects total + [], # no failed projects + 0, # merge request progress: still 0% + 10, # merge requests total, + true, # in progress + Time.now.utc ) end diff --git a/ee/spec/support/helpers/graphql/subscriptions/security/policies_sync_updated/helper.rb b/ee/spec/support/helpers/graphql/subscriptions/security/policies_sync_updated/helper.rb index a36bd3370d518cc508fda36c1320d0d1ae8ab2b5..94a10a9e91b7890842b618af00367843fff63d26 100644 --- a/ee/spec/support/helpers/graphql/subscriptions/security/policies_sync_updated/helper.rb +++ b/ee/spec/support/helpers/graphql/subscriptions/security/policies_sync_updated/helper.rb @@ -32,6 +32,7 @@ def security_policies_sync_updated_subscription_query(policy_configuration) mergeRequestsProgress mergeRequestsTotal inProgress + startedAt } } SUBSCRIPTION