[fix] Dealt with small subnets #842 - #860
Conversation
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@pandafy I am leaving some comments to make your review easier.
| # the required number of generated subnets | ||
| available = 2 ** (self.size - master_subnet.prefixlen) | ||
| # Account for the reserved subnet | ||
| available -= 1 |
There was a problem hiding this comment.
@pandafy the reserved subnet was not taken into account in this calculation.
Therefore, if a master subnet has only 2 available slots but the number of subnets is 2, the first time the rule will run will try to take 3 slots (1 for the reserved subnet plus the 2 requested) and will fail.
Were you aware of this?
There was a problem hiding this comment.
Great catch on accounting for the reserved subnet! That's a crucial detail I missed.
There was a problem hiding this comment.
@nemesifier I was actually unaware of this bug. Thanks for catching this.
| self.assertEqual(str(index.subnet.subnet), '10.0.0.1/32') | ||
| self.assertEqual(index.ip.ip_address, '10.0.0.1') | ||
|
|
||
| def test_slash_32_rule_ipv4_error(self): |
There was a problem hiding this comment.
Here I try to create a subnet division rule for a /32 master subnet, which cannot work.
| self.assertEqual(str(index.subnet.subnet), 'fd12:3456:7890::1/128') | ||
| self.assertEqual(index.ip.ip_address, 'fd12:3456:7890::1') | ||
|
|
||
| def test_slash_128_rule_ipv6_error(self): |
There was a problem hiding this comment.
Same here but for ipv6.
| next( | ||
| ip_network(str(self.master_subnet.subnet)).subnets(new_prefix=self.size) | ||
| )[self.number_of_ips] | ||
| )[self.number_of_ips - 1] |
There was a problem hiding this comment.
/32 and /128 subnets only have the zero index, trying to access 1 fails, therefore I had to subtract 1 here.
| max_subnet = subnet_obj.subnet | ||
| finally: | ||
| return max_subnet | ||
| return max_subnet |
There was a problem hiding this comment.
Another problem I found here @pandafy was that line 194 above can fail if there's no space for the reserved subnet and when that happens, max_subnet is not defined, but finally will execute return max_subnet anyway (even if line 194 fails), so in that case the code fails because max_subnet is not defined.
I think we shouldn't use finally here. That statement is designed to be used in situations when it's imperative to call some cleanup operations even in case of unexpected failures.
I learnt a lot going through your PR. I initially tried to address the /32 subnet issue with a workaround, but I realize now that it wasn't the correct approach. I was too focused on simply preventing the error message without fully understanding the root cause and its implications. |
There was a problem hiding this comment.
@pandafy what if we change the implementation to account for a reserved subnet only when using big subnets, eg: only when master subnet is larger than 24 on ipv4, so from subnets having 512 addresses?
Or should we make this configurable with an additional flag in the subnet division rule?
I wonder why we did this in the first place?
I think it makes the implementation harder and I am not sure it's really useful. If this is the case I think we can create a new issue for this to fix it.
It's not uncommon for users to want to use this feature but getting stuck on erros like the one we are trying to fix here. If we manage to make this feature easier to use I am pretty sure it's a very useful feature to have in OpenWISP.
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
I think, I have found a bug in the current implementation. Try to create a subnet division rule with the following configuration
The automation generated the following IPs for this subnet
All 10.8.0.8
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
My latest commit addresses the issue found during the review.
I wrote a new test which replicated the bug and was failing before applying the fix.
I also took advantage of this change to make one of the existing tests more detailed.
@pandafy when we write tests for features like these, we should not limit ourselves to check only for quantity of data generated, we have to make sure that the data we generate matches our expectations.
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
LGTM! I have also performed manual testing.
4 participants
Supersedes #859.
Fixes #842
@pandafy @praptisharma28 please review.