From 46160c0fd08b1628055de9fb94c009a924235d6f Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Fri, 24 Jul 2020 15:56:10 +1000 Subject: [PATCH 01/11] Check config first --- internal/source/domains.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index ca2a8b6e2..b35774be5 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -44,8 +44,12 @@ func NewDomains(config Config) (*Domains, error) { // TODO: choose domain source config via config.DomainConfigSource() // https://gitlab.com/gitlab-org/gitlab/-/issues/217912 - domains := &Domains{ - disk: disk.New(), + domains := &Domains{} + + if config.DomainConfigSource() == "disk" { + domains.disk = disk.New() + // TODO: start polling + return domains, nil } if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { -- GitLab From e5106118f14674565bedf7c3dc3733ce2739acc3 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 27 Jul 2020 11:43:02 +1000 Subject: [PATCH 02/11] Set disk source --- internal/source/domains.go | 33 +++++++++++++++++++-------------- internal/source/domains_test.go | 10 ++++++++++ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index b35774be5..81c53bd6d 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -41,29 +41,34 @@ type Domains struct { // not initialize `dm` as we later check the readiness by comparing it with a // nil value. func NewDomains(config Config) (*Domains, error) { - // TODO: choose domain source config via config.DomainConfigSource() - // https://gitlab.com/gitlab-org/gitlab/-/issues/217912 + domains := &Domains{ + disk: disk.New(), + } - domains := &Domains{} + // Creating a glClient will start polling connectivity in the background + glClient, glErr := newGitlabClient(config) - if config.DomainConfigSource() == "disk" { - domains.disk = disk.New() - // TODO: start polling + switch config.DomainConfigSource() { + // TODO: handle DomainConfigSource == "gitlab" || "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358 + case "disk": return domains, nil } - if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { - return domains, nil + if glErr == nil { + domains.gitlab = glClient + } else { + log.WithError(glErr).Warn("failed to create GitLab domains source") } - glClient, err := gitlab.New(config) - if err != nil { - return nil, err - } + return domains, nil +} - domains.gitlab = glClient +func newGitlabClient(config Config) (*gitlab.Gitlab, error) { + if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { + return nil, nil + } - return domains, nil + return gitlab.New(config) } // GetDomain retrieves a domain information from a source. We are using two diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 2382e7566..df41caa2f 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -52,6 +52,16 @@ func TestDomainSources(t *testing.T) { require.Nil(t, domains.gitlab) require.NotNil(t, domains.disk) }) + + t.Run("when source is set to disk", func(t *testing.T) { + domains, err := NewDomains(sourceConfig{ + domainSource: "disk", + }) + require.NoError(t, err) + + require.Nil(t, domains.gitlab) + require.NotNil(t, domains.disk) + }) } func TestGetDomain(t *testing.T) { -- GitLab From 023ff71b6c72fc9a3f507b9d299df9ac31bb72c7 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 27 Jul 2020 11:47:19 +1000 Subject: [PATCH 03/11] Handle gitlab client error --- internal/source/domains.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index 81c53bd6d..03abda934 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -47,6 +47,9 @@ func NewDomains(config Config) (*Domains, error) { // Creating a glClient will start polling connectivity in the background glClient, glErr := newGitlabClient(config) + if glErr != nil { + log.WithError(glErr).Warn("failed to create GitLab domains source") + } switch config.DomainConfigSource() { // TODO: handle DomainConfigSource == "gitlab" || "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358 @@ -54,12 +57,13 @@ func NewDomains(config Config) (*Domains, error) { return domains, nil } - if glErr == nil { - domains.gitlab = glClient - } else { - log.WithError(glErr).Warn("failed to create GitLab domains source") + // return glErr when no domain config source is specified + if glErr != nil { + return nil, glErr } + domains.gitlab = glClient + return domains, nil } -- GitLab From 95e3f2803e09b105a275f3760909175d4b3bbb04 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 27 Jul 2020 15:21:34 +1000 Subject: [PATCH 04/11] Set domain-config-source in acceptance test --- acceptance_test.go | 2 +- internal/source/domains.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index 2b0a7800c..acceaa8eb 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1727,7 +1727,7 @@ domains: gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t) - pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey} + pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", ""} teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{gitlabSourceConfigFile}, pagesArgs...) defer teardown() diff --git a/internal/source/domains.go b/internal/source/domains.go index 03abda934..9912f6b7e 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -47,22 +47,22 @@ func NewDomains(config Config) (*Domains, error) { // Creating a glClient will start polling connectivity in the background glClient, glErr := newGitlabClient(config) - if glErr != nil { - log.WithError(glErr).Warn("failed to create GitLab domains source") - } - switch config.DomainConfigSource() { // TODO: handle DomainConfigSource == "gitlab" || "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358 + switch config.DomainConfigSource() { case "disk": return domains, nil } // return glErr when no domain config source is specified if glErr != nil { + log.WithError(glErr).Warn("failed to create GitLab domains source") return nil, glErr } - domains.gitlab = glClient + if glClient != nil { + domains.gitlab = glClient + } return domains, nil } -- GitLab From 574317c8f7b3f56958a5604cbb39e57e49a7d32b Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 28 Jul 2020 14:32:49 +1000 Subject: [PATCH 05/11] Remove gitlabsourceconfig package --- .../gitlabsourceconfig/gitlabsourceconfig.go | 94 ------------------- 1 file changed, 94 deletions(-) delete mode 100644 internal/source/domains/gitlabsourceconfig/gitlabsourceconfig.go diff --git a/internal/source/domains/gitlabsourceconfig/gitlabsourceconfig.go b/internal/source/domains/gitlabsourceconfig/gitlabsourceconfig.go deleted file mode 100644 index ebc8b4859..000000000 --- a/internal/source/domains/gitlabsourceconfig/gitlabsourceconfig.go +++ /dev/null @@ -1,94 +0,0 @@ -package gitlabsourceconfig - -import ( - "bytes" - "io/ioutil" - "os" - "time" - - log "github.com/sirupsen/logrus" - "gopkg.in/yaml.v2" -) - -// GitlabSourceDomains holds the domains to be used with the gitlab source -type GitlabSourceDomains struct { - Enabled []string - Broken string - Rollout GitlabSourceRollout -} - -// GitlabSourceRollout holds the rollout strategy and percentage -type GitlabSourceRollout struct { - Stickiness string - Percentage int -} - -// GitlabSourceConfig holds the configuration for the gitlab source -type GitlabSourceConfig struct { - Domains GitlabSourceDomains -} - -// UpdateFromYaml updates the config -// We use new variable here (instead of using `config` directly) -// because if `content` is empty `yaml.Unmarshal` does not update -// the fields already set. -func (config *GitlabSourceConfig) UpdateFromYaml(content []byte) error { - updated := GitlabSourceConfig{} - - err := yaml.Unmarshal(content, &updated) - if err != nil { - return err - } - - *config = updated - - log.WithFields(log.Fields{ - "Enabled domains": config.Domains.Enabled, - "Broken domain": config.Domains.Broken, - "Rollout %": config.Domains.Rollout.Percentage, - "Rollout stickiness": config.Domains.Rollout.Stickiness, - }).Info("gitlab source config updated") - - return nil -} - -// WatchForGitlabSourceConfigChange polls the filesystem and updates test domains if needed. -func WatchForGitlabSourceConfigChange(config *GitlabSourceConfig, interval time.Duration) { - var lastContent []byte - - gitlabSourceConfigFile := os.Getenv("GITLAB_SOURCE_CONFIG_FILE") - if gitlabSourceConfigFile == "" { - gitlabSourceConfigFile = ".gitlab-source-config.yml" - } - - for { - content, err := readConfig(gitlabSourceConfigFile) - if err != nil { - log.WithError(err).Warn("Failed to read gitlab source config file") - - time.Sleep(interval) - continue - } - - if !bytes.Equal(lastContent, content) { - lastContent = content - - err = config.UpdateFromYaml(content) - if err != nil { - log.WithError(err).Warn("Failed to update gitlab source config") - } - } - - time.Sleep(interval) - } -} - -func readConfig(configfile string) ([]byte, error) { - content, err := ioutil.ReadFile(configfile) - - if err != nil && !os.IsNotExist(err) { - return nil, err - } - - return content, nil -} -- GitLab From 7c9cc5a4f19f1f160f49f0a8f227cfa7af8be181 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 28 Jul 2020 14:39:12 +1000 Subject: [PATCH 06/11] Set configSource to the Domains struct --- internal/source/domains.go | 86 +++++++------- internal/source/domains_test.go | 194 +++++++++++--------------------- internal/source/source_mock.go | 8 +- 3 files changed, 112 insertions(+), 176 deletions(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index 9912f6b7e..53956b6bf 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -1,40 +1,36 @@ package source import ( - "errors" + "fmt" "regexp" - "time" - - log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" - "gitlab.com/gitlab-org/gitlab-pages/internal/rollout" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/domains/gitlabsourceconfig" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab" ) var ( - gitlabSourceConfig gitlabsourceconfig.GitlabSourceConfig - // serverlessDomainRegex is a regular expression we use to check if a domain // is a serverless domain, to short circuit gitlab source rollout. It can be // removed after the rollout is done serverlessDomainRegex = regexp.MustCompile(`^[^.]+-[[:xdigit:]]{2}a1[[:xdigit:]]{10}f2[[:xdigit:]]{2}[[:xdigit:]]+-?.*`) ) -func init() { - // Start watching the config file for domains that will use the new `gitlab` source, - // to be removed once we switch completely to using it. - go gitlabsourceconfig.WatchForGitlabSourceConfigChange(&gitlabSourceConfig, 1*time.Minute) -} +type configSource int + +const ( + sourceGitlab configSource = iota + sourceDisk + sourceAuto +) // Domains struct represents a map of all domains supported by pages. It is // currently using two sources during the transition to the new GitLab domains // source. type Domains struct { - gitlab Source - disk *disk.Disk // legacy disk source + configSource configSource + gitlab Source + disk *disk.Disk // legacy disk source } // NewDomains is a factory method for domains initializing a mutex. It should @@ -42,34 +38,46 @@ type Domains struct { // nil value. func NewDomains(config Config) (*Domains, error) { domains := &Domains{ + // TODO: disable domains.disk https://gitlab.com/gitlab-org/gitlab-pages/-/issues/382 disk: disk.New(), } - // Creating a glClient will start polling connectivity in the background - glClient, glErr := newGitlabClient(config) + domains.setConfigSource(config) - // TODO: handle DomainConfigSource == "gitlab" || "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358 - switch config.DomainConfigSource() { - case "disk": - return domains, nil - } - - // return glErr when no domain config source is specified - if glErr != nil { - log.WithError(glErr).Warn("failed to create GitLab domains source") - return nil, glErr + // Creating a glClient will start polling connectivity in the background + glClient, err := newGitlabClient(config) + if err != nil && domains.configSource != sourceDisk { + return nil, err } - if glClient != nil { + // TODO: handle DomainConfigSource == "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358 + // attach gitlab by default when source is not disk (auto, gitlab) + if domains.configSource != sourceDisk { domains.gitlab = glClient } return domains, nil } +// defaults to disk +func (d *Domains) setConfigSource(config Config) { + switch config.DomainConfigSource() { + case "gitlab": + // TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/218357 + d.configSource = sourceGitlab + case "auto": + // TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/218358 + d.configSource = sourceAuto + case "disk": + fallthrough + default: + d.configSource = sourceDisk + } +} + func newGitlabClient(config Config) (*gitlab.Gitlab, error) { if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { - return nil, nil + return nil, fmt.Errorf("missing -internal-gitlab-server and/or -api-secret-key") } return gitlab.New(config) @@ -80,10 +88,6 @@ func newGitlabClient(config Config) (*gitlab.Gitlab, error) { // for some subset of domains, to test / PoC the new GitLab Domains Source that // we plan to use to replace the disk source. func (d *Domains) GetDomain(name string) (*domain.Domain, error) { - if name == gitlabSourceConfig.Domains.Broken { - return nil, errors.New("broken test domain used") - } - return d.source(name).GetDomain(name) } @@ -101,7 +105,7 @@ func (d *Domains) IsReady() bool { } func (d *Domains) source(domain string) Source { - if d.gitlab == nil || !d.gitlab.IsReady() { + if d.gitlab == nil { return d.disk } @@ -112,21 +116,11 @@ func (d *Domains) source(domain string) Source { return d.gitlab } - for _, name := range gitlabSourceConfig.Domains.Enabled { - if domain == name { - return d.gitlab - } - } - - r := gitlabSourceConfig.Domains.Rollout - - enabled, err := rollout.Rollout(domain, r.Percentage, r.Stickiness) - if err != nil { - log.WithError(err).Error("Rollout error") + if d.configSource == sourceDisk { return d.disk } - if enabled { + if d.gitlab.IsReady() { return d.gitlab } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index df41caa2f..8431810e6 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -1,7 +1,6 @@ package source import ( - "math/rand" "testing" "time" @@ -37,39 +36,59 @@ func (c sourceConfig) DomainConfigSource() string { } func TestDomainSources(t *testing.T) { - t.Run("when GitLab API URL has been provided", func(t *testing.T) { - domains, err := NewDomains(sourceConfig{api: "https://gitlab.com", secret: "abc"}) - require.NoError(t, err) - - require.NotNil(t, domains.gitlab) - require.NotNil(t, domains.disk) - }) - - t.Run("when GitLab API has not been provided", func(t *testing.T) { - domains, err := NewDomains(sourceConfig{}) - require.NoError(t, err) + tests := []struct { + name string + sourceConfig sourceConfig + wantErr bool + expectGitlabNil bool + }{ + { + name: "no_source_config", + sourceConfig: sourceConfig{}, + wantErr: false, + expectGitlabNil: true, + }, + { + name: "disk_source", + sourceConfig: sourceConfig{domainSource: "disk"}, + wantErr: false, + expectGitlabNil: true, + }, + { + name: "gitlab_source_success", + sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "gitlab"}, + wantErr: false, + }, + { + name: "gitlab_source_no_url", + sourceConfig: sourceConfig{api: "", secret: "abc", domainSource: "gitlab"}, + wantErr: true, + }, + { + name: "gitlab_source_no_secret", + sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "", domainSource: "gitlab"}, + wantErr: true, + }, + } - require.Nil(t, domains.gitlab) - require.NotNil(t, domains.disk) - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + domains, err := NewDomains(tt.sourceConfig) + if tt.wantErr { + require.EqualError(t, err, "missing -internal-gitlab-server and/or -api-secret-key") + return + } + require.NoError(t, err) - t.Run("when source is set to disk", func(t *testing.T) { - domains, err := NewDomains(sourceConfig{ - domainSource: "disk", + require.Equal(t, tt.expectGitlabNil, domains.gitlab == nil) + require.NotNil(t, domains.disk) }) - require.NoError(t, err) - - require.Nil(t, domains.gitlab) - require.NotNil(t, domains.disk) - }) + } } func TestGetDomain(t *testing.T) { - gitlabSourceConfig.Domains.Enabled = []string{"new-source-test.gitlab.io"} - gitlabSourceConfig.Domains.Broken = "pages-broken-poc.gitlab.io" - - t.Run("when requesting a test domain", func(t *testing.T) { - testDomain := gitlabSourceConfig.Domains.Enabled[0] + t.Run("when requesting an existing domain", func(t *testing.T) { + testDomain := "new-source-test.gitlab.io" newSource := NewMockSource() newSource.On("GetDomain", testDomain). @@ -78,47 +97,29 @@ func TestGetDomain(t *testing.T) { newSource.On("IsReady").Return(true).Once() defer newSource.AssertExpectations(t) - domains := newTestDomains(t, newSource) + domains := newTestDomains(t, newSource, sourceGitlab) - domains.GetDomain(testDomain) + domain, err := domains.GetDomain(testDomain) + require.NoError(t, err) + require.NotNil(t, domain) }) - t.Run("when requesting a non-test domain", func(t *testing.T) { + t.Run("when requesting a domain that doesn't exist", func(t *testing.T) { newSource := NewMockSource() + newSource.On("GetDomain", "does-not-exist.test.io"). + Return(nil, nil). + Once() newSource.On("IsReady").Return(true).Once() defer newSource.AssertExpectations(t) - domains := newTestDomains(t, newSource) - - domain, err := domains.GetDomain("domain.test.io") + domains := newTestDomains(t, newSource, sourceGitlab) + domain, err := domains.GetDomain("does-not-exist.test.io") require.NoError(t, err) require.Nil(t, domain) }) - t.Run("when requesting a broken test domain", func(t *testing.T) { - newSource := NewMockSource() - defer newSource.AssertExpectations(t) - - domains := newTestDomains(t, newSource) - - domain, err := domains.GetDomain("pages-broken-poc.gitlab.io") - - require.Nil(t, domain) - require.EqualError(t, err, "broken test domain used") - }) - - t.Run("when requesting a test domain in case of the source not being fully configured", func(t *testing.T) { - domains, err := NewDomains(sourceConfig{}) - require.NoError(t, err) - - domain, err := domains.GetDomain("new-source-test.gitlab.io") - - require.Nil(t, domain) - require.NoError(t, err) - }) - t.Run("when requesting a serverless domain", func(t *testing.T) { testDomain := "func-aba1aabbccddeef2abaabbcc.serverless.gitlab.io" @@ -126,13 +127,14 @@ 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) + domains := newTestDomains(t, newSource, sourceGitlab) - domains.GetDomain(testDomain) + domain, err := domains.GetDomain(testDomain) + require.NoError(t, err) + require.NotNil(t, domain) }) } @@ -150,78 +152,12 @@ func TestIsServerlessDomain(t *testing.T) { }) } -func TestGetDomainWithIncrementalrolloutOfGitLabSource(t *testing.T) { - // This will produce the following pseudo-random sequence: 5, 87, 68 - rand.Seed(42) - - // Generates FNV hash 4091421005, 4091421005 % 100 = 5 - domain05 := "test-domain-a.com" - // Generates FNV 2643293380, 2643293380 % 100 = 80 - domain80 := "test-domain-b.com" - - gitlabSourceConfig.Domains.Rollout.Percentage = 80 - - type testDomain struct { - name string - source string - } - - tests := map[string]struct { - stickiness string - domains []testDomain - }{ - // domain05 should always use gitlab source, - // domain80 should use disk source - "default stickiness": { - stickiness: "", - domains: []testDomain{ - {name: domain05, source: "gitlab"}, - {name: domain80, source: "disk"}, - {name: domain05, source: "gitlab"}, - }, - }, - // Given that randSeed(42) will produce the following pseudo-random sequence: - // {5, 87, 68} the first and third call for domain05 should use gitlab source, - // while the second one should use disk source - "no stickiness": { - stickiness: "random", - domains: []testDomain{ - {name: domain05, source: "gitlab"}, - {name: domain05, source: "disk"}, - {name: domain05, source: "gitlab"}, - }}, - } - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - gitlabSource := NewMockSource() - for _, d := range tc.domains { - if d.source == "gitlab" { - gitlabSource.On("GetDomain", d.name). - Return(&domain.Domain{Name: d.name}, nil). - Once() - } - } - gitlabSource.On("IsReady").Return(true) - defer gitlabSource.AssertExpectations(t) - - domains := newTestDomains(t, gitlabSource) - - gitlabSourceConfig.Domains.Rollout.Stickiness = tc.stickiness - - for _, domain := range tc.domains { - _, err := domains.GetDomain(domain.name) - require.NoError(t, err) - } - }) - } -} - -func newTestDomains(t *testing.T, gitlabSource *MockSource) *Domains { +func newTestDomains(t *testing.T, gitlabSource *MockSource, config configSource) *Domains { t.Helper() return &Domains{ - disk: disk.New(), - gitlab: gitlabSource, + configSource: config, + gitlab: gitlabSource, + disk: disk.New(), } } diff --git a/internal/source/source_mock.go b/internal/source/source_mock.go index 7c693eb13..d7cd15333 100644 --- a/internal/source/source_mock.go +++ b/internal/source/source_mock.go @@ -14,8 +14,14 @@ type MockSource struct { // GetDomain is a mocked function func (m *MockSource) GetDomain(name string) (*domain.Domain, error) { args := m.Called(name) + err := args.Error(1) - return args.Get(0).(*domain.Domain), args.Error(1) + d, ok := args.Get(0).(*domain.Domain) + if !ok { + return nil, err + } + + return d, err } func (m *MockSource) IsReady() bool { -- GitLab From ee43b1ab04e8aa2e17546035ea2901096dc0b823 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 28 Jul 2020 14:40:19 +1000 Subject: [PATCH 07/11] Update acceptance tests without gitlabsourceconfig --- acceptance_test.go | 27 ++++----------------------- helpers_test.go | 34 +++++++++------------------------- main.go | 1 + 3 files changed, 14 insertions(+), 48 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index acceaa8eb..abcf35923 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1714,22 +1714,11 @@ func TestGitlabDomainsSource(t *testing.T) { source := NewGitlabDomainsSourceStub(t) defer source.Close() - gitlabSourceConfig := ` -domains: - enabled: - - new-source-test.gitlab.io - broken: pages-broken-poc.gitlab.io -` - gitlabSourceConfigFile, cleanupGitlabSourceConfigFile := CreateGitlabSourceConfigFixtureFile(t, gitlabSourceConfig) - defer cleanupGitlabSourceConfigFile() - - gitlabSourceConfigFile = "GITLAB_SOURCE_CONFIG_FILE=" + gitlabSourceConfigFile - gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t) - pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", ""} + pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", "gitlab"} - teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{gitlabSourceConfigFile}, pagesArgs...) + teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{}, pagesArgs...) defer teardown() t.Run("when a domain exists", func(t *testing.T) { @@ -1737,7 +1726,8 @@ domains: require.NoError(t, err) defer response.Body.Close() - body, _ := ioutil.ReadAll(response.Body) + body, err := ioutil.ReadAll(response.Body) + require.NoError(t, err) require.Equal(t, http.StatusOK, response.StatusCode) require.Equal(t, "New Pages GitLab Source TEST OK\n", string(body)) @@ -1750,13 +1740,4 @@ domains: require.Equal(t, http.StatusNotFound, response.StatusCode) }) - - t.Run("broken domain is requested", func(t *testing.T) { - response, err := GetPageFromListener(t, httpListener, "pages-broken-poc.gitlab.io", "index.html") - require.NoError(t, err) - - defer response.Body.Close() - - require.Equal(t, http.StatusBadGateway, response.StatusCode) - }) } diff --git a/helpers_test.go b/helpers_test.go index 32bea87c7..a55399f4c 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -13,7 +13,6 @@ import ( "os" "os/exec" "path" - "path/filepath" "strings" "testing" "time" @@ -88,27 +87,6 @@ func CreateGitLabAPISecretKeyFixtureFile(t *testing.T) (filepath string) { return secretfile.Name() } -func CreateGitlabSourceConfigFixtureFile(t *testing.T, domains string) (filename string, cleanup func()) { - configfile, err := ioutil.TempFile("shared/pages", "gitlab-source-config-*") - require.NoError(t, err) - configfile.Close() - - cleanup = func() { - os.RemoveAll(configfile.Name()) - } - - require.NoError(t, ioutil.WriteFile(configfile.Name(), []byte(domains), 0644)) - - filename, err = filepath.Abs(configfile.Name()) - require.NoError(t, err) - - if os.Getenv("TEST_DAEMONIZE") != "" { - filename = filepath.Base(filename) - } - - return filename, cleanup -} - // ListenSpec is used to point at a gitlab-pages http server, preserving the // type of port it is (http, https, proxy) type ListenSpec struct { @@ -443,7 +421,12 @@ func waitForRoundtrips(t *testing.T, listeners []ListenSpec, timeout time.Durati } func NewGitlabDomainsSourceStub(t *testing.T) *httptest.Server { - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/internal/pages/status", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + + handler := func(w http.ResponseWriter, r *http.Request) { domain := r.URL.Query().Get("host") path := "shared/lookups/" + domain + ".json" @@ -462,9 +445,10 @@ func NewGitlabDomainsSourceStub(t *testing.T) *httptest.Server { require.NoError(t, err) t.Logf("GitLab domain %s source stub served lookup", domain) - }) + } + mux.HandleFunc("/api/v4/internal/pages", handler) - return httptest.NewServer(handler) + return httptest.NewServer(mux) } func newConfigFile(configs ...string) (string, error) { diff --git a/main.go b/main.go index c5c2047d2..1d3979225 100644 --- a/main.go +++ b/main.go @@ -289,6 +289,7 @@ func loadConfig() appConfig { "gitlab-server": config.GitLabServer, "internal-gitlab-server": config.InternalGitLabServer, "api-secret-key": *gitLabAPISecretKey, + "domain-config-source": config.DomainConfigurationSource, "auth-redirect-uri": config.RedirectURI, }).Debug("Start daemon with configuration") -- GitLab From 6678be1e1c01eeb43a32bdcca4912291909b41ce Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 28 Jul 2020 14:54:48 +1000 Subject: [PATCH 08/11] Remove yaml direct dependency --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 049b9c575..c46a657e0 100644 --- a/go.mod +++ b/go.mod @@ -31,6 +31,6 @@ require ( golang.org/x/sys v0.0.0-20200420163511-1957bb5e6d1f golang.org/x/tools v0.0.0-20200502202811-ed308ab3e770 gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect - gopkg.in/yaml.v2 v2.2.8 + gopkg.in/yaml.v2 v2.2.8 // indirect honnef.co/go/tools v0.0.1-2020.1.3 // indirect ) -- GitLab From c6e3c949c55cacf4a9762d029dde515f80e8aa89 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 4 Aug 2020 16:11:41 +1000 Subject: [PATCH 09/11] Make initialization of gitlab client more explicit Add more test cases for domains. Support sourceAuto and use IsReady for gitlab source. --- app_test.go | 3 +- internal/source/domains.go | 57 +++++++++++++++++++------------ internal/source/domains_test.go | 59 ++++++++++++++++++++++++--------- 3 files changed, 81 insertions(+), 38 deletions(-) diff --git a/app_test.go b/app_test.go index 3c485804a..f35e90c54 100644 --- a/app_test.go +++ b/app_test.go @@ -85,7 +85,8 @@ func TestHealthCheckMiddleware(t *testing.T) { app := theApp{ appConfig: appConfig{ - StatusPath: "/-/healthcheck", + StatusPath: "/-/healthcheck", + DomainConfigurationSource: "auto", }, } diff --git a/internal/source/domains.go b/internal/source/domains.go index 53956b6bf..e4a582ea9 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -4,6 +4,8 @@ import ( "fmt" "regexp" + "gitlab.com/gitlab-org/labkit/log" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab" @@ -42,45 +44,56 @@ func NewDomains(config Config) (*Domains, error) { disk: disk.New(), } - domains.setConfigSource(config) - - // Creating a glClient will start polling connectivity in the background - glClient, err := newGitlabClient(config) - if err != nil && domains.configSource != sourceDisk { + if err := domains.setConfigSource(config); err != nil { return nil, err } - // TODO: handle DomainConfigSource == "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358 - // attach gitlab by default when source is not disk (auto, gitlab) - if domains.configSource != sourceDisk { - domains.gitlab = glClient - } - return domains, nil } -// defaults to disk -func (d *Domains) setConfigSource(config Config) { +// setConfigSource and initialize gitlab source +// returns error if -domain-config-source is not valid +// returns error if -domain-config-source=gitlab and init fails +func (d *Domains) setConfigSource(config Config) error { + // TODO: Handle domain-config-source=auto https://gitlab.com/gitlab-org/gitlab/-/issues/218358 + // attach gitlab by default when source is not disk (auto, gitlab) switch config.DomainConfigSource() { case "gitlab": // TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/218357 d.configSource = sourceGitlab + return d.setGitLabClient(config) case "auto": - // TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/218358 + // TODO: handle DomainConfigSource == "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358 d.configSource = sourceAuto + return d.setGitLabClient(config) case "disk": - fallthrough - default: d.configSource = sourceDisk + default: + return fmt.Errorf("invalid option for -domain-config-source: %q", config.DomainConfigSource()) } + + return nil } -func newGitlabClient(config Config) (*gitlab.Gitlab, error) { - if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 { - return nil, fmt.Errorf("missing -internal-gitlab-server and/or -api-secret-key") +// setGitLabClient when domain-config-source is `gitlab` or `auto`, only return error for `gitlab` source +func (d *Domains) setGitLabClient(config Config) error { + // We want to notify users about any API issues + // Creating a glClient will start polling connectivity in the background + // and spam errors in log + glClient, err := gitlab.New(config) + if err != nil { + if d.configSource == sourceGitlab { + return err + } + + log.WithError(err).Warn("failed to initialize GitLab client for `-domain-config-source=auto`") + + return nil } - return gitlab.New(config) + d.gitlab = glClient + + return nil } // GetDomain retrieves a domain information from a source. We are using two @@ -120,7 +133,9 @@ func (d *Domains) source(domain string) Source { return d.disk } - if d.gitlab.IsReady() { + // TODO: handle sourceAuto https://gitlab.com/gitlab-org/gitlab/-/issues/218358 + // check IsReady for sourceAuto for now + if d.configSource == sourceGitlab || d.gitlab.IsReady() { return d.gitlab } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 8431810e6..173fc1d3d 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -35,47 +35,59 @@ func (c sourceConfig) DomainConfigSource() string { return c.domainSource } -func TestDomainSources(t *testing.T) { +func TestNewDomains(t *testing.T) { tests := []struct { name string sourceConfig sourceConfig - wantErr bool + expectedErr string expectGitlabNil bool }{ { - name: "no_source_config", - sourceConfig: sourceConfig{}, - wantErr: false, - expectGitlabNil: true, + name: "no_source_config", + sourceConfig: sourceConfig{}, + expectedErr: "invalid option for -domain-config-source: \"\"", + }, + { + name: "invalid_source_config", + sourceConfig: sourceConfig{domainSource: "invalid"}, + expectedErr: "invalid option for -domain-config-source: \"invalid\"", }, { name: "disk_source", sourceConfig: sourceConfig{domainSource: "disk"}, - wantErr: false, expectGitlabNil: true, }, + { + name: "auto_without_api_config", + sourceConfig: sourceConfig{domainSource: "auto"}, + expectGitlabNil: true, + }, + { + name: "auto_with_api_config", + sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "auto"}, + expectGitlabNil: false, + }, { name: "gitlab_source_success", sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "gitlab"}, - wantErr: false, }, { name: "gitlab_source_no_url", sourceConfig: sourceConfig{api: "", secret: "abc", domainSource: "gitlab"}, - wantErr: true, + expectedErr: "GitLab API URL or API secret has not been provided", }, { name: "gitlab_source_no_secret", sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "", domainSource: "gitlab"}, - wantErr: true, + expectedErr: "GitLab API URL or API secret has not been provided", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { domains, err := NewDomains(tt.sourceConfig) - if tt.wantErr { - require.EqualError(t, err, "missing -internal-gitlab-server and/or -api-secret-key") + if tt.expectedErr != "" { + require.EqualError(t, err, tt.expectedErr) return } require.NoError(t, err) @@ -87,14 +99,13 @@ func TestDomainSources(t *testing.T) { } func TestGetDomain(t *testing.T) { - t.Run("when requesting an existing domain", func(t *testing.T) { + t.Run("when requesting an existing domain for gitlab source", func(t *testing.T) { testDomain := "new-source-test.gitlab.io" newSource := NewMockSource() 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, sourceGitlab) @@ -104,12 +115,28 @@ func TestGetDomain(t *testing.T) { require.NotNil(t, domain) }) - t.Run("when requesting a domain that doesn't exist", func(t *testing.T) { + t.Run("when requesting an existing domain for auto source", func(t *testing.T) { + testDomain := "new-source-test.gitlab.io" + + newSource := NewMockSource() + 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, sourceAuto) + + domain, err := domains.GetDomain(testDomain) + require.NoError(t, err) + require.NotNil(t, domain) + }) + + t.Run("when requesting a domain that doesn't exist for gitlab source", func(t *testing.T) { newSource := NewMockSource() newSource.On("GetDomain", "does-not-exist.test.io"). Return(nil, nil). Once() - newSource.On("IsReady").Return(true).Once() defer newSource.AssertExpectations(t) -- GitLab From 31de8f5de768347daddaffc199cbf806fc763264 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 4 Aug 2020 16:26:50 +1000 Subject: [PATCH 10/11] Increase client_test timeout --- internal/source/gitlab/client/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go index ab90b4749..57c479d7e 100644 --- a/internal/source/gitlab/client/client_test.go +++ b/internal/source/gitlab/client/client_test.go @@ -283,7 +283,7 @@ func TestClientStatus(t *testing.T) { } func TestClientStatusClientTimeout(t *testing.T) { - timeout := 3 * time.Millisecond + timeout := 20 * time.Millisecond mux := http.NewServeMux() mux.HandleFunc("/api/v4/internal/pages/status", func(w http.ResponseWriter, r *http.Request) { -- GitLab From 654eee26e97a0f0b716bbdec65a85d8576f8b162 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 4 Aug 2020 16:39:27 +1000 Subject: [PATCH 11/11] Remove rollout package --- internal/rollout/rollout.go | 53 ------------------------------------- 1 file changed, 53 deletions(-) delete mode 100644 internal/rollout/rollout.go diff --git a/internal/rollout/rollout.go b/internal/rollout/rollout.go deleted file mode 100644 index aaf252891..000000000 --- a/internal/rollout/rollout.go +++ /dev/null @@ -1,53 +0,0 @@ -package rollout - -import ( - "errors" - "hash/fnv" - "math/rand" -) - -// Rollout returns true and no error when during this run something should -// happen for given actor according to the stickiness and likelihood passed -// as a percentage value to this function. It returns false rollout and an -// error if the percentage value is negative or higher than 100. -func Rollout(actor string, percentage int, stickiness string) (bool, error) { - if percentage < 0 || percentage > 100 { - return false, errors.New("Rollout value should be between 0 and 100 inclusive") - } - - if percentage == 0 { - return false, nil - } - - if percentage == 100 { - return true, nil - } - - switch stickiness { - case "random": - return random(percentage), nil - default: - return forActor(actor, percentage), nil - } -} - -// random guarantees no stickiness. For every call it will yield a random -// true/false based on the provided rollout percentage. -func random(percentage int) bool { - return rand.Intn(100) < percentage -} - -// forActor provides "stickines", i.e. guarantees that the same actor -// gets the same result every time. It also assures that an actor which is -// among the first 10% will also be among the first 20%. -func forActor(actor string, percentage int) bool { - h := fnv.New32a() - h.Write([]byte(actor)) - sum32 := h.Sum32() - - if sum32 == 0 { - return false - } - - return (sum32 % uint32(100)) < uint32(percentage) -} -- GitLab