Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bake: git auth support for remote definitions #2363

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Mar 27, 2024

fixes #2360

Adds support for Git auth when using remote bake definitions through BUILDX_BAKE_GIT_AUTH_TOKEN and BUILDX_BAKE_GIT_AUTH_HEADER envs var that will set GIT_AUTH_TOKEN and GIT_AUTH_HEADER secrets respectively, matching https://github.com/moby/buildkit/blob/ad5a64c68f20435c52a51db73ef9246d6a3feba6/client/llb/source.go#L270-L271

Also adds BUILDX_BAKE_GIT_SSH to be consistent so user can provide SSH sock paths.

@crazy-max crazy-max added this to the v0.14.0 milestone Mar 27, 2024
@crazy-max crazy-max force-pushed the bake-remote-token branch 2 times, most recently from b04c3f0 to 35bbed2 Compare March 28, 2024 07:47
@crazy-max crazy-max marked this pull request as ready for review March 28, 2024 07:47
@crazy-max crazy-max force-pushed the bake-remote-token branch 6 times, most recently from 23b8fd2 to d7a8f4d Compare March 28, 2024 09:36
@crazy-max
Copy link
Member Author

crazy-max commented Mar 28, 2024

@tonistiigi While adding tests I discovered a strange behavior with git source in BuildKit when authenticated. It seems most of the requests send the authorization header but not the one when checking the default branch in https://github.com/moby/buildkit/blob/ad5a64c68f20435c52a51db73ef9246d6a3feba6/source/git/source.go#L732 even if header is set in
https://github.com/moby/buildkit/blob/ad5a64c68f20435c52a51db73ef9246d6a3feba6/source/git/source.go#L350 or https://github.com/moby/buildkit/blob/ad5a64c68f20435c52a51db73ef9246d6a3feba6/source/git/source.go#L422.

