Display a warn message when there is binding ports and net mode is host - #35510
Conversation
thaJeztah
left a comment
There was a problem hiding this comment.
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
4d09a6ebc335a6c89a4c4f812d234d41c4cf6aa9040d8867bd7d852d4f01792bThere was a problem hiding this comment.
LOL, had to laugh a bit about your choice of wording 😄
I'll try to come up with an alternative 👍
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
ping @johnstep @jhowardmsft could you have a look? ^^
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I guess this could be a unit-test instead of an integration test?
|
ping @thaJeztah @jhowardmsft |
|
Seems OK, but I'd like @msabansal from the networking team here to confirm. |
|
LGTM. |
There was a problem hiding this comment.
nit: could use Godoc conventions: s/This tests/TestContainerWarningHostAndPublishPorts tests/
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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>
- What I did
Fixes #35500
When a container is created if
--networkis set tohostall the ports in the container are bound to the host. Thus, when adding-por--publishto the command line is meaningless.Unlike
docker runanddocker create,docker service createreturns 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)