summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlejandro Rodríguez <alejorro70@gmail.com>2017-04-28 13:52:09 -0300
committerAlejandro Rodríguez <alejorro70@gmail.com>2017-05-03 17:37:30 -0300
commitc45341c816d78d51aee84a6068d778b9cbc502c8 (patch)
tree769202931229d2e799bdd0f4a8d7c6e8f9ba8ff8
parentc1e2da9293bb036280c05ee6b99952b067bdc316 (diff)
downloadgitlab-ce-c45341c816d78d51aee84a6068d778b9cbc502c8.tar.gz
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.
-rw-r--r--changelogs/unreleased/29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce.yml4
-rw-r--r--lib/api/helpers/internal_helpers.rb52
-rw-r--r--lib/api/internal.rb10
-rw-r--r--lib/gitlab/gl_repository.rb16
-rw-r--r--lib/gitlab/repo_path.rb29
-rw-r--r--spec/lib/gitlab/gl_repository_spec.rb19
-rw-r--r--spec/lib/gitlab/repo_path_spec.rb24
-rw-r--r--spec/requests/api/helpers/internal_helpers_spec.rb32
-rw-r--r--spec/requests/api/internal_spec.rb77
-rw-r--r--spec/support/matchers/gitlab_git_matchers.rb6
10 files changed, 184 insertions, 85 deletions
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
+ 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
+
+ [project, wiki]
+ end
+
+ def self.strip_storage_path(repo_path, fail_on_not_found: true)
+ result = repo_path
- 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
+ storage = Gitlab.config.repositories.storages.values.find do |params|
+ repo_path.start_with?(params['path'])
end
- if result.nil?
+ 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