From 7a5f0ccd890ac9df2581126450e7278b141e6a57 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 15 Mar 2022 13:51:37 +0100 Subject: [PATCH 1/6] build: enable `-race` for `go test` Signed-off-by: Hidde Beydals --- Makefile | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 15a81b8f2..b19754584 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,7 @@ LIBGIT2_IMG ?= ghcr.io/fluxcd/golang-with-libgit2 LIBGIT2_TAG ?= libgit2-1.3.1 # Allows for defining additional Go test args, e.g. '-tags integration'. -GO_TEST_ARGS ?= +GO_TEST_ARGS ?= -race # Allows for defining additional Docker buildx arguments, # e.g. '--push'. @@ -15,7 +15,8 @@ BUILD_ARGS ?= # Architectures to build images for BUILD_PLATFORMS ?= linux/amd64,linux/arm64,linux/arm/v7 -# Go additional tag arguments, e.g. 'integration' +# Go additional tag arguments, e.g. 'integration', +# this is append to the tag arguments required for static builds GO_TAGS ?= # Produce CRDs that work back to Kubernetes 1.16 @@ -112,7 +113,7 @@ ifeq ($(shell uname -s),Darwin) endif test-api: ## Run api tests - cd api; go test ./... -coverprofile cover.out + cd api; go test $(GO_TEST_ARGS) ./... -coverprofile cover.out run: $(LIBGIT2) generate fmt vet manifests ## Run against the configured Kubernetes cluster in ~/.kube/config go run $(GO_STATIC_FLAGS) ./main.go From d38086bd7280f04e42b78ff7a83d7c95be739224 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 16 Mar 2022 22:22:07 +0530 Subject: [PATCH 2/6] Fix race condition in git proxy tests The variable used to store the information about proxied request was being written to in the proxy server request handler and read for assertion at the end of the test. Replace the boolean variable with an atomic counter to count the number of requests proxied, preventing the race condition. Signed-off-by: Sunny --- pkg/git/strategy/proxy/strategy_proxy_test.go | 55 ++++++++++++++----- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/pkg/git/strategy/proxy/strategy_proxy_test.go b/pkg/git/strategy/proxy/strategy_proxy_test.go index 6f0564eff..e27849c4d 100644 --- a/pkg/git/strategy/proxy/strategy_proxy_test.go +++ b/pkg/git/strategy/proxy/strategy_proxy_test.go @@ -24,6 +24,7 @@ import ( "net/url" "os" "strings" + "sync/atomic" "testing" "time" @@ -58,7 +59,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { gitImpl git.Implementation url string branch string - setupGitProxy func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) + setupGitProxy func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) shortTimeout bool wantUsedProxy bool wantError bool @@ -175,7 +176,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { gitImpl: libgit2.Implementation, url: "https://example.com/bar/test-reponame", branch: "main", - setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) { // Create the git server. gitServer, err := gittestserver.NewTempGitServer() g.Expect(err).ToNot(HaveOccurred()) @@ -210,7 +211,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { // Check if the host matches with the git server address and the user-agent is the expected git client. userAgent := ctx.Req.Header.Get("User-Agent") if strings.Contains(host, "example.com") && strings.Contains(userAgent, "libgit2") { - *proxyGotRequest = true + atomic.AddInt32(proxiedRequests, 1) return goproxy.OkConnect, u.Host } // Reject if it isn't our request. @@ -238,7 +239,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { gitImpl: libgit2.Implementation, url: "http://example.com/bar/test-reponame", branch: "main", - setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) { // Create the git server. gitServer, err := gittestserver.NewTempGitServer() g.Expect(err).ToNot(HaveOccurred()) @@ -258,8 +259,8 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { // The certificate used here is valid for both example.com and localhost. var proxyHandler goproxy.FuncReqHandler = func(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { userAgent := req.Header.Get("User-Agent") - if strings.Contains(req.Host, "example.com") && strings.Contains(userAgent, "libgit2") { - *proxyGotRequest = true + if strings.Contains(req.Host, "example.com") && strings.Contains(userAgent, "git") { + atomic.AddInt32(proxiedRequests, 1) req.Host = u.Host req.URL.Host = req.Host return req, nil @@ -282,14 +283,41 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { wantError: false, }, { - name: "libgit2_NO_PROXY", - gitImpl: libgit2.Implementation, + name: "gogit_HTTPS_PROXY", + gitImpl: gogit.Implementation, + url: "https://github.com/git-fixtures/basic", + branch: "master", + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) { + var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { + // We don't check for user agent as this handler is only going to process CONNECT requests, and because Go's net/http + // is the one making such a request on behalf of go-git, adding a check for the go net/http user agent (Go-http-client) + // would only allow false positives from any request originating from Go's net/http. + if strings.Contains(host, "github.com") { + atomic.AddInt32(proxiedRequests, 1) + return goproxy.OkConnect, host + } + // Reject if it isnt our request. + return goproxy.RejectConnect, host + } + proxy.OnRequest().HandleConnect(proxyHandler) + + // go-git does not allow to use an HTTPS proxy and a custom root CA at the same time. + // See https://github.com/fluxcd/source-controller/pull/524#issuecomment-1006673163. + return nil, func() {} + }, + shortTimeout: false, + wantUsedProxy: true, + wantError: false, + }, + { + name: "gogit_NO_PROXY", + gitImpl: gogit.Implementation, url: "https://192.0.2.1/bar/test-reponame", branch: "main", - setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) { var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { // We shouldn't hit the proxy so we just want to check for any interaction, then reject. - *proxyGotRequest = true + atomic.AddInt32(proxiedRequests, 1) return goproxy.RejectConnect, host } proxy.OnRequest().HandleConnect(proxyHandler) @@ -310,8 +338,8 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { proxy := goproxy.NewProxyHttpServer() proxy.Verbose = true - proxyGotRequest := false - authOpts, cleanup := tt.setupGitProxy(g, proxy, &proxyGotRequest) + proxiedRequests := int32(0) + authOpts, cleanup := tt.setupGitProxy(g, proxy, &proxiedRequests) defer cleanup() proxyServer := http.Server{ @@ -356,7 +384,8 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) } - g.Expect(proxyGotRequest).To(Equal(tt.wantUsedProxy)) + g.Expect(atomic.LoadInt32(&proxiedRequests) > 0).To(Equal(tt.wantUsedProxy)) + }) } } From d72a189e8825bd40eccb0f10c0500ede19b5e361 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 16 Mar 2022 23:07:25 +0530 Subject: [PATCH 3/6] internal/helm/getter: remove transport reuse test Since the transport reuse is dependent on the garbage collection, the result is inconsistent. It fails frequently when running the tests with the go race detector. Remove the test. Signed-off-by: Sunny --- internal/transport/transport_test.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index c07a88d59..f0bc387d6 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -34,23 +34,18 @@ func Test_TransportReuse(t *testing.T) { t.Errorf("error releasing transport t2: %v", err) } - t3 := NewOrIdle(nil) - if t2 != t3 { - t.Errorf("transported not reused") - } - - t4 := NewOrIdle(&tls.Config{ + t3 := NewOrIdle(&tls.Config{ ServerName: "testing", }) - if t4.TLSClientConfig == nil || t4.TLSClientConfig.ServerName != "testing" { + if t3.TLSClientConfig == nil || t3.TLSClientConfig.ServerName != "testing" { t.Errorf("TLSClientConfig not properly configured") } - err = Release(t4) + err = Release(t3) if err != nil { - t.Errorf("error releasing transport t4: %v", err) + t.Errorf("error releasing transport t3: %v", err) } - if t4.TLSClientConfig != nil { + if t3.TLSClientConfig != nil { t.Errorf("TLSClientConfig not cleared after release") } From a7ffb8c8aab20f91beb969e229e58738ef2b4a9f Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Mon, 13 Jun 2022 13:45:41 +0100 Subject: [PATCH 4/6] git: Update proxy tests Signed-off-by: Paulo Gomes --- pkg/git/strategy/proxy/strategy_proxy_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/git/strategy/proxy/strategy_proxy_test.go b/pkg/git/strategy/proxy/strategy_proxy_test.go index e27849c4d..dc06ab18f 100644 --- a/pkg/git/strategy/proxy/strategy_proxy_test.go +++ b/pkg/git/strategy/proxy/strategy_proxy_test.go @@ -79,7 +79,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { gitImpl: gogit.Implementation, url: "http://example.com/bar/test-reponame", branch: "main", - setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) { // Create the git server. gitServer, err := gittestserver.NewTempGitServer() g.Expect(err).ToNot(HaveOccurred()) @@ -102,7 +102,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { var proxyHandler goproxy.FuncReqHandler = func(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { userAgent := req.Header.Get("User-Agent") if strings.Contains(req.Host, "example.com") && strings.Contains(userAgent, "git") { - *proxyGotRequest = true + atomic.AddInt32(proxiedRequests, 1) req.Host = u.Host req.URL.Host = req.Host return req, nil @@ -130,13 +130,13 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { gitImpl: gogit.Implementation, url: "https://github.com/git-fixtures/basic", branch: "master", - setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) { var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { // We don't check for user agent as this handler is only going to process CONNECT requests, and because Go's net/http // is the one making such a request on behalf of go-git, adding a check for the go net/http user agent (Go-http-client) // would only allow false positives from any request originating from Go's net/http. if strings.Contains(host, "github.com") { - *proxyGotRequest = true + atomic.AddInt32(proxiedRequests, 1) return goproxy.OkConnect, host } // Reject if it isnt our request. @@ -157,10 +157,10 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) { gitImpl: gogit.Implementation, url: "https://192.0.2.1/bar/test-reponame", branch: "main", - setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxyGotRequest *bool) (*git.AuthOptions, cleanupFunc) { + setupGitProxy: func(g *WithT, proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) (*git.AuthOptions, cleanupFunc) { var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { // We shouldn't hit the proxy so we just want to check for any interaction, then reject. - *proxyGotRequest = true + atomic.AddInt32(proxiedRequests, 1) return goproxy.RejectConnect, host } proxy.OnRequest().HandleConnect(proxyHandler) From f1799dcb6b7d307d2dd4ee0f77954b2ea03133fc Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Mon, 13 Jun 2022 13:49:50 +0100 Subject: [PATCH 5/6] git: fix reconcileSource_authStrategy Co-authored-by: Sunny Signed-off-by: Paulo Gomes --- controllers/gitrepository_controller_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index c02e1320d..addd25cac 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -431,19 +431,19 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { } for _, tt := range tests { - obj := &sourcev1.GitRepository{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "auth-strategy-", - }, - Spec: sourcev1.GitRepositorySpec{ - Interval: metav1.Duration{Duration: interval}, - Timeout: &metav1.Duration{Duration: timeout}, - }, - } - t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) + obj := &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "auth-strategy-", + }, + Spec: sourcev1.GitRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + Timeout: &metav1.Duration{Duration: timeout}, + }, + } + server, err := gittestserver.NewTempGitServer() g.Expect(err).NotTo(HaveOccurred()) defer os.RemoveAll(server.Root()) From 230774cc80688e5cd80810e6f53c414aba0a70b6 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 14 Jun 2022 11:33:36 +0100 Subject: [PATCH 6/6] build: disable go test -race for arm64 Race detection is not working properly in our arm64 runners. It has been tested against both M1 and linux arm64 machines and in both cases the results were aligned with the other platforms. By disabling this we can ensure race detection is being enforced on the other platforms, and we can later review this position. Signed-off-by: Paulo Gomes --- .github/workflows/e2e.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 5553837ca..26a4c69e1 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -73,6 +73,14 @@ jobs: env: TEST_AZURE_ACCOUNT_NAME: ${{ secrets.TEST_AZURE_ACCOUNT_NAME }} TEST_AZURE_ACCOUNT_KEY: ${{ secrets.TEST_AZURE_ACCOUNT_KEY }} + + # Temporarily disabling -race for arm64 as our GitHub action + # runners don't seem to like it. The race detection was tested + # on both Apple M1 and Linux arm64 with successful results. + # + # We should reenable go test -race for arm64 runners once the + # current issue is resolved. + GO_TEST_ARGS: '' run: make test - name: Prepare id: prep