feat: Add gomod git auth #509

pmengelbert · 2025-01-29T19:38:55Z

See Commit Descriptions

source_test.go

test/testenv/buildx.go

source_test.go

generator_gomod.go

pmengelbert · 2025-02-03T15:57:07Z

Write docs for feature, including auto-propagating the auth from the git source

cpuguy83 · 2025-02-07T00:54:34Z

generator_gomod.go

@@ -1,14 +1,20 @@
 package dalec

 import (
+	"bytes"
+	_ "embed"


Looks like this is unused?

cpuguy83 · 2025-02-07T00:56:18Z

generator_gomod.go

-	gomodCacheDir = "/go/pkg/mod"
+	gomodCacheDir       = "/go/pkg/mod"
+	gitConfigMountpoint = "/dev/shm/git"
+	scriptRelativePath  = "/go_mod_download.sh"


Can you give this variable a better name?
Truly, I think you can define the const inside the function you are using it.

cpuguy83 · 2025-02-07T00:57:25Z

generator_gomod.go

+		const (
+			fourKB           = 4096
+			workDir          = "/work/src"
+			scriptMountPoint = "/tmp/mnt"


Let's give this a better path to make sure it's not overwriting anything in the builder image (just in case).
In other places I've done something like /tmp/dalec/internal/...

cpuguy83 · 2025-02-11T00:10:46Z

generator_gomod.go

@@ -54,6 +72,101 @@ func withGomod(g *SourceGenerator, srcSt, worker llb.State, opts ...llb.Constrai
 	}
 }

+func (g *SourceGenerator) gitconfigGeneratorScript() llb.State {


Have you considered utilizing GIT_ASKPASS or just more generally git credential helpers?
I think it would probably be worthwhile to handle secrets with a helper of some sort that can just read secrets directly from /run/secrets (where buildkit puts them) instead of trying to deal with the configuration file.

AFAIK GIT_ASKPASS won't work with github sources since username/password authentication is not allowed by github. I'll look into the credential helpers.

TIL about GIT_ASKPASS

source.go

pmengelbert · 2025-02-13T21:01:40Z

Fix rebase disaster.

MorrisLaw

looks good for my change requests; just have to get Brian G's approval too

pmengelbert · 2025-02-21T18:31:13Z

@cpuguy83 This should be ready for another round of review. I'll probably have to modify the tests, but I'd like to get your thoughts on the overall approach.

MorrisLaw · 2025-02-21T19:05:17Z

source.go

+	gm.Auth[host] = gomodAuth
+}
+
+// func (gm *GeneratorGomod) fillDefaults


cpuguy83 · 2025-02-25T17:47:09Z

cmd/frontend/git_credential_gomod.go

+	var errs []error
+	fill := func(k, v string) {
+		if v != "" {
+			if _, err := fmt.Fprintln(&buf, k+"="+v); err != nil {


Note that bytes.Buffer will never return an error on write and this can all be simplified to not bother with error handling.

cpuguy83 · 2025-02-25T17:58:14Z

frontend/build.go

+	return llb.Scratch().File(llb.Copy(f, srcPath, dalec.GitCredentialHelperGomod)), nil
+}
+
+func GomodGitCredentialHelperGetter(client gwclient.Client) func() (llb.State, error) {


Can this just be a closer in gateway.go rather than giving it an exported function?

cpuguy83 · 2025-02-25T17:59:14Z

cmd/frontend/main.go

 )

 func main() {
+	cmd := filepath.Base(os.Args[0])


Was it not possible to tell git to use a subcommand?

Fixed this. There are now 2 subcommands, frontend and credential-helper. If the subcommand is blank, it behaves as though the subcommand were frontend.

cpuguy83 · 2025-02-25T18:00:21Z

generator_gomod.go

 	return func(in llb.State) llb.State {
-		workDir := "/work/src"
+		const (
+			fourKB                       = 4096


Doesn't look like this is used anywhere?

Removed this and other unused consts.

cpuguy83 · 2025-02-25T18:01:27Z

generator_gomod.go

+		}
+
+		var b bytes.Buffer
+		enc := gob.NewEncoder(&b)


to avoid yaml messiness -- we've already got a go struct, why not keep it that way rather than translating it into another form and back?

Mostly because its a weird encoding and is actually less efficient than the stdlib json encoding.

Fixed this.

By using the filepath to convey the necessary information.

cpuguy83 · 2025-02-25T18:04:26Z

generator_gomod.go

+			continue
+		}
+
+		fmt.Fprintf(&script, `git config --global credential."https://%s".helper "gomod %s"`, host, configPath)


Let's give this a name that's more dalec specific?
dalec-git-credentials?

You can give it an absolute path also, so I'm just giving it the path to the frontend.

cpuguy83 · 2025-02-25T18:08:35Z

generator_gomod.go

+		}
+
+		for secret := range secrets {
+			llb.AddSecret("/run/secrets/"+secret, llb.SecretID(secret)).SetRunOption(ei)


What it help simplify things to mount the secret such that we can find it by host name instead of having to encode configs?
e.g. something like /run/secrets/dalec-git-auth/<host>

I was thinking the same thing earlier today. I'll set the destination path on the secrets instead.

We'll need to encode the auth type also (token or header).

cpuguy83 · 2025-02-25T18:10:27Z

source.go

@@ -18,6 +19,11 @@ import (
 	"github.com/pkg/errors"
 )

+const (
+	GitCredentialHelperGomod       = "git-credential-gomod"
+	GitCredentialHelperGomodSrcDir = "/usr/bin"


Doesn't seem like this is used.

Removed this and others.

Doesn't seem like this got removed?

cpuguy83 · 2025-02-25T18:11:58Z

source.go

+	Resolver             llb.ImageMetaResolver
+	Forward              ForwarderFunc
+	GetContext           func(string, ...llb.LocalOption) (*llb.State, error)
+	GitCredentialHelpers map[string]GitCredHelperGetter


Shouldn't there only ever be one helper here and it doesn't seem like it should be tied to any particular need (e.g. gomod).
This is how we make our buildkit secrets available to git, shouldn't need much more than that.

Changed this to a func() (llb.RunOption, error). The error is thrown if the frontend state can't be acquired. The runopt mounts the frontend at /usr/local/bin/frontend and can be used anywhere the credential helper is needed.

cpuguy83

Let's think about how we can test these changes in the integration suite.
I think if we can run a service on the host that is accessible to buildkit it shouldn't be a problem (other than just setting up multiple different git services).

cpuguy83 · 2025-02-27T00:56:48Z

cmd/frontend/main.go

+		args := flag.Args()[2:]
+		// skip os.Args[0] and "credential-helper"
+		gomodMain(args)
+	case "", frontendBasename:


Nit, I think we can remove the frontend subcommand and just assume if no subcommand is provided then it should be the frontend.

cpuguy83 · 2025-02-27T01:03:39Z

test/testenv/buildx.go

@@ -292,6 +307,16 @@ func (b *BuildxEnv) RunTest(ctx context.Context, t *testing.T, f TestFunc, opts
 	withProjectRoot(t, &so)
 	withResolveLocal(&so)

+	withSecrets(&so,


Seems like secrets should be a TestRunnerOpt

Also doesn't seem like we're actually using this in any tests?

It got added to the main RunTest. I'll move it to the test proper as a TestRunnerOpt.

Nevermind. This was from before I got rid of the integration tests. I changed it to use TestRunnerOpts and I'll use it when I add the integration tests.

pmengelbert · 2025-03-04T19:50:59Z

Let's think about how we can test these changes in the integration suite. I think if we can run a service on the host that is accessible to buildkit it shouldn't be a problem (other than just setting up multiple different git services).

I don't know how we do this without monkeying around with network namespaces. If that's the case, the tests would only run on linux. Setting the namespaces such that the frontend can communicate with a service running on the host requires root privileges.

cmd/frontend/main.go

source.go

cpuguy83 · 2025-03-07T23:47:59Z

Dockerfile

@@ -1,4 +1,4 @@
-FROM --platform=${BUILDPLATFORM} mcr.microsoft.com/oss/go/microsoft/golang:1.22@sha256:9e5243001d0f43d6e1c556a0ce8eedc1bc80eb37b5210036b289d2c601bc08b6 AS go
+FROM --platform=${BUILDPLATFORM} mcr.microsoft.com/oss/go/microsoft/golang:1.24@sha256:7c2024496593127e761a4610d628e7f653168342e7b60b21d6745adc236b0bd9 AS go


Should stick with 1.23 here (which, IIRC dependabot updated this week)

cpuguy83

The test can't run locally for me due to this error (beyond needing to add the platform to the image refs used).

go: host.docker.internal/user/private.git@v0.0.0: no secure protocol found for repository

cpuguy83 · 2025-03-07T23:48:40Z

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "test/fixtures/git/user/private"]


Is this something we need to commit to the repo?
I feel like we should be able to create a repo on the fly without submodules.

removed, now creating repo on the fly

cpuguy83 · 2025-03-07T23:51:22Z

frontend/debug/handle_sources.go

 		if err != nil {
 			return nil, nil, err
 		}

 		res, err := client.Solve(ctx, gwclient.SolveRequest{
 			Definition: def.ToPB(),
+			FrontendOpt: map[string]string{
+				"add-hosts": "host.docker.internal=10.0.2.2",


this should probably be copied in from the client opts rather than hard-coding it.

Mistake that I left this in, thanks.

cpuguy83 · 2025-03-07T23:51:55Z

frontend/gateway.go

@@ -74,6 +74,9 @@ func ForwarderFromClient(ctx context.Context, client gwclient.Client) dalec.Forw
 		req, err := newSolveRequest(
 			toDockerfile(ctx, st, dockerfileDt, spec, dalec.ProgressGroup("prepare dockerfile to forward to frontend")),
 			copyForForward(ctx, client),
+			func(sr *gwclient.SolveRequest) error {


What's the purpose of this?

Started doing something here, then took another approach. Left it in by mistake.

cpuguy83 · 2025-03-07T23:53:20Z

generator_gomod.go

@@ -41,19 +50,108 @@ func withGomod(g *SourceGenerator, srcSt, worker llb.State, opts ...llb.Constrai
 			paths = []string{"."}
 		}

+		sort.Strings(paths)


paths is already ordered by the user.

cpuguy83 · 2025-03-07T23:58:06Z

source.go

@@ -18,6 +19,11 @@ import (
 	"github.com/pkg/errors"
 )

+const (
+	GitCredentialHelperGomod       = "git-credential-gomod"
+	GitCredentialHelperGomodSrcDir = "/usr/bin"


Doesn't seem like this got removed?

cpuguy83 · 2025-03-08T00:11:48Z

test/source_test.go

+	return pid
+}
+
+func runGitServer(addr string, port int) {


Can you pass through a *testing.T shutdown the listener in a t.Cleanup function?

cpuguy83 · 2025-03-08T00:12:42Z

test/source_test.go

+			},
+		}
+
+		outsideAddr, insideAddr := getGitServerAddrs(ctx, t, c)


I don't think we are using these values anywhere other than runGitServer, maybe it makes sense to have runGitServer handle all the things here?

insideAddr gets added as an extra host a few lines down.

cpuguy83 · 2025-03-08T00:14:30Z

test/source_test.go

+	}))
+}
+
+func initGomodWorker(c gwclient.Client, host string, port int) llb.State {


Can you explain why we need to configure git this way?

Because you can't have a port number in a go module name. We tell git to use host:port so that when go invokes git to fetch the module, git knows to use host:port instead of just host. This is all because the git server is not running on 443/80, because we're starting our own git server on localhost and can't use those ports without privileges.

test/source_test.go

pmengelbert · 2025-03-10T16:29:38Z

The test can't run locally for me due to this error (beyond needing to add the platform to the image refs used).

go: host.docker.internal/user/private.git@v0.0.0: no secure protocol found for repository

Can you please try this again with the latest changes? I was experiencing the same error this morning but I believe it's fixed now.

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

The LLB has changed substantially since we adoped git credential helpers. We now check: - SSH sockets will be mounted - Secrets will be mounted as files (not provided in the environment) - The gomod credential helper will be mounted - The auth config and `go mod download` wrapper script will be mounted Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Instead, use the `gob` package to encode/decode the bytes. This is to avoid errors that might arise from the marshaling/unmarshaling to YAML. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

- Use the filepath of the secret mount to communicate with the credential helper - Clean up the credential helper - Handle cli flags properly - Modified the unit tests to reflect changes to llb operations - Various other small fixes as requested Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Some of the unit tests do not use valid hosts. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Also improve variable naming in credential helper. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

pmengelbert requested a review from a team as a code owner January 29, 2025 19:38

cpuguy83 added this to the v0.12.0 milestone Jan 29, 2025

MorrisLaw reviewed Jan 29, 2025

View reviewed changes

source_test.go Outdated Show resolved Hide resolved

test/testenv/buildx.go Outdated Show resolved Hide resolved

source_test.go Outdated Show resolved Hide resolved

generator_gomod.go Outdated Show resolved Hide resolved

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch from 91abd07 to d66c8cd Compare February 4, 2025 17:32

cpuguy83 requested changes Feb 11, 2025

View reviewed changes

cpuguy83 modified the milestones: v0.12.0, vNext Feb 13, 2025

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch 2 times, most recently from abed044 to a088848 Compare February 13, 2025 16:41

MorrisLaw approved these changes Feb 20, 2025

View reviewed changes

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch from 2588022 to 3149a03 Compare February 21, 2025 18:30

MorrisLaw reviewed Feb 21, 2025

View reviewed changes

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch from 2c71e12 to 2c1b3a9 Compare February 24, 2025 19:23

pmengelbert requested a review from cpuguy83 February 24, 2025 19:23

cpuguy83 requested changes Feb 25, 2025

View reviewed changes

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch from 7157dec to 0b2fb91 Compare February 26, 2025 18:58

pmengelbert requested a review from cpuguy83 February 26, 2025 19:01

cpuguy83 requested changes Feb 27, 2025

View reviewed changes

adamperlin reviewed Mar 6, 2025

View reviewed changes

cmd/frontend/main.go Outdated Show resolved Hide resolved

adamperlin reviewed Mar 6, 2025

View reviewed changes

source.go Outdated Show resolved Hide resolved

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch from 98c11a6 to bf5b5cc Compare March 7, 2025 23:39

cpuguy83 reviewed Mar 7, 2025

View reviewed changes

cpuguy83 requested changes Mar 8, 2025

View reviewed changes

adamperlin reviewed Mar 8, 2025

View reviewed changes

test/source_test.go Outdated Show resolved Hide resolved

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch from 922a79c to 7a679a8 Compare March 10, 2025 16:29

pmengelbert added 28 commits March 11, 2025 12:24

Improve map initialization

7b69699

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Pass pathname to function

d565702

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Use git credential helper for gomod git auth

0a360e0

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Remove dead code

11db2d3

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Address linter complaints

bc6792b

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Remove commented dead code

bf87d92

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Avoid marshaling/unmarshaling to yaml

79a387a

Instead, use the `gob` package to encode/decode the bytes. This is to avoid errors that might arise from the marshaling/unmarshaling to YAML. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add gomod auth docs

baffd2e

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Restore functioning of ssh auth

16ce9e4

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Remove frontend arg

f5f6fc2

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Rearrange integration test helpers

27e29e8

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Set up rootless detection, ip acquisition, host adding

7af838c

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Improve test organization

1d5a012

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Check rootless setups for proper env vars

6a69fee

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Get gomod auth working in integration tests

93c97e4

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Create git repo on the fly

0af18a0

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Remove git submodule

09a22d8

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add platform to images used

d0c60fe

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add context on docs to comments

cac505f

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add helpful comment

816731c

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Use assert library instead of require

3271ce1

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Fix errors after rebase

51cdbaf

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Address linter complaints

5b1caf0

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Update integration test builder images to use go 1.23

dd8f670

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Don't panic if host is not valid

0489b55

Some of the unit tests do not use valid hosts. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Substitute the build args in the gomod generator hostnames

Loading
Loading status checks…

a86107f

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

pmengelbert force-pushed the pmengelbert/troubleshoot_git_auth/2 branch from 8aaf10c to a86107f Compare March 11, 2025 16:26

Add comment to ssh auth for gomods

Loading
Loading status checks…

49fb675

Also improve variable naming in credential helper. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

		@@ -1,4 +1,4 @@
		FROM --platform=${BUILDPLATFORM} mcr.microsoft.com/oss/go/microsoft/golang:1.22@sha256:9e5243001d0f43d6e1c556a0ce8eedc1bc80eb37b5210036b289d2c601bc08b6 AS go
		FROM --platform=${BUILDPLATFORM} mcr.microsoft.com/oss/go/microsoft/golang:1.24@sha256:7c2024496593127e761a4610d628e7f653168342e7b60b21d6745adc236b0bd9 AS go

feat: Add gomod git auth #509

Are you sure you want to change the base?

feat: Add gomod git auth #509

Conversation

pmengelbert commented Jan 29, 2025

pmengelbert commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert commented Feb 13, 2025 • edited Loading

MorrisLaw left a comment

Choose a reason for hiding this comment

pmengelbert commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

pmengelbert commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

pmengelbert commented Mar 10, 2025

pmengelbert commented Feb 13, 2025 •

edited

Loading

pmengelbert Mar 4, 2025 •

edited

Loading

pmengelbert commented Mar 4, 2025 •

edited

Loading

pmengelbert Mar 10, 2025 •

edited

Loading