Commit 00ac76cc authored by Rémy Coutable's avatar Rémy Coutable

Cache the allowed user IDs for the performance bar, in Redis for 10 minutes

Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 75bd0c3a
module Gitlab module Gitlab
module PerformanceBar module PerformanceBar
ALLOWED_USER_IDS_KEY = 'performance_bar_allowed_user_ids'.freeze
# The time (in seconds) after which a set of allowed user IDs is expired
# automatically.
ALLOWED_USER_IDS_TIME_TO_LIVE = 10.minutes.to_i
def self.enabled?(current_user = nil) def self.enabled?(current_user = nil)
Feature.enabled?(:gitlab_performance_bar, current_user) Feature.enabled?(:gitlab_performance_bar, current_user)
end end
def self.allowed_user?(user) def self.allowed_user?(user)
return false unless allowed_group return false unless allowed_group_name
if RequestStore.active? allowed_user_ids.include?(user.id)
RequestStore.fetch('performance_bar:user_member_of_allowed_group') do
user_member_of_allowed_group?(user)
end
else
user_member_of_allowed_group?(user)
end
end end
def self.allowed_group def self.allowed_group_name
return nil unless Gitlab.config.performance_bar.allowed_group Gitlab.config.performance_bar.allowed_group
end
def self.allowed_user_ids
Gitlab::Redis.with do |redis|
if redis.exists(cache_key)
redis.smembers(cache_key).map(&:to_i)
else
group = Group.find_by_full_path(allowed_group_name)
# Redis#sadd doesn't accept an empty array, but we still want to use
# Redis to let us know that no users are allowed, so we set the
# array to [-1] in this case.
user_ids =
if group
GroupMembersFinder.new(group).execute
.pluck(:user_id).presence || [-1]
else
[-1]
end
redis.multi do
redis.sadd(cache_key, user_ids)
redis.expire(cache_key, ALLOWED_USER_IDS_TIME_TO_LIVE)
end
if RequestStore.active? user_ids
RequestStore.fetch('performance_bar:allowed_group') do
Group.find_by_full_path(Gitlab.config.performance_bar.allowed_group)
end end
else
Group.find_by_full_path(Gitlab.config.performance_bar.allowed_group)
end end
end end
def self.user_member_of_allowed_group?(user) def self.cache_key
GroupMembersFinder.new(allowed_group).execute.exists?(user_id: user.id) "#{ALLOWED_USER_IDS_KEY}:#{allowed_group_name}"
end end
end end
end end
...@@ -25,6 +25,27 @@ describe Gitlab::PerformanceBar do ...@@ -25,6 +25,27 @@ describe Gitlab::PerformanceBar do
end end
end end
shared_examples 'allowed user IDs are cached in Redis for 10 minutes' do
before do
# Warm the Redis cache
described_class.allowed_user?(user)
end
it 'caches the allowed user IDs in Redis', :redis do
expect do
expect(described_class.allowed_user?(user)).to be_truthy
end.not_to exceed_query_limit(0)
end
it 'caches the allowed user IDs for 10 minutes', :redis do
ttl_cached_user_ids = Gitlab::Redis.with do |redis|
redis.ttl(described_class.cache_key)
end
expect(ttl_cached_user_ids).to be <= 10.minutes
end
end
describe '.allowed_user?' do describe '.allowed_user?' do
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -45,6 +66,8 @@ describe Gitlab::PerformanceBar do ...@@ -45,6 +66,8 @@ describe Gitlab::PerformanceBar do
it 'returns false' do it 'returns false' do
expect(described_class.allowed_user?(user)).to be_falsy expect(described_class.allowed_user?(user)).to be_falsy
end end
it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
end end
context 'when user is a member of the allowed group' do context 'when user is a member of the allowed group' do
...@@ -55,28 +78,8 @@ describe Gitlab::PerformanceBar do ...@@ -55,28 +78,8 @@ describe Gitlab::PerformanceBar do
it 'returns true' do it 'returns true' do
expect(described_class.allowed_user?(user)).to be_truthy expect(described_class.allowed_user?(user)).to be_truthy
end end
end
end
end
describe '.allowed_group' do it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
before do
stub_performance_bar_setting(allowed_group: 'my-group')
end
context 'when allowed group does not exist' do
it 'returns false' do
expect(described_class.allowed_group).to be_falsy
end
end
context 'when allowed group exists' do
let!(:my_group) { create(:group, path: 'my-group') }
context 'when user is not a member of the allowed group' do
it 'returns the group' do
expect(described_class.allowed_group).to eq(my_group)
end
end end
end end
...@@ -85,21 +88,22 @@ describe Gitlab::PerformanceBar do ...@@ -85,21 +88,22 @@ describe Gitlab::PerformanceBar do
before do before do
create(:group, path: 'my-group') create(:group, path: 'my-group')
nested_my_group.add_developer(user)
stub_performance_bar_setting(allowed_group: 'my-org/my-group') stub_performance_bar_setting(allowed_group: 'my-org/my-group')
end end
it 'returns the nested group' do it 'returns the nested group' do
expect(described_class.allowed_group).to eq(nested_my_group) expect(described_class.allowed_user?(user)).to be_truthy
end end
end end
context 'when a nested group has the same path', :nested_groups do context 'when a nested group has the same path', :nested_groups do
before do before do
create(:group, :nested, path: 'my-group') create(:group, :nested, path: 'my-group').add_developer(user)
end end
it 'returns false' do it 'returns false' do
expect(described_class.allowed_group).to be_falsy expect(described_class.allowed_user?(user)).to be_falsy
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment