test: refactor git portforwarder under gitprovider (#521)

This refactors the git port forwarder abstraction to be underneath the
git provider. The git port forwarder is only relevant with the
in-cluster git server, and thus fits as an implementation detail of the
LocalProvider.

Author: sdowell

e2e/nomostest/git.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"kpt.dev/configsync/e2e/nomostest/gitproviders"
 	"kpt.dev/configsync/e2e/nomostest/retry"
 	"kpt.dev/configsync/e2e/nomostest/testing"
 	"kpt.dev/configsync/e2e/nomostest/testkubeclient"
@@ -96,9 +97,8 @@ type Repository struct {
 	// For other git providers, it appends a UUID to Name for uniqueness.
 	RemoteRepoName string
 
-	// RemoteURL is the remote URL of the repository.
-	// It is used to set the url for the remote origin using `git remote add origin <REMOTE_URL>.
-	RemoteURL string
+	// GitProvider is the provider that hosts the Git repositories.
+	GitProvider gitproviders.GitProvider
 
 	// Scheme used for encoding and decoding objects.
 	Scheme *runtime.Scheme
@@ -130,6 +130,7 @@ func NewRepository(nt *NT, repoType RepoType, nn types.NamespacedName, sourceFor
 		SafetyClusterRolePath: fmt.Sprintf("acme/cluster/cluster-role-%s.yaml", safetyName),
 		Scheme:                nt.Scheme,
 		Logger:                nt.Logger,
+		GitProvider:           nt.GitProvider,
 	}
 
 	repoName, err := nt.GitProvider.CreateRepository(namespacedName)
@@ -139,16 +140,6 @@ func NewRepository(nt *NT, repoType RepoType, nn types.NamespacedName, sourceFor
 		nt.T.Fatal(err)
 	}
 	g.RemoteRepoName = repoName
-	var port int
-	// port argument is used for in-cluster git provider with the local port forward.
-	// the argument is ignored for other providers, e.g. Gitlab/Bitbucket
-	if nt.gitRepoPortForwarder != nil {
-		port, err = nt.gitRepoPortForwarder.LocalPort()
-		if err != nil {
-			nt.T.Fatal(err)
-		}
-	}
-	g.RemoteURL = nt.GitProvider.RemoteURL(port, repoName)
 
 	g.init(nt.gitPrivateKeyPath)
 	g.initialCommit(sourceFormat)
@@ -162,18 +153,6 @@ func (g *Repository) ReInit(nt *NT, sourceFormat filesystem.SourceFormat) {
 
 	// Update test environment
 	g.T = nt.T
-	var port int
-	// port argument is used for in-cluster git provider with the local port forward.
-	// the argument is ignored for other providers, e.g. Gitlab/Bitbucket
-	if nt.gitRepoPortForwarder != nil {
-		var err error
-		port, err = nt.gitRepoPortForwarder.LocalPort()
-		if err != nil {
-			nt.T.Fatal(err)
-		}
-	}
-	// Update URL to use latest port-forward port
-	g.RemoteURL = nt.GitProvider.RemoteURL(port, g.RemoteRepoName)
 	// Reset repo contents
 	g.init(nt.gitPrivateKeyPath)
 	g.initialCommit(sourceFormat)
@@ -527,9 +506,12 @@ func (g *Repository) Push(refspec string, flags ...string) {
 	took, err := retry.Retry(1*time.Minute, func() error {
 		args := []string{"push"}
 		args = append(args, flags...)
-		args = append(args, g.RemoteURL, refspec)
+		remoteURL, err := g.GitProvider.RemoteURL(g.RemoteRepoName)
+		if err != nil {
+			return err
+		}
+		args = append(args, remoteURL, refspec)
 		cmd := g.gitCmd(args...)
-		var err error
 		out, err = cmd.CombinedOutput()
 		return err
 	})
@@ -539,7 +521,11 @@ func (g *Repository) Push(refspec string, flags ...string) {
 	}
 }
 
-func (g *Repository) pushAllToRemote(remote string) {
+func (g *Repository) pushAllToRemote() {
+	remote, err := g.GitProvider.RemoteURL(g.RemoteRepoName)
+	if err != nil {
+		g.T.Errorf("failed to get remote URL: %v", err)
+	}
 	cmd := g.gitCmd("push", remote, "--all")
 	out, err := cmd.CombinedOutput()
 	if err != nil {

e2e/nomostest/gitproviders/bitbucket.go

@@ -71,8 +71,8 @@ func (b *BitbucketClient) Type() string {
 
 // RemoteURL returns the Git URL for the Bitbucket repository.
 // name refers to the repo name in the format of <NAMESPACE>/<NAME> of RootSync|RepoSync.
-func (b *BitbucketClient) RemoteURL(_ int, name string) string {
-	return b.SyncURL(name)
+func (b *BitbucketClient) RemoteURL(name string) (string, error) {
+	return b.SyncURL(name), nil
 }
 
 // SyncURL returns a URL for Config Sync to sync from.

e2e/nomostest/gitproviders/git-provider.go

@@ -16,6 +16,7 @@ package gitproviders
 
 import (
 	"kpt.dev/configsync/e2e"
+	"kpt.dev/configsync/e2e/nomostest/portforwarder"
 	"kpt.dev/configsync/e2e/nomostest/testing"
 )
 
@@ -33,7 +34,7 @@ type GitProvider interface {
 	// For the testing git-server, RemoteURL uses localhost and forwarded port, while SyncURL uses the DNS.
 	// For other git providers, RemoteURL should be the same as SyncURL.
 	// name refers to the repo name in the format of <NAMESPACE>/<NAME> of RootSync|RepoSync.
-	RemoteURL(port int, name string) string
+	RemoteURL(name string) (string, error)
 
 	// SyncURL returns the git repository URL for Config Sync to sync from.
 	// name refers to the repo name in the format of <NAMESPACE>/<NAME> of RootSync|RepoSync.
@@ -43,8 +44,29 @@ type GitProvider interface {
 	DeleteObsoleteRepos() error
 }
 
+// GitProviderOpt is an optional parameter for instantiating a new GitProvider
+type GitProviderOpt func(opts *GitProviderOpts)
+
+// GitProviderOpts is the set of optional parameters for instantiating a new GitProvider
+type GitProviderOpts struct {
+	portForwarder *portforwarder.PortForwarder
+}
+
+// WithPortForwarder provides a PortForwarder for the GitProvider to use.
+// Required for LocalProvider in order to establish a PortForwarder to the in-cluster
+// git server.
+func WithPortForwarder(portForwarder *portforwarder.PortForwarder) GitProviderOpt {
+	return func(opts *GitProviderOpts) {
+		opts.portForwarder = portForwarder
+	}
+}
+
 // NewGitProvider creates a GitProvider for the specific provider type.
-func NewGitProvider(t testing.NTB, provider string) GitProvider {
+func NewGitProvider(t testing.NTB, provider string, opts ...GitProviderOpt) GitProvider {
+	options := GitProviderOpts{}
+	for _, opt := range opts {
+		opt(&options)
+	}
 	switch provider {
 	case e2e.Bitbucket:
 		client, err := newBitbucketClient()
@@ -59,6 +81,10 @@ func NewGitProvider(t testing.NTB, provider string) GitProvider {
 		}
 		return client
 	default:
-		return &LocalProvider{}
+		client, err := newLocalProvider(options)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return client
 	}
 }

e2e/nomostest/gitproviders/gitlab.go

@@ -56,8 +56,8 @@ func (g *GitlabClient) Type() string {
 }
 
 // RemoteURL returns the Git URL for the Gitlab project repository.
-func (g *GitlabClient) RemoteURL(_ int, name string) string {
-	return g.SyncURL(name)
+func (g *GitlabClient) RemoteURL(name string) (string, error) {
+	return g.SyncURL(name), nil
 }
 
 // SyncURL returns a URL for Config Sync to sync from.

e2e/nomostest/gitproviders/local.go

@@ -18,10 +18,23 @@ import (
 	"fmt"
 
 	"kpt.dev/configsync/e2e"
+	"kpt.dev/configsync/e2e/nomostest/portforwarder"
 )
 
 // LocalProvider refers to the test git-server running on the same test cluster.
-type LocalProvider struct{}
+type LocalProvider struct {
+	portForwarder *portforwarder.PortForwarder
+}
+
+func newLocalProvider(opts GitProviderOpts) (*LocalProvider, error) {
+	if opts.portForwarder == nil {
+		return nil, fmt.Errorf("must provide a PortForwarderFactory for LocalProvider")
+	}
+	provider := &LocalProvider{
+		portForwarder: opts.portForwarder,
+	}
+	return provider, nil
+}
 
 // Type returns the provider type.
 func (l *LocalProvider) Type() string {
@@ -30,8 +43,12 @@ func (l *LocalProvider) Type() string {
 
 // RemoteURL returns the Git URL for connecting to the test git-server.
 // name refers to the repo name in the format of <NAMESPACE>/<NAME> of RootSync|RepoSync.
-func (l *LocalProvider) RemoteURL(port int, name string) string {
-	return fmt.Sprintf("ssh://git@localhost:%d/git-server/repos/%s", port, name)
+func (l *LocalProvider) RemoteURL(name string) (string, error) {
+	port, err := l.portForwarder.LocalPort()
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("ssh://git@localhost:%d/git-server/repos/%s", port, name), nil
 }
 
 // SyncURL returns a URL for Config Sync to sync from.

e2e/nomostest/new.go

@@ -179,7 +179,6 @@ func SharedTestEnv(t nomostesting.NTB, opts *ntopts.New) *NT {
 		gitPrivateKeyPath:       sharedNt.gitPrivateKeyPath,
 		caCertPath:              sharedNt.caCertPath,
 		Scheme:                  sharedNt.Scheme,
-		GitProvider:             sharedNt.GitProvider,
 		RemoteRepositories:      sharedNt.RemoteRepositories,
 		WebhookDisabled:         sharedNt.WebhookDisabled,
 	}
@@ -257,7 +256,6 @@ func FreshTestEnv(t nomostesting.NTB, opts *ntopts.New) *NT {
 		NonRootRepos:            make(map[types.NamespacedName]*Repository),
 		MetricsExpectations:     testmetrics.NewSyncSetExpectations(t, scheme),
 		Scheme:                  scheme,
-		GitProvider:             gitproviders.NewGitProvider(t, *e2e.GitProvider),
 		RemoteRepositories:      make(map[types.NamespacedName]*Repository),
 		WebhookDisabled:         &webhookDisabled,
 		DefaultMetricsTimeout:   30 * time.Second,
@@ -318,8 +316,7 @@ func FreshTestEnv(t nomostesting.NTB, opts *ntopts.New) *NT {
 	if err := nt.KubeClient.Create(fake.NamespaceObject(metrics.MonitoringNamespace)); err != nil {
 		nt.T.Fatal(err)
 	}
-
-	if nt.GitProvider.Type() == e2e.Local {
+	if *e2e.GitProvider == e2e.Local {
 		if err := nt.KubeClient.Create(gitNamespace()); err != nil {
 			nt.T.Fatal(err)
 		}
@@ -367,10 +364,12 @@ func setupTestCase(nt *NT, opts *ntopts.New) {
 		allRepos = append(allRepos, repo)
 	}
 
-	if nt.GitProvider.Type() == e2e.Local {
+	var gitProviderOpts []gitproviders.GitProviderOpt
+	if *e2e.GitProvider == e2e.Local {
 		InitGitRepos(nt, allRepos...)
-		nt.portForwardGitServer()
+		gitProviderOpts = append(gitProviderOpts, gitproviders.WithPortForwarder(nt.portForwardGitServer()))
 	}
+	nt.GitProvider = gitproviders.NewGitProvider(nt.T, *e2e.GitProvider, gitProviderOpts...)
 
 	for name := range opts.RootRepos {
 		nt.RootRepos[name] = resetRepository(nt, RootRepo, RootSyncNN(name), opts.SourceFormat)

e2e/nomostest/nt.go

@@ -147,9 +147,6 @@ type NT struct {
 	// caCertPath is the path to the CA cert used for communicating with the Git server.
 	caCertPath string
 
-	// gitRepoPortForwarder is the local port forwarding for the git server deployment.
-	gitRepoPortForwarder *portforwarder.PortForwarder
-
 	// prometheusPortForwarder is the local port forwarding for the prometheus deployment.
 	prometheusPortForwarder *portforwarder.PortForwarder
 
@@ -592,11 +589,8 @@ func (nt *NT) portForwardOtelCollector() {
 }
 
 // portForwardGitServer forwards the git-server deployment to a port.
-func (nt *NT) portForwardGitServer() {
+func (nt *NT) portForwardGitServer() *portforwarder.PortForwarder {
 	nt.T.Helper()
-	if nt.gitRepoPortForwarder != nil {
-		nt.T.Fatal("git server port forward already initialized")
-	}
 	prevPodName := ""
 	resetGitRepos := func(newPort int, podName string) {
 		// pod unchanged, don't reset
@@ -615,14 +609,12 @@ func (nt *NT) portForwardGitServer() {
 		// re-init all repos
 		InitGitRepos(nt, allRepos...)
 		// attempt to recover by re-pushing the local repo states
-		for nn, remoteRepo := range nt.RemoteRepositories {
-			remoteURL := nt.GitProvider.RemoteURL(newPort, nn.String())
-			remoteRepo.pushAllToRemote(remoteURL)
-			remoteRepo.RemoteURL = remoteURL
+		for _, remoteRepo := range nt.RemoteRepositories {
+			remoteRepo.pushAllToRemote()
 		}
 		prevPodName = podName
 	}
-	nt.gitRepoPortForwarder = nt.newPortForwarder(
+	return nt.newPortForwarder(
 		testGitNamespace,
 		testGitServer,
 		":22",

e2e/nomostest/portforwarder/port_forwarder.go

@@ -118,17 +118,19 @@ func (pf *PortForwarder) LocalPort() (int, error) {
 }
 
 func (pf *PortForwarder) setPort(cmd *exec.Cmd, port int, pod string, async bool) {
+	pf.logger.Infof("updating port-forward %s:%d -> %s:%d", pf.pod, pf.localPort, pod, port)
 	if async {
 		pf.mux.Lock()
-		defer pf.mux.Unlock()
-	}
-	pf.logger.Infof("updating port-forward %s:%d -> %s:%d", pf.pod, pf.localPort, pod, port)
-	if pf.setPortCallback != nil {
-		pf.setPortCallback(port, pod)
 	}
 	pf.cmd = cmd
 	pf.localPort = port
 	pf.pod = pod
+	if async {
+		pf.mux.Unlock()
+	}
+	if pf.setPortCallback != nil {
+		pf.setPortCallback(port, pod)
+	}
 }
 
 // portForwardToPod establishes a port forwarding to the provided pod name. This
@@ -188,7 +190,7 @@ func (pf *PortForwarder) portForwardToDeployment(async bool) error {
 	// subprocess will be killed with the context, and we get a random port every
 	// time. However, this cleans up subprocesses in the interim.
 	if pf.cmd != nil {
-		pf.logger.Info("stopping port-forward process for %s/%s", pf.ns, pf.deployment)
+		pf.logger.Infof("stopping port-forward process for %s/%s", pf.ns, pf.deployment)
 		if err := pf.cmd.Process.Kill(); err != nil && errors.Is(err, os.ErrProcessDone) {
 			return errors.Wrap(err, "failed to kill port forward process")
 		}