pkg/urlutil: deprecate, and move to builder/remotecontext/urlutil by thaJeztah · Pull Request #43477 · moby/moby

thaJeztah · 2022-04-10T18:22:55Z

follow-up to #43476 (only last three commits are new)

pkg/urlutil: remove unused IsTransportURL()

This function is no longer used (either internally, or externally), so can be removed.

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.

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.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2022-04-11T19:52:43Z

@tonistiigi @crazy-max ptal

thaJeztah · 2022-04-11T19:55:41Z

builder/remotecontext/urlutil/urlutil.go

+//
+// | Build Syntax Suffix            | Git reference used   | Build Context Used |
+// |--------------------------------|----------------------|--------------------|
+// | my-repo.git                    | refs/heads/master    | /                  |


let me change this to main instead of master (and describe "default branch")

oh, actually, looks like we still have that hardcoded, so we'll have to look at that in a follow-up.

thaJeztah · 2022-04-11T22:10:38Z

Ah dang; can't create an alias;

These files import internal code: (either directly or indirectly)
  - pkg/urlutil/deprecated.go imports github.com/docker/docker/builder/remotecontext/urlutil

samuelkarp · 2022-04-12T08:25:56Z

pkg/urlutil/deprecated.go

+	// 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.
+	IsURL = urlutil.IsURL


Is there a particular reason you went with the variable approach here rather than providing these as functions? I can't think (off the top of my head) of any particular reason why it would break a consumer as-is, but (public) variables are writable globals, so it's conceivable that another package could modify these and inject unintended behavior into a consumer.

Bit of a habit (not sure if "good" or "bad") it's slightly easier to create a list of aliases when deprecating a package (which could be slightly clearer "hey, don't use this; here's where to look"), and in some cases may remove imports that are used in the signature of the aliased function 😅

and in some cases may remove imports that are used in the signature of the aliased function 😅

🤯 I can see why this can matter, though here both IsURL and IsGitURL are both func (string) bool, so there's nothing outside builtin types used.

Yup, you're right. In this case it doesn't bring much; let me change it

Updated 👍

This function is no longer used (either internally, or externally), so can be removed. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

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 <github@gone.nl>

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 <github@gone.nl>

tonistiigi

I wouldn't bother with pkg/urlutil and just remove it.

thaJeztah · 2022-04-12T21:34:58Z

Yeah, definitely not planning to keep it for long, but found it used in various external projects, so wanted to give a tiny amount of room for those to move away from it.

thaJeztah · 2022-04-12T23:17:42Z

Interesting error on CI (DCO check test)

git openssh-client && cd /workspace && hack/validate/dco
[2022-04-12T21:37:15.034Z] Unable to find image 'alpine:latest' locally
[2022-04-12T21:37:15.293Z] latest: Pulling from library/alpine
[2022-04-12T21:37:15.293Z] df9b9388f04a: Pulling fs layer
[2022-04-12T21:37:15.860Z] df9b9388f04a: Pull complete
[2022-04-12T21:37:15.860Z] Digest: sha256:4edbd2beb5f78b1014028f4fbb99f3237d9561100b6881aabbf5acce2c4f9454
[2022-04-12T21:37:15.860Z] Status: Downloaded newer image for alpine:latest
[2022-04-12T21:37:23.999Z] fatal: unsafe repository ('/workspace' is owned by someone else)
[2022-04-12T21:37:23.999Z] To add an exception for this directory, call:
[2022-04-12T21:37:23.999Z] 
[2022-04-12T21:37:23.999Z] 	git config --global --add safe.directory /workspace
script returned exit code 128

thaJeztah · 2022-04-12T23:20:27Z

I see it's using an alpine:latest image to run the check in; wondering if alpine was updated (or the git version in alpine)

samuelkarp

LGTM aside from whatever weirdness is going on with CI.

thaJeztah · 2022-04-13T07:21:25Z

Looks to be related to a vulnerability that was fixed in git; https://github.blog/2022-04-12-git-security-vulnerability-announced/

thaJeztah · 2022-04-13T08:34:38Z

Triggered CI with the DCO check skipped (to check if the rest of the pipeline also needs adjustments)

