Commit 0876b460 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'memoize_shell_secret_token' into 'master'

Memoize Github::Shell's secret token

## What does this MR do?

`API::Helpers#secret_token` was reading the secret file on every invocation. This MR reads the file in the `gitlab_shell_secret_token.rb` initializer and saves it as a class variable at `Gitlab::Shell.secret_token`

## Are there points in the code the reviewer needs to double check?

 - I'm not sure if the use of `cattr_accessor` is the best approach, or if should be moved into the `class << self` block?
 - Should `API::Helpers#secret_token` be removed in favor of using `Gitlab::Shell.secret_token`?

## Why was this MR needed?

Performance optimization.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/22510

See merge request !6599
parents 1be15162 1c462cf7
...@@ -48,6 +48,7 @@ v 8.13.0 (unreleased) ...@@ -48,6 +48,7 @@ v 8.13.0 (unreleased)
- Prevent flash alert text from being obscured when container is fluid - Prevent flash alert text from being obscured when container is fluid
- Append issue template to existing description !6149 (Joseph Frazier) - Append issue template to existing description !6149 (Joseph Frazier)
- Trending projects now only show public projects and the list of projects is cached for a day - Trending projects now only show public projects and the list of projects is cached for a day
- Memoize Gitlab Shell's secret token (!6599, Justin DiPierro)
- Revoke button in Applications Settings underlines on hover. - Revoke button in Applications Settings underlines on hover.
- Use higher size on Gitlab::Redis connection pool on Sidekiq servers - Use higher size on Gitlab::Redis connection pool on Sidekiq servers
- Add missing values to linter !6276 (Katarzyna Kobierska Ula Budziszewska) - Add missing values to linter !6276 (Katarzyna Kobierska Ula Budziszewska)
......
Gitlab::Shell.new.generate_and_link_secret_token Gitlab::Shell.ensure_secret_token!
...@@ -433,7 +433,7 @@ module API ...@@ -433,7 +433,7 @@ module API
end end
def secret_token def secret_token
File.read(Gitlab.config.gitlab_shell.secret_file).chomp Gitlab::Shell.secret_token
end end
def send_git_blob(repository, blob) def send_git_blob(repository, blob)
......
...@@ -17,6 +17,18 @@ module Gitlab ...@@ -17,6 +17,18 @@ module Gitlab
end end
class << self class << self
def secret_token
@secret_token ||= begin
File.read(Gitlab.config.gitlab_shell.secret_file).chomp
end
end
def ensure_secret_token!
return if File.exist?(File.join(Gitlab.config.gitlab_shell.path, '.gitlab_shell_secret'))
generate_and_link_secret_token
end
def version_required def version_required
@version_required ||= File.read(Rails.root. @version_required ||= File.read(Rails.root.
join('GITLAB_SHELL_VERSION')).strip join('GITLAB_SHELL_VERSION')).strip
...@@ -25,6 +37,25 @@ module Gitlab ...@@ -25,6 +37,25 @@ module Gitlab
def strip_key(key) def strip_key(key)
key.split(/ /)[0, 2].join(' ') key.split(/ /)[0, 2].join(' ')
end end
private
# Create (if necessary) and link the secret token file
def generate_and_link_secret_token
secret_file = Gitlab.config.gitlab_shell.secret_file
shell_path = Gitlab.config.gitlab_shell.path
unless File.size?(secret_file)
# Generate a new token of 16 random hexadecimal characters and store it in secret_file.
token = SecureRandom.hex(16)
File.write(secret_file, token)
end
link_path = File.join(shell_path, '.gitlab_shell_secret')
if File.exist?(shell_path) && !File.exist?(link_path)
FileUtils.symlink(secret_file, link_path)
end
end
end end
# Init new repository # Init new repository
...@@ -201,21 +232,6 @@ module Gitlab ...@@ -201,21 +232,6 @@ module Gitlab
File.exist?(full_path(storage, dir_name)) File.exist?(full_path(storage, dir_name))
end end
# Create (if necessary) and link the secret token file
def generate_and_link_secret_token
secret_file = Gitlab.config.gitlab_shell.secret_file
unless File.size?(secret_file)
# Generate a new token of 16 random hexadecimal characters and store it in secret_file.
token = SecureRandom.hex(16)
File.write(secret_file, token)
end
link_path = File.join(gitlab_shell_path, '.gitlab_shell_secret')
if File.exist?(gitlab_shell_path) && !File.exist?(link_path)
FileUtils.symlink(secret_file, link_path)
end
end
protected protected
def gitlab_shell_path def gitlab_shell_path
......
...@@ -78,7 +78,7 @@ namespace :gitlab do ...@@ -78,7 +78,7 @@ namespace :gitlab do
f.puts "PATH=#{ENV['PATH']}" f.puts "PATH=#{ENV['PATH']}"
end end
Gitlab::Shell.new.generate_and_link_secret_token Gitlab::Shell.ensure_secret_token!
end end
desc "GitLab | Setup gitlab-shell" desc "GitLab | Setup gitlab-shell"
......
...@@ -22,15 +22,15 @@ describe Gitlab::Shell, lib: true do ...@@ -22,15 +22,15 @@ describe Gitlab::Shell, lib: true do
it { expect(gitlab_shell.url_to_repo('diaspora')).to eq(Gitlab.config.gitlab_shell.ssh_path_prefix + "diaspora.git") } it { expect(gitlab_shell.url_to_repo('diaspora')).to eq(Gitlab.config.gitlab_shell.ssh_path_prefix + "diaspora.git") }
describe 'generate_and_link_secret_token' do describe 'memoized secret_token' do
let(:secret_file) { 'tmp/tests/.secret_shell_test' } let(:secret_file) { 'tmp/tests/.secret_shell_test' }
let(:link_file) { 'tmp/tests/shell-secret-test/.gitlab_shell_secret' } let(:link_file) { 'tmp/tests/shell-secret-test/.gitlab_shell_secret' }
before do before do
allow(Gitlab.config.gitlab_shell).to receive(:path).and_return('tmp/tests/shell-secret-test')
allow(Gitlab.config.gitlab_shell).to receive(:secret_file).and_return(secret_file) allow(Gitlab.config.gitlab_shell).to receive(:secret_file).and_return(secret_file)
allow(Gitlab.config.gitlab_shell).to receive(:path).and_return('tmp/tests/shell-secret-test')
FileUtils.mkdir('tmp/tests/shell-secret-test') FileUtils.mkdir('tmp/tests/shell-secret-test')
gitlab_shell.generate_and_link_secret_token Gitlab::Shell.ensure_secret_token!
end end
after do after do
...@@ -39,7 +39,10 @@ describe Gitlab::Shell, lib: true do ...@@ -39,7 +39,10 @@ describe Gitlab::Shell, lib: true do
end end
it 'creates and links the secret token file' do it 'creates and links the secret token file' do
secret_token = Gitlab::Shell.secret_token
expect(File.exist?(secret_file)).to be(true) expect(File.exist?(secret_file)).to be(true)
expect(File.read(secret_file).chomp).to eq(secret_token)
expect(File.symlink?(link_file)).to be(true) expect(File.symlink?(link_file)).to be(true)
expect(File.readlink(link_file)).to eq(secret_file) expect(File.readlink(link_file)).to eq(secret_file)
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