Merge branch 'feat/etag' into 'master'

feat: implement ETag support for zip serving

Closes #82

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

5 files changed, 210 insertions, 25 deletions

diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index ec97b128..b83425eb 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -109,7 +109,7 @@ func (reader *Reader) tryFile(h serving.Handler) bool {
 		return true
 	}
 
-	return reader.serveFile(ctx, h.Writer, h.Request, root, fullPath, h.LookupPath.HasAccessControl)
+	return reader.serveFile(ctx, h.Writer, h.Request, root, fullPath, h.LookupPath.SHA256, h.LookupPath.HasAccessControl)
 }
 
 func redirectPath(request *http.Request) string {
@@ -194,7 +194,7 @@ func (reader *Reader) resolvePath(ctx context.Context, root vfs.Root, subPath ..
 	return fullPath, nil
 }
 
-func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath string, accessControl bool) bool {
+func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath, sha string, accessControl bool) bool {
 	fullPath := reader.handleContentEncoding(ctx, w, r, root, origPath)
 
 	file, err := root.Open(ctx, fullPath)
@@ -211,6 +211,9 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h
 		return true
 	}
 
+	ce := w.Header().Get("Content-Encoding")
+	w.Header().Set("ETag", fmt.Sprintf("%q", etag(ce, sha)))
+
 	if !accessControl {
 		// Set caching headers
 		w.Header().Set("Cache-Control", "max-age=600")
@@ -242,6 +245,13 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h
 	return true
 }
 
+func etag(contentEncoding, sha string) string {
+	if contentEncoding == "" {
+		return sha
+	}
+	return fmt.Sprintf("%s-%s", sha, contentEncoding)
+}
+
 func (reader *Reader) serveCustomFile(ctx context.Context, w http.ResponseWriter, r *http.Request, code int, root vfs.Root, origPath string) error {
 	fullPath := reader.handleContentEncoding(ctx, w, r, root, origPath)
 
diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go
index fefc8cbe..2b603b5d 100644
--- a/internal/serving/disk/zip/serving_test.go
+++ b/internal/serving/disk/zip/serving_test.go
@@ -2,6 +2,8 @@ package zip
 
 import (
 	"crypto/sha256"
+	"encoding/hex"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -59,6 +61,15 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 				"If-Modified-Since": {time.Now().Format(http.TimeFormat)},
 			},
 		},
+		"accessing / If-Modified-Since fails": {
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+			extraHeaders: http.Header{
+				"If-Modified-Since": {time.Now().AddDate(-10, 0, 0).Format(http.TimeFormat)},
+			},
+		},
 		"accessing / If-Unmodified-Since": {
 			vfsPath:        httpURL,
 			path:           "/",
@@ -67,6 +78,15 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 				"If-Unmodified-Since": {time.Now().AddDate(-10, 0, 0).Format(http.TimeFormat)},
 			},
 		},
+		"accessing / If-Unmodified-Since fails": {
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+			extraHeaders: http.Header{
+				"If-Unmodified-Since": {time.Now().Format(http.TimeFormat)},
+			},
+		},
 		"accessing / from disk": {
 			vfsPath:        fileURL,
 			path:           "/",
@@ -77,13 +97,13 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			vfsPath:        httpURL,
 			path:           "",
 			expectedStatus: http.StatusFound,
-			expectedBody:   `<a href="//zip.gitlab.io/zip/">Found</a>.`,
+			expectedBody:   "<a href=\"//zip.gitlab.io/zip/\">Found</a>.\n\n",
 		},
 		"accessing without / from disk": {
 			vfsPath:        fileURL,
 			path:           "",
 			expectedStatus: http.StatusFound,
-			expectedBody:   `<a href="//zip.gitlab.io/zip/">Found</a>.`,
+			expectedBody:   "<a href=\"//zip.gitlab.io/zip/\">Found</a>.\n\n",
 		},
 		"accessing archive that is 404": {
 			vfsPath: testServerURL + "/invalid.zip",
@@ -101,6 +121,48 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			path:           "/index.html",
 			expectedStatus: http.StatusInternalServerError,
 		},
