From eb3b35b9b0cc55fb8464d9b0662e6b94aafc54cc Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 17 Aug 2020 22:19:56 -0700 Subject: Fix gitlab-shell not handling relative URLs over UNIX sockets From https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/4498#note_397401883, if you specify a relative path such as: ``` external_url 'http://gitlab.example.com/gitlab' ``` gitlab-shell doesn't have a way to pass the `/gitlab` to the host. For example, let's say we have: ``` gitlab_url: "http+unix://%2Fvar%2Fopt%2Fgitlab%2Fgitlab-workhorse%2Fsocket" ``` If we have `/gitlab` as the relative path, how do we specify what is the UNIX socket path and what is the relative path? If we specify: ``` gitlab_url: "http+unix:///var/opt/gitlab/gitlab-workhorse.socket/gitlab ``` This is ambiguous. Is the socket in `/var/opt/gitlab/gitlab-workhorse.socket/gitlab` or in `/var/opt/gitlab/gitlab-workhorse.socket`? To fix this, this merge request adds an optional `gitlab_relative_url_root` config parameter: ``` gitlab_url: "http+unix://%2Fvar%2Fopt%2Fgitlab%2Fgitlab-workhorse%2Fsocket" gitlab_relative_url_root: /gitlab ``` This is only used with UNIX domain sockets to disambiguate the socket and base URL path. If `gitlab_url` uses `http://` or `https://`, then `gitlab_relative_url_root` is ignored. Relates to https://gitlab.com/gitlab-org/gitlab-shell/-/issues/476 --- client/client_test.go | 127 ++++++++++++++++++++++++++------------------- client/httpclient.go | 16 ++++-- client/httpclient_test.go | 4 +- client/httpsclient_test.go | 2 +- config.yml.example | 4 ++ internal/config/config.go | 22 ++++---- 6 files changed, 105 insertions(+), 70 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index d520bbb..e92093a 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "net/http" "path" + "strings" "testing" "github.com/sirupsen/logrus" @@ -21,62 +22,26 @@ func TestClients(t *testing.T) { require.NoError(t, err) defer testDirCleanup() - requests := []testserver.TestRequestHandler{ - { - Path: "/api/v4/internal/hello", - Handler: func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, http.MethodGet, r.Method) - - fmt.Fprint(w, "Hello") - }, - }, - { - Path: "/api/v4/internal/post_endpoint", - Handler: func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, http.MethodPost, r.Method) - - b, err := ioutil.ReadAll(r.Body) - defer r.Body.Close() - - require.NoError(t, err) - - fmt.Fprint(w, "Echo: "+string(b)) - }, - }, - { - Path: "/api/v4/internal/auth", - Handler: func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, r.Header.Get(secretHeaderName)) - }, - }, - { - Path: "/api/v4/internal/error", - Handler: func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusBadRequest) - body := map[string]string{ - "message": "Don't do that", - } - json.NewEncoder(w).Encode(body) - }, - }, - { - Path: "/api/v4/internal/broken", - Handler: func(w http.ResponseWriter, r *http.Request) { - panic("Broken") - }, - }, - } - testCases := []struct { - desc string - caFile string - server func(*testing.T, []testserver.TestRequestHandler) (string, func()) + desc string + relativeURLRoot string + caFile string + server func(*testing.T, []testserver.TestRequestHandler) (string, func()) }{ { desc: "Socket client", server: testserver.StartSocketHttpServer, }, + { + desc: "Socket client with a relative URL at /", + relativeURLRoot: "/", + server: testserver.StartSocketHttpServer, + }, + { + desc: "Socket client with relative URL at /gitlab", + relativeURLRoot: "/gitlab", + server: testserver.StartSocketHttpServer, + }, { desc: "Http client", server: testserver.StartHttpServer, @@ -90,12 +55,12 @@ func TestClients(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - url, cleanup := tc.server(t, requests) + url, cleanup := tc.server(t, buildRequests(t, tc.relativeURLRoot)) defer cleanup() secret := "sssh, it's a secret" - httpClient := NewHTTPClient(url, tc.caFile, "", false, 1) + httpClient := NewHTTPClient(url, tc.relativeURLRoot, tc.caFile, "", false, 1) client, err := NewGitlabNetClient("", "", secret, httpClient) require.NoError(t, err) @@ -275,3 +240,61 @@ func testAuthenticationHeader(t *testing.T, client *GitlabNetClient) { assert.Equal(t, "sssh, it's a secret", string(header)) }) } + +func buildRequests(t *testing.T, relativeURLRoot string) []testserver.TestRequestHandler { + requests := []testserver.TestRequestHandler{ + { + Path: "/api/v4/internal/hello", + Handler: func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method) + + fmt.Fprint(w, "Hello") + }, + }, + { + Path: "/api/v4/internal/post_endpoint", + Handler: func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPost, r.Method) + + b, err := ioutil.ReadAll(r.Body) + defer r.Body.Close() + + require.NoError(t, err) + + fmt.Fprint(w, "Echo: "+string(b)) + }, + }, + { + Path: "/api/v4/internal/auth", + Handler: func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, r.Header.Get(secretHeaderName)) + }, + }, + { + Path: "/api/v4/internal/error", + Handler: func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + body := map[string]string{ + "message": "Don't do that", + } + json.NewEncoder(w).Encode(body) + }, + }, + { + Path: "/api/v4/internal/broken", + Handler: func(w http.ResponseWriter, r *http.Request) { + panic("Broken") + }, + }, + } + + relativeURLRoot = strings.Trim(relativeURLRoot, "/") + if relativeURLRoot != "" { + for i, r := range requests { + requests[i].Path = fmt.Sprintf("/%s%s", relativeURLRoot, r.Path) + } + } + + return requests +} diff --git a/client/httpclient.go b/client/httpclient.go index 63386f7..6635f1b 100644 --- a/client/httpclient.go +++ b/client/httpclient.go @@ -27,12 +27,12 @@ type HttpClient struct { Host string } -func NewHTTPClient(gitlabURL, caFile, caPath string, selfSignedCert bool, readTimeoutSeconds uint64) *HttpClient { +func NewHTTPClient(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, selfSignedCert bool, readTimeoutSeconds uint64) *HttpClient { var transport *http.Transport var host string if strings.HasPrefix(gitlabURL, unixSocketProtocol) { - transport, host = buildSocketTransport(gitlabURL) + transport, host = buildSocketTransport(gitlabURL, gitlabRelativeURLRoot) } else if strings.HasPrefix(gitlabURL, httpProtocol) { transport, host = buildHttpTransport(gitlabURL) } else if strings.HasPrefix(gitlabURL, httpsProtocol) { @@ -41,7 +41,6 @@ func NewHTTPClient(gitlabURL, caFile, caPath string, selfSignedCert bool, readTi return nil } - c := &http.Client{ Transport: correlation.NewInstrumentedRoundTripper(transport), Timeout: readTimeout(readTimeoutSeconds), @@ -52,8 +51,9 @@ func NewHTTPClient(gitlabURL, caFile, caPath string, selfSignedCert bool, readTi return client } -func buildSocketTransport(gitlabURL string) (*http.Transport, string) { +func buildSocketTransport(gitlabURL, gitlabRelativeURLRoot string) (*http.Transport, string) { socketPath := strings.TrimPrefix(gitlabURL, unixSocketProtocol) + transport := &http.Transport{ DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) { dialer := net.Dialer{} @@ -61,7 +61,13 @@ func buildSocketTransport(gitlabURL string) (*http.Transport, string) { }, } - return transport, socketBaseUrl + host := socketBaseUrl + gitlabRelativeURLRoot = strings.Trim(gitlabRelativeURLRoot, "/") + if gitlabRelativeURLRoot != "" { + host = host + "/" + gitlabRelativeURLRoot + } + + return transport, host } func buildHttpsTransport(caFile, caPath string, selfSignedCert bool, gitlabURL string) (*http.Transport, string) { diff --git a/client/httpclient_test.go b/client/httpclient_test.go index 1f0a4ed..fce0cd5 100644 --- a/client/httpclient_test.go +++ b/client/httpclient_test.go @@ -17,7 +17,7 @@ import ( func TestReadTimeout(t *testing.T) { expectedSeconds := uint64(300) - client := NewHTTPClient("http://localhost:3000", "", "", false, expectedSeconds) + client := NewHTTPClient("http://localhost:3000", "", "", "", false, expectedSeconds) require.NotNil(t, client) assert.Equal(t, time.Duration(expectedSeconds)*time.Second, client.Client.Timeout) @@ -96,7 +96,7 @@ func TestEmptyBasicAuthSettings(t *testing.T) { func setup(t *testing.T, username, password string, requests []testserver.TestRequestHandler) (*GitlabNetClient, func()) { url, cleanup := testserver.StartHttpServer(t, requests) - httpClient := NewHTTPClient(url, "", "", false, 1) + httpClient := NewHTTPClient(url, "", "", "", false, 1) client, err := NewGitlabNetClient(username, password, "", httpClient) require.NoError(t, err) diff --git a/client/httpsclient_test.go b/client/httpsclient_test.go index 6c3ae08..1c7435f 100644 --- a/client/httpsclient_test.go +++ b/client/httpsclient_test.go @@ -106,7 +106,7 @@ func setupWithRequests(t *testing.T, caFile, caPath string, selfSigned bool) (*G url, cleanup := testserver.StartHttpsServer(t, requests) - httpClient := NewHTTPClient(url, caFile, caPath, selfSigned, 1) + httpClient := NewHTTPClient(url, "", caFile, caPath, selfSigned, 1) client, err := NewGitlabNetClient("", "", "", httpClient) require.NoError(t, err) diff --git a/config.yml.example b/config.yml.example index 60435a3..6689ca8 100644 --- a/config.yml.example +++ b/config.yml.example @@ -15,6 +15,10 @@ user: git # "http+unix://%2Fpath%2Fto%2Fsocket" gitlab_url: "http+unix://%2Fhome%2Fgit%2Fgitlab%2Ftmp%2Fsockets%2Fgitlab-workhorse.socket" +# When a http+unix:// is used in gitlab_url, this is the relative URL root to GitLab. +# Not used if gitlab_url is http:// or https://. +# gitlab_relative_url_root: "/" + # See installation.md#using-https for additional HTTPS configuration details. http_settings: # read_timeout: 300 diff --git a/internal/config/config.go b/internal/config/config.go index be9efa3..e7abd59 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,16 +27,17 @@ type HttpSettingsConfig struct { } type Config struct { - RootDir string - LogFile string `yaml:"log_file"` - LogFormat string `yaml:"log_format"` - GitlabUrl string `yaml:"gitlab_url"` - GitlabTracing string `yaml:"gitlab_tracing"` - SecretFilePath string `yaml:"secret_file"` - Secret string `yaml:"secret"` - SslCertDir string `yaml:"ssl_cert_dir"` - HttpSettings HttpSettingsConfig `yaml:"http_settings"` - HttpClient *client.HttpClient + RootDir string + LogFile string `yaml:"log_file"` + LogFormat string `yaml:"log_format"` + GitlabUrl string `yaml:"gitlab_url"` + GitlabRelativeURLRoot string `yaml:"gitlab_relative_url_root"` + GitlabTracing string `yaml:"gitlab_tracing"` + SecretFilePath string `yaml:"secret_file"` + Secret string `yaml:"secret"` + SslCertDir string `yaml:"ssl_cert_dir"` + HttpSettings HttpSettingsConfig `yaml:"http_settings"` + HttpClient *client.HttpClient } func (c *Config) GetHttpClient() *client.HttpClient { @@ -46,6 +47,7 @@ func (c *Config) GetHttpClient() *client.HttpClient { client := client.NewHTTPClient( c.GitlabUrl, + c.GitlabRelativeURLRoot, c.HttpSettings.CaFile, c.HttpSettings.CaPath, c.HttpSettings.SelfSignedCert, -- cgit v1.2.1