From c45341c816d78d51aee84a6068d778b9cbc502c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Fri, 28 Apr 2017 13:52:09 -0300 Subject: [PATCH 1/3] Generate and handle a gl_repository param to pass around components This new param allows us to share project information between components that don't share or don't have access to the same filesystem mountpoints, for example between Gitaly and Rails or between Rails and Gitlab-Shell hooks. The previous parameters are still supported, but if found, gl_repository is prefered. The old parameters should be deprecated once all components support the new format. --- ...onger-send-absolute-paths-to-gitlab-ce.yml | 4 + lib/api/helpers/internal_helpers.rb | 52 ++++--------- lib/api/internal.rb | 10 ++- lib/gitlab/gl_repository.rb | 16 ++++ lib/gitlab/repo_path.rb | 31 +++++--- spec/lib/gitlab/gl_repository_spec.rb | 19 +++++ spec/lib/gitlab/repo_path_spec.rb | 24 ++++++ .../api/helpers/internal_helpers_spec.rb | 32 -------- spec/requests/api/internal_spec.rb | 77 ++++++++++++++++++- spec/support/matchers/gitlab_git_matchers.rb | 6 ++ 10 files changed, 185 insertions(+), 86 deletions(-) create mode 100644 changelogs/unreleased/29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce.yml create mode 100644 lib/gitlab/gl_repository.rb create mode 100644 spec/lib/gitlab/gl_repository_spec.rb delete mode 100644 spec/requests/api/helpers/internal_helpers_spec.rb create mode 100644 spec/support/matchers/gitlab_git_matchers.rb diff --git a/changelogs/unreleased/29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce.yml b/changelogs/unreleased/29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce.yml new file mode 100644 index 00000000000..1df8f695ef1 --- /dev/null +++ b/changelogs/unreleased/29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce.yml @@ -0,0 +1,4 @@ +--- +title: Generate and handle a gl_repository param to pass around components +merge_request: 10992 +author: diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 718f936a1fc..264df7271a3 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -1,48 +1,14 @@ module API module Helpers module InternalHelpers - # Project paths may be any of the following: - # * /repository/storage/path/namespace/project - # * /namespace/project - # * namespace/project - # - # In addition, they may have a '.git' extension and multiple namespaces - # - # Transform all these cases to 'namespace/project' - def clean_project_path(project_path, storages = Gitlab.config.repositories.storages.values) - project_path = project_path.sub(/\.git\z/, '') - - storages.each do |storage| - storage_path = File.expand_path(storage['path']) - - if project_path.start_with?(storage_path) - project_path = project_path.sub(storage_path, '') - break - end - end - - project_path.sub(/\A\//, '') - end - - def project_path - @project_path ||= clean_project_path(params[:project]) - end - def wiki? - @wiki ||= project_path.end_with?('.wiki') && - !Project.find_by_full_path(project_path) + set_project unless defined?(@wiki) + @wiki end def project - @project ||= begin - # Check for *.wiki repositories. - # Strip out the .wiki from the pathname before finding the - # project. This applies the correct project permissions to - # the wiki repository as well. - project_path.chomp!('.wiki') if wiki? - - Project.find_by_full_path(project_path) - end + set_project unless defined?(@project) + @project end def ssh_authentication_abilities @@ -66,6 +32,16 @@ module API ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action]) end + + private + + def set_project + if params[:gl_repository] + @project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository]) + else + @project, @wiki = Gitlab::RepoPath.parse(params[:project]) + end + end end end end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ebed26dd178..ddb2047f686 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -42,6 +42,10 @@ module API if access_status.status log_user_activity(actor) + # Project id to pass between components that don't share/don't have + # access to the same filesystem mounts + response[:gl_repository] = "#{wiki? ? 'wiki' : 'project'}-#{project.id}" + # Return the repository full path so that gitlab-shell has it when # handling ssh commands response[:repository_path] = @@ -134,11 +138,9 @@ module API return unless Gitlab::GitalyClient.enabled? - relative_path = Gitlab::RepoPath.strip_storage_path(params[:repo_path]) - project = Project.find_by_full_path(relative_path.sub(/\.(git|wiki)\z/, '')) - begin - Gitlab::GitalyClient::Notifications.new(project.repository).post_receive + repository = wiki? ? project.wiki.repository : project.repository + Gitlab::GitalyClient::Notifications.new(repository.raw_repository).post_receive rescue GRPC::Unavailable => e render_api_error!(e, 500) end diff --git a/lib/gitlab/gl_repository.rb b/lib/gitlab/gl_repository.rb new file mode 100644 index 00000000000..6997546049e --- /dev/null +++ b/lib/gitlab/gl_repository.rb @@ -0,0 +1,16 @@ +module Gitlab + module GlRepository + def self.parse(gl_repository) + match_data = /\A(project|wiki)-([1-9][0-9]*)\z/.match(gl_repository) + unless match_data + raise ArgumentError, "Invalid GL Repository \"#{gl_repository}\"" + end + + type, id = match_data.captures + project = Project.find_by(id: id) + wiki = type == 'wiki' + + [project, wiki] + end + end +end diff --git a/lib/gitlab/repo_path.rb b/lib/gitlab/repo_path.rb index 4b1d828c45c..878e03f61d7 100644 --- a/lib/gitlab/repo_path.rb +++ b/lib/gitlab/repo_path.rb @@ -2,18 +2,29 @@ module Gitlab module RepoPath NotFoundError = Class.new(StandardError) - def self.strip_storage_path(repo_path) - result = nil - - Gitlab.config.repositories.storages.values.each do |params| - storage_path = params['path'] - if repo_path.start_with?(storage_path) - result = repo_path.sub(storage_path, '') - break - end + def self.parse(repo_path) + project_path = strip_storage_path(repo_path.sub(/\.git\z/, ''), fail_on_not_found: false) + project = Project.find_by_full_path(project_path) + if project_path.end_with?('.wiki') && !project + project = Project.find_by_full_path(project_path.chomp('.wiki')) + wiki = true + else + wiki = false end - if result.nil? + [project, wiki] + end + + def self.strip_storage_path(repo_path, fail_on_not_found: true) + result = repo_path + + storage = Gitlab.config.repositories.storages.values.find do |params| + repo_path.start_with?(params['path']) + end + + if storage + result = result.sub(storage['path'], '') + elsif fail_on_not_found raise NotFoundError.new("No known storage path matches #{repo_path.inspect}") end diff --git a/spec/lib/gitlab/gl_repository_spec.rb b/spec/lib/gitlab/gl_repository_spec.rb new file mode 100644 index 00000000000..ac3558ab386 --- /dev/null +++ b/spec/lib/gitlab/gl_repository_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe ::Gitlab::GlRepository do + describe '.parse' do + set(:project) { create(:project) } + + it 'parses a project gl_repository' do + expect(described_class.parse("project-#{project.id}")).to eq([project, false]) + end + + it 'parses a wiki gl_repository' do + expect(described_class.parse("wiki-#{project.id}")).to eq([project, true]) + end + + it 'throws an argument error on an invalid gl_repository' do + expect { described_class.parse("badformat-#{project.id}") }.to raise_error(ArgumentError) + end + end +end diff --git a/spec/lib/gitlab/repo_path_spec.rb b/spec/lib/gitlab/repo_path_spec.rb index 0fb5d7646f2..f94c9c2e315 100644 --- a/spec/lib/gitlab/repo_path_spec.rb +++ b/spec/lib/gitlab/repo_path_spec.rb @@ -1,6 +1,30 @@ require 'spec_helper' describe ::Gitlab::RepoPath do + describe '.parse' do + set(:project) { create(:project) } + + it 'parses a full repository path' do + expect(described_class.parse(project.repository.path)).to eq([project, false]) + end + + it 'parses a full wiki path' do + expect(described_class.parse(project.wiki.repository.path)).to eq([project, true]) + end + + it 'parses a relative repository path' do + expect(described_class.parse(project.full_path + '.git')).to eq([project, false]) + end + + it 'parses a relative wiki path' do + expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, true]) + end + + it 'parses a relative path starting with /' do + expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, false]) + end + end + describe '.strip_storage_path' do before do allow(Gitlab.config.repositories).to receive(:storages).and_return({ diff --git a/spec/requests/api/helpers/internal_helpers_spec.rb b/spec/requests/api/helpers/internal_helpers_spec.rb deleted file mode 100644 index db716b340f1..00000000000 --- a/spec/requests/api/helpers/internal_helpers_spec.rb +++ /dev/null @@ -1,32 +0,0 @@ -require 'spec_helper' - -describe ::API::Helpers::InternalHelpers do - include described_class - - describe '.clean_project_path' do - project = 'namespace/project' - namespaced = File.join('namespace2', project) - - { - File.join(Dir.pwd, project) => project, - File.join(Dir.pwd, namespaced) => namespaced, - project => project, - namespaced => namespaced, - project + '.git' => project, - namespaced + '.git' => namespaced, - "/" + project => project, - "/" + namespaced => namespaced, - }.each do |project_path, expected| - context project_path do - # Relative and absolute storage paths, with and without trailing / - ['.', './', Dir.pwd, Dir.pwd + '/'].each do |storage_path| - context "storage path is #{storage_path}" do - subject { clean_project_path(project_path, [{ 'path' => storage_path }]) } - - it { is_expected.to eq(expected) } - end - end - end - end - end -end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 429f1a4e375..2ceb4648ece 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -180,6 +180,7 @@ describe API::Internal do expect(response).to have_http_status(200) expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo) + expect(json_response["gl_repository"]).to eq("wiki-#{project.id}") expect(user).not_to have_an_activity_record end end @@ -191,6 +192,7 @@ describe API::Internal do expect(response).to have_http_status(200) expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo) + expect(json_response["gl_repository"]).to eq("wiki-#{project.id}") expect(user).to have_an_activity_record end end @@ -202,6 +204,7 @@ describe API::Internal do expect(response).to have_http_status(200) expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) + expect(json_response["gl_repository"]).to eq("project-#{project.id}") expect(user).to have_an_activity_record end end @@ -213,6 +216,7 @@ describe API::Internal do expect(response).to have_http_status(200) expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) + expect(json_response["gl_repository"]).to eq("project-#{project.id}") expect(user).not_to have_an_activity_record end @@ -223,6 +227,7 @@ describe API::Internal do expect(response).to have_http_status(200) expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) + expect(json_response["gl_repository"]).to eq("project-#{project.id}") end end @@ -233,6 +238,7 @@ describe API::Internal do expect(response).to have_http_status(200) expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq(project.repository.path_to_repo) + expect(json_response["gl_repository"]).to eq("project-#{project.id}") end end end @@ -444,18 +450,39 @@ describe API::Internal do expect(json_response).to eq([]) end + + context 'with a gl_repository parameter' do + let(:gl_repository) { "project-#{project.id}" } + + it 'returns link to create new merge request' do + get api("/internal/merge_request_urls?gl_repository=#{gl_repository}&changes=#{changes}"), secret_token: secret_token + + expect(json_response).to match [{ + "branch_name" => "new_branch", + "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch", + "new_merge_request" => true + }] + end + end end describe 'POST /notify_post_receive' do let(:valid_params) do - { repo_path: project.repository.path, secret_token: secret_token } + { project: project.repository.path, secret_token: secret_token } + end + + let(:valid_wiki_params) do + { project: project.wiki.repository.path, secret_token: secret_token } end before do allow(Gitlab.config.gitaly).to receive(:enabled).and_return(true) end - it "calls the Gitaly client if it's enabled" do + it "calls the Gitaly client with the project's repository" do + expect(Gitlab::GitalyClient::Notifications). + to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)). + and_call_original expect_any_instance_of(Gitlab::GitalyClient::Notifications). to receive(:post_receive) @@ -464,6 +491,18 @@ describe API::Internal do expect(response).to have_http_status(200) end + it "calls the Gitaly client with the wiki's repository if it's a wiki" do + expect(Gitlab::GitalyClient::Notifications). + to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)). + and_call_original + expect_any_instance_of(Gitlab::GitalyClient::Notifications). + to receive(:post_receive) + + post api("/internal/notify_post_receive"), valid_wiki_params + + expect(response).to have_http_status(200) + end + it "returns 500 if the gitaly call fails" do expect_any_instance_of(Gitlab::GitalyClient::Notifications). to receive(:post_receive).and_raise(GRPC::Unavailable) @@ -472,6 +511,40 @@ describe API::Internal do expect(response).to have_http_status(500) end + + context 'with a gl_repository parameter' do + let(:valid_params) do + { gl_repository: "project-#{project.id}", secret_token: secret_token } + end + + let(:valid_wiki_params) do + { gl_repository: "wiki-#{project.id}", secret_token: secret_token } + end + + it "calls the Gitaly client with the project's repository" do + expect(Gitlab::GitalyClient::Notifications). + to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)). + and_call_original + expect_any_instance_of(Gitlab::GitalyClient::Notifications). + to receive(:post_receive) + + post api("/internal/notify_post_receive"), valid_params + + expect(response).to have_http_status(200) + end + + it "calls the Gitaly client with the wiki's repository if it's a wiki" do + expect(Gitlab::GitalyClient::Notifications). + to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)). + and_call_original + expect_any_instance_of(Gitlab::GitalyClient::Notifications). + to receive(:post_receive) + + post api("/internal/notify_post_receive"), valid_wiki_params + + expect(response).to have_http_status(200) + end + end end def project_with_repo_path(path) diff --git a/spec/support/matchers/gitlab_git_matchers.rb b/spec/support/matchers/gitlab_git_matchers.rb new file mode 100644 index 00000000000..c840cd4bf2d --- /dev/null +++ b/spec/support/matchers/gitlab_git_matchers.rb @@ -0,0 +1,6 @@ +RSpec::Matchers.define :gitlab_git_repository_with do |values| + match do |actual| + actual.is_a?(Gitlab::Git::Repository) && + values.all? { |k, v| actual.send(k) == v } + end +end From 5249157552bbf4cbf279b1decbd4a0e90e056077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 2 May 2017 18:15:12 -0300 Subject: [PATCH 2/3] Allow gl-repository strings as project identifiers in PostReceive worker --- app/workers/post_receive.rb | 31 ++++++++++++---------- lib/gitlab/git_post_receive.rb | 29 +++------------------ spec/workers/post_receive_spec.rb | 43 +++++++++++++++++++++++-------- 3 files changed, 53 insertions(+), 50 deletions(-) diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 015a41b6e82..39f03983821 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -2,27 +2,24 @@ class PostReceive include Sidekiq::Worker include DedicatedSidekiqQueue - def perform(repo_path, identifier, changes) - repo_relative_path = Gitlab::RepoPath.strip_storage_path(repo_path) + def perform(project_identifier, identifier, changes) + project, is_wiki = parse_project_identifier(project_identifier) + + if project.nil? + log("Triggered hook for non-existing project with identifier \"#{project_identifier}\"") + return false + end changes = Base64.decode64(changes) unless changes.include?(' ') # Use Sidekiq.logger so arguments can be correlated with execution # time and thread ID's. Sidekiq.logger.info "changes: #{changes.inspect}" if ENV['SIDEKIQ_LOG_ARGUMENTS'] - post_received = Gitlab::GitPostReceive.new(repo_relative_path, identifier, changes) + post_received = Gitlab::GitPostReceive.new(project, identifier, changes) - if post_received.project.nil? - log("Triggered hook for non-existing project with full path \"#{repo_relative_path}\"") - return false - end - - if post_received.wiki? + if is_wiki # Nothing defined here yet. - elsif post_received.regular_project? - process_project_changes(post_received) else - log("Triggered hook for unidentifiable repository type with full path \"#{repo_relative_path}\"") - false + process_project_changes(post_received) end end @@ -47,6 +44,14 @@ class PostReceive private + def parse_project_identifier(project_identifier) + if project_identifier.start_with?('/') + Gitlab::RepoPath.parse(project_identifier) + else + Gitlab::GlRepository.parse(project_identifier) + end + end + def log(message) Gitlab::GitLogger.error("POST-RECEIVE: #{message}") end diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb index 6babea144c7..0e14253ab4e 100644 --- a/lib/gitlab/git_post_receive.rb +++ b/lib/gitlab/git_post_receive.rb @@ -1,25 +1,12 @@ module Gitlab class GitPostReceive include Gitlab::Identifier - attr_reader :repo_path, :identifier, :changes, :project + attr_reader :project, :identifier, :changes - def initialize(repo_path, identifier, changes) - repo_path.gsub!(/\.git\z/, '') - repo_path.gsub!(/\A\//, '') - - @repo_path = repo_path + def initialize(project, identifier, changes) + @project = project @identifier = identifier @changes = deserialize_changes(changes) - - retrieve_project_and_type - end - - def wiki? - @type == :wiki - end - - def regular_project? - @type == :project end def identify(revision) @@ -28,16 +15,6 @@ module Gitlab private - def retrieve_project_and_type - @type = :project - @project = Project.find_by_full_path(@repo_path) - - if @repo_path.end_with?('.wiki') && !@project - @type = :wiki - @project = Project.find_by_full_path(@repo_path.gsub(/\.wiki\z/, '')) - end - end - def deserialize_changes(changes) changes = utf8_encode_changes(changes) changes.lines diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 5ab3c4a0e34..0260416dbe2 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -5,6 +5,7 @@ describe PostReceive do let(:wrongly_encoded_changes) { changes.encode("ISO-8859-1").force_encoding("UTF-8") } let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) } let(:project) { create(:project, :repository) } + let(:project_identifier) { "project-#{project.id}" } let(:key) { create(:key, user: project.owner) } let(:key_id) { key.shell_id } @@ -14,6 +15,26 @@ describe PostReceive do end end + context 'with a non-existing project' do + let(:project_identifier) { "project-123456789" } + let(:error_message) do + "Triggered hook for non-existing project with identifier \"#{project_identifier}\"" + end + + it "returns false and logs an error" do + expect(Gitlab::GitLogger).to receive(:error).with("POST-RECEIVE: #{error_message}") + expect(described_class.new.perform(project_identifier, key_id, base64_changes)).to be(false) + end + end + + context "with an absolute path as the project identifier" do + it "searches the project by full path" do + expect(Project).to receive(:find_by_full_path).with(project.full_path).and_call_original + + described_class.new.perform(pwd(project), key_id, base64_changes) + end + end + describe "#process_project_changes" do before do allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner) @@ -25,7 +46,7 @@ describe PostReceive do it "calls GitTagPushService" do expect_any_instance_of(GitPushService).to receive(:execute).and_return(true) expect_any_instance_of(GitTagPushService).not_to receive(:execute) - described_class.new.perform(pwd(project), key_id, base64_changes) + described_class.new.perform(project_identifier, key_id, base64_changes) end end @@ -35,7 +56,7 @@ describe PostReceive do it "calls GitTagPushService" do expect_any_instance_of(GitPushService).not_to receive(:execute) expect_any_instance_of(GitTagPushService).to receive(:execute).and_return(true) - described_class.new.perform(pwd(project), key_id, base64_changes) + described_class.new.perform(project_identifier, key_id, base64_changes) end end @@ -45,12 +66,12 @@ describe PostReceive do it "does not call any of the services" do expect_any_instance_of(GitPushService).not_to receive(:execute) expect_any_instance_of(GitTagPushService).not_to receive(:execute) - described_class.new.perform(pwd(project), key_id, base64_changes) + described_class.new.perform(project_identifier, key_id, base64_changes) end end context "gitlab-ci.yml" do - subject { described_class.new.perform(pwd(project), key_id, base64_changes) } + subject { described_class.new.perform(project_identifier, key_id, base64_changes) } context "creates a Ci::Pipeline for every change" do before do @@ -74,8 +95,8 @@ describe PostReceive do context "webhook" do it "fetches the correct project" do - expect(Project).to receive(:find_by_full_path).with(project.path_with_namespace).and_return(project) - described_class.new.perform(pwd(project), key_id, base64_changes) + expect(Project).to receive(:find_by).with(id: project.id.to_s) + described_class.new.perform(project_identifier, key_id, base64_changes) end it "does not run if the author is not in the project" do @@ -85,22 +106,22 @@ describe PostReceive do expect(project).not_to receive(:execute_hooks) - expect(described_class.new.perform(pwd(project), key_id, base64_changes)).to be_falsey + expect(described_class.new.perform(project_identifier, key_id, base64_changes)).to be_falsey end it "asks the project to trigger all hooks" do - allow(Project).to receive(:find_by_full_path).and_return(project) + allow(Project).to receive(:find_by).and_return(project) expect(project).to receive(:execute_hooks).twice expect(project).to receive(:execute_services).twice - described_class.new.perform(pwd(project), key_id, base64_changes) + described_class.new.perform(project_identifier, key_id, base64_changes) end it "enqueues a UpdateMergeRequestsWorker job" do - allow(Project).to receive(:find_by_full_path).and_return(project) + allow(Project).to receive(:find_by).and_return(project) expect(UpdateMergeRequestsWorker).to receive(:perform_async).with(project.id, project.owner.id, any_args) - described_class.new.perform(pwd(project), key_id, base64_changes) + described_class.new.perform(project_identifier, key_id, base64_changes) end end From 8bc381db90c92bca6ba868d1588af1ad1a41073b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 3 May 2017 18:07:54 -0300 Subject: [PATCH 3/3] Pass GL_REPOSITORY in Workhorse responses --- app/controllers/projects/git_http_controller.rb | 2 +- app/workers/post_receive.rb | 7 +++++++ lib/api/internal.rb | 2 +- lib/gitlab/gl_repository.rb | 4 ++++ lib/gitlab/workhorse.rb | 6 ++++-- spec/lib/gitlab/workhorse_spec.rb | 17 +++++++++++++++-- 6 files changed, 32 insertions(+), 6 deletions(-) diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 10adddb4636..9e4edcae101 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -59,7 +59,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController def render_ok set_workhorse_internal_api_content_type - render json: Gitlab::Workhorse.git_http_ok(repository, user, action_name) + render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name) end def render_http_not_allowed diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 39f03983821..127d8dfbb61 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -44,6 +44,13 @@ class PostReceive private + # To maintain backwards compatibility, we accept both gl_repository or + # repository paths as project identifiers. Our plan is to migrate to + # gl_repository only with the following plan: + # 9.2: Handle both possible values. Keep Gitlab-Shell sending only repo paths + # 9.3 (or patch release): Make GitLab Shell pass gl_repository if present + # 9.4 (or patch release): Make GitLab Shell always pass gl_repository + # 9.5 (or patch release): Handle only gl_repository as project identifier on this method def parse_project_identifier(project_identifier) if project_identifier.start_with?('/') Gitlab::RepoPath.parse(project_identifier) diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ddb2047f686..2a11790b215 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -44,7 +44,7 @@ module API # Project id to pass between components that don't share/don't have # access to the same filesystem mounts - response[:gl_repository] = "#{wiki? ? 'wiki' : 'project'}-#{project.id}" + response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?) # Return the repository full path so that gitlab-shell has it when # handling ssh commands diff --git a/lib/gitlab/gl_repository.rb b/lib/gitlab/gl_repository.rb index 6997546049e..07c0abcce23 100644 --- a/lib/gitlab/gl_repository.rb +++ b/lib/gitlab/gl_repository.rb @@ -1,5 +1,9 @@ module Gitlab module GlRepository + def self.gl_repository(project, is_wiki) + "#{is_wiki ? 'wiki' : 'project'}-#{project.id}" + end + def self.parse(gl_repository) match_data = /\A(project|wiki)-([1-9][0-9]*)\z/.match(gl_repository) unless match_data diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index c551f939df1..8c5ad01e8c2 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -16,15 +16,17 @@ module Gitlab SECRET_LENGTH = 32 class << self - def git_http_ok(repository, user, action) + def git_http_ok(repository, is_wiki, user, action) + project = repository.project repo_path = repository.path_to_repo params = { GL_ID: Gitlab::GlId.gl_id(user), + GL_REPOSITORY: Gitlab::GlRepository.gl_repository(project, is_wiki), RepoPath: repo_path, } if Gitlab.config.gitaly.enabled - address = Gitlab::GitalyClient.get_address(repository.project.repository_storage) + address = Gitlab::GitalyClient.get_address(project.repository_storage) params[:Repository] = repository.gitaly_repository.to_h feature_enabled = case action.to_s diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index b703e9808a8..beb1791a429 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -181,10 +181,23 @@ describe Gitlab::Workhorse, lib: true do let(:user) { create(:user) } let(:repo_path) { repository.path_to_repo } let(:action) { 'info_refs' } + let(:params) do + { GL_ID: "user-#{user.id}", GL_REPOSITORY: "project-#{project.id}", RepoPath: repo_path } + end - subject { described_class.git_http_ok(repository, user, action) } + subject { described_class.git_http_ok(repository, false, user, action) } - it { expect(subject).to include({ GL_ID: "user-#{user.id}", RepoPath: repo_path }) } + it { expect(subject).to include(params) } + + context 'when is_wiki' do + let(:params) do + { GL_ID: "user-#{user.id}", GL_REPOSITORY: "wiki-#{project.id}", RepoPath: repo_path } + end + + subject { described_class.git_http_ok(repository, true, user, action) } + + it { expect(subject).to include(params) } + end context 'when Gitaly is enabled' do let(:gitaly_params) do