From e6676287ca1f5836e9a66119553551d75a29c861 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Thu, 15 May 2025 18:34:34 +0900 Subject: [PATCH 1/6] git url: support query form 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 --- frontend/dockerfile/dfgitutil/git_ref.go | 5 +- frontend/dockerfile/dfgitutil/git_ref_test.go | 9 ++ util/gitutil/git_url.go | 89 ++++++++++++++++--- util/gitutil/git_url_test.go | 59 ++++++++++++ util/sshutil/scpurl.go | 34 +++++-- 5 files changed, 173 insertions(+), 23 deletions(-) diff --git a/frontend/dockerfile/dfgitutil/git_ref.go b/frontend/dockerfile/dfgitutil/git_ref.go index a2b37f8e09fc..d1746c916000 100644 --- a/frontend/dockerfile/dfgitutil/git_ref.go +++ b/frontend/dockerfile/dfgitutil/git_ref.go @@ -63,11 +63,14 @@ func ParseGitRef(ref string) (*GitRef, error) { return nil, cerrdefs.ErrInvalidArgument } else if strings.HasPrefix(ref, "github.com/") { res.IndistinguishableFromLocal = true // Deprecated - remote = gitutil.FromURL(&url.URL{ + remote, err = gitutil.FromURL(&url.URL{ Scheme: "https", Host: "github.com", Path: strings.TrimPrefix(ref, "github.com/"), }) + if err != nil { + return nil, err + } } else { remote, err = gitutil.ParseURL(ref) if errors.Is(err, gitutil.ErrUnknownProtocol) { diff --git a/frontend/dockerfile/dfgitutil/git_ref_test.go b/frontend/dockerfile/dfgitutil/git_ref_test.go index 5108ccebe02a..31156daafe88 100644 --- a/frontend/dockerfile/dfgitutil/git_ref_test.go +++ b/frontend/dockerfile/dfgitutil/git_ref_test.go @@ -138,6 +138,15 @@ func TestParseGitRef(t *testing.T) { ref: ".git", expected: nil, }, + { + ref: "https://github.com/docker/docker.git?ref=v1.0.0&subdir=/subdir", + expected: &GitRef{ + Remote: "https://github.com/docker/docker.git", + ShortName: "docker", + Commit: "v1.0.0", + SubDir: "/subdir", + }, + }, } for _, tt := range cases { t.Run(tt.ref, func(t *testing.T) { diff --git a/util/gitutil/git_url.go b/util/gitutil/git_url.go index 1c506322aed1..f81ad1676088 100644 --- a/util/gitutil/git_url.go +++ b/util/gitutil/git_url.go @@ -56,7 +56,7 @@ type GitURL struct { } // GitURLOpts is the buildkit-specific metadata extracted from the fragment -// of a remote URL. +// or the query of a remote URL. type GitURLOpts struct { // Ref is the git reference Ref string @@ -66,12 +66,63 @@ type GitURLOpts struct { // parseOpts splits a git URL fragment into its respective git // reference and subdirectory components. -func parseOpts(fragment string) *GitURLOpts { - if fragment == "" { - return nil +func parseOpts(fragment string, query url.Values) (*GitURLOpts, error) { + if fragment == "" && len(query) == 0 { + return nil, nil } - ref, subdir, _ := strings.Cut(fragment, ":") - return &GitURLOpts{Ref: ref, Subdir: subdir} + opts := &GitURLOpts{} + if fragment != "" { + opts.Ref, opts.Subdir, _ = strings.Cut(fragment, ":") + } + var tag, branch string + for k, v := range query { + switch len(v) { + case 0: + return nil, errors.Errorf("query %q has no value", k) + case 1: + if v[0] == "" { + return nil, errors.Errorf("query %q has no value", k) + } + // NOP + default: + return nil, errors.Errorf("query %q has multiple values", k) + } + switch k { + case "ref": + if opts.Ref != "" && opts.Ref != v[0] { + return nil, errors.Errorf("ref conflicts: %q vs %q", opts.Ref, v[0]) + } + opts.Ref = v[0] + case "tag": + tag = v[0] + case "branch": + branch = v[0] + case "subdir": + if opts.Subdir != "" && opts.Subdir != v[0] { + return nil, errors.Errorf("subdir conflicts: %q vs %q", opts.Subdir, v[0]) + } + opts.Subdir = v[0] + default: + return nil, errors.Errorf("unexpected query %q", k) + } + } + if tag != "" { + if opts.Ref != "" { + return nil, errors.New("tag conflicts with ref") + } + opts.Ref = "refs/tags/" + tag + } + if branch != "" { + if tag != "" { + // TODO: consider allowing this, when the tag actually exists on the branch + return nil, errors.New("branch conflicts with tag") + } + if opts.Ref != "" { + return nil, errors.New("branch conflicts with ref") + } + opts.Ref = "refs/heads/" + branch + } + return opts, nil } // ParseURL parses a BuildKit-style Git URL (that may contain additional @@ -86,11 +137,11 @@ func ParseURL(remote string) (*GitURL, error) { if err != nil { return nil, err } - return FromURL(url), nil + return FromURL(url) } if url, err := sshutil.ParseSCPStyleURL(remote); err == nil { - return fromSCPStyleURL(url), nil + return fromSCPStyleURL(url) } return nil, ErrUnknownProtocol @@ -105,28 +156,38 @@ func IsGitTransport(remote string) bool { return sshutil.IsImplicitSSHTransport(remote) } -func FromURL(url *url.URL) *GitURL { +func FromURL(url *url.URL) (*GitURL, error) { withoutOpts := *url withoutOpts.Fragment = "" + withoutOpts.RawQuery = "" + opts, err := parseOpts(url.Fragment, url.Query()) + if err != nil { + return nil, err + } return &GitURL{ Scheme: url.Scheme, User: url.User, Host: url.Host, Path: url.Path, - Opts: parseOpts(url.Fragment), + Opts: opts, Remote: withoutOpts.String(), - } + }, nil } -func fromSCPStyleURL(url *sshutil.SCPStyleURL) *GitURL { +func fromSCPStyleURL(url *sshutil.SCPStyleURL) (*GitURL, error) { withoutOpts := *url withoutOpts.Fragment = "" + withoutOpts.Query = nil + opts, err := parseOpts(url.Fragment, url.Query) + if err != nil { + return nil, err + } return &GitURL{ Scheme: SSHProtocol, User: url.User, Host: url.Host, Path: url.Path, - Opts: parseOpts(url.Fragment), + Opts: opts, Remote: withoutOpts.String(), - } + }, nil } diff --git a/util/gitutil/git_url_test.go b/util/gitutil/git_url_test.go index 3306b06f7bd2..be3409a413ea 100644 --- a/util/gitutil/git_url_test.go +++ b/util/gitutil/git_url_test.go @@ -151,6 +151,65 @@ func TestParseURL(t *testing.T) { Path: "/moby/buildkit", }, }, + { + url: "https://github.com/moby/buildkit?ref=v1.0.0&subdir=/subdir", + result: GitURL{ + Scheme: HTTPSProtocol, + Host: "github.com", + Path: "/moby/buildkit", + Opts: &GitURLOpts{Ref: "v1.0.0", Subdir: "/subdir"}, + }, + }, + { + url: "https://github.com/moby/buildkit?subdir=/subdir#v1.0.0", + result: GitURL{ + Scheme: HTTPSProtocol, + Host: "github.com", + Path: "/moby/buildkit", + Opts: &GitURLOpts{Ref: "v1.0.0", Subdir: "/subdir"}, + }, + }, + { + url: "https://github.com/moby/buildkit?tag=v1.0.0", + result: GitURL{ + Scheme: HTTPSProtocol, + Host: "github.com", + Path: "/moby/buildkit", + Opts: &GitURLOpts{Ref: "refs/tags/v1.0.0"}, + }, + }, + { + url: "https://github.com/moby/buildkit?branch=v1.0", + result: GitURL{ + Scheme: HTTPSProtocol, + Host: "github.com", + Path: "/moby/buildkit", + Opts: &GitURLOpts{Ref: "refs/heads/v1.0"}, + }, + }, + { + url: "https://github.com/moby/buildkit?ref=v1.0.0#v1.2.3", + err: true, + }, + { + url: "https://github.com/moby/buildkit?ref=v1.0.0&tag=v1.2.3", + err: true, + }, + { + // 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", + err: true, + }, + { + url: "git@github.com:moby/buildkit.git?subdir=/subdir#v1.0.0", + result: GitURL{ + Scheme: SSHProtocol, + Host: "github.com", + Path: "moby/buildkit.git", + User: url.User("git"), + Opts: &GitURLOpts{Ref: "v1.0.0", Subdir: "/subdir"}, + }, + }, } for _, test := range tests { t.Run(test.url, func(t *testing.T) { diff --git a/util/sshutil/scpurl.go b/util/sshutil/scpurl.go index 10491f32f08e..5d85f09a0ba6 100644 --- a/util/sshutil/scpurl.go +++ b/util/sshutil/scpurl.go @@ -1,13 +1,14 @@ package sshutil import ( - "errors" "fmt" "net/url" "regexp" + + "github.com/pkg/errors" ) -var gitSSHRegex = regexp.MustCompile("^([a-zA-Z0-9-_]+)@([a-zA-Z0-9-.]+):(.*?)(?:#(.*))?$") +var gitSSHRegex = regexp.MustCompile(`^([a-zA-Z0-9-_]+)@([a-zA-Z0-9-.]+):(.*?)(?:\?(.*?))?(?:#(.*))?$`) func IsImplicitSSHTransport(s string) bool { return gitSSHRegex.MatchString(s) @@ -18,6 +19,7 @@ type SCPStyleURL struct { Host string Path string + Query url.Values Fragment string } @@ -26,18 +28,34 @@ func ParseSCPStyleURL(raw string) (*SCPStyleURL, error) { if matches == nil { return nil, errors.New("invalid scp-style url") } + + rawQuery := matches[4] + vals := url.Values{} + if rawQuery != "" { + var err error + vals, err = url.ParseQuery(rawQuery) + if err != nil { + return nil, errors.Wrap(err, "invalid query in scp-style url") + } + } + return &SCPStyleURL{ User: url.User(matches[1]), Host: matches[2], Path: matches[3], - Fragment: matches[4], + Query: vals, + Fragment: matches[5], }, nil } -func (url *SCPStyleURL) String() string { - base := fmt.Sprintf("%s@%s:%s", url.User.String(), url.Host, url.Path) - if url.Fragment == "" { - return base +func (u *SCPStyleURL) String() string { + s := fmt.Sprintf("%s@%s:%s", u.User.String(), u.Host, u.Path) + + if len(u.Query) > 0 { + s += "?" + u.Query.Encode() + } + if u.Fragment != "" { + s += "#" + u.Fragment } - return base + "#" + url.Fragment + return s } From c8ce3722932ede6d7296f8d913d6e796225fe07f Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 28 Aug 2025 15:30:17 -0700 Subject: [PATCH 2/6] dfgitutil: add querystring style URLs to dfgitutil Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dfgitutil/git_ref.go | 100 ++++++++++++++++-- frontend/dockerfile/dfgitutil/git_ref_test.go | 86 +++++++++++++-- frontend/dockerfile/dockerfile2llb/convert.go | 2 +- frontend/dockerui/context.go | 2 +- util/gitutil/git_url.go | 82 +++----------- util/gitutil/git_url_test.go | 56 +++------- 6 files changed, 201 insertions(+), 127 deletions(-) diff --git a/frontend/dockerfile/dfgitutil/git_ref.go b/frontend/dockerfile/dfgitutil/git_ref.go index d1746c916000..1aaf340a5d68 100644 --- a/frontend/dockerfile/dfgitutil/git_ref.go +++ b/frontend/dockerfile/dfgitutil/git_ref.go @@ -2,6 +2,7 @@ package dfgitutil import ( + "log" "net/url" "strings" @@ -23,9 +24,12 @@ type GitRef struct { // e.g., "bar" for "https://github.com/foo/bar.git" ShortName string - // Commit is a commit hash, a tag, or branch name. - // Commit is optional. - Commit string + // Ref is a commit hash, a tag, or branch name. + // Ref is optional. + Ref string + + // Checksum is a commit hash. + Checksum string // SubDir is a directory path inside the repo. // SubDir is optional. @@ -60,14 +64,15 @@ func ParseGitRef(ref string) (*GitRef, error) { ) if strings.HasPrefix(ref, "./") || strings.HasPrefix(ref, "../") { - return nil, cerrdefs.ErrInvalidArgument + return nil, errors.WithStack(cerrdefs.ErrInvalidArgument) } else if strings.HasPrefix(ref, "github.com/") { res.IndistinguishableFromLocal = true // Deprecated - remote, err = gitutil.FromURL(&url.URL{ - Scheme: "https", - Host: "github.com", - Path: strings.TrimPrefix(ref, "github.com/"), - }) + u, err := url.Parse(ref) + if err != nil { + return nil, err + } + u.Scheme = "https" + remote, err = gitutil.FromURL(u) if err != nil { return nil, err } @@ -89,7 +94,7 @@ func ParseGitRef(ref string) (*GitRef, error) { // An HTTP(S) URL is considered to be a valid git ref only when it has the ".git[...]" suffix. case gitutil.HTTPProtocol, gitutil.HTTPSProtocol: if !strings.HasSuffix(remote.Path, ".git") { - return nil, cerrdefs.ErrInvalidArgument + return nil, errors.WithStack(cerrdefs.ErrInvalidArgument) } } } @@ -99,11 +104,84 @@ func ParseGitRef(ref string) (*GitRef, error) { _, res.Remote, _ = strings.Cut(res.Remote, "://") } if remote.Opts != nil { - res.Commit, res.SubDir = remote.Opts.Ref, remote.Opts.Subdir + res.Ref, res.SubDir = remote.Opts.Ref, remote.Opts.Subdir } repoSplitBySlash := strings.Split(res.Remote, "/") res.ShortName = strings.TrimSuffix(repoSplitBySlash[len(repoSplitBySlash)-1], ".git") + if err := res.loadQuery(remote.Query); err != nil { + log.Printf("query") + return nil, err + } + return res, nil } + +func (gf *GitRef) loadQuery(query url.Values) error { + if len(query) == 0 { + return nil + } + var tag, branch string + for k, v := range query { + switch len(v) { + case 0: + return errors.Errorf("query %q has no value", k) + case 1: + if v[0] == "" { + return errors.Errorf("query %q has no value", k) + } + // NOP + default: + return errors.Errorf("query %q has multiple values", k) + } + switch k { + case "ref": + if gf.Ref != "" && gf.Ref != v[0] { + return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, v[0]) + } + gf.Ref = v[0] + case "tag": + tag = v[0] + case "branch": + branch = v[0] + case "subdir": + if gf.SubDir != "" && gf.SubDir != v[0] { + return errors.Errorf("subdir conflicts: %q vs %q", gf.SubDir, v[0]) + } + gf.SubDir = v[0] + case "checksum", "commit": + gf.Checksum = v[0] + default: + return errors.Errorf("unexpected query %q", k) + } + } + if tag != "" { + const tagPrefix = "refs/tags/" + if !strings.HasPrefix(tag, tagPrefix) { + tag = tagPrefix + tag + } + if gf.Ref != "" && gf.Ref != tag { + return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, tag) + } + gf.Ref = tag + } + if branch != "" { + if tag != "" { + // TODO: consider allowing this, when the tag actually exists on the branch + return errors.New("branch conflicts with tag") + } + const branchPrefix = "refs/heads/" + if !strings.HasPrefix(branch, branchPrefix) { + branch = branchPrefix + branch + } + if gf.Ref != "" && gf.Ref != branch { + return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, branch) + } + gf.Ref = branch + } + if gf.Checksum != "" && gf.Ref == "" { + gf.Ref = gf.Checksum + } + return nil +} diff --git a/frontend/dockerfile/dfgitutil/git_ref_test.go b/frontend/dockerfile/dfgitutil/git_ref_test.go index 31156daafe88..0a8d53a168d2 100644 --- a/frontend/dockerfile/dfgitutil/git_ref_test.go +++ b/frontend/dockerfile/dfgitutil/git_ref_test.go @@ -1,6 +1,7 @@ package dfgitutil import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -10,6 +11,7 @@ func TestParseGitRef(t *testing.T) { cases := []struct { ref string expected *GitRef + err string }{ { ref: "https://example.com/", @@ -31,7 +33,7 @@ func TestParseGitRef(t *testing.T) { expected: &GitRef{ Remote: "https://example.com/foo.git", ShortName: "foo", - Commit: "deadbeef", + Ref: "deadbeef", }, }, { @@ -39,7 +41,7 @@ func TestParseGitRef(t *testing.T) { expected: &GitRef{ Remote: "https://example.com/foo.git", ShortName: "foo", - Commit: "release/1.2", + Ref: "release/1.2", }, }, { @@ -66,6 +68,15 @@ func TestParseGitRef(t *testing.T) { IndistinguishableFromLocal: true, }, }, + { + ref: "github.com/moby/buildkit#master", + expected: &GitRef{ + Remote: "github.com/moby/buildkit", + ShortName: "buildkit", + IndistinguishableFromLocal: true, + Ref: "master", + }, + }, { ref: "custom.xyz/moby/buildkit.git", expected: nil, @@ -114,7 +125,7 @@ func TestParseGitRef(t *testing.T) { expected: &GitRef{ Remote: "https://github.com/foo/bar.git", ShortName: "bar", - Commit: "baz/qux", + Ref: "baz/qux", SubDir: "quux/quuz", }, }, @@ -143,17 +154,80 @@ func TestParseGitRef(t *testing.T) { expected: &GitRef{ Remote: "https://github.com/docker/docker.git", ShortName: "docker", - Commit: "v1.0.0", + Ref: "v1.0.0", + SubDir: "/subdir", + }, + }, + { + ref: "https://github.com/moby/buildkit.git?subdir=/subdir#v1.0.0", + expected: &GitRef{ + Remote: "https://github.com/moby/buildkit.git", + ShortName: "buildkit", + Ref: "v1.0.0", + SubDir: "/subdir", + }, + }, + { + ref: "https://github.com/moby/buildkit.git?tag=v1.0.0", + expected: &GitRef{ + Remote: "https://github.com/moby/buildkit.git", + ShortName: "buildkit", + Ref: "refs/tags/v1.0.0", + }, + }, + { + ref: "github.com/moby/buildkit?tag=v1.0.0", + expected: &GitRef{ + Remote: "github.com/moby/buildkit", + ShortName: "buildkit", + Ref: "refs/tags/v1.0.0", + IndistinguishableFromLocal: true, + }, + }, + { + ref: "https://github.com/moby/buildkit.git?branch=v1.0", + expected: &GitRef{ + Remote: "https://github.com/moby/buildkit.git", + ShortName: "buildkit", + Ref: "refs/heads/v1.0", + }, + }, + { + ref: "https://github.com/moby/buildkit.git?ref=v1.0.0#v1.2.3", + err: "ref conflicts", + }, + { + ref: "https://github.com/moby/buildkit.git?ref=v1.0.0&tag=v1.2.3", + err: "ref conflicts", + }, + { + // TODO: consider allowing this, when the tag actually exists on the branch + ref: "https://github.com/moby/buildkit.git?tag=v1.0.0&branch=v1.0", + err: "branch conflicts with tag", + }, + { + ref: "git@github.com:moby/buildkit.git?subdir=/subdir#v1.0.0", + expected: &GitRef{ + Remote: "git@github.com:moby/buildkit.git", + ShortName: "buildkit", + Ref: "v1.0.0", SubDir: "/subdir", }, }, + { + ref: "https://github.com/moby/buildkit.git?invalid=123", + err: "unexpected query \"invalid\"", + }, } - for _, tt := range cases { - t.Run(tt.ref, func(t *testing.T) { + for i, tt := range cases { + t.Run(fmt.Sprintf("case%d", i+1), func(t *testing.T) { got, err := ParseGitRef(tt.ref) if tt.expected == nil { require.Nil(t, got) require.Error(t, err) + if tt.err != "" { + require.ErrorContains(t, err, tt.err) + } } else { require.NoError(t, err) require.Equal(t, tt.expected, got) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index f1270728f285..97e4c802f37f 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -1513,7 +1513,7 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error { return errors.New("source can't be a git ref for COPY") } // TODO: print a warning (not an error) if gitRef.UnencryptedTCP is true - commit := gitRef.Commit + commit := gitRef.Ref if gitRef.SubDir != "" { commit += ":" + gitRef.SubDir } diff --git a/frontend/dockerui/context.go b/frontend/dockerui/context.go index 8aa6a2d8a174..5e390337bf4f 100644 --- a/frontend/dockerui/context.go +++ b/frontend/dockerui/context.go @@ -145,7 +145,7 @@ func DetectGitContext(ref string, keepGit bool) (*llb.State, bool) { if err != nil { return nil, false } - commit := g.Commit + commit := g.Ref if g.SubDir != "" { commit += ":" + g.SubDir } diff --git a/util/gitutil/git_url.go b/util/gitutil/git_url.go index f81ad1676088..80717a67a660 100644 --- a/util/gitutil/git_url.go +++ b/util/gitutil/git_url.go @@ -47,9 +47,10 @@ type GitURL struct { Path string // User is the username/password to access the host User *url.Userinfo + // Query is the query parameters for the URL + Query url.Values // Opts can contain additional metadata Opts *GitURLOpts - // Remote is a valid URL remote to pass into the Git CLI tooling (i.e. // without the fragment metadata) Remote string @@ -66,63 +67,12 @@ type GitURLOpts struct { // parseOpts splits a git URL fragment into its respective git // reference and subdirectory components. -func parseOpts(fragment string, query url.Values) (*GitURLOpts, error) { - if fragment == "" && len(query) == 0 { - return nil, nil - } - opts := &GitURLOpts{} - if fragment != "" { - opts.Ref, opts.Subdir, _ = strings.Cut(fragment, ":") - } - var tag, branch string - for k, v := range query { - switch len(v) { - case 0: - return nil, errors.Errorf("query %q has no value", k) - case 1: - if v[0] == "" { - return nil, errors.Errorf("query %q has no value", k) - } - // NOP - default: - return nil, errors.Errorf("query %q has multiple values", k) - } - switch k { - case "ref": - if opts.Ref != "" && opts.Ref != v[0] { - return nil, errors.Errorf("ref conflicts: %q vs %q", opts.Ref, v[0]) - } - opts.Ref = v[0] - case "tag": - tag = v[0] - case "branch": - branch = v[0] - case "subdir": - if opts.Subdir != "" && opts.Subdir != v[0] { - return nil, errors.Errorf("subdir conflicts: %q vs %q", opts.Subdir, v[0]) - } - opts.Subdir = v[0] - default: - return nil, errors.Errorf("unexpected query %q", k) - } - } - if tag != "" { - if opts.Ref != "" { - return nil, errors.New("tag conflicts with ref") - } - opts.Ref = "refs/tags/" + tag - } - if branch != "" { - if tag != "" { - // TODO: consider allowing this, when the tag actually exists on the branch - return nil, errors.New("branch conflicts with tag") - } - if opts.Ref != "" { - return nil, errors.New("branch conflicts with ref") - } - opts.Ref = "refs/heads/" + branch +func parseOpts(fragment string) *GitURLOpts { + if fragment == "" { + return nil } - return opts, nil + ref, subdir, _ := strings.Cut(fragment, ":") + return &GitURLOpts{Ref: ref, Subdir: subdir} } // ParseURL parses a BuildKit-style Git URL (that may contain additional @@ -160,16 +110,17 @@ func FromURL(url *url.URL) (*GitURL, error) { withoutOpts := *url withoutOpts.Fragment = "" withoutOpts.RawQuery = "" - opts, err := parseOpts(url.Fragment, url.Query()) - if err != nil { - return nil, err + q := url.Query() + if len(q) == 0 { + q = nil } return &GitURL{ Scheme: url.Scheme, User: url.User, Host: url.Host, Path: url.Path, - Opts: opts, + Query: q, + Opts: parseOpts(url.Fragment), Remote: withoutOpts.String(), }, nil } @@ -178,16 +129,17 @@ func fromSCPStyleURL(url *sshutil.SCPStyleURL) (*GitURL, error) { withoutOpts := *url withoutOpts.Fragment = "" withoutOpts.Query = nil - opts, err := parseOpts(url.Fragment, url.Query) - if err != nil { - return nil, err + q := url.Query + if len(q) == 0 { + q = nil } return &GitURL{ Scheme: SSHProtocol, User: url.User, Host: url.Host, Path: url.Path, - Opts: opts, + Query: q, + Opts: parseOpts(url.Fragment), Remote: withoutOpts.String(), }, nil } diff --git a/util/gitutil/git_url_test.go b/util/gitutil/git_url_test.go index be3409a413ea..2cef897f2d9f 100644 --- a/util/gitutil/git_url_test.go +++ b/util/gitutil/git_url_test.go @@ -152,54 +152,18 @@ func TestParseURL(t *testing.T) { }, }, { - url: "https://github.com/moby/buildkit?ref=v1.0.0&subdir=/subdir", + url: "https://github.com/moby/buildkit?ref=v1.0.0&foo=bar#v1.2.3", result: GitURL{ Scheme: HTTPSProtocol, Host: "github.com", Path: "/moby/buildkit", - Opts: &GitURLOpts{Ref: "v1.0.0", Subdir: "/subdir"}, + Opts: &GitURLOpts{Ref: "v1.2.3"}, + Query: url.Values{ + "ref": {"v1.0.0"}, + "foo": {"bar"}, + }, }, }, - { - url: "https://github.com/moby/buildkit?subdir=/subdir#v1.0.0", - result: GitURL{ - Scheme: HTTPSProtocol, - Host: "github.com", - Path: "/moby/buildkit", - Opts: &GitURLOpts{Ref: "v1.0.0", Subdir: "/subdir"}, - }, - }, - { - url: "https://github.com/moby/buildkit?tag=v1.0.0", - result: GitURL{ - Scheme: HTTPSProtocol, - Host: "github.com", - Path: "/moby/buildkit", - Opts: &GitURLOpts{Ref: "refs/tags/v1.0.0"}, - }, - }, - { - url: "https://github.com/moby/buildkit?branch=v1.0", - result: GitURL{ - Scheme: HTTPSProtocol, - Host: "github.com", - Path: "/moby/buildkit", - Opts: &GitURLOpts{Ref: "refs/heads/v1.0"}, - }, - }, - { - url: "https://github.com/moby/buildkit?ref=v1.0.0#v1.2.3", - err: true, - }, - { - url: "https://github.com/moby/buildkit?ref=v1.0.0&tag=v1.2.3", - err: true, - }, - { - // 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", - err: true, - }, { url: "git@github.com:moby/buildkit.git?subdir=/subdir#v1.0.0", result: GitURL{ @@ -207,7 +171,12 @@ func TestParseURL(t *testing.T) { Host: "github.com", Path: "moby/buildkit.git", User: url.User("git"), - Opts: &GitURLOpts{Ref: "v1.0.0", Subdir: "/subdir"}, + Opts: &GitURLOpts{ + Ref: "v1.0.0", + }, + Query: url.Values{ + "subdir": {"/subdir"}, + }, }, }, } @@ -222,6 +191,7 @@ func TestParseURL(t *testing.T) { require.Equal(t, test.result.Host, remote.Host) require.Equal(t, test.result.Path, remote.Path) require.Equal(t, test.result.Opts, remote.Opts) + require.Equal(t, test.result.Query, remote.Query) require.Equal(t, test.result.User.String(), remote.User.String()) } }) From 81599425cbdf7a6ea417ede425b34a4a02094b0c Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 28 Aug 2025 16:10:49 -0700 Subject: [PATCH 3/6] llb: update Git to allow normalized property passing Signed-off-by: Tonis Tiigi --- client/llb/git_test.go | 94 +++++++++++++++++++ client/llb/source.go | 45 ++++++--- frontend/dockerfile/dfgitutil/git_ref.go | 22 ++--- frontend/dockerfile/dfgitutil/git_ref_test.go | 2 +- frontend/dockerfile/dockerfile2llb/convert.go | 20 ++-- frontend/dockerui/context.go | 28 ++++-- frontend/dockerui/namedcontext.go | 10 +- 7 files changed, 177 insertions(+), 44 deletions(-) create mode 100644 client/llb/git_test.go diff --git a/client/llb/git_test.go b/client/llb/git_test.go new file mode 100644 index 000000000000..38724981928e --- /dev/null +++ b/client/llb/git_test.go @@ -0,0 +1,94 @@ +package llb + +import ( + "context" + "testing" + + "github.com/moby/buildkit/solver/pb" + "github.com/stretchr/testify/require" +) + +func TestGit(t *testing.T) { + t.Parallel() + + type tcase struct { + name string + st State + identifier string + attrs map[string]string + } + + tcases := []tcase{ + { + name: "refarg", + st: Git("github.com/foo/bar.git", "ref"), + identifier: "git://github.com/foo/bar.git#ref", + attrs: map[string]string{ + "git.authheadersecret": "GIT_AUTH_HEADER", + "git.authtokensecret": "GIT_AUTH_TOKEN", + "git.fullurl": "https://github.com/foo/bar.git", + }, + }, + { + name: "refarg with subdir", + st: Git("github.com/foo/bar.git", "ref:subdir"), + identifier: "git://github.com/foo/bar.git#ref:subdir", + attrs: map[string]string{ + "git.authheadersecret": "GIT_AUTH_HEADER", + "git.authtokensecret": "GIT_AUTH_TOKEN", + "git.fullurl": "https://github.com/foo/bar.git", + }, + }, + { + name: "refarg with subdir func", + st: Git("github.com/foo/bar.git", "ref", GitSubDir("subdir")), + identifier: "git://github.com/foo/bar.git#ref:subdir", + attrs: map[string]string{ + "git.authheadersecret": "GIT_AUTH_HEADER", + "git.authtokensecret": "GIT_AUTH_TOKEN", + "git.fullurl": "https://github.com/foo/bar.git", + }, + }, + { + name: "refarg with override", + st: Git("github.com/foo/bar.git", "ref:dir", GitRef("v1.0")), + identifier: "git://github.com/foo/bar.git#v1.0:dir", + attrs: map[string]string{ + "git.authheadersecret": "GIT_AUTH_HEADER", + "git.authtokensecret": "GIT_AUTH_TOKEN", + "git.fullurl": "https://github.com/foo/bar.git", + }, + }, + { + name: "funcs only", + st: Git("github.com/foo/bar.git", "", GitRef("v1.0"), GitSubDir("dir")), + identifier: "git://github.com/foo/bar.git#v1.0:dir", + attrs: map[string]string{ + "git.authheadersecret": "GIT_AUTH_HEADER", + "git.authtokensecret": "GIT_AUTH_TOKEN", + "git.fullurl": "https://github.com/foo/bar.git", + }, + }, + } + + for _, tc := range tcases { + t.Run(tc.name, func(t *testing.T) { + st := tc.st + def, err := st.Marshal(context.TODO()) + + require.NoError(t, err) + + m, arr := parseDef(t, def.Def) + require.Equal(t, 2, len(arr)) + + dgst, idx := last(t, arr) + require.Equal(t, 0, idx) + require.Equal(t, m[dgst], arr[0]) + + g := arr[0].Op.(*pb.Op_Source).Source + + require.Equal(t, tc.identifier, g.Identifier) + require.Equal(t, tc.attrs, g.Attrs) + }) + } +} diff --git a/client/llb/source.go b/client/llb/source.go index d6eba0758981..23197d99403e 100644 --- a/client/llb/source.go +++ b/client/llb/source.go @@ -249,7 +249,7 @@ const ( // // By default the git repository is cloned with `--depth=1` to reduce the amount of data downloaded. // Additionally the ".git" directory is removed after the clone, you can keep ith with the [KeepGitDir] [GitOption]. -func Git(url, ref string, opts ...GitOption) State { +func Git(url, fragment string, opts ...GitOption) State { remote, err := gitutil.ParseURL(url) if errors.Is(err, gitutil.ErrUnknownProtocol) { url = "https://" + url @@ -259,6 +259,20 @@ func Git(url, ref string, opts ...GitOption) State { url = remote.Remote } + gi := &GitInfo{ + AuthHeaderSecret: GitAuthHeaderKey, + AuthTokenSecret: GitAuthTokenKey, + } + ref, subdir, ok := strings.Cut(fragment, ":") + if ref != "" { + GitRef(ref).SetGitOption(gi) + } + if ok && subdir != "" { + GitSubDir(subdir).SetGitOption(gi) + } + for _, o := range opts { + o.SetGitOption(gi) + } var id string if err != nil { // If we can't parse the URL, just use the full URL as the ID. The git @@ -269,18 +283,13 @@ func Git(url, ref string, opts ...GitOption) State { // for different protocols (e.g. https and ssh) that have the same // host/path/fragment combination. id = remote.Host + path.Join("/", remote.Path) - if ref != "" { - id += "#" + ref + if gi.Ref != "" || gi.SubDir != "" { + id += "#" + gi.Ref + if gi.SubDir != "" { + id += ":" + gi.SubDir + } } } - - gi := &GitInfo{ - AuthHeaderSecret: GitAuthHeaderKey, - AuthTokenSecret: GitAuthTokenKey, - } - for _, o := range opts { - o.SetGitOption(gi) - } attrs := map[string]string{} if gi.KeepGitDir { attrs[pb.AttrKeepGitDir] = "true" @@ -352,6 +361,20 @@ type GitInfo struct { KnownSSHHosts string MountSSHSock string Checksum string + Ref string + SubDir string +} + +func GitRef(v string) GitOption { + return gitOptionFunc(func(gi *GitInfo) { + gi.Ref = v + }) +} + +func GitSubDir(v string) GitOption { + return gitOptionFunc(func(gi *GitInfo) { + gi.SubDir = v + }) } func KeepGitDir() GitOption { diff --git a/frontend/dockerfile/dfgitutil/git_ref.go b/frontend/dockerfile/dfgitutil/git_ref.go index 1aaf340a5d68..54fb775750e1 100644 --- a/frontend/dockerfile/dfgitutil/git_ref.go +++ b/frontend/dockerfile/dfgitutil/git_ref.go @@ -2,7 +2,6 @@ package dfgitutil import ( - "log" "net/url" "strings" @@ -52,10 +51,8 @@ type GitRef struct { UnencryptedTCP bool } -// var gitURLPathWithFragmentSuffix = regexp.MustCompile(`\.git(?:#.+)?$`) - // ParseGitRef parses a git ref. -func ParseGitRef(ref string) (*GitRef, error) { +func ParseGitRef(ref string) (*GitRef, bool, error) { res := &GitRef{} var ( @@ -64,25 +61,25 @@ func ParseGitRef(ref string) (*GitRef, error) { ) if strings.HasPrefix(ref, "./") || strings.HasPrefix(ref, "../") { - return nil, errors.WithStack(cerrdefs.ErrInvalidArgument) + return nil, false, errors.WithStack(cerrdefs.ErrInvalidArgument) } else if strings.HasPrefix(ref, "github.com/") { res.IndistinguishableFromLocal = true // Deprecated u, err := url.Parse(ref) if err != nil { - return nil, err + return nil, false, err } u.Scheme = "https" remote, err = gitutil.FromURL(u) if err != nil { - return nil, err + return nil, false, err } } else { remote, err = gitutil.ParseURL(ref) if errors.Is(err, gitutil.ErrUnknownProtocol) { - return nil, err + return nil, false, err } if err != nil { - return nil, err + return nil, false, err } switch remote.Scheme { @@ -94,7 +91,7 @@ func ParseGitRef(ref string) (*GitRef, error) { // An HTTP(S) URL is considered to be a valid git ref only when it has the ".git[...]" suffix. case gitutil.HTTPProtocol, gitutil.HTTPSProtocol: if !strings.HasSuffix(remote.Path, ".git") { - return nil, errors.WithStack(cerrdefs.ErrInvalidArgument) + return nil, false, errors.WithStack(cerrdefs.ErrInvalidArgument) } } } @@ -111,11 +108,10 @@ func ParseGitRef(ref string) (*GitRef, error) { res.ShortName = strings.TrimSuffix(repoSplitBySlash[len(repoSplitBySlash)-1], ".git") if err := res.loadQuery(remote.Query); err != nil { - log.Printf("query") - return nil, err + return nil, true, err } - return res, nil + return res, true, nil } func (gf *GitRef) loadQuery(query url.Values) error { diff --git a/frontend/dockerfile/dfgitutil/git_ref_test.go b/frontend/dockerfile/dfgitutil/git_ref_test.go index 0a8d53a168d2..a391e81168e7 100644 --- a/frontend/dockerfile/dfgitutil/git_ref_test.go +++ b/frontend/dockerfile/dfgitutil/git_ref_test.go @@ -221,7 +221,7 @@ func TestParseGitRef(t *testing.T) { } for i, tt := range cases { t.Run(fmt.Sprintf("case%d", i+1), func(t *testing.T) { - got, err := ParseGitRef(tt.ref) + got, _, err := ParseGitRef(tt.ref) if tt.expected == nil { require.Nil(t, got) require.Error(t, err) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 97e4c802f37f..1334f8b2672d 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -1507,24 +1507,30 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error { for _, src := range cfg.params.SourcePaths { commitMessage.WriteString(" " + src) - gitRef, gitRefErr := dfgitutil.ParseGitRef(src) + gitRef, isGit, gitRefErr := dfgitutil.ParseGitRef(src) + if gitRefErr != nil && isGit { + return gitRefErr + } if gitRefErr == nil && !gitRef.IndistinguishableFromLocal { if !cfg.isAddCommand { return errors.New("source can't be a git ref for COPY") } // TODO: print a warning (not an error) if gitRef.UnencryptedTCP is true - commit := gitRef.Ref - if gitRef.SubDir != "" { - commit += ":" + gitRef.SubDir + gitOptions := []llb.GitOption{ + llb.WithCustomName(pgName), + llb.GitRef(gitRef.Ref), } - gitOptions := []llb.GitOption{llb.WithCustomName(pgName)} if cfg.keepGitDir { gitOptions = append(gitOptions, llb.KeepGitDir()) } if cfg.checksum != "" { gitOptions = append(gitOptions, llb.GitChecksum(cfg.checksum)) } - st := llb.Git(gitRef.Remote, commit, gitOptions...) + if gitRef.SubDir != "" { + gitOptions = append(gitOptions, llb.GitSubDir(gitRef.SubDir)) + } + + st := llb.Git(gitRef.Remote, "", gitOptions...) opts := append([]llb.CopyOption{&llb.CopyInfo{ Mode: chopt, CreateDestPath: true, @@ -2268,7 +2274,7 @@ func isHTTPSource(src string) bool { func isGitSource(src string) bool { // https://github.com/ORG/REPO.git is a git source, not an http source - if gitRef, gitErr := dfgitutil.ParseGitRef(src); gitRef != nil && gitErr == nil { + if gitRef, isGit, _ := dfgitutil.ParseGitRef(src); gitRef != nil && isGit { return true } return false diff --git a/frontend/dockerui/context.go b/frontend/dockerui/context.go index 5e390337bf4f..506cb21fffee 100644 --- a/frontend/dockerui/context.go +++ b/frontend/dockerui/context.go @@ -73,7 +73,10 @@ func (bc *Client) initContext(ctx context.Context) (*buildContext, error) { if v, err := strconv.ParseBool(opts[keyContextKeepGitDirArg]); err == nil { keepGit = v } - if st, ok := DetectGitContext(opts[localNameContext], keepGit); ok { + if st, ok, err := DetectGitContext(opts[localNameContext], keepGit); ok { + if err != nil { + return nil, err + } bctx.context = st bctx.dockerfile = st } else if st, filename, ok := DetectHTTPContext(opts[localNameContext]); ok { @@ -140,22 +143,27 @@ func (bc *Client) initContext(ctx context.Context) (*buildContext, error) { return bctx, nil } -func DetectGitContext(ref string, keepGit bool) (*llb.State, bool) { - g, err := dfgitutil.ParseGitRef(ref) +func DetectGitContext(ref string, keepGit bool) (*llb.State, bool, error) { + g, isGit, err := dfgitutil.ParseGitRef(ref) if err != nil { - return nil, false + return nil, isGit, err } - commit := g.Ref - if g.SubDir != "" { - commit += ":" + g.SubDir + gitOpts := []llb.GitOption{ + llb.GitRef(g.Ref), + WithInternalName("load git source " + ref), } - gitOpts := []llb.GitOption{WithInternalName("load git source " + ref)} if keepGit { gitOpts = append(gitOpts, llb.KeepGitDir()) } + if g.SubDir != "" { + gitOpts = append(gitOpts, llb.GitSubDir(g.SubDir)) + } + if g.Checksum != "" { + gitOpts = append(gitOpts, llb.GitChecksum(g.Checksum)) + } - st := llb.Git(g.Remote, commit, gitOpts...) - return &st, true + st := llb.Git(g.Remote, "", gitOpts...) + return &st, true, nil } func DetectHTTPContext(ref string) (*llb.State, string, bool) { diff --git a/frontend/dockerui/namedcontext.go b/frontend/dockerui/namedcontext.go index 9598d0b2d9bc..78e8b1dc4fe4 100644 --- a/frontend/dockerui/namedcontext.go +++ b/frontend/dockerui/namedcontext.go @@ -138,17 +138,23 @@ func (nc *NamedContext) load(ctx context.Context, count int) (*llb.State, *docke } return &st, &img, nil case "git": - st, ok := DetectGitContext(nc.input, true) + st, ok, err := DetectGitContext(nc.input, true) if !ok { return nil, nil, errors.Errorf("invalid git context %s", nc.input) } + if err != nil { + return nil, nil, err + } return st, nil, nil case "http", "https": - st, ok := DetectGitContext(nc.input, true) + st, ok, err := DetectGitContext(nc.input, true) if !ok { httpst := llb.HTTP(nc.input, llb.WithCustomName("[context "+nc.nameWithPlatform+"] "+nc.input)) st = &httpst } + if err != nil { + return nil, nil, err + } return st, nil, nil case "oci-layout": refSpec := strings.TrimPrefix(vv[1], "//") From 1ef2851e4d5f9a5c0427349d798c11a483c9df08 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 28 Aug 2025 19:24:18 -0700 Subject: [PATCH 4/6] dockerfile: add tests for querystring Git URLs Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dockerfile_addgit_test.go | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) diff --git a/frontend/dockerfile/dockerfile_addgit_test.go b/frontend/dockerfile/dockerfile_addgit_test.go index 60b56de630a7..d10cfc919c7d 100644 --- a/frontend/dockerfile/dockerfile_addgit_test.go +++ b/frontend/dockerfile/dockerfile_addgit_test.go @@ -2,6 +2,7 @@ package dockerfile import ( "bytes" + "fmt" "net/http" "net/http/httptest" "os" @@ -22,6 +23,7 @@ import ( var addGitTests = integration.TestFuncs( testAddGit, testAddGitChecksumCache, + testGitQueryString, ) func init() { @@ -350,6 +352,179 @@ COPY --from=src /repo/unique.txt / require.Equal(t, string(unique1), string(unique2), "cache should be matched and unique file content should be the same") } +func testGitQueryString(t *testing.T, sb integration.Sandbox) { + integration.SkipOnPlatform(t, "windows") + f := getFrontend(t, sb) + + gitDir, err := os.MkdirTemp("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(gitDir) + err = runShell(gitDir, []string{ + "git init", + "git config --local user.email test", + "git config --local user.name test", + "echo base >foo", + }...) + require.NoError(t, err) + + err = os.WriteFile(filepath.Join(gitDir, "Dockerfile"), []byte(` +FROM scratch +COPY foo out +`), 0600) + require.NoError(t, err) + + err = runShell(gitDir, []string{ + "git add Dockerfile foo", + "git commit -m initial", + "git tag v0.0.1", + "git branch base", + "echo feature >foo", + "mkdir sub", + "echo subfeature >sub/foo", + "cp Dockerfile sub/", + "git add foo sub", + "git commit -m feature", + "git branch feature", + "git checkout -B master base", + "echo v0.0.2 >foo", + "git add foo", + "git commit -m v0.0.2", + "git tag v0.0.2", + "echo latest >foo", + "git add foo", + "git commit -m latest", + "git tag latest", + "git update-server-info", + }...) + require.NoError(t, err) + + server := httptest.NewServer(http.FileServer(http.Dir(filepath.Clean(gitDir)))) + defer server.Close() + serverURL := server.URL + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + type tcase struct { + name string + url string + expectOut string + expectErr string + } + + tcases := []tcase{ + { + name: "old style ref", + url: serverURL + "/.git#v0.0.2", + expectOut: "v0.0.2\n", + }, + { + name: "querystring ref", + url: serverURL + "/.git?ref=base", + expectOut: "base\n", + }, + { + name: "querystring branch", + url: serverURL + "/.git?branch=base", + expectOut: "base\n", + }, + { + name: "querystring invalid branch", + url: serverURL + "/.git?branch=invalid", + expectErr: "repository does not contain ref", + }, + { + name: "tag as branch", + url: serverURL + "/.git?branch=v0.0.2", + expectErr: "repository does not contain ref", + }, + { + name: "allowed mixed refs", + url: serverURL + "/.git?tag=v0.0.2#refs/tags/v0.0.2", + expectOut: "v0.0.2\n", + }, + { + name: "mismatch refs", + url: serverURL + "/.git?tag=v0.0.2#refs/heads/master", + expectErr: "ref conflicts", + }, + { + name: "sub old-style", + url: serverURL + "/.git#feature:sub", + expectOut: "subfeature\n", + }, + { + name: "sub query", + url: serverURL + "/.git?subdir=sub&ref=feature", + expectOut: "subfeature\n", + }, + } + + for _, tc := range tcases { + t.Run("context_"+tc.name, func(t *testing.T) { + dest := t.TempDir() + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + FrontendAttrs: map[string]string{ + "context": tc.url, + }, + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: dest, + }, + }, + }, nil) + if tc.expectErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectErr) + return + } + require.NoError(t, err) + + dt, err := os.ReadFile(filepath.Join(dest, "out")) + require.NoError(t, err) + require.Equal(t, tc.expectOut, string(dt)) + }) + } + + for _, tc := range tcases { + dockerfile2 := fmt.Sprintf(` +FROM scratch +ADD %s /repo/ + `, tc.url) + inDir := integration.Tmpdir(t, + fstest.CreateFile("Dockerfile", []byte(dockerfile2), 0600), + ) + t.Run("add_"+tc.name, func(t *testing.T) { + dest := t.TempDir() + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: dest, + }, + }, + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: inDir, + dockerui.DefaultLocalNameContext: inDir, + }, + }, nil) + if tc.expectErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectErr) + return + } + require.NoError(t, err) + + dt, err := os.ReadFile(filepath.Join(dest, "/repo/foo")) + require.NoError(t, err) + require.Equal(t, tc.expectOut, string(dt)) + }) + } + +} + func applyTemplate(tmpl string, x any) (string, error) { var buf bytes.Buffer parsed, err := template.New("").Parse(tmpl) From 8ef9b54fda0fd13f86912f7db3beb1f10389dc1d Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 28 Aug 2025 19:29:27 -0700 Subject: [PATCH 5/6] llb: document fragment parameter in llb.Git Signed-off-by: Tonis Tiigi --- client/llb/source.go | 4 ++++ frontend/dockerfile/dockerfile_addgit_test.go | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/client/llb/source.go b/client/llb/source.go index 23197d99403e..b4e4412f54a2 100644 --- a/client/llb/source.go +++ b/client/llb/source.go @@ -247,6 +247,10 @@ const ( // Formats that utilize SSH may need to supply credentials as a [GitOption]. // You may need to check the source code for a full list of supported formats. // +// Fragment can be used to pass ref:subdir format that can set in (old-style) +// Docker Git URL format after # . This is provided for backwards compatibility. +// It is recommended to leave it empty and call GitRef(), GitSubdir() options instead. +// // By default the git repository is cloned with `--depth=1` to reduce the amount of data downloaded. // Additionally the ".git" directory is removed after the clone, you can keep ith with the [KeepGitDir] [GitOption]. func Git(url, fragment string, opts ...GitOption) State { diff --git a/frontend/dockerfile/dockerfile_addgit_test.go b/frontend/dockerfile/dockerfile_addgit_test.go index d10cfc919c7d..e6942b40109a 100644 --- a/frontend/dockerfile/dockerfile_addgit_test.go +++ b/frontend/dockerfile/dockerfile_addgit_test.go @@ -522,7 +522,6 @@ ADD %s /repo/ require.Equal(t, tc.expectOut, string(dt)) }) } - } func applyTemplate(tmpl string, x any) (string, error) { From 3afd92a996c767970eccde56f0505fb7e4f1d1c0 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 28 Aug 2025 20:17:02 -0700 Subject: [PATCH 6/6] dockerfile: add tests for Git query URL checksums Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dockerfile2llb/convert.go | 8 +++ frontend/dockerfile/dockerfile_addgit_test.go | 61 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 1334f8b2672d..f439ce5efd96 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -1523,6 +1523,14 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error { if cfg.keepGitDir { gitOptions = append(gitOptions, llb.KeepGitDir()) } + if cfg.checksum != "" && gitRef.Checksum != "" { + if cfg.checksum != gitRef.Checksum { + return errors.Errorf("checksum mismatch %q != %q", cfg.checksum, gitRef.Checksum) + } + } + if gitRef.Checksum != "" { + cfg.checksum = gitRef.Checksum + } if cfg.checksum != "" { gitOptions = append(gitOptions, llb.GitChecksum(cfg.checksum)) } diff --git a/frontend/dockerfile/dockerfile_addgit_test.go b/frontend/dockerfile/dockerfile_addgit_test.go index e6942b40109a..1c7dc1c30b1f 100644 --- a/frontend/dockerfile/dockerfile_addgit_test.go +++ b/frontend/dockerfile/dockerfile_addgit_test.go @@ -398,6 +398,23 @@ COPY foo out }...) require.NoError(t, err) + // get commit SHA for v0.0.2 + cmd := exec.Command("git", "rev-parse", "v0.0.2") + cmd.Dir = gitDir + dt, err := cmd.CombinedOutput() + require.NoError(t, err) + commitHashV2 := strings.TrimSpace(string(dt)) + require.Len(t, commitHashV2, 40) + + // get commit SHA for latest + cmd = exec.Command("git", "rev-parse", "latest") + cmd.Dir = gitDir + dt, err = cmd.CombinedOutput() + require.NoError(t, err) + commitHashLatest := strings.TrimSpace(string(dt)) + require.Len(t, commitHashLatest, 40) + require.NotEqual(t, commitHashV2, commitHashLatest) + server := httptest.NewServer(http.FileServer(http.Dir(filepath.Clean(gitDir)))) defer server.Close() serverURL := server.URL @@ -414,6 +431,12 @@ COPY foo out } tcases := []tcase{ + { + // if this commit is already cached then this will work and ignore tag atm because tag name has no influence to the output + name: "tag with invalid commit", + url: serverURL + "/.git?tag=v0.0.2&commit=" + commitHashLatest, + expectErr: "expected checksum to match", + }, { name: "old style ref", url: serverURL + "/.git#v0.0.2", @@ -444,6 +467,37 @@ COPY foo out url: serverURL + "/.git?tag=v0.0.2#refs/tags/v0.0.2", expectOut: "v0.0.2\n", }, + { + name: "v2 by commit", + url: serverURL + "/.git?commit=" + commitHashV2, + expectOut: "v0.0.2\n", + }, + { + name: "v2 ref by commit", + url: serverURL + "/.git?ref=" + commitHashV2, + expectOut: "v0.0.2\n", + }, + { + name: "tag with commit", + url: serverURL + "/.git?tag=v0.0.2&commit=" + commitHashV2, + expectOut: "v0.0.2\n", + }, + { + name: "commit with commit", + url: serverURL + "/.git?ref=" + commitHashV2 + "&commit=" + commitHashV2, + expectOut: "v0.0.2\n", + }, + { + name: "latest with commit", + url: serverURL + "/.git?commit=" + commitHashLatest, + expectOut: "latest\n", + }, + { + // this only works if there is already cache for commitHashLatest from previous case + name: "tag with invalid commit", + url: serverURL + "/.git?tag=v0.0.2&commit=" + commitHashLatest, + expectOut: "latest\n", + }, { name: "mismatch refs", url: serverURL + "/.git?tag=v0.0.2#refs/heads/master", @@ -488,6 +542,13 @@ COPY foo out }) } + cl, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + err = cl.Prune(sb.Context(), nil) + require.NoError(t, err) + for _, tc := range tcases { dockerfile2 := fmt.Sprintf(` FROM scratch