modified lib/gitlab/middleware/read_only/controller.rb@@ -99,6 +99,7 @@ def allowlisted_routes # https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/12 def workhorse_passthrough_route? # Calling route_hash may be expensive. Only do it if we think there's a possible match+ Rails.logger.error [request.path, request.post?, request.path.end_with?('.git/git-upload-pack'), route_hash[:controller], route_hash[:action]] return false unless request.post? && request.path.end_with?('.git/git-upload-pack')
Shows:
Started POST "/root/ci-test.git/git-upload-pack/" for ** at 2021-05-12 18:07:28 +0000["/root/ci-test.git/git-upload-pack/", true, false, nil, nil]
This seems to prevent Geo clones as well since the read_only? method will return true in here.
The issue is that request.url and request.path ends with / what breaks the workhorse_passthrough_route?. Until we figure out what changed in %13.11 from %13.10 we can apply the following workaround:
diff --git a/lib/gitlab/middleware/read_only/controller.rb b/lib/gitlab/middleware/read_only/controller.rbindex f79025bccfe..a7340a2f3d2 100644--- a/lib/gitlab/middleware/read_only/controller.rb+++ b/lib/gitlab/middleware/read_only/controller.rb@@ -83,7 +83,7 @@ def last_visited_url end def route_hash- @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {}+ @route_hash ||= Rails.application.routes.recognize_path(request.url.chomp('/'), { method: request.request_method }) rescue {} end def relative_url@@ -100,7 +100,7 @@ def allowlisted_routes def workhorse_passthrough_route? # Calling route_hash may be expensive. Only do it if we think there's a possible match return false unless request.post? &&- request.path.end_with?('.git/git-upload-pack')+ request.path.chomp('/').end_with?('.git/git-upload-pack') ALLOWLISTED_GIT_READ_ONLY_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) end
13.10 behavior for sanity checking, not having the final /:
Started POST "/root/13-10-test.git/git-upload-pack" for *** at 2021-05-12 18:42:46 +0000["/root/13-10-test.git/git-upload-pack", "http://domain.tld/root/13-10-test.git/git-upload-pack", "POST", true, true, "repositories/git_http", "git_upload_pack"]
It looks like it affects git-upload-pack too since we're calling it through PreAuthorizeHandler but with an empty suffix instead of authorize - so I'm thinking it leaves the trailing / in
Reverting the commit and using this to double check it's being called:
Douglas Barbosa Alexandrechanged title from Git clone/pull doesn't work through HTTP(s) with maintenance mode to Git clone/pull doesn't work through HTTP(s)
changed title from Git clone/pull doesn't work through HTTP(s) with maintenance mode to Git clone/pull doesn't work through HTTP(s)
It seems this should have been caught by our E2E tests on staging, but may have slipped by when the Geo secondary site wasn't working after the PG12 upgrade or when it was having certificate issues. @nwestbury to investigate further.
From Slack:
Nick Nguyen 7 hours ago@Nick Westbury What’s the current state of Geo E2E tests? IIRC we had to disable them against master because of issues using Google COS, but were still running them against staging. Should that have caught this? Or did this happen to get through during the window that the Geo node on staging wasn’t working after the PG12 upgrade?Nick Westbury 7 hours agoCorrect they were disabled on master, they are still running on staging though. The secondary Geo site for staging has had certificate issues recently and is currently not loading for me. I’m going to raise an issue for this now. I’ll need to look through all the tests to see if we are missing anything.Nick Westbury 7 hours agoFor running our E2E tests against reference environments, all the pieces to run these against nightly builds are available and i should be able to get this soon. I’m also going to be looking into how we can use GET to build environments from MR which could also help with this.