git url: support query form + support specifying checksum in query #5974

AkihiroSuda · 2025-05-15T10:04:59Z

Split from:

git url: support CSV form (also support verifying a tag with a commit hash) #5903

e.g.,

https://github.com/moby/example.git?ref=v1.0.0&commit=deadbeef&subdir=/subdir

# alias of ref=refs/tags/v1.0.0
https://github.com/moby/example.git?tag=v1.0.0

# alias of ref=refs/heads/v1.0
https://github.com/moby/example.git?branch=v1.0

Fix #4905, but the syntax differs from the original proposal.

The document will be added to
https://github.com/docker/docs/blob/main/content/manuals/build/concepts/context.md#url-fragments

tonistiigi · 2025-05-19T20:35:14Z

@AkihiroSuda Needs rebase after #5975

tonistiigi

One thing that is weird atm is the URL parsing in LLB (client lib and identifier) vs Dockerfile. The query form should really be only for Dockerfile/dockerui. In LLB the parameters are sent with attribute values instead. For historical reasons, the current LLB is kind of a mix where #ref is still part of the URL from initial implementation, but for example protocol of the URL is always git:// and the real protocol is sent as an attribute.

Currently, it looks like in LLB client we parse a full URL (with querystring etc.) but then we drop everything but Host/Path and send the original URL as attr (that I think will container querystring). Also #ref is not even part of the URL the user passes in the client function, but is glued to the identifier internally.

Then on LLB receiver side, this now handles querystring in this PR in the URL in NewGitIdentifier (although it looks that it would require client side to error for the querystring to be passed at all there), but actually the checksum gets set later from pb.AttrGitChecksum.

In order to fix this, I think we should separate these concepts. Assuming that we do need some level of parser in the LLB side, I think having two separate packages might be too much so maybe just two parser functions where the Dockerfile one would parse the querystring and other Dockerfile specific things and then internally call the more basic parser. Looking at the current return type, there is already some weird things there, eg. IndistinguishableFromLocal seems to be only used in Dockerfile, and UnencryptedTCP doesn't seem to be used anywhere at all.

I'm also ok with changing the signature for llb.Git, eg. adding GitSubdir() (maybe even GitRef()) so it is the same as other git properties instead of requiring microformat and URL parsing as input that gets converted anyway. On the identifier parser side, we do need to keep backwards compatibility though (but could deprecate the old behavior).

tonistiigi · 2025-05-22T05:12:54Z

source/git/identifier.go

