Support multiple static IPs and validate subnets#557
Conversation
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6603 |
|
Packit jobs failed. @containers/packit-build please check. |
|
/packit rebuild-failed |
Honny1
force-pushed
the
multi-static-ip-subnet
branch
from
1626621 to
4b6cbdb
Compare
Honny1
force-pushed
the
multi-static-ip-subnet
branch
from
4b6cbdb to
8e205ad
Compare
Honny1
force-pushed
the
multi-static-ip-subnet
branch
from
8e205ad to
7524bc5
Compare
|
|
||
| network := types.Network{Driver: "bridge", Subnets: ns} | ||
| _, err := libpodNet.NetworkCreate(network, nil) | ||
| if wantErr { |
There was a problem hiding this comment.
just practically speaking we do not need that bool just match on errContains != "" and drop the bool which makes the test case listing a bit simpler and avoids the issue where someone has set wantErr true but not provided an error message to match on
|
also cc @ashley-cui as you move the creation validation logic into netavark so this new check needs to be there as well but @Honny1 already added the same validate subnet function there containers/netavark@a85e493 so it can just be reused |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
I know nothing about the problem space. LGTM just looking at the implementation without understanding the surrounding code, one question:
| // IPv6Enabled to true if at least one subnet is ipv6. | ||
| func ValidateSubnets(network *types.Network, addGateway bool, usedNetworks []*net.IPNet) error { | ||
| for i := range network.Subnets { | ||
| for j := i + 1; j < len(network.Subnets); j++ { |
There was a problem hiding this comment.
https://issues.redhat.com/browse/RHEL-98277?focusedId=27450128&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-27450128 suggests that duplicate subnets were previously documented as working, although perhaps only partially.
Do we need to preserve that?!
There was a problem hiding this comment.
No, this just makes no sense to allow with this change anymore and with 6.0 with have enough room to just outright refuse it.
my testing it doesn't work at all when specifying a static ip via --network mv1:ip=... as the IPAM allocation logic is not designed to handle that.
I.e. it never really worked it was just something that happend to work. We never documented that and this comment was just me testing some things and should not be seen as encouragement of that possibility.
Fixes allocation logic to support containers with multiple static IPs from different subnets, while preventing invalid subnet configurations. Fixes: https://issues.redhat.com/browse/RHEL-98277 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Honny1
force-pushed
the
multi-static-ip-subnet
branch
from
7524bc5 to
ac26a49
Compare
4 participants
Fixes allocation logic to support containers with multiple static IPs from different subnets, while preventing invalid subnet configurations.
Fixes: https://issues.redhat.com/browse/RHEL-98277
Related: containers/netavark#1379