Skip to content
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

Approximate aggregation : IP sort #222

Closed
wants to merge 5 commits into from

Conversation

dreamkinn
Copy link

@dreamkinn dreamkinn commented Sep 17, 2023

Hi, here is a modest contribution proposal to the AggregateApproxIPV4s feature.
TL;DR : In its current state, the algorithm used to heuristically aggregate IPv4s in approximate subnets gives different results depending on the order in which the addresses are provided.

The algorithm creates a new /24 subnet for each new IP address that is not already contained in a previously seen subnet, and then for each new address matching the subnet, it specializes the subnet mask. The found subnets are therefore highly dependent on the order in which the IPs were provided in the input.
Here is a short example :

~:$ cat ips_ordered
File: ips_ordered
10.10.0.1
10.10.0.2
10.10.0.255
~:$ cat ips_ordered | mapcidr -aa -silent
10.10.0.0/24                   # Expected output
~:$ cat ips
File: ips
10.10.0.1
10.10.0.255
10.10.0.2
~:$ cat ips | mapcidr -aa -silent
10.10.0.0/30                   # Misleading output

In the first case (ordered input), the algorithm first creates a /24 subnet (10.10.0.1), then specializes in /30 (10.10.0.2), and finally opens back up to /24 to match the .255. This generates a nice "guessed" /24 subnet that contains all of the input.

However, in the second case, the algorithm creates a /24 (10.10.0.1), stays at /24 (10.10.0.255), and then specializes to /30 because of the .2. The result of this is an output in /30 which misleads the user to think that no IP outside the /30 range exists in the dataset.

The contribution I propose is a sort of the input IP addresses at the start of the function which :

  • Does create an overhead
  • But leads to uniform output

Please let me know if this is a known/intended behaviour.

PS

The sort feature did not fix the output in my case :

~:$ cat ips | mapcidr -aa -silent -s
10.10.0.1
10.10.0.2
10.10.0.255
10.10.0.0/30

@ehsandeep ehsandeep changed the base branch from main to dev September 19, 2023 14:10
@Mzack9999 Mzack9999 mentioned this pull request Sep 19, 2023
@Mzack9999
Copy link
Member

Superseded by #223 - Opened a new PR with the content of this as I'm unable to push fixes on remote

@ehsandeep
Copy link
Member

@dreamkinn thanks for the PR, these changes are now merged into #223

@ehsandeep ehsandeep closed this Sep 20, 2023
@ehsandeep ehsandeep removed the request for review from Mzack9999 September 20, 2023 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants