bootstrapping normalises ssh urls without scheme (#3712)

* support ssh urls with scheme (shorter scp-like) * acceptance test using ssh short url * update docs * review suggestion
weaveworks · Dec 12, 2023 · 531449f · 531449f
1 parent 1172c1d
commit 531449f
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 52 deletions.
diff --git a/cmd/gitops/app/bootstrap/cmd_acceptance_test.go b/cmd/gitops/app/bootstrap/cmd_acceptance_test.go
@@ -73,41 +73,51 @@ func TestBootstrapCmd(t *testing.T) {
 	g.SetDefaultEventuallyPollingInterval(defaultInterval)
 	testLog := testr.New(t)
 
+	// ssh git repo configuration
 	privateKeyFile := os.Getenv("GIT_PRIVATEKEY_PATH")
 	g.Expect(privateKeyFile).NotTo(BeEmpty())
 
+	privateKeyPassword := os.Getenv("GIT_PRIVATEKEY_PASSWORD")
+	g.Expect(privateKeyPassword).NotTo(BeEmpty())
+
 	repoURLSSH := os.Getenv("GIT_REPO_URL_SSH")
 	g.Expect(repoURLSSH).NotTo(BeEmpty())
+
+	repoURLSSHNoScheme := os.Getenv("GIT_REPO_URL_SSH_NO_SCHEME")
+	g.Expect(repoURLSSHNoScheme).NotTo(BeEmpty())
+
+	privateKeyFlag := fmt.Sprintf("--private-key=%s", privateKeyFile)
+	privateKeyPasswordFlag := fmt.Sprintf("--private-key-password=%s", privateKeyPassword)
+	gitRepoUrlSshNoSchemeFlag := fmt.Sprintf("--repo-url=%s", repoURLSSHNoScheme)
+
+	// https git repo configuration
 	repoURLHTTPS := os.Getenv("GIT_REPO_URL_HTTPS")
 	g.Expect(repoURLHTTPS).NotTo(BeEmpty())
+
 	gitUsername := os.Getenv("GIT_USERNAME")
 	g.Expect(gitUsername).NotTo(BeEmpty())
+
 	gitPassword := os.Getenv("GIT_PASSWORD")
 	g.Expect(gitPassword).NotTo(BeEmpty())
+
+	// git repo configuration
 	gitBranch := os.Getenv("GIT_BRANCH")
 	g.Expect(gitBranch).NotTo(BeEmpty())
+
 	gitRepoPath := os.Getenv("GIT_REPO_PATH")
 	g.Expect(gitRepoPath).NotTo(BeEmpty())
-	privateKeyPassword := os.Getenv("GIT_PRIVATEKEY_PASSWORD")
-	g.Expect(privateKeyPassword).NotTo(BeEmpty())
-	oidcClientSecret := os.Getenv("OIDC_CLIENT_SECRET")
-	g.Expect(oidcClientSecret).NotTo(BeEmpty())
-
-	privateKeyFlag := fmt.Sprintf("--private-key=%s", privateKeyFile)
-	privateKeyPasswordFlag := fmt.Sprintf("--private-key-password=%s", privateKeyPassword)
-
-	kubeconfigFlag := fmt.Sprintf("--kubeconfig=%s", kubeconfigPath)
-
-	repoHTTPSURLFlag := fmt.Sprintf("--repo-url=%s", repoURLHTTPS)
-
-	gitUsernameFlag := fmt.Sprintf("--git-username=%s", gitUsername)
-	gitPasswordFlag := fmt.Sprintf("--git-password=%s", gitPassword)
 
 	gitBranchFlag := fmt.Sprintf("--branch=%s", gitBranch)
 	gitRepoPathFlag := fmt.Sprintf("--repo-path=%s", gitRepoPath)
 
+	// oidc configuration
+	oidcClientSecret := os.Getenv("OIDC_CLIENT_SECRET")
+	g.Expect(oidcClientSecret).NotTo(BeEmpty())
+
 	oidcClientSecretFlag := fmt.Sprintf("--client-secret=%s", oidcClientSecret)
 
+	kubeconfigFlag := fmt.Sprintf("--kubeconfig=%s", kubeconfigPath)
+
 	_ = k8sClient.Create(context.Background(), &fluxSystemNamespace)
 
 	tests := []struct {
@@ -147,8 +157,8 @@ func TestBootstrapCmd(t *testing.T) {
 				"--password=admin123",
 				"--discovery-url=https://dex-01.wge.dev.weave.works/.well-known/openid-configuration",
 				"--client-id=weave-gitops-enterprise",
-				gitUsernameFlag, gitPasswordFlag, gitBranchFlag, gitRepoPathFlag,
-				repoHTTPSURLFlag,
+				gitRepoUrlSshNoSchemeFlag, gitBranchFlag, gitRepoPathFlag,
+				privateKeyFlag, privateKeyPasswordFlag,
 				oidcClientSecretFlag, "-s",
 				"--components-extra=policy-agent,tf-controller",
 				"--bootstrap-flux",

diff --git a/docs/cli/bootstrap.md b/docs/cli/bootstrap.md
@@ -375,6 +375,7 @@ Entitlement stage
 - `GIT_PRIVATEKEY_PATH`: path to the private key to do the git operations.
 - `GIT_PRIVATEKEY_PASSWORD`: password protecting access to private key
 - `GIT_REPO_URL_SSH`: git ssh url for the repo wge configuration repo.
+- `GIT_REPO_URL_SSH_NO_SCHEME`: git ssh url for the repo wge configuration repo without scheme like `git@github.com:weaveworks/cli-dev.git` 
 - `GIT_REPO_URL_HTTPS`: git https url for the repo wge configuration repo.
 - `GIT_USERNAME`: git username for testing https auth
 - `GIT_PASSWORD`: git password for testing https auth

diff --git a/pkg/bootstrap/steps/bootstrap_flux_repo_config_test.go b/pkg/bootstrap/steps/bootstrap_flux_repo_config_test.go
@@ -89,7 +89,7 @@ func TestCreateGitRepositoryConfig(t *testing.T) {
 			},
 			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
 				assert.Error(t, err)
-				assert.Contains(t, err.Error(), "unsupported repository scheme: ssl")
+				assert.Contains(t, err.Error(), "invalid repository scheme")
 				return true
 			},
 		},
@@ -110,13 +110,14 @@ func TestCreateGitRepositoryConfig(t *testing.T) {
 				},
 			},
 			config: &Config{
-				GitRepository: GitRepositoryConfig{},
-			},
-			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
-				assert.Error(t, err)
-				assert.Contains(t, err.Error(), "repository scheme cannot be empty")
-				return true
+				GitRepository: GitRepositoryConfig{
+					Url:    "ssh://git@github.com/my-org-name/my-repo-name",
+					Path:   "test/test",
+					Branch: "main",
+					Scheme: sshScheme,
+				},
 			},