I'll have a look at fixing the DCO stage to adjust for the git security fix (looks to be because the source is checked out on the worker by Jenkins, but then accessed inside a container, in which case the user doesn't match the user on the host, triggering the security warning)

thaJeztah · 2022-04-13T14:05:57Z

opened #43485 to fix the DCO check

thaJeztah added area/builder Build status/2-code-review impact/deprecation kind/refactor PR's that refactor, or clean-up code labels Apr 10, 2022

thaJeztah added this to the 22.04.0 milestone Apr 10, 2022

thaJeztah force-pushed the deprecate_urlutil branch 3 times, most recently from 828167b to da8bf54 Compare April 11, 2022 16:08

thaJeztah mentioned this pull request Apr 11, 2022

daemon/logger: remove uses of pkg/urlutil, fix fluentd validation and parsing #43476

Merged

thaJeztah force-pushed the deprecate_urlutil branch from da8bf54 to 47b4161 Compare April 11, 2022 19:52

thaJeztah marked this pull request as ready for review April 11, 2022 19:52

thaJeztah requested a review from tonistiigi as a code owner April 11, 2022 19:52

thaJeztah commented Apr 11, 2022

View reviewed changes

thaJeztah force-pushed the deprecate_urlutil branch from 47b4161 to 183ab02 Compare April 12, 2022 08:03

thaJeztah requested a review from tianon as a code owner April 12, 2022 08:03

thaJeztah force-pushed the deprecate_urlutil branch from 183ab02 to 9675870 Compare April 12, 2022 08:14

samuelkarp reviewed Apr 12, 2022

View reviewed changes

thaJeztah force-pushed the deprecate_urlutil branch 3 times, most recently from 54888cf to 271e36e Compare April 12, 2022 14:11

thaJeztah mentioned this pull request Apr 12, 2022

cli/command/image: remove use of docker/docker/pkg/urlutil docker/cli#3555

Merged

thaJeztah force-pushed the deprecate_urlutil branch from 271e36e to 0d7dce7 Compare April 12, 2022 17:56

thaJeztah added 3 commits April 12, 2022 19:57

pkg/urlutil: remove unused IsTransportURL()

074bc1c

This function is no longer used (either internally, or externally), so can be removed. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

builder/remotecontext/urlutil: simplify and improve documentation

4492509

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 <github@gone.nl>

thaJeztah force-pushed the deprecate_urlutil branch from 0d7dce7 to 4492509 Compare April 12, 2022 17:58

tonistiigi approved these changes Apr 12, 2022

View reviewed changes

samuelkarp approved these changes Apr 13, 2022

View reviewed changes

thaJeztah mentioned this pull request Apr 13, 2022

Jenkinsfile: add workaround for CVE-2022-24765 #43485

Merged

thaJeztah mentioned this pull request Apr 13, 2022

[20.10 backport] Jenkinsfile: add workaround for CVE-2022-24765 #43486

Merged

thaJeztah merged commit 538f533 into moby:master Apr 13, 2022

thaJeztah deleted the deprecate_urlutil branch April 13, 2022 15:16

thaJeztah mentioned this pull request Jul 12, 2022

race conditions with setPermissions #43725

Open

thaJeztah mentioned this pull request Oct 6, 2022

remove aliases for deprecated pkg/urlutil, pkg/fsutils, pkg/pubsub #44261

Merged

thaJeztah mentioned this pull request Jul 10, 2025

Move builder under daemon #50365

Merged

Conversation

thaJeztah commented Apr 10, 2022

pkg/urlutil: remove unused IsTransportURL()

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

builder/remotecontext/urlutil: simplify and improve documentation

Uh oh!

thaJeztah commented Apr 11, 2022

Uh oh!

thaJeztah Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 11, 2022

Uh oh!

samuelkarp Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

samuelkarp Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 12, 2022

Uh oh!

thaJeztah commented Apr 12, 2022

Uh oh!

thaJeztah commented Apr 12, 2022

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 13, 2022

Uh oh!

thaJeztah commented Apr 13, 2022

Uh oh!

thaJeztah commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants