[management] Exclude proxy from peer approval#5588
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA boolean parameter in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/shared/grpc/proxy_auth.go`:
- Around line 74-82: Restore the original proxy token validation in both
interceptors by calling interceptor.validateProxyToken(ctx) and handling its
error: if validateProxyToken returns an error, log the failure with
log.WithContext(ctx).Warnf("proxy auth failed: %v", err) and return the error
(preventing request handling); only when validateProxyToken succeeds set ctx =
context.WithValue(ctx, ProxyTokenContextKey, token) using the returned
*types.ProxyAccessToken. Ensure you apply this fix in both places where an empty
ProxyAccessToken was injected so failure limiting and last-used auditing
continue to work.
In `@management/server/peer.go`:
- Line 749: The call to integratedPeerValidator.PreparePeer conflates the
temporary flag with embedded-proxy semantics by passing temporary ||
newPeer.ProxyMeta.Embedded; change this to pass the embedded proxy value
explicitly so the validator can distinguish the concepts—i.e., update the
PreparePeer invocation to pass newPeer.ProxyMeta.Embedded (not ORed with
temporary) and, if needed, add a separate parameter to PreparePeer (and its
implementations) to accept an embeddedProxy bool while leaving the existing
temporary parameter semantics unchanged; ensure references using PreparePeer
(and any validator implementations) are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b584d134-b461-4492-8859-80d1726bd7ea
📒 Files selected for processing (2)
management/internals/shared/grpc/proxy_auth.gomanagement/server/peer.go
| } | ||
|
|
||
| newPeer = am.integratedPeerValidator.PreparePeer(ctx, accountID, newPeer, peerAddConfig.GroupsToAdd, settings.Extra, temporary) | ||
| newPeer = am.integratedPeerValidator.PreparePeer(ctx, accountID, newPeer, peerAddConfig.GroupsToAdd, settings.Extra, temporary || newPeer.ProxyMeta.Embedded) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PreparePeer implementations =="
rg -n -C4 'func .*PreparePeer\(.*temporary bool' .
echo
echo "== Temporary flag usage in management server =="
rg -n -C4 '\btemporary\b' management/server -g '!**/*_test.go'
echo
echo "== PreparePeer call sites =="
rg -n -C2 'PreparePeer\(' .Repository: netbirdio/netbird
Length of output: 20452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== IntegratedValidator implementations (non-mock) =="
rg -n 'type.*struct' management/server/integrations/integrated_validator/ --type go
rg -n 'func.*PreparePeer' management/server/integrations/ -g '!**/*_test.go' -A 8
echo
echo "== Check if there are integrations directory implementations =="
find management/server/integrations -name '*validator*.go' -type f | grep -v _test.go
echo
echo "== Search for PreparePeer implementations more broadly =="
rg -n 'func.*PreparePeer' . -g '!**/*_test.go' -g '!**/mock*'Repository: netbirdio/netbird
Length of output: 349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Look for IntegratedValidator struct implementations =="
find management/server/integrations -name '*.go' -type f | grep -v _test.go | xargs cat | grep -A 20 "type.*struct" | head -60
echo
echo "== Search for files that might contain IntegratedValidator implementations =="
ls -la management/server/integrations/
echo
echo "== Check what's in integrated_validator directory =="
ls -la management/server/integrations/integrated_validator/
echo
echo "== Look for any non-mock PreparePeer implementation =="
rg 'func \(.*\) PreparePeer' management/ -g '!**/*_test.go' -g '!**/*mock*'Repository: netbirdio/netbird
Length of output: 3197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check git history for IntegratedValidator to see if implementations existed before =="
git log --all --oneline -- management/server/integrations/integrated_validator/ | head -10
echo
echo "== Search for TODO or FIXME comments related to implementations =="
rg 'TODO|FIXME' management/server/integrations/integrated_validator/
echo
echo "== Check if temporary flag is documented or has comments =="
rg -B 2 -A 2 'temporary' management/server/integrations/integrated_validator/interface.goRepository: netbirdio/netbird
Length of output: 839
Clarify semantics: temporary flag now conflates two unrelated concepts.
Line 749 passes temporary || newPeer.ProxyMeta.Embedded to PreparePeer, blending temporary user-added peer semantics with embedded proxy semantics into a single boolean. Currently no validator implementations exist, but the temporary flag already carries multiple meanings elsewhere (Ephemeral state, user permission validation, expiration control). Embedding proxy logic into this overloaded flag reduces clarity and makes future validator implementations more error-prone. Consider passing newPeer.ProxyMeta.Embedded directly to the validator so it can make explicit decisions about proxy handling separate from temporary peer logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/peer.go` at line 749, The call to
integratedPeerValidator.PreparePeer conflates the temporary flag with
embedded-proxy semantics by passing temporary || newPeer.ProxyMeta.Embedded;
change this to pass the embedded proxy value explicitly so the validator can
distinguish the concepts—i.e., update the PreparePeer invocation to pass
newPeer.ProxyMeta.Embedded (not ORed with temporary) and, if needed, add a
separate parameter to PreparePeer (and its implementations) to accept an
embeddedProxy bool while leaving the existing temporary parameter semantics
unchanged; ensure references using PreparePeer (and any validator
implementations) are updated accordingly.
|
2 participants
Describe your changes
Issue ticket number and link
#5571
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit