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 <[email protected]>

Author: darkowlzz

pkg/git/strategy/proxy/strategy_proxy_test.go

@@ -24,6 +24,7 @@ import (
 	"net/url"
 	"os"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -59,7 +60,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
@@ -176,7 +177,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())
@@ -211,7 +212,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.
@@ -239,7 +240,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())
@@ -259,8 +260,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
@@ -283,14 +284,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)
@@ -311,8 +339,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{
@@ -357,7 +385,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))
+
 		})
 	}
 }