-
Notifications
You must be signed in to change notification settings - Fork 128
Support multiple static IPs per subnet #1379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
4805a3a to
f806e8d
Compare
| assert "${#lines[@]}" == 2 "only two interfaces (lo, eth0) in the netns, the tmp ipvlan interface should be gone" | ||
| } | ||
|
|
||
| @test "ipvlan multiple static ips single subnet" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will macvlan, too, support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should. Should I add tests for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a macvlan test cases.
| **Note on `static_ips`**: When multiple subnets are configured in the network, you can specify multiple static IP addresses. Each IP will be assigned to the subnet it belongs to. For example, with subnets `10.88.0.0/16` and `10.89.0.0/16`, you could specify: | ||
|
|
||
| ``` | ||
| "static_ips": ["10.88.0.50", "10.88.0.51", "10.89.0.100"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need ability to specify static IP address in CIDR format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that you need multiple static IPs that are not in the same subnet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple static IP addresses that part of multiple smaller subnets of a large configured subnet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what the exact intention is here? I can't really understand the benefits of setting a subnet different from that of the actual network the interface is connected to, even if that subnet is part of the larger subnet the network uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree different subnets on a static ip address allocation is not something I like to support.
The way to do that would be multiple --subnet options on network create.
Change from index-based IP/subnet pairing to subnet-aware matching. Now all static IPs that belong to a subnet are assigned to that subnet, allowing multiple IPs per subnet and proper multi-subnet distribution. Fixes: https://issues.redhat.com/browse/RHEL-98277 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
f806e8d to
3a18af2
Compare
| Ok(Some(val)) | ||
| } | ||
|
|
||
| fn validate_subnets(network: &types::Network) -> NetavarkResult<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write some rust unit tests for this please. That allows us to cover some more subnet combinations and is faster than dealing with the more expensive integration tests
|
|
||
|
|
||
|
|
||
| @test "macvlan multiple static ips single subnet" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a ton of duplication between the testfiles here, I would really prefer if we can start deduplicating some of this.
i.e. compare test_port_fw function to generate a config dynamically for tests.
Right now it duplicates all test files for each driver which is not great.
| // check if the static IP belongs to this subnet | ||
| if subnet.subnet.contains(static_ip) { | ||
| let container_address: ipnet::IpNet = | ||
| match format!("{}/{}", static_ip, subnet_mask_cidr).parse() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre existing but it makes no sense to combine this into a string to parse it again, we can create the subnet directly ipnet::IpNet::new(ip, prefix_len)
Change from index-based IP/subnet pairing to subnet-aware matching. Now all static IPs that belong to a subnet are assigned to that subnet, allowing multiple IPs per subnet and proper multi-subnet distribution.
Fixes: https://issues.redhat.com/browse/RHEL-98277