+			wantErr: assert.NoError,
 		},
 	}
 	for _, tt := range tests {

diff --git a/pkg/bootstrap/steps/bootstrap_git_repo_config.go b/pkg/bootstrap/steps/bootstrap_git_repo_config.go
@@ -3,6 +3,7 @@ package steps
 import (
 	"fmt"
 	"net/url"
+	"strings"
 )
 
 const (
@@ -54,27 +55,53 @@ type GitRepositoryConfig struct {
 	Scheme string
 }
 
-// NewGitRepositoryConfig creates new configuration out of the user input and discovered state
+// NewGitRepositoryConfig creates new Git repository configuration from valid input parameters.
 func NewGitRepositoryConfig(url string, branch string, path string) (GitRepositoryConfig, error) {
 	var scheme string
 	var err error
+	var normalisedUrl string
 
 	if url != "" {
-		scheme, err = parseRepoScheme(url)
+		normalisedUrl, scheme, err = normaliseUrl(url)
 		if err != nil {
 			return GitRepositoryConfig{}, fmt.Errorf("error parsing repo scheme: %v", err)
 		}
 	}
 
 	return GitRepositoryConfig{
-		Url:    url,
+		Url:    normalisedUrl,
 		Branch: branch,
 		Path:   path,
 		Scheme: scheme,
 	}, nil
 
 }
 
+// normaliseUrl normalises the given url to meet standard URL syntax. The main motivation to have this function
+// is to support Git server URLs in "shorter scp-like syntax for the SSH protocol" as described in https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
+// and followed by popular Git server providers like GitHub (git@github.com:weaveworks/weave-gitops.git) and GitLab (i.e. git@gitlab.com:gitlab-org/gitlab-foss.git).
+// Returns the normalisedUrl, as well the scheme and an error if any.
+func normaliseUrl(repoURL string) (normalisedUrl string, scheme string, err error) {
+	// transform in case of ssh like git@github.com:username/repository.git
+	if strings.Contains(repoURL, "@") && !strings.Contains(repoURL, "://") {
+		repoURL = "ssh://" + strings.Replace(repoURL, ":", "/", 1)
+	}
+
+	repositoryURL, err := url.Parse(repoURL)
+	if err != nil {
+		return "", "", fmt.Errorf("error parsing repository URL: %v", err)
+	}
+
+	switch repositoryURL.Scheme {
+	case sshScheme:
+		return repositoryURL.String(), sshScheme, nil
+	case httpsScheme:
+		return repositoryURL.String(), httpsScheme, nil
+	default:
+		return "", "", fmt.Errorf("invalid repository scheme: %s", repositoryURL.Scheme)
+	}
+}
+
 // NewGitRepositoryConfig step to configure the flux git repository
 func NewGitRepositoryConfigStep(config GitRepositoryConfig) BootstrapStep {
 	// create steps
@@ -138,22 +165,3 @@ func createGitRepositoryConfig(input []StepInput, c *Config) ([]StepOutput, erro
 	c.Logger.Actionf("configured repo: %s", c.GitRepository.Url)
 	return []StepOutput{}, nil
 }
-
-func parseRepoScheme(repoURL string) (string, error) {
-	repositoryURL, err := url.Parse(repoURL)
-	if err != nil {
-		return "", fmt.Errorf("incorrect repository url %s:%v", repoURL, err)
-	}
-	var scheme string
-	switch repositoryURL.Scheme {
-	case "":
-		return "", fmt.Errorf("repository scheme cannot be empty")
-	case sshScheme:
-		scheme = sshScheme
-	case httpsScheme:
-		scheme = httpsScheme
-	default:
-		return "", fmt.Errorf("unsupported repository scheme: %s", repositoryURL.Scheme)
-	}
-	return scheme, nil
-}
diff --git a/pkg/bootstrap/steps/bootstrap_git_repo_config_test.go b/pkg/bootstrap/steps/bootstrap_git_repo_config_test.go
@@ -42,12 +42,13 @@ func TestNewGitRepositoryConfig(t *testing.T) {
 				branch: "main",
 				path:   "clusters/management",
 			},
-			want: GitRepositoryConfig{},
-			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
-				assert.Error(t, err)
-				assert.Contains(t, err.Error(), "repository scheme cannot be empty")
-				return true
+			want: GitRepositoryConfig{
+				Url:    "ssh://git@github.com/example/cli-dev",
+				Branch: "main",
+				Path:   "clusters/management",
+				Scheme: sshScheme,
 			},
+			wantErr: assert.NoError,
 		},
 		{
 			name: "should create config for valid https url ",
@@ -77,3 +78,61 @@ func TestNewGitRepositoryConfig(t *testing.T) {
 		})
 	}
 }
