Skip to content

[fix] Dealt with small subnets #842 #860

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 31, 2024
Merged

[fix] Dealt with small subnets #842 #860

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb8f6b8
[fix] Dealt with small subnets #842
nemesifier May 14, 2024
0883dfc
[fix] Fixed bug uncovered during review
nemesifier May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions openwisp_controller/subnet_division/base/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ def _validate_existing_fields(self):

def _validate_master_subnet_consistency(self):
master_subnet = self.master_subnet.subnet
# Validate size of generated subnet is not greater than size of master subnet
# Ensure that the size of the generated subnet
# is not greater than size of master subnet
try:
next(master_subnet.subnets(new_prefix=self.size))
except ValueError:
Expand All @@ -118,13 +119,19 @@ def _validate_master_subnet_consistency(self):
}
)

# Validate master subnet can accommodate required number of generated subnets
if self.number_of_subnets > (2 ** (self.size - master_subnet.prefixlen)):
# Ensure that master subnet can accommodate
# the required number of generated subnets
available = 2 ** (self.size - master_subnet.prefixlen)
# Account for the reserved subnet
available -= 1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch on accounting for the reserved subnet! That's a crucial detail I missed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nemesifier I was actually unaware of this bug. Thanks for catching this.

if self.number_of_subnets >= available:
raise ValidationError(
{
'number_of_subnets': _(
f'Master subnet cannot accommodate {self.number_of_subnets} '
f'subnets of size /{self.size}'
'The master subnet is too small to acommodate the '
'requested "number of subnets" plus the reserved '
'subnet, please increase the size of the master '
'subnet or decrease the "size of subnets" field.'
)
}
)
Expand All @@ -139,11 +146,10 @@ def _validate_master_subnet_consistency(self):
)

def _validate_ip_address_consistency(self):
# Validate individual generated subnet can accommodate required number of IPs
try:
next(
ip_network(str(self.master_subnet.subnet)).subnets(new_prefix=self.size)
)[self.number_of_ips]
)[self.number_of_ips - 1]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/32 and /128 subnets only have the zero index, trying to access 1 fails, therefore I had to subtract 1 here.

except IndexError:
raise ValidationError(
{
Expand Down
18 changes: 13 additions & 5 deletions openwisp_controller/subnet_division/rule_types/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,7 @@ def get_max_subnet(master_subnet, division_rule):
subnet_obj.full_clean()
subnet_obj.save()
max_subnet = subnet_obj.subnet
finally:
return max_subnet
return max_subnet
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep a note of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep a note of this.


@staticmethod
def create_subnets(config, division_rule, max_subnet, generated_indexes):
Expand Down Expand Up @@ -234,17 +233,26 @@ def create_subnets(config, division_rule, max_subnet, generated_indexes):
def create_ips(config, division_rule, generated_subnets, generated_indexes):
generated_ips = []
for subnet_obj in generated_subnets:
for ip_id in range(1, division_rule.number_of_ips + 1):
for ip_index in range(1, division_rule.number_of_ips + 1):
# don't assign first ip address of a subnet,
# unless we are working with small subnets
if subnet_obj.subnet.num_addresses != division_rule.number_of_ips:
subnet_index = ip_index
# this allows handling /32, /128 or cases in which
# the number of requested ip addresses matches exactly
# what is available in the subnet
else:
subnet_index = 0
ip_obj = IpAddress(
subnet_id=subnet_obj.id,
ip_address=str(subnet_obj.subnet[ip_id]),
ip_address=str(subnet_obj.subnet[subnet_index]),
)
ip_obj.full_clean()
generated_ips.append(ip_obj)

generated_indexes.append(
SubnetDivisionIndex(
keyword=f'{subnet_obj.name}_ip{ip_id}',
keyword=f'{subnet_obj.name}_ip{ip_index}',
subnet_id=subnet_obj.id,
ip_id=ip_obj.id,
rule_id=division_rule.id,
Expand Down
88 changes: 84 additions & 4 deletions openwisp_controller/subnet_division/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,11 @@ def test_field_validations(self):
rule.full_clean()
expected_message_dict = {
'number_of_subnets': [
'Master subnet cannot accommodate 99999999 subnets of size /28'
'The master subnet is too small to acommodate '
'the requested "number of subnets" plus the '
'reserved subnet, please increase the size of '
'the master subnet or decrease the '
'"size of subnets" field.'
]
}
self.assertDictEqual(
Expand Down Expand Up @@ -246,6 +250,73 @@ def test_field_validations(self):
{'organization': ['Organization should be same as the subnet']},
)

def test_slash_32_rule_ipv4(self):
rule = self._get_vpn_subdivision_rule(
size=32, number_of_ips=1, number_of_subnets=1
)
self.config.templates.add(self.template)
rule.subnetdivisionindex_set.count()
index_queryset = rule.subnetdivisionindex_set.filter(
config_id=self.config.id, subnet_id__isnull=False, ip_id__isnull=False
)
self.assertEqual(index_queryset.count(), 1)
index = index_queryset.first()
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):
Copy link
Member Author

@nemesifier nemesifier May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I try to create a subnet division rule for a /32 master subnet, which cannot work.

master_ipv4 = self._get_master_subnet(subnet='192.168.1.1/32')
self.vpn_server.subnet = master_ipv4
self.vpn_server.save()
try:
self._get_vpn_subdivision_rule(
size=32, number_of_ips=1, number_of_subnets=1, master_subnet=master_ipv4
)
except ValidationError as e:
self.assertIn('number_of_subnets', e.message_dict)
self.assertIn(
'The master subnet is too small to acommodate',
e.message_dict['number_of_subnets'][0],
)
else:
self.fail('Expected error not raised')

def test_slash_128_rule_ipv6(self):
master_ipv6 = self._get_master_subnet(subnet='fd12:3456:7890::/48')
self.vpn_server.subnet = master_ipv6
self.vpn_server.save()
rule = self._get_vpn_subdivision_rule(
size=128, number_of_ips=1, number_of_subnets=1, master_subnet=master_ipv6
)
self.config.templates.add(self.template)
index_queryset = rule.subnetdivisionindex_set.filter(
config_id=self.config.id, subnet_id__isnull=False, ip_id__isnull=False
)
self.assertEqual(index_queryset.count(), 1)
index = index_queryset.first()
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):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here but for ipv6.

master_ipv6 = self._get_master_subnet(subnet='fd12:3456:7890::/128')
self.vpn_server.subnet = master_ipv6
self.vpn_server.save()
try:
self._get_vpn_subdivision_rule(
size=128,
number_of_ips=1,
number_of_subnets=1,
master_subnet=master_ipv6,
)
except ValidationError as e:
self.assertIn('number_of_subnets', e.message_dict)
self.assertIn(
'The master subnet is too small to acommodate',
e.message_dict['number_of_subnets'][0],
)
else:
self.fail('Expected error not raised')

def test_rule_label_updated(self):
new_rule_label = 'TSDR'
rule = self._get_vpn_subdivision_rule(label='VPN_OW')
Expand Down Expand Up @@ -418,11 +489,20 @@ def test_multiple_vpnclient_delete(self):
@patch('logging.Logger.error')
def test_subnets_exhausted(self, mocked_logger):
subnet = self._get_master_subnet(
'10.0.0.0/28', master_subnet=self.master_subnet
)
'10.0.0.0/29', master_subnet=self.master_subnet
)
# The master subnet can acommodate
# this rule only once:
# A /29 has 4 /31 slots available
# Minus the reserved subnet = 3
# Each run will eat 2 slots.
# Hence we expect this to run fine the
# first time but fail the second time.
self._get_vpn_subdivision_rule(
master_subnet=subnet,
size=29,
size=31,
number_of_ips=2,
number_of_subnets=2,
)
self.vpn_server.subnet = subnet
self.vpn_server.save()
Expand Down