Bug 1896446: Fix private git clones behind a proxy

Switch between the "default" and global git configuration depending if a
.gitconfig file has been generated:

- Use the "default" .gitconfig specified by the GIT_CONFIG environment variable
  if a git configuration file was generated when source secrets were used to
  initialize the git envrionment.
- Use the global .gitconfig if no source secret was set up.
- Updated godoc to indicate when a .gitconfig file was generated and return values
  for setupGitEnvironment.
- Add missing methods to the SCMAuthContext interface. Update functions
to return SCMAuthContext interfaces.
- Add test package for unit test framework code

pkg/build/builder/cmd/builder.go

@@ -188,29 +188,38 @@ func newBuilderConfigFromEnvironment(out io.Writer, needsDocker bool) (*builderC
 	return cfg, nil
 }
 
-func (c *builderConfig) setupGitEnvironment() (string, []string, error) {
+// setupGitEnvironment configures the context for git commands to run. Returns
+// the following:
+//
+// 1. Path to the source secrets directory
+// 2. A list of set environment variables for git
+// 3. The path to the context's .gitconfig file if set, and
+// 4. An error if raised
+func (c *builderConfig) setupGitEnvironment() (string, []string, string, error) {
 
 	// For now, we only handle git. If not specified, we're done
 	gitSource := c.build.Spec.Source.Git
 	if gitSource == nil {
-		return "", []string{}, nil
+		return "", []string{}, "", nil
 	}
 
 	sourceSecret := c.build.Spec.Source.SourceSecret
 	gitEnv := []string{"GIT_ASKPASS=true"}
+	var outGitConfigFile string
 	// If a source secret is present, set it up and add its environment variables
 	if sourceSecret != nil {
 		// TODO: this should be refactored to let each source type manage which secrets
 		// it accepts
 		sourceURL, err := s2igit.Parse(gitSource.URI)
 		if err != nil {
-			return "", nil, fmt.Errorf("cannot parse build URL: %s", gitSource.URI)
+			return "", nil, "", fmt.Errorf("cannot parse build URL: %s", gitSource.URI)
 		}
 		scmAuths := scmauth.GitAuths(sourceURL)
 
-		secretsEnv, overrideURL, err := scmAuths.Setup(c.sourceSecretDir)
+		secretsEnv, overrideURL, gitConfigFile, err := scmAuths.Setup(c.sourceSecretDir)
+		outGitConfigFile = gitConfigFile
 		if err != nil {
-			return c.sourceSecretDir, nil, fmt.Errorf("cannot setup source secret: %v", err)
+			return c.sourceSecretDir, nil, outGitConfigFile, fmt.Errorf("cannot setup source secret: %v", err)
 		}
 		if overrideURL != nil {
 			gitSource.URI = overrideURL.String()
@@ -223,33 +232,40 @@ func (c *builderConfig) setupGitEnvironment() (string, []string, error) {
 		gitEnv = append(gitEnv, fmt.Sprintf("NO_PROXY=%s", *gitSource.NoProxy))
 		gitEnv = append(gitEnv, fmt.Sprintf("no_proxy=%s", *gitSource.NoProxy))
 	}
-	return c.sourceSecretDir, bld.MergeEnv(os.Environ(), gitEnv), nil
+	return c.sourceSecretDir, bld.MergeEnv(os.Environ(), gitEnv), outGitConfigFile, nil
 }
 
-// setupProxyConfig sets up a global git proxy configuration with the provided git client.
-// This is a work-around for Bug 1875639.
+// setupProxyConfig sets up a git proxy configuration with the provided git client, and directory
+// containing a local `.gitconfig` file if this directory contains a source secret.
 //
-// See also: https://bugzilla.redhat.com/show_bug.cgi?id=1875639
-func (c *builderConfig) setupProxyConfig(gitClient git.Repository) error {
+// This is a work-around for Bug 1875639 - see https://bugzilla.redhat.com/show_bug.cgi?id=1875639
+func (c *builderConfig) setupProxyConfig(gitClient git.Repository, gitConfigFile string) error {
 	gitSource := c.build.Spec.Source.Git
 	if gitSource == nil {
 		return nil
 	}
 	if gitSource.HTTPProxy != nil && len(*gitSource.HTTPProxy) > 0 {
-		err := gitClient.AddGlobalConfig("http.proxy", *gitSource.HTTPProxy)
-		if err != nil {
+		if err := c.addGitConfig(gitClient, gitConfigFile, "http.proxy", *gitSource.HTTPProxy); err != nil {
 			return err
 		}
 	}
 	if gitSource.HTTPSProxy != nil && len(*gitSource.HTTPSProxy) > 0 {
-		err := gitClient.AddGlobalConfig("https.proxy", *gitSource.HTTPSProxy)
-		if err != nil {
+		if err := c.addGitConfig(gitClient, gitConfigFile, "https.proxy", *gitSource.HTTPSProxy); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
+func (c *builderConfig) addGitConfig(gitClient git.Repository, gitConfigFile string, parameter string, value string) error {
+	if len(gitConfigFile) == 0 {
+		log.V(5).Infof("Adding parameter %s to global .gitconfig", parameter)
+		return gitClient.AddGlobalConfig(parameter, value)
+	}
+	log.V(5).Infof("Adding parameter %s to .gitconfig %s", parameter, gitConfigFile)
+	return gitClient.AddConfig("", parameter, value)
+}
+
 // clone is responsible for cloning the source referenced in the buildconfig
 func (c *builderConfig) clone() error {
 	ctx := timing.NewContext(context.Background())
@@ -258,17 +274,17 @@ func (c *builderConfig) clone() error {
 		c.build.Status.Stages = timing.GetStages(ctx)
 		bld.HandleBuildStatusUpdate(c.build, c.buildsClient, sourceRev)
 	}()
-	secretTmpDir, gitEnv, err := c.setupGitEnvironment()
+	secretTmpDir, gitEnv, gitConfigFile, err := c.setupGitEnvironment()
 	if err != nil {
 		return err
 	}
 	defer os.RemoveAll(secretTmpDir)
 
 	gitClient := git.NewRepositoryWithEnv(gitEnv)
-	err = c.setupProxyConfig(gitClient)
-	if err != nil {
+	if err = c.setupProxyConfig(gitClient, gitConfigFile); err != nil {
 		return err
 	}
+
 	buildDir := bld.InputContentPath
 	sourceInfo, err := bld.GitClone(ctx, gitClient, c.build.Spec.Source.Git, c.build.Spec.Revision, buildDir)
 	if err != nil {

pkg/build/builder/cmd/builder_test.go

@@ -0,0 +1,173 @@
+package cmd
+
+import (
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	buildv1 "github.com/openshift/api/build/v1"
+	"github.com/openshift/builder/test/unit/fake"
+)
+
+func TestSetupProxyConfig(t *testing.T) {
+	fakeHTTPProxy := "http://proxy.io"
+	fakeHTTPSProxy := "https://proxy.io"
+
+	cases := []struct {
+		name                  string
+		expectError           bool
+		build                 *buildv1.Build
+		inputGitConfig        string
+		expectedConfigs       map[string]string
+		expectedLocalConfigs  map[string]string
+		expectedGlobalConfigs map[string]string
+	}{
+		{
+			name:  "no git",
+			build: &buildv1.Build{},
+		},
+		{
+			name: "git clone no proxy no auth",
+			build: &buildv1.Build{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-build-1",
+					Namespace: "test",
+				},
+				Spec: buildv1.BuildSpec{
+					CommonSpec: buildv1.CommonSpec{
+						Source: buildv1.BuildSource{
+							Type: buildv1.BuildSourceGit,
+							Git: &buildv1.GitBuildSource{
+								URI: "https://githost.dev/myorg/myrepo.git",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "git clone proxy no auth",
+			build: &buildv1.Build{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-build-1",
+					Namespace: "test",
+				},
+				Spec: buildv1.BuildSpec{
+					CommonSpec: buildv1.CommonSpec{
+						Source: buildv1.BuildSource{
+							Type: buildv1.BuildSourceGit,
+							Git: &buildv1.GitBuildSource{
+								URI: "https://githost.dev/myorg/myrepo.git",
+								ProxyConfig: buildv1.ProxyConfig{
+									HTTPProxy:  &fakeHTTPProxy,
+									HTTPSProxy: &fakeHTTPSProxy,
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedGlobalConfigs: map[string]string{
+				"http.proxy":  fakeHTTPProxy,
+				"https.proxy": fakeHTTPSProxy,
+			},
+		},
+		{
+			name: "git clone no proxy auth",
+			build: &buildv1.Build{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-build-1",
+					Namespace: "test",
+				},
+				Spec: buildv1.BuildSpec{
+					CommonSpec: buildv1.CommonSpec{
+						Source: buildv1.BuildSource{
+							Type: buildv1.BuildSourceGit,
+							Git: &buildv1.GitBuildSource{
+								URI: "https://githost.dev/myorg/myrepo.git",
+							},
+							SourceSecret: &corev1.LocalObjectReference{
+								Name: "git-auth",
+							},
+						},
+					},
+				},
+			},
+			inputGitConfig: ".gitconfig.test",
+		},
+		{
+			name: "git clone proxy auth",
+			build: &buildv1.Build{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-build-1",
+					Namespace: "test",
+				},
+				Spec: buildv1.BuildSpec{
+					CommonSpec: buildv1.CommonSpec{
+						Source: buildv1.BuildSource{
+							Type: buildv1.BuildSourceGit,
+							Git: &buildv1.GitBuildSource{
+								URI: "https://githost.dev/myorg/myrepo.git",
+								ProxyConfig: buildv1.ProxyConfig{
+									HTTPProxy:  &fakeHTTPProxy,
+									HTTPSProxy: &fakeHTTPSProxy,
+								},
+							},
+							SourceSecret: &corev1.LocalObjectReference{
+								Name: "git-auth",
+							},
+						},
+					},
+				},
+			},
+			inputGitConfig: ".gitconfig.test",
+			expectedConfigs: map[string]string{
+				"http.proxy":  fakeHTTPProxy,
+				"https.proxy": fakeHTTPSProxy,
+			},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			builder := &builderConfig{}
+			builder.build = tc.build
+			gitClient := fake.NewFakeGitRepository()
+			err := builder.setupProxyConfig(gitClient, tc.inputGitConfig)
+			if err != nil && !tc.expectError {
+				t.Errorf("received unexpected error: %v", err)
+			}
+			if err == nil && tc.expectError {
+				t.Error("expected to receive error, got none")
+			}
+
+			if len(tc.expectedConfigs) != len(gitClient.Configs) {
+				t.Errorf("expected default configs to have %d entries, got %d", len(tc.expectedConfigs), len(gitClient.Configs))
+			}
+
+			if len(tc.expectedLocalConfigs) != len(gitClient.LocalConfigs) {
+				t.Errorf("expected default configs to have %d entries, got %d", len(tc.expectedLocalConfigs), len(gitClient.LocalConfigs))
+			}
+
+			if len(tc.expectedGlobalConfigs) != len(gitClient.GlobalConfigs) {
+				t.Errorf("expected global configs to have %d entries, got %d", len(tc.expectedGlobalConfigs), len(gitClient.GlobalConfigs))
+			}
+
+			verifyGitConfigs("default", tc.expectedConfigs, gitClient.Configs, t)
+			verifyGitConfigs("local", tc.expectedLocalConfigs, gitClient.LocalConfigs, t)
+			verifyGitConfigs("global", tc.expectedGlobalConfigs, gitClient.GlobalConfigs, t)
+		})
+	}
+}
+
+func verifyGitConfigs(name string, expectedMap, actualMap map[string]string, t *testing.T) {
+	for config, expected := range expectedMap {
+		actual, present := actualMap[config]
+		if !present {
+			t.Errorf("expected %s git configuration %s was not set", name, config)
+		}
+		if actual != expected {
+			t.Errorf("expected %s git config %s to be %s, got %s", name, config, expected, actual)
+		}
+	}
+}

pkg/build/builder/cmd/scmauth/cacert.go

@@ -22,13 +22,14 @@ type CACert struct {
 }
 
 // Setup creates a .gitconfig fragment that points to the given ca.crt
-func (s CACert) Setup(baseDir string, context SCMAuthContext) error {
+// Returns the location of the .gitconfig file, and error if raised.
+func (s CACert) Setup(baseDir string, context SCMAuthContext) (string, error) {
 	if !(s.SourceURL.Type == s2igit.URLTypeURL && s.SourceURL.URL.Scheme == "https" && s.SourceURL.URL.Opaque == "") {
-		return nil
+		return "", nil
 	}
 	gitconfig, err := ioutil.TempFile("", "ca.crt.")
 	if err != nil {
-		return err
+		return "", err
 	}
 	defer gitconfig.Close()
 	content := fmt.Sprintf(CACertConfig, filepath.Join(baseDir, CACertName))
@@ -39,11 +40,11 @@ func (s CACert) Setup(baseDir string, context SCMAuthContext) error {
 }
 
 // Name returns the name of this auth method.
-func (_ CACert) Name() string {
+func (CACert) Name() string {
 	return CACertName
 }
 
 // Handles returns true if the secret is a CA certificate
-func (_ CACert) Handles(name string) bool {
+func (CACert) Handles(name string) bool {
 	return name == CACertName
 }

pkg/build/builder/cmd/scmauth/cacert_test.go

@@ -25,8 +25,11 @@ func TestCACertSetup(t *testing.T) {
 	secretDir := secretDir(t, "ca.crt")
 	defer os.RemoveAll(secretDir)
 
-	err := caCert.Setup(secretDir, context)
+	configFile, err := caCert.Setup(secretDir, context)
 	gitConfig, _ := context.Get("GIT_CONFIG")
+	if configFile != gitConfig {
+		t.Errorf("expected .gitconfig from Setup %s to match GIT_CONFIG value %s", configFile, gitConfig)
+	}
 	defer cleanupConfig(gitConfig)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -42,12 +45,16 @@ func TestCACertSetupNoSSL(t *testing.T) {
 	secretDir := secretDir(t, "ca.crt")
 	defer os.RemoveAll(secretDir)
 
-	err := caCert.Setup(secretDir, context)
+	configFile, err := caCert.Setup(secretDir, context)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	_, gitConfigPresent := context.Get("GIT_CONFIG")
+
+	if len(configFile) > 0 {
+		t.Errorf("expected .gitconfig from Setup to be empty, got %s", configFile)
+	}
+	value, gitConfigPresent := context.Get("GIT_CONFIG")
 	if gitConfigPresent {
-		t.Fatalf("git config not expected")
+		t.Errorf("expected GIT_CONFIG to be unset, got %s", value)
 	}
 }

pkg/build/builder/cmd/scmauth/context.go

@@ -10,7 +10,9 @@ type defaultSCMContext struct {
 	overrideURL *url.URL
 }
 
-func NewDefaultSCMContext() *defaultSCMContext {
+// NewDefaultSCMContext creates a new environment context for setting up source control management
+// tools, i.e. git.
+func NewDefaultSCMContext() SCMAuthContext {
 	return &defaultSCMContext{
 		vars: make(map[string]string),
 	}

pkg/build/builder/cmd/scmauth/gitconfig.go

@@ -9,18 +9,19 @@ const GitConfigName = ".gitconfig"
 // GitConfig implements SCMAuth interface for using a custom .gitconfig file
 type GitConfig struct{}
 
-// Setup adds the secret .gitconfig as an include to the .gitconfig file to be used in the build
-func (_ GitConfig) Setup(baseDir string, context SCMAuthContext) error {
+// Setup adds the secret .gitconfig as an include to the .gitconfig file to be used in the build.
+// Returns the path to the generated .gitconfig and error if raised
+func (GitConfig) Setup(baseDir string, context SCMAuthContext) (string, error) {
 	log.V(4).Infof("Adding user-provided gitconfig %s to build gitconfig", filepath.Join(baseDir, GitConfigName))
 	return ensureGitConfigIncludes(filepath.Join(baseDir, GitConfigName), context)
 }
 
 // Name returns the name of this auth method.
-func (_ GitConfig) Name() string {
+func (GitConfig) Name() string {
 	return GitConfigName
 }
 
 // Handles returns true if the secret file is a gitconfig
-func (_ GitConfig) Handles(name string) bool {
+func (GitConfig) Handles(name string) bool {
 	return name == GitConfigName
 }