Skip IPAddr validation for "host-gateway" string by arkodg · Pull Request #2293 · docker/cli

arkodg · 2020-01-27T19:59:05Z

Relates to - moby/moby#40007
The above PR added support in moby, that detects if
a special string "host-gateway" is added to the IP
section of --add-host, and if true, replaces it with
a special IP value (value of --host-gateway-ip Daemon flag
which defaults to the IP of the default bridge).

This PR is needed to skip the validation for the above
feature

Signed-off-by: Arko Dasgupta [email protected]

silvin-lubecki · 2020-01-28T15:05:40Z

Related to #2290

thaJeztah · 2020-01-30T15:34:19Z

 	DefaultNamedPipe = `//./pipe/docker_engine`
+	// HostGatewayName defines a special string which users can append to --add-host
+	// to add an extra entry in /etc/hosts that maps host.docker.internal to the host IP
+	HostGatewayName = "host-gateway"


This should be a const; also wondering if there's a const for this in moby/moby that we can use

thaJeztah · 2020-01-30T15:35:14Z

-		return "", fmt.Errorf("invalid IP address in add-host: %q", arr[1])
+	// Skip IPaddr validation for "host-gateway" string
+	if arr[1] != HostGatewayName {
+		if _, err := ValidateIPAddress(arr[1]); err != nil {


Actually slightly on the fence if we should validate at all in the cli, or if we should just pass it to the daemon and have the daemon validate / produce an error (haven't checked yet what the UX looks like in that case)

silvin-lubecki · 2020-01-30T15:35:38Z

 	DefaultNamedPipe = `//./pipe/docker_engine`
+	// HostGatewayName defines a special string which users can append to --add-host
+	// to add an extra entry in /etc/hosts that maps host.docker.internal to the host IP
+	HostGatewayName = "host-gateway"


I think this value should be used directly from moby

thaJeztah · 2020-01-30T15:44:25Z

Tested what it looks like if we remove the IP address validation;

docker create --add-host foo:fooobar nginx:alpine
Error response from daemon: invalid IP address in add-host: "fooobar"

docker run --add-host foo:fooobar nginx:alpine
docker: Error response from daemon: invalid IP address in add-host: "fooobar".

And, I think those errors look reasonable (have not tried docker stack deploy though)

thaJeztah · 2020-01-30T15:49:05Z

docker service create also validates on the service, but appears to be retrying creation; perhaps it's only validating the moment the container is created and not when the service is created;

docker service create --host foo:foooobar nginx:alpine
opbff21havwdz5fyrjk68so11
overall progress: 0 out of 1 tasks
1/1: invalid IP address in add-host: "foooobar"

...

overall progress: 0 out of 1 tasks
1/1: assigned  [======================>                            ]

...

overall progress: 0 out of 1 tasks
1/1: invalid IP address in add-host: "foooobar"

arkodg · 2020-01-30T21:09:49Z

sure, there is already an identical validation in the daemon, and this PR is just here to unblock this feature.
we can completely remove ValidateHost from the cli, but I'm not sure what the design philosophy is for cli and daemon arg validation

thaJeztah · 2020-01-30T23:36:17Z

Overall, I prefer delegating validation to the daemon as much as possible; at most validating well-formedness of values (which in this case was possible before).

Keeping the daemon as source of truth not only keeps complexity out of the cli code, it also prevents situations where the cli talks to a different (older/newer) version of the daemon, or (eg) a daemon running on a different platform.

We should probably (as a follow up) look at the docker service create example though, because that UX isn't ideal (if possible, validate earlier, or perhaps give up if the config is invalid instead of keep retrying)

arkodg · 2020-01-31T03:51:15Z

@thaJeztah IMHO for the docker service create case, I think keeping the logic here makes sense, updated the PR removing the const

silvin-lubecki

LGTM

silvin-lubecki · 2020-01-31T10:29:37Z

@arkodg The vendoring looks weird:

These files were changed:

?? vendor/github.com/opencontainers/runc/libcontainer/cgroups/
?? vendor/github.com/opencontainers/runc/libcontainer/configs/

silvin-lubecki · 2020-01-31T10:29:52Z

Please also rebase 🦁

thaJeztah · 2020-02-11T19:06:01Z

@arkodg could you;

move the vendor update to a separate commit (first commit)
- only including changes needed related to that update (I see some tests failing, which could be related to changes upstream)
move local changes to for this feature to a commit coming after that (combine the local changes in the current commits)

Bumping to the lastest refpoint to include the HostGatewayName constant from moby Signed-off-by: Arko Dasgupta <[email protected]>

arkodg · 2020-02-11T22:45:23Z

@thaJeztah the issue is with make cross using the latest docker/docker refpoint

thaJeztah · 2020-02-12T17:54:40Z

opened #2333 to update vendoring

thaJeztah · 2020-02-13T14:27:02Z

@arkodg I think this was a bad rebase, as I see no local code-changes anymore, other than adding the import 🤔

Noticed this when I rebased the PR (also the commit message was now only about vndr); see https://github.com/docker/cli/compare/master...thaJeztah:carry_2293_add_host_gateway_dns_entry?expand=1

thaJeztah · 2020-02-13T14:31:25Z

@silvin-lubecki found an old commit here; 709cf07

Updating my branch with that

arkodg · 2020-02-13T16:41:14Z

@thaJeztah I removed the old commit from the PR after you suggested to rc the CI issue by separating the commits to a vendor commit and the feature commit :)

yuraku · 2020-02-22T23:39:22Z

Hi guys. Is there any ETA on when this PR will be merged? 🤞🏻

GordonTheTurtle added the status/0-triage label Jan 27, 2020

arkodg requested a review from thaJeztah January 27, 2020 19:59

thaJeztah requested changes Jan 30, 2020

View reviewed changes

silvin-lubecki reviewed Jan 30, 2020

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Jan 30, 2020

thaJeztah added this to the next milestone Jan 30, 2020

arkodg requested review from silvin-lubecki and thaJeztah January 31, 2020 03:51

silvin-lubecki approved these changes Jan 31, 2020

View reviewed changes

arkodg force-pushed the add-host-gateway-dns-entry branch from 709cf07 to 296426c Compare January 31, 2020 18:17

thaJeztah mentioned this pull request Feb 11, 2020

Support host.docker.internal in dockerd on Linux moby/moby#40007

Merged

Bump docker/docker vndr refpoint

85390b4

Bumping to the lastest refpoint to include the HostGatewayName constant from moby Signed-off-by: Arko Dasgupta <[email protected]>

arkodg force-pushed the add-host-gateway-dns-entry branch from 296426c to 85390b4 Compare February 11, 2020 20:01

thaJeztah mentioned this pull request Feb 13, 2020

Skip IPAddr validation for "host-gateway" string #2337

Merged

silvin-lubecki closed this in #2337 Feb 27, 2020

Uh oh!

Conversation

arkodg commented Jan 27, 2020

Uh oh!

silvin-lubecki commented Jan 28, 2020

Uh oh!

thaJeztah Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 30, 2020

Uh oh!

thaJeztah commented Jan 30, 2020

Uh oh!

arkodg commented Jan 30, 2020

Uh oh!

thaJeztah commented Jan 30, 2020

Uh oh!

arkodg commented Jan 31, 2020

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki commented Jan 31, 2020

Uh oh!

silvin-lubecki commented Jan 31, 2020

Uh oh!

thaJeztah commented Feb 11, 2020

Uh oh!

arkodg commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Feb 12, 2020

Uh oh!

thaJeztah commented Feb 13, 2020

Uh oh!

thaJeztah commented Feb 13, 2020

Uh oh!

arkodg commented Feb 13, 2020

Uh oh!

yuraku commented Feb 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arkodg commented Feb 11, 2020 •

edited

Loading