Feature/99 use allow multiple times#126
Conversation
feat: support use allow* multiple times in env, flag and docker labels
WalkthroughThe PR adds multi-value allowlist support: flags, environment variables, and Docker labels can supply multiple regexes per HTTP method. Config types and parsing were changed to collect and compile slices of regexes; request handling now checks any-match against those slices. README and changelog updated for v1.12.0. ChangesAllowlist multi-value support
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Flags
participant ENV as Environment Variables
participant Labels as Docker Labels
participant Config as Configuration System
participant Regexes as Compiled Regexes
participant Handler as Request Handler
CLI->>Config: Provide repeated -allow* values
ENV->>Config: getAllowFromEnv() returns SP_ALLOW_* map
Labels->>Config: extractLabelData() returns label-derived regex lists
Config->>Config: Merge sources per HTTP method (params preferred)
Config->>Regexes: Compile and store []*regexp.Regexp per method
Handler->>Regexes: matchURL(path, []*regexp.Regexp)
Regexes-->>Handler: true/false (any match)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Signed-off-by: Wolfgang Ellsässer <git2026@wollomatic.dev>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config_test.go`:
- Around line 72-97: The test is flaky due to relying on map iteration order
when comparing regex slices; update regexMapsEqual to perform an
order-insensitive comparison: for each entry in map a (method key), fetch
corresponding bRegexes from map b, convert both aRegexes and bRegexes to slices
of strings via ar.String(), sort both string slices (e.g., using sort.Strings),
then compare lengths and each element for equality; keep the function name
regexMapsEqual and ensure extractLabelData tests remain unchanged so the
comparison no longer depends on element ordering.
In `@internal/config/config.go`:
- Around line 164-171: The current merge adds environment regexes into each
mr[i].regexStrings before flag parsing, causing env values to persist even when
a CLI -allow<METHOD> was provided; update the merge logic in the
getAllowFromEnv/initialization block so that for each mr entry (mr[i].method)
you only append env param{value:v, from:fromEnv} if there are no CLI-supplied
values for that method, or alternatively mark env values and let the compile
loop prefer params with source CLI over fromEnv and discard env entries when CLI
entries exist; adjust handling around getAllowFromEnv, mr[i].regexStrings, param
and fromEnv so CLI flags take precedence per method.
- Around line 649-655: Extract the method token from allowSpec using strings.Cut
(as done later) and compare that token exactly against rx.method instead of
using strings.HasPrefix; allow either the exact token (allowSpec == method) or a
dot-suffix (strings.HasPrefix(allowSpec, method+".")). Update the ContainsFunc
predicate that currently uses strings.HasPrefix to first do method, _, _ :=
strings.Cut(allowSpec, ".") and then return method == rx.method, before calling
compileRegexp(labelValue, method, "docker container label") so only true HTTP
methods are accepted and labels like "socket-proxy.allow.getextra" no longer
match.
In `@README.md`:
- Around line 96-101: Update the README examples so repeated-allow env vars are
shown as separate, copy-pasteable shell assignments instead of a single quoted
token; specifically split the ambiguous example into distinct lines like
SP_ALLOW_GET="/v1\..{1,2}/(version|containers/.*|events.*)" and
SP_ALLOW_HEAD=".*" and present the repeated-GET example as two separate
assignments (e.g., SP_ALLOW_GET="/version" and SP_ALLOW_GET_2="/_ping") so users
can copy each variable directly; adjust surrounding text to make clear
SP_ALLOW_GET and SP_ALLOW_GET_2 are separate env vars.
🪄 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: 0a266871-4158-4bce-b97c-4742328eba09
📒 Files selected for processing (7)
README.mdcmd/socket-proxy/handlehttprequest.gointernal/config/config.gointernal/config/config_test.gointernal/config/env.gointernal/config/env_test.gointernal/config/param.go
| if !regexMapsEqual(got, tt.want) { | ||
| t.Errorf("extractLabelData() = %v, want %v", got, tt.want) | ||
| } | ||
| if !reflect.DeepEqual(got2, tt.want2) { | ||
| t.Errorf("extractLabelData() = %v, want %v", got2, tt.want2) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool { | ||
| if len(a) != len(b) { | ||
| return false | ||
| } | ||
| for method, aRegexes := range a { | ||
| bRegexes, ok := b[method] | ||
| if !ok || len(aRegexes) != len(bRegexes) { | ||
| return false | ||
| } | ||
| for i, ar := range aRegexes { | ||
| if ar.String() != bRegexes[i].String() { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go specification range over maps iteration order unspecified
💡 Result:
Yes, the Go language specification states that the iteration order over maps using range is unspecified. Maps are defined as unordered groups of elements. The official Go blog "Go maps in action" explicitly confirms: "When iterating over a map with a range loop, the iteration order is not specified and is not guaranteed to be the same from one iteration to the next." Since Go 1.0, the runtime randomizes the iteration order to prevent reliance on any specific order, which could lead to portability issues or bugs if maps are rehashed.
Citations:
- 1: https://go.dev/blog/maps
- 2: https://go.dev/ref/spec
- 3: https://stackoverflow.com/questions/55925822/why-are-iterations-over-maps-random
🏁 Script executed:
# First, find the extractLabelData function
rg -A 20 "func extractLabelData" internal/config/config.goRepository: wollomatic/socket-proxy
Length of output: 1092
🏁 Script executed:
# Also examine the test to see how it sets up expected values
rg -B 10 "Test_extractLabelData" internal/config/config_test.goRepository: wollomatic/socket-proxy
Length of output: 244
🏁 Script executed:
# Check what container.Summary.Labels is
rg -A 5 "type.*Labels" internal/Repository: wollomatic/socket-proxy
Length of output: 49
🏁 Script executed:
# Get more context on the Test_extractLabelData function and the test cases
rg -A 50 "func Test_extractLabelData" internal/config/config_test.go | head -70Repository: wollomatic/socket-proxy
Length of output: 1386
🏁 Script executed:
# Search for the test table definition to see how tt.want is constructed
rg "want.*map\[string\]" internal/config/config_test.go -B 5 -A 5Repository: wollomatic/socket-proxy
Length of output: 995
🏁 Script executed:
# Check if want values are hardcoded or constructed dynamically
rg "tt\.want\s*=" internal/config/config_test.go -B 3 -A 3Repository: wollomatic/socket-proxy
Length of output: 49
🏁 Script executed:
# Check the full extractLabelData function to see if there's any sorting
rg -A 40 "func extractLabelData" internal/config/config.goRepository: wollomatic/socket-proxy
Length of output: 1186
🏁 Script executed:
# Check if there are any other tests or assertions that might handle ordering
rg "regexMapsEqual\|reflect.DeepEqual" internal/config/config_test.go -B 3 -A 1Repository: wollomatic/socket-proxy
Length of output: 49
Fix test flakiness caused by unspecified map iteration order.
The test input uses a map for Docker labels, which has unspecified iteration order in Go. The extractLabelData function iterates over cntr.Labels and appends regexes to slices in the order encountered. With positional comparison in regexMapsEqual, the test can fail randomly when labels are iterated in a different order than expected—for example, "socket-proxy.allow.get.1" before "socket-proxy.allow.get.0" would produce regexes in a different sequence.
Replace the positional comparison with order-insensitive comparison by sorting regex strings before comparing.
🧪 Proposed order-insensitive comparison
import (
"reflect"
"regexp"
+ "slices"
"testing"
"github.com/wollomatic/socket-proxy/internal/docker/api/types/container"
)
@@
func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool {
if len(a) != len(b) {
return false
}
for method, aRegexes := range a {
bRegexes, ok := b[method]
if !ok || len(aRegexes) != len(bRegexes) {
return false
}
- for i, ar := range aRegexes {
- if ar.String() != bRegexes[i].String() {
- return false
- }
+ aStrings := regexStrings(aRegexes)
+ bStrings := regexStrings(bRegexes)
+ slices.Sort(aStrings)
+ slices.Sort(bStrings)
+ if !slices.Equal(aStrings, bStrings) {
+ return false
}
}
return true
}
+
+func regexStrings(regexes []*regexp.Regexp) []string {
+ values := make([]string, 0, len(regexes))
+ for _, rx := range regexes {
+ values = append(values, rx.String())
+ }
+ return values
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !regexMapsEqual(got, tt.want) { | |
| t.Errorf("extractLabelData() = %v, want %v", got, tt.want) | |
| } | |
| if !reflect.DeepEqual(got2, tt.want2) { | |
| t.Errorf("extractLabelData() = %v, want %v", got2, tt.want2) | |
| } | |
| }) | |
| } | |
| } | |
| func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool { | |
| if len(a) != len(b) { | |
| return false | |
| } | |
| for method, aRegexes := range a { | |
| bRegexes, ok := b[method] | |
| if !ok || len(aRegexes) != len(bRegexes) { | |
| return false | |
| } | |
| for i, ar := range aRegexes { | |
| if ar.String() != bRegexes[i].String() { | |
| return false | |
| } | |
| } | |
| } | |
| return true | |
| func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool { | |
| if len(a) != len(b) { | |
| return false | |
| } | |
| for method, aRegexes := range a { | |
| bRegexes, ok := b[method] | |
| if !ok || len(aRegexes) != len(bRegexes) { | |
| return false | |
| } | |
| aStrings := regexStrings(aRegexes) | |
| bStrings := regexStrings(bRegexes) | |
| slices.Sort(aStrings) | |
| slices.Sort(bStrings) | |
| if !slices.Equal(aStrings, bStrings) { | |
| return false | |
| } | |
| } | |
| return true | |
| } | |
| func regexStrings(regexes []*regexp.Regexp) []string { | |
| values := make([]string, 0, len(regexes)) | |
| for _, rx := range regexes { | |
| values = append(values, rx.String()) | |
| } | |
| return values | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config_test.go` around lines 72 - 97, The test is flaky due
to relying on map iteration order when comparing regex slices; update
regexMapsEqual to perform an order-insensitive comparison: for each entry in map
a (method key), fetch corresponding bRegexes from map b, convert both aRegexes
and bRegexes to slices of strings via ar.String(), sort both string slices
(e.g., using sort.Strings), then compare lengths and each element for equality;
keep the function name regexMapsEqual and ensure extractLabelData tests remain
unchanged so the comparison no longer depends on element ordering.
| if slices.ContainsFunc(mr, func(rx methodRegex) bool { | ||
| // allowSpec starts with the method name like socket-proxy.allow.get.1 | ||
| return strings.HasPrefix(allowSpec, rx.method) | ||
| }) { | ||
| // extract the method name from allowSpec | ||
| method, _, _ := strings.Cut(allowSpec, ".") | ||
| r, err := compileRegexp(labelValue, method, "docker container label") |
There was a problem hiding this comment.
Require an exact method label or method-plus-dot suffix.
HasPrefix also accepts labels such as socket-proxy.allow.getextra, producing a GETEXTRA allow entry. Parse the method first, then compare it exactly against supported HTTP methods.
🛡️ Proposed stricter label parsing
- if slices.ContainsFunc(mr, func(rx methodRegex) bool {
- // allowSpec starts with the method name like socket-proxy.allow.get.1
- return strings.HasPrefix(allowSpec, rx.method)
- }) {
- // extract the method name from allowSpec
- method, _, _ := strings.Cut(allowSpec, ".")
+ method, _, _ := strings.Cut(allowSpec, ".")
+ if slices.ContainsFunc(mr, func(rx methodRegex) bool {
+ return method == rx.method
+ }) {
r, err := compileRegexp(labelValue, method, "docker container label")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config.go` around lines 649 - 655, Extract the method token
from allowSpec using strings.Cut (as done later) and compare that token exactly
against rx.method instead of using strings.HasPrefix; allow either the exact
token (allowSpec == method) or a dot-suffix (strings.HasPrefix(allowSpec,
method+".")). Update the ContainsFunc predicate that currently uses
strings.HasPrefix to first do method, _, _ := strings.Cut(allowSpec, ".") and
then return method == rx.method, before calling compileRegexp(labelValue,
method, "docker container label") so only true HTTP methods are accepted and
labels like "socket-proxy.allow.getextra" no longer match.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (1)
96-96:⚠️ Potential issue | 🟡 MinorRepeated-allow examples are still not copy-pasteable and have grammar issues.
Line 96 wraps two CLI flags inside a single quoted shell token, and line 101 wraps two env-var assignments in one quoted string — neither will work if pasted into a shell as-is. The phrase "use allow
GETmultiple times" is also ungrammatical (per LanguageTool: should be "allows using" or similar).📝 Proposed documentation tweak
-+ `'-allowGET=/version -allowGET=/_ping'` use allow `GET` multiple times ++ `'-allowGET=/version' '-allowGET=/_ping'` allows using `GET` multiple times-+ `'SP_ALLOW_GET="/version" SP_ALLOW_GET_2="/_ping"'` use allow `GET` multiple times ++ `SP_ALLOW_GET=/version` and `SP_ALLOW_GET_2=/_ping` allow using `GET` multiple timesAlso applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 96, Update the README examples so the CLI and env-var snippets are copy-pasteable and fix the grammar: split the combined quoted token `'-allowGET=/version -allowGET=/_ping'` into two separate flags (e.g. `-allowGET=/version -allowGET=/_ping` unquoted or each as its own quoted token) and similarly separate the combined env assignment on line with `ALLOW_GET` so each assignment is standalone; also replace the phrase "use allow `GET` multiple times" with grammatically correct wording such as "allows using `GET` multiple times" or "use `-allowGET` multiple times" so readers can paste examples directly into a shell and read a correct sentence.internal/config/config.go (1)
690-703:⚠️ Potential issue | 🟡 MinorLabel prefix matching still accepts non-method labels (e.g.
getextra).
strings.HasPrefix(allowSpec, method)matches labels such assocket-proxy.allow.getextra(allowSpecGETEXTRA) becauseHasPrefix("GETEXTRA", "GET")is true. Thenstrings.Cut("GETEXTRA", ".")returns"GETEXTRA"as the method (no dot present), and an entryallowedRequests["GETEXTRA"]is silently created. This entry will never match real requests, but the misconfiguration is accepted instead of being rejected/ignored, masking user errors.Parse the method first, then compare exactly against the supported HTTP methods (allowing only the exact name or a
.<suffix>form).🛡️ Proposed stricter label parsing
- if slices.ContainsFunc(supportedHTTPMethods, func(method string) bool { - // allowSpec starts with the method name like socket-proxy.allow.get.1 - return strings.HasPrefix(allowSpec, method) - }) { - // extract the method name from allowSpec - method, _, _ := strings.Cut(allowSpec, ".") - r, err := compileRegexp(labelValue, method, "docker container label") + method, suffix, hasSuffix := strings.Cut(allowSpec, ".") + if slices.Contains(supportedHTTPMethods, method) { + _ = suffix + _ = hasSuffix + r, err := compileRegexp(labelValue, method, "docker container label")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 690 - 703, The label parsing currently accepts prefixes like GETEXTRA because it checks HasPrefix against supportedHTTPMethods; instead change the logic in the loop that builds allowSpec to first split allowSpec into method and remainder using strings.Cut (or strings.SplitN) e.g. method, rest, _ := strings.Cut(allowSpec, "."), normalize method (strings.ToUpper) and then verify the method is exactly one of supportedHTTPMethods (use slices.Contains(supportedHTTPMethods, method)); only if that exact match succeeds (allowing rest to be empty or present) proceed to call compileRegexp(labelValue, method, "docker container label") and append to allowedRequests[method]; otherwise skip/ignore the label to avoid creating invalid keys like "GETEXTRA".
🧹 Nitpick comments (4)
cmd/socket-proxy/handlehttprequest.go (2)
44-51: LGTM — optional: short-circuit on first match is already there.
matchURLcorrectly returns on the first matching regex, so the average cost is bounded by the position of the matching pattern. For very large per-method allowlists, you could combine the regexes into a single alternation (^(?:p1|p2|...)$) at compile time inconfig.goto avoid the per-request slice scan, but for typical usage this is a non-issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/socket-proxy/handlehttprequest.go` around lines 44 - 51, matchURL currently iterates over a slice of regexes and short-circuits on first match; to optimize for very large per-method allowlists, modify the configuration compilation (e.g., in config.go) to combine allowed patterns into a single alternation regex per method (^(?:p1|p2|...)$) at startup, expose those combined *regexp.Regexp* instances (e.g., allowedURIsCombined) and update matchURL (or callers) to use the single compiled regex (e.g., call allowedURIsCombined.MatchString(requestURI)) instead of scanning a slice; ensure existing callers to matchURL or the allowlist loader are updated to accept the combined-regex representation and that error handling/logging remains when compilation fails.
23-31: Defensive check: handle empty slice as "no regexes".If
AllowedRequests[r.Method]ever ended up containing an empty[]*regexp.Regexp(e.g. a future code path that pre-populates the map without appending), theexistsbranch would pass andmatchURLwould correctly fall through to "path not allowed". That is benign today (the only writers inconfig.goonly append after a successful compile, so an empty slice is never stored), but it's worth a brief assertion or comment so the invariant is not silently broken by future refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/socket-proxy/handlehttprequest.go` around lines 23 - 31, The code assumes allowList.AllowedRequests[r.Method] is non-empty when present; add a defensive check after retrieving allowed to treat an empty []*regexp.Regexp as "no regexes" (not allowed): if len(allowed) == 0 call communicateBlockedRequest(w, r, "path not allowed", http.StatusForbidden) and return (or optionally log/assert) before calling matchURL; reference allowList.AllowedRequests, r.Method, matchURL, and communicateBlockedRequest when making this change.internal/config/config.go (1)
568-570: Minor: plain-text print of multiple regexes is hard to read.
AllowedRequestsis nowmap[string][]*regexp.Regexp. Printing the slice via%swill render it as[pat1 pat2 ...]on a single line, which loses the previous one-row-per-method clarity (especially for users with many regexes per method). Consider iterating the slice and printing one row per regex, e.g.:♻️ Proposed change
- for method, regex := range allowList.AllowedRequests { - fmt.Printf(" %-8s %s\n", method, regex) - } + for method, regexes := range allowList.AllowedRequests { + for _, r := range regexes { + fmt.Printf(" %-8s %s\n", method, r) + } + }The same applies to the JSON path on lines 547–558, where
regexis now a slice and would be logged as a list — acceptable, but an explicit per-regex loop would keep log entries one-per-pattern and be easier to grep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 568 - 570, The current loop over allowList.AllowedRequests assumes each map value is a single regex and prints the slice with %s, producing a compressed, hard-to-read single-line list; update the loop that iterates "for method, regex := range allowList.AllowedRequests" to treat regex as a slice (e.g., []*regexp.Regexp) and iterate its elements, printing one fmt.Printf row per individual pattern (include method and the individual regexp.String() or pattern) so each pattern appears on its own line; also apply the same per-regex iteration and one-row-per-pattern printing to the JSON path logging block referenced around the earlier lines (the block that currently prints the slice as a list) to preserve one-pattern-per-line clarity for grepability.README.md (1)
279-279: Optional: tighten the changelog entry wording."support use of allow* multiple times" reads awkwardly. Consider rewording for consistency with the rest of the changelog.
📝 Suggested wording
-1.12 - support use of allow* multiple times in env, flag and docker labels (thanks [`@qianlongzt`](https://github.com/qianlongzt)) +1.12 - support specifying `-allow*` multiple times via flags, environment variables, and Docker labels (thanks [`@qianlongzt`](https://github.com/qianlongzt))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 279, Reword the changelog entry "1.12 - support use of allow* multiple times in env, flag and docker labels" to match the project's changelog style and read more clearly; specifically, replace the awkward phrasing around "allow*" with something like "1.12 - support multiple occurrences of allow* in environment variables, flags, and Docker labels" (edit the line containing the "1.12" entry and the "allow*" text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 144: Update the README example label 'socket-proxy.allow.head.1=/exec' to
a clearer phrasing by changing the trailing comment to "allow HEAD `/exec`
requests to socket-proxy" (or similar concise wording) and add a short
parenthetical note calling out the '.1' prefix ambiguity so readers know it may
be required depending on the label parser; reference the label-parsing logic in
internal/config/config.go to verify whether the '.1' suffix is necessary and
adjust the example or wording accordingly.
---
Duplicate comments:
In `@internal/config/config.go`:
- Around line 690-703: The label parsing currently accepts prefixes like
GETEXTRA because it checks HasPrefix against supportedHTTPMethods; instead
change the logic in the loop that builds allowSpec to first split allowSpec into
method and remainder using strings.Cut (or strings.SplitN) e.g. method, rest, _
:= strings.Cut(allowSpec, "."), normalize method (strings.ToUpper) and then
verify the method is exactly one of supportedHTTPMethods (use
slices.Contains(supportedHTTPMethods, method)); only if that exact match
succeeds (allowing rest to be empty or present) proceed to call
compileRegexp(labelValue, method, "docker container label") and append to
allowedRequests[method]; otherwise skip/ignore the label to avoid creating
invalid keys like "GETEXTRA".
In `@README.md`:
- Line 96: Update the README examples so the CLI and env-var snippets are
copy-pasteable and fix the grammar: split the combined quoted token
`'-allowGET=/version -allowGET=/_ping'` into two separate flags (e.g.
`-allowGET=/version -allowGET=/_ping` unquoted or each as its own quoted token)
and similarly separate the combined env assignment on line with `ALLOW_GET` so
each assignment is standalone; also replace the phrase "use allow `GET` multiple
times" with grammatically correct wording such as "allows using `GET` multiple
times" or "use `-allowGET` multiple times" so readers can paste examples
directly into a shell and read a correct sentence.
---
Nitpick comments:
In `@cmd/socket-proxy/handlehttprequest.go`:
- Around line 44-51: matchURL currently iterates over a slice of regexes and
short-circuits on first match; to optimize for very large per-method allowlists,
modify the configuration compilation (e.g., in config.go) to combine allowed
patterns into a single alternation regex per method (^(?:p1|p2|...)$) at
startup, expose those combined *regexp.Regexp* instances (e.g.,
allowedURIsCombined) and update matchURL (or callers) to use the single compiled
regex (e.g., call allowedURIsCombined.MatchString(requestURI)) instead of
scanning a slice; ensure existing callers to matchURL or the allowlist loader
are updated to accept the combined-regex representation and that error
handling/logging remains when compilation fails.
- Around line 23-31: The code assumes allowList.AllowedRequests[r.Method] is
non-empty when present; add a defensive check after retrieving allowed to treat
an empty []*regexp.Regexp as "no regexes" (not allowed): if len(allowed) == 0
call communicateBlockedRequest(w, r, "path not allowed", http.StatusForbidden)
and return (or optionally log/assert) before calling matchURL; reference
allowList.AllowedRequests, r.Method, matchURL, and communicateBlockedRequest
when making this change.
In `@internal/config/config.go`:
- Around line 568-570: The current loop over allowList.AllowedRequests assumes
each map value is a single regex and prints the slice with %s, producing a
compressed, hard-to-read single-line list; update the loop that iterates "for
method, regex := range allowList.AllowedRequests" to treat regex as a slice
(e.g., []*regexp.Regexp) and iterate its elements, printing one fmt.Printf row
per individual pattern (include method and the individual regexp.String() or
pattern) so each pattern appears on its own line; also apply the same per-regex
iteration and one-row-per-pattern printing to the JSON path logging block
referenced around the earlier lines (the block that currently prints the slice
as a list) to preserve one-pattern-per-line clarity for grepability.
In `@README.md`:
- Line 279: Reword the changelog entry "1.12 - support use of allow* multiple
times in env, flag and docker labels" to match the project's changelog style and
read more clearly; specifically, replace the awkward phrasing around "allow*"
with something like "1.12 - support multiple occurrences of allow* in
environment variables, flags, and Docker labels" (edit the line containing the
"1.12" entry and the "allow*" text).
🪄 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: 8293ef6b-7264-40de-bd9c-8e93947b81e3
📒 Files selected for processing (4)
README.mdcmd/socket-proxy/handlehttprequest.gointernal/config/config.gointernal/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/config_test.go
| labels: | ||
| - 'socket-proxy.allow.get=.*' # allow all GET requests to socket-proxy | ||
| - 'socket-proxy.allow.head=/version' # HEAD `/version` requests to socket-proxy | ||
| - 'socket-proxy.allow.head.1=/exec' # another HEAD `exec` requests to socket-proxy |
There was a problem hiding this comment.
Minor: clarify the second HEAD label example.
The trailing comment "another HEAD exec requests to socket-proxy" is missing a verb and reads awkwardly. Consider tightening to something like "allow HEAD /exec requests to socket-proxy". Also note that the prefix in the second label uses .1; depending on the label parser's prefix matching this may or may not be required to disambiguate from the first one — see the related comment in internal/config/config.go.
📝 Proposed wording
- - 'socket-proxy.allow.head=/version' # HEAD `/version` requests to socket-proxy
- - 'socket-proxy.allow.head.1=/exec' # another HEAD `exec` requests to socket-proxy
+ - 'socket-proxy.allow.head=/version' # allow HEAD `/version` requests to socket-proxy
+ - 'socket-proxy.allow.head.1=/exec' # also allow HEAD `/exec` requests to socket-proxy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 144, Update the README example label
'socket-proxy.allow.head.1=/exec' to a clearer phrasing by changing the trailing
comment to "allow HEAD `/exec` requests to socket-proxy" (or similar concise
wording) and add a short parenthetical note calling out the '.1' prefix
ambiguity so readers know it may be required depending on the label parser;
reference the label-parsing logic in internal/config/config.go to verify whether
the '.1' suffix is necessary and adjust the example or wording accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (2)
143-144:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten the HEAD label comments for clarity.
Both inline comments are awkward; add an explicit verb and use
/execconsistently.📝 Suggested wording
- - 'socket-proxy.allow.head=/version' # HEAD `/version` requests to socket-proxy - - 'socket-proxy.allow.head.1=/exec' # another HEAD `exec` requests to socket-proxy + - 'socket-proxy.allow.head=/version' # allow HEAD `/version` requests to socket-proxy + - 'socket-proxy.allow.head.1=/exec' # also allow HEAD `/exec` requests to socket-proxy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 143 - 144, Update the inline comments for the labels 'socket-proxy.allow.head=/version' and 'socket-proxy.allow.head.1=/exec' to use an explicit verb and consistent path naming (use "/exec" consistently) — e.g., reword the comments to clearly state "allow HEAD requests to socket-proxy for /version" and "allow HEAD requests to socket-proxy for /exec" so the intent is unambiguous.
96-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the repeated
allowexamples copy-pasteable and fix grammar.The examples still read as a single token and “use allow” is ungrammatical. Please split repeated values into distinct assignments/flags and switch to “allows using”.
📝 Suggested wording
-+ `'-allowGET=/version -allowGET=/_ping'` use allow `GET` multiple times ++ `'-allowGET=/version' '-allowGET=/_ping'` allows using `GET` multiple times ... -+ `'SP_ALLOW_GET="/version" SP_ALLOW_GET_2="/_ping"'` use allow `GET` multiple times ++ `SP_ALLOW_GET="/version"` and `SP_ALLOW_GET_2="/_ping"` allow using `GET` multiple times🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 96 - 101, Update the examples so repeated allow entries are shown as separate, copy-pasteable assignments/flags and fix the grammar to "allows using"; specifically change the flag example `'-allowGET=/version -allowGET=/_ping'` to two separate flags (e.g., `-allowGET=/version -allowGET=/_ping` shown as distinct) or show them as separate env vars using `SP_ALLOW_GET="/version" SP_ALLOW_GET_2="/_ping"`, and update the sentence "use allow `GET` multiple times" to "allows using `GET` multiple times"; ensure examples for `SP_ALLOW_GET`, `SP_ALLOW_GET_2`, and `SP_ALLOW_HEAD` are each on their own assignment lines for copy/paste.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 279: Update the changelog entry for "1.12" to improve grammar and
consistency: change the sentence mentioning "allow*" to use plural "flags" and
clarify items, e.g., rewrite the line to read something like "1.12 - Support
using allow* multiple times in env vars, flags, and Docker labels" so it
references "1.12" and the "allow*" pattern and uses consistent plural forms for
env vars, flags, and Docker labels.
---
Duplicate comments:
In `@README.md`:
- Around line 143-144: Update the inline comments for the labels
'socket-proxy.allow.head=/version' and 'socket-proxy.allow.head.1=/exec' to use
an explicit verb and consistent path naming (use "/exec" consistently) — e.g.,
reword the comments to clearly state "allow HEAD requests to socket-proxy for
/version" and "allow HEAD requests to socket-proxy for /exec" so the intent is
unambiguous.
- Around line 96-101: Update the examples so repeated allow entries are shown as
separate, copy-pasteable assignments/flags and fix the grammar to "allows
using"; specifically change the flag example `'-allowGET=/version
-allowGET=/_ping'` to two separate flags (e.g., `-allowGET=/version
-allowGET=/_ping` shown as distinct) or show them as separate env vars using
`SP_ALLOW_GET="/version" SP_ALLOW_GET_2="/_ping"`, and update the sentence "use
allow `GET` multiple times" to "allows using `GET` multiple times"; ensure
examples for `SP_ALLOW_GET`, `SP_ALLOW_GET_2`, and `SP_ALLOW_HEAD` are each on
their own assignment lines for copy/paste.
🪄 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: 97708588-9fcb-4bba-b3f3-83e0b217f90e
📒 Files selected for processing (5)
README.mdinternal/config/env.gointernal/docker/api/types/events/events.gointernal/docker/api/types/filters/parse.gointernal/go-connections/sockets/sockets.go
💤 Files with no reviewable changes (3)
- internal/docker/api/types/filters/parse.go
- internal/docker/api/types/events/events.go
- internal/go-connections/sockets/sockets.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/env.go
|
|
||
| 1.11 - add per-container allowlists specified by Docker container labels (thanks [@amanda-wee](https://github.com/amanda-wee)) | ||
|
|
||
| 1.12 - support use of allow* multiple times in env, flag and docker labels (thanks [@qianlongzt](https://github.com/qianlongzt)) |
There was a problem hiding this comment.
Polish the changelog sentence for grammar and consistency.
The line is understandable, but “flag” should be plural and the phrasing can be cleaner.
📝 Suggested wording
-1.12 - support use of allow* multiple times in env, flag and docker labels (thanks [`@qianlongzt`](https://github.com/qianlongzt))
+1.12 - support using `allow*` multiple times via env vars, flags, and Docker labels (thanks [`@qianlongzt`](https://github.com/qianlongzt))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1.12 - support use of allow* multiple times in env, flag and docker labels (thanks [@qianlongzt](https://github.com/qianlongzt)) | |
| 1.12 - support using `allow*` multiple times via env vars, flags, and Docker labels (thanks [`@qianlongzt`](https://github.com/qianlongzt)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 279, Update the changelog entry for "1.12" to improve
grammar and consistency: change the sentence mentioning "allow*" to use plural
"flags" and clarify items, e.g., rewrite the line to read something like "1.12 -
Support using allow* multiple times in env vars, flags, and Docker labels" so it
references "1.12" and the "allow*" pattern and uses consistent plural forms for
env vars, flags, and Docker labels.
2 participants
Summary by CodeRabbit
New Features
-allow*entries per HTTP method via CLI flags, environment variables and Docker labels for finer allowlist control.Bug Fixes
Documentation
allow*across params, env vars and labels.Tests