Merge branch '485-use-httpfs-with-httprange' into 'master'

Allow registering a file protocol in the zip VFS

See merge request gitlab-org/gitlab-pages!429

14 files changed, 105 insertions, 34 deletions

diff --git a/app.go b/app.go
index ec0c36a5..096a1c28 100644
--- a/app.go
+++ b/app.go
@@ -517,6 +517,7 @@ func runApp(config appConfig) {
 			CleanupInterval:    config.ZipCacheCleanup,
 			RefreshInterval:    config.ZipCacheRefresh,
 			OpenTimeout:        config.ZipeOpenTimeout,
+			AllowedPaths:       []string{config.PagesRoot},
 		},
 	}
 
diff --git a/app_config.go b/app_config.go
index 0dd192d5..dbd349ad 100644
--- a/app_config.go
+++ b/app_config.go
@@ -3,6 +3,7 @@ package main
 import "time"
 
 type appConfig struct {
+	PagesRoot              string
 	Domain                 string
 	ArtifactsServer        string
 	ArtifactsServerTimeout int
diff --git a/internal/config/config.go b/internal/config/config.go
index c52beef8..e2956bb6 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -15,4 +15,5 @@ type ZipServing struct {
 	CleanupInterval    time.Duration
 	RefreshInterval    time.Duration
 	OpenTimeout        time.Duration
+	AllowedPaths       []string
 }
diff --git a/internal/httprange/http_ranged_reader_test.go b/internal/httprange/http_ranged_reader_test.go
index b17d06b1..d32e5619 100644
--- a/internal/httprange/http_ranged_reader_test.go
+++ b/internal/httprange/http_ranged_reader_test.go
@@ -18,6 +18,10 @@ const (
 	testDataLen = len(testData)
 )
 
+var testClient = &http.Client{
+	Timeout: 100 * time.Millisecond,
+}
+
 func TestSectionReader(t *testing.T) {
 	tests := map[string]struct {
 		sectionOffset   int
@@ -80,7 +84,7 @@ func TestSectionReader(t *testing.T) {
 	testServer := newTestServer(t, nil)
 	defer testServer.Close()
 
-	resource, err := NewResource(context.Background(), testServer.URL+"/resource")
+	resource, err := NewResource(context.Background(), testServer.URL+"/resource", testClient)
 	require.NoError(t, err)
 
 	for name, tt := range tests {
@@ -163,7 +167,7 @@ func TestReadAt(t *testing.T) {
 	testServer := newTestServer(t, nil)
 	defer testServer.Close()
 
-	resource, err := NewResource(context.Background(), testServer.URL+"/resource")
+	resource, err := NewResource(context.Background(), testServer.URL+"/resource", testClient)
 	require.NoError(t, err)
 
 	for name, tt := range tests {
@@ -202,7 +206,7 @@ func TestReadAtMultipart(t *testing.T) {
 	})
 	defer testServer.Close()
 
-	resource, err := NewResource(context.Background(), testServer.URL+"/resource")
+	resource, err := NewResource(context.Background(), testServer.URL+"/resource", testClient)
 	require.NoError(t, err)
 	require.Equal(t, int32(1), counter)
 
@@ -247,7 +251,7 @@ func TestReadContextCanceled(t *testing.T) {
 	testServer := newTestServer(t, nil)
 	defer testServer.Close()
 
-	resource, err := NewResource(context.Background(), testServer.URL+"/resource")
+	resource, err := NewResource(context.Background(), testServer.URL+"/resource", testClient)
 	require.NoError(t, err)
 
 	rr := NewRangedReader(resource)
diff --git a/internal/httprange/http_reader.go b/internal/httprange/http_reader.go
index d1523177..c2b81b4d 100644
--- a/internal/httprange/http_reader.go
+++ b/internal/httprange/http_reader.go
@@ -6,9 +6,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"time"
 
-	"gitlab.com/gitlab-org/gitlab-pages/internal/httptransport"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
@@ -49,22 +47,6 @@ type Reader struct {
 // ensure that Reader is seekable
 var _ vfs.SeekableFile = &Reader{}
 
-// TODO: make this configurable/take an http client when creating a reader/ranged reader
-//  instead https://gitlab.com/gitlab-org/gitlab-pages/-/issues/457
-var httpClient = &http.Client{
-	// The longest time the request can be executed
-	Timeout: 30 * time.Minute,
-	Transport: httptransport.NewMeteredRoundTripper(
-		// TODO: register file protocol https://gitlab.com/gitlab-org/gitlab-pages/-/issues/485
-		nil,
-		"httprange_client",
-		metrics.HTTPRangeTraceDuration,
-		metrics.HTTPRangeRequestDuration,
-		metrics.HTTPRangeRequestsTotal,
-		httptransport.DefaultTTFBTimeout,
-	),
-}
-
 // ensureResponse is set before reading from it.
 // It will do the request if the reader hasn't got it yet.
 func (r *Reader) ensureResponse() error {
@@ -79,7 +61,7 @@ func (r *Reader) ensureResponse() error {
 
 	metrics.HTTPRangeOpenRequests.Inc()
 
-	res, err := httpClient.Do(req)
+	res, err := r.Resource.httpClient.Do(req)
 	if err != nil {
 		metrics.HTTPRangeOpenRequests.Dec()
 		return err
diff --git a/internal/httprange/http_reader_test.go b/internal/httprange/http_reader_test.go
index 97bfbf24..01edb2e5 100644
--- a/internal/httprange/http_reader_test.go
+++ b/internal/httprange/http_reader_test.go
@@ -13,7 +13,7 @@ func TestSeekAndRead(t *testing.T) {
 	testServer := newTestServer(t, nil)
 	defer testServer.Close()
 
-	resource, err := NewResource(context.Background(), testServer.URL+"/data")
+	resource, err := NewResource(context.Background(), testServer.URL+"/data", testClient)
 	require.NoError(t, err)
 
 	tests := map[string]struct {
diff --git a/internal/httprange/resource.go b/internal/httprange/resource.go
index 8b908fe8..4ef5f0ea 100644
--- a/internal/httprange/resource.go
+++ b/internal/httprange/resource.go
@@ -20,6 +20,8 @@ type Resource struct {
 
 	url atomic.Value
 	err atomic.Value
+
+	httpClient *http.Client
 }
 
 func (r *Resource) URL() string {
@@ -67,8 +69,8 @@ func (r *Resource) Request() (*http.Request, error) {
 	return req, nil
 }
 
-func NewResource(ctx context.Context, url string) (*Resource, error) {
-	// the `h.URL` is likely pre-signed URL that only supports GET requests
+func NewResource(ctx context.Context, url string, httpClient *http.Client) (*Resource, error) {
+	// the `h.URL` is likely pre-signed URL or a file:// scheme that only supports GET requests
 	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
 		return nil, err
@@ -94,6 +96,7 @@ func NewResource(ctx context.Context, url string) (*Resource, error) {
 	resource := &Resource{
 		ETag:         res.Header.Get("ETag"),
 		LastModified: res.Header.Get("Last-Modified"),
+		httpClient:   httpClient,
 	}
 
 	resource.SetURL(url)
diff --git a/internal/httprange/resource_test.go b/internal/httprange/resource_test.go
index 1d6481fc..6b1d9c72 100644
--- a/internal/httprange/resource_test.go
+++ b/internal/httprange/resource_test.go
@@ -91,7 +91,7 @@ func TestNewResource(t *testing.T) {
 			}))
 			defer testServer.Close()
 
-			got, err := NewResource(context.Background(), testServer.URL+tt.url)
+			got, err := NewResource(context.Background(), testServer.URL+tt.url, testClient)
 			if tt.expectedErrMsg != "" {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), tt.expectedErrMsg)
diff --git a/internal/httptransport/metered_round_tripper.go b/internal/httptransport/metered_round_tripper.go
index fc652086..ff79faca 100644
--- a/internal/httptransport/metered_round_tripper.go
+++ b/internal/httptransport/metered_round_tripper.go
@@ -39,7 +39,7 @@ func NewMeteredRoundTripper(transport *http.Transport, name string, tracerVec, d
 	}
 }
 
-// RoundTripper wraps the original http.Transport into a meteredRoundTripper which
+// RoundTrip wraps the original http.Transport into a meteredRoundTripper which
 // reports metrics on request duration, tracing and request count
 func (mrt *meteredRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
 	start := time.Now()
@@ -82,3 +82,9 @@ func (mrt *meteredRoundTripper) logResponse(req *http.Request, resp *http.Respon
 		l.Traceln("response")
 	}
 }
+
+// RegisterProtocol allows to call RegisterProtocol on the meteredRoundTripper's transport
+// outside of this package
+func (mrt *meteredRoundTripper) RegisterProtocol(scheme string, rt http.RoundTripper) {
+	mrt.next.(*http.Transport).RegisterProtocol(scheme, rt)
+}
diff --git a/internal/httptransport/transport.go b/internal/httptransport/transport.go
index fcadc5fe..0b437a67 100644
--- a/internal/httptransport/transport.go
+++ b/internal/httptransport/transport.go
@@ -28,6 +28,12 @@ var (
 	DefaultTransport = NewTransport()
 )
 
+// Transport wraps a RoundTripper so it can be extended and modified outside of this package
+type Transport interface {
+	http.RoundTripper
+	RegisterProtocol(scheme string, rt http.RoundTripper)
+}
+
 // NewTransport initializes an http.Transport with a custom dialer that includes TLS Root CAs.
 // It sets default connection values such as timeouts and max idle connections.
 func NewTransport() *http.Transport {
diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go
index e64a761a..249dae1f 100644
--- a/internal/serving/disk/zip/serving_test.go
+++ b/internal/serving/disk/zip/serving_test.go
@@ -4,6 +4,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"testing"
 	"time"
 
@@ -18,6 +19,12 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public-without-dirs.zip")
 	defer cleanup()
 
+	wd, err := os.Getwd()
+	require.NoError(t, err)
+
+	httpURL := testServerURL + "/public.zip"
+	fileURL := "file://" + wd + "/group/zip.gitlab.io/public-without-dirs.zip"
+
 	tests := map[string]struct {
 		vfsPath        string
 		path           string
@@ -25,19 +32,37 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 		expectedBody   string
 	}{
 		"accessing /index.html": {
-			vfsPath:        testServerURL + "/public.zip",
+			vfsPath:        httpURL,
+			path:           "/index.html",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+		},
+		"accessing /index.html from disk": {
+			vfsPath:        fileURL,
 			path:           "/index.html",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 		},
 		"accessing /": {
-			vfsPath:        testServerURL + "/public.zip",
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+		},
+		"accessing / from disk": {
+			vfsPath:        fileURL,
 			path:           "/",
 			expectedStatus: http.StatusOK,
 			expectedBody:   "zip.gitlab.io/project/index.html\n",
 		},
 		"accessing without /": {
-			vfsPath:        testServerURL + "/public.zip",
+			vfsPath:        httpURL,
+			path:           "",
+			expectedStatus: http.StatusFound,
+			expectedBody:   `<a href="//zip.gitlab.io/zip/">Found</a>.`,
+		},
+		"accessing without / from disk": {
+			vfsPath:        fileURL,
 			path:           "",
 			expectedStatus: http.StatusFound,
 			expectedBody:   `<a href="//zip.gitlab.io/zip/">Found</a>.`,
@@ -53,6 +78,11 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			path:           "/index.html",
 			expectedStatus: http.StatusInternalServerError,
 		},
+		"accessing file:// outside of allowedPaths": {
+			vfsPath:        "file:///some/file/outside/path",
+			path:           "/index.html",
+			expectedStatus: http.StatusInternalServerError,
+		},
 	}
 
 	cfg := &config.Config{
@@ -61,11 +91,12 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			CleanupInterval:    5 * time.Second,
 			RefreshInterval:    5 * time.Second,
 			OpenTimeout:        5 * time.Second,
+			AllowedPaths:       []string{wd},
 		},
 	}
 
 	s := Instance()
-	err := s.Reconfigure(cfg)
+	err = s.Reconfigure(cfg)
 	require.NoError(t, err)
 
 	for name, test := range tests {
diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go
index 9826cdd6..981881c0 100644
--- a/internal/vfs/zip/archive.go
+++ b/internal/vfs/zip/archive.go
@@ -119,7 +119,7 @@ func (a *zipArchive) readArchive(url string) {
 	ctx, cancel := context.WithTimeout(context.Background(), a.openTimeout)
 	defer cancel()
 
-	a.resource, a.err = httprange.NewResource(ctx, url)
+	a.resource, a.err = httprange.NewResource(ctx, url, a.fs.httpClient)
 	if a.err != nil {
 		metrics.ZipOpened.WithLabelValues("error").Inc()
 		return
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index b27424c6..b69522f9 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -3,10 +3,14 @@ package zip
 import (
 	"context"
 	"errors"
+	"net/http"
 	"net/url"
 	"sync"
 	"time"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/httpfs"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/httptransport"
+
 	"github.com/patrickmn/go-cache"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
@@ -45,6 +49,7 @@ type zipVFS struct {
 	readlinkCache   *lruCache
 
 	archiveCount int64
+	httpClient   *http.Client
 }
 
 // New creates a zipVFS instance that can be used by a serving request
@@ -54,6 +59,19 @@ func New(cfg *config.ZipServing) vfs.VFS {
 		cacheRefreshInterval:    cfg.RefreshInterval,
 		cacheCleanupInterval:    cfg.CleanupInterval,
 		openTimeout:             cfg.OpenTimeout,
+		httpClient: &http.Client{
+			// TODO: make this timeout configurable
+			// https://gitlab.com/gitlab-org/gitlab-pages/-/issues/457
+			Timeout: 30 * time.Minute,
+			Transport: httptransport.NewMeteredRoundTripper(
+				httptransport.NewTransport(),
+				"zip_vfs",
+				metrics.HTTPRangeTraceDuration,
+				metrics.HTTPRangeRequestDuration,
+				metrics.HTTPRangeRequestsTotal,
+				httptransport.DefaultTTFBTimeout,
+			),
+		},
 	}
 
 	zipVFS.resetCache()
@@ -76,11 +94,27 @@ func (fs *zipVFS) Reconfigure(cfg *config.Config) error {
 	fs.cacheRefreshInterval = cfg.Zip.RefreshInterval
 	fs.cacheCleanupInterval = cfg.Zip.CleanupInterval
 
+	if err := fs.reconfigureTransport(cfg); err != nil {
+		return err
+	}
+
 	fs.resetCache()
 
 	return nil
 }
 
+func (fs *zipVFS) reconfigureTransport(cfg *config.Config) error {
+	fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths)
+	if err != nil {
+		return err
+	}
+
+	fs.httpClient.Transport.(httptransport.Transport).
+		RegisterProtocol("file", http.NewFileTransport(fsTransport))
+
+	return nil
+}
+
 func (fs *zipVFS) resetCache() {
 	fs.cache = cache.New(fs.cacheExpirationInterval, fs.cacheCleanupInterval)
 	fs.cache.OnEvicted(func(s string, i interface{}) {
diff --git a/main.go b/main.go
index c6b2c451..9921bc96 100644
--- a/main.go
+++ b/main.go
@@ -160,6 +160,8 @@ func setGitLabAPISecretKey(secretFile string, config *appConfig) {
 func configFromFlags() appConfig {
 	var config appConfig
 
+	config.PagesRoot = *pagesRoot
+
 	config.Domain = strings.ToLower(*pagesDomain)
 	config.RedirectHTTP = *redirectHTTP
 	config.HTTP2 = *useHTTP2
@@ -343,7 +345,7 @@ func appMain() {
 	}
 
 	if *daemonUID != 0 || *daemonGID != 0 {
-		if err := daemonize(config, *daemonUID, *daemonGID, *daemonInplaceChroot, *pagesRoot); err != nil {
+		if err := daemonize(config, *daemonUID, *daemonGID, *daemonInplaceChroot, config.PagesRoot); err != nil {
 			errortracking.Capture(err)
 			fatal(err, "could not create pages daemon")
 		}