diff options
-rw-r--r-- | internal/source/gitlab/cache/cache.go | 4 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry.go | 107 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry_test.go | 153 | ||||
-rw-r--r-- | internal/source/gitlab/cache/memstore.go | 20 |
4 files changed, 242 insertions, 42 deletions
diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go index 77291033..71b8e745 100644 --- a/internal/source/gitlab/cache/cache.go +++ b/internal/source/gitlab/cache/cache.go @@ -80,14 +80,14 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup { } if entry.NeedsRefresh() { - entry.Refresh(c.client, c.store) + entry.Refresh(c.store) metrics.DomainsSourceCacheHit.Inc() return entry.Lookup() } metrics.DomainsSourceCacheMiss.Inc() - return entry.Retrieve(ctx, c.client) + return entry.Retrieve(ctx) } // Status calls the client Status to check connectivity with the API diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index c960be8a..0b980774 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -3,9 +3,11 @@ package cache import ( "context" "errors" + "os" "sync" "time" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) @@ -13,27 +15,30 @@ import ( // holds a pointer to *api.Lookup when the domain lookup has been retrieved // successfully type Entry struct { - domain string - created time.Time - retrieve *sync.Once - refresh *sync.Once - mux *sync.RWMutex - retrieved chan struct{} - response *api.Lookup - refreshTimeout time.Duration - retriever *Retriever + domain string + created time.Time + refreshedOriginalTimestamp time.Time + retrieve *sync.Once + refresh *sync.Once + mux *sync.RWMutex + retrieved chan struct{} + response *api.Lookup + refreshTimeout time.Duration + expirationTimeout time.Duration + retriever *Retriever } -func newCacheEntry(domain string, refreshTimeout time.Duration, retriever *Retriever) *Entry { +func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Duration, retriever *Retriever) *Entry { return &Entry{ - domain: domain, - created: time.Now(), - retrieve: &sync.Once{}, - refresh: &sync.Once{}, - mux: &sync.RWMutex{}, - retrieved: make(chan struct{}), - refreshTimeout: refreshTimeout, - retriever: retriever, + domain: domain, + created: time.Now(), + retrieve: &sync.Once{}, + refresh: &sync.Once{}, + mux: &sync.RWMutex{}, + retrieved: make(chan struct{}), + refreshTimeout: refreshTimeout, + expirationTimeout: entryExpirationTimeout, + retriever: retriever, } } @@ -43,7 +48,7 @@ func (e *Entry) IsUpToDate() bool { e.mux.RLock() defer e.mux.RUnlock() - return e.isResolved() && !e.isExpired() + return e.isResolved() && !e.isOutdated() } // NeedsRefresh return true if the entry has been resolved correctly but it has @@ -52,7 +57,7 @@ func (e *Entry) NeedsRefresh() bool { e.mux.RLock() defer e.mux.RUnlock() - return e.isResolved() && e.isExpired() + return e.isResolved() && e.isOutdated() } // Lookup returns a retriever Lookup response. @@ -64,7 +69,7 @@ func (e *Entry) Lookup() *api.Lookup { } // Retrieve perform a blocking retrieval of the cache entry response. -func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lookup) { +func (e *Entry) Retrieve(ctx context.Context) (lookup *api.Lookup) { // We run the code within an additional func() to run both `e.setResponse` // and `e.retrieve.Retrieve` asynchronously. e.retrieve.Do(func() { go func() { e.setResponse(e.retriever.Retrieve(e.domain)) }() }) @@ -79,17 +84,30 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo return lookup } -// Refresh will update the entry in the store only when it gets resolved. -func (e *Entry) Refresh(client api.Client, store Store) { +// Refresh will update the entry in the store only when it gets resolved successfully. +// If an existing successful entry exists, it will only be replaced if the new resolved +// entry is successful too. +// Errored refreshed Entry responses will not replace the previously successful entry.response +// for a maximum time of e.expirationTimeout. +func (e *Entry) Refresh(store Store) { e.refresh.Do(func() { - go func() { - entry := newCacheEntry(e.domain, e.refreshTimeout, e.retriever) + go e.refreshFunc(store) + }) +} - entry.Retrieve(context.Background(), client) +func (e *Entry) refreshFunc(store Store) { + entry := newCacheEntry(e.domain, e.refreshTimeout, e.expirationTimeout, e.retriever) - store.ReplaceOrCreate(e.domain, entry) - }() - }) + entry.Retrieve(context.Background()) + + // do not replace existing Entry `e.response` when `entry.response` has an error + // and `e` has not expired. See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/281. + if !e.isExpired() && entry.hasTemporaryError() { + entry.response = e.response + entry.refreshedOriginalTimestamp = e.created + } + + store.ReplaceOrCreate(e.domain, entry) } func (e *Entry) setResponse(lookup api.Lookup) { @@ -100,10 +118,39 @@ func (e *Entry) setResponse(lookup api.Lookup) { close(e.retrieved) } -func (e *Entry) isExpired() bool { +func (e *Entry) isOutdated() bool { + if !e.refreshedOriginalTimestamp.IsZero() { + return time.Since(e.refreshedOriginalTimestamp) > e.refreshTimeout + } + return time.Since(e.created) > e.refreshTimeout } func (e *Entry) isResolved() bool { return e.response != nil } + +func (e *Entry) isExpired() bool { + if !e.refreshedOriginalTimestamp.IsZero() { + return time.Since(e.refreshedOriginalTimestamp) > e.expirationTimeout + } + + return time.Since(e.created) > e.expirationTimeout +} + +func (e *Entry) domainExists() bool { + return !errors.Is(e.response.Error, domain.ErrDomainDoesNotExist) +} + +// hasTemporaryError checks currently refreshed entry for errors after resolving the lookup again +// and is different to domain.ErrDomainDoesNotExist (this is an edge case to prevent serving +// a page right after being deleted). +func (e *Entry) hasTemporaryError() bool { + if os.Getenv("FF_DISABLE_REFRESH_TEMPORARY_ERROR") == "true" { + return false + } + + return e.response != nil && + e.response.Error != nil && + e.domainExists() +} diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go index 36aee522..0ccbeed2 100644 --- a/internal/source/gitlab/cache/entry_test.go +++ b/internal/source/gitlab/cache/entry_test.go @@ -1,11 +1,17 @@ package cache import ( + "context" + "errors" + "net/http" + "os" "testing" "time" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/config" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) @@ -49,7 +55,7 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, nil) + entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, testCacheConfig.CacheExpiry, nil) if tt.resolved { entry.response = &api.Lookup{} } @@ -62,3 +68,148 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) { }) } } + +func TestEntryRefresh(t *testing.T) { + client := &lookupMock{ + successCount: 1, + responses: map[string]api.Lookup{ + "test.gitlab.io": api.Lookup{ + Name: "test.gitlab.io", + Domain: &api.VirtualDomain{ + LookupPaths: nil, + }, + }, + "error.gitlab.io": api.Lookup{ + Name: "error.gitlab.io", + Error: errors.New("something went wrong"), + }, + }, + } + cc := &config.Cache{ + CacheExpiry: 100 * time.Millisecond, + EntryRefreshTimeout: time.Millisecond, + RetrievalTimeout: 50 * time.Millisecond, + MaxRetrievalInterval: time.Millisecond, + MaxRetrievalRetries: 1, + } + + store := newMemStore(client, cc) + + t.Run("entry is the same after refreshed lookup has error", func(t *testing.T) { + entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever) + originalEntryCreated := entry.created + + ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout) + defer cancel() + + lookup := entry.Retrieve(ctx) + require.NoError(t, lookup.Error) + + require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh") + + entry.refreshFunc(store) + + require.True(t, client.failed, "refresh should have failed") + + storedEntry := loadEntry(t, "test.gitlab.io", store) + + require.NoError(t, storedEntry.Lookup().Error, "resolving failed but lookup should still be valid") + require.Equal(t, storedEntry.refreshedOriginalTimestamp.UnixNano(), originalEntryCreated.UnixNano(), + "refreshed entry timestamp should be the same as the original entry created timestamp") + require.Equal(t, storedEntry.Lookup(), entry.Lookup(), "lookup should be the same") + }) + + t.Run("entry is the different when FF_DISABLE_REFRESH_TEMPORARY_ERROR is set to true", func(t *testing.T) { + client.failed = false + err := os.Setenv("FF_DISABLE_REFRESH_TEMPORARY_ERROR", "true") + + entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever) + + ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout) + defer cancel() + + lookup := entry.Retrieve(ctx) + require.NoError(t, lookup.Error) + + require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh") + + require.NoError(t, err) + + entry.refreshFunc(store) + + require.True(t, client.failed, "refresh should have failed") + + storedEntry := loadEntry(t, "test.gitlab.io", store) + + require.Error(t, storedEntry.Lookup().Error, "resolving failed") + require.True(t, storedEntry.refreshedOriginalTimestamp.IsZero()) + require.NotEqual(t, storedEntry.Lookup(), entry.Lookup(), "lookup should be different") + }) + + t.Run("entry is different after it expired and calling refresh on it", func(t *testing.T) { + client.failed = false + + entry := newCacheEntry("error.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever) + + ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout) + defer cancel() + + lookup := entry.Retrieve(ctx) + require.Error(t, lookup.Error) + require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh") + + // wait for entry to expire + time.Sleep(cc.CacheExpiry) + // refreshing the entry after it has expired should create a completely new one + entry.refreshFunc(store) + + require.True(t, client.failed, "refresh should have failed") + + storedEntry := loadEntry(t, "error.gitlab.io", store) + require.NotEqual(t, storedEntry, entry, "stored entry should be different") + require.Greater(t, storedEntry.created.UnixNano(), entry.created.UnixNano(), "") + }) +} + +func loadEntry(t *testing.T, domain string, store Store) *Entry { + t.Helper() + + i, exists := store.(*memstore).store.Get(domain) + require.True(t, exists) + + return i.(*Entry) +} + +type lookupMock struct { + currentCount int + successCount int + failed bool + responses map[string]api.Lookup +} + +func (lm *lookupMock) GetLookup(ctx context.Context, domainName string) api.Lookup { + lookup := api.Lookup{ + Name: domainName, + } + + lookup, ok := lm.responses[domainName] + if !ok { + lookup.Error = domain.ErrDomainDoesNotExist + return lookup + } + + // return error after lm.successCount + lm.currentCount++ + if lm.currentCount > lm.successCount { + lm.currentCount = 0 + lm.failed = true + + lookup.Error = http.ErrServerClosed + } + + return lookup +} + +func (lm *lookupMock) Status() error { + return nil +} diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go index 0bf9bb79..8ba069e4 100644 --- a/internal/source/gitlab/cache/memstore.go +++ b/internal/source/gitlab/cache/memstore.go @@ -11,20 +11,22 @@ import ( ) type memstore struct { - store *cache.Cache - mux *sync.RWMutex - retriever *Retriever - entryRefreshTimeout time.Duration + store *cache.Cache + mux *sync.RWMutex + retriever *Retriever + entryRefreshTimeout time.Duration + entryExpirationTimeout time.Duration } func newMemStore(client api.Client, cc *config.Cache) Store { retriever := NewRetriever(client, cc.RetrievalTimeout, cc.MaxRetrievalInterval, cc.MaxRetrievalRetries) return &memstore{ - store: cache.New(cc.CacheExpiry, cc.CacheCleanupInterval), - mux: &sync.RWMutex{}, - retriever: retriever, - entryRefreshTimeout: cc.EntryRefreshTimeout, + store: cache.New(cc.CacheExpiry, cc.CacheCleanupInterval), + mux: &sync.RWMutex{}, + retriever: retriever, + entryRefreshTimeout: cc.EntryRefreshTimeout, + entryExpirationTimeout: cc.CacheExpiry, } } @@ -46,7 +48,7 @@ func (m *memstore) LoadOrCreate(domain string) *Entry { return entry.(*Entry) } - newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.retriever) + newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.entryExpirationTimeout, m.retriever) m.store.SetDefault(domain, newEntry) return newEntry |