Conversation
…rved IP allocation eql? delegated to == which loses CIDR prefix via IPAddr#coerce_other, causing eql?/hash to disagree and making Set dedup non-deterministic. Fix both to include prefix. Also change sort key to [to_i, prefix] so larger blocks always precede the /32s they contain, ensuring find_next_available_ip skips entire reserved ranges.
WalkthroughThe changes modify how IP addresses and CIDR blocks are compared and hashed in the IP allocation system. The Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb`:
- Around line 47-51: The equality and hash implementations in IpAddrOrCidr
ignore address family causing IPv4 and IPv6 addresses with the same
integer/prefix to collide; update the eql? method (the equality check that
currently compares `@ipaddr.to_i` and `@ipaddr.prefix`) to also compare the address
family (e.g., `@ipaddr.family` or using ipv4?/ipv6?) and change the hash method to
include the same family value (e.g., include `@ipaddr.family` in the array used to
compute the hash) so that family is part of both equality and hashing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0554b280-a5ef-4ddc-8135-62fbc2037f04
📒 Files selected for processing (3)
src/bosh-director/lib/bosh/director/deployment_plan/ip_provider/ip_repo.rbsrc/bosh-director/lib/bosh/director/ip_addr_or_cidr.rbsrc/bosh-director/spec/unit/bosh/director/ip_addr_or_cidr_spec.rb
| other.is_a?(IpAddrOrCidr) && @ipaddr.to_i == other.to_i && @ipaddr.prefix == other.prefix | ||
| end | ||
|
|
||
| def hash | ||
| @ipaddr.hash | ||
| [@ipaddr.to_i, @ipaddr.prefix].hash |
There was a problem hiding this comment.
Include address family in eql?/hash to prevent cross-family collisions.
On Line 47 and Line 51, equality/hash ignore family. Distinct values like IPv4 0.0.0.1/32 and IPv6 ::1/32 can collapse as one Set element.
💡 Proposed fix
def eql?(other)
- other.is_a?(IpAddrOrCidr) && `@ipaddr.to_i` == other.to_i && `@ipaddr.prefix` == other.prefix
+ other.is_a?(IpAddrOrCidr) &&
+ `@ipaddr.to_i` == other.to_i &&
+ `@ipaddr.prefix` == other.prefix &&
+ `@ipaddr.ipv4`? == other.ipv4?
end
def hash
- [`@ipaddr.to_i`, `@ipaddr.prefix`].hash
+ [`@ipaddr.to_i`, `@ipaddr.prefix`, `@ipaddr.ipv4`?].hash
end📝 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.
| other.is_a?(IpAddrOrCidr) && @ipaddr.to_i == other.to_i && @ipaddr.prefix == other.prefix | |
| end | |
| def hash | |
| @ipaddr.hash | |
| [@ipaddr.to_i, @ipaddr.prefix].hash | |
| other.is_a?(IpAddrOrCidr) && | |
| `@ipaddr.to_i` == other.to_i && | |
| `@ipaddr.prefix` == other.prefix && | |
| `@ipaddr.ipv4`? == other.ipv4? | |
| end | |
| def hash | |
| [`@ipaddr.to_i`, `@ipaddr.prefix`, `@ipaddr.ipv4`?].hash |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb` around lines 47 - 51,
The equality and hash implementations in IpAddrOrCidr ignore address family
causing IPv4 and IPv6 addresses with the same integer/prefix to collide; update
the eql? method (the equality check that currently compares `@ipaddr.to_i` and
`@ipaddr.prefix`) to also compare the address family (e.g., `@ipaddr.family` or
using ipv4?/ipv6?) and change the hash method to include the same family value
(e.g., include `@ipaddr.family` in the array used to compute the hash) so that
family is part of both equality and hashing.
cf-foundation-community-automation
bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
1 participant
What is this change about?
Follow-up fix to #2657 to eliminate residual flaky behaviour in IP allocation from reserved CIDR ranges.
PR #2657 fixed the algorithmic logic in
find_next_available_ip(pruning by last address, usingoverlaps?, capturingblocking_ipbefore mutation). However, the very second integration run in Concourse failed. It looks like the issue is still present under under certain Ruby hash seeds:Changes:
IpAddrOrCidr#eql?: now compares bothto_iandprefixdirectly, treating objects with the same base address but different prefix lengths as distinctIpAddrOrCidr#hash: now uses[to_i, prefix].hashto satisfy the contractfind_next_available_ipsort key: changed fromip.to_ito[ip.to_i, ip.prefix]so larger blocks (smaller prefix) always sort before the/32s they containPlease provide contextual information.
What tests have you run against this PR?
IpAddrOrCidrunit specs pass (spec/unit/bosh/director/ip_addr_or_cidr_spec.rb)IpRepounit specs pass, including the CIDR block tests added by Fix IP Allocation Bug: Reserved Range Not Detected #2657 (spec/unit/bosh/director/deployment_plan/ip_provider/ip_repo_spec.rb)#eql?and#hashcovering:eql?true, equal hashes, deduplicates in Seteql?false, different hashes, coexist as distinct Set elementseql?falseHow should this change be described in bosh release notes?
Fixed: IP allocation could intermittently assign addresses from reserved CIDR ranges due to non-deterministic Set behaviour in the IP dedup logic.
Does this PR introduce a breaking change?
No. This is a bug fix. The observable behaviour - that IPs are not allocated from reserved ranges — is unchanged; the fix makes it deterministic.