From 194bf0568619be8df50a51ed5979a34d4f697b7d Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Fri, 24 Jul 2020 15:38:42 +1000 Subject: [PATCH 1/3] Add polling to the domains package Fix linter --- internal/source/domains.go | 64 +++++++++++++++++++++++++++++---- internal/source/domains_test.go | 38 +++++++++----------- 2 files changed, 73 insertions(+), 29 deletions(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index 77e1aa1d4..de4136f84 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -2,7 +2,9 @@ package source import ( "errors" + "fmt" "regexp" + "sync" "time" log "github.com/sirupsen/logrus" @@ -33,6 +35,7 @@ func init() { // currently using two sources during the transition to the new GitLab domains // source. type Domains struct { + mu *sync.RWMutex gitlab Source disk *disk.Disk // legacy disk source } @@ -44,19 +47,63 @@ func NewDomains(config Config) (*Domains, error) { // TODO: choose domain source config via config.DomainConfigSource() // https://gitlab.com/gitlab-org/gitlab/-/issues/217912 + domains := &Domains{ + mu: &sync.RWMutex{}, + disk: disk.New(), + } + + if err := domains.setGitlabSource(config); err != nil { + log.WithError(err).Error("failed to set GitLab domains source") + return domains, nil + } + + return domains, nil +} + +func (d *Domains) setGitlabSource(config Config) error { + d.mu.Lock() + defer d.mu.Unlock() + if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { - return &Domains{disk: disk.New()}, nil + return nil } - gitlab, err := gitlab.New(config) + gitlabClient, err := gitlab.New(config) if err != nil { - return nil, err + return fmt.Errorf("failed to set GitLab client: %w", err) } - return &Domains{ - gitlab: gitlab, - disk: disk.New(), - }, nil + d.gitlab = gitlabClient + + d.checkGitLabStatus(gitlabClient) + + return nil +} + +func (d *Domains) checkGitLabStatus(gitlabClient *gitlab.Gitlab) { + gitlabErr := make(chan error) + go func() { + defer close(gitlabErr) + err := gitlabClient.Poll(gitlab.DefaultPollingMaxRetries, gitlab.DefaultPollingInterval) + if err != nil { + gitlabErr <- err + } + }() + + go func() { + err := <-gitlabErr + if err != nil { + log.WithError(err).Error("failed to connect to the GitLab API") + d.disableGitlabSource() + } + }() +} + +func (d *Domains) disableGitlabSource() { + d.mu.Lock() + defer d.mu.Unlock() + + d.gitlab = nil } // GetDomain retrieves a domain information from a source. We are using two @@ -85,6 +132,9 @@ func (d *Domains) IsReady() bool { } func (d *Domains) source(domain string) Source { + d.mu.RLock() + defer d.mu.RUnlock() + if d.gitlab == nil { return d.disk } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 24008b08e..da6f38775 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -2,6 +2,7 @@ package source import ( "math/rand" + "sync" "testing" "time" @@ -67,10 +68,7 @@ func TestGetDomain(t *testing.T) { Once() defer newSource.AssertExpectations(t) - domains := &Domains{ - disk: disk.New(), - gitlab: newSource, - } + domains := newTestDomains(t, newSource) domains.GetDomain(testDomain) }) @@ -79,10 +77,7 @@ func TestGetDomain(t *testing.T) { newSource := NewMockSource() defer newSource.AssertExpectations(t) - domains := &Domains{ - disk: disk.New(), - gitlab: newSource, - } + domains := newTestDomains(t, newSource) domain, err := domains.GetDomain("domain.test.io") @@ -94,10 +89,7 @@ func TestGetDomain(t *testing.T) { newSource := NewMockSource() defer newSource.AssertExpectations(t) - domains := &Domains{ - disk: disk.New(), - gitlab: newSource, - } + domains := newTestDomains(t, newSource) domain, err := domains.GetDomain("pages-broken-poc.gitlab.io") @@ -124,10 +116,7 @@ func TestGetDomain(t *testing.T) { Once() defer newSource.AssertExpectations(t) - domains := &Domains{ - disk: disk.New(), - gitlab: newSource, - } + domains := newTestDomains(t, newSource) domains.GetDomain(testDomain) }) @@ -156,8 +145,6 @@ func TestGetDomainWithIncrementalrolloutOfGitLabSource(t *testing.T) { // Generates FNV 2643293380, 2643293380 % 100 = 80 domain80 := "test-domain-b.com" - diskSource := disk.New() - gitlabSourceConfig.Domains.Rollout.Percentage = 80 type testDomain struct { @@ -203,10 +190,7 @@ func TestGetDomainWithIncrementalrolloutOfGitLabSource(t *testing.T) { } defer gitlabSource.AssertExpectations(t) - domains := &Domains{ - disk: diskSource, - gitlab: gitlabSource, - } + domains := newTestDomains(t, gitlabSource) gitlabSourceConfig.Domains.Rollout.Stickiness = tc.stickiness @@ -217,3 +201,13 @@ func TestGetDomainWithIncrementalrolloutOfGitLabSource(t *testing.T) { }) } } + +func newTestDomains(t *testing.T, gitlabSource *MockSource) *Domains { + t.Helper() + + return &Domains{ + mu: &sync.RWMutex{}, + disk: disk.New(), + gitlab: gitlabSource, + } +} -- GitLab From 3c58f5f641c8f8dd51358fd812cffeb6491e75cc Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 27 Jul 2020 11:13:51 +1000 Subject: [PATCH 2/3] Use IsReady instead --- internal/source/domains.go | 56 +++------------------------ internal/source/domains_test.go | 6 +++ internal/source/gitlab/gitlab.go | 8 ++++ internal/source/gitlab/gitlab_poll.go | 1 + internal/source/source.go | 1 + internal/source/source_mock.go | 6 +++ 6 files changed, 28 insertions(+), 50 deletions(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index de4136f84..09007c85e 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -2,7 +2,6 @@ package source import ( "errors" - "fmt" "regexp" "sync" "time" @@ -52,58 +51,18 @@ func NewDomains(config Config) (*Domains, error) { disk: disk.New(), } - if err := domains.setGitlabSource(config); err != nil { - log.WithError(err).Error("failed to set GitLab domains source") - return domains, nil - } - - return domains, nil -} - -func (d *Domains) setGitlabSource(config Config) error { - d.mu.Lock() - defer d.mu.Unlock() - if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { - return nil + return domains, nil } - gitlabClient, err := gitlab.New(config) + glClient, err := gitlab.New(config) if err != nil { - return fmt.Errorf("failed to set GitLab client: %w", err) + return nil, err } - d.gitlab = gitlabClient + domains.gitlab = glClient - d.checkGitLabStatus(gitlabClient) - - return nil -} - -func (d *Domains) checkGitLabStatus(gitlabClient *gitlab.Gitlab) { - gitlabErr := make(chan error) - go func() { - defer close(gitlabErr) - err := gitlabClient.Poll(gitlab.DefaultPollingMaxRetries, gitlab.DefaultPollingInterval) - if err != nil { - gitlabErr <- err - } - }() - - go func() { - err := <-gitlabErr - if err != nil { - log.WithError(err).Error("failed to connect to the GitLab API") - d.disableGitlabSource() - } - }() -} - -func (d *Domains) disableGitlabSource() { - d.mu.Lock() - defer d.mu.Unlock() - - d.gitlab = nil + return domains, nil } // GetDomain retrieves a domain information from a source. We are using two @@ -132,10 +91,7 @@ func (d *Domains) IsReady() bool { } func (d *Domains) source(domain string) Source { - d.mu.RLock() - defer d.mu.RUnlock() - - if d.gitlab == nil { + if d.gitlab == nil || !d.gitlab.IsReady() { return d.disk } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index da6f38775..12c3220af 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -66,6 +66,7 @@ func TestGetDomain(t *testing.T) { newSource.On("GetDomain", testDomain). Return(&domain.Domain{Name: testDomain}, nil). Once() + newSource.On("IsReady").Return(true).Once() defer newSource.AssertExpectations(t) domains := newTestDomains(t, newSource) @@ -75,6 +76,8 @@ func TestGetDomain(t *testing.T) { t.Run("when requesting a non-test domain", func(t *testing.T) { newSource := NewMockSource() + newSource.On("IsReady").Return(true).Once() + defer newSource.AssertExpectations(t) domains := newTestDomains(t, newSource) @@ -114,6 +117,8 @@ func TestGetDomain(t *testing.T) { newSource.On("GetDomain", testDomain). Return(&domain.Domain{Name: testDomain}, nil). Once() + newSource.On("IsReady").Return(true).Once() + defer newSource.AssertExpectations(t) domains := newTestDomains(t, newSource) @@ -188,6 +193,7 @@ func TestGetDomainWithIncrementalrolloutOfGitLabSource(t *testing.T) { Once() } } + gitlabSource.On("IsReady").Return(true) defer gitlabSource.AssertExpectations(t) domains := newTestDomains(t, gitlabSource) diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 5bacb603c..2635d864c 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -104,3 +104,11 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) { return &serving.Request{Serving: defaultServing()}, errors.New("could not match lookup path") } + +// IsReady returns the value of Gitlab `isReady` which is updated by `Poll`. +func (g *Gitlab) IsReady() bool { + g.mu.RLock() + defer g.mu.RUnlock() + + return g.isReady +} diff --git a/internal/source/gitlab/gitlab_poll.go b/internal/source/gitlab/gitlab_poll.go index 706448222..9fce7250c 100644 --- a/internal/source/gitlab/gitlab_poll.go +++ b/internal/source/gitlab/gitlab_poll.go @@ -15,6 +15,7 @@ const ( // poll tries to call the /internal/pages/status API endpoint once plus // `retries` every `interval`. +// It updates the `isReady` value when successful. // TODO: Remove in https://gitlab.com/gitlab-org/gitlab/-/issues/218357 func (g *Gitlab) poll(retries int, interval time.Duration) { var err error diff --git a/internal/source/source.go b/internal/source/source.go index 4b43b8f4b..5540066c7 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -5,4 +5,5 @@ import "gitlab.com/gitlab-org/gitlab-pages/internal/domain" // Source represents an abstract interface of a domains configuration source. type Source interface { GetDomain(string) (*domain.Domain, error) + IsReady() bool } diff --git a/internal/source/source_mock.go b/internal/source/source_mock.go index ee24d804e..7c693eb13 100644 --- a/internal/source/source_mock.go +++ b/internal/source/source_mock.go @@ -18,6 +18,12 @@ func (m *MockSource) GetDomain(name string) (*domain.Domain, error) { return args.Get(0).(*domain.Domain), args.Error(1) } +func (m *MockSource) IsReady() bool { + args := m.Called() + + return args.Get(0).(bool) +} + // NewMockSource returns a new Source mock for testing func NewMockSource() *MockSource { return &MockSource{} -- GitLab From 1350a433466a309883b2c67ea5f40bc94cee8382 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 27 Jul 2020 11:28:06 +1000 Subject: [PATCH 3/3] Remove domains lock --- internal/source/domains.go | 3 --- internal/source/domains_test.go | 2 -- 2 files changed, 5 deletions(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index 09007c85e..ca2a8b6e2 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -3,7 +3,6 @@ package source import ( "errors" "regexp" - "sync" "time" log "github.com/sirupsen/logrus" @@ -34,7 +33,6 @@ func init() { // currently using two sources during the transition to the new GitLab domains // source. type Domains struct { - mu *sync.RWMutex gitlab Source disk *disk.Disk // legacy disk source } @@ -47,7 +45,6 @@ func NewDomains(config Config) (*Domains, error) { // https://gitlab.com/gitlab-org/gitlab/-/issues/217912 domains := &Domains{ - mu: &sync.RWMutex{}, disk: disk.New(), } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 12c3220af..2382e7566 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -2,7 +2,6 @@ package source import ( "math/rand" - "sync" "testing" "time" @@ -212,7 +211,6 @@ func newTestDomains(t *testing.T, gitlabSource *MockSource) *Domains { t.Helper() return &Domains{ - mu: &sync.RWMutex{}, disk: disk.New(), gitlab: gitlabSource, } -- GitLab