From babdf5ea9aae2bff2267c5047b2714e00649fd11 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Oct 2019 10:06:17 +0200 Subject: [PATCH 01/39] Add abstract GitLab domains source interface --- internal/auth/auth.go | 1 - internal/source/disk/disk.go | 64 ++++++++++++++++++++++++++++++++ internal/source/disk/map.go | 4 +- internal/source/disk/map_test.go | 2 +- internal/source/domains.go | 49 +----------------------- internal/source/source.go | 10 +++++ 6 files changed, 79 insertions(+), 51 deletions(-) create mode 100644 internal/source/disk/disk.go create mode 100644 internal/source/source.go diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 2e8473b4f..320dcb114 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -108,7 +108,6 @@ func (a *Auth) checkSession(w http.ResponseWriter, r *http.Request) (*sessions.S // TryAuthenticate tries to authenticate user and fetch access token if request is a callback to auth func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, domains *source.Domains) bool { - if a == nil { return false } diff --git a/internal/source/disk/disk.go b/internal/source/disk/disk.go new file mode 100644 index 000000000..8faa989c8 --- /dev/null +++ b/internal/source/disk/disk.go @@ -0,0 +1,64 @@ +package disk + +import ( + "strings" + "sync" + "time" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" +) + +// Domains struct represents a map of all domains supported by pages. It is +// currently reading them from disk. +type Disk struct { + dm Map + lock *sync.RWMutex +} + +// NewDomains is a factory method for domains initializing a mutex. It should +// not initialize `dm` as we later check the readiness by comparing it with a +// nil value. +func New() *Disk { + return &Disk{ + lock: &sync.RWMutex{}, + } +} + +// GetDomain returns a domain from the domains map +func (d *Disk) GetDomain(host string) *domain.Domain { + host = strings.ToLower(host) + d.lock.RLock() + defer d.lock.RUnlock() + domain, _ := d.dm[host] + + return domain +} + +// HasDomain checks for presence of a domain in the domains map +func (d *Disk) HasDomain(host string) bool { + d.lock.RLock() + defer d.lock.RUnlock() + + host = strings.ToLower(host) + _, isPresent := d.dm[host] + + return isPresent +} + +// Ready checks if the domains source is ready for work +func (d *Disk) Ready() bool { + return d.dm != nil +} + +// Watch starts the domain source, in this case it is reading domains from +// groups on disk concurrently +func (d *Disk) Watch(rootDomain string) { + go watch(rootDomain, d.updateDomains, time.Second) +} + +func (d *Disk) updateDomains(dm Map) { + d.lock.Lock() + defer d.lock.Unlock() + + d.dm = dm +} diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index b58433019..88f70c3f9 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -226,8 +226,8 @@ const ( updateFile = ".update" ) -// Watch polls the filesystem and kicks off a new domain directory scan when needed. -func Watch(rootDomain string, updater domainsUpdater, interval time.Duration) { +// watch polls the filesystem and kicks off a new domain directory scan when needed. +func watch(rootDomain string, updater domainsUpdater, interval time.Duration) { lastUpdate := []byte("no-update") for { diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go index c15f29c61..09e9f630e 100644 --- a/internal/source/disk/map_test.go +++ b/internal/source/disk/map_test.go @@ -156,7 +156,7 @@ func TestWatch(t *testing.T) { require.NoError(t, os.RemoveAll(updateFile)) update := make(chan Map) - go Watch("gitlab.io", func(dm Map) { + go watch("gitlab.io", func(dm Map) { update <- dm }, time.Microsecond*50) diff --git a/internal/source/domains.go b/internal/source/domains.go index cd2d89c55..eadf25c49 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -1,19 +1,13 @@ package source import ( - "strings" - "sync" - "time" - - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" ) // Domains struct represents a map of all domains supported by pages. It is // currently reading them from disk. type Domains struct { - dm disk.Map - lock *sync.RWMutex + Source } // NewDomains is a factory method for domains initializing a mutex. It should @@ -21,45 +15,6 @@ type Domains struct { // nil value. func NewDomains() *Domains { return &Domains{ - lock: &sync.RWMutex{}, + Source: disk.New(), } } - -// GetDomain returns a domain from the domains map -func (d *Domains) GetDomain(host string) *domain.Domain { - host = strings.ToLower(host) - d.lock.RLock() - defer d.lock.RUnlock() - domain, _ := d.dm[host] - - return domain -} - -// HasDomain checks for presence of a domain in the domains map -func (d *Domains) HasDomain(host string) bool { - d.lock.RLock() - defer d.lock.RUnlock() - - host = strings.ToLower(host) - _, isPresent := d.dm[host] - - return isPresent -} - -// Ready checks if the domains source is ready for work -func (d *Domains) Ready() bool { - return d.dm != nil -} - -// Watch starts the domain source, in this case it is reading domains from -// groups on disk concurrently -func (d *Domains) Watch(rootDomain string) { - go disk.Watch(rootDomain, d.updateDomains, time.Second) -} - -func (d *Domains) updateDomains(dm disk.Map) { - d.lock.Lock() - defer d.lock.Unlock() - - d.dm = dm -} diff --git a/internal/source/source.go b/internal/source/source.go new file mode 100644 index 000000000..18577493d --- /dev/null +++ b/internal/source/source.go @@ -0,0 +1,10 @@ +package source + +import "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + +type Source interface { + GetDomain(string) *domain.Domain + HasDomain(string) bool + Watch(rootDomain string) + Ready() bool +} -- GitLab From 23c07620ed3366180819fe56c4d2ca23e5fd3e2c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Oct 2019 10:15:23 +0200 Subject: [PATCH 02/39] Make it possible to switch source for some domains --- internal/source/domains.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/source/domains.go b/internal/source/domains.go index eadf25c49..df3758838 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -1,6 +1,7 @@ package source import ( + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" ) @@ -18,3 +19,10 @@ func NewDomains() *Domains { Source: disk.New(), } } + +// GetDomain overridden here allows us to switch behavior and the domains +// source for some subset of domains, to test / PoC the new GitLab Domains +// Source that we plan to introduce +func (d *Domains) GetDomain(name string) *domain.Domain { + return d.Source.GetDomain(name) +} -- GitLab From de5e5bd27dae2a4d3c95beea5f45800df78fb8f3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Oct 2019 10:15:35 +0200 Subject: [PATCH 03/39] Fix comments / annotations in the disk source package --- internal/source/disk/disk.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/source/disk/disk.go b/internal/source/disk/disk.go index 8faa989c8..20524ef13 100644 --- a/internal/source/disk/disk.go +++ b/internal/source/disk/disk.go @@ -8,16 +8,16 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/domain" ) -// Domains struct represents a map of all domains supported by pages. It is -// currently reading them from disk. +// Disk struct represents a map of all domains supported by pages that are +// stored on a disk with corresponding `config.json`. type Disk struct { dm Map lock *sync.RWMutex } -// NewDomains is a factory method for domains initializing a mutex. It should -// not initialize `dm` as we later check the readiness by comparing it with a -// nil value. +// New is a factory method for the Disk source. It is initializing a mutex. It +// should not initialize `dm` as we later check the readiness by comparing it +// with a nil value. func New() *Disk { return &Disk{ lock: &sync.RWMutex{}, -- GitLab From c274655af61211729aab59e820bdfc935a0abdf1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 11 Oct 2019 10:36:05 +0200 Subject: [PATCH 04/39] Add scaffold of gitlab domains config source --- go.mod | 6 ++++-- go.sum | 8 ++++---- internal/source/gitlab/client.go | 7 +++++++ internal/source/gitlab/gitlab.go | 30 ++++++++++++++++++++++++++++++ internal/source/gitlab/lookup.go | 19 +++++++++++++++++++ internal/source/source.go | 1 + 6 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 internal/source/gitlab/client.go create mode 100644 internal/source/gitlab/gitlab.go create mode 100644 internal/source/gitlab/lookup.go diff --git a/go.mod b/go.mod index 37325f2b2..5866e7fa6 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,8 @@ require ( github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 github.com/karrick/godirwalk v1.10.12 github.com/kr/pretty v0.1.0 // indirect + github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect + github.com/modern-go/reflect2 v1.0.1 // indirect github.com/namsral/flag v1.7.4-pre github.com/prometheus/client_golang v1.1.0 github.com/rs/cors v1.7.0 @@ -22,9 +24,9 @@ require ( gitlab.com/gitlab-org/labkit v0.0.0-20190902063225-3253d7975ca7 gitlab.com/lupine/go-mimedb v0.0.0-20180307000149-e8af1d659877 golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7 - golang.org/x/lint v0.0.0-20190909230951-414d861bb4ac + golang.org/x/lint v0.0.0-20190930215403-16217165b5de golang.org/x/net v0.0.0-20190909003024-a7b16738d86b golang.org/x/sys v0.0.0-20190910064555-bbd175535a8b - golang.org/x/tools v0.0.0-20190917032747-2dc213d980bc + golang.org/x/tools v0.0.0-20191010201905-e5ffc44a6fee gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 // indirect ) diff --git a/go.sum b/go.sum index 01a25859b..30ffb3792 100644 --- a/go.sum +++ b/go.sum @@ -139,8 +139,8 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7 h1:0hQKqeLdqlt5iIwVOBErRisrHJAN57yOiPRQItI20fU= golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/lint v0.0.0-20180702182130-06c8688daad7/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= -golang.org/x/lint v0.0.0-20190909230951-414d861bb4ac h1:8R1esu+8QioDxo4E4mX6bFztO+dMTM49DNAaWfO5OeY= -golang.org/x/lint v0.0.0-20190909230951-414d861bb4ac/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= +golang.org/x/lint v0.0.0-20190930215403-16217165b5de h1:5hukYrvBGR8/eNkX5mdUezrA6JiaEZDtJb9Ei+1LlBs= +golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20181114220301-adae6a3d119a/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -175,8 +175,8 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/tools v0.0.0-20180828015842-6cd1fcedba52/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= golang.org/x/tools v0.0.0-20190425150028-36563e24a262/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q= -golang.org/x/tools v0.0.0-20190917032747-2dc213d980bc h1:AzQrNvr65FlhSjBpg0eVCY43QLsuOqtzWGtjcBqT6J8= -golang.org/x/tools v0.0.0-20190917032747-2dc213d980bc/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20191010201905-e5ffc44a6fee h1:Cgj5oVkw7Gktu56MAiU0r1u0jyuT6jmtOzcAJwLj89c= +golang.org/x/tools v0.0.0-20191010201905-e5ffc44a6fee/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/appengine v1.1.0 h1:igQkv0AAhEIvTEpD5LIpAfav2eeVO9HBTjvKHVJPRSs= google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM= diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client.go new file mode 100644 index 000000000..73abfaa79 --- /dev/null +++ b/internal/source/gitlab/client.go @@ -0,0 +1,7 @@ +package gitlab + +// client is an internal HTTP client used for communication with GitLab +// instance +type client interface { + Resolve(domain string) *Lookup +} diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go new file mode 100644 index 000000000..5eccff174 --- /dev/null +++ b/internal/source/gitlab/gitlab.go @@ -0,0 +1,30 @@ +package gitlab + +import "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + +// Gitlab source represent a new domains configuration source. We fetch all the +// information about domains from GitLab instance. +type Gitlab struct { + client + lookups []Lookup +} + +// GetDomain return a representation of a domain that we have fetched from +// GitLab +func (g *Gitlab) GetDomain(name string) *domain.Domain { + return nil +} + +// HasDomain checks if a domain is known to GitLab +func (g *Gitlab) HasDomain(name string) bool { + return false +} + +// Watch starts Gitlab domains source +func (g *Gitlab) Watch(rootDomain string) { +} + +// Ready checks if Gitlab domains source can be used +func (g *Gitlab) Ready() bool { + return false +} diff --git a/internal/source/gitlab/lookup.go b/internal/source/gitlab/lookup.go new file mode 100644 index 000000000..6e2e0c7c0 --- /dev/null +++ b/internal/source/gitlab/lookup.go @@ -0,0 +1,19 @@ +package gitlab + +// Lookup defines a response that GitLab can send, which we can unmarshall +type Lookup struct { + Domain string + CertificateCert string + CertificateKey string + Serving string + Prefix string + LookupPaths []struct { + ProjectID int + HTTPSOnly bool + AccessControl bool + Source struct { + Type string + Path string + } + } +} diff --git a/internal/source/source.go b/internal/source/source.go index 18577493d..ffde0c900 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -2,6 +2,7 @@ package source 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 HasDomain(string) bool -- GitLab From a446e916228d47cd512baf45849380268562e2bc Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 30 Oct 2019 11:55:35 +0100 Subject: [PATCH 05/39] Add scaffold of the gitlab client source cache --- go.mod | 3 +-- go.sum | 2 ++ internal/source/gitlab/cache.go | 33 ++++++++++++++++++++++++++++++++ internal/source/gitlab/client.go | 2 +- internal/source/gitlab/gitlab.go | 23 ++++++++++++++++------ internal/source/gitlab/lookup.go | 3 +-- 6 files changed, 55 insertions(+), 11 deletions(-) create mode 100644 internal/source/gitlab/cache.go diff --git a/go.mod b/go.mod index 5866e7fa6..59bd759a6 100644 --- a/go.mod +++ b/go.mod @@ -13,9 +13,8 @@ require ( github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 github.com/karrick/godirwalk v1.10.12 github.com/kr/pretty v0.1.0 // indirect - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect - github.com/modern-go/reflect2 v1.0.1 // indirect github.com/namsral/flag v1.7.4-pre + github.com/patrickmn/go-cache v2.1.0+incompatible github.com/prometheus/client_golang v1.1.0 github.com/rs/cors v1.7.0 github.com/sirupsen/logrus v1.4.2 diff --git a/go.sum b/go.sum index 30ffb3792..d08aed585 100644 --- a/go.sum +++ b/go.sum @@ -83,6 +83,8 @@ github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+W github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/gomega v1.4.3/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= github.com/opentracing/opentracing-go v1.0.2/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o= +github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= +github.com/patrickmn/go-cache v2.1.0+incompatible/go.mod h1:3Qf8kWWT7OJRJbdiICTKqZju1ZixQ/KpMGzzAfe6+WQ= github.com/philhofer/fwd v1.0.0/go.mod h1:gk3iGcWd9+svBvR0sR+KPcfE+RNWozjowpeBVG3ZVNU= github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= diff --git a/internal/source/gitlab/cache.go b/internal/source/gitlab/cache.go new file mode 100644 index 000000000..19efcc1e8 --- /dev/null +++ b/internal/source/gitlab/cache.go @@ -0,0 +1,33 @@ +package gitlab + +import ( + "time" + + cache "github.com/patrickmn/go-cache" +) + +type Cache struct { + shortCache *cache.Cache + longCache *cache.Cache +} + +func NewCache() *Cache { + return &Cache{ + shortCache: cache.New(5*time.Second, 10*time.Second), + longCache: cache.New(5*time.Minute, 10*time.Minute), + } +} + +// GetLookup is going to return a Lookup identified by a domain name using +// following algorithm: +// - if a domain lookup is present in the short cache it will return just it +// - if it is not present in a short cache it will check the long cache +// - if it is present in a long cache it will return the long cache version and +// run an update in a separate thread that will fetch the lookup from the +// GitLab source and replace the short and long cache entries +// - if a domain lookup is not present in the long cache we will fetch the +// lookup from the domain source and client will need to wait +// TODO use sync.Once to synchronize retrieval +func (c *Cache) GetLookup(domain string, retrieve func() *Lookup) *Lookup { + return nil +} diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client.go index 73abfaa79..202c6943b 100644 --- a/internal/source/gitlab/client.go +++ b/internal/source/gitlab/client.go @@ -2,6 +2,6 @@ package gitlab // client is an internal HTTP client used for communication with GitLab // instance -type client interface { +type Client interface { Resolve(domain string) *Lookup } diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 5eccff174..8ac8fd855 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -1,12 +1,17 @@ package gitlab -import "gitlab.com/gitlab-org/gitlab-pages/internal/domain" +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" +) // Gitlab source represent a new domains configuration source. We fetch all the // information about domains from GitLab instance. type Gitlab struct { - client - lookups []Lookup + client Client + cache Cache } // GetDomain return a representation of a domain that we have fetched from @@ -17,14 +22,20 @@ func (g *Gitlab) GetDomain(name string) *domain.Domain { // HasDomain checks if a domain is known to GitLab func (g *Gitlab) HasDomain(name string) bool { - return false + return g.GetDomain(name) != nil +} + +// Resolve is supposed to get the serving lookup path based on the request from +// the GitLab source +func (g *Gitlab) Resolve(*http.Request) (*serving.LookupPath, string, error) { + return nil, "", nil } -// Watch starts Gitlab domains source +// Watch starts Gitlab domains source TODO remove func (g *Gitlab) Watch(rootDomain string) { } -// Ready checks if Gitlab domains source can be used +// Ready checks if Gitlab domains source can be used TODO remove func (g *Gitlab) Ready() bool { return false } diff --git a/internal/source/gitlab/lookup.go b/internal/source/gitlab/lookup.go index 6e2e0c7c0..593ba8f94 100644 --- a/internal/source/gitlab/lookup.go +++ b/internal/source/gitlab/lookup.go @@ -5,9 +5,8 @@ type Lookup struct { Domain string CertificateCert string CertificateKey string - Serving string - Prefix string LookupPaths []struct { + Prefix string ProjectID int HTTPSOnly bool AccessControl bool -- GitLab From eb17ac651ffe0a8c4f01c541c807ae793db55043 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 30 Oct 2019 12:09:21 +0100 Subject: [PATCH 06/39] Fix linter warning in gitlab source code --- internal/source/gitlab/cache.go | 2 ++ internal/source/gitlab/client.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/source/gitlab/cache.go b/internal/source/gitlab/cache.go index 19efcc1e8..ee21740fb 100644 --- a/internal/source/gitlab/cache.go +++ b/internal/source/gitlab/cache.go @@ -6,11 +6,13 @@ import ( cache "github.com/patrickmn/go-cache" ) +// Cache is a short and long caching mechanism for GitLab source type Cache struct { shortCache *cache.Cache longCache *cache.Cache } +// NewCache creates a new instance of Cache and sets default expiration func NewCache() *Cache { return &Cache{ shortCache: cache.New(5*time.Second, 10*time.Second), diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client.go index 202c6943b..5ec45d9d8 100644 --- a/internal/source/gitlab/client.go +++ b/internal/source/gitlab/client.go @@ -1,6 +1,6 @@ package gitlab -// client is an internal HTTP client used for communication with GitLab +// Client is an internal HTTP client used for communication with GitLab // instance type Client interface { Resolve(domain string) *Lookup -- GitLab From 08fb0bfc792db7c1b006ea066904d19a3456f99f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 23 Nov 2019 15:20:16 +0100 Subject: [PATCH 07/39] Add note about transitional domain source interface --- app.go | 4 ++-- internal/source/disk/disk.go | 14 ++++++++------ internal/source/disk/map.go | 4 ++-- internal/source/disk/map_test.go | 4 ++-- internal/source/source.go | 4 ++-- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/app.go b/app.go index 09bf039eb..ddbe8457a 100644 --- a/app.go +++ b/app.go @@ -53,7 +53,7 @@ type theApp struct { } func (a *theApp) isReady() bool { - return a.domains.Ready() + return a.domains.IsReady() } func (a *theApp) ServeTLS(ch *tls.ClientHelloInfo) (*tls.Certificate, error) { @@ -348,7 +348,7 @@ func (a *theApp) Run() { a.listenMetricsFD(&wg, a.ListenMetrics) } - a.domains.Watch(a.Domain) + a.domains.Start(a.Domain) wg.Wait() } diff --git a/internal/source/disk/disk.go b/internal/source/disk/disk.go index 20524ef13..2283cad4f 100644 --- a/internal/source/disk/disk.go +++ b/internal/source/disk/disk.go @@ -45,15 +45,17 @@ func (d *Disk) HasDomain(host string) bool { return isPresent } -// Ready checks if the domains source is ready for work -func (d *Disk) Ready() bool { +// IsReady checks if the domains source is ready for work. The disk source is +// ready after traversing entire filesystem and reading all domains' +// configuration files. +func (d *Disk) IsReady() bool { return d.dm != nil } -// Watch starts the domain source, in this case it is reading domains from -// groups on disk concurrently -func (d *Disk) Watch(rootDomain string) { - go watch(rootDomain, d.updateDomains, time.Second) +// Start starts the domain source, in this case it is reading domains from +// groups on disk concurrently. +func (d *Disk) Start(rootDomain string) { + go Watch(rootDomain, d.updateDomains, time.Second) } func (d *Disk) updateDomains(dm Map) { diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go index 88f70c3f9..b58433019 100644 --- a/internal/source/disk/map.go +++ b/internal/source/disk/map.go @@ -226,8 +226,8 @@ const ( updateFile = ".update" ) -// watch polls the filesystem and kicks off a new domain directory scan when needed. -func watch(rootDomain string, updater domainsUpdater, interval time.Duration) { +// Watch polls the filesystem and kicks off a new domain directory scan when needed. +func Watch(rootDomain string, updater domainsUpdater, interval time.Duration) { lastUpdate := []byte("no-update") for { diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go index 09e9f630e..23cac3ddc 100644 --- a/internal/source/disk/map_test.go +++ b/internal/source/disk/map_test.go @@ -140,7 +140,7 @@ func writeRandomTimestamp(t *testing.T) { n, _ := rand.Read(b) require.True(t, n > 0, "read some random bytes") - temp, err := ioutil.TempFile(".", "TestWatch") + temp, err := ioutil.TempFile(".", "TestIsWatch") require.NoError(t, err) _, err = temp.Write(b) require.NoError(t, err, "write to tempfile") @@ -156,7 +156,7 @@ func TestWatch(t *testing.T) { require.NoError(t, os.RemoveAll(updateFile)) update := make(chan Map) - go watch("gitlab.io", func(dm Map) { + go Watch("gitlab.io", func(dm Map) { update <- dm }, time.Microsecond*50) diff --git a/internal/source/source.go b/internal/source/source.go index ffde0c900..f7407283b 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -6,6 +6,6 @@ import "gitlab.com/gitlab-org/gitlab-pages/internal/domain" type Source interface { GetDomain(string) *domain.Domain HasDomain(string) bool - Watch(rootDomain string) - Ready() bool + Start(rootDomain string) // Deprecated - transitional interface + IsReady() bool // Deprecated - transitional interface } -- GitLab From bc61f03703dffb79c3d9d887385b968315362caa Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 23 Nov 2019 16:41:13 +0100 Subject: [PATCH 08/39] Implement a transitional domains source --- app.go | 2 +- internal/source/disk/disk.go | 4 +-- internal/source/domains.go | 54 ++++++++++++++++++++++++---- internal/source/domains_test.go | 61 ++++++++++++++++++++++++++++++++ internal/source/gitlab/gitlab.go | 16 ++++----- internal/source/source.go | 4 +-- 6 files changed, 119 insertions(+), 22 deletions(-) create mode 100644 internal/source/domains_test.go diff --git a/app.go b/app.go index ddbe8457a..7c927dfed 100644 --- a/app.go +++ b/app.go @@ -348,7 +348,7 @@ func (a *theApp) Run() { a.listenMetricsFD(&wg, a.ListenMetrics) } - a.domains.Start(a.Domain) + a.domains.Read(a.Domain) wg.Wait() } diff --git a/internal/source/disk/disk.go b/internal/source/disk/disk.go index 2283cad4f..a3ae8901d 100644 --- a/internal/source/disk/disk.go +++ b/internal/source/disk/disk.go @@ -52,9 +52,9 @@ func (d *Disk) IsReady() bool { return d.dm != nil } -// Start starts the domain source, in this case it is reading domains from +// Read starts the domain source, in this case it is reading domains from // groups on disk concurrently. -func (d *Disk) Start(rootDomain string) { +func (d *Disk) Read(rootDomain string) { go Watch(rootDomain, d.updateDomains, time.Second) } diff --git a/internal/source/domains.go b/internal/source/domains.go index df3758838..92bab663a 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -3,12 +3,21 @@ package source import ( "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" ) +var newSourceDomains = []string{ + "pages-project-poc.gitlab.io", + "pages-namespace-poc.gitlab.io", + "pages-custom-poc.grzegorz.co", +} + // Domains struct represents a map of all domains supported by pages. It is -// currently reading them from disk. +// currently using two sources during the transition to the new GitLab domains +// source. type Domains struct { - Source + gitlab Source + disk *disk.Disk // legacy disk source } // NewDomains is a factory method for domains initializing a mutex. It should @@ -16,13 +25,44 @@ type Domains struct { // nil value. func NewDomains() *Domains { return &Domains{ - Source: disk.New(), + disk: disk.New(), + gitlab: gitlab.New(), } } -// GetDomain overridden here allows us to switch behavior and the domains -// source for some subset of domains, to test / PoC the new GitLab Domains -// Source that we plan to introduce +// GetDomain retrieves a domain information from a source. We are using two +// sources here because it allows us to switch behavior and the domain source +// 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 { - return d.Source.GetDomain(name) + return d.source(name).GetDomain(name) +} + +// HasDomain checks if domain exists. It is using new and the legacy domains +// source. +func (d *Domains) HasDomain(name string) bool { + return d.source(name).HasDomain(name) +} + +// Start starts the disk domain source. It is DEPRECATED, because we want to +// remove it entirely when disk source gets removed. +func (d *Domains) Read(rootDomain string) { + d.disk.Read(rootDomain) +} + +// IsReady checks if the disk domain source managed to traverse entire pages +// filesystem and is ready for use. It is DEPRECATED, because we want to remove +// it entirely when disk source gets removed. +func (d *Domains) IsReady() bool { + return d.disk.IsReady() +} + +func (d *Domains) source(domain string) Source { + for _, name := range newSourceDomains { + if domain == name { + return d.gitlab + } + } + + return d.disk } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go new file mode 100644 index 000000000..1da50db8b --- /dev/null +++ b/internal/source/domains_test.go @@ -0,0 +1,61 @@ +package source + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" +) + +type mockSource struct { + mock.Mock +} + +func (m mockSource) GetDomain(name string) *domain.Domain { + args := m.Called(name) + + return args.Get(0).(*domain.Domain) +} + +func (m mockSource) HasDomain(name string) bool { + args := m.Called(name) + + return args.Bool(0) +} + +func TestSourceTransition(t *testing.T) { + testDomain := newSourceDomains[0] + + t.Run("when requesting a test domain", func(t *testing.T) { + newSource := new(mockSource) + newSource.On("GetDomain", testDomain). + Return(&domain.Domain{Name: testDomain}). + Once() + newSource.On("HasDomain", testDomain).Return(true).Once() + defer newSource.AssertExpectations(t) + + domains := &Domains{ + disk: disk.New(), + gitlab: newSource, + } + + domains.GetDomain(testDomain) + domains.HasDomain(testDomain) + }) + + t.Run("when requesting a non-test domain", func(t *testing.T) { + newSource := new(mockSource) + defer newSource.AssertExpectations(t) + + domains := &Domains{ + disk: disk.New(), + gitlab: newSource, + } + + assert.Nil(t, domains.GetDomain("some.test.io")) + assert.False(t, domains.HasDomain("some.test.io")) + }) +} diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 8ac8fd855..b0efb01de 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -14,13 +14,20 @@ type Gitlab struct { cache Cache } +// New returns a new instance of gitlab domain source. +func New() *Gitlab { + return &Gitlab{} +} + // GetDomain return a representation of a domain that we have fetched from // GitLab +// It should return source.Lookup TODO func (g *Gitlab) GetDomain(name string) *domain.Domain { return nil } // HasDomain checks if a domain is known to GitLab +// TODO lookup status code etc. func (g *Gitlab) HasDomain(name string) bool { return g.GetDomain(name) != nil } @@ -30,12 +37,3 @@ func (g *Gitlab) HasDomain(name string) bool { func (g *Gitlab) Resolve(*http.Request) (*serving.LookupPath, string, error) { return nil, "", nil } - -// Watch starts Gitlab domains source TODO remove -func (g *Gitlab) Watch(rootDomain string) { -} - -// Ready checks if Gitlab domains source can be used TODO remove -func (g *Gitlab) Ready() bool { - return false -} diff --git a/internal/source/source.go b/internal/source/source.go index f7407283b..da63cbcc3 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -2,10 +2,8 @@ package source import "gitlab.com/gitlab-org/gitlab-pages/internal/domain" -// Source represents an abstract interface of a domains configuration source +// Source represents an abstract interface of a domains configuration source. type Source interface { GetDomain(string) *domain.Domain HasDomain(string) bool - Start(rootDomain string) // Deprecated - transitional interface - IsReady() bool // Deprecated - transitional interface } -- GitLab From 5ffd39e41866614de47ff427d3156062a6879e1c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sun, 24 Nov 2019 11:29:53 +0100 Subject: [PATCH 09/39] Respond with 502 if a domain can not be retrieved from a source --- app.go | 22 ++++++++++++++++------ internal/auth/auth.go | 11 +++++++---- internal/domain/domain.go | 6 ++---- internal/source/disk/disk.go | 17 ++++------------- internal/source/domains.go | 8 +------- internal/source/domains_test.go | 23 +++++++++-------------- internal/source/gitlab/gitlab.go | 12 +++--------- internal/source/source.go | 3 +-- 8 files changed, 43 insertions(+), 59 deletions(-) diff --git a/app.go b/app.go index 7c927dfed..f64421a42 100644 --- a/app.go +++ b/app.go @@ -61,7 +61,7 @@ func (a *theApp) ServeTLS(ch *tls.ClientHelloInfo) (*tls.Certificate, error) { return nil, nil } - if domain := a.domain(ch.ServerName); domain != nil { + if domain, _ := a.domain(ch.ServerName); domain != nil { tls, _ := domain.EnsureCertificate() return tls, nil } @@ -86,16 +86,18 @@ func (a *theApp) redirectToHTTPS(w http.ResponseWriter, r *http.Request, statusC http.Redirect(w, r, u.String(), statusCode) } -func (a *theApp) getHostAndDomain(r *http.Request) (host string, domain *domain.Domain) { +func (a *theApp) getHostAndDomain(r *http.Request) (string, *domain.Domain, error) { host, _, err := net.SplitHostPort(r.Host) if err != nil { host = r.Host } - return host, a.domain(host) + domain, err := a.domain(host) + + return host, domain, err } -func (a *theApp) domain(host string) *domain.Domain { +func (a *theApp) domain(host string) (*domain.Domain, error) { return a.domains.GetDomain(host) } @@ -163,7 +165,15 @@ func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, ht // downstream middlewares to use func (a *theApp) routingMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - host, domain := a.getHostAndDomain(r) + // if we could not retrieve a domain from domains source we break the + // middleware chain and simply respond with 502 after logging this + host, domain, err := a.getHostAndDomain(r) + if err != nil { + log.WithError(err).Error("could not fetch domain information from a source") + + httperrors.Serve502(w) + return + } r = request.WithHostAndDomain(r, host, domain) @@ -289,7 +299,7 @@ func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler { } func (a *theApp) buildHandlerPipeline() (http.Handler, error) { - // Handlers should be applied in reverse order + // Handlers should be applied in a reverse order handler := a.serveFileOrNotFoundHandler() if !a.DisableCrossOriginRequests { handler = corsHandler.Handler(handler) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 320dcb114..d84fc6900 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -198,14 +198,17 @@ func (a *Auth) checkAuthenticationResponse(session *sessions.Session, w http.Res http.Redirect(w, r, redirectURI, 302) } -func (a *Auth) domainAllowed(domain string, domains *source.Domains) bool { - domainConfigured := (domain == a.pagesDomain) || strings.HasSuffix("."+domain, a.pagesDomain) +func (a *Auth) domainAllowed(name string, domains *source.Domains) bool { + isConfigured := (name == a.pagesDomain) || strings.HasSuffix("."+name, a.pagesDomain) - if domainConfigured { + if isConfigured { return true } - return domains.HasDomain(domain) + domain, err := domains.GetDomain(name) + + // domain exists and there is no error + return (domain != nil && err == nil) } func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, domains *source.Domains) bool { diff --git a/internal/domain/domain.go b/internal/domain/domain.go index f7eba5cad..28eb31960 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -162,10 +162,8 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) { // ServeFileHTTP returns true if something was served, false if not. func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { if d.isUnconfigured() || !d.HasLookupPath(r) { - // TODO: this seems to be wrong: - // as we should rather return false, - // and fallback to `ServeNotFoundHTTP` - // to handle this case + // TODO: this seems to be wrong: as we should rather return false, and + // fallback to `ServeNotFoundHTTP` to handle this case httperrors.Serve404(w) return true } diff --git a/internal/source/disk/disk.go b/internal/source/disk/disk.go index a3ae8901d..b79d222de 100644 --- a/internal/source/disk/disk.go +++ b/internal/source/disk/disk.go @@ -24,25 +24,16 @@ func New() *Disk { } } -// GetDomain returns a domain from the domains map -func (d *Disk) GetDomain(host string) *domain.Domain { +// GetDomain returns a domain from the domains map if it exists +func (d *Disk) GetDomain(host string) (*domain.Domain, error) { host = strings.ToLower(host) - d.lock.RLock() - defer d.lock.RUnlock() - domain, _ := d.dm[host] - return domain -} - -// HasDomain checks for presence of a domain in the domains map -func (d *Disk) HasDomain(host string) bool { d.lock.RLock() defer d.lock.RUnlock() - host = strings.ToLower(host) - _, isPresent := d.dm[host] + domain, _ := d.dm[host] - return isPresent + return domain, nil } // IsReady checks if the domains source is ready for work. The disk source is diff --git a/internal/source/domains.go b/internal/source/domains.go index 92bab663a..f9dadef83 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -34,16 +34,10 @@ func NewDomains() *Domains { // sources here because it allows us to switch behavior and the domain source // 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 { +func (d *Domains) GetDomain(name string) (*domain.Domain, error) { return d.source(name).GetDomain(name) } -// HasDomain checks if domain exists. It is using new and the legacy domains -// source. -func (d *Domains) HasDomain(name string) bool { - return d.source(name).HasDomain(name) -} - // Start starts the disk domain source. It is DEPRECATED, because we want to // remove it entirely when disk source gets removed. func (d *Domains) Read(rootDomain string) { diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 1da50db8b..64ec5f8d1 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" @@ -14,27 +15,20 @@ type mockSource struct { mock.Mock } -func (m mockSource) GetDomain(name string) *domain.Domain { +func (m mockSource) GetDomain(name string) (*domain.Domain, error) { args := m.Called(name) - return args.Get(0).(*domain.Domain) + return args.Get(0).(*domain.Domain), args.Error(1) } -func (m mockSource) HasDomain(name string) bool { - args := m.Called(name) - - return args.Bool(0) -} - -func TestSourceTransition(t *testing.T) { +func TestHasDomain(t *testing.T) { testDomain := newSourceDomains[0] t.Run("when requesting a test domain", func(t *testing.T) { newSource := new(mockSource) newSource.On("GetDomain", testDomain). - Return(&domain.Domain{Name: testDomain}). + Return(&domain.Domain{Name: testDomain}, nil). Once() - newSource.On("HasDomain", testDomain).Return(true).Once() defer newSource.AssertExpectations(t) domains := &Domains{ @@ -43,7 +37,6 @@ func TestSourceTransition(t *testing.T) { } domains.GetDomain(testDomain) - domains.HasDomain(testDomain) }) t.Run("when requesting a non-test domain", func(t *testing.T) { @@ -55,7 +48,9 @@ func TestSourceTransition(t *testing.T) { gitlab: newSource, } - assert.Nil(t, domains.GetDomain("some.test.io")) - assert.False(t, domains.HasDomain("some.test.io")) + domain, err := domains.GetDomain("domain.test.io") + + require.NoError(t, err) + assert.Nil(t, domain) }) } diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index b0efb01de..09cd06b9a 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -1,6 +1,7 @@ package gitlab import ( + "errors" "net/http" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" @@ -21,15 +22,8 @@ func New() *Gitlab { // GetDomain return a representation of a domain that we have fetched from // GitLab -// It should return source.Lookup TODO -func (g *Gitlab) GetDomain(name string) *domain.Domain { - return nil -} - -// HasDomain checks if a domain is known to GitLab -// TODO lookup status code etc. -func (g *Gitlab) HasDomain(name string) bool { - return g.GetDomain(name) != nil +func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) { + return nil, errors.New("not implemented") } // Resolve is supposed to get the serving lookup path based on the request from diff --git a/internal/source/source.go b/internal/source/source.go index da63cbcc3..4b43b8f4b 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -4,6 +4,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 - HasDomain(string) bool + GetDomain(string) (*domain.Domain, error) } -- GitLab From ccbe6b5ca4b56d4453567275b9232c58443d151e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sun, 24 Nov 2019 11:46:55 +0100 Subject: [PATCH 10/39] Add an acceptance test for broken domains source --- acceptance_test.go | 12 ++++++++++++ internal/source/domains.go | 8 ++++++++ internal/source/domains_test.go | 15 +++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/acceptance_test.go b/acceptance_test.go index ec807d3dd..fe598b0c7 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -432,6 +432,18 @@ func TestPageNotAvailableIfNotLoaded(t *testing.T) { require.Equal(t, http.StatusServiceUnavailable, rsp.StatusCode) } +func TestPageNotAvailableInDomainSource(t *testing.T) { + skipUnlessEnabled(t) + teardown := RunPagesProcessWithoutWait(t, *pagesBinary, listeners, "", "-pages-root=shared/invalid-pages") + defer teardown() + waitForRoundtrips(t, listeners, 5*time.Second) + + rsp, err := GetPageFromListener(t, httpListener, "pages-broken-poc.gitlab.io", "index.html") + require.NoError(t, err) + defer rsp.Body.Close() + require.Equal(t, http.StatusBadGateway, rsp.StatusCode) +} + func TestObscureMIMEType(t *testing.T) { skipUnlessEnabled(t) teardown := RunPagesProcessWithoutWait(t, *pagesBinary, listeners, "") diff --git a/internal/source/domains.go b/internal/source/domains.go index f9dadef83..da07fe0ca 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -1,6 +1,8 @@ package source import ( + "errors" + "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" @@ -12,6 +14,8 @@ var newSourceDomains = []string{ "pages-custom-poc.grzegorz.co", } +var brokenSourceDomain = "pages-broken-poc.gitlab.io" + // 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. @@ -35,6 +39,10 @@ func NewDomains() *Domains { // 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 == brokenSourceDomain { + return nil, errors.New("broken test domain used") + } + return d.source(name).GetDomain(name) } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 64ec5f8d1..79fa7081e 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -53,4 +53,19 @@ func TestHasDomain(t *testing.T) { require.NoError(t, err) assert.Nil(t, domain) }) + + t.Run("when requesting a broken test domain", func(t *testing.T) { + newSource := new(mockSource) + defer newSource.AssertExpectations(t) + + domains := &Domains{ + disk: disk.New(), + gitlab: newSource, + } + + domain, err := domains.GetDomain("pages-broken-poc.gitlab.io") + + assert.Nil(t, domain) + assert.EqualError(t, err, "broken test domain used") + }) } -- GitLab From 0b986be3d3b4b5ab440eb4888338fb6e91935572 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sun, 24 Nov 2019 11:50:38 +0100 Subject: [PATCH 11/39] Do not pass mock Mutex by value in domain source test --- internal/source/domains_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 79fa7081e..74ac84c6d 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -15,7 +15,7 @@ type mockSource struct { mock.Mock } -func (m mockSource) GetDomain(name string) (*domain.Domain, error) { +func (m *mockSource) GetDomain(name string) (*domain.Domain, error) { args := m.Called(name) return args.Get(0).(*domain.Domain), args.Error(1) -- GitLab From fec8b9e92f41755aa45a65bc24e48ffe26560942 Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Fri, 18 Oct 2019 10:27:10 +1300 Subject: [PATCH 12/39] Add HTTP client to consume GitLab internal API for Pages At the moment this supports only getting virtual domain configuration for given host. Related to https://gitlab.com/gitlab-org/gitlab-pages/issues/253. --- go.mod | 1 + go.sum | 2 + internal/source/gitlab/client.go | 138 +++++++++++++++++++++++++- internal/source/gitlab/client_test.go | 120 ++++++++++++++++++++++ internal/source/gitlab/response.go | 21 ++++ 5 files changed, 278 insertions(+), 4 deletions(-) create mode 100644 internal/source/gitlab/client_test.go create mode 100644 internal/source/gitlab/response.go diff --git a/go.mod b/go.mod index 59bd759a6..e9a503324 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.12 require ( github.com/certifi/gocertifi v0.0.0-20190905060710-a5e0173ced67 // indirect + github.com/dgrijalva/jwt-go v3.2.0+incompatible github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835 github.com/getsentry/raven-go v0.1.2 // indirect github.com/golang/mock v1.3.1 diff --git a/go.sum b/go.sum index d08aed585..a4d327397 100644 --- a/go.sum +++ b/go.sum @@ -17,6 +17,8 @@ github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd/go.mod h1:sE github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dgrijalva/jwt-go v3.2.0+incompatible h1:7qlOGliEKZXTDg6OTjfoBKDXWrumCAMpl/TFQ4/5kLM= +github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835 h1:roDmqJ4Qes7hrDOsWsMCce0vQHz3xiMPjJ9m4c2eeNs= github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835/go.mod h1:BjL/N0+C+j9uNX+1xcNuM9vdSIcXCZrQZUYbXOFbgN8= diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client.go index 5ec45d9d8..bd422a4ed 100644 --- a/internal/source/gitlab/client.go +++ b/internal/source/gitlab/client.go @@ -1,7 +1,137 @@ package gitlab -// Client is an internal HTTP client used for communication with GitLab -// instance -type Client interface { - Resolve(domain string) *Lookup +import ( + "encoding/json" + "errors" + "net/http" + "net/url" + "time" + + jwt "github.com/dgrijalva/jwt-go" + + "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" +) + +// Client is a HTTP client to access Pages internal API +type Client struct { + secretKey []byte + baseURL *url.URL + httpClient *http.Client +} + +var ( + errUnknown = errors.New("Unknown") + errNoContent = errors.New("No Content") + errUnauthorized = errors.New("Unauthorized") + errNotFound = errors.New("Not Found") +) + +// NewClient initializes and returns new Client +func NewClient(baseURL string, secretKey []byte) (*Client, error) { + url, err := url.Parse(baseURL) + if err != nil { + return nil, err + } + + return &Client{ + secretKey: secretKey, + baseURL: url, + httpClient: &http.Client{ + Timeout: 5 * time.Second, + Transport: httptransport.Transport, + }, + }, nil +} + +// GetVirtualDomain returns VirtualDomain configuration for the given host +func (gc *Client) GetVirtualDomain(host string) (*VirtualDomain, error) { + params := map[string]string{"host": host} + + resp, err := gc.get("/api/v4/internal/pages", params) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + var domain VirtualDomain + err = json.NewDecoder(resp.Body).Decode(&domain) + if err != nil { + return nil, err + } + + return &domain, nil +} + +func (gc *Client) get(path string, params map[string]string) (*http.Response, error) { + endpoint, err := gc.endpoint(path, params) + if err != nil { + return nil, err + } + + req, err := gc.request("GET", endpoint) + if err != nil { + return nil, err + } + + resp, err := gc.httpClient.Do(req) + if err != nil { + return nil, err + } + + switch { + case resp.StatusCode == http.StatusOK: + return resp, nil + case resp.StatusCode == http.StatusNoContent: + return resp, errNoContent + case resp.StatusCode == http.StatusUnauthorized: + return resp, errUnauthorized + case resp.StatusCode == http.StatusNotFound: + return resp, errNotFound + default: + return resp, errUnknown + } +} + +func (gc *Client) endpoint(path string, params map[string]string) (*url.URL, error) { + endpoint, err := gc.baseURL.Parse(path) + if err != nil { + return nil, err + } + + values := url.Values{} + for key, value := range params { + values.Add(key, value) + } + endpoint.RawQuery = values.Encode() + + return endpoint, nil +} + +func (gc *Client) request(method string, endpoint *url.URL) (*http.Request, error) { + req, err := http.NewRequest("GET", endpoint.String(), nil) + if err != nil { + return nil, err + } + + token, err := gc.token() + if err != nil { + return nil, err + } + req.Header.Set("Gitlab-Pages-Api-Request", token) + + return req, nil +} + +func (gc *Client) token() (string, error) { + claims := jwt.StandardClaims{ + Issuer: "gitlab-pages", + ExpiresAt: time.Now().Add(1 * time.Minute).Unix(), + } + + token, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(gc.secretKey) + if err != nil { + return "", err + } + + return token, nil } diff --git a/internal/source/gitlab/client_test.go b/internal/source/gitlab/client_test.go new file mode 100644 index 000000000..d8c86fad7 --- /dev/null +++ b/internal/source/gitlab/client_test.go @@ -0,0 +1,120 @@ +package gitlab + +import ( + "encoding/base64" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + jwt "github.com/dgrijalva/jwt-go" +) + +var ( + encodedSecret = "e41rcFh7XBA7sNABWVCe2AZvxMsy6QDtJ8S9Ql1UiN8=" // 32 bytes, base64 encoded +) + +func TestNewValidBaseURL(t *testing.T) { + _, err := NewClient("https://gitlab.com", secretKey()) + require.NoError(t, err) +} + +func TestNewInvalidBaseURL(t *testing.T) { + client, err := NewClient("%", secretKey()) + require.Error(t, err) + require.Nil(t, client) +} + +func TestGetVirtualDomainForErrorResponses(t *testing.T) { + tests := map[int]string{ + http.StatusNoContent: "No Content", + http.StatusUnauthorized: "Unauthorized", + http.StatusNotFound: "Not Found", + } + + for statusCode, expectedError := range tests { + name := fmt.Sprintf("%d %s", statusCode, expectedError) + t.Run(name, func(t *testing.T) { + mux := http.NewServeMux() + + mux.HandleFunc("/api/v4/internal/pages", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(statusCode) + }) + + server := httptest.NewServer(mux) + defer server.Close() + + client, _ := NewClient(server.URL, secretKey()) + + actual, err := client.GetVirtualDomain("group.gitlab.io") + + require.EqualError(t, err, expectedError) + require.Nil(t, actual) + }) + } +} + +func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) { + mux := http.NewServeMux() + + mux.HandleFunc("/api/v4/internal/pages", func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "group.gitlab.io", r.FormValue("host")) + + if checkRequest(r.Header.Get("Gitlab-Pages-Api-Request")) { + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"certificate":"foo","key":"bar","lookup_paths":[{"project_id":123,"access_control":false,"source":{"type":"file","path":"mygroup/myproject/public/"},"https_only":true,"prefix":"/myproject/"}]}`) + } else { + w.WriteHeader(http.StatusUnauthorized) + } + }) + + server := httptest.NewServer(mux) + defer server.Close() + + client, _ := NewClient(server.URL, secretKey()) + + actual, err := client.GetVirtualDomain("group.gitlab.io") + require.NoError(t, err) + + require.Equal(t, "foo", actual.Certificate) + require.Equal(t, "bar", actual.Key) + + lookupPath := actual.LookupPaths[0] + require.Equal(t, 123, lookupPath.ProjectID) + require.Equal(t, false, lookupPath.AccessControl) + require.Equal(t, true, lookupPath.HTTPSOnly) + require.Equal(t, "/myproject/", lookupPath.Prefix) + + require.Equal(t, "file", lookupPath.Source.Type) + require.Equal(t, "mygroup/myproject/public/", lookupPath.Source.Path) +} + +func checkRequest(tokenString string) bool { + token, _ := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) { + if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { + return nil, fmt.Errorf("Unexpected signing method: %v", token.Header["alg"]) + } + + return secretKey(), nil + }) + + claims, ok := token.Claims.(jwt.MapClaims) + if !ok || !token.Valid { + return false + } + + if _, ok := claims["exp"]; !ok { + return false + } + + return claims["iss"] == "gitlab-pages" +} + +func secretKey() []byte { + secretKey, _ := base64.StdEncoding.DecodeString(encodedSecret) + return secretKey +} diff --git a/internal/source/gitlab/response.go b/internal/source/gitlab/response.go new file mode 100644 index 000000000..20597362e --- /dev/null +++ b/internal/source/gitlab/response.go @@ -0,0 +1,21 @@ +package gitlab + +// LookupPath represents a lookup path for a GitLab Pages virtual domain +type LookupPath struct { + ProjectID int `json:"project_id,omitempty"` + AccessControl bool `json:"access_control,omitempty"` + HTTPSOnly bool `json:"https_only,omitempty"` + Prefix string `json:"prefix,omitempty"` + Source struct { + Type string `json:"type,omitempty"` + Path string `json:"path,omitempty"` + } +} + +// VirtualDomain represents a GitLab Pages virtual domain +type VirtualDomain struct { + Certificate string `json:"certificate,omitempty"` + Key string `json:"key,omitempty"` + + LookupPaths []LookupPath `json:"lookup_paths"` +} -- GitLab From 2ac77b951abec0fd3c1589f645061e277ae21395 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 25 Nov 2019 14:02:24 +0100 Subject: [PATCH 13/39] Extract gitlab source response to a separate package --- internal/source/gitlab/client.go | 5 +++-- .../gitlab/{response.go => domain/lookup_path.go} | 12 ++---------- internal/source/gitlab/domain/virtual_domain.go | 10 ++++++++++ 3 files changed, 15 insertions(+), 12 deletions(-) rename internal/source/gitlab/{response.go => domain/lookup_path.go} (51%) create mode 100644 internal/source/gitlab/domain/virtual_domain.go diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client.go index bd422a4ed..1fbbe2aba 100644 --- a/internal/source/gitlab/client.go +++ b/internal/source/gitlab/client.go @@ -10,6 +10,7 @@ import ( jwt "github.com/dgrijalva/jwt-go" "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" ) // Client is a HTTP client to access Pages internal API @@ -44,7 +45,7 @@ func NewClient(baseURL string, secretKey []byte) (*Client, error) { } // GetVirtualDomain returns VirtualDomain configuration for the given host -func (gc *Client) GetVirtualDomain(host string) (*VirtualDomain, error) { +func (gc *Client) GetVirtualDomain(host string) (*domain.VirtualDomain, error) { params := map[string]string{"host": host} resp, err := gc.get("/api/v4/internal/pages", params) @@ -53,7 +54,7 @@ func (gc *Client) GetVirtualDomain(host string) (*VirtualDomain, error) { } defer resp.Body.Close() - var domain VirtualDomain + var domain domain.VirtualDomain err = json.NewDecoder(resp.Body).Decode(&domain) if err != nil { return nil, err diff --git a/internal/source/gitlab/response.go b/internal/source/gitlab/domain/lookup_path.go similarity index 51% rename from internal/source/gitlab/response.go rename to internal/source/gitlab/domain/lookup_path.go index 20597362e..7ce916084 100644 --- a/internal/source/gitlab/response.go +++ b/internal/source/gitlab/domain/lookup_path.go @@ -1,6 +1,6 @@ -package gitlab +package domain -// LookupPath represents a lookup path for a GitLab Pages virtual domain +// LookupPath represents a lookup path for a virtual domain type LookupPath struct { ProjectID int `json:"project_id,omitempty"` AccessControl bool `json:"access_control,omitempty"` @@ -11,11 +11,3 @@ type LookupPath struct { Path string `json:"path,omitempty"` } } - -// VirtualDomain represents a GitLab Pages virtual domain -type VirtualDomain struct { - Certificate string `json:"certificate,omitempty"` - Key string `json:"key,omitempty"` - - LookupPaths []LookupPath `json:"lookup_paths"` -} diff --git a/internal/source/gitlab/domain/virtual_domain.go b/internal/source/gitlab/domain/virtual_domain.go new file mode 100644 index 000000000..9a1c17ccf --- /dev/null +++ b/internal/source/gitlab/domain/virtual_domain.go @@ -0,0 +1,10 @@ +package domain + +// VirtualDomain represents a GitLab Pages virtual domain that is being sent +// from GitLab +type VirtualDomain struct { + Certificate string `json:"certificate,omitempty"` + Key string `json:"key,omitempty"` + + LookupPaths []LookupPath `json:"lookup_paths"` +} -- GitLab From 026b0df87d65e2219c8ce4a507dfe3b59258003a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 25 Nov 2019 14:05:59 +0100 Subject: [PATCH 14/39] Move gitlab source client and cache to separate packages --- internal/source/gitlab/cache.go | 35 ------------------- internal/source/gitlab/cache/cache.go | 10 ++++++ internal/source/gitlab/{ => client}/client.go | 2 +- .../source/gitlab/{ => client}/client_test.go | 2 +- internal/source/gitlab/gitlab.go | 6 ++-- 5 files changed, 16 insertions(+), 39 deletions(-) delete mode 100644 internal/source/gitlab/cache.go create mode 100644 internal/source/gitlab/cache/cache.go rename internal/source/gitlab/{ => client}/client.go (99%) rename internal/source/gitlab/{ => client}/client_test.go (99%) diff --git a/internal/source/gitlab/cache.go b/internal/source/gitlab/cache.go deleted file mode 100644 index ee21740fb..000000000 --- a/internal/source/gitlab/cache.go +++ /dev/null @@ -1,35 +0,0 @@ -package gitlab - -import ( - "time" - - cache "github.com/patrickmn/go-cache" -) - -// Cache is a short and long caching mechanism for GitLab source -type Cache struct { - shortCache *cache.Cache - longCache *cache.Cache -} - -// NewCache creates a new instance of Cache and sets default expiration -func NewCache() *Cache { - return &Cache{ - shortCache: cache.New(5*time.Second, 10*time.Second), - longCache: cache.New(5*time.Minute, 10*time.Minute), - } -} - -// GetLookup is going to return a Lookup identified by a domain name using -// following algorithm: -// - if a domain lookup is present in the short cache it will return just it -// - if it is not present in a short cache it will check the long cache -// - if it is present in a long cache it will return the long cache version and -// run an update in a separate thread that will fetch the lookup from the -// GitLab source and replace the short and long cache entries -// - if a domain lookup is not present in the long cache we will fetch the -// lookup from the domain source and client will need to wait -// TODO use sync.Once to synchronize retrieval -func (c *Cache) GetLookup(domain string, retrieve func() *Lookup) *Lookup { - return nil -} diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go new file mode 100644 index 000000000..cd95c82f2 --- /dev/null +++ b/internal/source/gitlab/cache/cache.go @@ -0,0 +1,10 @@ +package cache + +// Cache is a short and long caching mechanism for GitLab source +type Cache struct { +} + +// NewCache creates a new instance of Cache and sets default expiration +func NewCache() *Cache { + return &Cache{} +} diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client/client.go similarity index 99% rename from internal/source/gitlab/client.go rename to internal/source/gitlab/client/client.go index 1fbbe2aba..467bcaa47 100644 --- a/internal/source/gitlab/client.go +++ b/internal/source/gitlab/client/client.go @@ -1,4 +1,4 @@ -package gitlab +package client import ( "encoding/json" diff --git a/internal/source/gitlab/client_test.go b/internal/source/gitlab/client/client_test.go similarity index 99% rename from internal/source/gitlab/client_test.go rename to internal/source/gitlab/client/client_test.go index d8c86fad7..3d0fdad2e 100644 --- a/internal/source/gitlab/client_test.go +++ b/internal/source/gitlab/client/client_test.go @@ -1,4 +1,4 @@ -package gitlab +package client import ( "encoding/base64" diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 09cd06b9a..40d3256fe 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -6,13 +6,15 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/cache" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client" ) // Gitlab source represent a new domains configuration source. We fetch all the // information about domains from GitLab instance. type Gitlab struct { - client Client - cache Cache + client client.Client + cache cache.Cache } // New returns a new instance of gitlab domain source. -- GitLab From 7b3fee685aa5b3cf9f800e3687c2fafecf9956ac Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 25 Nov 2019 14:14:17 +0100 Subject: [PATCH 15/39] Rrun go mod tidy to fix dependencies check --- go.mod | 1 - go.sum | 2 -- 2 files changed, 3 deletions(-) diff --git a/go.mod b/go.mod index e9a503324..f76c0c92f 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,6 @@ require ( github.com/karrick/godirwalk v1.10.12 github.com/kr/pretty v0.1.0 // indirect github.com/namsral/flag v1.7.4-pre - github.com/patrickmn/go-cache v2.1.0+incompatible github.com/prometheus/client_golang v1.1.0 github.com/rs/cors v1.7.0 github.com/sirupsen/logrus v1.4.2 diff --git a/go.sum b/go.sum index a4d327397..14a9bf73a 100644 --- a/go.sum +++ b/go.sum @@ -85,8 +85,6 @@ github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+W github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/gomega v1.4.3/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= github.com/opentracing/opentracing-go v1.0.2/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o= -github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= -github.com/patrickmn/go-cache v2.1.0+incompatible/go.mod h1:3Qf8kWWT7OJRJbdiICTKqZju1ZixQ/KpMGzzAfe6+WQ= github.com/philhofer/fwd v1.0.0/go.mod h1:gk3iGcWd9+svBvR0sR+KPcfE+RNWozjowpeBVG3ZVNU= github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= -- GitLab From d2e70a9eb308ba9c7fdec9ed19cb44fa235a0a67 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 25 Nov 2019 14:18:15 +0100 Subject: [PATCH 16/39] Remove unused gitlab source Lookup type --- internal/source/gitlab/lookup.go | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 internal/source/gitlab/lookup.go diff --git a/internal/source/gitlab/lookup.go b/internal/source/gitlab/lookup.go deleted file mode 100644 index 593ba8f94..000000000 --- a/internal/source/gitlab/lookup.go +++ /dev/null @@ -1,18 +0,0 @@ -package gitlab - -// Lookup defines a response that GitLab can send, which we can unmarshall -type Lookup struct { - Domain string - CertificateCert string - CertificateKey string - LookupPaths []struct { - Prefix string - ProjectID int - HTTPSOnly bool - AccessControl bool - Source struct { - Type string - Path string - } - } -} -- GitLab From 677a15554a5ccb3138593bb6f0d5a37efc8a32f6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 25 Nov 2019 15:36:46 +0100 Subject: [PATCH 17/39] Make it possible to pass client config to gitlab source --- app.go | 2 +- app_config.go | 10 ++++++++++ internal/auth/auth.go | 6 +++--- internal/auth/auth_test.go | 21 +++++++++++++++----- internal/source/config.go | 8 ++++++++ internal/source/domains.go | 4 ++-- internal/source/gitlab/cache/cache.go | 4 ++-- internal/source/gitlab/client/client.go | 12 ++++++++--- internal/source/gitlab/client/client_test.go | 15 ++------------ internal/source/gitlab/client/config.go | 8 ++++++++ internal/source/gitlab/gitlab.go | 8 ++++---- 11 files changed, 65 insertions(+), 33 deletions(-) create mode 100644 internal/source/config.go create mode 100644 internal/source/gitlab/client/config.go diff --git a/app.go b/app.go index f64421a42..eeefaeaf1 100644 --- a/app.go +++ b/app.go @@ -413,7 +413,7 @@ func (a *theApp) listenMetricsFD(wg *sync.WaitGroup, fd uintptr) { } func runApp(config appConfig) { - a := theApp{appConfig: config, domains: source.NewDomains()} + a := theApp{appConfig: config, domains: source.NewDomains(config)} err := logging.ConfigureLogging(a.LogFormat, a.LogVerbose) if err != nil { diff --git a/app_config.go b/app_config.go index b870626d9..379a3696b 100644 --- a/app_config.go +++ b/app_config.go @@ -34,3 +34,13 @@ type appConfig struct { SentryEnvironment string CustomHeaders []string } + +// GitlabServerURL returns URL to a GitLab instance. +func (config appConfig) GitlabServerURL() string { + return config.GitLabServer +} + +// GitlabClientSecret returns GitLab server access token. +func (config appConfig) GitlabClientSecret() []byte { + return []byte(config.ClientSecret) +} diff --git a/internal/auth/auth.go b/internal/auth/auth.go index d84fc6900..f30c74070 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -107,7 +107,7 @@ func (a *Auth) checkSession(w http.ResponseWriter, r *http.Request) (*sessions.S } // TryAuthenticate tries to authenticate user and fetch access token if request is a callback to auth -func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, domains *source.Domains) bool { +func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, domains source.Source) bool { if a == nil { return false } @@ -198,7 +198,7 @@ func (a *Auth) checkAuthenticationResponse(session *sessions.Session, w http.Res http.Redirect(w, r, redirectURI, 302) } -func (a *Auth) domainAllowed(name string, domains *source.Domains) bool { +func (a *Auth) domainAllowed(name string, domains source.Source) bool { isConfigured := (name == a.pagesDomain) || strings.HasSuffix("."+name, a.pagesDomain) if isConfigured { @@ -211,7 +211,7 @@ func (a *Auth) domainAllowed(name string, domains *source.Domains) bool { return (domain != nil && err == nil) } -func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, domains *source.Domains) bool { +func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, domains source.Source) bool { // If request is for authenticating via custom domain if shouldProxyAuth(r) { domain := r.URL.Query().Get("domain") diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index c082cfdf7..0278b595c 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -10,10 +10,11 @@ import ( "testing" "github.com/gorilla/sessions" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/request" - "gitlab.com/gitlab-org/gitlab-pages/internal/source" ) func createAuth(t *testing.T) *Auth { @@ -29,6 +30,16 @@ func defaultCookieStore() sessions.Store { return createCookieStore("something-very-secret") } +type mockSource struct { + mock.Mock +} + +func (m *mockSource) GetDomain(name string) (*domain.Domain, error) { + args := m.Called(name) + + return args.Get(0).(*domain.Domain), args.Error(1) +} + // Gorilla's sessions use request context to save session // Which makes session sharable between test code and actually manipulating session // Which leads to negative side effects: we can't test encryption, and cookie params @@ -56,7 +67,7 @@ func TestTryAuthenticate(t *testing.T) { require.NoError(t, err) r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true) - require.Equal(t, false, auth.TryAuthenticate(result, r, source.NewDomains())) + require.Equal(t, false, auth.TryAuthenticate(result, r, new(mockSource))) } func TestTryAuthenticateWithError(t *testing.T) { @@ -67,7 +78,7 @@ func TestTryAuthenticateWithError(t *testing.T) { require.NoError(t, err) r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true) - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewDomains())) + require.Equal(t, true, auth.TryAuthenticate(result, r, new(mockSource))) require.Equal(t, 401, result.Code) } @@ -84,7 +95,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) { session.Values["state"] = "state" session.Save(r, result) - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewDomains())) + require.Equal(t, true, auth.TryAuthenticate(result, r, new(mockSource))) require.Equal(t, 401, result.Code) } @@ -124,7 +135,7 @@ func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) { }) result := httptest.NewRecorder() - require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewDomains())) + require.Equal(t, true, auth.TryAuthenticate(result, r, new(mockSource))) require.Equal(t, 302, result.Code) require.Equal(t, "https://pages.gitlab-example.com/project/", result.Header().Get("Location")) require.Equal(t, 600, result.Result().Cookies()[0].MaxAge) diff --git a/internal/source/config.go b/internal/source/config.go new file mode 100644 index 000000000..9ca398cf2 --- /dev/null +++ b/internal/source/config.go @@ -0,0 +1,8 @@ +package source + +// Config represents an interface that is configuration provider for client +// capable of comunicating with GitLab +type Config interface { + GitlabServerURL() string + GitlabClientSecret() []byte +} diff --git a/internal/source/domains.go b/internal/source/domains.go index da07fe0ca..1db285a48 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -27,10 +27,10 @@ type Domains struct { // NewDomains is a factory method for domains initializing a mutex. It should // not initialize `dm` as we later check the readiness by comparing it with a // nil value. -func NewDomains() *Domains { +func NewDomains(config Config) *Domains { return &Domains{ disk: disk.New(), - gitlab: gitlab.New(), + gitlab: gitlab.New(config), } } diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go index cd95c82f2..95c4c57bd 100644 --- a/internal/source/gitlab/cache/cache.go +++ b/internal/source/gitlab/cache/cache.go @@ -4,7 +4,7 @@ package cache type Cache struct { } -// NewCache creates a new instance of Cache and sets default expiration -func NewCache() *Cache { +// New creates a new instance of Cache and sets default expiration +func New() *Cache { return &Cache{} } diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index 467bcaa47..c4c7bb528 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -11,6 +11,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" + "gitlab.com/gitlab-org/labkit/log" ) // Client is a HTTP client to access Pages internal API @@ -28,10 +29,10 @@ var ( ) // NewClient initializes and returns new Client -func NewClient(baseURL string, secretKey []byte) (*Client, error) { +func NewClient(baseURL string, secretKey []byte) *Client { url, err := url.Parse(baseURL) if err != nil { - return nil, err + log.WithError(err).Fatal("could not parse GitLab server URL") } return &Client{ @@ -41,7 +42,12 @@ func NewClient(baseURL string, secretKey []byte) (*Client, error) { Timeout: 5 * time.Second, Transport: httptransport.Transport, }, - }, nil + } +} + +// NewFromConfig creates a new client from Config struct +func NewFromConfig(config Config) *Client { + return NewClient(config.GitlabServerURL(), config.GitlabClientSecret()) } // GetVirtualDomain returns VirtualDomain configuration for the given host diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go index 3d0fdad2e..678cef787 100644 --- a/internal/source/gitlab/client/client_test.go +++ b/internal/source/gitlab/client/client_test.go @@ -17,17 +17,6 @@ var ( encodedSecret = "e41rcFh7XBA7sNABWVCe2AZvxMsy6QDtJ8S9Ql1UiN8=" // 32 bytes, base64 encoded ) -func TestNewValidBaseURL(t *testing.T) { - _, err := NewClient("https://gitlab.com", secretKey()) - require.NoError(t, err) -} - -func TestNewInvalidBaseURL(t *testing.T) { - client, err := NewClient("%", secretKey()) - require.Error(t, err) - require.Nil(t, client) -} - func TestGetVirtualDomainForErrorResponses(t *testing.T) { tests := map[int]string{ http.StatusNoContent: "No Content", @@ -47,7 +36,7 @@ func TestGetVirtualDomainForErrorResponses(t *testing.T) { server := httptest.NewServer(mux) defer server.Close() - client, _ := NewClient(server.URL, secretKey()) + client := NewClient(server.URL, secretKey()) actual, err := client.GetVirtualDomain("group.gitlab.io") @@ -75,7 +64,7 @@ func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) { server := httptest.NewServer(mux) defer server.Close() - client, _ := NewClient(server.URL, secretKey()) + client := NewClient(server.URL, secretKey()) actual, err := client.GetVirtualDomain("group.gitlab.io") require.NoError(t, err) diff --git a/internal/source/gitlab/client/config.go b/internal/source/gitlab/client/config.go new file mode 100644 index 000000000..dd8112da5 --- /dev/null +++ b/internal/source/gitlab/client/config.go @@ -0,0 +1,8 @@ +package client + +// Config represents an interface that is configuration provider for client +// capable of comunicating with GitLab +type Config interface { + GitlabServerURL() string + GitlabClientSecret() []byte +} diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 40d3256fe..f44c8e056 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -13,13 +13,13 @@ import ( // Gitlab source represent a new domains configuration source. We fetch all the // information about domains from GitLab instance. type Gitlab struct { - client client.Client - cache cache.Cache + client *client.Client + cache *cache.Cache } // New returns a new instance of gitlab domain source. -func New() *Gitlab { - return &Gitlab{} +func New(config client.Config) *Gitlab { + return &Gitlab{client: client.NewFromConfig(config), cache: cache.New()} } // GetDomain return a representation of a domain that we have fetched from -- GitLab From c5bdfd0e034e79e460d2fb663f63fe41d46e5bd1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 25 Nov 2019 15:54:57 +0100 Subject: [PATCH 18/39] Fix gitlab source client imports ordering --- internal/source/gitlab/client/client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index c4c7bb528..43cc84bfe 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -9,9 +9,10 @@ import ( jwt "github.com/dgrijalva/jwt-go" + "gitlab.com/gitlab-org/labkit/log" + "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" - "gitlab.com/gitlab-org/labkit/log" ) // Client is a HTTP client to access Pages internal API -- GitLab From 5681803e1073edbc4fc3fa9c8f2f48ab6199b887 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Nov 2019 13:20:51 +0100 Subject: [PATCH 19/39] Simplify gitlab source config --- internal/source/config.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/source/config.go b/internal/source/config.go index 9ca398cf2..9cf87bc65 100644 --- a/internal/source/config.go +++ b/internal/source/config.go @@ -1,8 +1,7 @@ package source +import "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client" + // Config represents an interface that is configuration provider for client // capable of comunicating with GitLab -type Config interface { - GitlabServerURL() string - GitlabClientSecret() []byte -} +type Config client.Config -- GitLab From 806c6dcec76021c78026dc0301331dbe983a938d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Nov 2019 14:34:50 +0100 Subject: [PATCH 20/39] Implement raw gitlab source and prepare interfaces for testing --- internal/serving/lookup_path.go | 5 ++-- internal/source/gitlab/client.go | 10 +++++++ internal/source/gitlab/gitlab.go | 51 ++++++++++++++++++++++++++++---- 3 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 internal/source/gitlab/client.go diff --git a/internal/serving/lookup_path.go b/internal/serving/lookup_path.go index ba6e8f7a9..cace3f435 100644 --- a/internal/serving/lookup_path.go +++ b/internal/serving/lookup_path.go @@ -1,9 +1,10 @@ package serving // LookupPath holds a domain project configuration needed to handle a request +// TODO We might want to swap Location and Path names. type LookupPath struct { - Location string - Path string + Location string // Location is a path to a project requested in a request + Path string // Path is an internal and serving-specific location of a document IsNamespaceProject bool IsHTTPSOnly bool HasAccessControl bool diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client.go new file mode 100644 index 000000000..1485fc25b --- /dev/null +++ b/internal/source/gitlab/client.go @@ -0,0 +1,10 @@ +package gitlab + +import "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" + +// Client interace represents a client capable of fetching a virtual domain +// from an external API +type Client interface { + // GetVirtualDomain retrieves a virtual domain from an external API + GetVirtualDomain(host string) (*domain.VirtualDomain, error) +} diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index f44c8e056..18da0c0bb 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -2,7 +2,9 @@ package gitlab import ( "errors" + "net" "net/http" + "strings" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" @@ -13,8 +15,8 @@ import ( // Gitlab source represent a new domains configuration source. We fetch all the // information about domains from GitLab instance. type Gitlab struct { - client *client.Client - cache *cache.Cache + client Client + cache *cache.Cache // WIP } // New returns a new instance of gitlab domain source. @@ -25,11 +27,50 @@ func New(config client.Config) *Gitlab { // GetDomain return a representation of a domain that we have fetched from // GitLab func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) { - return nil, errors.New("not implemented") + response, err := g.client.GetVirtualDomain(name) + if err != nil { + return nil, err + } + + domain := domain.Domain{ + Name: name, + CertificateCert: response.Certificate, + CertificateKey: response.Key, + Resolver: g, + } + + return &domain, nil } // Resolve is supposed to get the serving lookup path based on the request from // the GitLab source -func (g *Gitlab) Resolve(*http.Request) (*serving.LookupPath, string, error) { - return nil, "", nil +func (g *Gitlab) Resolve(r *http.Request) (*serving.LookupPath, string, error) { + domain, _, err := net.SplitHostPort(r.Host) + if err != nil { + return nil, "", err + } + + response, err := g.client.GetVirtualDomain(domain) + if err != nil { + return nil, "", err + } + + for _, lookup := range response.LookupPaths { + if strings.Contains(r.URL.Path, lookup.Prefix) { + lookupPath := &serving.LookupPath{ + Location: lookup.Prefix, + Path: lookup.Source.Path, + IsNamespaceProject: false, // TODO is this still relevant? it is not served in the API + IsHTTPSOnly: lookup.HTTPSOnly, + HasAccessControl: lookup.AccessControl, + ProjectID: uint64(lookup.ProjectID), + } + + requestPath := strings.Replace(r.URL.Path, lookup.Prefix, "", 1) + + return lookupPath, requestPath, nil + } + } + + return nil, "", errors.New("could not match lookup path") } -- GitLab From 204b225d2308a61586ad7557d1f2f86bebfe73e6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Nov 2019 15:22:02 +0100 Subject: [PATCH 21/39] Make new gitlab domains source more testable --- internal/auth/auth_test.go | 21 +++-------- internal/source/domains_test.go | 17 ++------- internal/source/gitlab/client/client_stub.go | 31 ++++++++++++++++ .../client/testdata/test.gitlab.io.json | 1 + internal/source/gitlab/gitlab_test.go | 35 +++++++++++++++++++ internal/source/source_mock.go | 24 +++++++++++++ 6 files changed, 99 insertions(+), 30 deletions(-) create mode 100644 internal/source/gitlab/client/client_stub.go create mode 100644 internal/source/gitlab/client/testdata/test.gitlab.io.json create mode 100644 internal/source/gitlab/gitlab_test.go create mode 100644 internal/source/source_mock.go diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 0278b595c..92e1e8c7d 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -10,11 +10,10 @@ import ( "testing" "github.com/gorilla/sessions" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/request" + "gitlab.com/gitlab-org/gitlab-pages/internal/source" ) func createAuth(t *testing.T) *Auth { @@ -30,16 +29,6 @@ func defaultCookieStore() sessions.Store { return createCookieStore("something-very-secret") } -type mockSource struct { - mock.Mock -} - -func (m *mockSource) GetDomain(name string) (*domain.Domain, error) { - args := m.Called(name) - - return args.Get(0).(*domain.Domain), args.Error(1) -} - // Gorilla's sessions use request context to save session // Which makes session sharable between test code and actually manipulating session // Which leads to negative side effects: we can't test encryption, and cookie params @@ -67,7 +56,7 @@ func TestTryAuthenticate(t *testing.T) { require.NoError(t, err) r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true) - require.Equal(t, false, auth.TryAuthenticate(result, r, new(mockSource))) + require.Equal(t, false, auth.TryAuthenticate(result, r, source.NewMockSource())) } func TestTryAuthenticateWithError(t *testing.T) { @@ -78,7 +67,7 @@ func TestTryAuthenticateWithError(t *testing.T) { require.NoError(t, err) r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true) - require.Equal(t, true, auth.TryAuthenticate(result, r, new(mockSource))) + require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) require.Equal(t, 401, result.Code) } @@ -95,7 +84,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) { session.Values["state"] = "state" session.Save(r, result) - require.Equal(t, true, auth.TryAuthenticate(result, r, new(mockSource))) + require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) require.Equal(t, 401, result.Code) } @@ -135,7 +124,7 @@ func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) { }) result := httptest.NewRecorder() - require.Equal(t, true, auth.TryAuthenticate(result, r, new(mockSource))) + require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) require.Equal(t, 302, result.Code) require.Equal(t, "https://pages.gitlab-example.com/project/", result.Header().Get("Location")) require.Equal(t, 600, result.Result().Cookies()[0].MaxAge) diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 74ac84c6d..5125f4620 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -4,28 +4,17 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/disk" ) -type mockSource struct { - mock.Mock -} - -func (m *mockSource) GetDomain(name string) (*domain.Domain, error) { - args := m.Called(name) - - return args.Get(0).(*domain.Domain), args.Error(1) -} - func TestHasDomain(t *testing.T) { testDomain := newSourceDomains[0] t.Run("when requesting a test domain", func(t *testing.T) { - newSource := new(mockSource) + newSource := NewMockSource() newSource.On("GetDomain", testDomain). Return(&domain.Domain{Name: testDomain}, nil). Once() @@ -40,7 +29,7 @@ func TestHasDomain(t *testing.T) { }) t.Run("when requesting a non-test domain", func(t *testing.T) { - newSource := new(mockSource) + newSource := NewMockSource() defer newSource.AssertExpectations(t) domains := &Domains{ @@ -55,7 +44,7 @@ func TestHasDomain(t *testing.T) { }) t.Run("when requesting a broken test domain", func(t *testing.T) { - newSource := new(mockSource) + newSource := NewMockSource() defer newSource.AssertExpectations(t) domains := &Domains{ diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go new file mode 100644 index 000000000..48dbf5494 --- /dev/null +++ b/internal/source/gitlab/client/client_stub.go @@ -0,0 +1,31 @@ +package client + +import ( + "encoding/json" + "os" + + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" +) + +// StubClient is a stubbed client used for testing +type StubClient struct { + file string +} + +// GetVirtualDomain reads a test fixture and unmarshalls it +func (m *StubClient) GetVirtualDomain(host string) (domain *domain.VirtualDomain, err error) { + f, err := os.Open(m.file) + defer f.Close() + if err != nil { + return nil, err + } + + err = json.NewDecoder(f).Decode(&domain) + + return domain, err +} + +// NewStubClient return a stubbed client +func NewStubClient(fixture string) *StubClient { + return &StubClient{file: fixture} +} diff --git a/internal/source/gitlab/client/testdata/test.gitlab.io.json b/internal/source/gitlab/client/testdata/test.gitlab.io.json new file mode 100644 index 000000000..dfe65ad0d --- /dev/null +++ b/internal/source/gitlab/client/testdata/test.gitlab.io.json @@ -0,0 +1 @@ +{"certificate":"some--cert","key":"some--key","lookup_paths":[{"project_id":123,"access_control":false,"https_only":true,"prefix":"/my/pages/project","source":{"type":"file","path":"/some/path/to/project/"}}]} \ No newline at end of file diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go new file mode 100644 index 000000000..74cbcb794 --- /dev/null +++ b/internal/source/gitlab/gitlab_test.go @@ -0,0 +1,35 @@ +package gitlab + +import ( + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/cache" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client" +) + +func TestGetDomain(t *testing.T) { + client := client.NewStubClient("client/testdata/test.gitlab.io.json") + source := Gitlab{client: client, cache: cache.New()} + + domain, err := source.GetDomain("test.gitlab.io") + require.NoError(t, err) + + assert.Equal(t, "test.gitlab.io", domain.Name) +} + +func TestResolve(t *testing.T) { + client := client.NewStubClient("client/testdata/test.gitlab.io.json") + source := Gitlab{client: client, cache: cache.New()} + target := "https://test.gitlab.io:443/my/pages/project/path/index.html" + request := httptest.NewRequest("GET", target, nil) + + lookup, subpath, err := source.Resolve(request) + require.NoError(t, err) + + assert.Equal(t, "/my/pages/project", lookup.Location) + assert.Equal(t, "/path/index.html", subpath) +} diff --git a/internal/source/source_mock.go b/internal/source/source_mock.go new file mode 100644 index 000000000..ee24d804e --- /dev/null +++ b/internal/source/source_mock.go @@ -0,0 +1,24 @@ +package source + +import ( + "github.com/stretchr/testify/mock" + + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" +) + +// MockSource can be used for testing +type MockSource struct { + mock.Mock +} + +// GetDomain is a mocked function +func (m *MockSource) GetDomain(name string) (*domain.Domain, error) { + args := m.Called(name) + + return args.Get(0).(*domain.Domain), args.Error(1) +} + +// NewMockSource returns a new Source mock for testing +func NewMockSource() *MockSource { + return &MockSource{} +} -- GitLab From f7d89e372df5d4c59c1a175bb807578f4350391b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Nov 2019 15:25:35 +0100 Subject: [PATCH 22/39] Rename package that holds GitLab API response types --- internal/source/gitlab/{domain => api}/lookup_path.go | 2 +- internal/source/gitlab/{domain => api}/virtual_domain.go | 4 ++-- internal/source/gitlab/client.go | 4 ++-- internal/source/gitlab/client/client.go | 6 +++--- internal/source/gitlab/client/client_stub.go | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) rename internal/source/gitlab/{domain => api}/lookup_path.go (96%) rename internal/source/gitlab/{domain => api}/virtual_domain.go (89%) diff --git a/internal/source/gitlab/domain/lookup_path.go b/internal/source/gitlab/api/lookup_path.go similarity index 96% rename from internal/source/gitlab/domain/lookup_path.go rename to internal/source/gitlab/api/lookup_path.go index 7ce916084..b04076386 100644 --- a/internal/source/gitlab/domain/lookup_path.go +++ b/internal/source/gitlab/api/lookup_path.go @@ -1,4 +1,4 @@ -package domain +package api // LookupPath represents a lookup path for a virtual domain type LookupPath struct { diff --git a/internal/source/gitlab/domain/virtual_domain.go b/internal/source/gitlab/api/virtual_domain.go similarity index 89% rename from internal/source/gitlab/domain/virtual_domain.go rename to internal/source/gitlab/api/virtual_domain.go index 9a1c17ccf..200c06de8 100644 --- a/internal/source/gitlab/domain/virtual_domain.go +++ b/internal/source/gitlab/api/virtual_domain.go @@ -1,7 +1,7 @@ -package domain +package api // VirtualDomain represents a GitLab Pages virtual domain that is being sent -// from GitLab +// from GitLab API type VirtualDomain struct { Certificate string `json:"certificate,omitempty"` Key string `json:"key,omitempty"` diff --git a/internal/source/gitlab/client.go b/internal/source/gitlab/client.go index 1485fc25b..5b6cd07c2 100644 --- a/internal/source/gitlab/client.go +++ b/internal/source/gitlab/client.go @@ -1,10 +1,10 @@ package gitlab -import "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" +import "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" // Client interace represents a client capable of fetching a virtual domain // from an external API type Client interface { // GetVirtualDomain retrieves a virtual domain from an external API - GetVirtualDomain(host string) (*domain.VirtualDomain, error) + GetVirtualDomain(host string) (*api.VirtualDomain, error) } diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index 43cc84bfe..b6a7b059b 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -12,7 +12,7 @@ import ( "gitlab.com/gitlab-org/labkit/log" "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) // Client is a HTTP client to access Pages internal API @@ -52,7 +52,7 @@ func NewFromConfig(config Config) *Client { } // GetVirtualDomain returns VirtualDomain configuration for the given host -func (gc *Client) GetVirtualDomain(host string) (*domain.VirtualDomain, error) { +func (gc *Client) GetVirtualDomain(host string) (*api.VirtualDomain, error) { params := map[string]string{"host": host} resp, err := gc.get("/api/v4/internal/pages", params) @@ -61,7 +61,7 @@ func (gc *Client) GetVirtualDomain(host string) (*domain.VirtualDomain, error) { } defer resp.Body.Close() - var domain domain.VirtualDomain + var domain api.VirtualDomain err = json.NewDecoder(resp.Body).Decode(&domain) if err != nil { return nil, err diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go index 48dbf5494..0e24fe7f2 100644 --- a/internal/source/gitlab/client/client_stub.go +++ b/internal/source/gitlab/client/client_stub.go @@ -4,7 +4,7 @@ import ( "encoding/json" "os" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) // StubClient is a stubbed client used for testing @@ -13,7 +13,7 @@ type StubClient struct { } // GetVirtualDomain reads a test fixture and unmarshalls it -func (m *StubClient) GetVirtualDomain(host string) (domain *domain.VirtualDomain, err error) { +func (m *StubClient) GetVirtualDomain(host string) (domain *api.VirtualDomain, err error) { f, err := os.Open(m.file) defer f.Close() if err != nil { -- GitLab From 84812ac14c428b7af6d6a185bc1894b7ea47dc45 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 27 Nov 2019 14:02:21 +0100 Subject: [PATCH 23/39] Add acceptance tests for new domains source --- acceptance_test.go | 24 +++++++++++++++++++ helpers_test.go | 19 +++++++++++++++ internal/serving/disk/reader.go | 2 +- internal/source/domains.go | 1 + internal/source/gitlab/gitlab.go | 14 ++++------- shared/lookups/new-source.test.io.json | 1 + .../new-source.test.io/public/index.html | 1 + 7 files changed, 51 insertions(+), 11 deletions(-) create mode 100644 shared/lookups/new-source.test.io.json create mode 100644 shared/pages/group/new-source.test.io/public/index.html diff --git a/acceptance_test.go b/acceptance_test.go index fe598b0c7..3ed692540 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -16,6 +16,7 @@ import ( "time" "github.com/namsral/flag" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1531,3 +1532,26 @@ func TestApiSecretKeyFlagIsSupported(t *testing.T) { teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "-api-secret-key", "/path/to/secret.key") defer teardown() } + +func TestGitlabDomainsSource(t *testing.T) { + skipUnlessEnabled(t) + + source := NewGitlabDomainsSourceStub(t) + defer source.Close() + + sourceArgs := []string{ + "-gitlab-server", source.URL, + // "-api-secret-key", "/path/to/secret.key", + } + teardown := RunPagesProcess(t, *pagesBinary, listeners, "", sourceArgs...) + defer teardown() + + response, err := GetPageFromListener(t, httpListener, "new-source.test.io", "/my/pages/project/") + require.NoError(t, err) + + defer response.Body.Close() + body, _ := ioutil.ReadAll(response.Body) + + assert.Equal(t, http.StatusOK, response.StatusCode) + assert.Equal(t, "New Pages GitLab Source PoC OK\n", string(body)) +} diff --git a/helpers_test.go b/helpers_test.go index ad7c65f1c..a61aaa277 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -5,9 +5,11 @@ import ( "crypto/tls" "crypto/x509" "fmt" + "io" "io/ioutil" "net" "net/http" + "net/http/httptest" "os" "os/exec" "strings" @@ -375,3 +377,20 @@ func waitForRoundtrips(t *testing.T, listeners []ListenSpec, timeout time.Durati require.Equal(t, len(listeners), nListening, "all listeners must be accepting TCP connections") } + +func NewGitlabDomainsSourceStub(t *testing.T) *httptest.Server { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + domain := r.URL.Query().Get("host") + + fixture, err := os.Open("shared/lookups/" + domain + ".json") + defer fixture.Close() + require.NoError(t, err) + + _, err = io.Copy(w, fixture) + require.NoError(t, err) + + t.Logf("GitLab domain %s source stub served lookup", domain) + }) + + return httptest.NewServer(handler) +} diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index b52b5cffe..fb2201a11 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -27,7 +27,7 @@ func (reader *Reader) tryFile(h serving.Handler) error { if locationError, _ := err.(*locationDirectoryError); locationError != nil { if endsWithSlash(urlPath) { fullPath, err = reader.resolvePath(h.LookupPath.Path, h.SubPath, "index.html") - } else { + } else { // TODO why are we doing that? In tests it redirects to HTTPS. // Concat Host with URL.Path redirectPath := "//" + host + "/" redirectPath += strings.TrimPrefix(urlPath, "/") diff --git a/internal/source/domains.go b/internal/source/domains.go index 1db285a48..a69387e0f 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -12,6 +12,7 @@ var newSourceDomains = []string{ "pages-project-poc.gitlab.io", "pages-namespace-poc.gitlab.io", "pages-custom-poc.grzegorz.co", + "new-source.test.io", // used in acceptance tests } var brokenSourceDomain = "pages-broken-poc.gitlab.io" diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 18da0c0bb..eeabd028d 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -2,7 +2,6 @@ package gitlab import ( "errors" - "net" "net/http" "strings" @@ -45,12 +44,7 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) { // Resolve is supposed to get the serving lookup path based on the request from // the GitLab source func (g *Gitlab) Resolve(r *http.Request) (*serving.LookupPath, string, error) { - domain, _, err := net.SplitHostPort(r.Host) - if err != nil { - return nil, "", err - } - - response, err := g.client.GetVirtualDomain(domain) + response, err := g.client.GetVirtualDomain(r.Host) if err != nil { return nil, "", err } @@ -59,14 +53,14 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.LookupPath, string, error) { if strings.Contains(r.URL.Path, lookup.Prefix) { lookupPath := &serving.LookupPath{ Location: lookup.Prefix, - Path: lookup.Source.Path, - IsNamespaceProject: false, // TODO is this still relevant? it is not served in the API + Path: strings.TrimPrefix(lookup.Source.Path, "/"), // TODO test + IsNamespaceProject: false, // TODO is this still relevant? it is not served in the API IsHTTPSOnly: lookup.HTTPSOnly, HasAccessControl: lookup.AccessControl, ProjectID: uint64(lookup.ProjectID), } - requestPath := strings.Replace(r.URL.Path, lookup.Prefix, "", 1) + requestPath := strings.TrimPrefix(r.URL.Path, lookup.Prefix) return lookupPath, requestPath, nil } diff --git a/shared/lookups/new-source.test.io.json b/shared/lookups/new-source.test.io.json new file mode 100644 index 000000000..5dfbd9ba4 --- /dev/null +++ b/shared/lookups/new-source.test.io.json @@ -0,0 +1 @@ +{"certificate":"","key":"","lookup_paths":[{"project_id":123,"access_control":false,"https_only":false,"prefix":"/my/pages/project","source":{"type":"file","path":"/group/new-source.test.io/public"}}]} diff --git a/shared/pages/group/new-source.test.io/public/index.html b/shared/pages/group/new-source.test.io/public/index.html new file mode 100644 index 000000000..e6e1e58e2 --- /dev/null +++ b/shared/pages/group/new-source.test.io/public/index.html @@ -0,0 +1 @@ +New Pages GitLab Source PoC OK -- GitLab From 21cf7b066bb33c374a4044e24107fbcf36c71ff6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 28 Nov 2019 11:18:13 +0100 Subject: [PATCH 24/39] Add case for a request resolution for a namespace project --- .../client/testdata/test.gitlab.io.json | 2 +- internal/source/gitlab/gitlab.go | 4 +-- internal/source/gitlab/gitlab_test.go | 29 +++++++++++++++---- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/internal/source/gitlab/client/testdata/test.gitlab.io.json b/internal/source/gitlab/client/testdata/test.gitlab.io.json index dfe65ad0d..a13c2c853 100644 --- a/internal/source/gitlab/client/testdata/test.gitlab.io.json +++ b/internal/source/gitlab/client/testdata/test.gitlab.io.json @@ -1 +1 @@ -{"certificate":"some--cert","key":"some--key","lookup_paths":[{"project_id":123,"access_control":false,"https_only":true,"prefix":"/my/pages/project","source":{"type":"file","path":"/some/path/to/project/"}}]} \ No newline at end of file +{"certificate":"some--cert","key":"some--key","lookup_paths":[{"project_id":123,"access_control":false,"https_only":true,"prefix":"/my/pages/project","source":{"type":"file","path":"/some/path/to/project/"}},{"project_id":124,"access_control":false,"https_only":true,"prefix":"/my/second-project","source":{"type":"file","path":"/some/path/to/project-2/"}},{"project_id":125,"access_control":false,"https_only":true,"prefix":"/","source":{"type":"file","path":"/some/path/to/project-3/"}}]} diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index eeabd028d..6a55244bd 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -53,8 +53,8 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.LookupPath, string, error) { if strings.Contains(r.URL.Path, lookup.Prefix) { lookupPath := &serving.LookupPath{ Location: lookup.Prefix, - Path: strings.TrimPrefix(lookup.Source.Path, "/"), // TODO test - IsNamespaceProject: false, // TODO is this still relevant? it is not served in the API + Path: strings.TrimPrefix(lookup.Source.Path, "/"), + IsNamespaceProject: (lookup.Prefix == "/" && len(response.LookupPaths) > 1), IsHTTPSOnly: lookup.HTTPSOnly, HasAccessControl: lookup.AccessControl, ProjectID: uint64(lookup.ProjectID), diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go index 74cbcb794..21945e747 100644 --- a/internal/source/gitlab/gitlab_test.go +++ b/internal/source/gitlab/gitlab_test.go @@ -24,12 +24,29 @@ func TestGetDomain(t *testing.T) { func TestResolve(t *testing.T) { client := client.NewStubClient("client/testdata/test.gitlab.io.json") source := Gitlab{client: client, cache: cache.New()} - target := "https://test.gitlab.io:443/my/pages/project/path/index.html" - request := httptest.NewRequest("GET", target, nil) - lookup, subpath, err := source.Resolve(request) - require.NoError(t, err) + t.Run("when requesting a nested group project", func(t *testing.T) { + target := "https://test.gitlab.io:443/my/pages/project/path/index.html" + request := httptest.NewRequest("GET", target, nil) + + lookup, subpath, err := source.Resolve(request) + require.NoError(t, err) + + assert.Equal(t, "/path/index.html", subpath) + assert.Equal(t, "/my/pages/project", lookup.Location) + assert.False(t, lookup.IsNamespaceProject) + }) + + t.Run("when request a nested group project", func(t *testing.T) { + target := "https://test.gitlab.io:443/path/to/index.html" + request := httptest.NewRequest("GET", target, nil) + + lookup, subpath, err := source.Resolve(request) + require.NoError(t, err) - assert.Equal(t, "/my/pages/project", lookup.Location) - assert.Equal(t, "/path/index.html", subpath) + assert.Equal(t, "path/to/index.html", subpath) + assert.Equal(t, "some/path/to/project-3/", lookup.Path) + assert.Equal(t, "/", lookup.Location) + assert.True(t, lookup.IsNamespaceProject) + }) } -- GitLab From 27ae232f8a3fcca7dadbc809c276e1c76547700c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 28 Nov 2019 11:36:03 +0100 Subject: [PATCH 25/39] Make project location / path / prefix less confusing --- internal/serving/lookup_path.go | 3 +-- internal/source/disk/custom.go | 3 +-- internal/source/disk/group.go | 5 ++--- internal/source/gitlab/gitlab.go | 2 +- internal/source/gitlab/gitlab_test.go | 4 ++-- 5 files changed, 7 insertions(+), 10 deletions(-) diff --git a/internal/serving/lookup_path.go b/internal/serving/lookup_path.go index cace3f435..a31a71653 100644 --- a/internal/serving/lookup_path.go +++ b/internal/serving/lookup_path.go @@ -1,9 +1,8 @@ package serving // LookupPath holds a domain project configuration needed to handle a request -// TODO We might want to swap Location and Path names. type LookupPath struct { - Location string // Location is a path to a project requested in a request + Prefix string // Project prefix, for example, /my/project in group.gitlab.io/my/project/index.html Path string // Path is an internal and serving-specific location of a document IsNamespaceProject bool IsHTTPSOnly bool diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go index 8a080f208..cc4f3f4c0 100644 --- a/internal/source/disk/custom.go +++ b/internal/source/disk/custom.go @@ -12,10 +12,9 @@ type customProjectResolver struct { path string } -// TODO tests func (p *customProjectResolver) Resolve(r *http.Request) (*serving.LookupPath, string, error) { lookupPath := &serving.LookupPath{ - Location: "/", + Prefix: "/", Path: p.path, IsNamespaceProject: false, IsHTTPSOnly: p.config.HTTPSOnly, diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go index 59aedc739..7094e7a21 100644 --- a/internal/source/disk/group.go +++ b/internal/source/disk/group.go @@ -68,7 +68,6 @@ func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*projectConfig, st // return the group project if it exists. if host := host.FromRequest(r); host != "" { if groupProject := g.projects[host]; groupProject != nil { - // TODOHERE: the location here should be "/", so we return "" return groupProject, "/", host, strings.Join(split[1:], "/") } } @@ -79,14 +78,14 @@ func (g *Group) getProjectConfigWithSubpath(r *http.Request) (*projectConfig, st // Resolve tries to find project and its config recursively for a given request // to a group domain func (g *Group) Resolve(r *http.Request) (*serving.LookupPath, string, error) { - projectConfig, location, projectPath, subPath := g.getProjectConfigWithSubpath(r) + projectConfig, prefix, projectPath, subPath := g.getProjectConfigWithSubpath(r) if projectConfig == nil { return nil, "", nil // it is not an error when project does not exist } lookupPath := &serving.LookupPath{ - Location: location, + Prefix: prefix, Path: filepath.Join(g.name, projectPath, "public"), IsNamespaceProject: projectConfig.NamespaceProject, IsHTTPSOnly: projectConfig.HTTPSOnly, diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 6a55244bd..577bde4c2 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -52,7 +52,7 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.LookupPath, string, error) { for _, lookup := range response.LookupPaths { if strings.Contains(r.URL.Path, lookup.Prefix) { lookupPath := &serving.LookupPath{ - Location: lookup.Prefix, + Prefix: lookup.Prefix, Path: strings.TrimPrefix(lookup.Source.Path, "/"), IsNamespaceProject: (lookup.Prefix == "/" && len(response.LookupPaths) > 1), IsHTTPSOnly: lookup.HTTPSOnly, diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go index 21945e747..c1ce95c39 100644 --- a/internal/source/gitlab/gitlab_test.go +++ b/internal/source/gitlab/gitlab_test.go @@ -32,8 +32,8 @@ func TestResolve(t *testing.T) { lookup, subpath, err := source.Resolve(request) require.NoError(t, err) + assert.Equal(t, "/my/pages/project", lookup.Prefix) assert.Equal(t, "/path/index.html", subpath) - assert.Equal(t, "/my/pages/project", lookup.Location) assert.False(t, lookup.IsNamespaceProject) }) @@ -44,9 +44,9 @@ func TestResolve(t *testing.T) { lookup, subpath, err := source.Resolve(request) require.NoError(t, err) + assert.Equal(t, "/", lookup.Prefix) assert.Equal(t, "path/to/index.html", subpath) assert.Equal(t, "some/path/to/project-3/", lookup.Path) - assert.Equal(t, "/", lookup.Location) assert.True(t, lookup.IsNamespaceProject) }) } -- GitLab From 4ac210acda6408e21a0772c9bb4819f5fb2f4bb6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 28 Nov 2019 11:36:15 +0100 Subject: [PATCH 26/39] Simplify acceptance tests for gitlab source a bit --- acceptance_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index 4a50553dd..ed114c8a0 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1533,11 +1533,7 @@ func TestGitlabDomainsSource(t *testing.T) { source := NewGitlabDomainsSourceStub(t) defer source.Close() - sourceArgs := []string{ - "-gitlab-server", source.URL, - // "-api-secret-key", "/path/to/secret.key", - } - teardown := RunPagesProcess(t, *pagesBinary, listeners, "", sourceArgs...) + teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "-gitlab-server", source.URL) defer teardown() response, err := GetPageFromListener(t, httpListener, "new-source.test.io", "/my/pages/project/") -- GitLab From 18e0901b298d922ebda8295b675ac1df0f4bf77f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 28 Nov 2019 11:43:52 +0100 Subject: [PATCH 27/39] Rename new source test fixture to avoid using existing domain --- acceptance_test.go | 4 ++-- internal/source/domains.go | 2 +- ...new-source.test.io.json => new-source-test.gitlab.io.json} | 2 +- .../pages/group/new-source-test.gitlab.io/public/index.html | 1 + shared/pages/group/new-source.test.io/public/index.html | 1 - 5 files changed, 5 insertions(+), 5 deletions(-) rename shared/lookups/{new-source.test.io.json => new-source-test.gitlab.io.json} (61%) create mode 100644 shared/pages/group/new-source-test.gitlab.io/public/index.html delete mode 100644 shared/pages/group/new-source.test.io/public/index.html diff --git a/acceptance_test.go b/acceptance_test.go index ed114c8a0..5faa0e38c 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1536,12 +1536,12 @@ func TestGitlabDomainsSource(t *testing.T) { teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "-gitlab-server", source.URL) defer teardown() - response, err := GetPageFromListener(t, httpListener, "new-source.test.io", "/my/pages/project/") + response, err := GetPageFromListener(t, httpListener, "new-source-test.gitlab.io", "/my/pages/project/") require.NoError(t, err) defer response.Body.Close() body, _ := ioutil.ReadAll(response.Body) assert.Equal(t, http.StatusOK, response.StatusCode) - assert.Equal(t, "New Pages GitLab Source PoC OK\n", string(body)) + assert.Equal(t, "New Pages GitLab Source TEST OK\n", string(body)) } diff --git a/internal/source/domains.go b/internal/source/domains.go index a69387e0f..1b847cc91 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -11,8 +11,8 @@ import ( var newSourceDomains = []string{ "pages-project-poc.gitlab.io", "pages-namespace-poc.gitlab.io", + "new-source-test.gitlab.io", // used also in acceptance tests "pages-custom-poc.grzegorz.co", - "new-source.test.io", // used in acceptance tests } var brokenSourceDomain = "pages-broken-poc.gitlab.io" diff --git a/shared/lookups/new-source.test.io.json b/shared/lookups/new-source-test.gitlab.io.json similarity index 61% rename from shared/lookups/new-source.test.io.json rename to shared/lookups/new-source-test.gitlab.io.json index 5dfbd9ba4..8ff94e786 100644 --- a/shared/lookups/new-source.test.io.json +++ b/shared/lookups/new-source-test.gitlab.io.json @@ -1 +1 @@ -{"certificate":"","key":"","lookup_paths":[{"project_id":123,"access_control":false,"https_only":false,"prefix":"/my/pages/project","source":{"type":"file","path":"/group/new-source.test.io/public"}}]} +{"certificate":"","key":"","lookup_paths":[{"project_id":123,"access_control":false,"https_only":false,"prefix":"/my/pages/project","source":{"type":"file","path":"/group/new-source-test.gitlab.io/public"}}]} diff --git a/shared/pages/group/new-source-test.gitlab.io/public/index.html b/shared/pages/group/new-source-test.gitlab.io/public/index.html new file mode 100644 index 000000000..00e11d669 --- /dev/null +++ b/shared/pages/group/new-source-test.gitlab.io/public/index.html @@ -0,0 +1 @@ +New Pages GitLab Source TEST OK diff --git a/shared/pages/group/new-source.test.io/public/index.html b/shared/pages/group/new-source.test.io/public/index.html deleted file mode 100644 index e6e1e58e2..000000000 --- a/shared/pages/group/new-source.test.io/public/index.html +++ /dev/null @@ -1 +0,0 @@ -New Pages GitLab Source PoC OK -- GitLab From 738d560e79d3488277fa1355a5df4484c32a251c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 28 Nov 2019 16:13:00 +0100 Subject: [PATCH 28/39] Make fetching domains from domain source more resilient --- internal/source/gitlab/client/client.go | 4 +++- internal/source/gitlab/client/client_stub.go | 5 +++-- internal/source/gitlab/gitlab.go | 4 ++++ internal/source/gitlab/gitlab_test.go | 22 +++++++++++++++----- 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index a612f751c..6b4750477 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -29,6 +29,8 @@ var ( errNotFound = errors.New("Not Found") ) +var tokenTimeout = 30 * time.Second + // NewClient initializes and returns new Client baseUrl is // appConfig.GitLabServer secretKey is appConfig.GitLabAPISecretKey func NewClient(baseURL string, secretKey []byte) *Client { @@ -137,7 +139,7 @@ func (gc *Client) request(method string, endpoint *url.URL) (*http.Request, erro func (gc *Client) token() (string, error) { claims := jwt.StandardClaims{ Issuer: "gitlab-pages", - ExpiresAt: time.Now().Add(5 * time.Second).Unix(), + ExpiresAt: time.Now().Add(tokenTimeout).Unix(), } token, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(gc.secretKey) diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go index 0e24fe7f2..809d60d3a 100644 --- a/internal/source/gitlab/client/client_stub.go +++ b/internal/source/gitlab/client/client_stub.go @@ -13,16 +13,17 @@ type StubClient struct { } // GetVirtualDomain reads a test fixture and unmarshalls it -func (m *StubClient) GetVirtualDomain(host string) (domain *api.VirtualDomain, err error) { +func (m *StubClient) GetVirtualDomain(host string) (*api.VirtualDomain, error) { f, err := os.Open(m.file) defer f.Close() if err != nil { return nil, err } + var domain api.VirtualDomain err = json.NewDecoder(f).Decode(&domain) - return domain, err + return &domain, err } // NewStubClient return a stubbed client diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 577bde4c2..4abfe2250 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -31,6 +31,10 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) { return nil, err } + if response == nil { + return nil, errors.New("could not fetch a domain information") + } + domain := domain.Domain{ Name: name, CertificateCert: response.Certificate, diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go index c1ce95c39..8dd3cc5f5 100644 --- a/internal/source/gitlab/gitlab_test.go +++ b/internal/source/gitlab/gitlab_test.go @@ -12,13 +12,25 @@ import ( ) func TestGetDomain(t *testing.T) { - client := client.NewStubClient("client/testdata/test.gitlab.io.json") - source := Gitlab{client: client, cache: cache.New()} + t.Run("when the response if correct", func(t *testing.T) { + client := client.NewStubClient("client/testdata/test.gitlab.io.json") + source := Gitlab{client: client, cache: cache.New()} + + domain, err := source.GetDomain("test.gitlab.io") + require.NoError(t, err) - domain, err := source.GetDomain("test.gitlab.io") - require.NoError(t, err) + assert.Equal(t, "test.gitlab.io", domain.Name) + }) + + t.Run("when the response is not valid", func(t *testing.T) { + client := client.NewStubClient("/dev/null") + source := Gitlab{client: client, cache: cache.New()} - assert.Equal(t, "test.gitlab.io", domain.Name) + domain, err := source.GetDomain("test.gitlab.io") + + assert.NotNil(t, err) + assert.Nil(t, domain) + }) } func TestResolve(t *testing.T) { -- GitLab From bd16d63cd84f28c3992f45eef9b57c93b3cc572b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 28 Nov 2019 16:23:57 +0100 Subject: [PATCH 29/39] Sanitize pages URL before calculating lookup path --- internal/source/gitlab/gitlab.go | 9 ++++++--- internal/source/gitlab/gitlab_test.go | 13 ++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 4abfe2250..7df07f9c9 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -3,6 +3,7 @@ package gitlab import ( "errors" "net/http" + "path" "strings" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" @@ -54,7 +55,9 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.LookupPath, string, error) { } for _, lookup := range response.LookupPaths { - if strings.Contains(r.URL.Path, lookup.Prefix) { + urlPath := path.Clean(r.URL.Path) + + if strings.HasPrefix(urlPath, lookup.Prefix) { lookupPath := &serving.LookupPath{ Prefix: lookup.Prefix, Path: strings.TrimPrefix(lookup.Source.Path, "/"), @@ -64,9 +67,9 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.LookupPath, string, error) { ProjectID: uint64(lookup.ProjectID), } - requestPath := strings.TrimPrefix(r.URL.Path, lookup.Prefix) + requestPath := strings.TrimPrefix(urlPath, lookup.Prefix) - return lookupPath, requestPath, nil + return lookupPath, strings.TrimPrefix(requestPath, "/"), nil } } diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go index 8dd3cc5f5..affb1694c 100644 --- a/internal/source/gitlab/gitlab_test.go +++ b/internal/source/gitlab/gitlab_test.go @@ -45,7 +45,7 @@ func TestResolve(t *testing.T) { require.NoError(t, err) assert.Equal(t, "/my/pages/project", lookup.Prefix) - assert.Equal(t, "/path/index.html", subpath) + assert.Equal(t, "path/index.html", subpath) assert.False(t, lookup.IsNamespaceProject) }) @@ -61,4 +61,15 @@ func TestResolve(t *testing.T) { assert.Equal(t, "some/path/to/project-3/", lookup.Path) assert.True(t, lookup.IsNamespaceProject) }) + + t.Run("when request path has not been sanitized", func(t *testing.T) { + target := "https://test.gitlab.io:443/something/../something/../my/pages/project/index.html" + request := httptest.NewRequest("GET", target, nil) + + lookup, subpath, err := source.Resolve(request) + require.NoError(t, err) + + assert.Equal(t, "/my/pages/project", lookup.Prefix) + assert.Equal(t, "index.html", subpath) + }) } -- GitLab From 84a6b46d36c5d83bdc41dda596d7f6bcfe4d67f3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 28 Nov 2019 17:03:23 +0100 Subject: [PATCH 30/39] Use ENV variables to obtain a list of test new source domains --- acceptance_test.go | 7 +++++-- helpers_test.go | 4 ++++ internal/source/domains.go | 22 +++++++++++++++------- internal/source/domains_test.go | 5 ++++- 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index 5faa0e38c..6f2e5fb45 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -435,7 +435,9 @@ func TestPageNotAvailableIfNotLoaded(t *testing.T) { func TestPageNotAvailableInDomainSource(t *testing.T) { skipUnlessEnabled(t) - teardown := RunPagesProcessWithoutWait(t, *pagesBinary, listeners, "", "-pages-root=shared/invalid-pages") + + brokenDomain := "GITLAB_NEW_SOURCE_BROKEN_DOMAIN=pages-broken-poc.gitlab.io" + teardown := RunPagesProcessWithEnvs(t, false, *pagesBinary, listeners, "", []string{brokenDomain}, "-pages-root=shared/invalid-pages") defer teardown() waitForRoundtrips(t, listeners, 5*time.Second) @@ -1533,7 +1535,8 @@ func TestGitlabDomainsSource(t *testing.T) { source := NewGitlabDomainsSourceStub(t) defer source.Close() - teardown := RunPagesProcess(t, *pagesBinary, listeners, "", "-gitlab-server", source.URL) + newSourceDomains := "GITLAB_NEW_SOURCE_DOMAINS=new-source-test.gitlab.io,other-test.gitlab.io" + teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{newSourceDomains}, "-gitlab-server", source.URL) defer teardown() response, err := GetPageFromListener(t, httpListener, "new-source-test.gitlab.io", "/my/pages/project/") diff --git a/helpers_test.go b/helpers_test.go index a61aaa277..b13fb18fe 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -147,6 +147,10 @@ func RunPagesProcessWithSSLCertFile(t *testing.T, pagesPath string, listeners [] return runPagesProcess(t, true, pagesPath, listeners, promPort, []string{"SSL_CERT_FILE=" + sslCertFile}, extraArgs...) } +func RunPagesProcessWithEnvs(t *testing.T, wait bool, pagesPath string, listeners []ListenSpec, promPort string, envs []string, extraArgs ...string) (teardown func()) { + return runPagesProcess(t, wait, pagesPath, listeners, promPort, envs, extraArgs...) +} + func RunPagesProcessWithAuth(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string) (teardown func()) { return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, "-auth-client-id=1", "-auth-client-secret=1", diff --git a/internal/source/domains.go b/internal/source/domains.go index 1b847cc91..44c1f30a0 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -2,20 +2,28 @@ package source import ( "errors" + "os" + "strings" "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" ) -var newSourceDomains = []string{ - "pages-project-poc.gitlab.io", - "pages-namespace-poc.gitlab.io", - "new-source-test.gitlab.io", // used also in acceptance tests - "pages-custom-poc.grzegorz.co", -} +var newSourceDomains []string +var brokenSourceDomain string + +func init() { + testDomains := os.Getenv("GITLAB_NEW_SOURCE_DOMAINS") + if testDomains != "" { + newSourceDomains = strings.Split(testDomains, ",") + } -var brokenSourceDomain = "pages-broken-poc.gitlab.io" + brokenDomain := os.Getenv("GITLAB_NEW_SOURCE_BROKEN_DOMAIN") + if brokenDomain != "" { + brokenSourceDomain = brokenDomain + } +} // 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 diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 5125f4620..6854c3590 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -11,9 +11,12 @@ import ( ) func TestHasDomain(t *testing.T) { - testDomain := newSourceDomains[0] + newSourceDomains = []string{"new-source-test.gitlab.io"} + brokenSourceDomain = "pages-broken-poc.gitlab.io" t.Run("when requesting a test domain", func(t *testing.T) { + testDomain := newSourceDomains[0] + newSource := NewMockSource() newSource.On("GetDomain", testDomain). Return(&domain.Domain{Name: testDomain}, nil). -- GitLab From 570d6e165d79a41597b8574026ab3d27629553c9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 11:23:17 +0100 Subject: [PATCH 31/39] Remove unnecessary change from disk/map_test.go --- internal/source/disk/map_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go index 23cac3ddc..c15f29c61 100644 --- a/internal/source/disk/map_test.go +++ b/internal/source/disk/map_test.go @@ -140,7 +140,7 @@ func writeRandomTimestamp(t *testing.T) { n, _ := rand.Read(b) require.True(t, n > 0, "read some random bytes") - temp, err := ioutil.TempFile(".", "TestIsWatch") + temp, err := ioutil.TempFile(".", "TestWatch") require.NoError(t, err) _, err = temp.Write(b) require.NoError(t, err, "write to tempfile") -- GitLab From 9e4e8df6142dceb64c45c6c1d5b7de65a8f3a0e8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 11:27:05 +0100 Subject: [PATCH 32/39] Extract GitLab API connection timeout to a variable --- internal/source/gitlab/client/client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index 6b4750477..9d8e4f48e 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -30,6 +30,7 @@ var ( ) var tokenTimeout = 30 * time.Second +var connectionTimeout = 10 * time.Second // NewClient initializes and returns new Client baseUrl is // appConfig.GitLabServer secretKey is appConfig.GitLabAPISecretKey @@ -43,7 +44,7 @@ func NewClient(baseURL string, secretKey []byte) *Client { secretKey: secretKey, baseURL: url, httpClient: &http.Client{ - Timeout: 5 * time.Second, + Timeout: connectionTimeout, Transport: httptransport.Transport, }, } -- GitLab From e02ad7970ec061391fe3f34736f851fa947f8922 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 11:46:17 +0100 Subject: [PATCH 33/39] Simplify passing URL params in GitLab client code --- internal/source/gitlab/client/client.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index 9d8e4f48e..e60db068a 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -57,7 +57,8 @@ func NewFromConfig(config Config) *Client { // GetVirtualDomain returns VirtualDomain configuration for the given host func (gc *Client) GetVirtualDomain(host string) (*api.VirtualDomain, error) { - params := map[string]string{"host": host} + params := url.Values{} + params.Set("host", host) resp, err := gc.get("/api/v4/internal/pages", params) if resp != nil { @@ -77,7 +78,7 @@ func (gc *Client) GetVirtualDomain(host string) (*api.VirtualDomain, error) { return &domain, nil } -func (gc *Client) get(path string, params map[string]string) (*http.Response, error) { +func (gc *Client) get(path string, params url.Values) (*http.Response, error) { endpoint, err := gc.endpoint(path, params) if err != nil { return nil, err @@ -107,17 +108,13 @@ func (gc *Client) get(path string, params map[string]string) (*http.Response, er } } -func (gc *Client) endpoint(path string, params map[string]string) (*url.URL, error) { +func (gc *Client) endpoint(path string, params url.Values) (*url.URL, error) { endpoint, err := gc.baseURL.Parse(path) if err != nil { return nil, err } - values := url.Values{} - for key, value := range params { - values.Add(key, value) - } - endpoint.RawQuery = values.Encode() + endpoint.RawQuery = params.Encode() return endpoint, nil } -- GitLab From 0aa30f1feca22cb104cb136f60b0efcba9aac207 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 11:49:54 +0100 Subject: [PATCH 34/39] Link a few bug and technical debt issues in code --- internal/serving/disk/reader.go | 5 ++++- internal/serving/lookup_path.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index fb2201a11..ce4f1d8b6 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -27,7 +27,10 @@ func (reader *Reader) tryFile(h serving.Handler) error { if locationError, _ := err.(*locationDirectoryError); locationError != nil { if endsWithSlash(urlPath) { fullPath, err = reader.resolvePath(h.LookupPath.Path, h.SubPath, "index.html") - } else { // TODO why are we doing that? In tests it redirects to HTTPS. + } else { + // TODO why are we doing that? In tests it redirects to HTTPS. This seems wrong, + // issue about this: https://gitlab.com/gitlab-org/gitlab-pages/issues/273 + // Concat Host with URL.Path redirectPath := "//" + host + "/" redirectPath += strings.TrimPrefix(urlPath, "/") diff --git a/internal/serving/lookup_path.go b/internal/serving/lookup_path.go index a31a71653..4360358be 100644 --- a/internal/serving/lookup_path.go +++ b/internal/serving/lookup_path.go @@ -4,7 +4,7 @@ package serving type LookupPath struct { Prefix string // Project prefix, for example, /my/project in group.gitlab.io/my/project/index.html Path string // Path is an internal and serving-specific location of a document - IsNamespaceProject bool + IsNamespaceProject bool // IsNamespaceProject is DEPRECATED, see https://gitlab.com/gitlab-org/gitlab-pages/issues/272 IsHTTPSOnly bool HasAccessControl bool ProjectID uint64 -- GitLab From 4e911e51a38d87de26ef1747bdb33fdf22e17531 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 11:56:19 +0100 Subject: [PATCH 35/39] Ensure that a GitLab API response is never nil --- internal/source/gitlab/client/client.go | 5 ++++- internal/source/gitlab/gitlab.go | 4 ---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index e60db068a..c2424c8b7 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -55,7 +55,8 @@ func NewFromConfig(config Config) *Client { return NewClient(config.GitlabServerURL(), config.GitlabAPISecret()) } -// GetVirtualDomain returns VirtualDomain configuration for the given host +// GetVirtualDomain returns VirtualDomain configuration for the given host. It +// returns an error if non-nil `*api.VirtualDomain` can not be retuned. func (gc *Client) GetVirtualDomain(host string) (*api.VirtualDomain, error) { params := url.Values{} params.Set("host", host) @@ -63,6 +64,8 @@ func (gc *Client) GetVirtualDomain(host string) (*api.VirtualDomain, error) { resp, err := gc.get("/api/v4/internal/pages", params) if resp != nil { defer resp.Body.Close() + } else { + return nil, errors.New("empty response returned") } if err != nil { diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 7df07f9c9..7cb88e42d 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -32,10 +32,6 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) { return nil, err } - if response == nil { - return nil, errors.New("could not fetch a domain information") - } - domain := domain.Domain{ Name: name, CertificateCert: response.Certificate, -- GitLab From d8535b5c20a0ee10652bb8f7c11a11ecd3a34aeb Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 11:58:22 +0100 Subject: [PATCH 36/39] Add comment about making client settings configurable --- internal/source/gitlab/client/client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go index c2424c8b7..6c9327dc2 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -29,6 +29,7 @@ var ( errNotFound = errors.New("Not Found") ) +// TODO make these values configurable https://gitlab.com/gitlab-org/gitlab-pages/issues/274 var tokenTimeout = 30 * time.Second var connectionTimeout = 10 * time.Second -- GitLab From 1d40c784c28bc4fd4510f843354224655ce7ec6e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 15:30:31 +0100 Subject: [PATCH 37/39] Pretty print new JSON test data / fixtures --- .../client/testdata/test.gitlab.io.json | 37 ++++++++++++++++++- shared/lookups/new-source-test.gitlab.io.json | 17 ++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/internal/source/gitlab/client/testdata/test.gitlab.io.json b/internal/source/gitlab/client/testdata/test.gitlab.io.json index a13c2c853..923c73440 100644 --- a/internal/source/gitlab/client/testdata/test.gitlab.io.json +++ b/internal/source/gitlab/client/testdata/test.gitlab.io.json @@ -1 +1,36 @@ -{"certificate":"some--cert","key":"some--key","lookup_paths":[{"project_id":123,"access_control":false,"https_only":true,"prefix":"/my/pages/project","source":{"type":"file","path":"/some/path/to/project/"}},{"project_id":124,"access_control":false,"https_only":true,"prefix":"/my/second-project","source":{"type":"file","path":"/some/path/to/project-2/"}},{"project_id":125,"access_control":false,"https_only":true,"prefix":"/","source":{"type":"file","path":"/some/path/to/project-3/"}}]} +{ + "certificate": "some--cert", + "key": "some--key", + "lookup_paths": [ + { + "access_control": false, + "https_only": true, + "prefix": "/my/pages/project", + "project_id": 123, + "source": { + "path": "/some/path/to/project/", + "type": "file" + } + }, + { + "access_control": false, + "https_only": true, + "prefix": "/my/second-project", + "project_id": 124, + "source": { + "path": "/some/path/to/project-2/", + "type": "file" + } + }, + { + "access_control": false, + "https_only": true, + "prefix": "/", + "project_id": 125, + "source": { + "path": "/some/path/to/project-3/", + "type": "file" + } + } + ] +} diff --git a/shared/lookups/new-source-test.gitlab.io.json b/shared/lookups/new-source-test.gitlab.io.json index 8ff94e786..0332b6c2b 100644 --- a/shared/lookups/new-source-test.gitlab.io.json +++ b/shared/lookups/new-source-test.gitlab.io.json @@ -1 +1,16 @@ -{"certificate":"","key":"","lookup_paths":[{"project_id":123,"access_control":false,"https_only":false,"prefix":"/my/pages/project","source":{"type":"file","path":"/group/new-source-test.gitlab.io/public"}}]} +{ + "certificate": "", + "key": "", + "lookup_paths": [ + { + "access_control": false, + "https_only": false, + "prefix": "/my/pages/project", + "project_id": 123, + "source": { + "path": "/group/new-source-test.gitlab.io/public", + "type": "file" + } + } + ] +} -- GitLab From ffbe202a012f0366d4f2fb24eb1959c90e0f2ff5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 15:33:49 +0100 Subject: [PATCH 38/39] Fix invalid godoc comment for function in source/domains.go --- internal/source/domains.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/source/domains.go b/internal/source/domains.go index 44c1f30a0..f47884ef7 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -55,7 +55,7 @@ func (d *Domains) GetDomain(name string) (*domain.Domain, error) { return d.source(name).GetDomain(name) } -// Start starts the disk domain source. It is DEPRECATED, because we want to +// Read starts the disk domain source. It is DEPRECATED, because we want to // remove it entirely when disk source gets removed. func (d *Domains) Read(rootDomain string) { d.disk.Read(rootDomain) -- GitLab From 16e6e7e947fcc9235bdea9c72d0cbcc2dbd21bd0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 29 Nov 2019 15:34:10 +0100 Subject: [PATCH 39/39] Simplify gitlab client stub in tests --- internal/source/gitlab/client/client_stub.go | 11 +++-------- internal/source/gitlab/gitlab_test.go | 6 +++--- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go index 809d60d3a..6dc0af852 100644 --- a/internal/source/gitlab/client/client_stub.go +++ b/internal/source/gitlab/client/client_stub.go @@ -9,12 +9,12 @@ import ( // StubClient is a stubbed client used for testing type StubClient struct { - file string + File string } // GetVirtualDomain reads a test fixture and unmarshalls it -func (m *StubClient) GetVirtualDomain(host string) (*api.VirtualDomain, error) { - f, err := os.Open(m.file) +func (c StubClient) GetVirtualDomain(host string) (*api.VirtualDomain, error) { + f, err := os.Open(c.File) defer f.Close() if err != nil { return nil, err @@ -25,8 +25,3 @@ func (m *StubClient) GetVirtualDomain(host string) (*api.VirtualDomain, error) { return &domain, err } - -// NewStubClient return a stubbed client -func NewStubClient(fixture string) *StubClient { - return &StubClient{file: fixture} -} diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go index affb1694c..02751eea0 100644 --- a/internal/source/gitlab/gitlab_test.go +++ b/internal/source/gitlab/gitlab_test.go @@ -13,7 +13,7 @@ import ( func TestGetDomain(t *testing.T) { t.Run("when the response if correct", func(t *testing.T) { - client := client.NewStubClient("client/testdata/test.gitlab.io.json") + client := client.StubClient{File: "client/testdata/test.gitlab.io.json"} source := Gitlab{client: client, cache: cache.New()} domain, err := source.GetDomain("test.gitlab.io") @@ -23,7 +23,7 @@ func TestGetDomain(t *testing.T) { }) t.Run("when the response is not valid", func(t *testing.T) { - client := client.NewStubClient("/dev/null") + client := client.StubClient{File: "/dev/null"} source := Gitlab{client: client, cache: cache.New()} domain, err := source.GetDomain("test.gitlab.io") @@ -34,7 +34,7 @@ func TestGetDomain(t *testing.T) { } func TestResolve(t *testing.T) { - client := client.NewStubClient("client/testdata/test.gitlab.io.json") + client := client.StubClient{File: "client/testdata/test.gitlab.io.json"} source := Gitlab{client: client, cache: cache.New()} t.Run("when requesting a nested group project", func(t *testing.T) { -- GitLab