[bug/change] In Hierarchical View, Master Subnet show subordinate IPs as Available

[morganrallen]

First off this could just be my misunderstanding how this should be displayed, if so some clarification could be helpful anyhow.

I've got 3 subnets

• 10.10.0.0/16 < master
• | 10.10.0.0/24
• | 10.10.1.0/24

Viewing from the top it looks like 100% of IPs are available. But coming into one of the subordinate subnets we can see that's not really the case.

So not sure what the behavior should be but my immediate feeling is, since this is a subnet hierarchy those should be considered unavailable. If this is correct I can probably take a crack at a patch, I'm hoping this will work out for an upcoming project.

[nemesifier]

@morganrallen smells like a bug! Thanks for reporting.

[morganrallen]

OK cool, I've even dug into this a bit and validating subordinate subnets going up the hierarchy is quite straight forward but ensuring an IP isn't used by a subordinate subnet was going to take a bit more.

@@ -244,9 +273,19 @@ class AbstractIpAddress(TimeStampedEditableModel):
             raise ValidationError(
                 {'ip_address': _('IP address does not belong to the subnet')}
             )
+
+        subnet_list = [self.subnet_id]
+
+        # traverse up the subnet list to gather id to check
+        if self.subnet.master_subnet is not None:
+            ms = self.subnet.master_subnet
+            while ms is not None:
+                subnet_list.append(ms.id)
+                ms = ms.master_subnet
+
         addresses = (
             load_model('openwisp_ipam', 'IpAddress')
-            .objects.filter(subnet=self.subnet_id)
+            .objects.filter(subnet__in=subnet_list)

[nemesifier]

OK cool, I've even dug into this a bit and validating subordinate subnets going up the hierarchy is quite straight forward but ensuring an IP isn't used by a subordinate subnet was going to take a bit more.

@@ -244,9 +273,19 @@ class AbstractIpAddress(TimeStampedEditableModel):
             raise ValidationError(
                 {'ip_address': _('IP address does not belong to the subnet')}
             )
+
+        subnet_list = [self.subnet_id]
+
+        # traverse up the subnet list to gather id to check
+        if self.subnet.master_subnet is not None:
+            ms = self.subnet.master_subnet
+            while ms is not None:
+                subnet_list.append(ms.id)
+                ms = ms.master_subnet
+
         addresses = (
             load_model('openwisp_ipam', 'IpAddress')
-            .objects.filter(subnet=self.subnet_id)
+            .objects.filter(subnet__in=subnet_list)

Are you talking about validation now?
I thought we were talking about visualization. Can you please clarify your last statement?

[morganrallen]

I was assuming if the visualization is incorrect than validation is also incorrect and the complete fix would be in multiple locations.

[nemesifier]

I was assuming if the visualization is incorrect than validation is also incorrect and the complete fix would be in multiple locations.

I see, but the logic for visualization and validation is not the same, we dealt recently with one of these issues: #87. if you find issues with validation please open a bug report like that one. In this issue we shall focus on visualization.

So, could you make it extremely clear what the expected outcome would be here?

I guess the expected outcome is that when visualizing the root subnet, we consider the space occupied in the child subnets as occupied also in the root/master subnets.

Right? Or do you have anything else in mind?

[morganrallen]

I guess the expected outcome is that when visualizing the root subnet, we consider the space occupied in the child subnets as occupied also in the root/master subnets.

Yes, that captures it well. I'll open a separate issue if the IP validation is still an actual issue.

[nemesifier]

While further investigating this issue I found that another thing we could change is to expand the tree view in the master subnet by default, that is, show the tree instead of need to click on it as in the GIF below: