[management, proxy] Add OR mode for reverse proxy access restrictions - #6887
Open
lixmal wants to merge 4 commits into
Open
[management, proxy] Add OR mode for reverse proxy access restrictions#6887lixmal wants to merge 4 commits into
lixmal wants to merge 4 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesAccess restriction matching
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ManagementAPI
participant ProxyServer
participant RestrictFilter
participant GeoLookup
participant CrowdSec
ManagementAPI->>ProxyServer: provide allow_match restriction
ProxyServer->>RestrictFilter: pass AllowMatch in FilterConfig
RestrictFilter->>RestrictFilter: normalize mode and check blocklists
RestrictFilter->>GeoLookup: resolve country when required
GeoLookup-->>RestrictFilter: country result or unavailable
RestrictFilter->>CrowdSec: evaluate IP after allow decision
CrowdSec-->>RestrictFilter: enforcement result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/internals/modules/reverseproxy/service/service.go (1)
825-829: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve
allow_matchwhen it is the only restriction.Both omission predicates ignore
r.AllowMatch, so an explicitallow_matchwithout CIDR/country/CrowdSec fields is silently omitted from both API and proto output.
management/internals/modules/reverseproxy/service/service.go#L825-L829: includer.AllowMatch == ""in the early-return condition.management/internals/modules/reverseproxy/service/service.go#L855-L859: apply the same condition to proto conversion.management/internals/modules/reverseproxy/service/service_test.go#L1305-L1338: add an allow-match-only conversion test asserting both outputs remain non-nil.Proposed fix
- r.CrowdSecMode == "" { + r.CrowdSecMode == "" && r.AllowMatch == "" { return nil }🤖 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 `@management/internals/modules/reverseproxy/service/service.go` around lines 825 - 829, Preserve an allow-match-only restriction in both conversion paths. In management/internals/modules/reverseproxy/service/service.go lines 825-829, update restrictionsToAPI to also require r.AllowMatch == "" before returning nil; apply the same condition in the proto conversion at lines 855-859. In management/internals/modules/reverseproxy/service/service_test.go lines 1305-1338, add a conversion test with only AllowMatch set and assert both API and proto outputs are non-nil.
🤖 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.
Outside diff comments:
In `@management/internals/modules/reverseproxy/service/service.go`:
- Around line 825-829: Preserve an allow-match-only restriction in both
conversion paths. In
management/internals/modules/reverseproxy/service/service.go lines 825-829,
update restrictionsToAPI to also require r.AllowMatch == "" before returning
nil; apply the same condition in the proto conversion at lines 855-859. In
management/internals/modules/reverseproxy/service/service_test.go lines
1305-1338, add a conversion test with only AllowMatch set and assert both API
and proto outputs are non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3824b8d-a0f1-4cb0-8205-1b4ccda989c6
⛔ Files ignored due to path filters (1)
shared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
management/internals/modules/reverseproxy/service/service.gomanagement/internals/modules/reverseproxy/service/service_test.goproxy/internal/restrict/restrict.goproxy/internal/restrict/restrict_test.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.proto
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
This was referenced Jul 24, 2026
Open
8 tasks
lixmal
force-pushed
the
reverse-proxy-allow-match-or
branch
from
August 19, 2026 10:05
f05f3fc to
2355d49
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Access restrictions on reverse-proxy services currently combine the different allowlists with AND, so a connection must match every configured allowlist. That makes rules like "allow country X OR CIDR Y" impossible to express, and adding a CIDR allowlist silently denies country-allowed clients (and vice versa). This adds an
allow_matchoption to control how the allowlists combine.allow_matchto access restrictions:all(default, AND) requires matching every allowlist,any(OR) requires matching at least one, e.g. an allowed country or an allowed CIDRallfor an empty or unknown value so behavior never loosens unexpectedlyallow_matchset (treated asall)Issue ticket number and link
#5862
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:
netbirdio/docs#884
Summary by CodeRabbit
allow_matchto access restrictions to control allowlist combination: all (AND) or any (OR), with all as default.allow_match.