daemon: automatically set network EnableIPv6 if needed#46455
daemon: automatically set network EnableIPv6 if needed#46455thaJeztah merged 1 commit intomoby:masterfrom
Conversation
|
oh! can you remove the for the |
daemon/network.go
Outdated
| if network.HasIPv6Subnets(create.IPAM) && !create.EnableIPv6 { | ||
| create.EnableIPv6 = true | ||
| } |
There was a problem hiding this comment.
Overall this looks fine; starting to wonder now if we should have something that returns the IPAM config;
ValidateIPAMalso loops over all configs (including checking for IPv6)- But the only thing returned is an error (i.e., we throw away some of the work)
We could have some function that handles the IPAM config, returns it (which may include setting some additional "runtime" data perhaps)
ipamConfig, err := foo.ParseIPAMConfig()Or perhaps this should be a libnetwork.NetworkOptionIPAM(create.IPAM). We should update NetworkOption to return an error (maybe I already had something like that in one of my PRs), so that it can return an error if the option was invalid.
Eitherway; that's definitely fine to be looked at separately 😅
There was a problem hiding this comment.
Or perhaps this should be a libnetwork.NetworkOptionIPAM(create.IPAM).
Nope, that's something I'd really like to prevent in the future. With a tighter integration between moby and libnetwork, we should instead put data validation / normalization closer to the API such that we can expect data to be valid early in the call stack. That should make it easier to build a mental model of what's going on. With that boundary, if we need to check the soundness of some data down the stack, it should only be for defensive reasons.
starting to wonder now if we should have something that returns the IPAM config;
IMO it's always harder to reason about some code that validate and mutate data at the same time. I don't expect Unless we've good reasons to tie both steps together, I'd prefer to stick with this implementation.
fd2f68a to
5cc9050
Compare
|
oh! there's a build-failure somewhere; |
5cc9050 to
7be4ad1
Compare
PR 4f47013 added a validation step to `NetworkCreate` to ensure no IPv6 subnet could be set on a network if its `EnableIPv6` parameter is false. Before that, the daemon was accepting such request but was doing nothing with the IPv6 subnet. This validation step is now deleted, and we automatically set `EnableIPv6` if an IPv6 subnet was specified. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
7be4ad1 to
5d5eeac
Compare
|
Hi! So basically this PR ignores a set enable_ipv6: false value inside a docker-compose.yml? Why does this option still exists then if it is ignored even if it is false? Previously we could had the ipv6 subnet added to the docker-compose.yml and let our users decide if they want to and don't want to use ipv6, so they disabled it accordingly to our docs. Now they have to set the enable_ipv6 parameter to false and remove the ipv6 subnet from the network config? With the new update to 25.0.0 our users with deactivated ipv6 now run into problems and they would have to modify their setup again. Maybe this is a bug, that it is ignored at all? Referencing: mailcow/mailcow-dockerized#5668 |
|
@DerLinkman could you open a ticket with details? It helps tracking, as comments on PRs can easily get overlooked / lost. |
- What I did
PR #45759 added a validation step to
NetworkCreateto ensure no IPv6 subnet could be set on a network if itsEnableIPv6parameter is false.Before that, the daemon was accepting such request but was doing nothing with the IPv6 subnet.
This validation step is now deleted, and we automatically set
EnableIPv6if an IPv6 subnet was specified.