Skip to content

feat(acl-filter,config): Add restriction for user ACLs "scope: flow"#1655

Open
qmonnet wants to merge 1 commit into
mainfrom
pr/qmonnet/acl-validate-scope
Open

feat(acl-filter,config): Add restriction for user ACLs "scope: flow"#1655
qmonnet wants to merge 1 commit into
mainfrom
pr/qmonnet/acl-validate-scope

Conversation

@qmonnet

@qmonnet qmonnet commented Jul 21, 2026

Copy link
Copy Markdown
Member

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

@qmonnet qmonnet self-assigned this Jul 21, 2026
Copilot AI review requested due to automatic review settings July 21, 2026 17:09
@qmonnet
qmonnet requested a review from a team as a code owner July 21, 2026 17:09
@qmonnet
qmonnet requested review from Fredi-raspall and removed request for a team July 21, 2026 17:09
@qmonnet qmonnet added the area/acl Related to ACLs (Access Control Lists) label Jul 21, 2026
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

ACL validation now restricts scope: flow to peering manifests with masquerade or port-forwarding support. Conversion tests pass explicit scope values, and filter tests use masquerade-backed overlays for related-flow behavior.

ACL flow-scope validation

Layer / File(s) Summary
Flow-scope validation and coverage
config/src/external/overlay/acl.rs
Validation uses shared ConfigResult, checks expose support for flow scope, and adds acceptance and rejection tests.
Scoped rule conversion fixtures
config/src/converters/k8s/config/acl.rs
Conversion test helpers and call sites now pass explicit packet, flow, or unset scope values.

Masquerade-backed filter tests

Layer / File(s) Summary
Related-flow filter coverage
acl-filter/src/tests.rs
Adds a masquerade overlay builder and uses it for allowed-reply and explicit-deny flow-scope tests.

Possibly related issues

Possibly related PRs

Suggested reviewers: fredi-raspall

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the new validation restriction for user ACLs with scope: flow.
Description check ✅ Passed The description matches the change by explaining the new validation restriction and when scope: flow is supported.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@qmonnet qmonnet linked an issue Jul 21, 2026 that may be closed by this pull request

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between fcdf254 and 2e7d576.

📒 Files selected for processing (3)
  • acl-filter/src/tests.rs
  • config/src/converters/k8s/config/acl.rs
  • config/src/external/overlay/acl.rs

Comment thread config/src/external/overlay/acl.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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: flow validation during ACL rule validation, with accompanying unit tests in the config model.
  • Update k8s ACL converter test helpers to plumb the CRD scope field through rule construction.
  • Adjust acl-filter semantic 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.

Comment thread config/src/external/overlay/acl.rs
Comment thread config/src/converters/k8s/config/acl.rs Outdated
Comment thread config/src/converters/k8s/config/acl.rs Outdated
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>
Copilot AI review requested due to automatic review settings July 21, 2026 19:49
@qmonnet
qmonnet force-pushed the pr/qmonnet/acl-validate-scope branch from 2e7d576 to d13384d Compare July 21, 2026 19:49

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
config/src/external/overlay/acl.rs (1)

1094-1106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2e7d576 and d13384d.

📒 Files selected for processing (3)
  • acl-filter/src/tests.rs
  • config/src/converters/k8s/config/acl.rs
  • config/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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/acl Related to ACLs (Access Control Lists)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User ACL: Polish validation (scope: flow)

2 participants