Adding a check for subnet pool overlap

[abhi]

A part of the ipam library allows to use same subnets for 2 different networks. However this is not honored elsewhere in the whole library and leads to inconsistent behavior. The semantic is not honored for overlapping subnets as well. The request for overlapping subnets is rejected.

This change ensures that semantics are kept consistent by rejecting requests for an already allocated pool as well. So users will not be able to create networks with already allocated subnet/overlapping subnet

[ctelfer]

Seems to cause TestIpamReleaseOnNetDriverFailures to fail. Was this intended?

Also, another very naive Q without knowing the code: this seems to check whether a particular subnet was previously allocated by looking up said subnet in a map. Does this mean it will fail to detect overlap when there is partial overlap? For example net1 = 10.1.0.0/16 and net2 = 10.1.1.0/24?

[ctelfer]

Disregard the test cases query. Looks like you already fixed it. :) 👍

[abhi]

The map only holds the keys. If there is partial overlap, key should ideally be different. In that case the next condition should verify the overlap case.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@d5818e7). Click here to learn what that means.
The diff coverage is 100%.

@@ Coverage Diff @@## master #2148 +/- ##
=========================================
Coverage ? 40.46% =========================================
Files ? 139 Lines ? 22494 Branches ? 0 =========================================
Hits ? 9103 Misses ? 12052 Partials ? 1339

Impacted Files	Coverage Δ
ipam/structures.go	85.26% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5818e7...e65fabd. Read the comment docs.

[selansen]

LGTM

[ctelfer]

Ah, thanks. Should have read the rest of the code. 👍

[euanh]

@abhi This pull request changes the semantics of RequestPool when the same pool is requested repeatedly. Previously this would return the same pool with an incremented reference count; now requesting the same pool is an error. Is there a bug or design document which says why this change is being made? If so, could you please update your commit message to refer to that?

[selansen]

My Earlier understanding was we do support IP pool overlap for overlay network but not for bridge network which is technically correct. But now discussion concluded this will lead into issues and there is no customer use case for that. I had a discussion yesterday with @mark-church on this. We are planning we will stop network creation itself when customer tries to create network with overlapping network like the way we do for bridge networks. Currently when we create network, until there is a service gets created on the network we don't allocate subnet address for the network. But if the user sends subnet range while creating network, we shall compare it with existing subnet and reject them while creating network . We shall discuss in detail in upcoming meeting.

[selansen]

I tested Abhi's fix . Even though We get debug Error message , swarm stills creates network. I will look into how Bridge networks throws error and stops network creation and see if I can fix for overlay network instead of waiting for service creation to throw error.

Driver type is missing in the network ls output. Looks like its still broken.

docker@ELANGO-CE18-2-ubuntu-0:~$ docker network ls
NETWORK ID NAME DRIVER SCOPE
gdl5nkbmzqjy nw5 overlay swarm
jckjtrucqzae nw6 swarm

May 23 14:53:13 ELANGO-CE18-2-ubuntu-0 dockerd[17147]: time="2018-05-23T14:53:13.204622267-07:00" level=debug msg="Failed allocation of unallocated network z1g3d4bricg5gtinveth1roey" error="failed allocating pools and gateway IP for network z1g3d4bricg5gtinveth1roey: Pool overlaps with other one on this address space" module=node node.id=m6c9lto0r3o0eo9uy848z9nuk
May 23 14

docker@ELANGO-CE18-2-ubuntu-0:$ docker network inspect nw5
[
{
"Name": "nw5",
"Id": "gdl5nkbmzqjypkz4kus1xgqkj",
"Created": "2018-05-23T21:46:54.802855025Z",
"Scope": "swarm",
"Driver": "overlay",
"EnableIPv6": false,
"IPAM": {
"Driver": "default",
"Options": null,
"Config": [
{
"Subnet": "10.11.100.0/24",
"Gateway": "10.11.100.1"
}
]
},
"Internal": false,
"Attachable": false,
"Ingress": false,
"ConfigFrom": {
"Network": ""
},
"ConfigOnly": false,
"Containers": null,
"Options": {
"com.docker.network.driver.overlay.vxlanid_list": "4097"
},
"Labels": null
}
]
docker@ELANGO-CE18-2-ubuntu-0:$ docker network inspect nw6
[
{
"Name": "nw6",
"Id": "jckjtrucqzae6yhth3jlmmicw",
"Created": "2018-05-23T21:53:13.19483636Z",
"Scope": "swarm",
"Driver": "",
"EnableIPv6": false,
"IPAM": {
"Driver": "default",
"Options": null,
"Config": [
{
"Subnet": "10.11.100.0/24"
}
]
},
"Internal": false,
"Attachable": false,
"Ingress": false,
"ConfigFrom": {
"Network": ""
},
"ConfigOnly": false,
"Containers": null,
"Options": null,
"Labels": null
}
]

[euanh]

@selansen Ok. I'm working on a unit test for overlapping network allocations.

[abhi]

@euanh I will not be adding a design document for a bug fix. The semantics is not honored anywhere else in the repository. It just leads to misleading conventions that ppl end up using not knowing what really happens.
It my fault to not add description in the PR. I usually do :D but we had extensive discussion on slack and other issues to keep maintainers in sync. I will update the PR with the description.

@fcrisciani I think we need to this for 18.03 bp ?

[fcrisciani]

@abhi yep would be great to have

[fcrisciani]

LGTM

[euanh]

@abhi 👍 I wasn't asking for a design doc for a bug fix, just for a reference or explanation of the changes being made. Your edited PR description makes everything clear, in case we wonder in future why this change was made. :)

[MilosQL]

This check makes no sense for macvlan networks. #2334

[ShyLionTjmn]

Please, address #2334