[client] Fix forwarder peers never excluded from lazy connections#6674
Open
riccardomanfrin wants to merge 4 commits into
Open
[client] Fix forwarder peers never excluded from lazy connections#6674riccardomanfrin wants to merge 4 commits into
riccardomanfrin wants to merge 4 commits into
Conversation
Pure stylistic refactor: pull the AllowedIPs match into a named peerRoutesAddr helper and document why forward-target peers are excluded from lazy connections. No behavior change; the existing address match is preserved as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
toExcludedLazyPeers compares AllowedIPs (CIDR) against the unmasked TranslatedAddress, so forward-target peers are never excluded. This test asserts the peer is excluded and fails on the current behavior; the fix follows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
peerRoutesAddr compared AllowedIPs (CIDR, e.g. a peer's overlay IP as /32) against the unmasked TranslatedAddress string, so the match never fired and forward-target peers were never excluded from lazy connections. Use prefix containment so a routed address matches the peer's AllowedIP. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR changes how ChangesLazy peer exclusion logic
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ToExcludedLazyPeers
participant peerRoutesAddr
participant prefixesContain
participant ExcludedPeerSet
ToExcludedLazyPeers->>peerRoutesAddr: check peer AllowedIps for TranslatedAddress
peerRoutesAddr->>prefixesContain: check prefixes contain address
prefixesContain-->>peerRoutesAddr: contains result
peerRoutesAddr-->>ToExcludedLazyPeers: contains result
ToExcludedLazyPeers->>ExcludedPeerSet: add matching peer
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
Instead of re-parsing the network map AllowedIPs strings, look up the already-parsed []netip.Prefix from peerStore.AllowedIPs (the same typed value the lazy manager itself consumes). A down/lazy peer still has its conn in the store, so exclusion is unaffected by connection state. Extract a pure prefixesContain helper and unit-test it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/engine_lazy_exclude_test.go (1)
44-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a case where the target peer isn't yet in
peerStore.This test only covers the scenario where the peer's
peerStoreentry already has matchingAllowedIps. It doesn't cover the case wheretoExcludedLazyPeersis invoked for a peer that hasn't been added topeerStoreyet (i.e.,AllowedIPsreturnsok=false), which is the scenario flagged as a potential regression risk inengine.go. Adding this case would confirm the intended fallback behavior (or expose the gap).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/engine_lazy_exclude_test.go` around lines 44 - 67, Add a test case for Engine.toExcludedLazyPeers covering the fallback path when the forward-target peer is not yet present in peerStore and AllowedIPs returns ok=false. Reuse the existing TestToExcludedLazyPeers_ForwardTarget structure, but omit the target peer from the ConnStore and assert the intended exclusion behavior for that peer while keeping the non-target peer unaffected. This will validate the Engine.peerStore/AllowedIPs lookup path and protect the regression risk called out in engine.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/internal/engine_lazy_exclude_test.go`:
- Around line 44-67: Add a test case for Engine.toExcludedLazyPeers covering the
fallback path when the forward-target peer is not yet present in peerStore and
AllowedIPs returns ok=false. Reuse the existing
TestToExcludedLazyPeers_ForwardTarget structure, but omit the target peer from
the ConnStore and assert the intended exclusion behavior for that peer while
keeping the non-target peer unaffected. This will validate the
Engine.peerStore/AllowedIPs lookup path and protect the regression risk called
out in engine.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 839b8185-0b73-44f7-bb23-a312d43cf21a
📒 Files selected for processing (2)
client/internal/engine.goclient/internal/engine_lazy_exclude_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
toExcludedLazyPeerscompared each peer'sAllowedIPs(CIDR, e.g. an overlay IP as/32) againstForwardRule.TranslatedAddress.String()(unmasked), so the match never fired and forward-target peers were never excluded from lazy connections — inbound forwarded traffic can't wake a lazy peer, so those peers must stay permanent. Extracted apeerRoutesAddrhelper and switched to prefix containment.Issue ticket number and link
Internal bug found while investigating lazy-connection DNS/forwarding breakage. Broken line on main: https://github.com/netbirdio/netbird/blob/main/client/internal/engine.go#L2568
Stack
Checklist
Documentation
Select exactly one:
Internal lazy-connection exclusion fix. No public API, config, or CLI change.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
N/A
Summary by CodeRabbit
Bug Fixes
Tests