Fix data race with lookup pathssh-fix-data-race-lookup

Since the API response for domains is cached, it's possible that
multiple clients for the same host may cause a data race when sorting
the lookup paths. To ensure this doesn't happen, the simplest solution
here is to make a copy of the structure before working with it. We can
optimize this later by ensuring the paths are sorted as desired before
we cache it.

Relates to https://gitlab.com/gitlab-org/gitlab-pages/-/issues/646

Changelog: fixed

2 files changed, 107 insertions, 12 deletions

diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index abe645fe..233973a8 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -74,11 +74,12 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) {
 	}
 
 	urlPath := path.Clean(r.URL.Path)
-	size := len(response.Domain.LookupPaths)
 
-	sortLookupsByPrefixLengthDesc(response.Domain.LookupPaths)
+	lookupPaths := append([]api.LookupPath{}, response.Domain.LookupPaths...)
+	size := len(lookupPaths)
+	sortLookupsByPrefixLengthDesc(lookupPaths)
 
-	for _, lookup := range response.Domain.LookupPaths {
+	for _, lookup := range lookupPaths {
 		isSubPath := strings.HasPrefix(urlPath, lookup.Prefix)
 		isRootPath := urlPath == path.Clean(lookup.Prefix)
 
@@ -103,7 +104,8 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) {
 	logging.LogRequest(r).WithError(domain.ErrDomainDoesNotExist).WithFields(
 		log.Fields{
 			"lookup_paths_count": size,
-			"lookup_paths":       response.Domain.LookupPaths,
+			"lookup_paths":       lookupPaths,
+			"url_path":           urlPath,
 		}).Error("could not find project lookup path")
 
 	return nil, domain.ErrDomainDoesNotExist
diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go
index e7f80387..3d0bab46 100644
--- a/internal/source/gitlab/gitlab_test.go
+++ b/internal/source/gitlab/gitlab_test.go
@@ -7,16 +7,43 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/cache"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/mock"
 )
 
+var testCacheConfig = config.Cache{
+	CacheExpiry:          time.Second,
+	CacheCleanupInterval: time.Second / 2,
+	EntryRefreshTimeout:  time.Second / 2,
+	RetrievalTimeout:     time.Second,
+	MaxRetrievalInterval: time.Second / 3,
+	MaxRetrievalRetries:  3,
+}
+
+const (
+	defaultClientConnTimeout = 10 * time.Second
+	defaultJWTTokenExpiry    = 30 * time.Second
+)
+
+type lookupPathTest struct {
+	file                string
+	target              string
+	expectedPrefix      string
+	expectedPath        string
+	expectedSubPath     string
+	expectedIsNamespace bool
+}
+
 func TestGetDomain(t *testing.T) {
 	tests := map[string]struct {
 		file          string
@@ -125,16 +152,66 @@ func TestResolve(t *testing.T) {
 	}
 }
 
+// Test validates https://gitlab.com/gitlab-org/gitlab-pages/-/issues/646
+func TestResolveLookupPathsConcurrentNetRequests(t *testing.T) {
+	tests := map[string]lookupPathTest{
+		"when requesting the group root project with root path": {
+			file:                "client/testdata/group-first.gitlab.io.json",
+			target:              "https://group-first.gitlab.io:443/",
+			expectedPrefix:      "/",
+			expectedPath:        "some/path/group/",
+			expectedSubPath:     "",
+			expectedIsNamespace: true,
+		},
+		"when requesting another project with path": {
+			file:                "client/testdata/group-first.gitlab.io.json",
+			target:              "https://group-first.gitlab.io:443/my/second-project/index.html",
+			expectedPrefix:      "/my/second-project/",
+			expectedPath:        "some/path/to/project-2/",
+			expectedSubPath:     "index.html",
+			expectedIsNamespace: false,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			mux := http.NewServeMux()
+
+			mux.HandleFunc("/api/v4/internal/pages", func(w http.ResponseWriter, r *http.Request) {
+				f, err := os.Open(test.file)
+				if err != nil {
+					w.WriteHeader(http.StatusBadGateway)
+					return
+				}
+				defer f.Close()
+
+				body, err := io.ReadAll(f)
+				require.NoError(t, err)
+				w.Write(body)
+			})
+
+			server := httptest.NewServer(mux)
+			defer server.Close()
+
+			client, err := client.NewClient(server.URL, []byte("secret"), defaultClientConnTimeout, defaultJWTTokenExpiry)
+			require.NoError(t, err)
+
+			wg := &sync.WaitGroup{}
+			cache := cache.NewCache(client, &testCacheConfig)
+
+			for i := 0; i < 3; i++ {
+				go sendResolveRequest(t, wg, cache, test)
+				wg.Add(1)
+			}
+
+			wg.Wait()
+		})
+	}
+}
+
 // Test proves fix for https://gitlab.com/gitlab-org/gitlab-pages/-/issues/576
 func TestResolveLookupPathsOrderDoesNotMatter(t *testing.T) {
-	tests := map[string]struct {
-		file                string
-		target              string
-		expectedPrefix      string
-		expectedPath        string
-		expectedSubPath     string
-		expectedIsNamespace bool
-	}{
+	tests := map[string]lookupPathTest{
 		"when requesting the group root project with root path": {
 			file:                "client/testdata/group-first.gitlab.io.json",
 			target:              "https://group-first.gitlab.io:443/",
@@ -207,3 +284,19 @@ func NewMockClient(t *testing.T, file string, mockedLookup *api.Lookup) *mock.Mo
 
 	return mockClient
 }
+
+func sendResolveRequest(t *testing.T, wg *sync.WaitGroup, resolver api.Resolver, test lookupPathTest) {
+	request := httptest.NewRequest(http.MethodGet, test.target, nil)
+
+	source := Gitlab{client: resolver, enableDisk: true}
+
+	response, err := source.Resolve(request)
+	require.NoError(t, err)
+
+	require.Equal(t, test.expectedPrefix, response.LookupPath.Prefix)
+	require.Equal(t, test.expectedPath, response.LookupPath.Path)
+	require.Equal(t, test.expectedSubPath, response.SubPath)
+	require.Equal(t, test.expectedIsNamespace, response.LookupPath.IsNamespaceProject)
+
+	wg.Done()
+}