+
+func Test_normaliseUrl(t *testing.T) {
+	tests := []struct {
+		name              string
+		url               string
+		wantNormalisedUrl string
+		wantScheme        string
+		wantErr           assert.ErrorAssertionFunc
+	}{
+		{
+			name:              "should normalise https url with .git",
+			url:               "https://github.com/username/repository.git",
+			wantNormalisedUrl: "https://github.com/username/repository.git",
+			wantScheme:        httpsScheme,
+			wantErr:           assert.NoError,
+		},
+		{
+			name:              "should normalise https url",
+			url:               "https://github.com/username/repository",
+			wantNormalisedUrl: "https://github.com/username/repository",
+			wantScheme:        httpsScheme,
+			wantErr:           assert.NoError,
+		},
+		{
+			name:              "should normalise ssh url without scheme",
+			url:               "git@github.com:weaveworks/weave-gitops.git",
+			wantNormalisedUrl: "ssh://git@github.com/weaveworks/weave-gitops.git",
+			wantScheme:        sshScheme,
+			wantErr:           assert.NoError,
+		},
+		{
+			name:              "should normalise ssh url with scheme",
+			url:               "ssh://git@github.com/username/repository.git",
+			wantNormalisedUrl: "ssh://git@github.com/username/repository.git",
+			wantScheme:        sshScheme,
+			wantErr:           assert.NoError,
+		},
+		{
+			name: "should fail for invalid url",
+			url:  "invalid_url",
+			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), "invalid repository scheme")
+				return true
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotNormalisedUrl, gotScheme, err := normaliseUrl(tt.url)
+			if !tt.wantErr(t, err, fmt.Sprintf("normaliseUrl(%v)", tt.url)) {
+				return
+			}
+			assert.Equalf(t, tt.wantNormalisedUrl, gotNormalisedUrl, "normaliseUrl(%v)", tt.url)
+			assert.Equalf(t, tt.wantScheme, gotScheme, "normaliseUrl(%v)", tt.url)
+		})
+	}
+}
diff --git a/pkg/bootstrap/steps/flux.go b/pkg/bootstrap/steps/flux.go
@@ -34,7 +34,7 @@ func verifyFluxInstallation(input []StepInput, c *Config) ([]StepOutput, error)
 	}
 	c.Logger.Successf(fluxExistingInstallMsg)
 
-	c.Logger.Actionf("verifying flux reconcillation")
+	c.Logger.Actionf("verifying flux reconciliation")
 	out, err = runner.Run("flux", "reconcile", "kustomization", "flux-system")
 	if err != nil {
 		return []StepOutput{}, fmt.Errorf("flux bootstrapped error: %v. %s", string(out), fluxFatalErrorMsg)
@@ -45,7 +45,7 @@ func verifyFluxInstallation(input []StepInput, c *Config) ([]StepOutput, error)
 	if err != nil {
 		return []StepOutput{}, fmt.Errorf("failed to get flux repository: %v", err)
 	}
-	scheme, err := parseRepoScheme(repo.Spec.URL)
+	_, scheme, err := normaliseUrl(repo.Spec.URL)
 	if err != nil {
 		return []StepOutput{}, fmt.Errorf("failed to parse flux repository: %v", err)
 	}