-
Notifications
You must be signed in to change notification settings - Fork 2k
Warn when using host-ip for published ports #1017
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
Conversation
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
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 |
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.
SGTM 🐸
|
|
||
| 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) |
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.
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.
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 I used it based on other examples I found in the code-base (e.g.
cli/cli/compose/loader/loader.go
Line 475 in cea4d37
| logrus.Warnf("network %s: network.external.name is deprecated in favor of network.name", name) |
|
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). |
|
For backwards compatibility, I think it needs to just be a warning. |
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
|
Having a warning is an improvement. Is any one looking at adding support for specifying addresses? |

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

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)