-
Notifications
You must be signed in to change notification settings - Fork 616
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
Do not allow service creation on ingress network #1600
Conversation
Current coverage is 53.80% (diff: 39.13%)@@ master #1600 diff @@
==========================================
Files 84 84
Lines 13929 13957 +28
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 7507 7510 +3
- Misses 5417 5432 +15
- Partials 1005 1015 +10
|
continue | ||
} | ||
if _, ok := network.Spec.Annotations.Labels["com.docker.swarm.internal"]; ok { | ||
return grpc.Errorf(codes.InvalidArgument, "Service cannot be explicitly connected to the ingress 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.
The error message probably should extract the name from the network since may be in the future we may have more than one internal network. Also It should probably be rephrased to something like Service cannot be explicitly attached to "ingress" network which is an internal 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.
Changed the error, retested (updated outcome in the description). Thanks.
Signed-off-by: Alessandro Boch <[email protected]>
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
LGTM |
LGTM, thanks for working on it @aboch |
Just a heads up, it also closes this issue: moby/moby#25396 |
LGTM |
// FindNetwork is a utility function which returns the first | ||
// network for which the target string matches the ID, or | ||
// the name or the ID prefix. | ||
func FindNetwork(tx ReadTx, target string) *api.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.
@aboch does this function need to return errors?
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.
No, it is a best effort function on the same line of the existing Getnetwork()
.
If one query fails (ID, name or id pefix), it moves to the next one.
Eventually, the outcome will be the same as we would have if we reported an error happening during one query: We would eventually not be able to identify an network.
From the caller perspective, it will be a failure regardless.
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.
Thanks for clarifying.
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
LGTM |
@LK4D4 can you please create a PR against docker/1.12.x with this change ? |
cherry-picking moby/swarmkit#1600 and moby/libnetwork#1489 Signed-off-by: Madhu Venugopal <[email protected]>
PR moby#1600 added a FindNetwork function to the store package to resolve a NetworkAttachment's target by ID, name, or name prefix. I think this is based on a misleading comment in the NetworkAttachment definition saying that the target can be a name or an ID. In fact, it's always an ID, and existing code passes it directly to GetNetwork. Clarify the comment and remove FindNetwork. Signed-off-by: Aaron Lehmann <[email protected]>
PR moby#1600 added a FindNetwork function to the store package to resolve a NetworkAttachment's target by ID, name, or name prefix. I think this is based on a misleading comment in the NetworkAttachment definition saying that the target can be a name or an ID. In fact, it's always an ID, and existing code passes it directly to GetNetwork. Clarify the comment and remove FindNetwork. Signed-off-by: Aaron Lehmann <[email protected]>
Related to moby/moby#27147