Display a warn message when there is binding ports and net mode is host#35510

boaz0 · 2017-11-15T15:09:37Z

- What I did
Fixes #35500

When a container is created if --network is set to host all the ports in the container are bound to the host. Thus, when adding -p or --publish to the command line is meaningless.

Unlike docker run and docker create, docker service create returns an error message when network mode is host and port bindings are given.

- How I did it

This patch however suggests to send a warning message to the client when
such a case occurs.

By adding it to the warnings returned from verifyPlatformContainerSettings

- How to verify it

Added integration test to that so just run make test-integration.

- Description for the changelog

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

thaJeztah

Left some comments, but functionally looks to be working :)

$ docker run -dit --name foo --network=host -p 80:80 busybox
WARNING: Setting published IPs is useless when network mode is set to host
4d09a6ebc335a6c89a4c4f812d234d41c4cf6aa9040d8867bd7d852d4f01792b

thaJeztah · 2017-11-15T15:40:18Z

daemon/container.go

-	return verifyPlatformContainerSettings(daemon, hostConfig, config, update)
+	platformWarns, err := verifyPlatformContainerSettings(daemon, hostConfig, config, update)
+	if hostConfig.NetworkMode.IsHost() && len(hostConfig.PortBindings) > 0 {
+		platformWarns = append(platformWarns, "Setting published IPs is useless when network mode is set to host")


LOL, had to laugh a bit about your choice of wording 😄

I'll try to come up with an alternative 👍

thaJeztah · 2017-11-15T15:44:32Z

daemon/container.go

@@ -330,5 +330,9 @@ func (daemon *Daemon) verifyContainerSettings(platform string, hostConfig *conta
 	}

 	// Now do platform-specific verification
-	return verifyPlatformContainerSettings(daemon, hostConfig, config, update)
+	platformWarns, err := verifyPlatformContainerSettings(daemon, hostConfig, config, update)


Can you check the error here, and return? It "looks" safe in this case, but it's best practice to handle errors;

platformWarns, err := verifyPlatformContainerSettings(daemon, hostConfig, config, update) if err != nil { return nil, err }

Now that platformWarns no longer contains "just" platform-specific warnings, we should probably rename it to something more generic (just warnings?)

thaJeztah · 2017-11-15T15:46:14Z

daemon/container.go

@@ -330,5 +330,9 @@ func (daemon *Daemon) verifyContainerSettings(platform string, hostConfig *conta
 	}

 	// Now do platform-specific verification
-	return verifyPlatformContainerSettings(daemon, hostConfig, config, update)
+	platformWarns, err := verifyPlatformContainerSettings(daemon, hostConfig, config, update)
+	if hostConfig.NetworkMode.IsHost() && len(hostConfig.PortBindings) > 0 {


Related to the above; I don't think host networking is supported on Windows (although with LCOW .. perhaps it's still relevant here); I'll have to check that one. If not supported on Windows we should probably move it in verifyPlatformContainerSettings()

ping @johnstep @jhowardmsft could you have a look? ^^

@msabansal Probably one for you to answer. PTAL.

vdemeester

LGTM 🐸

thaJeztah · 2017-11-27T23:33:33Z

daemon/container.go

+		return warnings, err
+	}
+	if hostConfig.NetworkMode.IsHost() && len(hostConfig.PortBindings) > 0 {
+		warnings = append(warnings, "Setting published ports is meaningless when network mode is set to host")


Thanks! Saw you slightly changed the message; thinking a bit more about wording; would this work perhaps;

Published ports are discarded when using host network mode

Sure, no problem.

thaJeztah · 2018-01-16T12:00:21Z

integration/container/create_test.go

+
+// This tests that a warning is returned when setting network mode to host
+// and specifying published ports.
+func TestCreateWarningHostAndPublish(t *testing.T) {


I guess this could be a unit-test instead of an integration test?

boaz0 · 2018-02-15T22:03:22Z

ping @thaJeztah @jhowardmsft

lowenna · 2018-02-15T22:06:28Z

Seems OK, but I'd like @msabansal from the networking team here to confirm.

msabansal · 2018-02-15T22:26:47Z

LGTM.

thaJeztah

LGTM

thaJeztah · 2018-02-16T10:53:12Z

daemon/container_test.go

+	"github.com/stretchr/testify/require"
+)
+
+// This tests that a warning is returned when setting network mode to host and specifying published ports.


nit: could use Godoc conventions: s/This tests/TestContainerWarningHostAndPublishPorts tests/

vdemeester · 2018-02-16T13:50:10Z

daemon/container_test.go:1::warning: file is not gofmted with -s (gofmt)

codecov · 2018-02-18T11:42:39Z

Codecov Report

Merging #35510 into master will increase coverage by 0.04%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master   #35510      +/-   ##
==========================================
+ Coverage   34.27%   34.32%   +0.04%     
==========================================
  Files         609      611       +2     
  Lines       45287    45489     +202     
==========================================
+ Hits        15523    15612      +89     
- Misses      27816    27870      +54     
- Partials     1948     2007      +59

Impacted Files	Coverage Δ
daemon/container.go	`25.27% <71.42%> (+5.73%)`	⬆️
pkg/discovery/file/file.go	`90.38% <0%> (-5.77%)`	⬇️
restartmanager/restartmanager.go	`33.33% <0%> (-3.18%)`	⬇️
daemon/cluster/executor/container/adapter.go	`9.16% <0%> (-0.84%)`	⬇️
builder/fscache/fscache.go	`58.84% <0%> (-0.58%)`	⬇️
daemon/graphdriver/devmapper/deviceset.go	`39.85% <0%> (-0.21%)`	⬇️
vendor/github.com/docker/distribution/errors.go	`21.42% <0%> (ø)`
vendor/github.com/docker/distribution/registry.go	`52.08% <0%> (ø)`
daemon/daemon.go	`4.5% <0%> (+0.15%)`	⬆️
pkg/archive/archive.go	`68.05% <0%> (+0.63%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5e7537...6e78fdb. Read the comment docs.

When a container is created if "--network" is set to "host" all the ports in the container are bound to the host. Thus, adding "-p" or "--publish" to the command-line is meaningless. Unlike "docker run" and "docker create", "docker service create" sends an error message when network mode is host and port bindings are given This patch however suggests to send a warning message to the client when such a case occurs. The warning message is added to "warnings" which are returned from "verifyPlatformContainerSettings". Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

boaz0 · 2018-02-19T08:08:15Z

thanks @vdemeester @jhowardmsft @msabansal @thaJeztah

boaz0 requested review from dnephin and vdemeester as code owners November 15, 2017 15:09

GordonTheTurtle added the status/0-triage label Nov 15, 2017

thaJeztah requested changes Nov 15, 2017

View reviewed changes

thaJeztah added status/1-design-review and removed status/0-triage labels Nov 15, 2017

vdemeester approved these changes Nov 27, 2017

View reviewed changes

thaJeztah reviewed Nov 27, 2017

View reviewed changes

GordonTheTurtle assigned stevvooe Nov 30, 2017

thaJeztah reviewed Jan 16, 2018

View reviewed changes

vdemeester requested review from thaJeztah and johnstep February 16, 2018 10:48

thaJeztah approved these changes Feb 16, 2018

View reviewed changes

thaJeztah added the rebuild/* label Feb 16, 2018

GordonTheTurtle removed the rebuild/* label Feb 16, 2018

thaJeztah added status/4-merge impact/changelog and removed status/1-design-review labels Feb 16, 2018

vdemeester merged commit 35d69f1 into moby:master Feb 19, 2018

boaz0 deleted the fix_35500 branch February 19, 2018 08:08

thaJeztah mentioned this pull request Dec 11, 2019

Improve error-handling when combining "-p" / "--publish" with "host" network for services #40306

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display a warn message when there is binding ports and net mode is host#35510

Display a warn message when there is binding ports and net mode is host #35510

boaz0 commented Nov 15, 2017

thaJeztah left a comment

thaJeztah Nov 15, 2017

thaJeztah Nov 15, 2017

thaJeztah Nov 15, 2017

thaJeztah Jan 16, 2018

lowenna Jan 19, 2018

vdemeester left a comment

thaJeztah Nov 27, 2017

boaz0 Nov 27, 2017

thaJeztah Jan 16, 2018

boaz0 commented Feb 15, 2018

lowenna commented Feb 15, 2018

msabansal commented Feb 15, 2018

thaJeztah left a comment

thaJeztah Feb 16, 2018

vdemeester commented Feb 16, 2018

codecov bot commented Feb 18, 2018 •
edited

Loading

boaz0 commented Feb 19, 2018

Display a warn message when there is binding ports and net mode is host#35510

Display a warn message when there is binding ports and net mode is host #35510

Conversation

boaz0 commented Nov 15, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 commented Feb 15, 2018

lowenna commented Feb 15, 2018

msabansal commented Feb 15, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Feb 16, 2018

codecov bot commented Feb 18, 2018 • edited Loading

Codecov Report

boaz0 commented Feb 19, 2018

codecov bot commented Feb 18, 2018 •
edited

Loading