feat: support use allow* multiple times in env, flag and docker labels - #86
Conversation
| > [!NOTE] | ||
| > Starting with version 1.6.0, the socket-proxy container image is also available on GHCR. | ||
| > Starting with version todo, the socket-proxy can set multiple times -allow* in params or environment of docker labels |
Uh oh!
There was an error while loading. Please reload this page.
@coderabbitai review the PR |
Tip For best results, initiate chat on the files or code changes.
Just let me know what you'd like me to do! |
Let me perform a comprehensive review of the code changes. ✅ Actions performedReview triggered.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces support for specifying multiple allow rules per HTTP method instead of requiring a single regex pattern. It modifies the configuration structure to store multiple compiled regular expressions per method, adds parsing helpers for environment variables and command-line parameters, and updates the request matching logic to iterate over all applicable patterns. Changes
Sequence DiagramsequenceDiagram
actor User
participant Parser as Config Parser
participant Env as Environment<br/>Parser
participant Params as Param<br/>Collector
participant Compiler as Regex<br/>Compiler
participant Config as AllowList<br/>Config
participant Handler as Request<br/>Handler
participant Matcher as URL<br/>Matcher
User->>Parser: Load configuration
activate Parser
Parser->>Env: Parse SP_ALLOW_* variables
activate Env
Env->>Env: Extract method & patterns<br/>(SP_ALLOW_GET, GET_1, etc.)
Env-->>Parser: map[method][]patterns
deactivate Env
Parser->>Params: Collect flag values<br/>(-allowGET, -allowGET, ...)
activate Params
Params->>Params: Aggregate by method
Params-->>Parser: map[method][]patterns
deactivate Params
Parser->>Compiler: Compile patterns to regex
activate Compiler
Compiler->>Compiler: For each method,<br/>compile all patterns
Compiler-->>Parser: map[method][]*Regexp
deactivate Compiler
Parser->>Config: Populate AllowedRequests
activate Config
Config->>Config: Store as slices per method
deactivate Config
Parser-->>User: Config ready
deactivate Parser
User->>Handler: Send HTTP request
activate Handler
Handler->>Matcher: Match request path
activate Matcher
Matcher->>Matcher: Get regex slice for method
Matcher->>Matcher: Iterate & test patterns
Matcher-->>Handler: Match found?
deactivate Matcher
Handler-->>User: Allow or deny request
deactivate Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
README.md (1)
30-30:⚠️ Potential issue | 🟡 MinorReplace the placeholder version and reword this note.
Line 30 still uses
todoand the sentence is unclear. Please replace it with the concrete release version and clearer wording (for example, “Starting with version X.Y.Z,-allow*can be specified multiple times via CLI flags, environment variables, and Docker labels.”).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 30, Replace the placeholder "todo" with the actual release version (e.g., X.Y.Z) and rewrite the sentence to be clearer: change the line containing "todo" and the phrase "-allow*" so it reads something like “Starting with version X.Y.Z, `-allow*` can be specified multiple times via CLI flags, environment variables, and Docker labels.” Ensure you update the exact string that currently contains "todo" and the `-allow*` token.
🧹 Nitpick comments (3)
README.md (1)
97-103: Tighten wording in the new repeated-allow examples.Line 97 and Line 102 read awkwardly (“allow use
GETmultiple times”). Suggest rephrasing to “GETcan be specified multiple times” for readability and consistency.Proposed docs wording tweak
-+ `'-allowGET=/version' '-allowGET=/_ping'` allow use `GET` multiple times++ `'-allowGET=/version' '-allowGET=/_ping'` shows that `GET` can be specified multiple times.-+ `'SP_ALLOW_GET="/version" SP_ALLOW_GET_2=/_ping'` allow use `GET` multiple times++ `'SP_ALLOW_GET="/version" SP_ALLOW_GET_2=/_ping'` shows that `GET` can be specified multiple times.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 97 - 103, Update the README examples wording that currently says "allow use `GET` multiple times" to a clearer phrase like "`GET` can be specified multiple times" wherever that exact text appears (e.g., adjacent to the quoted examples with `-allowGET=/version` / `-allowGET=/_ping` and the env examples `SP_ALLOW_GET="/v1\..{1,2}/(version|containers/.*|events.*)"` and `SP_ALLOW_GET="/version" SP_ALLOW_GET_2=/_ping"`); keep the surrounding example strings (`SP_ALLOW_GET`, `SP_ALLOW_HEAD`, and the quoted patterns) unchanged, only replace the awkward sentence to improve readability and consistency.internal/config/env.go (1)
7-7: Consider using camelCase for the unexported constant.Go convention prefers camelCase for unexported identifiers. Consider renaming
sp_allowPrefixtospAllowPrefix.♻️ Suggested change
-const sp_allowPrefix = "SP_ALLOW_"+const spAllowPrefix = "SP_ALLOW_"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/env.go` at line 7, The constant sp_allowPrefix uses snake_case but Go convention prefers camelCase for unexported identifiers; rename the identifier to spAllowPrefix (update its declaration and all references to sp_allowPrefix in this package, e.g., in internal/config/env.go) to follow Go naming conventions and keep it unexported.internal/config/config_test.go (1)
47-58: Consider explicitly settingwantErr: falsefor completeness.While the zero value for
boolisfalse, explicitly settingwantErr: falseimproves readability and makes the test expectation clearer.💡 Suggested change
{ name: "non-allow labels are ignored", cntr: container.Summary{ Labels: map[string]string{ "socket-proxy.allow.get": "regex1", "other.label": "value", }, }, want: map[string][]*regexp.Regexp{ "GET": {regexp.MustCompile("^regex1$")}, }, + want2: nil,+ wantErr: false, },🤖 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 47 - 58, Add an explicit wantErr: false to the table-driven test case named "non-allow labels are ignored" so the expectation is clear; locate the test case that builds container.Summary with Labels ("socket-proxy.allow.get": "regex1", "other.label": "value") and add the wantErr: false field alongside the existing want map (map[string][]*regexp.Regexp{"GET": {regexp.MustCompile("^regex1$")}}).
🤖 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-74: The test currently uses reflect.DeepEqual to compare maps
of *regexp.Regexp which is brittle; replace that comparison with a helper that
compares regex pattern strings instead (e.g., add a regexMapsEqual function that
iterates the map returned by extractLabelData and compares lengths and each
regexp's String() values), then use regexMapsEqual(got, tt.want) in place of
reflect.DeepEqual; ensure the helper handles nil/length mismatches and preserves
ordering when comparing slices for each key.
In `@internal/config/config.go`:
- Around line 164-173: Call getAllowFromEnv(os.Environ()) once before the loop
and reuse its returned map instead of invoking it inside each iteration;
specifically, compute allowFromEnv := getAllowFromEnv(os.Environ()) prior to
iterating over mr, then inside the for i := range mr loop use that allowFromEnv
to lookup values and append param{value: v, from: fromEnv} to mr[i].regexStrings
— this avoids repeated work and preserves existing behavior.
In `@internal/config/env_test.go`:
- Around line 25-34: The test cases in env_test.go define two table entries with
the identical name field "multiple entries for one method"; rename the second
test's name to a distinct string (e.g. "multiple entries for one method with _2"
or "multiple entries for one method (alt)") so each table-driven test case (the
name fields used in the test struct) is unique and test output can be
disambiguated when running the tests.
In `@README.md`:
- Line 243: The Markdown table row for `-shutdowngracetime` /
`SP_SHUTDOWNGRACETIME` has an extra trailing cell delimiter (`| |`) which
causes a column mismatch (MD056); edit the README table row containing
`-shutdowngracetime` and `SP_SHUTDOWNGRACETIME` to remove the extra trailing `| |` so the row has the same number of cells as the header and other rows.
---
Duplicate comments:
In `@README.md`:
- Line 30: Replace the placeholder "todo" with the actual release version (e.g.,
X.Y.Z) and rewrite the sentence to be clearer: change the line containing "todo"
and the phrase "-allow*" so it reads something like “Starting with version
X.Y.Z, `-allow*` can be specified multiple times via CLI flags, environment
variables, and Docker labels.” Ensure you update the exact string that currently
contains "todo" and the `-allow*` token.
---
Nitpick comments:
In `@internal/config/config_test.go`:
- Around line 47-58: Add an explicit wantErr: false to the table-driven test
case named "non-allow labels are ignored" so the expectation is clear; locate
the test case that builds container.Summary with Labels
("socket-proxy.allow.get": "regex1", "other.label": "value") and add the
wantErr: false field alongside the existing want map
(map[string][]*regexp.Regexp{"GET": {regexp.MustCompile("^regex1$")}}).
In `@internal/config/env.go`:
- Line 7: The constant sp_allowPrefix uses snake_case but Go convention prefers
camelCase for unexported identifiers; rename the identifier to spAllowPrefix
(update its declaration and all references to sp_allowPrefix in this package,
e.g., in internal/config/env.go) to follow Go naming conventions and keep it
unexported.
In `@README.md`:
- Around line 97-103: Update the README examples wording that currently says
"allow use `GET` multiple times" to a clearer phrase like "`GET` can be
specified multiple times" wherever that exact text appears (e.g., adjacent to
the quoted examples with `-allowGET=/version` / `-allowGET=/_ping` and the env
examples `SP_ALLOW_GET="/v1\..{1,2}/(version|containers/.*|events.*)"` and
`SP_ALLOW_GET="/version" SP_ALLOW_GET_2=/_ping"`); keep the surrounding example
strings (`SP_ALLOW_GET`, `SP_ALLOW_HEAD`, and the quoted patterns) unchanged,
only replace the awkward sentence to improve readability and consistency.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
qianlongzt
commented
Mar 1, 2026
@wollomatic I’ve fixed all the comments from CodeRabbit. Is this ready to merge? |
wollomatic
commented
Mar 1, 2026
Hi @qianlongzt, thank you very much! I have planned to check this later and to create a build for testing. |
efea0f9
into
wollomatic:feature/99-use-allow-multiple-timesUh oh!
There was an error while loading. Please reload this page.
wollomatic
commented
Mar 1, 2026
Container image to test: |
fix: #99
I test env, flag and docker labels
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests