chore: Simplified GetRepoHTTPClient function (#9396)

* chore: Simplified GetRepoHTTPClient function Signed-off-by: ls0f <lovedboy.tk@qq.com> * simplified code and improve unit test coverage Signed-off-by: ls0f <lovedboy.tk@qq.com>
argoproj · May 25, 2022 · f831c07 · f831c07
1 parent 489f85a
commit f831c07
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 56 deletions.
diff --git a/util/git/client.go b/util/git/client.go
@@ -203,46 +203,30 @@ func GetRepoHTTPClient(repoURL string, insecure bool, creds Creds, proxyURL stri
 
 		return &cert, nil
 	}
-
+	transport := &http.Transport{
+		Proxy: proxyFunc,
+		TLSClientConfig: &tls.Config{
+			GetClientCertificate: clientCertFunc,
+		},
+		DisableKeepAlives: true,
+	}
+	customHTTPClient.Transport = transport
 	if insecure {
-		customHTTPClient.Transport = &http.Transport{
-			Proxy: proxyFunc,
-			TLSClientConfig: &tls.Config{
-				InsecureSkipVerify:   true,
-				GetClientCertificate: clientCertFunc,
-			},
-			DisableKeepAlives: true,
-		}
-	} else {
-		parsedURL, err := url.Parse(repoURL)
-		if err != nil {
-			return customHTTPClient
-		}
-		serverCertificatePem, err := certutil.GetCertificateForConnect(parsedURL.Host)
-		if err != nil {
-			return customHTTPClient
-		} else if len(serverCertificatePem) > 0 {
-			certPool := certutil.GetCertPoolFromPEMData(serverCertificatePem)
-			customHTTPClient.Transport = &http.Transport{
-				Proxy: proxyFunc,
-				TLSClientConfig: &tls.Config{
-					RootCAs:              certPool,
-					GetClientCertificate: clientCertFunc,
-				},
-				DisableKeepAlives: true,
-			}
-		} else {
-			// else no custom certificate stored.
-			customHTTPClient.Transport = &http.Transport{
-				Proxy: proxyFunc,
-				TLSClientConfig: &tls.Config{
-					GetClientCertificate: clientCertFunc,
-				},
-				DisableKeepAlives: true,
-			}
-		}
+		transport.TLSClientConfig.InsecureSkipVerify = true
+		return customHTTPClient
+	}
+	parsedURL, err := url.Parse(repoURL)
+	if err != nil {
+		return customHTTPClient
+	}
+	serverCertificatePem, err := certutil.GetCertificateForConnect(parsedURL.Host)
+	if err != nil {
+		return customHTTPClient
+	}
+	if len(serverCertificatePem) > 0 {
+		certPool := certutil.GetCertPoolFromPEMData(serverCertificatePem)
+		transport.TLSClientConfig.RootCAs = certPool
 	}
-
 	return customHTTPClient
 }
 

diff --git a/util/git/git_test.go b/util/git/git_test.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
+	"github.com/argoproj/argo-cd/v2/common"
 	"github.com/argoproj/argo-cd/v2/test/fixture/log"
 	"github.com/argoproj/argo-cd/v2/test/fixture/path"
 	"github.com/argoproj/argo-cd/v2/test/fixture/test"
