feat: add support for multi-octet and dash-based IP ranges#689
Conversation
WalkthroughAdds ExpandIPPattern to enumerate multi-octet Nmap-style IPv4 ranges and integrates it into CLI processing so dotted dash ranges are expanded into /32 CIDRs (or aggregated) before existing aggregation/sort/shuffle/count paths. Also adds tests and tweaks IP/CIDR utilities' error-return behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as mapcidr CLI
participant Expander as ExpandIPPattern
participant Aggregator as Aggregate/Sort/Shuffle/Count
participant Processor as commonFunc / Output
User->>CLI: Provide input (e.g., 192.168.0-1.1-2)
CLI->>CLI: Detect dotted IPv4 with dash (strings.Count == 3)
CLI->>Expander: ExpandIPPattern(pattern)
alt Expansion success
Expander-->>CLI: [net.IP list]
CLI->>CLI: For each IP -> form "/32"
alt flags require aggregation/sort/shuffle/count
CLI->>Aggregator: ParseCIDR and enqueue to allCidrs
Aggregator-->>Processor: Emit aggregated/sorted results
else direct processing
CLI->>Processor: commonFunc(ip/32)
end
else Expansion error
Expander-->>CLI: error
CLI->>CLI: Fatal log and exit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jjhwan-h
force-pushed
the
feat/660-support-ip-patterns
branch
from
dbf127f to
a551595
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/mapcidr/main.go (3)
444-452: Validate IP range endpoints before appending to ipRangeListnet.ParseIP can return nil; passing nils downstream makes error messages opaque. Fail fast with a clear message.
- for _, ipstr := range strings.Split(cidr, "-") { - ipRange = append(ipRange, net.ParseIP(ipstr)) - } + for _, ipstr := range strings.Split(cidr, "-") { + p := net.ParseIP(strings.TrimSpace(ipstr)) + if p == nil { + gologger.Fatal().Msgf("invalid IP in range: %q", ipstr) + } + ipRange = append(ipRange, p) + }
475-475: Clarify comment to match all bulk-processing conditionsSmall doc tweak to stay in sync with the if-condition.
- // In case of coalesce/shuffle we need to know all the cidrs and aggregate them by calling the proper function + // For aggregate/shuffle/sort/aggregate-approx/count we need to collect all CIDRs and process in bulk
421-442: Avoid materializing huge expansions in memory (prefer streaming/iterator)
Expanding multi‑octet ranges into []net.IP can explode memory/time (e.g., 10.0-255.0-255.0-255). Consider a streaming enumerator (channel/generator) so main.go consumes IPs incrementally, or detect and warn/abort on extreme cardinalities before allocation.Would you like me to draft a streaming ExpandIPPatternChan(pattern string) (<-chan net.IP, error) and the corresponding integration here?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/mapcidr/main.go(2 hunks)cmd/mapcidr/main_test.go(1 hunks)ip.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ip.go
- cmd/mapcidr/main_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/mapcidr/main.go (1)
ip.go (1)
ExpandIPPattern(1217-1279)
| for _, cidr := range cidrsToProcess { | ||
| // Add IPs into ipRangeList which are passed as input. Example - "192.168.0.0-192.168.0.5" | ||
|
|
||
| if strings.Contains(cidr, "-") { | ||
| // Try to parse as multi-octet range | ||
| if strings.Count(cidr, ".") == 3 { | ||
| ips, err := mapcidr.ExpandIPPattern(cidr) | ||
| if err != nil { | ||
| gologger.Fatal().Msgf("%s\n", err) | ||
| } | ||
|
|
||
| for _, ip := range ips { | ||
| ipCidr := ip.String() + "/32" | ||
| if options.Aggregate || options.Shuffle || hasSort || options.AggregateApprox || options.Count { | ||
| _, ipnet, _ := net.ParseCIDR(ipCidr) | ||
| allCidrs = append(allCidrs, ipnet) | ||
| } else { | ||
| commonFunc(ipCidr, outputchan) | ||
| } | ||
| } | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
Apply MatchIP/FilterIP to expanded IPs in aggregate/sort/shuffle/count paths; also guard against ip:port and check ParseCIDR error
Currently, expanded IPs bypass the per‑IP Match/Filter logic when any of aggregate/sort/shuffle/aggregate‑approx/count are enabled, which changes semantics vs. non‑expanded inputs. This can leak filtered IPs into outputs. Also, avoid treating ip:port strings as multi‑octet patterns and harden ParseCIDR error handling.
Apply this diff:
- // Try to parse as multi-octet range
- if strings.Count(cidr, ".") == 3 {
+ // Try to parse as multi-octet range (IPv4 only; avoid ip:port or CIDR-with-slash here)
+ if strings.Count(cidr, ".") == 3 && !strings.Contains(cidr, ":") && !strings.Contains(cidr, "/") {
ips, err := mapcidr.ExpandIPPattern(cidr)
if err != nil {
gologger.Fatal().Msgf("%s\n", err)
}
- for _, ip := range ips {
- ipCidr := ip.String() + "/32"
+ for _, ip := range ips {
+ ipStr := ip.String()
+ // Maintain parity with commonFunc filtering for expanded IPs
+ if len(matchIPSet) > 0 {
+ if _, ok := matchIPSet[ipStr]; !ok {
+ continue
+ }
+ } else if len(filterIPSet) > 0 {
+ if _, ok := filterIPSet[ipStr]; ok {
+ continue
+ }
+ }
+ ipCidr := ipStr + "/32"
if options.Aggregate || options.Shuffle || hasSort || options.AggregateApprox || options.Count {
- _, ipnet, _ := net.ParseCIDR(ipCidr)
- allCidrs = append(allCidrs, ipnet)
+ if _, ipnet, err := net.ParseCIDR(ipCidr); err != nil {
+ gologger.Fatal().Msgf("%s\n", err)
+ } else {
+ allCidrs = append(allCidrs, ipnet)
+ }
} else {
commonFunc(ipCidr, outputchan)
}
}
continue
}Add the following helper initialization inside process(), once, just after the var block and before the main loop (reuse prepareIPsFromCidrFlagList):
// Precompute exact-match sets for expanded IP filtering parity
matchIPSet := map[string]struct{}{}
filterIPSet := map[string]struct{}{}
if options.MatchIP != nil {
for _, ip := range prepareIPsFromCidrFlagList(options.MatchIP) {
matchIPSet[ip] = struct{}{}
}
}
if options.FilterIP != nil {
for _, ip := range prepareIPsFromCidrFlagList(options.FilterIP) {
filterIPSet[ip] = struct{}{}
}
}🤖 Prompt for AI Agents
In cmd/mapcidr/main.go around lines 421-442: expanded multi-octet ranges
currently bypass per-IP MatchIP/FilterIP when
Aggregate/Shuffle/Sort/AggregateApprox/Count are enabled and also can
misinterpret ip:port as a multi-octet pattern and ignore ParseCIDR errors; to
fix, add the suggested precomputed matchIPSet and filterIPSet once after the var
block and before the main loop (reuse prepareIPsFromCidrFlagList), then inside
the expanded-IP branch (the loop over ips) first skip patterns containing ':' so
we don't treat ip:port as a multi-octet range, for each expanded ip apply
MatchIP/FilterIP using the precomputed sets (skip or continue if filtered or
doesn't match), build ipCidr := ip.String()+"/32" and call net.ParseCIDR on
ipCidr and check for parse error (handle/log/continue on error) before appending
ipnet to allCidrs or calling commonFunc — this ensures expanded IPs follow the
same match/filter semantics and ParseCIDR failures are handled.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cidr.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: release-test
- GitHub Check: Test Builds
🔇 Additional comments (1)
cidr.go (1)
278-284: Internal callers unaffected; verify external usage
Only one internal call site ignores errors and won’t break on an empty slice vs nil. Since this is a public API change, ensure any external consumers don’t rely on a nil return.
| // func isPowerOfTwoPlusOne(x int) bool { | ||
| // return isPowerOfTwo(x - 1) | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove dead code instead of commenting it out.
While the documentation explaining why this function is obsolete is excellent, commented-out code should be deleted entirely. Version control preserves the history if needed.
Apply this diff to remove the dead code:
-// isPowerOfTwoPlusOne returns if a number is a power of 2 plus 1
-//
-// NOTE(dwisiswant0): This function is no longer used. The logic in
-// SplitIPNetIntoN was refactored to correctly handle non-power-of-two splits by
-// first splitting into the largest power-of-two less than or equal to n, and
-// then iteratively dividing the last subnet. This removed the need for this
-// specific check.
-//
-// nolint:all
-// func isPowerOfTwoPlusOne(x int) bool {
-// return isPowerOfTwo(x - 1)
-// }
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // func isPowerOfTwoPlusOne(x int) bool { | |
| // return isPowerOfTwo(x - 1) | |
| // } |
🤖 Prompt for AI Agents
In cidr.go around lines 259 to 261, remove the commented-out dead function
isPowerOfTwoPlusOne (the three lines currently commented); delete the commented
code entirely from the file (leave the existing documentation/explanation
intact) so the dead code is not kept in comments — rely on VCS history if
needed.
| // func reverseIPNet(ipnets []*net.IPNet) { | ||
| // for i, j := 0, len(ipnets)-1; i < j; i, j = i+1, j-1 { | ||
| // ipnets[i], ipnets[j] = ipnets[j], ipnets[i] | ||
| // } | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Remove dead code and verify no references remain.
Similar to isPowerOfTwoPlusOne, this commented-out function should be deleted entirely. Additionally, unlike the previous function, there's no explanation for why it's obsolete.
Apply this diff to remove the dead code:
-// reverseIPNet reverses an ipnet slice
-//
-// nolint:all
-// func reverseIPNet(ipnets []*net.IPNet) {
-// for i, j := 0, len(ipnets)-1; i < j; i, j = i+1, j-1 {
-// ipnets[i], ipnets[j] = ipnets[j], ipnets[i]
-// }
-// }
-Run the following script to verify reverseIPNet is not referenced elsewhere:
🏁 Script executed:
#!/bin/bash
# Description: Search for any references to reverseIPNet in the codebase
# Search for function calls or mentions
rg -nP '\breverseIPNet\b' --type=go -C3Length of output: 370
Remove the unused reverseIPNet code.
The commented-out function is unreferenced; delete it entirely.
--- a/cidr.go
+++ b/cidr.go
@@ -268,7 +268,0 @@
-// reverseIPNet reverses an ipnet slice
-//
-// nolint:all
-// func reverseIPNet(ipnets []*net.IPNet) {
-// for i, j := 0, len(ipnets)-1; i < j; i, j = i+1, j-1 {
-// ipnets[i], ipnets[j] = ipnets[j], ipnets[i]
-// }
-// }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // func reverseIPNet(ipnets []*net.IPNet) { | |
| // for i, j := 0, len(ipnets)-1; i < j; i, j = i+1, j-1 { | |
| // ipnets[i], ipnets[j] = ipnets[j], ipnets[i] | |
| // } | |
| // } |
🤖 Prompt for AI Agents
In cidr.go around lines 271 to 275, there's an unused commented-out function
reverseIPNet; remove the entire commented block (the function and its
surrounding comment markers) so the file contains no dead/commented code, then
run gofmt/goimports to tidy imports and formatting.
2 participants
Summary
This PR resolves #660 by adding support for multi-octet patterns (e.g.,
192.168.0-1.1-2), dash-based (e.g.,192.168.1.1-5)Changes
ExpandIPPatternto expand multi-octet expressions into validnet.IPlists.process) to handle mixed inputs.FilterIP)Aggregate)SortAscending/SortDescending)Examples
Related Issue
related #660
Summary by CodeRabbit