Skip to content

policy: reduce routes sent to peers based on packetfilter #2561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
kradalby merged 11 commits into from
May 4, 2025
Merged

policy: reduce routes sent to peers based on packetfilter #2561

kradalby merged 11 commits into from
May 4, 2025

Conversation

@kradalby
Copy link
Collaborator

@kradalby kradalby commented May 3, 2025 •
edited
Loading

Fixes #2365

@kradalby kradalby requested a review from juanfont as a code owner May 3, 2025 08:13
@ghost
Copy link

ghost commented May 3, 2025 •
edited by ghost
Loading

Pull Request Revisions

RevisionDescription
r4
Route accessibility filtering implementedNew feature limits route distribution to only routes accessible to specific nodes
r3
Updated routing and ACL filtering logicRefactored policy management to improve route and node filtering based on ACL rules, adding more granular route access control and introducing a new ReduceRoutes function to filter routes for specific nodes
r2
Enhanced ACL and route filtering mechanismAdded new route reduction method in policy package with comprehensive ACL filtering for subnet routes, including support for granular access control based on source IPs and destination networks
r1
Policy route and node filtering updatedIntroduced ReduceNodes and ReduceRoutes methods to handle policy-based filtering of nodes and routes more flexibly, with accompanying test cases and implementation changes.
✅ AI review completed for r4
Help React with emojis to give feedback on AI-generated reviews:
  • 👍 means the feedback was helpful and actionable
  • 👎 means the feedback was incorrect or unhelpful
💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at team@review.ai.

@kradalby kradalby mentioned this pull request May 3, 2025
Closed
4 tasks
ghost
ghost approved these changes May 3, 2025
View reviewed changes
@kradalby kradalby force-pushed the kradalby/reduce-routes branch from b860003 to 1929942 Compare May 3, 2025 21:00
ghost
ghost approved these changes May 3, 2025
View reviewed changes
kradalby added 10 commits May 4, 2025 16:01
@kradalby
notifier: use convenience funcs
388bf5c 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
policy: reduce routes based on policy
0d17cdd 
Fixes juanfont#2365

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
hsic: more helper methods
a026a9a 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
policy: more test cases
3c5e358 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
integration: add route with filter acl integration test
c38667e 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
integration: correct route reduce test, now failing
8b11ab3 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
mapper: compare peer routes against node
9b14563 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
hs: more output to debug strings
471ba2e 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
types/node: slice.ContainsFunc
399e832 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby
policy: more reduce route test
9aaa458 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby kradalby force-pushed the kradalby/reduce-routes branch from 1929942 to 9aaa458 Compare May 4, 2025 14:12
ghost
ghost approved these changes May 4, 2025
View reviewed changes
@kradalby
changelog: add entry for route filter
e93cd8b 
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
ghost
ghost reviewed May 4, 2025
View reviewed changes
hscontrol/policy/policy.go
Copy link

@ghost ghost May 4, 2025

Choose a reason for hiding this comment

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

In the ReduceFilterRules function in hscontrol/policy/policy.go, there's a comment typo at line 76-77: 'ensure they are note removed' should be 'ensure they are not removed'.

ghost
ghost reviewed May 4, 2025
View reviewed changes
hscontrol/mapper/tail.go
Copy link

@ghost ghost May 4, 2025

Choose a reason for hiding this comment

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

In hscontrol/mapper/tail.go, consider adding error handling when calling policy.ReduceRoutes in case the function's behavior changes in the future to potentially return an error. Currently, it silently continues execution if there are any issues with route reduction.

@kradalby kradalby merged commit 45e38cb into juanfont:main May 4, 2025
143 of 146 checks passed
@hanjo hanjo mentioned this pull request May 17, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nblock nblock nblock approved these changes

@juanfont juanfont Awaiting requested review from juanfont juanfont is a code owner

@ohdearaugustin ohdearaugustin Awaiting requested review from ohdearaugustin ohdearaugustin is a code owner

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Subnet routes are pushed to clients when not in allowed ACL

2 participants

@kradalby @nblock