+		"accessing / If-None-Match": {
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusNotModified,
+			extraHeaders: http.Header{
+				"If-None-Match": {fmt.Sprintf("%q", sha(httpURL))},
+			},
+		},
+		"accessing / If-None-Match fails": {
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+			extraHeaders: http.Header{
+				"If-None-Match": {fmt.Sprintf("%q", "badetag")},
+			},
+		},
+		"accessing / If-Match": {
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusOK,
+			expectedBody:   "zip.gitlab.io/project/index.html\n",
+			extraHeaders: http.Header{
+				"If-Match": {fmt.Sprintf("%q", sha(httpURL))},
+			},
+		},
+		"accessing / If-Match fails": {
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusPreconditionFailed,
+			extraHeaders: http.Header{
+				"If-Match": {fmt.Sprintf("%q", "wrongetag")},
+			},
+		},
+		"accessing / If-Match fails2": {
+			vfsPath:        httpURL,
+			path:           "/",
+			expectedStatus: http.StatusPreconditionFailed,
+			extraHeaders: http.Header{
+				"If-Match": {","},
+			},
+		},
 	}
 
 	cfg := &config.Config{
@@ -123,10 +185,9 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 			w.Code = 0 // ensure that code is not set, and it is being set by handler
 			r := httptest.NewRequest(http.MethodGet, "http://zip.gitlab.io/zip"+test.path, nil)
 
-			r.Header = test.extraHeaders
-
-			sha := sha256.Sum256([]byte(test.vfsPath))
-			etag := string(sha[:])
+			if test.extraHeaders != nil {
+				r.Header = test.extraHeaders
+			}
 
 			handler := serving.Handler{
 				Writer:  w,
@@ -134,7 +195,7 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 				LookupPath: &serving.LookupPath{
 					Prefix: "/zip/",
 					Path:   test.vfsPath,
-					SHA256: etag,
+					SHA256: sha(test.vfsPath),
 				},
 				SubPath: test.path,
 			}
@@ -156,13 +217,22 @@ func TestZip_ServeFileHTTP(t *testing.T) {
 
 			if test.expectedStatus == http.StatusOK {
 				require.NotEmpty(t, resp.Header.Get("Last-Modified"))
+				require.NotEmpty(t, resp.Header.Get("ETag"))
 			}
 
-			require.Contains(t, string(body), test.expectedBody)
+			if test.expectedStatus != http.StatusInternalServerError {
+				require.Equal(t, test.expectedBody, string(body))
+			}
 		})
 	}
 }
 
