From 3216fef936a5e9922d89281430324b96fcfb98c9 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 12 Sep 2024 12:30:02 +0100 Subject: [PATCH 1/4] feat(git): log parsed gitURL and warn if local --- git/git.go | 20 +++++++++++++++++++- git/git_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/git/git.go b/git/git.go index 1404f089..5a510999 100644 --- a/git/git.go +++ b/git/git.go @@ -40,6 +40,7 @@ type CloneRepoOptions struct { Depth int CABundle []byte ProxyOptions transport.ProxyOptions + Logf log.Func } // CloneRepo will clone the repository at the given URL into the given path. @@ -52,6 +53,7 @@ func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) { if err != nil { return false, fmt.Errorf("parse url %q: %w", opts.RepoURL, err) } + opts.Logf(log.LevelInfo, "#1: Parsed Git URL as %q", parsed.Redacted()) if parsed.Hostname() == "dev.azure.com" { // Azure DevOps requires capabilities multi_ack / multi_ack_detailed, // which are not fully implemented and by default are included in @@ -73,6 +75,7 @@ func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) { transport.UnsupportedCapabilities = []capability.Capability{ capability.ThinPack, } + opts.Logf(log.LevelInfo, "#1: Workaround for Azure DevOps: marking thin-pack as unsupported") } err = opts.Storage.MkdirAll(opts.Path, 0o755) @@ -219,7 +222,12 @@ func SetupRepoAuth(options *options.Options) transport.AuthMethod { options.Logger(log.LevelInfo, "#1: ❔ No Git URL supplied!") return nil } - if strings.HasPrefix(options.GitURL, "http://") || strings.HasPrefix(options.GitURL, "https://") { + parsedURL, err := giturls.Parse(options.GitURL) + if err != nil { + options.Logger(log.LevelError, "#1: ❌ Failed to parse Git URL: %s", err.Error()) + } + + if parsedURL.Scheme == "http" || parsedURL.Scheme == "https" { // Special case: no auth if options.GitUsername == "" && options.GitPassword == "" { options.Logger(log.LevelInfo, "#1: 👤 Using no authentication!") @@ -235,6 +243,15 @@ func SetupRepoAuth(options *options.Options) transport.AuthMethod { } } + if parsedURL.Scheme == "file" { + // go-git will try to fallback to using the `git` command for local + // filesystem clones. However, it's more likely than not that the + // `git` command is not present in the container image. Log a warning + // but continue. Also, no auth. + options.Logger(log.LevelWarn, "#1: 🚧 Using local filesystem clone! This requires the git executable to be present!") + return nil + } + // Generally git clones over SSH use the 'git' user, but respect // GIT_USERNAME if set. if options.GitUsername == "" { @@ -302,6 +319,7 @@ func CloneOptionsFromOptions(options options.Options) (CloneRepoOptions, error) SingleBranch: options.GitCloneSingleBranch, Depth: int(options.GitCloneDepth), CABundle: caBundle, + Logf: options.Logger, } cloneOpts.RepoAuth = SetupRepoAuth(&options) diff --git a/git/git_test.go b/git/git_test.go index 14656886..af369412 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -95,6 +95,7 @@ func TestCloneRepo(t *testing.T) { Path: "/", RepoURL: srv.URL, Storage: clientFS, + Logf: testLog(t), }) require.NoError(t, err) require.False(t, cloned) @@ -117,6 +118,7 @@ func TestCloneRepo(t *testing.T) { Username: tc.username, Password: tc.password, }, + Logf: testLog(t), }) require.Equal(t, tc.expectClone, cloned) if tc.expectError != "" { @@ -150,6 +152,7 @@ func TestCloneRepo(t *testing.T) { Path: "/workspace", RepoURL: authURL.String(), Storage: clientFS, + Logf: testLog(t), }) require.Equal(t, tc.expectClone, cloned) if tc.expectError != "" { @@ -199,6 +202,7 @@ func TestShallowCloneRepo(t *testing.T) { Username: "test", Password: "test", }, + Logf: testLog(t), }) require.Error(t, err) }) @@ -227,6 +231,7 @@ func TestShallowCloneRepo(t *testing.T) { Username: "test", Password: "test", }, + Logf: testLog(t), }) require.NoError(t, err) for _, path := range []string{"README.md", "foo", "baz"} { @@ -264,6 +269,7 @@ func TestCloneRepoSSH(t *testing.T) { HostKeyCallback: gossh.InsecureIgnoreHostKey(), }, }, + Logf: testLog(t), }) // TODO: ideally, we want to test the entire cloning flow. // For now, this indicates successful ssh key auth. @@ -296,6 +302,7 @@ func TestCloneRepoSSH(t *testing.T) { HostKeyCallback: gossh.InsecureIgnoreHostKey(), }, }, + Logf: testLog(t), }) require.ErrorContains(t, err, "handshake failed") require.False(t, cloned) @@ -325,6 +332,7 @@ func TestCloneRepoSSH(t *testing.T) { HostKeyCallback: gossh.FixedHostKey(randKeygen(t).PublicKey()), }, }, + Logf: testLog(t), }) require.ErrorContains(t, err, "ssh: host key mismatch") require.False(t, cloned) @@ -453,6 +461,33 @@ func TestSetupRepoAuth(t *testing.T) { auth := git.SetupRepoAuth(opts) require.Nil(t, auth) // TODO: actually test SSH_AUTH_SOCK }) + + t.Run("NoHostname/RepoOnly", func(t *testing.T) { + opts := &options.Options{ + GitURL: "repo", + Logger: testLog(t), + } + auth := git.SetupRepoAuth(opts) + require.Nil(t, auth) + }) + + t.Run("NoHostname/Org/Repo", func(t *testing.T) { + opts := &options.Options{ + GitURL: "org/repo", + Logger: testLog(t), + } + auth := git.SetupRepoAuth(opts) + require.Nil(t, auth) + }) + + t.Run("NoHostname/AbsolutePathish", func(t *testing.T) { + opts := &options.Options{ + GitURL: "/org/repo", + Logger: testLog(t), + } + auth := git.SetupRepoAuth(opts) + require.Nil(t, auth) + }) } func mustRead(t *testing.T, fs billy.Filesystem, path string) string { From b212f96cae71dc7ec050fe646e0e9ea281e79335 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 12 Sep 2024 12:45:31 +0100 Subject: [PATCH 2/4] fixup! feat(git): log parsed gitURL and warn if local --- git/git.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git/git.go b/git/git.go index 5a510999..536bab59 100644 --- a/git/git.go +++ b/git/git.go @@ -206,6 +206,8 @@ func LogHostKeyCallback(logger log.Func) gossh.HostKeyCallback { // | https?://host.tld/repo | Not Set | Set | HTTP Basic | // | https?://host.tld/repo | Set | Not Set | HTTP Basic | // | https?://host.tld/repo | Set | Set | HTTP Basic | +// | file://path/to/repo | - | - | None | +// | path/to/repo | - | - | None | // | All other formats | - | - | SSH | // // For SSH authentication, the default username is "git" but will honour @@ -225,6 +227,7 @@ func SetupRepoAuth(options *options.Options) transport.AuthMethod { parsedURL, err := giturls.Parse(options.GitURL) if err != nil { options.Logger(log.LevelError, "#1: ❌ Failed to parse Git URL: %s", err.Error()) + return nil } if parsedURL.Scheme == "http" || parsedURL.Scheme == "https" { From 15979f2b6194bd0b25268ce30cc6057774b9978e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 12 Sep 2024 14:28:13 +0100 Subject: [PATCH 3/4] address PR comments --- envbuilder.go | 56 +++++++++++++++++++----------------- git/git.go | 53 ++++++++++++++++------------------ git/git_test.go | 76 +++++++++++++++---------------------------------- 3 files changed, 78 insertions(+), 107 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 07b34807..8d744ff0 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -112,22 +112,25 @@ func Run(ctx context.Context, opts options.Options) error { var fallbackErr error var cloned bool if opts.GitURL != "" { - cloneOpts, err := git.CloneOptionsFromOptions(opts) - if err != nil { - return fmt.Errorf("git clone options: %w", err) - } - endStage := startStage("📦 Cloning %s to %s...", newColor(color.FgCyan).Sprintf(opts.GitURL), - newColor(color.FgCyan).Sprintf(cloneOpts.Path), + newColor(color.FgCyan).Sprintf(opts.WorkspaceFolder), ) - stageNum := stageNumber - w := git.ProgressWriter(func(line string) { opts.Logger(log.LevelInfo, "#%d: %s", stageNum, line) }) + logStage := func(format string, args ...any) { + opts.Logger(log.LevelInfo, "#%d: %s", stageNum, fmt.Sprintf(format, args...)) + } + + cloneOpts, err := git.CloneOptionsFromOptions(logStage, opts) + if err != nil { + return fmt.Errorf("git clone options: %w", err) + } + + w := git.ProgressWriter(func(line string) { logStage(line) }) defer w.Close() cloneOpts.Progress = w - cloned, fallbackErr = git.CloneRepo(ctx, cloneOpts) + cloned, fallbackErr = git.CloneRepo(ctx, logStage, cloneOpts) if fallbackErr == nil { if cloned { endStage("📦 Cloned repository!") @@ -144,7 +147,7 @@ func Run(ctx context.Context, opts options.Options) error { // Always clone the repo in remote repo build mode into a location that // we control that isn't affected by the users changes. if opts.RemoteRepoBuildMode { - cloneOpts, err := git.CloneOptionsFromOptions(opts) + cloneOpts, err := git.CloneOptionsFromOptions(logStage, opts) if err != nil { return fmt.Errorf("git clone options: %w", err) } @@ -155,12 +158,11 @@ func Run(ctx context.Context, opts options.Options) error { newColor(color.FgCyan).Sprintf(cloneOpts.Path), ) - stageNum := stageNumber - w := git.ProgressWriter(func(line string) { opts.Logger(log.LevelInfo, "#%d: %s", stageNum, line) }) + w := git.ProgressWriter(func(line string) { logStage(line) }) defer w.Close() cloneOpts.Progress = w - fallbackErr = git.ShallowCloneRepo(ctx, cloneOpts) + fallbackErr = git.ShallowCloneRepo(ctx, logStage, cloneOpts) if fallbackErr == nil { endStage("📦 Cloned repository!") buildTimeWorkspaceFolder = cloneOpts.Path @@ -891,25 +893,28 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error) var fallbackErr error var cloned bool if opts.GitURL != "" { + endStage := startStage("📦 Cloning %s to %s...", + newColor(color.FgCyan).Sprintf(opts.GitURL), + newColor(color.FgCyan).Sprintf(opts.WorkspaceFolder), + ) + stageNum := stageNumber + logStage := func(format string, args ...any) { + opts.Logger(log.LevelInfo, "#%d: %s", stageNum, fmt.Sprintf(format, args...)) + } + // In cache probe mode we should only attempt to clone the full // repository if remote repo build mode isn't enabled. if !opts.RemoteRepoBuildMode { - cloneOpts, err := git.CloneOptionsFromOptions(opts) + cloneOpts, err := git.CloneOptionsFromOptions(logStage, opts) if err != nil { return nil, fmt.Errorf("git clone options: %w", err) } - endStage := startStage("📦 Cloning %s to %s...", - newColor(color.FgCyan).Sprintf(opts.GitURL), - newColor(color.FgCyan).Sprintf(cloneOpts.Path), - ) - - stageNum := stageNumber - w := git.ProgressWriter(func(line string) { opts.Logger(log.LevelInfo, "#%d: %s", stageNum, line) }) + w := git.ProgressWriter(func(line string) { logStage(line) }) defer w.Close() cloneOpts.Progress = w - cloned, fallbackErr = git.CloneRepo(ctx, cloneOpts) + cloned, fallbackErr = git.CloneRepo(ctx, logStage, cloneOpts) if fallbackErr == nil { if cloned { endStage("📦 Cloned repository!") @@ -923,7 +928,7 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error) _ = w.Close() } else { - cloneOpts, err := git.CloneOptionsFromOptions(opts) + cloneOpts, err := git.CloneOptionsFromOptions(logStage, opts) if err != nil { return nil, fmt.Errorf("git clone options: %w", err) } @@ -934,12 +939,11 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error) newColor(color.FgCyan).Sprintf(cloneOpts.Path), ) - stageNum := stageNumber - w := git.ProgressWriter(func(line string) { opts.Logger(log.LevelInfo, "#%d: %s", stageNum, line) }) + w := git.ProgressWriter(func(line string) { logStage(line) }) defer w.Close() cloneOpts.Progress = w - fallbackErr = git.ShallowCloneRepo(ctx, cloneOpts) + fallbackErr = git.ShallowCloneRepo(ctx, logStage, cloneOpts) if fallbackErr == nil { endStage("📦 Cloned repository!") buildTimeWorkspaceFolder = cloneOpts.Path diff --git a/git/git.go b/git/git.go index 536bab59..14cc0ebe 100644 --- a/git/git.go +++ b/git/git.go @@ -12,7 +12,6 @@ import ( "github.com/coder/envbuilder/options" giturls "github.com/chainguard-dev/git-urls" - "github.com/coder/envbuilder/log" "github.com/go-git/go-billy/v5" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" @@ -40,7 +39,6 @@ type CloneRepoOptions struct { Depth int CABundle []byte ProxyOptions transport.ProxyOptions - Logf log.Func } // CloneRepo will clone the repository at the given URL into the given path. @@ -48,12 +46,12 @@ type CloneRepoOptions struct { // be cloned again. // // The bool returned states whether the repository was cloned or not. -func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) { +func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOptions) (bool, error) { parsed, err := giturls.Parse(opts.RepoURL) if err != nil { return false, fmt.Errorf("parse url %q: %w", opts.RepoURL, err) } - opts.Logf(log.LevelInfo, "#1: Parsed Git URL as %q", parsed.Redacted()) + logf("Parsed Git URL as %q", parsed.Redacted()) if parsed.Hostname() == "dev.azure.com" { // Azure DevOps requires capabilities multi_ack / multi_ack_detailed, // which are not fully implemented and by default are included in @@ -75,7 +73,7 @@ func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) { transport.UnsupportedCapabilities = []capability.Capability{ capability.ThinPack, } - opts.Logf(log.LevelInfo, "#1: Workaround for Azure DevOps: marking thin-pack as unsupported") + logf("Workaround for Azure DevOps: marking thin-pack as unsupported") } err = opts.Storage.MkdirAll(opts.Path, 0o755) @@ -134,7 +132,7 @@ func CloneRepo(ctx context.Context, opts CloneRepoOptions) (bool, error) { // clone will not be performed. // // The bool returned states whether the repository was cloned or not. -func ShallowCloneRepo(ctx context.Context, opts CloneRepoOptions) error { +func ShallowCloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOptions) error { opts.Depth = 1 opts.SingleBranch = true @@ -153,7 +151,7 @@ func ShallowCloneRepo(ctx context.Context, opts CloneRepoOptions) error { } } - cloned, err := CloneRepo(ctx, opts) + cloned, err := CloneRepo(ctx, logf, opts) if err != nil { return err } @@ -185,14 +183,14 @@ func ReadPrivateKey(path string) (gossh.Signer, error) { // LogHostKeyCallback is a HostKeyCallback that just logs host keys // and does nothing else. -func LogHostKeyCallback(logger log.Func) gossh.HostKeyCallback { +func LogHostKeyCallback(logger func(string, ...any)) gossh.HostKeyCallback { return func(hostname string, remote net.Addr, key gossh.PublicKey) error { var sb strings.Builder _ = knownhosts.WriteKnownHost(&sb, hostname, remote, key) // skeema/knownhosts uses a fake public key to determine the host key // algorithms. Ignore this one. if s := sb.String(); !strings.Contains(s, "fake-public-key ZmFrZSBwdWJsaWMga2V5") { - logger(log.LevelInfo, "#1: 🔑 Got host key: %s", strings.TrimSpace(s)) + logger("🔑 Got host key: %s", strings.TrimSpace(s)) } return nil } @@ -219,27 +217,27 @@ func LogHostKeyCallback(logger log.Func) gossh.HostKeyCallback { // If SSH_KNOWN_HOSTS is not set, the SSH auth method will be configured // to accept and log all host keys. Otherwise, host key checking will be // performed as usual. -func SetupRepoAuth(options *options.Options) transport.AuthMethod { +func SetupRepoAuth(logf func(string, ...any), options *options.Options) transport.AuthMethod { if options.GitURL == "" { - options.Logger(log.LevelInfo, "#1: ❔ No Git URL supplied!") + logf("❔ No Git URL supplied!") return nil } parsedURL, err := giturls.Parse(options.GitURL) if err != nil { - options.Logger(log.LevelError, "#1: ❌ Failed to parse Git URL: %s", err.Error()) + logf("❌ Failed to parse Git URL: %s", err.Error()) return nil } if parsedURL.Scheme == "http" || parsedURL.Scheme == "https" { // Special case: no auth if options.GitUsername == "" && options.GitPassword == "" { - options.Logger(log.LevelInfo, "#1: 👤 Using no authentication!") + logf("👤 Using no authentication!") return nil } // Basic Auth // NOTE: we previously inserted the credentials into the repo URL. // This was removed in https://github.com/coder/envbuilder/pull/141 - options.Logger(log.LevelInfo, "#1: 🔒 Using HTTP basic authentication!") + logf("🔒 Using HTTP basic authentication!") return &githttp.BasicAuth{ Username: options.GitUsername, Password: options.GitPassword, @@ -251,7 +249,7 @@ func SetupRepoAuth(options *options.Options) transport.AuthMethod { // filesystem clones. However, it's more likely than not that the // `git` command is not present in the container image. Log a warning // but continue. Also, no auth. - options.Logger(log.LevelWarn, "#1: 🚧 Using local filesystem clone! This requires the git executable to be present!") + logf("🚧 Using local filesystem clone! This requires the git executable to be present!") return nil } @@ -262,30 +260,30 @@ func SetupRepoAuth(options *options.Options) transport.AuthMethod { } // Assume SSH auth for all other formats. - options.Logger(log.LevelInfo, "#1: 🔑 Using SSH authentication!") + logf("🔑 Using SSH authentication!") var signer ssh.Signer if options.GitSSHPrivateKeyPath != "" { s, err := ReadPrivateKey(options.GitSSHPrivateKeyPath) if err != nil { - options.Logger(log.LevelError, "#1: ❌ Failed to read private key from %s: %s", options.GitSSHPrivateKeyPath, err.Error()) + logf("❌ Failed to read private key from %s: %s", options.GitSSHPrivateKeyPath, err.Error()) } else { - options.Logger(log.LevelInfo, "#1: 🔑 Using %s key!", s.PublicKey().Type()) + logf("🔑 Using %s key!", s.PublicKey().Type()) signer = s } } // If no SSH key set, fall back to agent auth. if signer == nil { - options.Logger(log.LevelError, "#1: 🔑 No SSH key found, falling back to agent!") + logf("🔑 No SSH key found, falling back to agent!") auth, err := gitssh.NewSSHAgentAuth(options.GitUsername) if err != nil { - options.Logger(log.LevelError, "#1: ❌ Failed to connect to SSH agent: %s", err.Error()) + logf("❌ Failed to connect to SSH agent: " + err.Error()) return nil // nothing else we can do } if os.Getenv("SSH_KNOWN_HOSTS") == "" { - options.Logger(log.LevelWarn, "#1: 🔓 SSH_KNOWN_HOSTS not set, accepting all host keys!") - auth.HostKeyCallback = LogHostKeyCallback(options.Logger) + logf("🔓 SSH_KNOWN_HOSTS not set, accepting all host keys!") + auth.HostKeyCallback = LogHostKeyCallback(logf) } return auth } @@ -303,35 +301,34 @@ func SetupRepoAuth(options *options.Options) transport.AuthMethod { // Duplicated code due to Go's type system. if os.Getenv("SSH_KNOWN_HOSTS") == "" { - options.Logger(log.LevelWarn, "#1: 🔓 SSH_KNOWN_HOSTS not set, accepting all host keys!") - auth.HostKeyCallback = LogHostKeyCallback(options.Logger) + logf("🔓 SSH_KNOWN_HOSTS not set, accepting all host keys!") + auth.HostKeyCallback = LogHostKeyCallback(logf) } return auth } -func CloneOptionsFromOptions(options options.Options) (CloneRepoOptions, error) { +func CloneOptionsFromOptions(logf func(string, ...any), options options.Options) (CloneRepoOptions, error) { caBundle, err := options.CABundle() if err != nil { return CloneRepoOptions{}, err } cloneOpts := CloneRepoOptions{ + RepoURL: options.GitURL, Path: options.WorkspaceFolder, Storage: options.Filesystem, Insecure: options.Insecure, SingleBranch: options.GitCloneSingleBranch, Depth: int(options.GitCloneDepth), CABundle: caBundle, - Logf: options.Logger, } - cloneOpts.RepoAuth = SetupRepoAuth(&options) + cloneOpts.RepoAuth = SetupRepoAuth(logf, &options) if options.GitHTTPProxyURL != "" { cloneOpts.ProxyOptions = transport.ProxyOptions{ URL: options.GitHTTPProxyURL, } } - cloneOpts.RepoURL = options.GitURL return cloneOpts, nil } diff --git a/git/git_test.go b/git/git_test.go index af369412..e7a58f90 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -13,12 +13,10 @@ import ( "testing" "github.com/coder/envbuilder/git" - "github.com/coder/envbuilder/options" - - "github.com/coder/envbuilder/log" "github.com/coder/envbuilder/testutil/gittest" "github.com/coder/envbuilder/testutil/mwtest" + "github.com/go-git/go-billy/v5" "github.com/go-git/go-billy/v5/memfs" "github.com/go-git/go-billy/v5/osfs" @@ -91,11 +89,10 @@ func TestCloneRepo(t *testing.T) { clientFS := memfs.New() // A repo already exists! _ = gittest.NewRepo(t, clientFS) - cloned, err := git.CloneRepo(context.Background(), git.CloneRepoOptions{ + cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/", RepoURL: srv.URL, Storage: clientFS, - Logf: testLog(t), }) require.NoError(t, err) require.False(t, cloned) @@ -110,7 +107,7 @@ func TestCloneRepo(t *testing.T) { srv := httptest.NewServer(authMW(gittest.NewServer(srvFS))) clientFS := memfs.New() - cloned, err := git.CloneRepo(context.Background(), git.CloneRepoOptions{ + cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/workspace", RepoURL: srv.URL, Storage: clientFS, @@ -118,7 +115,6 @@ func TestCloneRepo(t *testing.T) { Username: tc.username, Password: tc.password, }, - Logf: testLog(t), }) require.Equal(t, tc.expectClone, cloned) if tc.expectError != "" { @@ -148,11 +144,10 @@ func TestCloneRepo(t *testing.T) { authURL.User = url.UserPassword(tc.username, tc.password) clientFS := memfs.New() - cloned, err := git.CloneRepo(context.Background(), git.CloneRepoOptions{ + cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/workspace", RepoURL: authURL.String(), Storage: clientFS, - Logf: testLog(t), }) require.Equal(t, tc.expectClone, cloned) if tc.expectError != "" { @@ -194,7 +189,7 @@ func TestShallowCloneRepo(t *testing.T) { require.NoError(t, err) require.NoError(t, f.Close()) - err = git.ShallowCloneRepo(context.Background(), git.CloneRepoOptions{ + err = git.ShallowCloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/repo", RepoURL: srv.URL, Storage: clientFS, @@ -202,7 +197,6 @@ func TestShallowCloneRepo(t *testing.T) { Username: "test", Password: "test", }, - Logf: testLog(t), }) require.Error(t, err) }) @@ -223,7 +217,7 @@ func TestShallowCloneRepo(t *testing.T) { clientFS := memfs.New() - err := git.ShallowCloneRepo(context.Background(), git.CloneRepoOptions{ + err := git.ShallowCloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/repo", RepoURL: srv.URL, Storage: clientFS, @@ -231,7 +225,6 @@ func TestShallowCloneRepo(t *testing.T) { Username: "test", Password: "test", }, - Logf: testLog(t), }) require.NoError(t, err) for _, path := range []string{"README.md", "foo", "baz"} { @@ -257,7 +250,7 @@ func TestCloneRepoSSH(t *testing.T) { gitURL := tr.String() clientFS := memfs.New() - cloned, err := git.CloneRepo(context.Background(), git.CloneRepoOptions{ + cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/workspace", RepoURL: gitURL, Storage: clientFS, @@ -269,7 +262,6 @@ func TestCloneRepoSSH(t *testing.T) { HostKeyCallback: gossh.InsecureIgnoreHostKey(), }, }, - Logf: testLog(t), }) // TODO: ideally, we want to test the entire cloning flow. // For now, this indicates successful ssh key auth. @@ -290,7 +282,7 @@ func TestCloneRepoSSH(t *testing.T) { clientFS := memfs.New() anotherKey := randKeygen(t) - cloned, err := git.CloneRepo(context.Background(), git.CloneRepoOptions{ + cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/workspace", RepoURL: gitURL, Storage: clientFS, @@ -302,7 +294,6 @@ func TestCloneRepoSSH(t *testing.T) { HostKeyCallback: gossh.InsecureIgnoreHostKey(), }, }, - Logf: testLog(t), }) require.ErrorContains(t, err, "handshake failed") require.False(t, cloned) @@ -321,7 +312,7 @@ func TestCloneRepoSSH(t *testing.T) { gitURL := tr.String() clientFS := memfs.New() - cloned, err := git.CloneRepo(context.Background(), git.CloneRepoOptions{ + cloned, err := git.CloneRepo(context.Background(), t.Logf, git.CloneRepoOptions{ Path: "/workspace", RepoURL: gitURL, Storage: clientFS, @@ -332,7 +323,6 @@ func TestCloneRepoSSH(t *testing.T) { HostKeyCallback: gossh.FixedHostKey(randKeygen(t).PublicKey()), }, }, - Logf: testLog(t), }) require.ErrorContains(t, err, "ssh: host key mismatch") require.False(t, cloned) @@ -343,19 +333,16 @@ func TestCloneRepoSSH(t *testing.T) { func TestSetupRepoAuth(t *testing.T) { t.Setenv("SSH_AUTH_SOCK", "") t.Run("Empty", func(t *testing.T) { - opts := &options.Options{ - Logger: testLog(t), - } - auth := git.SetupRepoAuth(opts) + opts := &options.Options{} + auth := git.SetupRepoAuth(t.Logf, opts) require.Nil(t, auth) }) t.Run("HTTP/NoAuth", func(t *testing.T) { opts := &options.Options{ GitURL: "http://host.tld/repo", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) require.Nil(t, auth) }) @@ -364,9 +351,8 @@ func TestSetupRepoAuth(t *testing.T) { GitURL: "http://host.tld/repo", GitUsername: "user", GitPassword: "pass", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) ba, ok := auth.(*githttp.BasicAuth) require.True(t, ok) require.Equal(t, opts.GitUsername, ba.Username) @@ -378,9 +364,8 @@ func TestSetupRepoAuth(t *testing.T) { GitURL: "https://host.tld/repo", GitUsername: "user", GitPassword: "pass", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) ba, ok := auth.(*githttp.BasicAuth) require.True(t, ok) require.Equal(t, opts.GitUsername, ba.Username) @@ -392,9 +377,8 @@ func TestSetupRepoAuth(t *testing.T) { opts := &options.Options{ GitURL: "ssh://host.tld/repo", GitSSHPrivateKeyPath: kPath, - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) _, ok := auth.(*gitssh.PublicKeys) require.True(t, ok) }) @@ -404,9 +388,8 @@ func TestSetupRepoAuth(t *testing.T) { opts := &options.Options{ GitURL: "git@host.tld:repo/path", GitSSHPrivateKeyPath: kPath, - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) _, ok := auth.(*gitssh.PublicKeys) require.True(t, ok) }) @@ -417,9 +400,8 @@ func TestSetupRepoAuth(t *testing.T) { opts := &options.Options{ GitURL: "git://git@host.tld:repo/path", GitSSHPrivateKeyPath: kPath, - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) _, ok := auth.(*gitssh.PublicKeys) require.True(t, ok) }) @@ -430,9 +412,8 @@ func TestSetupRepoAuth(t *testing.T) { GitURL: "host.tld:12345/repo/path", GitSSHPrivateKeyPath: kPath, GitUsername: "user", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) _, ok := auth.(*gitssh.PublicKeys) require.True(t, ok) }) @@ -442,9 +423,8 @@ func TestSetupRepoAuth(t *testing.T) { opts := &options.Options{ GitURL: "ssh://git@host.tld:repo/path", GitSSHPrivateKeyPath: kPath, - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) pk, ok := auth.(*gitssh.PublicKeys) require.True(t, ok) require.NotNil(t, pk.Signer) @@ -456,36 +436,32 @@ func TestSetupRepoAuth(t *testing.T) { t.Run("SSH/NoAuthMethods", func(t *testing.T) { opts := &options.Options{ GitURL: "ssh://git@host.tld:repo/path", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) require.Nil(t, auth) // TODO: actually test SSH_AUTH_SOCK }) t.Run("NoHostname/RepoOnly", func(t *testing.T) { opts := &options.Options{ GitURL: "repo", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) require.Nil(t, auth) }) t.Run("NoHostname/Org/Repo", func(t *testing.T) { opts := &options.Options{ GitURL: "org/repo", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) require.Nil(t, auth) }) t.Run("NoHostname/AbsolutePathish", func(t *testing.T) { opts := &options.Options{ GitURL: "/org/repo", - Logger: testLog(t), } - auth := git.SetupRepoAuth(opts) + auth := git.SetupRepoAuth(t.Logf, opts) require.Nil(t, auth) }) } @@ -509,12 +485,6 @@ func randKeygen(t *testing.T) gossh.Signer { return signer } -func testLog(t *testing.T) log.Func { - return func(_ log.Level, format string, args ...interface{}) { - t.Logf(format, args...) - } -} - // nolint:gosec // Throw-away key for testing. DO NOT REUSE. var testKey = `-----BEGIN OPENSSH PRIVATE KEY----- b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW From 9c62de0b86505d22371b2cbcff20794e08ce8e60 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 12 Sep 2024 14:36:49 +0100 Subject: [PATCH 4/4] fix %(MISSING) in ProgressWriter due to fmt.Sprintf --- envbuilder.go | 8 ++++---- git/git.go | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 8d744ff0..3df35622 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -126,7 +126,7 @@ func Run(ctx context.Context, opts options.Options) error { return fmt.Errorf("git clone options: %w", err) } - w := git.ProgressWriter(func(line string) { logStage(line) }) + w := git.ProgressWriter(logStage) defer w.Close() cloneOpts.Progress = w @@ -158,7 +158,7 @@ func Run(ctx context.Context, opts options.Options) error { newColor(color.FgCyan).Sprintf(cloneOpts.Path), ) - w := git.ProgressWriter(func(line string) { logStage(line) }) + w := git.ProgressWriter(logStage) defer w.Close() cloneOpts.Progress = w @@ -910,7 +910,7 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error) return nil, fmt.Errorf("git clone options: %w", err) } - w := git.ProgressWriter(func(line string) { logStage(line) }) + w := git.ProgressWriter(logStage) defer w.Close() cloneOpts.Progress = w @@ -939,7 +939,7 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error) newColor(color.FgCyan).Sprintf(cloneOpts.Path), ) - w := git.ProgressWriter(func(line string) { logStage(line) }) + w := git.ProgressWriter(logStage) defer w.Close() cloneOpts.Progress = w diff --git a/git/git.go b/git/git.go index 14cc0ebe..7d132c3a 100644 --- a/git/git.go +++ b/git/git.go @@ -349,7 +349,7 @@ func (w *progressWriter) Close() error { return err2 } -func ProgressWriter(write func(line string)) io.WriteCloser { +func ProgressWriter(write func(line string, args ...any)) io.WriteCloser { reader, writer := io.Pipe() done := make(chan struct{}) go func() { @@ -365,6 +365,8 @@ func ProgressWriter(write func(line string)) io.WriteCloser { if line == "" { continue } + // Escape % signs so that they don't get interpreted as format specifiers + line = strings.Replace(line, "%", "%%", -1) write(strings.TrimSpace(line)) } }