From af7a9460523ed7d8c730fb60cb7d714d97daba77 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Wed, 28 Feb 2024 14:15:48 +0100 Subject: [PATCH 1/5] Poc: HTTP/2 endpoint for fetching gRPC git data from Gitaly --- .../repositories/git_ssh_controller.rb | 37 +++++++++++++ .../experiment/git_via_workhorse.yml | 9 +++ config/routes/git_http.rb | 4 ++ lib/api/internal/base.rb | 4 +- workhorse/cmd/gitlab-workhorse/main.go | 6 +- workhorse/internal/git/ssh.go | 55 +++++++++++++++++++ workhorse/internal/gitaly/gitaly.go | 6 ++ workhorse/internal/upstream/routes.go | 1 + 8 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 app/controllers/repositories/git_ssh_controller.rb create mode 100644 config/feature_flags/experiment/git_via_workhorse.yml create mode 100644 workhorse/internal/git/ssh.go diff --git a/app/controllers/repositories/git_ssh_controller.rb b/app/controllers/repositories/git_ssh_controller.rb new file mode 100644 index 00000000000000..b24373a10da7c6 --- /dev/null +++ b/app/controllers/repositories/git_ssh_controller.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Repositories + class GitSshController < Repositories::ApplicationController + include WorkhorseRequest + + skip_around_action :set_session_storage + skip_before_action :verify_authenticity_token + skip_around_action :sessionless_bypass_admin_mode! + + before_action :parse_repo_path + + # The endpoint is not protected by any access checks + def upload_pack + set_workhorse_internal_api_content_type + + render json: Gitlab::Workhorse.git_http_ok(repository, repo_type, nil, :git_upload_pack) + end + + private + + attr_reader :repo_type, :container, :project + + def repository_path + @repository_path ||= params[:repository_path] + end + + def parse_repo_path + @container, @project, @repo_type, @redirected_path = Gitlab::RepoPath.parse(repository_path) + end + + def repository + repo_type.repository_for(container) + end + strong_memoize_attr :repository + end +end diff --git a/config/feature_flags/experiment/git_via_workhorse.yml b/config/feature_flags/experiment/git_via_workhorse.yml new file mode 100644 index 00000000000000..22649db6537c7c --- /dev/null +++ b/config/feature_flags/experiment/git_via_workhorse.yml @@ -0,0 +1,9 @@ +--- +name: git_via_workhorse +feature_issue_url: +introduced_by_url: +rollout_issue_url: +milestone: '16.10' +group: group::source code +type: experiment +default_enabled: false diff --git a/config/routes/git_http.rb b/config/routes/git_http.rb index 6899a89cc7dc0e..07eeae08356d76 100644 --- a/config/routes/git_http.rb +++ b/config/routes/git_http.rb @@ -10,6 +10,10 @@ post '/git-receive-pack', action: :git_receive_pack end + scope(controller: :git_ssh) do + post '/ssh-upload-pack', action: :upload_pack + end + # NOTE: LFS routes are exposed on all repository types, but we still check for # LFS availability on the repository container in LfsRequest#lfs_check_access! diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 87b3838fb85e16..6b741e29bf629e 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -78,7 +78,9 @@ def check_allowed(params) "uploadpack.allowAnySHA1InWant=true"], gitaly: gitaly_payload(params[:action]), gl_console_messages: check_result.console_messages, - need_audit: need_git_audit_event? + need_audit: need_git_audit_event?, + git_via_workhorse: Feature.enabled?(:git_via_workhorse, type: :experiment), + project_url_prefix: ::Gitlab::Routing.url_helpers.project_url(project, format: 'git') }.merge!(actor.key_details) # Custom option for git-receive-pack command diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index 4d14d60df92da5..e4dd67b534f01b 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -12,6 +12,9 @@ import ( "syscall" "time" + "golang.org/x/net/http2" + "golang.org/x/net/http2/h2c" + "gitlab.com/gitlab-org/labkit/fips" "gitlab.com/gitlab-org/labkit/log" "gitlab.com/gitlab-org/labkit/monitoring" @@ -241,6 +244,7 @@ func run(boot bootConfig, cfg config.Config) error { gitaly.InitializeSidechannelRegistry(accessLogger) + http2Server := &http2.Server{} up := wrapRaven(upstream.NewUpstream(cfg, accessLogger, watchKeyFn)) done := make(chan os.Signal, 1) @@ -261,7 +265,7 @@ func run(boot bootConfig, cfg config.Config) error { } syscall.Umask(oldUmask) - srv := &http.Server{Handler: up} + srv := &http.Server{Handler: h2c.NewHandler(up, http2Server)} for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) } diff --git a/workhorse/internal/git/ssh.go b/workhorse/internal/git/ssh.go new file mode 100644 index 00000000000000..c409a4696ce7b8 --- /dev/null +++ b/workhorse/internal/git/ssh.go @@ -0,0 +1,55 @@ +package git + +import ( + "net/http" + "time" + + "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" + + "gitlab.com/gitlab-org/gitaly/v16/client" + + "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/gitaly" +) + +type FlushWriter struct { + http.ResponseWriter + controller *http.ResponseController +} + +func (f *FlushWriter) Write(p []byte) (int, error) { + n, err := f.ResponseWriter.Write(p) + if err != nil { + return n, err + } + + return n, f.controller.Flush() +} + +func SshUploadPack(a *api.API) http.Handler { + return repoPreAuthorizeHandler(a, sshUploadPack) +} + +func sshUploadPack(w http.ResponseWriter, r *http.Request, a *api.Response) { + conn, registry, err := gitaly.Connection(a.GitalyServer) + if err != nil { + w.WriteHeader(500) + return + } + + request := &gitalypb.SSHUploadPackWithSidechannelRequest{ + Repository: &a.Repository, + GitProtocol: "version=2", + GitConfigOptions: a.GitConfigOptions, + } + + out := &FlushWriter{ResponseWriter: w, controller: http.NewResponseController(w)} + + w.WriteHeader(200) + + client.UploadPackWithSidechannelWithResult(r.Context(), conn, registry, r.Body, out, out, request) + + // Temporary workaround: client.UploadPackWithSidechannelWithResult execution is finished before all the content is written to out + // Need to find a way to ensure that all the content is written before returning from the function + time.Sleep(100 * time.Millisecond) +} diff --git a/workhorse/internal/gitaly/gitaly.go b/workhorse/internal/gitaly/gitaly.go index 98f73b40a9d51e..01cffccbac1970 100644 --- a/workhorse/internal/gitaly/gitaly.go +++ b/workhorse/internal/gitaly/gitaly.go @@ -121,6 +121,12 @@ func NewDiffClient(ctx context.Context, server api.GitalyServer) (context.Contex return withOutgoingMetadata(ctx, server), &DiffClient{grpcClient}, nil } +func Connection(server api.GitalyServer) (*grpc.ClientConn, *gitalyclient.SidechannelRegistry, error) { + conn, err := getOrCreateConnection(server) + + return conn, sidechannelRegistry, err +} + func getOrCreateConnection(server api.GitalyServer) (*grpc.ClientConn, error) { key := getCacheKey(server) diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index bc0640029bf705..0edb6883886571 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -242,6 +242,7 @@ func configureRoutes(u *upstream) { u.route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.UploadPack(api)), withMatcher(isContentType("application/x-git-upload-pack-request"))), u.route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.ReceivePack(api)), withMatcher(isContentType("application/x-git-receive-pack-request"))), u.route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, requestBodyUploader, withMatcher(isContentType("application/octet-stream"))), + u.route("POST", gitProjectPattern+`ssh-upload-pack\z`, git.SshUploadPack(api)), // CI Artifacts u.route("POST", apiPattern+`v4/jobs/[0-9]+/artifacts\z`, contentEncodingHandler(upload.Artifacts(api, signingProxy, preparer, &u.Config))), -- GitLab From 9d5e0ca9dd36abbc6d7ea851a4bc760c4ad706c5 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Fri, 1 Mar 2024 16:11:40 +0100 Subject: [PATCH 2/5] Fixes failing specs --- config/feature_flags/experiment/git_via_workhorse.yml | 2 +- spec/requests/api/internal/base_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/config/feature_flags/experiment/git_via_workhorse.yml b/config/feature_flags/experiment/git_via_workhorse.yml index 22649db6537c7c..2a9253d72d9c0c 100644 --- a/config/feature_flags/experiment/git_via_workhorse.yml +++ b/config/feature_flags/experiment/git_via_workhorse.yml @@ -1,7 +1,7 @@ --- name: git_via_workhorse feature_issue_url: -introduced_by_url: +introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/146227' rollout_issue_url: milestone: '16.10' group: group::source code diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index 0969ef93ef64cb..292eb042253dbb 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -620,6 +620,10 @@ def request_response(request:, call:, method:, metadata:) # rubocop:disable Lint context 'git push with personal snippet' do subject { push(key, personal_snippet, env: env.to_json, changes: snippet_changes) } + before do + stub_feature_flags(git_via_workhorse: false) + end + it 'responds with success' do subject @@ -638,6 +642,10 @@ def request_response(request:, call:, method:, metadata:) # rubocop:disable Lint context 'git pull with personal snippet' do subject { pull(key, personal_snippet) } + before do + stub_feature_flags(git_via_workhorse: false) + end + it 'responds with success' do subject -- GitLab From 1eb2e8f16833cebe3478a54776262c9eeb740a8e Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Thu, 7 Mar 2024 21:53:04 +0100 Subject: [PATCH 3/5] Introduce GitSshRpcUrl service --- lib/api/internal/base.rb | 3 +-- lib/gitlab/git_ssh_rpc_url.rb | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 lib/gitlab/git_ssh_rpc_url.rb diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 6b741e29bf629e..f0a8e4ea5ede63 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -79,8 +79,7 @@ def check_allowed(params) gitaly: gitaly_payload(params[:action]), gl_console_messages: check_result.console_messages, need_audit: need_git_audit_event?, - git_via_workhorse: Feature.enabled?(:git_via_workhorse, type: :experiment), - project_url_prefix: ::Gitlab::Routing.url_helpers.project_url(project, format: 'git') + git_rpc_url: ::Gitlab::GitSshRpcUrl.new(project, params[:action]).execute }.merge!(actor.key_details) # Custom option for git-receive-pack command diff --git a/lib/gitlab/git_ssh_rpc_url.rb b/lib/gitlab/git_ssh_rpc_url.rb new file mode 100644 index 00000000000000..50a076c5e152fc --- /dev/null +++ b/lib/gitlab/git_ssh_rpc_url.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Gitlab + class GitSshRpcUrl # rubocop:disable Gitlab/NamespacedClass -- TODO + ACTION_TO_PATH = { + 'git-upload-pack' => '/ssh-upload-pack' + }.freeze + + def initialize(project, action) + @project = project + @action = action + end + + def execute + return unless Feature.enabled?(:git_via_workhorse, type: :experiment) + + path = ACTION_TO_PATH[action.to_s] + + return if path.blank? + + url = ::Gitlab::Routing.url_helpers.project_url(project, format: 'git') + + url + path + end + + private + + attr_reader :action, :project + end +end -- GitLab From 4fffe5275ccdfafda4d4889d033e334509df162e Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Wed, 27 Mar 2024 14:14:53 +0100 Subject: [PATCH 4/5] Send auth token from GitLab Rails --- .../repositories/git_ssh_controller.rb | 30 +++++++------------ lib/api/internal/base.rb | 3 +- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/app/controllers/repositories/git_ssh_controller.rb b/app/controllers/repositories/git_ssh_controller.rb index b24373a10da7c6..5cc6b002232379 100644 --- a/app/controllers/repositories/git_ssh_controller.rb +++ b/app/controllers/repositories/git_ssh_controller.rb @@ -1,16 +1,9 @@ # frozen_string_literal: true module Repositories - class GitSshController < Repositories::ApplicationController - include WorkhorseRequest + class GitSshController < Repositories::GitHttpClientController + GITLAB_SHELL_JWT_ISSUER = "gitlab-shell" - skip_around_action :set_session_storage - skip_before_action :verify_authenticity_token - skip_around_action :sessionless_bypass_admin_mode! - - before_action :parse_repo_path - - # The endpoint is not protected by any access checks def upload_pack set_workhorse_internal_api_content_type @@ -19,19 +12,16 @@ def upload_pack private - attr_reader :repo_type, :container, :project - - def repository_path - @repository_path ||= params[:repository_path] - end - - def parse_repo_path - @container, @project, @repo_type, @redirected_path = Gitlab::RepoPath.parse(repository_path) + def authenticate_user + payload, _ = JSONWebToken::HMACToken.decode(request.headers['Authorization'], ::Gitlab::Shell.secret_token) + render_access_denied! unless payload['iss'] == GITLAB_SHELL_JWT_ISSUER + rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::ImmatureSignature => ex + Gitlab::ErrorTracking.track_exception(ex) + render_access_denied! end - def repository - repo_type.repository_for(container) + def render_access_denied! + render(plain: 'Access is denied', status: :unauthorized) end - strong_memoize_attr :repository end end diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index f0a8e4ea5ede63..e0a8af7cd66faf 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -79,7 +79,8 @@ def check_allowed(params) gitaly: gitaly_payload(params[:action]), gl_console_messages: check_result.console_messages, need_audit: need_git_audit_event?, - git_rpc_url: ::Gitlab::GitSshRpcUrl.new(project, params[:action]).execute + git_rpc_url: ::Gitlab::GitSshRpcUrl.new(project, params[:action]).execute, + git_rpc_auth_header: headers["Gitlab-Shell-Api-Request"] }.merge!(actor.key_details) # Custom option for git-receive-pack command -- GitLab From 371d47e3d4c03ca668db7f1fc1ad465385a81519 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Wed, 27 Mar 2024 17:06:41 +0100 Subject: [PATCH 5/5] Remove h2c Upgrade --- workhorse/cmd/gitlab-workhorse/main.go | 6 +----- workhorse/go.mod | 4 ++-- workhorse/go.sum | 8 ++++---- workhorse/internal/git/ssh.go | 27 ++++++++++++++------------ 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/workhorse/cmd/gitlab-workhorse/main.go b/workhorse/cmd/gitlab-workhorse/main.go index e4dd67b534f01b..4d14d60df92da5 100644 --- a/workhorse/cmd/gitlab-workhorse/main.go +++ b/workhorse/cmd/gitlab-workhorse/main.go @@ -12,9 +12,6 @@ import ( "syscall" "time" - "golang.org/x/net/http2" - "golang.org/x/net/http2/h2c" - "gitlab.com/gitlab-org/labkit/fips" "gitlab.com/gitlab-org/labkit/log" "gitlab.com/gitlab-org/labkit/monitoring" @@ -244,7 +241,6 @@ func run(boot bootConfig, cfg config.Config) error { gitaly.InitializeSidechannelRegistry(accessLogger) - http2Server := &http2.Server{} up := wrapRaven(upstream.NewUpstream(cfg, accessLogger, watchKeyFn)) done := make(chan os.Signal, 1) @@ -265,7 +261,7 @@ func run(boot bootConfig, cfg config.Config) error { } syscall.Umask(oldUmask) - srv := &http.Server{Handler: h2c.NewHandler(up, http2Server)} + srv := &http.Server{Handler: up} for _, l := range listeners { go func(l net.Listener) { finalErrors <- srv.Serve(l) }(l) } diff --git a/workhorse/go.mod b/workhorse/go.mod index b325b9b03d3941..72211bf46d8628 100644 --- a/workhorse/go.mod +++ b/workhorse/go.mod @@ -16,7 +16,7 @@ require ( github.com/johannesboyne/gofakes3 v0.0.0-20240217095638-c55a48f17be6 github.com/jpillora/backoff v1.0.0 github.com/mitchellh/copystructure v1.2.0 - github.com/prometheus/client_golang v1.19.0 + github.com/prometheus/client_golang v1.19.1-0.20240328134234-93cf5d4f5f78 github.com/redis/go-redis/v9 v9.5.1 github.com/sebest/xff v0.0.0-20210106013422-671bd2870b3a github.com/sirupsen/logrus v1.9.3 @@ -94,7 +94,7 @@ require ( github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c // indirect - github.com/prometheus/client_model v0.5.0 // indirect + github.com/prometheus/client_model v0.6.0 // indirect github.com/prometheus/common v0.48.0 // indirect github.com/prometheus/procfs v0.12.0 // indirect github.com/prometheus/prometheus v0.50.1 // indirect diff --git a/workhorse/go.sum b/workhorse/go.sum index f47f8ea97eb7a9..e2a125d1e53a0c 100644 --- a/workhorse/go.sum +++ b/workhorse/go.sum @@ -373,11 +373,11 @@ github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRI github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c h1:ncq/mPwQF4JjgDlrVEn3C11VoGHZN7m8qihwgMEtzYw= github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c/go.mod h1:OmDBASR4679mdNQnz2pUhc2G8CO2JrUAVFDRBDP/hJE= -github.com/prometheus/client_golang v1.19.0 h1:ygXvpU1AoN1MhdzckN+PyD9QJOSD4x7kmXYlnfbA6JU= -github.com/prometheus/client_golang v1.19.0/go.mod h1:ZRM9uEAypZakd+q/x7+gmsvXdURP+DABIEIjnmDdp+k= +github.com/prometheus/client_golang v1.19.1-0.20240328134234-93cf5d4f5f78 h1:rSOwhjTtzeuOZS3pO9Gzy0vrGMHSR5s7eWiMKBTV8ns= +github.com/prometheus/client_golang v1.19.1-0.20240328134234-93cf5d4f5f78/go.mod h1:kDK4t8GKrX8Q1xkeHV0TTro2F3HIgGRx7X1Kt3GEku8= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= -github.com/prometheus/client_model v0.5.0 h1:VQw1hfvPvk3Uv6Qf29VrPF32JB6rtbgI6cYPYQjL0Qw= -github.com/prometheus/client_model v0.5.0/go.mod h1:dTiFglRmd66nLR9Pv9f0mZi7B7fk5Pm3gvsjB5tr+kI= +github.com/prometheus/client_model v0.6.0 h1:k1v3CzpSRUTrKMppY35TLwPvxHqBu0bYgxZzqGIgaos= +github.com/prometheus/client_model v0.6.0/go.mod h1:NTQHnmxFpouOD0DpvP4XujX3CdOAGQPoaGhyTchlyt8= github.com/prometheus/common v0.48.0 h1:QO8U2CdOzSn1BBsmXJXduaaW+dY/5QLjfB8svtSzKKE= github.com/prometheus/common v0.48.0/go.mod h1:0/KsvlIEfPQCQ5I2iNSAWKPZziNCvRs5EC6ILDTlAPc= github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo= diff --git a/workhorse/internal/git/ssh.go b/workhorse/internal/git/ssh.go index c409a4696ce7b8..3852d1e52e4b7a 100644 --- a/workhorse/internal/git/ssh.go +++ b/workhorse/internal/git/ssh.go @@ -1,8 +1,8 @@ package git import ( + "fmt" "net/http" - "time" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" @@ -10,6 +10,7 @@ import ( "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" "gitlab.com/gitlab-org/gitlab/workhorse/internal/gitaly" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/fail" ) type FlushWriter struct { @@ -31,25 +32,27 @@ func SshUploadPack(a *api.API) http.Handler { } func sshUploadPack(w http.ResponseWriter, r *http.Request, a *api.Response) { + controller := http.NewResponseController(w) + if err := controller.EnableFullDuplex(); err != nil { + fail.Request(w, r, fmt.Errorf("enabling full duplex: %v", err)) + + return + } + conn, registry, err := gitaly.Connection(a.GitalyServer) if err != nil { - w.WriteHeader(500) + fail.Request(w, r, fmt.Errorf("look up for gitaly connection: %v", err)) + return } + w.WriteHeader(200) + request := &gitalypb.SSHUploadPackWithSidechannelRequest{ Repository: &a.Repository, - GitProtocol: "version=2", + GitProtocol: r.Header.Get("Git-Protocol"), GitConfigOptions: a.GitConfigOptions, } - - out := &FlushWriter{ResponseWriter: w, controller: http.NewResponseController(w)} - - w.WriteHeader(200) - + out := &FlushWriter{ResponseWriter: w, controller: controller} client.UploadPackWithSidechannelWithResult(r.Context(), conn, registry, r.Body, out, out, request) - - // Temporary workaround: client.UploadPackWithSidechannelWithResult execution is finished before all the content is written to out - // Need to find a way to ensure that all the content is written before returning from the function - time.Sleep(100 * time.Millisecond) } -- GitLab