feat: non-docker acls#549
Conversation
📝 WalkthroughWalkthroughThe PR adds the traefik/paerser submodule with a local go.mod replace, applies a parser patch, updates CI to init submodules and apply the patch, restructures config to nested app-based ACLs, and extends AccessControlsService to consult static app ACLs before falling back to Docker label-based ACLs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant AccessControls as AccessControlsService
participant Config
participant Docker as DockerService
Note over Client,Proxy: Incoming request for domain
Client->>Proxy: request(domain)
Proxy->>AccessControls: GetAccessControls(domain)
AccessControls->>Config: lookupStaticACLs(domain)
alt Static ACL found
Config-->>AccessControls: App (static ACL)
AccessControls-->>Proxy: return static App ACL
else Static ACL not found
Config-->>AccessControls: not found
AccessControls->>Docker: query labels by domain
Docker-->>AccessControls: labels -> App (derived)
AccessControls-->>Proxy: return App ACL from labels
end
Proxy-->>Client: enforce/access response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #549 +/- ##
==========================================
- Coverage 20.22% 20.08% -0.15%
==========================================
Files 37 37
Lines 2151 2166 +15
==========================================
Hits 435 435
- Misses 1686 1701 +15
Partials 30 30 ☔ 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 (2)
paerser (1)
1-1: Verify the paerser submodule commit is from a stable release.The submodule pins to a specific commit hash. Please confirm this commit exists in the upstream traefik/paerser repository and is preferably from a tagged release for reproducibility and maintenance.
Per the AI summary, the CI workflows apply a
patches/nested_maps.diffpatch to this submodule. Ensure the patch:
- Is compatible with this specific commit hash
- Is version-controlled and documented (e.g., in CONTRIBUTING.md or a patch changelog)
- Is consistently applied across all build contexts (CI, Docker builds, local development)
Also, verify that the
go.modreplace directive correctly points to./paerserand is in sync with this commit hash.Consider whether pinning to a commit hash (rather than a tagged release) is sustainable for long-term maintenance. If this commit doesn't correspond to a release tag, you may want to request an upstream release or document the rationale for the custom commit pin.
internal/config/config.go (1)
166-209: Well-structured ACL configuration types.The nested composition provides clear separation of concerns.
One observation: there's a type inconsistency between fields using comma-separated strings (e.g.,
AppUsers.Allow,AppPath.Allow) and those using[]string(e.g.,AppIP.Allow). If this is intentional to match Docker label parsing conventions, consider adding a brief comment explaining the rationale. Otherwise, unifying to[]stringwould improve API consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.yml.gitmodulesCONTRIBUTING.mdDockerfileDockerfile.devDockerfile.distrolessdocker-compose.dev.ymlgo.modinternal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/controller/proxy_controller_test.gointernal/service/access_controls_service.gopaerserpatches/nested_maps.diff
💤 Files with no reviewable changes (1)
- docker-compose.dev.yml
🧰 Additional context used
🧬 Code graph analysis (3)
internal/controller/proxy_controller_test.go (2)
internal/service/access_controls_service.go (1)
NewAccessControlsService(16-21)internal/config/config.go (1)
App(166-173)
internal/bootstrap/service_bootstrap.go (2)
internal/service/access_controls_service.go (1)
NewAccessControlsService(16-21)internal/config/config.go (1)
Apps(162-164)
internal/service/access_controls_service.go (2)
internal/service/docker_service.go (1)
DockerService(15-19)internal/config/config.go (2)
App(166-173)Config(17-33)
🪛 LanguageTool
CONTRIBUTING.md
[style] ~23-~23: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ubmodules for some dependencies, so you need to initialize them with: ```sh git submod...
(REP_NEED_TO_VB)
[style] ~47-~47: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...Apply patches Some of the dependencies need to be patched in order to work correctly w...
(REP_NEED_TO_VB)
[style] ~47-~47: Consider a more concise word here.
Context: ... of the dependencies need to be patched in order to work correctly with the project, you ca...
(IN_ORDER_TO_PREMIUM)
[style] ~54-~54: Consider a more concise word here.
Context: ...s.diff ``` ## Create your .env file In order to configure the app you need to create an...
(IN_ORDER_TO_PREMIUM)
[style] ~55-~55: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...file In order to configure the app you need to create an environment file, this can be...
(REP_NEED_TO_VB)
[style] ~59-~59: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ctly work with Traefik and you will not need to do any building in your host machine. T...
(REP_NEED_TO_VB)
🔇 Additional comments (25)
CONTRIBUTING.md (4)
21-28: LGTM! Clear submodule initialization instructions.The submodule initialization steps are correctly documented and properly placed in the workflow before dependency installation.
45-51: LGTM! Patch application documented correctly.The patch application step is properly documented with the correct
git applycommand and target directory, aligning with the CI workflow changes.
32-35: LGTM! Correct Go module download command.The
go mod downloadcommand is correct and the updated wording improves clarity.
59-59: LGTM! Documentation wording improvements.The capitalization and wording updates for Docker and Traefik improve documentation consistency and professionalism.
Also applies to: 69-69
patches/nested_maps.diff (2)
1-43: LGTM! Comprehensive test case for nested map parsing.The test case properly validates the parsing of nested maps under the root key with the appropriate environment variable format and expected output structure.
44-93: LGTM! Consistent refactoring improves nested map handling.The refactoring to use
node.Kindconsistently throughout the metadata processing logic is sound. The special handling for string nodes with children (lines 54-57) properly coerces them to struct type, which addresses edge cases in nested map parsing..gitmodules (1)
1-4: LGTM! Standard submodule configuration.The paerser submodule is correctly configured with the appropriate repository URL and ignore policy, aligning with the documented workflow in CONTRIBUTING.md.
Dockerfile.dev (1)
5-5: LGTM! Necessary for local paerser submodule resolution.The COPY instruction correctly stages the paerser submodule directory before Go module resolution, which is essential given the replace directive in go.mod.
go.mod (1)
7-8: LGTM!The replace directive correctly maps the paerser dependency to the local submodule, aligning with the submodule initialization steps in the CI/CD workflows.
Dockerfile.distroless (1)
31-32: LGTM!The paerser directory is correctly copied before the Go module download step, ensuring the local module replacement is available during the build process.
Dockerfile (1)
31-32: LGTM!Consistent with Dockerfile.distroless, the paerser directory is correctly copied before the Go module download step.
internal/controller/proxy_controller_test.go (1)
44-44: LGTM!The test correctly updates the constructor call to match the new signature, passing an empty map for static ACL configuration which is appropriate for the test context.
.github/workflows/release.yml (2)
42-51: LGTM!The workflow correctly bumps the Go version and adds submodule initialization and patch application steps, consistent with the changes in ci.yml. This ensures the paerser submodule is available before building.
140-147: LGTM!Submodule initialization and patch application are consistently applied across all image build jobs (regular and distroless, both amd64 and arm64), ensuring the paerser directory is available in all Docker build contexts.
.github/workflows/nightly.yml (1)
64-73: LGTM!The nightly workflow consistently applies the same Go version bump and submodule initialization pattern as the CI and release workflows, ensuring uniform build environments across all workflow types.
internal/service/access_controls_service.go (5)
4-9: LGTM!The new imports (errors, strings, log) are appropriately used for error handling, domain parsing, and debug logging in the static ACL lookup functionality.
11-21: LGTM!The constructor signature change to accept static ACL configuration is appropriate and aligns with the test and bootstrap updates. The static map field enables in-memory ACL lookups before falling back to Docker labels.
23-25: LGTM!The Init method simplification is appropriate since static ACLs are now provided via the constructor from the configuration, eliminating the need for initialization-time loading.
42-54: LGTM!The ACL lookup priority is correctly implemented: static configuration first, with a fallback to Docker labels. This provides a smooth migration path and maintains backward compatibility with existing Docker-based ACL configurations. The debug logging helps trace which ACL source is being used.
27-40: The subdomain extraction for app name fallback matching has limitations with multi-level subdomains.The app name matching fallback using
strings.SplitN(domain, ".", 2)[0]extracts only the first component before the first dot. This works for simple domains like "localhost" or "myapp.example.com" matching app name "myapp", but fails with multi-level subdomains: "api.v1.myapp.example.com" would extract "api", not "myapp".Since the exact domain match is tried first (line 29), this is only problematic if:
- No exact domain match is configured
- The incoming domain has multiple subdomains
- The app key is not the outermost subdomain
Consider whether the app naming convention guarantees that app keys are always the outermost subdomain, or if this fallback should use a different strategy.
.github/workflows/ci.yml (2)
23-30: No action needed. The patch filepatches/nested_maps.diffexists in the repository and is properly tracked.
21-22: Go 1.24.0 is a stable, production-ready release. Released on February 11, 2025, this version is safe to use in the workflow.internal/bootstrap/service_bootstrap.go (1)
60-60: LGTM!The constructor call correctly passes the static Apps configuration map to enable static ACL lookup. The initialization order is appropriate since
dockerServiceis already initialized at this point.internal/config/config.go (2)
28-28: LGTM!The
Appsfield is correctly typed asmap[string]Appand integrates well with the existing config structure for the new static ACLs feature.
162-164: TheAppswrapper struct is actively used in the codebase. It's used as a generic type parameter ininternal/service/docker_service.go:86for decoding Docker labels viadecoders.DecodeLabels[config.Apps]. This wrapper serves a distinct purpose fromConfig.Appsand is not dead code.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
CONTRIBUTING.md (1)
8-8: Change "Golang" to "Go" in the version requirement.Use "Go 1.24.0+" instead of "Golang 1.24.0+" to match official Go documentation conventions. "Go" is the standard terminology; "Golang" is informal.
-- Golang 1.24.0+ +- Go 1.24.0+
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
23-23: Consider reducing repetition of "you need to" phrase.The static analysis tools flagged repeated use of "you need to" across lines 23, 47, 55, and 59. While not a blocker, varying the phrasing (e.g., "initialize them with", "apply patches with", "create an environment file by copying") would improve readability and flow. Similarly, "in order to" (lines 47, 54) could be replaced with more concise alternatives like "to" or restructured syntax.
🔎 Examples of tightened phrasing
-The project uses Git submodules for some dependencies, so you need to initialize them with: +The project uses Git submodules for some dependencies. Initialize them with: -Some of the dependencies need to be patched in order to work correctly with the project, you can apply the patches by running: +Some dependencies require patches to work correctly with the project. Apply them by running: -In order to configure the app you need to create an environment file, this can be done by copying the `.env.example` file to `.env` and modifying the environment variables to suit your needs. +Configure the app by copying `.env.example` to `.env` and modifying the environment variables as needed.Also applies to: 47-47, 55-55, 59-59
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONTRIBUTING.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-15T12:10:22.860Z
Learnt from: steveiliop56
Repo: steveiliop56/tinyauth PR: 355
File: internal/utils/decoders/decoders.go:76-80
Timestamp: 2025-09-15T12:10:22.860Z
Learning: When reviewing Go code, always check the project's Go version in go.mod before flagging range over integer syntax as invalid. Go 1.22+ supports `for i := range n` syntax where n is an integer, iterating from 0 to n-1.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-09-15T12:10:22.860Z
Learnt from: steveiliop56
Repo: steveiliop56/tinyauth PR: 355
File: internal/utils/decoders/decoders.go:76-80
Timestamp: 2025-09-15T12:10:22.860Z
Learning: Go 1.22 introduced the ability to range over integers using `for i := range n` syntax, which iterates from 0 to n-1. This affects code review suggestions for Go projects that target Go 1.22 or later.
Applied to files:
CONTRIBUTING.md
🪛 LanguageTool
CONTRIBUTING.md
[style] ~23-~23: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ubmodules for some dependencies, so you need to initialize them with: ```sh git submod...
(REP_NEED_TO_VB)
[style] ~47-~47: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...Apply patches Some of the dependencies need to be patched in order to work correctly w...
(REP_NEED_TO_VB)
[style] ~47-~47: Consider a more concise word here.
Context: ... of the dependencies need to be patched in order to work correctly with the project, you ca...
(IN_ORDER_TO_PREMIUM)
[style] ~54-~54: Consider a more concise word here.
Context: ...s.diff ``` ## Create your .env file In order to configure the app you need to create an...
(IN_ORDER_TO_PREMIUM)
[style] ~55-~55: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...file In order to configure the app you need to create an environment file, this can be...
(REP_NEED_TO_VB)
[style] ~59-~59: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ctly work with Traefik and you will not need to do any building in your host machine. T...
(REP_NEED_TO_VB)
⏰ 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 (3)
CONTRIBUTING.md (3)
21-28: Submodule initialization section is well-placed and correctly structured.The new "Initialize submodules" section appropriately documents the necessary setup step for the traefik/paerser dependency. The command sequence is clear and follows standard git practices.
35-35: Correct tool choice:go mod downloadinstead ofgo mod tidy.Changing from
go mod tidytogo mod downloadis semantically correct. The download command fetches module sources, while tidy would clean up the go.mod/go.sum files. For a development workflow, downloading dependencies is the appropriate step.
45-51: Patch application section is accurate and necessary.The new "Apply patches" section correctly documents the patch application step with the proper
git apply --directorysyntax. This aligns with the PR's goal of applying the parser patch in the submodule context before building dependencies.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.