Merge pull request #43477 from thaJeztah/deprecate_urlutil

pkg/urlutil: deprecate, and move to builder/remotecontext/urlutil

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"

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"

builder/remotecontext/urlutil/urlutil.go

@@ -0,0 +1,88 @@
+// 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 (
+	"regexp"
+	"strings"
+)
+
+// urlPathWithFragmentSuffix matches fragments to use as Git reference and build
+// context from the Git repository. See IsGitURL for details.
+var urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$")
+
+// 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 strings.HasPrefix(str, "https://") || strings.HasPrefix(str, "http://")
+}
+
+// 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
+	}
+	for _, prefix := range []string{"git://", "github.com/", "git@"} {
+		if strings.HasPrefix(str, prefix) {
+			return true
+		}
+	}
+	return false
+}

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"
 
@@ -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)
-		}
-	}
-}

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" }

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

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)
+}

pkg/urlutil/urlutil.go

@@ -1,52 +0,0 @@
-// Package urlutil provides helper function to check urls kind.
-// It supports http urls, git urls and transport url (tcp://, …)
-package urlutil // import "github.com/docker/docker/pkg/urlutil"
-
-import (
-	"regexp"
-	"strings"
-)
-
-var (
-	validPrefixes = map[string][]string{
-		"url": {"http://", "https://"},
-
-		// 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@"},
-		"transport": {"tcp://", "tcp+tls://", "udp://", "unix://", "unixgram://"},
-	}
-	urlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$")
-)
-
-// IsURL returns true if the provided str is an HTTP(S) URL.
-func IsURL(str string) bool {
-	return checkURL(str, "url")
-}
-
-// IsGitURL returns true if the provided str is a git repository URL.
-func IsGitURL(str string) bool {
-	if IsURL(str) && urlPathWithFragmentSuffix.MatchString(str) {
-		return true
-	}
-	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) {
-			return true
-		}
-	}
-	return false
-}