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

Handle adjacent prefixes in ipinfo tool aggregate #193

Conversation

talhahwahla
Copy link
Contributor

@talhahwahla talhahwahla commented Nov 20, 2023

In #187, we identified that ipinfo tool aggregate does not work for adjacent CIDRs.

Checking adjacency based on two conditions:

  • prefixes have the same mask length,
  • the second prefix is exactly one larger than the first prefix

This works for IPv4 only.

Copy link

linear bot commented Nov 20, 2023

BE-2715 Handle adjacent prefixes in `ipinfo tool aggregate`

Ref #187

$ ipinfo tool aggregate 203.97.2.0/24 203.97.3.0/24 
203.97.2.0/24
203.97.3.0/24

The tool ignores the adjacent prefixes, doesn't optimize them. We should combined adjacent prefixes under a single, shorter-length prefix, for example, for the above it should show: 203.97.2.0/23.

cc uman

@talhahwahla
Copy link
Contributor Author

Turns out we already had an internal implementation here, using it.

Comparing result with another tool.

$ cat list.txt 
103.4.108.0/24
103.4.109.0/24
103.4.110.0/23
114.141.208.0/21
114.141.208.0/24
182.16.140.0/23
182.16.142.0/23
182.16.142.0/24
27.0.10.0/24
27.0.11.0/24
27.0.8.0/22
27.0.8.0/23
27.0.8.0/24
27.0.9.0/24
27.0.9.1
8.8.8.8
99.9.9.9
b06b:482e:2113:4b1a:6567:e1c:5b8b:bf5d/64

$ cat list.txt | go run . tool aggregate
27.0.8.0/22
103.4.108.0/22
114.141.208.0/21
182.16.140.0/22
8.8.8.8
99.9.9.9

$ cat list.txt | aggregate -q
27.0.8.0/22
103.4.108.0/22
114.141.208.0/21
182.16.140.0/22

@talhahwahla talhahwahla marked this pull request as ready for review November 20, 2023 14:37
Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

We should move all this generic CIDR code into lib/cidr.go or something?

@talhahwahla talhahwahla force-pushed the talhahameed/be-2715-handle-adjacent-prefixes-in-ipinfo-tool-aggregate branch from 3a82aba to 2aedf33 Compare November 21, 2023 06:50
@UmanShahzad
Copy link
Contributor

@talhahwahla in the example you gave, the Ipv6 entry disappears. Intended?

@talhahwahla
Copy link
Contributor Author

@UmanShahzad yes, accepting and aggregating the Ipv4 only. Alternative is to print the Ipv6 as it is, which I guess might be confusing.

@UmanShahzad UmanShahzad merged commit d2f94f9 into master Nov 21, 2023
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

2 participants