+func sha(path string) string {
+	sha := sha256.Sum256([]byte(path))
+	s := hex.EncodeToString(sha[:])
+	return s
+}
+
 var chdirSet = false
 
 func newZipFileServerURL(t *testing.T, zipFilePath string) (string, func()) {
diff --git a/test/acceptance/auth_test.go b/test/acceptance/auth_test.go
index ad5c343a..d115dba5 100644
--- a/test/acceptance/auth_test.go
+++ b/test/acceptance/auth_test.go
@@ -539,7 +539,10 @@ func testAccessControl(t *testing.T, runPages runPagesFunc) {
 			defer rsp3.Body.Close()
 
 			require.Equal(t, tt.status, rsp3.StatusCode)
-			require.Equal(t, "", rsp3.Header.Get("Cache-Control"))
+
+			// Make sure there are no cache headers
+			require.Empty(t, rsp3.Header.Values("Cache-Control"))
+			require.Empty(t, rsp3.Header.Values("Expires"))
 
 			if tt.redirectBack {
 				loc3, err := url.Parse(rsp3.Header.Get("Location"))
diff --git a/test/acceptance/testdata/api_responses.go b/test/acceptance/testdata/api_responses.go
index 5061dcaf..baa70f8f 100644
--- a/test/acceptance/testdata/api_responses.go
+++ b/test/acceptance/testdata/api_responses.go
@@ -2,6 +2,7 @@ package testdata
 
 import (
 	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -165,7 +166,7 @@ func generateVirtualDomainFromDir(dir, rootDomain string, perPrefixConfig map[st
 
 			sourcePath := fmt.Sprintf("file://%s", wd+"/"+dir+project)
 			sum := sha256.Sum256([]byte(sourcePath))
-			sha := string(sum[:])
+			sha := hex.EncodeToString(sum[:])
 
 			lookupPath := api.LookupPath{
 				ProjectID:     cfg.projectID,
@@ -204,7 +205,7 @@ func customDomain(config projectConfig) responseFn {
 
 		sourcePath := fmt.Sprintf("file://%s/%s/public.zip", wd, config.pathOnDisk)
 		sum := sha256.Sum256([]byte(sourcePath))
-		sha := string(sum[:])
+		sha := hex.EncodeToString(sum[:])
 
 		return api.VirtualDomain{
 			Certificate: "",
diff --git a/test/acceptance/zip_test.go b/test/acceptance/zip_test.go
index fcaecf25..42734d14 100644
--- a/test/acceptance/zip_test.go
+++ b/test/acceptance/zip_test.go
@@ -1,6 +1,7 @@
 package acceptance_test
 
 import (
+	"fmt"
 	"io"
 	"net"
 	"net/http"
@@ -9,9 +10,14 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/feature"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
 
 func TestZipServing(t *testing.T) {
+	testhelpers.StubFeatureFlagValue(t, feature.HandleCacheHeaders.EnvVariable, true)
+
 	runObjectStorage(t, "../../shared/pages/group/zip.gitlab.io/public.zip")
 
 	RunPagesProcess(t,
@@ -88,6 +94,11 @@ func TestZipServing(t *testing.T) {
 
 			require.Equal(t, tt.expectedStatusCode, response.StatusCode)
 
+			if tt.expectedStatusCode == http.StatusOK {
+				require.NotEmpty(t, response.Header.Get("ETag"))
+				require.NotEmpty(t, response.Header.Get("Last-Modified"))
+			}
+
 			body, err := io.ReadAll(response.Body)
 			require.NoError(t, err)
 
@@ -108,47 +119,132 @@ func TestZipServingCache(t *testing.T) {
 		urlSuffix          string
 		expectedStatusCode int
 		expectedContent    string
-		extraHeaders       http.Header
+		extraHeaders       func(string) http.Header
 	}{
+		"base_domain_if_none_match": {
+			host:               "zip.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusNotModified,
+			extraHeaders: func(etag string) http.Header {
+				return http.Header{
+					"If-None-Match": {etag},
+				}
+			},
+		},
+		"base_domain_if_none_match_fail": {
+			host:               "zip.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "zip.gitlab.io/project/index.html\n",
+			extraHeaders: func(etag string) http.Header {
+				return http.Header{
+					"If-None-Match": {fmt.Sprintf("%q", "badetag")},
+				}
+			},
+		},
+		"base_domain_if_match": {
+			host:               "zip.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "zip.gitlab.io/project/index.html\n",
+			extraHeaders: func(etag string) http.Header {
+				return http.Header{
+					"If-Match": {etag},
+				}
+			},
+		},
+		"base_domain_if_match_fail": {
+			host:               "zip.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusPreconditionFailed,
+			extraHeaders: func(etag string) http.Header {
+				return http.Header{
+					"If-Match": {fmt.Sprintf("%q", "wrongetag")},
+				}
+			},
+		},
+		"base_domain_if_match_fail2": {
+			host:               "zip.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusPreconditionFailed,
+			extraHeaders: func(etag string) http.Header {
+				return http.Header{
+					"If-Match": {","},
+				}
+			},
+		},
 		"base_domain_if_modified": {
 			host:               "zip.gitlab.io",
 			urlSuffix:          "/",
 			expectedStatusCode: http.StatusNotModified,
-			extraHeaders: http.Header{
-				"If-Modified-Since": {time.Now().Format(http.TimeFormat)},
+			extraHeaders: func(string) http.Header {
+				return http.Header{
+					"If-Modified-Since": {time.Now().Format(http.TimeFormat)},
+				}
+			},
+		},
+		"base_domain_if_modified_fails": {
+			host:               "zip.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "zip.gitlab.io/project/index.html\n",
+			extraHeaders: func(string) http.Header {
+				return http.Header{
+					"If-Modified-Since": {time.Now().AddDate(-10, 0, 0).Format(http.TimeFormat)},
+				}
 			},
 		},
 		"base_domain_if_unmodified": {
 			host:               "zip.gitlab.io",
 			urlSuffix:          "/",
 			expectedStatusCode: http.StatusPreconditionFailed,
-			extraHeaders: http.Header{
-				"If-Unmodified-Since": {time.Now().AddDate(-10, 0, 0).Format(http.TimeFormat)},
+			extraHeaders: func(string) http.Header {
+				return http.Header{
+					"If-Unmodified-Since": {time.Now().AddDate(-10, 0, 0).Format(http.TimeFormat)},
+				}
+			},
+		},
+		"base_domain_if_unmodified_fails": {
+			host:               "zip.gitlab.io",
+			urlSuffix:          "/",
+			expectedStatusCode: http.StatusOK,
+			expectedContent:    "zip.gitlab.io/project/index.html\n",
+			extraHeaders: func(string) http.Header {
+				return http.Header{
+					"If-Unmodified-Since": {time.Now().Format(http.TimeFormat)},
+				}
 			},
 		},
 	}
 
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			response, err := GetPageFromListenerWithHeaders(t, httpListener, tt.host, tt.urlSuffix, tt.extraHeaders)
+			// send a request to get the ETag
+			response, err := GetPageFromListener(t, httpListener, tt.host, tt.urlSuffix)
 			require.NoError(t, err)
 			defer response.Body.Close()
+			require.Equal(t, http.StatusOK, response.StatusCode)
 
-			require.Equal(t, tt.expectedStatusCode, response.StatusCode)
+			etag := response.Header.Get("ETag")
+			require.NotEmpty(t, etag)
 
-			if tt.expectedStatusCode == http.StatusOK {
-				require.NotEmpty(t, response.Header.Get("Last-Modified"))
-			}
+			// actual test
+			rsp, err := GetPageFromListenerWithHeaders(t, httpListener, tt.host, tt.urlSuffix, tt.extraHeaders(etag))
+			require.NoError(t, err)
+			require.Equal(t, tt.expectedStatusCode, rsp.StatusCode)
 
-			body, err := io.ReadAll(response.Body)
+			body, err := io.ReadAll(rsp.Body)
 			require.NoError(t, err)
 
-			require.Contains(t, string(body), tt.expectedContent, "content mismatch")
+			defer rsp.Body.Close()
+			require.Equal(t, tt.expectedContent, string(body), "content mismatch")
 		})
 	}
 }
 
 func TestZipServingFromDisk(t *testing.T) {
+	testhelpers.StubFeatureFlagValue(t, feature.HandleCacheHeaders.EnvVariable, true)
+
 	RunPagesProcess(t,
 		withListeners([]ListenSpec{httpListener}),
 	)
@@ -223,6 +319,11 @@ func TestZipServingFromDisk(t *testing.T) {
 
 			require.Equal(t, tt.expectedStatusCode, response.StatusCode)
 
+			if tt.expectedStatusCode == http.StatusOK {
+				require.NotEmpty(t, response.Header.Get("ETag"))
+				require.NotEmpty(t, response.Header.Get("Last-Modified"))
+			}
+
 			body, err := io.ReadAll(response.Body)
 			require.NoError(t, err)