fix(network): Relax Constraints for Resizing Dynamic Range Reservations LP: #2143090#280
Conversation
|
Check where you would like a Mattermost message to be sent to when CI completes and this PR is merged
|
735e40e to
fc872ef
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes validation for resizing existing dynamic IP ranges when there are already allocated IPs inside the current range, which previously caused the “Requested dynamic range conflicts…” error by treating the range as discontinuous.
Changes:
- Updates dynamic range overlap validation to permit resizing when only the newly-added segments are within unused space.
- Adds regression tests ensuring an existing dynamic range can shrink/expand even with an allocated IP inside the original range.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/maasserver/models/iprange.py |
Adjusts overlap/duplicate validation to allow certain dynamic-range resizes with in-range allocations. |
src/maasserver/models/tests/test_iprange.py |
Adds tests covering shrink/expand of an existing dynamic range when an IP inside it is allocated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_modify_existing_dynamic_range_can_shrink_with_allocated_ip(self): | ||
| subnet = make_plain_subnet() | ||
| iprange = IPRange( | ||
| subnet=subnet, | ||
| type=IPRANGE_TYPE.DYNAMIC, | ||
| start_ip="192.168.0.10", | ||
| end_ip="192.168.0.50", | ||
| ) | ||
| iprange.save() | ||
|
|
||
| factory.make_StaticIPAddress( | ||
| subnet=subnet, | ||
| alloc_type=IPADDRESS_TYPE.AUTO, | ||
| ip="192.168.0.30", | ||
| ) | ||
|
|
||
| iprange.end_ip = "192.168.0.49" | ||
| iprange.clean() | ||
| iprange.save() | ||
|
|
||
| def test_modify_existing_dynamic_range_can_expand_with_allocated_ip(self): | ||
| subnet = make_plain_subnet() | ||
| iprange = IPRange( | ||
| subnet=subnet, | ||
| type=IPRANGE_TYPE.DYNAMIC, | ||
| start_ip="192.168.0.10", | ||
| end_ip="192.168.0.50", | ||
| ) | ||
| iprange.save() | ||
|
|
||
| factory.make_StaticIPAddress( | ||
| subnet=subnet, | ||
| alloc_type=IPADDRESS_TYPE.AUTO, | ||
| ip="192.168.0.30", | ||
| ) | ||
|
|
||
| iprange.end_ip = "192.168.0.60" | ||
| iprange.clean() | ||
| iprange.save() |
|
Thanks for the fix. Implementation
TestingI ran additional cases on top of the PR tests, including the subnet-change guard. Expected to pass on resize:
All of the above passed. Expected to fail on resize:
All of the above were rejected as expected. |
|
Nice work on the caching and subnet guard! Could you rebase onto upstream master when you get a chance? Some of the tests were failing are fixed there now. On v3 side, the logic lives in src/maasapiserver/v3/api/public/models/requests/ipranges.py inside to_builder(), called from update_fabric_vlan_subnet_iprange() in the handler. Subnet move is less of an issue there since the URL pins the subnet, but the resize-with-lease-inside-pool bug will still show up. We can create a seperate PR for that. |
22860a9 to
34b548d
Compare
|
Thanks for the review! Good idea to add tests for the failure cases. I just rebased and added some tests to cover the ones you mentioned: resizing, moving, or creating dynamic ranges over a busy IP. I'll look into the v3 API and put up changes in a separate PR. Thank you for the pointers regarding that |
cf1fb73 to
9ff29df
Compare
…ns LP: #2143090 When an IP is allocated in a dynamic range reservation, the resize operation validator views the range as discontinuous and instead sees multiple contiguous blocks separated by each allocated IP. As a result, shrinking and expanding the reservation are both rejected with "Requested dynamic range conflicts with an existing IP address or range" despite the requested action being otherwise sensible (LP: #2143090) This commit addresses the issue by making sure the new boundaries are contained by an unused range
…ging Subnets Add tests to cover the expected failure of a resize over a busy IP, ensure that moving a dynamic range to a new subnet with a busy IP fails validation, and finally ensure that creation of a range over a busy IP is also disallowed
d1d5d25 to
638b2fa
Compare
|
lgtm. Thank you for the edits. Just one last test to make this concrete. |
|
|
||
| self._raise_validation_error(message) | ||
|
|
||
| def _is_existing_dynamic_range_resize_allowed( |
There was a problem hiding this comment.
why is this logic specific only for dynamic ranges? what about the other reserved ranges?
There was a problem hiding this comment.
The other types of reserved ranges use different validation logic. This is specified in L255 in src/maasserver/models/iprange.py
# Reserved ranges can overlap allocated IPs but not other ranges.
# Dynamic ranges cannot overlap anything (no ranges or IPs).
So Reserved ranges can overlap IPs and only hit a conflict when the range is overlapping another range. Because of that, resizing a reserved range is unaffected by allocations inside it. As a result when an IP address within a Reserved range was allocated, it would not affect the validation of the operation.
However, for dynamic range reservations, a resize should also be allowed in this case (because the IP allocation belongs to the existing reservation). However, the validation logic was incorrect and would reject such a resize because it viewed the allocated IP as splitting a continuous block of IPs and thought there was no way to modify the range reservation without overlapping an IP, which is not allowed.
| shrinking (no added segments) or if all newly added segments fit | ||
| within available unused ranges. Returns False if the original range | ||
| was not dynamic, the range moved to a different subnet, or any added | ||
| segment would overlap allocated IPs or other ranges. |
There was a problem hiding this comment.
would overlap allocated IPs
and where would this logic be in this function?
There was a problem hiding this comment.
In _validate_duplicates_and_overlaps the unused ranges are computed in this line:
unused = self.subnet.get_ipranges_available_for_dynamic_range(
exclude_ip_range_id=self.id
)
That unused value is passed to this helper function and in this function we look at the proposed new range reservation boundaries and ensure that both are in unused ranges. Any extension to the left (eg. decreasing the lower bound of the range) must be in the same unused range as the current lower bound. Similar story for expansions to the right (increasing the upper bound of the reservation). If there is another allocated IP, the get_ipranges_available_for_dynamic_range will have split the available ranges on that IP, and then the following code block will return False (rejecting any proposed resize that would overlap an allocated IP) since the proposed boundary would not be in the same unused range as the current one
if not any(
added_start_ip in unused_range and added_end_ip in unused_range
for unused_range in unused
):
return False
When an IP is allocated in a dynamic range reservation, the resize operation validator views the range as discontinuous and instead sees multiple contiguous blocks separated by each allocated IP. As a result, shrinking and expanding the reservation are both rejected with "Requested dynamic range conflicts with an existing IP address or range" despite the requested action being otherwise sensible
This commit addresses the issue by making sure the new boundaries are contained by an unused range
Resolves LP:2143090