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.
There was a problem hiding this comment.
There is an explicit family filter in try_to_allocate_dynamic_ip and this scenario is not possible to happen.
There was a problem hiding this comment.
@neddp, you're absolutely right — I already have this noted in my long-term memory from PR #2657. The address-family filter applied in try_to_allocate_dynamic_ip (lines 93–97 of ip_repo.rb) ensures that addresses_we_cant_allocate is always homogeneous in family before eql?/hash is ever consulted, making a cross-family collision impossible in practice. I apologize for the noise — this comment was mistaken.
🧠 Learnings used
Learnt from: neddp
Repo: cloudfoundry/bosh PR: 2657
File: src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb:87-92
Timestamp: 2026-03-31T15:00:15.416Z
Learning: In `src/bosh-director/lib/bosh/director/deployment_plan/ip_provider/ip_repo.rb`, the `try_to_allocate_dynamic_ip` method applies an explicit address-family filter on `addresses_we_cant_allocate` (lines ~93–97) before calling `find_next_available_ip`. IPv4 entries are removed when the subnet is IPv6, and IPv6 entries are removed otherwise. Therefore, `overlaps?` in `IpAddrOrCidr` is never invoked with mixed-family (IPv4 vs IPv6) inputs — do not flag the absence of a family guard in `overlaps?` as a bug.
Learnt from: aramprice
Repo: cloudfoundry/bosh PR: 2704
File: src/bosh-director/spec/unit/bosh/director/cloud_config/cloud_configs_consolidator_spec.rb:89-91
Timestamp: 2026-03-31T09:34:42.259Z
Learning: In `src/bosh-director/spec/unit/bosh/director/cloud_config/cloud_configs_consolidator_spec.rb`, the `find_by_ids` method (using `dataset.where(id: ids).all`) does not guarantee a deterministic return order. Therefore, in the `#create_from_model_ids` spec, using `contain_exactly` (instead of an ordered equality matcher like `.with([...])`) is intentional and correct — do not flag this as an issue.
There was a problem hiding this comment.
Would adding it still make sense because there isn't a way to control how the class will be used? That is to say even though the code isn't currently impacted by the lack of #ipv4? in the #hash method for completeness.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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
github-project-automation
bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
2 participants
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 test run in Concourse failed. It looks like the issue is still present 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.