close
Skip to content

Warn when using host-ip for published ports - #1017

Merged
cpuguy83 merged 1 commit into
docker:masterfrom
thaJeztah:warn-when-using-host-ip
Apr 25, 2018
Merged

Warn when using host-ip for published ports#1017
cpuguy83 merged 1 commit into
docker:masterfrom
thaJeztah:warn-when-using-host-ip

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

Swarm Mode services don't support binding to a specific IP-address. Print a warning that the IP-address will be ignored if one is specified.

relates to #1016

- Description for the changelog

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

Warn when using host-ip for published ports for services

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

Copy link
Copy Markdown
Member Author

I was working on this a while back, and recalled I had this branch; should check if the validation/warning is printed in the right part of the code 😅 (may need a unit test as well), but thought; let me push this

@vdemeester vdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM 🐸

Comment thread opts/port.go

for _, binding := range portBindings[port] {
if binding.HostIP != "" && binding.HostIP != "0.0.0.0" {
logrus.Warnf("ignoring IP-address (%s:%s:%s) service will listen on '0.0.0.0'", binding.HostIP, binding.HostPort, port)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be logrus? I think usually we just print directly to stderr? But also it's weird to write to stdio in this package.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I used it based on other examples I found in the code-base (e.g.

logrus.Warnf("network %s: network.external.name is deprecated in favor of network.name", name)
), but open to suggestions 👍

@richard-delorenzi

Copy link
Copy Markdown

Is a warning enough? e.g. “Warning: your data has just been exposed” secure by default, says that it should be an error (no deploy).

@cpuguy83

Copy link
Copy Markdown
Collaborator

For backwards compatibility, I think it needs to just be a warning.
The default is not to expose ports, so you are already exposing things. It's also strictly a CLI bug, so it's not like some API client can hit moby and not realize they've made a mistake.

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83
cpuguy83 merged commit 99bd7ed into docker:master Apr 25, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.05.0 milestone Apr 25, 2018
@thaJeztah
thaJeztah deleted the warn-when-using-host-ip branch April 25, 2018 22:11
@richard-delorenzi

Copy link
Copy Markdown

Having a warning is an improvement. Is any one looking at adding support for specifying addresses?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants