-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip IPAddr validation for "host-gateway" string #2293
Conversation
Related to #2290 |
opts/hosts.go
Outdated
@@ -24,6 +24,9 @@ var ( | |||
DefaultTLSHost = fmt.Sprintf("tcp://%s:%d", DefaultHTTPHost, DefaultTLSHTTPPort) | |||
// DefaultNamedPipe defines the default named pipe used by docker on Windows | |||
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a const
; also wondering if there's a const for this in moby/moby that we can use
opts/hosts.go
Outdated
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
opts/hosts.go
Outdated
@@ -24,6 +24,9 @@ var ( | |||
DefaultTLSHost = fmt.Sprintf("tcp://%s:%d", DefaultHTTPHost, DefaultTLSHTTPPort) | |||
// DefaultNamedPipe defines the default named pipe used by docker on Windows | |||
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this value should be used directly from moby
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 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" |
sure, there is already an identical validation in the daemon, and this PR is just here to unblock this feature. |
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 |
@thaJeztah IMHO for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@arkodg The vendoring looks weird:
|
Please also rebase 🦁 |
709cf07
to
296426c
Compare
@arkodg could you;
|
Bumping to the lastest refpoint to include the HostGatewayName constant from moby Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
296426c
to
85390b4
Compare
@thaJeztah the issue is with |
opened #2333 to update vendoring |
@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 |
@silvin-lubecki found an old commit here; 709cf07 Updating my branch with that |
@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 :) |
Hi guys. Is there any ETA on when this PR will be merged? 🤞🏻 |
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 arko.dasgupta@docker.com