-
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
Host and Bridge network support in docker stack deploy #132
Conversation
be attached to special networks such as host and bridge. This fix brings in the required changes to make sure the stack file accepts these networks as well. Signed-off-by: Madhu Venugopal <madhu@docker.com>
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 👼
Codecov Report
@@ Coverage Diff @@
## master #132 +/- ##
=========================================
+ Coverage 46.12% 46.23% +0.1%
=========================================
Files 161 161
Lines 11006 11008 +2
=========================================
+ Hits 5077 5089 +12
+ Misses 5639 5629 -10
Partials 290 290 |
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.
generally looks good, just one question
cli/compose/convert/service.go
Outdated
netAttachConfig := swarm.NetworkAttachmentConfig{ | ||
Target: target, | ||
} | ||
if container.NetworkMode(target).IsUserDefined() { |
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.
What would happen with the old logic when using a not user defined network?
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.
Prior to 17.06, the only networks that are supported in swarm-mode (especially via stack) are user-defined networks.
Its the node-local feature in 17.06 that brings in the support for host
and bridge
.
Hence, the older versions were never impacted by this lack of support.
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.
What happens if I try to attach to the host or bridge network with a network alias?
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 we either need to warn that the aliases are being ignored, or we should add them as we would for other networks and let it fail on the server (assuming that's the behaviour).
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.
@dnephin Service Discovery is not supported in host
and bridge
networks. This is known already and is same for compose already.
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 personally dont think it is required, because, there is no Service Discovery to begin with in these networks and hence the concept of alias doesnt even arise.
The users of such host
mode understands that (as this is not unique to stack... the same thing is applicable to compose already)
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.
Yes, you would expect this behaviour because you're familiar with it :)
A user has no idea of the limitations of host
, and silently ignoring the aliases will lead to lots of confusion. Here's what I get with the latest docker-compose release:
ERROR: for examples_web_1 network-scoped alias is supported only for containers in user defined networks
We should keep the same behaviour and let the server error out for us. So I think we should revert this part of the diff.
If you agree, I can take care of it.
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.
@dnephin sorry am confused. If we remove this, then stack cannot support --network=host
? or are you proposing another way for user to disable the alias functionality ?
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 pushed another commit. It only sets the default alias if the network is user defined. That way if a user tries to use host/bridge with aliases, they will get an appropriate error, instead of silent failure.
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.
okay. now I understand your above points. SGTM
@mavenugo @vdemeester I pushed a commit which adds unit tests for We should add another test case for |
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin changes LGTM |
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 👼
@vdemeester @thaJeztah can this be merged and picked for 17.06 RC2 ? |
With the introduction of node-local network support, docker services can
be attached to special networks such as host and bridge. This fix brings
in the required changes to make sure the stack file accepts these
networks as well.
Fixes #131
Signed-off-by: Madhu Venugopal madhu@docker.com