Do not allow service creation on ingress network#1600
Do not allow service creation on ingress network#1600LK4D4 merged 1 commit intomoby:masterfrom aboch:bi
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
|
manager/controlapi/service.go
Outdated
| 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.
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.
Changed the error, retested (updated outcome in the description). Thanks.
Signed-off-by: Alessandro Boch <aboch@docker.com>
|
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.
@aboch does this function need to return errors?
There was a problem hiding this comment.
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.
Thanks for clarifying.
|
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 <madhu@docker.com>
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 <aaron.lehmann@docker.com>
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 <aaron.lehmann@docker.com>
9 participants
Related to moby/moby#27147