Patch/group 5

[dirtycajunrice]

No description provided.

[nightvisi0n]

Hey @dirtycajunrice, I took a look at the network specific changes of this PR and have a few remarks. Waiting on your view about them ✌️

[nightvisi0n]

Good idea to detect user defined networks.
But I can imagine scenarios where this breaks. Technically, it's perfectly fine to have a network definition with a custom subnet assigned (and fixed IP addresses for the containers), but without any gateway defined. Docker doesn't support this yet (moby/moby#37432 moby/libnetwork#2317) with the default bridge driver, but it may already be possible with another network driver. Even thus my previous approach (try/except) isn't technically great, I think it's a safer way to determine whether the network is user defined.

[dirtycajunrice]

Docker networks are routed networks (Layer 3). While it "may be possible" for someone to design their network as a Layer 2 network and try to route via arp ip acknowledgement in the future, this is silly and i would argue that we can enforce "best practice" by not allowing for that!

[nightvisi0n]

Docker networks are routed networks (Layer 3).

As far as I know, that's not true (depending on the network driver used).
E.g. the most common one (bridge) is just using standard linux bridging, which is performed in the data link layer (L2). Routing between different docker bridge networks is only an additional feature, which is not handled by the bridge itself, but by a different part of the kernel. Thus, technically there doesn't have to be a default gateway on (docker) bridge networks in order to connect multiple containers within the same network segment. Currently it's not allowed to have no default gateway when creating a bridged docker network, but that might change in the future (see the PRs I've linked in my first comment.)
Another example is the macvlan driver: https://docs.docker.com/network/macvlan/#use-an-ipvlan-instead-of-macvlan

try to route via arp ip acknowledgement in the future, this is silly and i would argue that we can enforce "best practice" by not allowing for that!

I don't really get what you are meaning by "arp ip acknowledgement". If you mean standard ARP, I can't agree that it is "silly". Most of the "internet" won't work without this very basic L1 protocol.
If you mean we should generally not allow networks without a default gateway defined I also can't agree with that. Many (most?) docker networks in simple (single docker host) scenarios are created only to seperate networking traffic between different containers (e.g. to create a "private" network between an application container and a database container). There is no reason to have a default gateway in this scenario, as it is not expected that traffic from this network leaves the segment.

---

Leaving aside these very technical aspects your implementation unfortunately does not solve the initial problem - differentiating between docker networks with user defined subnets (docker network create --subnet 10.13.37.0/24 --gateway 10.13.37.1 test) from docker networks without user defined subnets (docker network create test). Just to recap, this is necessary in order to decide whether to carry over ip addresses, because that's only possible with user defined subnets (APIError: 400 Client Error: Bad Request ("user specified IP address is supported only when connecting to networks with user configured subnets")) Now, the problem with your approach (decision based on the existence of a gateway in the network config) is, that both networks do have gateways defined in their config:

docker network create test-default
docker network create --subnet 10.13.37.0/24 --gateway 10.13.37.1 test-user-defined-subnet
docker run -d --name=test --network=test-default fedora:28 sleep 999999
docker network connect test-user-defined-subnet test

import docker
client = docker.from_env()
container = client.containers.get('test')
print(container.attrs['NetworkSettings']['Networks'])

{'test-default': {'IPAMConfig': None,
[..]
  'Gateway': '172.21.0.1',
  'IPAddress': '172.21.0.2',
[..]
},
 'test-user-defined-subnet': {'IPAMConfig': {},
[..]
  'Gateway': '10.13.37.1',
  'IPAddress': '10.13.37.2',
[..]
}}

Now, trying to attach a new container to the test-default network while specifying an ip address does not work (just to double check):

docker run -d --name=test2 fedora:28 sleep 999999

network = client.networks.get('test-default')
network.connect(container=container, ipv4_address='172.21.0.42')

APIError: 400 Client Error: Bad Request ("user specified IP address is supported only when connecting to networks with user configured subnets")

The only reason why this exception is not fired in your implementation is, because you currently assign the ip addresses to the config of the old container (network_config vs. new_network_config). I realized this only now during writing this comment, as I was really wondering why this exception is not fired with this implementation.

My suggestion is still to just try to set the address and catch the exception if it fails (following the EAFP python coding style. Therefore we can be confident to catch every use case regarding docker networking and ip addressing.

[dirtycajunrice]

HA! i totally blew by that assignment mistake. And What you are saying is fair. Ill switch it over

[dirtycajunrice]

See relevant section again after pushes

[nightvisi0n]

Ok, after testing it seems that actually there are two different exceptions we have to account for:

APIError: 400 Client Error: Bad Request ("user specified IP address is supported only when connecting to networks with user configured subnets")

When connecting to a user defined network that has no subnet/gateway defined.

Error: 400 Client Error: Bad Request ("user specified IP address is supported on user defined networks only")

When connecting to the default bridge network.

Either we check for both messages or we look for the com.docker.network.bridge.default_bridge label (which the default bridge network has assigned)

---

Another thing I noticed during testing: My error message if network.connect fails is using the default string representation of the network object, which leads to cryptic error messages like this: Unable to attach updated container to network "<Network: 45a33638f9>".
We should print network.name instead 🙂

[dirtycajunrice]

i like explicit errors over the label, I added both line checks + network.name. can you test again please

[nightvisi0n]

LGTM 👍