diff options
author | Vladimir Shushlin <vshushlin@gitlab.com> | 2022-07-25 12:07:33 +0300 |
---|---|---|
committer | Vladimir Shushlin <vshushlin@gitlab.com> | 2022-07-25 12:07:33 +0300 |
commit | 4d2b5871703931acb597b414d26b2e7835bad5b5 (patch) | |
tree | cc796489b32ec2bc440fc234506e94f83a4800dc | |
parent | 3962a8f039a8f82a83e958c9526e5083a12d6f31 (diff) | |
parent | 9bf12d385279f8386716cc0af4372ebcd5ae6f9d (diff) |
Merge branch 'sh-fix-data-race-lookup' into 'master'
Fix data race with lookup paths
See merge request gitlab-org/gitlab-pages!822
-rw-r--r-- | internal/source/gitlab/api/lookup.go | 23 | ||||
-rw-r--r-- | internal/source/gitlab/client/client.go | 3 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 12 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab_test.go | 114 |
4 files changed, 116 insertions, 36 deletions
diff --git a/internal/source/gitlab/api/lookup.go b/internal/source/gitlab/api/lookup.go index 73a3ce43..f7d8987a 100644 --- a/internal/source/gitlab/api/lookup.go +++ b/internal/source/gitlab/api/lookup.go @@ -1,8 +1,31 @@ package api +import ( + "encoding/json" + "io" + "sort" +) + // Lookup defines an API lookup action with a response that GitLab sends type Lookup struct { Name string Error error Domain *VirtualDomain } + +func (l *Lookup) ParseDomain(r io.Reader) { + l.Error = json.NewDecoder(r).Decode(&l.Domain) + + if l.Domain != nil { + sortLookupsByPrefixLengthDesc(l.Domain.LookupPaths) + } +} + +// Ensure lookupPaths are sorted by prefix length to ensure the group level +// domain with prefix "/" is the last one to be checked. +// See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/576 +func sortLookupsByPrefixLengthDesc(lookups []LookupPath) { + sort.SliceStable(lookups, func(i, j int) bool { + return len(lookups[i].Prefix) > len(lookups[j].Prefix) + }) +} diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index de8ae2a5..4b6ea98d 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -2,7 +2,6 @@ package client import ( "context" - "encoding/json" "errors" "fmt" "io" @@ -128,7 +127,7 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup { }() lookup := api.Lookup{Name: host} - lookup.Error = json.NewDecoder(resp.Body).Decode(&lookup.Domain) + lookup.ParseDomain(resp.Body) return lookup } diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index abe645fe..ab5bc490 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -5,7 +5,6 @@ import ( "errors" "net/http" "path" - "sort" "strings" "gitlab.com/gitlab-org/labkit/log" @@ -76,8 +75,6 @@ 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) - for _, lookup := range response.Domain.LookupPaths { isSubPath := strings.HasPrefix(urlPath, lookup.Prefix) isRootPath := urlPath == path.Clean(lookup.Prefix) @@ -108,12 +105,3 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) { return nil, domain.ErrDomainDoesNotExist } - -// Ensure lookupPaths are sorted by prefix length to ensure the group level -// domain with prefix "/" is the last one to be checked. -// See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/576 -func sortLookupsByPrefixLengthDesc(lookups []api.LookupPath) { - sort.SliceStable(lookups, func(i, j int) bool { - return len(lookups[i].Prefix) > len(lookups[j].Prefix) - }) -} diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go index e7f80387..5a63edf2 100644 --- a/internal/source/gitlab/gitlab_test.go +++ b/internal/source/gitlab/gitlab_test.go @@ -2,21 +2,42 @@ package gitlab import ( "context" - "encoding/json" "io" "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, +} + +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 @@ -42,7 +63,7 @@ func TestGetDomain(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - mockClient := NewMockClient(t, tc.file, tc.mockLookup) + mockClient := NewMockClient(t, tc.file, tc.mockLookup, false) source := Gitlab{client: mockClient} domain, err := source.GetDomain(context.Background(), tc.domain) @@ -109,7 +130,7 @@ func TestResolve(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - mockClient := NewMockClient(t, tc.file, nil) + mockClient := NewMockClient(t, tc.file, nil, false) source := Gitlab{client: mockClient, enableDisk: true} request := httptest.NewRequest(http.MethodGet, tc.target, nil) @@ -125,16 +146,46 @@ func TestResolve(t *testing.T) { } } +// Test validates https://gitlab.com/gitlab-org/gitlab-pages/-/issues/646 with go test -race +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) { + wg := &sync.WaitGroup{} + mockClient := NewMockClient(t, test.file, nil, true) + cache := cache.NewCache(mockClient, &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/", @@ -155,8 +206,9 @@ func TestResolveLookupPathsOrderDoesNotMatter(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - mockClient := NewMockClient(t, test.file, nil) - source := Gitlab{client: mockClient, enableDisk: true} + mockClient := NewMockClient(t, test.file, nil, true) + cache := cache.NewCache(mockClient, &testCacheConfig) + source := Gitlab{client: cache, enableDisk: true} request := httptest.NewRequest(http.MethodGet, test.target, nil) @@ -171,17 +223,19 @@ func TestResolveLookupPathsOrderDoesNotMatter(t *testing.T) { } } -func NewMockClient(t *testing.T, file string, mockedLookup *api.Lookup) *mock.MockClientStub { +func NewMockClient(t *testing.T, file string, mockedLookup *api.Lookup, useCache bool) *mock.MockClientStub { mockCtrl := gomock.NewController(t) mockClient := mock.NewMockClientStub(mockCtrl) - mockClient.EXPECT(). - Resolve(gomock.Any(), gomock.Any()). - DoAndReturn(func(ctx context.Context, domain string) *api.Lookup { - lookup := mockClient.GetLookup(ctx, domain) - return &lookup - }). - Times(1) + if !useCache { + mockClient.EXPECT(). + Resolve(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, domain string) *api.Lookup { + lookup := mockClient.GetLookup(ctx, domain) + return &lookup + }). + Times(1) + } mockClient.EXPECT(). GetLookup(gomock.Any(), gomock.Any()). @@ -199,7 +253,7 @@ func NewMockClient(t *testing.T, file string, mockedLookup *api.Lookup) *mock.Mo } defer f.Close() - lookup.Error = json.NewDecoder(f).Decode(&lookup.Domain) + lookup.ParseDomain(f) return lookup }). @@ -207,3 +261,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() +} |