@@ -151,22 +152,22 @@ func TestCustomHTTPClient(t *testing.T) {
 	assert.NotNil(t, client)
 	assert.NotNil(t, client.Transport)
 	if client.Transport != nil {
-		httpClient := client.Transport.(*http.Transport)
-		assert.NotNil(t, httpClient.TLSClientConfig)
-
-		assert.Equal(t, false, httpClient.TLSClientConfig.InsecureSkipVerify)
-
-		assert.NotNil(t, httpClient.TLSClientConfig.GetClientCertificate)
-		if httpClient.TLSClientConfig.GetClientCertificate != nil {
-			cert, err := httpClient.TLSClientConfig.GetClientCertificate(nil)
+		transport := client.Transport.(*http.Transport)
+		assert.NotNil(t, transport.TLSClientConfig)
+		assert.Equal(t, true, transport.DisableKeepAlives)
+		assert.Equal(t, false, transport.TLSClientConfig.InsecureSkipVerify)
+		assert.NotNil(t, transport.TLSClientConfig.GetClientCertificate)
+		assert.Nil(t, transport.TLSClientConfig.RootCAs)
+		if transport.TLSClientConfig.GetClientCertificate != nil {
+			cert, err := transport.TLSClientConfig.GetClientCertificate(nil)
 			assert.NoError(t, err)
 			if err == nil {
 				assert.NotNil(t, cert)
 				assert.NotEqual(t, 0, len(cert.Certificate))
 				assert.NotNil(t, cert.PrivateKey)
 			}
 		}
-		proxy, err := httpClient.Proxy(nil)
+		proxy, err := transport.Proxy(nil)
 		assert.Nil(t, err)
 		assert.Equal(t, "http://proxy:5000", proxy.String())
 	}
@@ -182,14 +183,14 @@ func TestCustomHTTPClient(t *testing.T) {
 	assert.NotNil(t, client)
 	assert.NotNil(t, client.Transport)
 	if client.Transport != nil {
-		httpClient := client.Transport.(*http.Transport)
-		assert.NotNil(t, httpClient.TLSClientConfig)
-
-		assert.Equal(t, true, httpClient.TLSClientConfig.InsecureSkipVerify)
-
-		assert.NotNil(t, httpClient.TLSClientConfig.GetClientCertificate)
-		if httpClient.TLSClientConfig.GetClientCertificate != nil {
-			cert, err := httpClient.TLSClientConfig.GetClientCertificate(nil)
+		transport := client.Transport.(*http.Transport)
+		assert.NotNil(t, transport.TLSClientConfig)
+		assert.Equal(t, true, transport.DisableKeepAlives)
+		assert.Equal(t, true, transport.TLSClientConfig.InsecureSkipVerify)
+		assert.NotNil(t, transport.TLSClientConfig.GetClientCertificate)
+		assert.Nil(t, transport.TLSClientConfig.RootCAs)
+		if transport.TLSClientConfig.GetClientCertificate != nil {
+			cert, err := transport.TLSClientConfig.GetClientCertificate(nil)
 			assert.NoError(t, err)
 			if err == nil {
 				assert.NotNil(t, cert)
@@ -199,10 +200,28 @@ func TestCustomHTTPClient(t *testing.T) {
 		}
 		req, err := http.NewRequest("GET", "http://proxy-from-env:7878", nil)
 		assert.Nil(t, err)
-		proxy, err := httpClient.Proxy(req)
+		proxy, err := transport.Proxy(req)
 		assert.Nil(t, err)
 		assert.Equal(t, "http://proxy-from-env:7878", proxy.String())
 	}
+	// GetRepoHTTPClient with root ca
+	cert, err := ioutil.ReadFile("../../test/fixture/certs/argocd-test-server.crt")
+	assert.NoError(t, err)
+	temppath := t.TempDir()
+	defer os.RemoveAll(temppath)
+	err = ioutil.WriteFile(filepath.Join(temppath, "127.0.0.1"), cert, 0666)
+	assert.NoError(t, err)
+	os.Setenv(common.EnvVarTLSDataPath, temppath)
+	client = GetRepoHTTPClient("https://127.0.0.1", false, creds, "")
+	assert.NotNil(t, client)
+	assert.NotNil(t, client.Transport)
+	if client.Transport != nil {
+		transport := client.Transport.(*http.Transport)
+		assert.NotNil(t, transport.TLSClientConfig)
+		assert.Equal(t, true, transport.DisableKeepAlives)
+		assert.Equal(t, false, transport.TLSClientConfig.InsecureSkipVerify)
+		assert.NotNil(t, transport.TLSClientConfig.RootCAs)
+	}
 }
 
 func TestLsRemote(t *testing.T) {