feat(acl-filter,config): Add restriction for user ACLs "scope: flow"#1655
feat(acl-filter,config): Add restriction for user ACLs "scope: flow"#1655qmonnet wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesACL validation now restricts ACL flow-scope validation
Masquerade-backed filter tests
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@config/src/external/overlay/acl.rs`:
- Around line 421-424: Fix the format string in the ACL validation error
construction to close the rule name’s quote before the explanatory text,
preserving the existing self.name interpolation and message content.
🪄 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: CHILL
Plan: Pro
Run ID: 7a08a4e0-fdc1-4dc1-ba3b-8ab2814c636f
📒 Files selected for processing (3)
acl-filter/src/tests.rsconfig/src/converters/k8s/config/acl.rsconfig/src/external/overlay/acl.rs
There was a problem hiding this comment.
Pull request overview
Adds config-time validation to reject user ACL rules with scope: flow when the peering configuration cannot guarantee flow tracking (currently only established via masquerade / port-forwarding), aligning behavior with the current dataplane capabilities described in #1625.
Changes:
- Add
scope: flowvalidation during ACL rule validation, with accompanying unit tests in the config model. - Update k8s ACL converter test helpers to plumb the CRD
scopefield through rule construction. - Adjust
acl-filtersemantic tests to build an overlay that includes masquerade so flow-scoped ACLs pass new validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| config/src/external/overlay/acl.rs | Introduces scope: flow validation at config validation time and adds tests covering acceptance/rejection. |
| config/src/converters/k8s/config/acl.rs | Updates test rule-builder helper to accept scope and updates test cases accordingly. |
| acl-filter/src/tests.rs | Updates flow-scope tests to use a masquerade peering so the new config validation accepts scope: flow. |
For user ACLs, "scope: flow" is only supported when the peering uses either masquerade or port forwarding for each connection; otherwise, we do not currently track flow, and cannot apply flow-scoped rules. Add the relevant restriction at validation time. Link: #1625 Signed-off-by: Quentin Monnet <qmo@qmon.net>
2e7d576 to
d13384d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/src/external/overlay/acl.rs (1)
1094-1106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the port-forwarding acceptance path.
The positive test covers masquerade only; add an equivalent port-forwarding case to protect the other supported branch.
🤖 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 `@config/src/external/overlay/acl.rs` around lines 1094 - 1106, Extend test_flow_scope_valid_with_flow_tracking to cover the port-forwarding branch in addition to masquerade. Build the right-hand manifest with the existing port-forwarding fixture or helper, create the equivalent rule and pattern, and assert validation succeeds for that case.
🤖 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 `@config/src/external/overlay/acl.rs`:
- Around line 1094-1106: Extend test_flow_scope_valid_with_flow_tracking to
cover the port-forwarding branch in addition to masquerade. Build the right-hand
manifest with the existing port-forwarding fixture or helper, create the
equivalent rule and pattern, and assert validation succeeds for that case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad2d1cec-6513-4fb9-ae07-ec4bff98b9a4
📒 Files selected for processing (3)
acl-filter/src/tests.rsconfig/src/converters/k8s/config/acl.rsconfig/src/external/overlay/acl.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- acl-filter/src/tests.rs
- config/src/converters/k8s/config/acl.rs
For user ACLs, "scope: flow" is only supported when the peering uses either masquerade or port forwarding for each connection; otherwise, we do not currently track flow, and cannot apply flow-scoped rules.
Add the relevant restriction at validation time.
Link: #1625