diff options
author | Nick Thomas <nick@gitlab.com> | 2017-09-28 21:59:58 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2017-10-02 16:05:50 +0300 |
commit | e8e40691069f1475ee7798a584352a80e892b635 (patch) | |
tree | 36a0dad27af2230c494af83110637d409c46a670 | |
parent | b322a578e5cfa144f5d0c5be3f43b2a0c9d8d42e (diff) |
Rework the artifacts proxy to operate within Pages group domains
-rw-r--r-- | acceptance_test.go | 41 | ||||
-rw-r--r-- | helpers_test.go | 3 | ||||
-rw-r--r-- | internal/artifact/artifact.go | 98 | ||||
-rw-r--r-- | internal/artifact/artifact_test.go | 261 |
4 files changed, 191 insertions, 212 deletions
diff --git a/acceptance_test.go b/acceptance_test.go index d6fc1400..f4e7e007 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -212,22 +212,25 @@ func TestStatusPage(t *testing.T) { assert.Equal(t, http.StatusOK, rsp.StatusCode) } -func TestProxyRequest(t *testing.T) { +func TestArtifactProxyRequest(t *testing.T) { skipUnlessEnabled(t) content := "<!DOCTYPE html><html><head><title>Title of the document</title></head><body></body></html>" contentLength := int64(len(content)) testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.URL.Path { - case "/projects/1/jobs/2/artifacts/delayed_200.html": + switch r.URL.RawPath { + case "/api/v4/projects/group%2Fproject/jobs/1/artifacts/delayed_200.html": time.Sleep(2 * time.Second) - case "/projects/1/jobs/2/artifacts/200.html": + fallthrough + case "/api/v4/projects/group%2Fproject/jobs/1/artifacts/200.html", + "/api/v4/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/200.html": w.Header().Set("Content-Type", "text/html; charset=utf-8") fmt.Fprint(w, content) - case "/projects/1/jobs/2/artifacts/500.html": + case "/api/v4/projects/group%2Fproject/jobs/1/artifacts/500.html": w.Header().Set("Content-Type", "text/html; charset=utf-8") w.WriteHeader(http.StatusInternalServerError) fmt.Fprint(w, content) default: + t.Logf("Unexpected r.URL.RawPath: %q", r.URL.RawPath) w.Header().Set("Content-Type", "text/html; charset=utf-8") w.WriteHeader(http.StatusNotFound) fmt.Fprint(w, content) @@ -235,6 +238,7 @@ func TestProxyRequest(t *testing.T) { })) defer testServer.Close() cases := []struct { + Host string Path string Status int BinaryOption string @@ -245,7 +249,8 @@ func TestProxyRequest(t *testing.T) { Description string }{ { - "200.html", + "group.gitlab-example.com", + "/-/project/-/jobs/1/artifacts/200.html", http.StatusOK, "", content, @@ -255,7 +260,19 @@ func TestProxyRequest(t *testing.T) { "basic proxied request", }, { - "delayed_200.html", + "group.gitlab-example.com", + "/-/subgroup/project/-/jobs/1/artifacts/200.html", + http.StatusOK, + "", + content, + contentLength, + "max-age=3600", + "text/html; charset=utf-8", + "basic proxied request for subgroup", + }, + { + "group.gitlab-example.com", + "/-/project/-/jobs/1/artifacts/delayed_200.html", http.StatusBadGateway, "-artifacts-server-timeout=1", "", @@ -265,7 +282,8 @@ func TestProxyRequest(t *testing.T) { "502 error while attempting to proxy", }, { - "404.html", + "group.gitlab-example.com", + "/-/project/-/jobs/1/artifacts/404.html", http.StatusNotFound, "", "", @@ -275,7 +293,8 @@ func TestProxyRequest(t *testing.T) { "Proxying 404 from server", }, { - "500.html", + "group.gitlab-example.com", + "/-/project/-/jobs/1/artifacts/500.html", http.StatusInternalServerError, "", "", @@ -288,9 +307,9 @@ func TestProxyRequest(t *testing.T) { for _, c := range cases { t.Run(fmt.Sprintf("Proxy Request Test: %s", c.Description), func(t *testing.T) { - teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "-artifacts-server="+testServer.URL, c.BinaryOption) + teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "-artifacts-server="+testServer.URL+"/api/v4", c.BinaryOption) defer teardown() - resp, err := GetPageFromListener(t, httpListener, "artifact~1~2.gitlab-example.com", c.Path) + resp, err := GetPageFromListener(t, httpListener, c.Host, c.Path) require.NoError(t, err) defer resp.Body.Close() assert.Equal(t, c.Status, resp.StatusCode) diff --git a/helpers_test.go b/helpers_test.go index c1d1bf34..9f372224 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "os/exec" + "strings" "testing" "time" @@ -113,6 +114,8 @@ func (l ListenSpec) URL(suffix string) string { scheme = "https" } + suffix = strings.TrimPrefix(suffix, "/") + return fmt.Sprintf("%s://%s/%s", scheme, l.JoinHostPort(), suffix) } diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go index bcb525ac..100ec90d 100644 --- a/internal/artifact/artifact.go +++ b/internal/artifact/artifact.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/url" + "path" "regexp" "strconv" "strings" @@ -14,58 +15,68 @@ import ( ) const ( - baseURL = "/projects/%s/jobs/%s/artifacts" - hostPatternTemplate = `(?i)\Aartifact~(\d+)~(\d+)\.%s\z` - minStatusCode = 200 - maxStatusCode = 299 + // Format a non-/-suffixed URL, an escaped project full_path, a job ID and + // a /-prefixed file path into an URL string + apiURLTemplate = "%s/projects/%s/jobs/%s/artifacts%s" + + minStatusCode = 200 + maxStatusCode = 299 +) + +var ( + // Captures subgroup + project, job ID and artifacts path + pathExtractor = regexp.MustCompile(`(?i)\A/-/(.*)/-/jobs/(\d+)/artifacts(/[^?]*)\z`) ) -// Artifact is a struct that is made up of a url.URL, http.Client, and -// regexp.Regexp that is used to proxy requests where applicable. +// Artifact proxies requests for artifact files to the GitLab artifacts API type Artifact struct { - server string - client *http.Client - pattern *regexp.Regexp + server string + suffix string + client *http.Client } // New when provided the arguments defined herein, returns a pointer to an // Artifact that is used to proxy requests. -func New(s string, timeout int, pagesDomain string) *Artifact { +func New(server string, timeoutSeconds int, pagesDomain string) *Artifact { return &Artifact{ - server: s, - client: &http.Client{Timeout: time.Second * time.Duration(timeout)}, - pattern: hostPatternGen(pagesDomain), + server: strings.TrimRight(server, "/"), + suffix: "." + strings.ToLower(pagesDomain), + client: &http.Client{Timeout: time.Second * time.Duration(timeoutSeconds)}, } - } // TryMakeRequest will attempt to proxy a request and write it to the argument // http.ResponseWriter, ultimately returning a bool that indicates if the // http.ResponseWriter has been written to in any capacity. func (a *Artifact) TryMakeRequest(host string, w http.ResponseWriter, r *http.Request) bool { - if a == nil || a.server == "" { + if a == nil || a.server == "" || host == "" { return false } - reqURL, ok := a.buildURL(host, r.URL.Path) + reqURL, ok := a.BuildURL(host, r.URL.Path) if !ok { return false } + a.makeRequest(w, reqURL) + return true +} + +func (a *Artifact) makeRequest(w http.ResponseWriter, reqURL *url.URL) { resp, err := a.client.Get(reqURL.String()) if err != nil { httperrors.Serve502(w) - return true + return } if resp.StatusCode == http.StatusNotFound { httperrors.Serve404(w) - return true + return } if resp.StatusCode == http.StatusInternalServerError { httperrors.Serve500(w) - return true + return } // we only cache responses within the 2xx series response codes @@ -77,44 +88,43 @@ func (a *Artifact) TryMakeRequest(host string, w http.ResponseWriter, r *http.Re w.Header().Set("Content-Length", strconv.FormatInt(resp.ContentLength, 10)) w.WriteHeader(resp.StatusCode) io.Copy(w, resp.Body) - return true + return } -// buildURL returns a pointer to a url.URL for where the request should be +// BuildURL returns a pointer to a url.URL for where the request should be // proxied to. The returned bool will indicate if there is some sort of issue // with the url while it is being generated. -func (a *Artifact) buildURL(host, path string) (*url.URL, bool) { - ids := a.pattern.FindAllStringSubmatch(host, -1) - if len(ids) != 1 || len(ids[0]) != 3 { +// +// The URL is generated from the host (which contains the top-level group and +// ends with the pagesDomain) and the path (which contains any subgroups, the +// project, a job ID and a path +// for the artifact file we want to download) +func (a *Artifact) BuildURL(host, requestPath string) (*url.URL, bool) { + if !strings.HasSuffix(strings.ToLower(host), a.suffix) { return nil, false } - strippedIds := ids[0][1:3] - body := fmt.Sprintf(baseURL, strippedIds[0], strippedIds[1]) - ourPath := a.server - if strings.HasSuffix(ourPath, "/") { - ourPath = ourPath[0:len(ourPath)-1] + body - } else { - ourPath = ourPath + body + topGroup := host[0 : len(host)-len(a.suffix)] + + parts := pathExtractor.FindAllStringSubmatch(requestPath, 1) + if len(parts) != 1 || len(parts[0]) != 4 { + return nil, false } - if len(path) == 0 || strings.HasPrefix(path, "/") { - ourPath = ourPath + path - } else { - ourPath = ourPath + "/" + path + restOfPath := strings.TrimLeft(strings.TrimRight(parts[0][1], "/"), "/") + if len(restOfPath) == 0 { + return nil, false } - u, err := url.Parse(ourPath) + jobID := parts[0][2] + artifactPath := parts[0][3] + + projectID := url.PathEscape(path.Join(topGroup, restOfPath)) + generated := fmt.Sprintf(apiURLTemplate, a.server, projectID, jobID, artifactPath) + + u, err := url.Parse(generated) if err != nil { return nil, false } return u, true } - -// hostPatternGen returns a pointer to a regexp.Regexp that is made up of -// the constant hostPatternTemplate and the argument which represents the pages domain. -// This is used to ensure that the requested page meets not only the hostPatternTemplate -// requirements, but is suffixed with the proper pagesDomain. -func hostPatternGen(pagesDomain string) *regexp.Regexp { - return regexp.MustCompile(fmt.Sprintf(hostPatternTemplate, regexp.QuoteMeta(pagesDomain))) -} diff --git a/internal/artifact/artifact_test.go b/internal/artifact/artifact_test.go index ad2285f6..62962513 100644 --- a/internal/artifact/artifact_test.go +++ b/internal/artifact/artifact_test.go @@ -1,15 +1,15 @@ -package artifact +package artifact_test import ( "fmt" "net/http" "net/http/httptest" "net/url" - "regexp" "testing" - "time" "github.com/stretchr/testify/assert" + + "gitlab.com/gitlab-org/gitlab-pages/internal/artifact" ) func TestTryMakeRequest(t *testing.T) { @@ -17,17 +17,20 @@ func TestTryMakeRequest(t *testing.T) { contentType := "text/html; charset=utf-8" testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", contentType) - switch r.URL.Path { - case "/projects/1/jobs/2/artifacts/200.html": + switch r.URL.RawPath { + case "/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/200.html": w.WriteHeader(http.StatusOK) - case "/projects/1/jobs/2/artifacts/max-caching.html": + case "/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/max-caching.html": w.WriteHeader(http.StatusIMUsed) - case "/projects/1/jobs/2/artifacts/non-caching.html": + case "/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/non-caching.html": w.WriteHeader(http.StatusTeapot) - case "/projects/1/jobs/2/artifacts/500.html": + case "/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/500.html": w.WriteHeader(http.StatusInternalServerError) - case "/projects/1/jobs/2/artifacts/404.html": + case "/projects/group%2Fsubgroup%2Fgroup%2Fproject/jobs/1/artifacts/404.html": w.WriteHeader(http.StatusNotFound) + default: + t.Log("Surprising r.URL.RawPath", r.URL.RawPath) + w.WriteHeader(999) } fmt.Fprint(w, content) })) @@ -72,22 +75,21 @@ func TestTryMakeRequest(t *testing.T) { } for _, c := range cases { - result := httptest.NewRecorder() - reqURL, err := url.Parse(c.Path) - assert.NoError(t, err) - r := &http.Request{URL: reqURL} - art := &Artifact{ - server: testServer.URL, - client: &http.Client{Timeout: time.Second * time.Duration(1)}, - pattern: regexp.MustCompile(fmt.Sprintf(hostPatternTemplate, "gitlab-example.io")), - } + t.Run(c.Description, func(t *testing.T) { + result := httptest.NewRecorder() + reqURL, err := url.Parse("/-/subgroup/project/-/jobs/1/artifacts" + c.Path) + assert.NoError(t, err) + r := &http.Request{URL: reqURL} + art := artifact.New(testServer.URL, 1, "gitlab-example.io") + + assert.True(t, art.TryMakeRequest("group.gitlab-example.io", result, r)) + assert.Equal(t, c.Status, result.Code) + assert.Equal(t, c.ContentType, result.Header().Get("Content-Type")) + assert.Equal(t, c.Length, result.Header().Get("Content-Length")) + assert.Equal(t, c.CacheControl, result.Header().Get("Cache-Control")) + assert.Equal(t, c.Content, string(result.Body.Bytes())) - assert.True(t, art.TryMakeRequest("artifact~1~2.gitlab-example.io", result, r)) - assert.Equal(t, c.ContentType, result.Header().Get("Content-Type")) - assert.Equal(t, c.Length, result.Header().Get("Content-Length")) - assert.Equal(t, c.CacheControl, result.Header().Get("Cache-Control")) - assert.Equal(t, c.Content, string(result.Body.Bytes())) - assert.Equal(t, c.Status, result.Code) + }) } } @@ -103,201 +105,146 @@ func TestBuildURL(t *testing.T) { }{ { "https://gitlab.com/api/v4", - "artifact~1~2.gitlab.io", - "/path/to/file.txt", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts/path/to/file.txt", + "group.gitlab.io", + "/-/project/-/jobs/1/artifacts/", + `https://gitlab.com/api/v4/projects/group%2Fproject/jobs/1/artifacts/`, "gitlab.io", true, - "basic case", + "Basic case", }, { "https://gitlab.com/api/v4/", - "artifact~1~2.gitlab.io", - "/path/to/file.txt", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts/path/to/file.txt", - "gitlab.io", - true, - "basic case 2", - }, - { - "https://gitlab.com/api/v4", - "artifact~1~2.gitlab.io", - "path/to/file.txt", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts/path/to/file.txt", + "group.gitlab.io", + "/-/project/-/jobs/1/artifacts/", + "https://gitlab.com/api/v4/projects/group%2Fproject/jobs/1/artifacts/", "gitlab.io", true, - "basic case 3", + "API URL has trailing slash", }, { "https://gitlab.com/api/v4/", - "artifact~1~2.gitlab.io", - "path/to/file.txt", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts/path/to/file.txt", + "GROUP.GITLAB.IO", + "/-/SUBGROUP/PROJECT/-/JOBS/1/ARTIFACTS/PATH/TO/FILE.txt", + "https://gitlab.com/api/v4/projects/GROUP%2FSUBGROUP%2FPROJECT/jobs/1/artifacts/PATH/TO/FILE.txt", "gitlab.io", true, - "basic case 4", + "Uppercase names", }, { "https://gitlab.com/api/v4", - "artifact~1~2.gitlab.io", + "group.gitlab.io", + "/-/project/-/jobs/1foo1/artifacts/", "", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts", "gitlab.io", - true, - "basic case 5", + false, + "Job ID has letters", }, { - "https://gitlab.com/api/v4/", - "artifact~1~2.gitlab.io", + "https://gitlab.com/api/v4", + "group.gitlab.io", + "/-/project/-/jobs/1$1/artifacts/", "", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts", "gitlab.io", - true, - "basic case 6", + false, + "Job ID has special characters", }, { "https://gitlab.com/api/v4", - "artifact~1~2.gitlab.io", - "/", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts/", - "gitlab.io", - true, - "basic case 7", - }, - { - "https://gitlab.com/api/v4/", - "artifact~1~2.gitlab.io", - "/", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts/", + "group.gitlab.io", + "/-/project/-/jobs/1/artifacts/path/to/file.txt", + "https://gitlab.com/api/v4/projects/group%2Fproject/jobs/1/artifacts/path/to/file.txt", "gitlab.io", true, - "basic case 8", + "Artifact in subdirectory", }, { "https://gitlab.com/api/v4", - "artifact~100000~200000.gitlab.io", - "/file.txt", - "https://gitlab.com/api/v4/projects/100000/jobs/200000/artifacts/file.txt", + "group.gitlab.io", + "/-/subgroup1/sub.group2/project/-/jobs/1/artifacts/path/to/file.txt", + "https://gitlab.com/api/v4/projects/group%2Fsubgroup1%2Fsub.group2%2Fproject/jobs/1/artifacts/path/to/file.txt", "gitlab.io", true, - "expanded case", - }, - { - "https://gitlab.com/api/v4/", - "artifact~1~2.gitlab.io", - "/file.txt", - "https://gitlab.com/api/v4/projects/1/jobs/2/artifacts/file.txt", - "gitlab.io", - true, - "server with tailing slash", + "Basic subgroup case", }, { "https://gitlab.com/api/v4", - "artifact~A~B.gitlab.io", - "/index.html", - "", - "example.com", - false, - "non matching domain and request", - }, - { - "", - "artifact~A~B.gitlab.io", - "", - "", - "", - false, - "un-parseable Host", - }, - } - - for _, c := range cases { - a := &Artifact{server: c.RawServer, pattern: regexp.MustCompile(fmt.Sprintf(hostPatternTemplate, c.PagesDomain))} - u, ok := a.buildURL(c.Host, c.Path) - assert.Equal(t, c.Ok, ok, c.Description) - if c.Ok { - assert.Equal(t, c.Expected, u.String(), c.Description) - } - } -} - -func TestMatchHostGen(t *testing.T) { - cases := []struct { - URLHost string - PagesDomain string - Expected bool - Description string - }{ - { - "artifact~1~2.gitlab.io", + "group.gitlab.io", + "/-//project/-/jobs/1/artifacts/", + "https://gitlab.com/api/v4/projects/group%2Fproject/jobs/1/artifacts/", "gitlab.io", true, - "basic case", + "Leading / in remainder of project path", }, { - "ARTIFACT~1~2.gitlab.io", + "https://gitlab.com/api/v4", + "group.gitlab.io", + "/-/subgroup/project//-/jobs/1/artifacts/", + "https://gitlab.com/api/v4/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/", "gitlab.io", true, - "capital letters case", + "Trailing / in remainder of project path", }, { - "ARTIFACT~11234~2908908.gitlab.io", + "https://gitlab.com/api/v4", + "group.gitlab.io", + "/-//subgroup/project//-/jobs/1/artifacts/", + "https://gitlab.com/api/v4/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/", "gitlab.io", true, - "additional capital letters case", + "Leading and trailing /", }, { - "artifact~10000~20000.gitlab.io", + "https://gitlab.com/api/v4", + "group.name.gitlab.io", + "/-/subgroup/project/-/jobs/1/artifacts/", + "https://gitlab.com/api/v4/projects/group.name%2Fsubgroup%2Fproject/jobs/1/artifacts/", "gitlab.io", true, - "expanded case", + "Toplevel group has period", }, { - "artifact~86753095555~55550935768.gitlab.io", + "https://gitlab.com/api/v4", + "gitlab.io.gitlab.io", + "/-/project/-/jobs/1/artifacts/", + "https://gitlab.com/api/v4/projects/gitlab.io%2Fproject/jobs/1/artifacts/", "gitlab.io", true, - "large number case", - }, - { - "artifact~one~two.gitlab.io", - "gitlab.io", - false, - "letters rather than numbers", + "Toplevel group matches pages domain", }, { - "artifact~One111~tWo222.gitlab.io", - "gitlab.io", - false, - "Mixture of alphanumeric", - }, - { - "artifact~!@#$%~%$#@!.gitlab.io", - "gitlab.io", - false, - "special characters", - }, - { - "artifact~1.gitlab.io", - "gitlab.io", - false, - "not enough ids", - }, - { - "artifact~1~2~34444~1~4.gitlab.io", + "https://gitlab.com/api/v4", + "group.gitlab.io", + "/-/project/-/jobs/1/artifacts", + "", "gitlab.io", false, - "too many ids", + "No artifact specified", }, { - "artifact~1~2.gitlab.io", - "otherhost.io", + "https://gitlab.com/api/v4", + "group.gitlab.io", + "/index.html", + "", + "example.com", false, - "different domain / suffix", + "non matching domain and request", }, } for _, c := range cases { - reg := hostPatternGen(c.PagesDomain) - assert.Equal(t, c.Expected, reg.MatchString(c.URLHost), c.Description) + t.Run(c.Description, func(t *testing.T) { + a := artifact.New(c.RawServer, 1, c.PagesDomain) + u, ok := a.BuildURL(c.Host, c.Path) + + msg := c.Description + " - generated URL: " + if u != nil { + msg = msg + u.String() + } + + assertOk := assert.Equal(t, c.Ok, ok, msg) + if assertOk && c.Ok { + assert.Equal(t, c.Expected, u.String(), c.Description) + } + }) } } |