From 074bc1c3ab6c6d506200a002b6e8809e4cf3ce38 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 10 Apr 2022 15:28:17 +0200 Subject: [PATCH 1/3] pkg/urlutil: remove unused IsTransportURL() This function is no longer used (either internally, or externally), so can be removed. Signed-off-by: Sebastiaan van Stijn --- pkg/urlutil/urlutil.go | 10 ++-------- pkg/urlutil/urlutil_test.go | 15 --------------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/pkg/urlutil/urlutil.go b/pkg/urlutil/urlutil.go index 9cf348c723..aafd4314b4 100644 --- a/pkg/urlutil/urlutil.go +++ b/pkg/urlutil/urlutil.go @@ -1,5 +1,5 @@ // Package urlutil provides helper function to check urls kind. -// It supports http urls, git urls and transport url (tcp://, …) +// It supports http and git urls. package urlutil // import "github.com/docker/docker/pkg/urlutil" import ( @@ -18,8 +18,7 @@ var ( // // Going forward, no additional prefixes should be added, and users should // be encouraged to use explicit URLs (https://github.com/user/repo.git) instead. - "git": {"git://", "github.com/", "git@"}, - "transport": {"tcp://", "tcp+tls://", "udp://", "unix://", "unixgram://"}, + "git": {"git://", "github.com/", "git@"}, } urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") ) @@ -37,11 +36,6 @@ func IsGitURL(str string) bool { return checkURL(str, "git") } -// IsTransportURL returns true if the provided str is a transport (tcp, tcp+tls, udp, unix) URL. -func IsTransportURL(str string) bool { - return checkURL(str, "transport") -} - func checkURL(str, kind string) bool { for _, prefix := range validPrefixes[kind] { if strings.HasPrefix(str, prefix) { diff --git a/pkg/urlutil/urlutil_test.go b/pkg/urlutil/urlutil_test.go index 6660368316..b1df56d508 100644 --- a/pkg/urlutil/urlutil_test.go +++ b/pkg/urlutil/urlutil_test.go @@ -18,13 +18,6 @@ var ( invalidGitUrls = []string{ "http://github.com/docker/docker.git:#branch", } - transportUrls = []string{ - "tcp://example.com", - "tcp+tls://example.com", - "udp://example.com", - "unix:///example", - "unixgram:///example", - } ) func TestIsGIT(t *testing.T) { @@ -46,11 +39,3 @@ func TestIsGIT(t *testing.T) { } } } - -func TestIsTransport(t *testing.T) { - for _, url := range transportUrls { - if !IsTransportURL(url) { - t.Fatalf("%q should be detected as valid Transport url", url) - } - } -} From 5f89a6a78e16b094e66e2878f092bda32459374f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 10 Apr 2022 16:57:45 +0200 Subject: [PATCH 2/3] pkg/urlutil: deprecate, and move to builder/remotecontext/urlutil pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts. - IsURL() only does a very rudimentary check for http(s):// prefixes, without any other validation, but due to its name may give incorrect expectations. - IsGitURL() is written specifically with docker build remote git contexts in mind, and has handling for backward-compatibility, where strings that are not URLs, but start with "github.com/" are accepted. Because of the above, this patch: - moves the package inside builder/remotecontext, close to where it's intended to be used (ideally this would be part of build/remotecontext itself, but this package imports many other dependencies, which would introduce those as extra dependencies in the CLI). - deprecates pkg/urlutil, but adds aliases as there are some external consumers. Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/copy.go | 2 +- builder/remotecontext/detect.go | 2 +- .../remotecontext}/urlutil/urlutil.go | 2 +- .../remotecontext}/urlutil/urlutil_test.go | 2 +- hack/make.ps1 | 6 ++++++ hack/validate/pkg-imports | 6 ++++++ pkg/urlutil/deprecated.go | 21 +++++++++++++++++++ 7 files changed, 37 insertions(+), 4 deletions(-) rename {pkg => builder/remotecontext}/urlutil/urlutil.go (93%) rename {pkg => builder/remotecontext}/urlutil/urlutil_test.go (91%) create mode 100644 pkg/urlutil/deprecated.go diff --git a/builder/dockerfile/copy.go b/builder/dockerfile/copy.go index 6d8b19beb1..8eb79140d8 100644 --- a/builder/dockerfile/copy.go +++ b/builder/dockerfile/copy.go @@ -16,6 +16,7 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/builder/remotecontext" + "github.com/docker/docker/builder/remotecontext/urlutil" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/containerfs" "github.com/docker/docker/pkg/idtools" @@ -23,7 +24,6 @@ import ( "github.com/docker/docker/pkg/progress" "github.com/docker/docker/pkg/streamformatter" "github.com/docker/docker/pkg/system" - "github.com/docker/docker/pkg/urlutil" "github.com/moby/buildkit/frontend/dockerfile/instructions" specs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" diff --git a/builder/remotecontext/detect.go b/builder/remotecontext/detect.go index 09f4d28434..3dae780275 100644 --- a/builder/remotecontext/detect.go +++ b/builder/remotecontext/detect.go @@ -11,9 +11,9 @@ import ( "github.com/containerd/continuity/driver" "github.com/docker/docker/api/types/backend" "github.com/docker/docker/builder" + "github.com/docker/docker/builder/remotecontext/urlutil" "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/fileutils" - "github.com/docker/docker/pkg/urlutil" "github.com/moby/buildkit/frontend/dockerfile/dockerignore" "github.com/moby/buildkit/frontend/dockerfile/parser" "github.com/pkg/errors" diff --git a/pkg/urlutil/urlutil.go b/builder/remotecontext/urlutil/urlutil.go similarity index 93% rename from pkg/urlutil/urlutil.go rename to builder/remotecontext/urlutil/urlutil.go index aafd4314b4..01827d925d 100644 --- a/pkg/urlutil/urlutil.go +++ b/builder/remotecontext/urlutil/urlutil.go @@ -1,6 +1,6 @@ // Package urlutil provides helper function to check urls kind. // It supports http and git urls. -package urlutil // import "github.com/docker/docker/pkg/urlutil" +package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" import ( "regexp" diff --git a/pkg/urlutil/urlutil_test.go b/builder/remotecontext/urlutil/urlutil_test.go similarity index 91% rename from pkg/urlutil/urlutil_test.go rename to builder/remotecontext/urlutil/urlutil_test.go index b1df56d508..6906118321 100644 --- a/pkg/urlutil/urlutil_test.go +++ b/builder/remotecontext/urlutil/urlutil_test.go @@ -1,4 +1,4 @@ -package urlutil // import "github.com/docker/docker/pkg/urlutil" +package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" import "testing" diff --git a/hack/make.ps1 b/hack/make.ps1 index ea6d5269b3..a183253e27 100644 --- a/hack/make.ps1 +++ b/hack/make.ps1 @@ -251,6 +251,12 @@ Function Validate-PkgImports($headCommit, $upstreamCommit) { $files=@(); $files = Invoke-Expression "git diff $upstreamCommit...$headCommit --diff-filter=ACMR --name-only -- `'pkg\*.go`'" $badFiles=@(); $files | ForEach-Object{ $file=$_ + if ($file -eq "pkg\urlutil\deprecated.go") { + # pkg/urlutil is deprecated, but has a temporary alias to help migration, + # see https://github.com/moby/moby/pull/43477 + # TODO(thaJeztah) remove this exception once pkg/urlutil aliases are removed + return + } # For the current changed file, get its list of dependencies, sorted and uniqued. $imports = Invoke-Expression "go list -e -f `'{{ .Deps }}`' $file" if ($LASTEXITCODE -ne 0) { Throw "Failed go list for dependencies on $file" } diff --git a/hack/validate/pkg-imports b/hack/validate/pkg-imports index f82e315f83..bba762d99f 100755 --- a/hack/validate/pkg-imports +++ b/hack/validate/pkg-imports @@ -10,6 +10,12 @@ unset IFS badFiles=() for f in "${files[@]}"; do + if [ "$f" = "pkg/urlutil/deprecated.go" ]; then + # pkg/urlutil is deprecated, but has a temporary alias to help migration, + # see https://github.com/moby/moby/pull/43477 + # TODO(thaJeztah) remove this exception once pkg/urlutil aliases are removed + continue + fi IFS=$'\n' badImports=($(go list -e -f '{{ join .Deps "\n" }}' "$f" | sort -u | grep -vE '^github.com/docker/docker/pkg/' | grep -vE '^github.com/docker/docker/vendor' | grep -E '^github.com/docker/docker' || true)) unset IFS diff --git a/pkg/urlutil/deprecated.go b/pkg/urlutil/deprecated.go new file mode 100644 index 0000000000..c09e3efd47 --- /dev/null +++ b/pkg/urlutil/deprecated.go @@ -0,0 +1,21 @@ +package urlutil // import "github.com/docker/docker/pkg/urlutil" + +import "github.com/docker/docker/builder/remotecontext/urlutil" + +// IsURL returns true if the provided str is an HTTP(S) URL. +// +// Deprecated: use github.com/docker/docker/builder/remotecontext/urlutil.IsURL +// to detect build-context type, or use strings.HasPrefix() to check if the +// string has a https:// or http:// prefix. +func IsURL(str string) bool { + // TODO(thaJeztah) when removing this alias, remove the exception from hack/validate/pkg-imports and hack/make.ps1 (Validate-PkgImports) + return urlutil.IsURL(str) +} + +// IsGitURL returns true if the provided str is a git repository URL. +// +// Deprecated: use github.com/docker/docker/builder/remotecontext/urlutil.IsGitURL +func IsGitURL(str string) bool { + // TODO(thaJeztah) when removing this alias, remove the exception from hack/validate/pkg-imports and hack/make.ps1 (Validate-PkgImports) + return urlutil.IsGitURL(str) +} From 449250994fb9834d172a019f4d5069cbe859346e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 10 Apr 2022 20:08:13 +0200 Subject: [PATCH 3/3] builder/remotecontext/urlutil: simplify and improve documentation Simplify some of the logic, and add documentation about the package, as well as warnings that this package should not be used as a general- purpose utility. Signed-off-by: Sebastiaan van Stijn --- builder/remotecontext/urlutil/urlutil.go | 92 +++++++++++++++++------- 1 file changed, 67 insertions(+), 25 deletions(-) diff --git a/builder/remotecontext/urlutil/urlutil.go b/builder/remotecontext/urlutil/urlutil.go index 01827d925d..d4078942e9 100644 --- a/builder/remotecontext/urlutil/urlutil.go +++ b/builder/remotecontext/urlutil/urlutil.go @@ -1,5 +1,8 @@ -// Package urlutil provides helper function to check urls kind. -// It supports http and git urls. +// Package urlutil provides helper function to check if a given build-context +// location should be considered a URL or a remote Git repository. +// +// This package is specifically written for use with docker build contexts, and +// should not be used as a general-purpose utility. package urlutil // import "github.com/docker/docker/builder/remotecontext/urlutil" import ( @@ -7,37 +10,76 @@ import ( "strings" ) -var ( - validPrefixes = map[string][]string{ - "url": {"http://", "https://"}, +// urlPathWithFragmentSuffix matches fragments to use as Git reference and build +// context from the Git repository. See IsGitURL for details. +var urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") - // The github.com/ prefix is a special case used to treat context-paths - // starting with `github.com` as a git URL if the given path does not - // exist locally. The "github.com/" prefix is kept for backward compatibility, - // and is a legacy feature. - // - // Going forward, no additional prefixes should be added, and users should - // be encouraged to use explicit URLs (https://github.com/user/repo.git) instead. - "git": {"git://", "github.com/", "git@"}, - } - urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") -) - -// IsURL returns true if the provided str is an HTTP(S) URL. +// IsURL returns true if the provided str is an HTTP(S) URL by checking if it +// has a http:// or https:// scheme. No validation is performed to verify if the +// URL is well-formed. func IsURL(str string) bool { - return checkURL(str, "url") + return strings.HasPrefix(str, "https://") || strings.HasPrefix(str, "http://") } -// IsGitURL returns true if the provided str is a git repository URL. +// IsGitURL returns true if the provided str is a remote git repository "URL". +// +// This function only performs a rudimentary check (no validation is performed +// to ensure the URL is well-formed), and is written specifically for use with +// docker build, with some logic for backward compatibility with older versions +// of docker: do not use this function as a general-purpose utility. +// +// The following patterns are considered to be a Git URL: +// +// - https://(.*).git(?:#.+)?$ git repository URL with optional fragment, as +// known to be used by GitHub and GitLab. +// - http://(.*).git(?:#.+)?$ same, but non-TLS +// - git://(.*) URLs using git:// scheme +// - git@(.*) +// - github.com/ see description below +// +// The github.com/ prefix is a special case used to treat context-paths +// starting with "github.com/" as a git URL if the given path does not +// exist locally. The "github.com/" prefix is kept for backward compatibility, +// and is a legacy feature. +// +// Going forward, no additional prefixes should be added, and users should +// be encouraged to use explicit URLs (https://github.com/user/repo.git) instead. +// +// Note that IsGitURL does not check if "github.com/" prefixes exist as a local +// path. Code using this function should check if the path exists locally before +// using it as a URL. +// +// Fragments +// +// Git URLs accept context configuration in their fragment section, separated by +// a colon (`:`). The first part represents the reference to check out, and can +// be either a branch, a tag, or a remote reference. The second part represents +// a subdirectory inside the repository to use as the build context. +// +// For example,the following URL uses a directory named "docker" in the branch +// "container" in the https://github.com/myorg/my-repo.git repository: +// +// https://github.com/myorg/my-repo.git#container:docker +// +// The following table represents all the valid suffixes with their build +// contexts: +// +// | Build Syntax Suffix | Git reference used | Build Context Used | +// |--------------------------------|----------------------|--------------------| +// | my-repo.git | refs/heads/master | / | +// | my-repo.git#mytag | refs/tags/my-tag | / | +// | my-repo.git#mybranch | refs/heads/my-branch | / | +// | my-repo.git#pull/42/head | refs/pull/42/head | / | +// | my-repo.git#:directory | refs/heads/master | /directory | +// | my-repo.git#master:directory | refs/heads/master | /directory | +// | my-repo.git#mytag:directory | refs/tags/my-tag | /directory | +// | my-repo.git#mybranch:directory | refs/heads/my-branch | /directory | +// func IsGitURL(str string) bool { if IsURL(str) && urlPathWithFragmentSuffix.MatchString(str) { return true } - return checkURL(str, "git") -} - -func checkURL(str, kind string) bool { - for _, prefix := range validPrefixes[kind] { + for _, prefix := range []string{"git://", "github.com/", "git@"} { if strings.HasPrefix(str, prefix) { return true }