Merge pull request #615 from fluxcd/go-test-race

Paulo Gomes · web-flow · commit 0a993a8770b2 · 2022-06-14T15:14:01.000+01:00
build: enable `-race` for `go test`
diff --git 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
diff --git a/Makefile b/Makefile
@@ -7,15 +7,16 @@ 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'.
 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
diff --git 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())
diff --git 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")
 	}
 
diff --git 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
@@ -78,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())
@@ -101,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
@@ -129,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.
@@ -156,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)
@@ -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))
+
 		})
 	}
 }
