From 64ce9ab608be99b8d9da619dcfc191fe0b0ea01c Mon Sep 17 00:00:00 2001
From: Sylvester Chin <schin@gitlab.com>
Date: Thu, 18 Jul 2024 08:20:32 +0000
Subject: [PATCH] Guard PATs `last_used_at` updates with exclusive lease
---
.../last_used_service.rb | 30 +++++++++++-
.../use_lease_for_pat_last_used_update.yml | 9 ++++
.../last_used_service_spec.rb | 46 +++++++++++++++++++
3 files changed, 83 insertions(+), 2 deletions(-)
create mode 100644 config/feature_flags/gitlab_com_derisk/use_lease_for_pat_last_used_update.yml
diff --git a/app/services/personal_access_tokens/last_used_service.rb b/app/services/personal_access_tokens/last_used_service.rb
index f9a515a50795..3e8da55bc950 100644
--- a/app/services/personal_access_tokens/last_used_service.rb
+++ b/app/services/personal_access_tokens/last_used_service.rb
@@ -2,6 +2,10 @@
module PersonalAccessTokens
class LastUsedService
+ include ExclusiveLeaseGuard
+
+ LEASE_TIMEOUT = 60.seconds.to_i
+
def initialize(personal_access_token)
@personal_access_token = personal_access_token
end
@@ -14,13 +18,35 @@ def execute
# would be updated when using #touch).
return unless update?
- ::Gitlab::Database::LoadBalancing::Session.without_sticky_writes do
- @personal_access_token.update_column(:last_used_at, Time.zone.now)
+ with_lease do
+ ::Gitlab::Database::LoadBalancing::Session.without_sticky_writes do
+ @personal_access_token.update_column(:last_used_at, Time.zone.now)
+ end
end
end
private
+ def lease_timeout
+ LEASE_TIMEOUT
+ end
+
+ def lease_key
+ @lease_key ||= "pat:last_used_update_lock:#{@personal_access_token.id}"
+ end
+
+ def with_lease
+ return yield unless Feature.enabled?(
+ :use_lease_for_pat_last_used_update,
+ Feature.current_request,
+ type: :gitlab_com_derisk
+ )
+
+ try_obtain_lease do
+ yield
+ end
+ end
+
def update?
return false if ::Gitlab::Database.read_only?
diff --git a/config/feature_flags/gitlab_com_derisk/use_lease_for_pat_last_used_update.yml b/config/feature_flags/gitlab_com_derisk/use_lease_for_pat_last_used_update.yml
new file mode 100644
index 000000000000..349e999bc69e
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/use_lease_for_pat_last_used_update.yml
@@ -0,0 +1,9 @@
+---
+name: use_lease_for_pat_last_used_update
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/468851
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/158577
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/471276
+milestone: '17.2'
+group: group::scalability
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/spec/services/personal_access_tokens/last_used_service_spec.rb b/spec/services/personal_access_tokens/last_used_service_spec.rb
index f450a6b01257..22c52d6a5af8 100644
--- a/spec/services/personal_access_tokens/last_used_service_spec.rb
+++ b/spec/services/personal_access_tokens/last_used_service_spec.rb
@@ -3,6 +3,8 @@
require 'spec_helper'
RSpec.describe PersonalAccessTokens::LastUsedService, feature_category: :system_access do
+ include ExclusiveLeaseHelpers
+
describe '#execute' do
subject { described_class.new(personal_access_token).execute }
@@ -13,12 +15,56 @@
expect { subject }.to change { personal_access_token.last_used_at }
end
+ it 'obtains an exclusive lease before updating' do
+ Gitlab::Redis::SharedState.with do |redis|
+ expect(redis).to receive(:set).with(
+ "#{Gitlab::ExclusiveLease::PREFIX}:pat:last_used_update_lock:#{personal_access_token.id}",
+ anything,
+ nx: true,
+ ex: described_class::LEASE_TIMEOUT
+ ).and_call_original
+ end
+
+ expect { subject }.to change { personal_access_token.last_used_at }
+ end
+
it 'does not run on read-only GitLab instances' do
allow(::Gitlab::Database).to receive(:read_only?).and_return(true)
expect { subject }.not_to change { personal_access_token.last_used_at }
end
+ context 'when lease is already acquired by another process' do
+ let(:lease_key) { "pat:last_used_update_lock:#{personal_access_token.id}" }
+
+ before do
+ stub_exclusive_lease_taken(lease_key, timeout: described_class::LEASE_TIMEOUT)
+ end
+
+ it 'does not update last_used_at' do
+ expect { subject }.not_to change { personal_access_token.last_used_at }
+ end
+ end
+
+ context 'when use_lease_for_pat_last_used_update flag is disabled' do
+ before do
+ stub_feature_flags(use_lease_for_pat_last_used_update: false)
+ end
+
+ it 'does not obtain an exclusive lease before updating' do
+ Gitlab::Redis::SharedState.with do |redis|
+ expect(redis).not_to receive(:set).with(
+ "#{Gitlab::ExclusiveLease::PREFIX}:pat:last_used_update_lock:#{personal_access_token.id}",
+ anything,
+ nx: true,
+ ex: described_class::LEASE_TIMEOUT
+ )
+ end
+
+ expect { subject }.to change { personal_access_token.last_used_at }
+ end
+ end
+
context 'when database load balancing is configured' do
let!(:service) { described_class.new(personal_access_token) }
--
GitLab