Skip to content

[management] Revert "[management] allow local routing peer resource (#5814)"#5847

Merged
pascal-fischer merged 1 commit intomainfrom
revert/local-routing-peer-resource
Apr 10, 2026
Merged

[management] Revert "[management] allow local routing peer resource (#5814)"#5847
pascal-fischer merged 1 commit intomainfrom
revert/local-routing-peer-resource

Conversation

@pascal-fischer
Copy link
Copy Markdown
Collaborator

@pascal-fischer pascal-fischer commented Apr 10, 2026

Describe your changes

There was some miscommunication. This feature will not yet be added.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

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

  • Breaking Changes
    • Removed the on_routing_peer property from network resource API endpoints and responses. This property is no longer available when creating, updating, or retrieving network resources.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 10, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

📝 Walkthrough

Walkthrough

This PR removes the OnRoutingPeer field from the NetworkResource struct and eliminates all associated initialization, storage, retrieval, and firewall rule generation logic throughout the codebase.

Changes

Cohort / File(s) Summary
Core Resource Type
management/server/networks/resources/manager.go, management/server/networks/resources/types/resource.go
Removed OnRoutingPeer parameter from NewNetworkResource() function signature; deleted field from NetworkResource struct; updated ToAPIResponse(), FromAPIRequest(), and Copy() methods to exclude the field.
Database/Storage Layer
management/server/store/sql_store.go, management/server/store/sql_store_test.go
Removed on_routing_peer column selection from SQL query in getNetworkResources(); removed row scanning logic for the field; updated test constructor call to match new signature.
Network Map Firewall Logic
management/server/types/networkmap_components.go
Removed peer/local resource firewall rule generation when isRoutingPeer && resource.OnRoutingPeer; deleted getLocalResourceFirewallRules() function; simplified getNetworkResourcesRoutesToSync() return signature; replaced golang.org/x/exp/maps with standard maps package.
API Schema & Generated Types
shared/management/http/api/openapi.yml, shared/management/http/api/types.gen.go
Removed on_routing_peer boolean property from NetworkResourceMinimum schema definition; removed OnRoutingPeer *bool JSON field from NetworkResource, NetworkResourceMinimum, and NetworkResourceRequest Go structs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • crn4

Poem

🐰 With a whisker-twitch and a hop so light,
We've removed OnRoutingPeer—the code's now right!
No firewall tangles, no routing confusion,
Just clean, simple resources—a perfect conclusion! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description is largely incomplete. It provides a brief explanation for the revert but lacks key details: missing issue ticket link, no stack information, no checklist items selected, and minimal context about what was reverted. Add the issue ticket number/link that prompted the revert, select appropriate checklist item (appears to be a refactor/revert), and provide more context about why the feature communication failed.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: reverting a specific feature (local routing peer resource) by referencing the original PR number.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert/local-routing-peer-resource

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
management/server/types/networkmap_components.go (1)

695-739: Consider splitting the router and source-peer paths.

This function is still doing router membership detection, posture filtering, source-peer aggregation, and route materialization in one loop. Extracting the addSourcePeers branch and the peer can consume resource branch into small helpers would make future changes here much easier to reason about.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@management/server/types/networkmap_components.go` around lines 695 - 739, The
getNetworkResourcesRoutesToSync function is doing router membership detection,
posture filtering, source-peer aggregation, and route materialization in one
loop; extract and replace the two main branches into small helpers: create
collectSourcePeersForResource(resource *NetworkResource, policy *ResourcePolicy,
peers []string, addSourcePeers bool) which uses c.getPostureValidPeers and
updates allSourcePeers, and create materializeRoutesForConsumingPeer(resource
*NetworkResource, peerID string, networkRoutingPeers map[string]*Router, policy
*ResourcePolicy) which checks slices.Contains(peers, peerID) and
c.ValidatePostureChecksOnPeer and calls c.getNetworkResourcesRoutes for each
router to append to routes; keep router detection (networkRoutingPeers, router,
addSourcePeers) in getNetworkResourcesRoutesToSync and call these helpers inside
the policy loop (use names ResourcePoliciesMap, getNetworkResourcesRoutes,
getPostureValidPeers, ValidatePostureChecksOnPeer to locate relevant logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@management/server/types/networkmap_components.go`:
- Around line 695-739: The getNetworkResourcesRoutesToSync function is doing
router membership detection, posture filtering, source-peer aggregation, and
route materialization in one loop; extract and replace the two main branches
into small helpers: create collectSourcePeersForResource(resource
*NetworkResource, policy *ResourcePolicy, peers []string, addSourcePeers bool)
which uses c.getPostureValidPeers and updates allSourcePeers, and create
materializeRoutesForConsumingPeer(resource *NetworkResource, peerID string,
networkRoutingPeers map[string]*Router, policy *ResourcePolicy) which checks
slices.Contains(peers, peerID) and c.ValidatePostureChecksOnPeer and calls
c.getNetworkResourcesRoutes for each router to append to routes; keep router
detection (networkRoutingPeers, router, addSourcePeers) in
getNetworkResourcesRoutesToSync and call these helpers inside the policy loop
(use names ResourcePoliciesMap, getNetworkResourcesRoutes, getPostureValidPeers,
ValidatePostureChecksOnPeer to locate relevant logic).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: da8dd6b2-c654-48c7-b4e8-009e6f1e0636

📥 Commits

Reviewing files that changed from the base of the PR and between 2a8aacc and 6d6b41b.

📒 Files selected for processing (7)
  • management/server/networks/resources/manager.go
  • management/server/networks/resources/types/resource.go
  • management/server/store/sql_store.go
  • management/server/store/sql_store_test.go
  • management/server/types/networkmap_components.go
  • shared/management/http/api/openapi.yml
  • shared/management/http/api/types.gen.go
💤 Files with no reviewable changes (2)
  • shared/management/http/api/openapi.yml
  • shared/management/http/api/types.gen.go

@pascal-fischer pascal-fischer merged commit ee588e1 into main Apr 10, 2026
45 checks passed
@pascal-fischer pascal-fischer deleted the revert/local-routing-peer-resource branch April 10, 2026 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants