Conversation
WalkthroughThis PR adds end-to-end ACL flag support: CLI ACL flags are extracted at startup, passed into the bootstrap/app, and injected into the AccessControlsService. ACL config is now assembled from environment and CLI flags (flags override env) using new utils and decoder helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Startup
participant Root as cmd/root.go
participant Bootstrap as bootstrap.NewBootstrapApp
participant ACLService as AccessControlsService
participant Utils as utils.GetACLsConfig
participant Decoders as Decoders
CLI->>Root: start with args
Root->>Root: ExtractACLFlags(os.Args[1:]) -> aclFlags
Root->>Bootstrap: NewBootstrapApp(conf, aclFlags)
Bootstrap->>ACLService: New + SetACLFlags(aclFlags)
ACLService->>Utils: GetACLsConfig(env, aclFlags)
Utils->>Decoders: DecodeACLEnv(env)
Utils->>Decoders: DecodeACLFlags(flags)
Utils->>ACLService: Merged ACL config (flags override env)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
nicotsx
force-pushed
the
refactor/acl-decoder
branch
from
a0fa45f to
9014be9
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #435 +/- ##
==========================================
- Coverage 23.15% 22.18% -0.98%
==========================================
Files 36 36
Lines 2237 2335 +98
==========================================
Hits 518 518
- Misses 1684 1782 +98
Partials 35 35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/access_controls_service.go (1)
29-41: Clean refactoring to centralized ACL loading.The delegation to
utils.GetACLsConfigsimplifies the initialization logic and enables proper merging of environment and flag-based configurations. The error handling is appropriately preserved.Consider renaming
envACLstoaclConfigoraclssince it now contains merged environment and flag-based configurations, not just environment variables:-envACLs config.Apps +aclConfig config.AppsAnd update references accordingly (lines 38, 44, 48-49).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/root.go(5 hunks)internal/bootstrap/app_bootstrap.go(2 hunks)internal/service/access_controls_service.go(1 hunks)internal/utils/app_utils.go(1 hunks)internal/utils/decoders/decoders.go(2 hunks)internal/utils/decoders/env_decoder.go(1 hunks)internal/utils/decoders/flags_decoder.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
internal/utils/decoders/env_decoder.go (2)
internal/utils/decoders/env_decoder_test.go (1)
TestDecodeEnv(11-37)internal/utils/decoders/flags_decoder_test.go (1)
TestDecodeFlags(11-37)
internal/utils/decoders/flags_decoder.go (1)
internal/utils/decoders/flags_decoder_test.go (1)
TestDecodeFlags(11-37)
internal/bootstrap/app_bootstrap.go (1)
internal/config/config.go (1)
Config(17-44)
internal/utils/app_utils.go (3)
internal/config/config.go (2)
Apps(125-127)App(129-136)internal/utils/decoders/env_decoder.go (1)
DecodeACLEnv(21-33)internal/utils/decoders/flags_decoder.go (1)
DecodeACLFlags(24-37)
internal/service/access_controls_service.go (3)
internal/service/docker_service.go (1)
DockerService(14-18)internal/config/config.go (1)
Apps(125-127)internal/utils/app_utils.go (1)
GetACLsConfig(212-245)
cmd/root.go (2)
internal/bootstrap/app_bootstrap.go (1)
NewBootstrapApp(45-50)internal/utils/app_utils.go (1)
ExtractACLFlags(247-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
internal/utils/decoders/flags_decoder.go (1)
24-37: LGTM!The
DecodeACLFlagsfunction correctly mirrors the existingDecodeFlagspattern while using ACL-specific normalization. The implementation properly filters flags, normalizes keys with the hyphen delimiter, and delegates to the parser.internal/utils/decoders/decoders.go (1)
132-171: LGTM!The
normalizeACLKeysfunction correctly filters ACL-prefixed keys and usesParsePathto resolve nested structures. The lowerCamelCase conversion ensures consistent key formatting for the parser.cmd/root.go (4)
17-29: LGTM!The addition of the
aclFlagsfield and its initialization in the constructor is clean and follows Go conventions. The explicit viper initialization also improves clarity.
37-39: Enabling unknown flags for dynamic ACL configuration.Setting
UnknownFlags: trueallows dynamically-named ACL flags (e.g.,--apps-myapp-config-domain) to pass through Cobra's validation. This is appropriate for the use case, though it means typos in standard flags won't be caught by Cobra.
124-124: LGTM!The ACL flags are properly wired into the bootstrap layer, enabling the configuration to flow through to the service layer.
134-135: LGTM!ACL flags are extracted at the appropriate time (before command registration) using
os.Args, ensuring they're available when the bootstrap process begins.internal/utils/decoders/env_decoder.go (1)
21-33: LGTM!The
DecodeACLEnvfunction correctly implements ACL-specific environment variable decoding, using the underscore delimiter appropriate for environment variables.internal/bootstrap/app_bootstrap.go (3)
39-43: LGTM!The addition of the
aclFlagsfield toBootstrapAppis clean and follows the existing struct pattern.
45-50: LGTM!The constructor correctly accepts and initializes the
aclFlagsparameter, maintaining consistency with the existing initialization pattern.
145-145: LGTM!The ACL flags are properly wired to the service before initialization, ensuring they're available when
Init()processes the configuration.internal/service/access_controls_service.go (2)
12-23: LGTM!The addition of the
aclFlagsfield and its initialization in the constructor is clean and consistent with the service design pattern.
25-27: LGTM!The
SetACLFlagssetter is straightforward and enables external configuration of ACL flags before initialization.internal/utils/app_utils.go (2)
212-245: LGTM!The
GetACLsConfigfunction correctly merges ACL configurations from environment variables and command-line flags, with flags taking precedence. The implementation follows the established pattern fromGetOAuthProvidersConfigand includes proper nil checks.
247-260: LGTM!The
ExtractACLFlagsfunction correctly filters ACL-related command-line flags by prefix and extracts key-value pairs. The use ofSplitNwith limit 2 properly handles values containing "=" characters.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/utils/decoders/decoders.go (1)
174-188: Filter empty string segments from ParsePath output (past review concern not addressed).As flagged in the previous review,
ParsePathcan return empty strings when processing map elements or when input contains empty segments. These empty strings pass through unfiltered and create malformed keys like"tinyauth.acl..field"at line 180-188, which will likely cause parser errors or silent config misapplication.The check at line 176 only verifies that
parsedPartsis not completely empty—it doesn't remove empty string elements within the slice.Apply this diff to filter out empty segments after calling
ParsePath:parsedParts := ParsePath(parts[startIdx:], 0, rootType) if len(parsedParts) == 0 { continue } + + // Filter out empty strings to prevent malformed keys + filtered := make([]string, 0, len(parsedParts)) + for _, part := range parsedParts { + if part != "" { + filtered = append(filtered, part) + } + } + + if len(filtered) == 0 { + continue + } final := "tinyauth." + root - for _, part := range parsedParts { + for _, part := range filtered { if strings.Contains(part, "_") { final += "." + part } else { final += "." + strcase.LowerCamelCase(part) } } normalized[final] = v
🧹 Nitpick comments (2)
internal/utils/decoders/decoders.go (2)
11-19: Remove duplicate bounds check.The bounds check
if idx >= len(parts)is performed twice: once at lines 11-13 (entry point) and again at lines 17-19 (inside the Map handling block). The second check at lines 17-19 is redundant since the first check already guaranteesidx < len(parts)when execution reaches line 15.Apply this diff to remove the duplicate check:
if t.Kind() == reflect.Map { - - if idx >= len(parts) { - return []string{} - } - elemType := t.Elem()
52-69: Document the two-pass struct field search logic.The function searches struct fields in two passes: first for Map types (lines 52-60), then for matching field names (lines 62-69). This prioritization means that if a struct has both a Map field and a regular field matching
parts[idx], the Map will be preferred. This behavior is non-obvious and could lead to confusion during maintenance.Add a brief comment before line 52 explaining the search order:
if t.Kind() == reflect.Struct { + // First pass: check for Map fields that can consume remaining parts for i := 0; i < t.NumField(); i++ {And before line 62:
+ // Second pass: find direct field name matches for i := 0; i < t.NumField(); i++ {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/utils/decoders/decoders.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/utils/decoders/decoders.go (1)
173-173: Review comment is incorrect—the boundary condition cannot occur.The original concern assumes
startIdx == len(parts)is possible, but the preceding logic makes this impossible:
- Initial constraint (line 150):
len(parts) >= 2always holds- Case 1 (tinyauth, line 161):
len(parts) >= 3is mandatory, andstartIdx = 2, so2 < len(parts)is always true- Case 2 (root, line 167):
len(parts) >= 2from initial check, andstartIdx = 1, so1 < len(parts)is always trueThe check at line 173 is therefore always true and is defensive programming, not a bug. ParsePath also safely handles empty slices by returning
[]string{}ifidx >= len(parts).Likely an incorrect or invalid review comment.
| keyParts := parts[idx:keyEndIdx] | ||
| keyName := strings.ToLower(strings.Join(keyParts, "_")) |
There was a problem hiding this comment.
🧩 Analysis chain
Validate input parts to prevent empty string segments.
If parts contains empty strings (e.g., from splitting "apps--field" by "-"), then keyName at line 44 can become an empty string when keyParts contains only empty elements. This empty string propagates into the result array and later causes malformed keys like "tinyauth.acl..field" in normalizeACLKeys.
While the past review suggested filtering in normalizeACLKeys, it's more robust to prevent empty strings at the source.
Consider one of these approaches:
Option 1: Filter empty strings from keyParts before joining:
keyParts := parts[idx:keyEndIdx]
+ filtered := make([]string, 0, len(keyParts))
+ for _, part := range keyParts {
+ if part != "" {
+ filtered = append(filtered, part)
+ }
+ }
+ if len(filtered) == 0 {
+ return []string{}
+ }
- keyName := strings.ToLower(strings.Join(keyParts, "_"))
+ keyName := strings.ToLower(strings.Join(filtered, "_"))Option 2: Validate at entry that parts contains no empty strings (add after line 11):
if idx >= len(parts) {
return []string{}
}
+ // Skip empty parts
+ for idx < len(parts) && parts[idx] == "" {
+ idx++
+ }
+ if idx >= len(parts) {
+ return []string{}
+ }Run this script to check if there are existing test cases or usage patterns that might produce empty string segments:
🏁 Script executed:
#!/bin/bash
# Search for Split calls that might produce empty strings
rg -nP --type=go 'strings\.Split.*["\047]-["\047]'
# Search for how parts are constructed before calling ParsePath
rg -nP --type=go -B5 'ParsePath\(' | grep -E 'Split|parts.*='Length of output: 79
🏁 Script executed:
#!/bin/bash
# Examine the decoders.go file around lines 43-44
echo "=== Content of decoders.go lines 35-55 ==="
sed -n '35,55p' internal/utils/decoders/decoders.go
# Show the full ParsePath function signature and initial logic
echo -e "\n=== ParsePath function signature and initial lines ==="
sed -n '1,100p' internal/utils/decoders/decoders.go | tail -50
# Find how ParsePath is called
echo -e "\n=== All ParsePath call sites ==="
rg -nP --type=go 'ParsePath\(' -B3 -A1Length of output: 3425
🏁 Script executed:
#!/bin/bash
# Get the full normalizeKeys function to see how parts array is built
echo "=== normalizeKeys function (lines 88-175) ==="
sed -n '88,175p' internal/utils/decoders/decoders.go
# Also check for any test cases that might show expected behavior
echo -e "\n=== Test files for decoders ==="
find . -name '*decoder*test*.go' -o -name '*test*decoder*.go' | head -10
# Look for test cases that call ParsePath or normalizeKeys
echo -e "\n=== Test content ==="
fd -e 'go' -path '*/test*' | xargs grep -l 'ParsePath\|normalizeKeys' 2>/dev/null | head -5Length of output: 2080
🏁 Script executed:
#!/bin/bash
# Look at test files to see if edge cases with empty strings or double delimiters are tested
echo "=== Test file list ==="
ls -la internal/utils/decoders/*test.go
echo -e "\n=== Content of env_decoder_test.go ==="
cat internal/utils/decoders/env_decoder_test.go | head -100
echo -e "\n=== Search for test cases with double delimiters or consecutive separators ==="
grep -rn '\-\-\|__' internal/utils/decoders/*test.go
echo -e "\n=== Search for any validation of parts array in ParsePath ==="
sed -n '10,50p' internal/utils/decoders/decoders.goLength of output: 2657
Filter empty string segments from keyParts before joining at line 44.
The concern is valid. Go's strings.Split() produces empty strings when consecutive delimiters exist (e.g., splitting "tinyauth__apps" by "_" yields ["tinyauth", "", "apps"]). This empty string can propagate into keyParts at line 43, causing strings.Join(keyParts, "_") to produce malformed keys like "_field" or "". The current code has no validation preventing this.
Implement Option 1 from the original suggestion—filter empty strings before joining:
keyParts := parts[idx:keyEndIdx]
+ filtered := make([]string, 0, len(keyParts))
+ for _, part := range keyParts {
+ if part != "" {
+ filtered = append(filtered, part)
+ }
+ }
+ if len(filtered) == 0 {
+ return []string{}
+ }
- keyName := strings.ToLower(strings.Join(keyParts, "_"))
+ keyName := strings.ToLower(strings.Join(filtered, "_"))This handles the issue at the source where keys are constructed, preventing malformed output downstream.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/utils/decoders/decoders.go around lines 43 to 44, keyParts may
contain empty strings from strings.Split which leads to malformed keys when
joined; filter out any empty string elements from keyParts before calling
strings.Join so the resulting keyName contains only non-empty segments (e.g.,
build a new slice, append only parts with len>0, then join with "_"), ensuring
empty segments are removed at the source before creating keyName.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } | ||
|
|
||
| app := bootstrap.NewBootstrapApp(conf) | ||
| app := bootstrap.NewBootstrapApp(conf, c.aclFlags) |
There was a problem hiding this comment.
The flags can be parsed directly here avoiding the need to store them in the rootCmd struct. Since we are fetching directly from os.Args, cobra will not touch them so nothing to worry about.
| // Create services | ||
| dockerService := service.NewDockerService() | ||
| aclsService := service.NewAccessControlsService(dockerService) | ||
| aclsService.SetACLFlags(app.aclFlags) |
There was a problem hiding this comment.
Why use a function and not pass them in the initialization function?
| for i := idx + 1; i < len(parts); i++ { | ||
| found := false | ||
|
|
||
| for j := 0; j < elemType.NumField(); j++ { |
There was a problem hiding this comment.
These loops can probably be replaced with:
for j := range elemType.NumField() {
...
}Also applies to lines 52 and 62.
| keyParts := parts[idx:keyEndIdx] | ||
| keyName := strings.ToLower(strings.Join(keyParts, "_")) |
|
Closing - This logic will be completely removed and Traefik's parser will be used in v5. |
2 participants
Implement a new recursive algorithm to parse ACL flags and environment variables and expand it.
Recursively navigates nested struct/map types using reflection to normalize flat key paths into hierarchical structures.
Allows stuff like:
Summary by CodeRabbit
--apps-/--tinyauth-apps-) as an alternative to environment variables.