See https://github.com/docker/buildx/actions/runs/8465151975/job/23191259148#step:7:635

    testutilserve.go:52: Request Method: GET
    testutilserve.go:53: Request URL: /test.git/info/refs?service=git-upload-pack
    testutilserve.go:54: Request Headers:
    testutilserve.go:57:   Cache-Control: no-cache
    testutilserve.go:57:   User-Agent: git/2.43.0
    testutilserve.go:57:   Accept: */*
    testutilserve.go:57:   Accept-Encoding: deflate, gzip, br
    testutilserve.go:57:   Pragma: no-cache
    testutilserve.go:57:   Git-Protocol: version=2
    testutilserve.go:60: access token used: "cptgkeatnyrttvqj1pmrlj8sv"
    testutilserve.go:62: basic auth: user="" pass=""
    bake.go:175: 
        	Error Trace:	/src/tests/bake.go:175
        	            				/src/vendor/github.com/moby/buildkit/util/testutil/integration/run.go:93
        	            				/src/vendor/github.com/moby/buildkit/util/testutil/integration/run.go:207
        	Error:      	Received unexpected error:
        	            	exit status 1
        	Test:       	TestIntegration/TestBakeRemoteAuth/worker=docker-container
        	Messages:   	ERROR: failed to solve: failed to load cache key: error fetching default branch for repository http://127.0.0.1:43081/test.git: git error: exit status 128
        	            	stderr:
        	            	fatal: could not read Username for 'http://127.0.0.1:43081/': terminal prompts disabled

So for now in this test I just check the very first request is authenticated but I think we need a fix on BuildKit side.

Works fine though when testing directly:

$ BUILDX_BAKE_GIT_AUTH_TOKEN=... docker buildx bake https://github.com/docker/test-docker-action.git image --print
#0 building with "default" instance using docker driver

#1 [internal] load git source https://github.com/docker/test-docker-action.git
#1 0.520 ref: refs/heads/master HEAD
#1 0.520 99d5da274d46b04a3936bbb9811880a56b9cc238       HEAD
#1 1.005 99d5da274d46b04a3936bbb9811880a56b9cc238       refs/heads/master
#1 CACHED
{
  "group": {
    "default": {
      "targets": [
        "image"
      ]
    }
  },
  "target": {
    "image": {
      "context": "https://github.com/docker/test-docker-action.git",
      "dockerfile": "./multistage.Dockerfile",
      "platforms": [
        "linux/amd64",
        "linux/arm64"
      ]
    }
  }
}

Edit

seems to be a limitation with ls-remote command that does not provide an option to directly set custom HTTP headers 😞. Maybe instead of using a git command to list refs we could do a simple HTTP request for smart service?

Edit2

after testing with:

$ curl https://github.com/docker/buildx/info/refs?service=git-upload-pack --output -
001e# service=git-upload-pack
000001558abef5908705e49f7ba88ef8c957e1127b597a2a HEADmulti_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed allow-tip-sha1-in-want allow-reachable-sha1-in-want no-done symref=HEAD:refs/heads/master filter object-format=sha1 agent=git/github-de75ed0166ec
007eef78ae7bcd872a51be7b271f7fe8696349cd6e4a refs/heads/dependabot/go_modules/github.com/docker/docker-26.0.0-rc3incompatible
003f8abef5908705e49f7ba88ef8c957e1127b597a2a refs/heads/master
003e86bdced7766639d56baa4c7c449a4f6468490f87 refs/heads/v0.10
003e4e547752afe829ccd0a5b5ccfe4901c7cde9f88b refs/heads/v0.11
003e89154c7d33034e912b6775f2016715f07ca4ae96 refs/heads/v0.12
003ed7069985e5cae70826eee1b595b80ac3cb517697 refs/heads/v0.13
...

It seems we could use symref=HEAD:refs/heads/master?

Test on gitlab:

$ curl https://gitlab.com/nvidia/container-images/cuda.git/info/refs?service=git-upload-pack --output -
001e# service=git-upload-pack
00000148f66651a424c86057620809f176f5ee252ac62f60 HEADmulti_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed allow-tip-sha1-in-want allow-reachable-sha1-in-want no-done symref=HEAD:refs/heads/master filter object-format=sha1 agent=git/2.43.2
003ff66651a424c86057620809f176f5ee252ac62f60 refs/heads/master
0049d0a86ed3be14c5f97ca398719228de32452ea6e1 refs/merge-requests/13/head
004a32374c14fee5f89e8592c3abb4c6546be77c71d4 refs/merge-requests/13/merge

Works with basic auth as well.

Also we should make sure this is a server supporting Git's smart service by checking Content-Type: application/x-git-upload-pack-advertisement header, othwise fallback to current beahvior with ls-remote command.

bake/remote.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

Will these always be for bake only, or could these be for "build" as well? (curious if the BUILDX_BAKE_GIT_AUTH_HEADER env vars must contain _BAKE_)

@crazy-max
Copy link
Member Author

crazy-max commented Mar 28, 2024

Will these always be for bake only, or could these be for "build" as well? (curious if the BUILDX_BAKE_GIT_AUTH_HEADER env vars must contain _BAKE_)

Only bake, this is used to build remote bake definitions. For build we already use secrets to build from a remote Git context.

@thaJeztah
Copy link
Member

Ah! So.. naive question; if that doesn't require special env-vars; why does bake need them?

@thaJeztah
Copy link
Member

Re-reading; you mention remote definitions (so equivalent to "remote dockerfile"); I know docker build / docker buildx build also allows building from a remote Dockerfile (Dockerfile could be remote, but build-context could be local) but not sure it currently does from a "git" URL; if that's not supported, then ... perhaps it should as well?

@crazy-max
Copy link
Member Author

Ah! So.. naive question; if that doesn't require special env-vars; why does bake need them?

Because a remote definition is not a build context per-say. We can't set secrets directly as this will be used also in the underlying builds from the definition. What @tonistiigi suggested in #2360 (comment) with a builtin target name could work but UX wise this is not great.

@crazy-max
Copy link
Member Author

crazy-max commented Mar 28, 2024

Re-reading; you mention remote definitions (so equivalent to "remote dockerfile"); I know docker build / docker buildx build also allows building from a remote Dockerfile (Dockerfile could be remote, but build-context could be local) but not sure it currently does from a "git" URL; if that's not supported, then ... perhaps it should as well?

Yes indeed I don't think we have that for the Dockerfile atm. For bake we have "builtin" ways to use either a local or remote context, as well as merging local and remote definitions: https://docs.docker.com/build/bake/remote-definition/

@dvdksn Talking about merging local and remote bake definitions, I don't think we have this documented atm #1838

Edit: nevermind https://docs.docker.com/build/bake/remote-definition/#remote-definition-with-the---file-flag

}

var gitAuthSecrets []*controllerapi.Secret
if _, ok := os.LookupEnv("BUILDX_BAKE_GIT_AUTH_TOKEN"); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably out of scope, but unclear to me how the controller takes Env keys as input. Doesn't the controller have its own env if it is running in another process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum good point, it seems to work when connecting to local controller. I don't think there is another process except when using --invoke and setting envs through invoke config:

repeated string Env = 3;

bake/remote.go Outdated
if err == nil {
session = append(session, ssh)
var sshSpecs []*controllerapi.SSH
if v := os.Getenv("BUILDX_BAKE_GIT_SSH"); v != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ID of this needs to be set with MountSSHSock() if not default. Setting it to other ID would not work otherwise. So probably it should just be the path.

@crazy-max crazy-max mentioned this pull request Apr 4, 2024
@crazy-max crazy-max force-pushed the bake-remote-token branch 2 times, most recently from 020ac59 to 8cb85e5 Compare April 4, 2024 12:02
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
@crazy-max crazy-max merged commit 59cc107 into docker:master Apr 4, 2024
63 checks passed
@crazy-max crazy-max deleted the bake-remote-token branch April 4, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auth support for remote bake definitions
3 participants