-		repo.Subdir = u.Fragment.Subdir
+	if u.Opts != nil {
+		repo.Ref = u.Opts.Ref
+		repo.Checksum = u.Opts.Checksum


This is the place where it is getting checksum from querystring in LLB while it really comes from pb.AttrGitChecksum

Looks like there is still a wrong type in here after parsing as the checksum should only come from props (I don't think this is some backwards compatibility?).

tonistiigi · 2025-06-05T00:26:37Z

@AkihiroSuda Any update?

crazy-max · 2025-08-18T08:34:27Z

Rebased, but refactoring the parser may take some time. Unlikely in this week.

@AkihiroSuda Do you have time to look at it? We plan to do first v0.24 RC next week.

AkihiroSuda · 2025-08-20T09:08:48Z

Looking at the current return type, there is already some weird things there, eg. IndistinguishableFromLocal seems to be only used in Dockerfile, and UnencryptedTCP doesn't seem to be used anywhere at all.

Moving these weirdo to frontend/dockerfile/dfgitutil:

mv util/gitutil/git_ref.go frontend/dockerfile/dfgitutil/git_ref.go #6152

AkihiroSuda · 2025-08-20T10:19:22Z

Added refactoring commits

util/gitutil: add ParseURLBasic
client/llb: add Git2

Fix issue 4905, but the syntax differs from the original proposal. The document will be added to https://github.com/docker/docs/blob/main/content/manuals/build/concepts/context.md#url-fragments Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

Prior to this commit, `ADD https://github.com/moby/example.git?invalid=42` was fetching a single HTML file without causing an error. Now it returns an error `unexpected query "invalid"`. Hint: use `git show --ignore-all-space` to review this commit. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

ParseURLBasic is a simplified variant of ParseURL. Basically, the full ParseURL should be used only in the LLB frontends such as `dockerfile`. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

Similar to `Git`, but does not parse git url fragments and queries. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

crazy-max · 2025-08-22T13:40:24Z

util/gitutil/git_url_test.go

+			// TODO: consider allowing this, when the tag actually exists on the branch
+			url: "https://github.com/moby/buildkit?tag=v1.0.0&branch=v1.0",


I guess this is similar to Checksum but could potentially be a validation of both branch and commit?

crazy-max

I was testing these changes and so far so good (with also vendoring in buildx client docker/buildx#3380) but I found a use case that would be needed related to checksum validation.

This relates to how the Docker GitHub Actions currently handle Git context. Currently we always checkout by commit sha if ref is not a pull request: https://github.com/docker/actions-toolkit/blob/633bcf1936ffff0e05a4bbe2b23c71abf6f4111e/src/context.ts#L45-L60

That can be be problematic because then git tag is not pulled similar to https://github.com/crazy-max/buildkit/actions/runs/16967780276/job/48096258733#step:7:451. (see also docker/actions-toolkit#677)

But with query form we could use ref=refs/tags/v0.23.0&commit=28c90eadc4c12cc78155ad59ca5f486220241d2a so it would fetch the tag and validate commit sha. That's good!

But what should we do for a another ref like default branch? I don't think having ref=refs/heads/master&commit=28c90eadc4c12cc78155ad59ca5f486220241d2a with strict checksum validation makes a lot of sense as there might be race condition when build runs on CI.

WDYT if we have another var to opt-out for checksum validation while keeping ref and commit like:

ref=refs/heads/master&commit=28c90eadc4c12cc78155ad59ca5f486220241d2a&checksum=false

tonistiigi · 2025-08-27T07:49:19Z

WDYT if we have another var to opt-out for checksum validation while keeping ref and commit like:

We could have another option like fetch-by-commit that could ignore ref (just set it blindly) if the intention is to support refs that used to be a specific commit. But if commit is set then buildkit should always guarantee that this is the correct commit that was checked out. Could also have something like fetch-by-commit=fast-forward for the heads/master case as master branch is always expected to be either commit or based on the commit. For the PR CI case where PR gets forced pushed, I think we should just fail the build in our actions, as no point in building a ref of the old PR if it has already been forced pushed to something else (new CI would start anyway).

tonistiigi · 2025-08-27T07:50:42Z

client/llb/source.go

+}
+
+// Git2 is similar to Git but takes [gitutil.GitURLBase] as the argument.
+func Git2(remote gitutil.GitURLBase, opts ...GitOption) State {


Not sure if we want to go with numeric functions in here. GitFromRemote ?

tonistiigi · 2025-08-27T07:52:33Z

client/llb/source.go

+	})
+}
+
+func GitIDSuffix(v string) GitOption {


I don't think we need to make this public if only used internally. Same for prop.

tonistiigi · 2025-08-27T08:00:15Z

frontend/dockerfile/dockerfile2llb/convert.go

+			if gu.Remote != "" {
+				fullURL = gu.Remote
+			}
+			gitOptions := []llb.GitOption{llb.WithCustomName(pgName), llb.GitChecksum(gitRef.Checksum),


I don't quite get why do we need to call ParseGitRef and then still pass a bunch of the options manually. Why can't we convert to a structure that already can be connected to llb. There are so many duplicates in here: user needs to pass parsed GitURLBase structure, but then still needs to pass GitFullURL as well. Why is the commit passed to both GitChecksum and GitIDSuffix. These look like internal behaviors what the user should not know about. If we parse the input URL we should have remote(full), ref, commit, subdir and we should be able to pass them without duplicates.

tonistiigi · 2025-08-27T08:04:18Z

source/git/identifier.go

-		repo.Subdir = u.Fragment.Subdir
+	if u.Opts != nil {
+		repo.Ref = u.Opts.Ref
+		repo.Checksum = u.Opts.Checksum


Looks like there is still a wrong type in here after parsing as the checksum should only come from props (I don't think this is some backwards compatibility?).

tonistiigi · 2025-08-27T08:06:33Z

util/gitutil/git_url.go

-func parseOpts(fragment string) *GitURLOpts {
-	if fragment == "" {
-		return nil
+func parseOpts(fragment string, query url.Values) (*GitURLOpts, error) {


Querystring is only part of the new dockerfile-style URLs so I don't understand why this parsing is in this pacakge.

AkihiroSuda · 2025-08-29T05:28:58Z

Carried and merged as:

Support querystring form Git URLs #6172

Thanks @tonistiigi 🙇

github-actions bot added area/testing area/util area/source labels May 15, 2025

AkihiroSuda force-pushed the giturl-query branch from 0d0c788 to 8179cf4 Compare May 15, 2025 10:40

github-actions bot added area/dockerfile area/frontend labels May 15, 2025

AkihiroSuda force-pushed the giturl-query branch 2 times, most recently from 921c77c to 12fb4fa Compare May 15, 2025 11:11

This was referenced May 15, 2025

git url: support CSV form (also support verifying a tag with a commit hash) #5903

Closed

git source: add AttrGitChecksum #5975

Merged

AkihiroSuda added this to the v0.22.0 milestone May 15, 2025

tonistiigi modified the milestones: v0.22.0, v0.23.0 May 16, 2025

thompson-shaun requested a review from tonistiigi May 16, 2025 17:24

thompson-shaun assigned tonistiigi and AkihiroSuda May 16, 2025

AkihiroSuda force-pushed the giturl-query branch from 12fb4fa to b8a4e2b Compare May 20, 2025 09:18

github-actions bot added area/llb area/client labels May 20, 2025

AkihiroSuda marked this pull request as draft May 20, 2025 09:32

AkihiroSuda force-pushed the giturl-query branch from b8a4e2b to 2808636 Compare May 20, 2025 09:42

This comment was marked as resolved.

Sign in to view

AkihiroSuda marked this pull request as ready for review May 20, 2025 10:19

AkihiroSuda force-pushed the giturl-query branch from 2808636 to 848c83f Compare May 20, 2025 17:55

tonistiigi reviewed May 22, 2025

View reviewed changes

AkihiroSuda force-pushed the giturl-query branch from 848c83f to 2699321 Compare June 5, 2025 06:09

AkihiroSuda mentioned this pull request Jun 5, 2025

git url: support query form #6016

Closed

AkihiroSuda modified the milestones: v0.23.0, v0.24.0 Jun 10, 2025

cokybit linked an issue Jun 22, 2025 that may be closed by this pull request

.github/dependabot.yml cokybit/automatic-goggles#4

Open

crazy-max mentioned this pull request Aug 14, 2025

hack: use bake to build buildkit images #6107

Open

AkihiroSuda force-pushed the giturl-query branch from f978441 to 7de593a Compare August 20, 2025 10:18

AkihiroSuda force-pushed the giturl-query branch from 7de593a to e4eb28e Compare August 20, 2025 10:21

This comment was marked as resolved.

Sign in to view

AkihiroSuda marked this pull request as draft August 20, 2025 10:38

AkihiroSuda added 4 commits August 22, 2025 15:28

git url: support query form

0d50a8b

Fix issue 4905, but the syntax differs from the original proposal. The document will be added to https://github.com/docker/docs/blob/main/content/manuals/build/concepts/context.md#url-fragments Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

git url: support specifying checksum in query

7331897

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

util/gitutil: add ParseURLBasic

7c8ee7d

ParseURLBasic is a simplified variant of ParseURL. Basically, the full ParseURL should be used only in the LLB frontends such as `dockerfile`. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the giturl-query branch 3 times, most recently from f06fdae to 3b48201 Compare August 22, 2025 08:15

client/llb: add Git2

35db37f

Similar to `Git`, but does not parse git url fragments and queries. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the giturl-query branch from 3b48201 to 35db37f Compare August 22, 2025 08:41

AkihiroSuda marked this pull request as ready for review August 22, 2025 12:33

crazy-max reviewed Aug 22, 2025

View reviewed changes

crazy-max reviewed Aug 25, 2025

View reviewed changes

crazy-max mentioned this pull request Aug 25, 2025

test buildkit-5974 docker/buildx#3380

Closed

tonistiigi reviewed Aug 27, 2025

View reviewed changes

tonistiigi mentioned this pull request Aug 28, 2025

Support querystring form Git URLs #6172

Merged

AkihiroSuda closed this Aug 29, 2025

		// TODO: consider allowing this, when the tag actually exists on the branch
		url: "https://github.com/moby/buildkit?tag=v1.0.0&branch=v1.0",

git url: support query form + support specifying checksum in query #5974

git url: support query form + support specifying checksum in query #5974

Conversation

AkihiroSuda commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented May 19, 2025

Uh oh!

This comment was marked as resolved.

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

tonistiigi May 22, 2025

Choose a reason for hiding this comment

Uh oh!

tonistiigi Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Jun 5, 2025

Uh oh!

crazy-max commented Aug 18, 2025

Uh oh!

AkihiroSuda commented Aug 20, 2025

Uh oh!

AkihiroSuda commented Aug 20, 2025

Uh oh!

This comment was marked as resolved.

crazy-max Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazy-max left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Aug 27, 2025

Uh oh!

tonistiigi Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

tonistiigi Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

tonistiigi Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

tonistiigi Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

tonistiigi Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Aug 29, 2025

Uh oh!

Uh oh!

AkihiroSuda commented May 15, 2025 •

edited

Loading

crazy-max Aug 22, 2025 •

edited

Loading

crazy-max left a comment •

edited

Loading