feat(access-control): Add support for Kubernetes Label#627
Open
contre95 wants to merge 12 commits intosteveiliop56:mainfrom
Open
feat(access-control): Add support for Kubernetes Label#627contre95 wants to merge 12 commits intosteveiliop56:mainfrom
contre95 wants to merge 12 commits intosteveiliop56:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces pluggable label provider support to the system by creating a Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant Config
participant KubeService
participant DockerService as DockerService
participant AccessControls as AccessControlsService
participant LabelCache as In-Memory Cache
Bootstrap->>Config: Read LabelProvider setting
Config-->>Bootstrap: Provider: "kubernetes" or "docker"
alt LabelProvider == "kubernetes"
Bootstrap->>KubeService: NewKubernetesService()
activate KubeService
KubeService->>KubeService: Init()
KubeService->>KubeService: Detect Ingress v1/v1beta1 APIs
KubeService->>KubeService: Start event watchers
KubeService->>LabelCache: Initialize maps/indices
KubeService-->>Bootstrap: Ready
deactivate KubeService
Bootstrap->>AccessControls: NewAccessControlsService(kubeService)
else LabelProvider == "docker"
Bootstrap->>DockerService: NewDockerService()
activate DockerService
DockerService-->>Bootstrap: Ready
deactivate DockerService
Bootstrap->>AccessControls: NewAccessControlsService(dockerService)
end
activate AccessControls
AccessControls->>AccessControls: Store labelProvider
deactivate AccessControls
sequenceDiagram
participant Client
participant AccessControls as AccessControlsService
participant LabelProvider as LabelProvider<br/>(Kubernetes or Docker)
participant Cache as Ingress/Docker<br/>Cache
participant K8sAPI as Kubernetes API<br/>(if applicable)
Client->>AccessControls: GetAccessControls(domain)
activate AccessControls
AccessControls->>AccessControls: Check static config
alt Found in static config
AccessControls-->>Client: Return static labels
else Not in static config
AccessControls->>LabelProvider: GetLabels(domain)
activate LabelProvider
LabelProvider->>Cache: Check cache by domain/appName
alt Found in cache
LabelProvider-->>AccessControls: Return cached labels
else Not in cache
LabelProvider->>K8sAPI: Query Ingress resources
K8sAPI-->>LabelProvider: Return Ingress annotations
LabelProvider->>Cache: Decode & update cache
LabelProvider-->>AccessControls: Return labels
end
deactivate LabelProvider
AccessControls-->>Client: Return labels
end
deactivate AccessControls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
Author
|
Hi @steveiliop56, before I continue with this feature. I wanted to check if this is something you wanted in Tinyauth. I also wanted to ask if I should part from |
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/service/kubernetes_service.go`:
- Around line 41-71: The Init() method of KubernetesService currently swallows
errors and returns nil; change the three error paths (failure from
rest.InClusterConfig()/clientcmd.BuildConfigFromFlags, dynamic.NewForConfig, and
discovery.NewDiscoveryClientForConfig) to return the actual err instead of nil
while still setting k.started = false as needed so bootstrap can detect
failures; update KubernetesService.Init() to propagate these errors (references:
KubernetesService.Init, rest.InClusterConfig, clientcmd.BuildConfigFromFlags,
dynamic.NewForConfig, discovery.NewDiscoveryClientForConfig, and k.started).
- Around line 166-225: GetLabels currently calls getLabelsFromGVR which always
List()s cluster-wide and ignores k.cache; populate and consult k.cache (the
existing cache field) instead of listing on each call: on startup implement
watchIngressV1() and watchIngressV1beta1() to maintain the cache
(add/update/delete entries keyed by namespace/name and indexed by domain/app
name), then change GetLabels and getLabelsFromGVR to first try lookups against
k.cache (use domain and the app name prefix) and only fall back to a scoped List
with namespace/label selectors if cache miss; ensure cache access is
concurrency-safe and that watchers handle resyncs and errors.
In `@README.md`:
- Line 17: Update the README text that currently uses "Github" to use the
official branding "GitHub"; locate the sentence containing "OAuth with Google,
Github or any other provider" and replace "Github" with "GitHub" so the README
uses the correct capitalization.
🧹 Nitpick comments (1)
internal/config/config.go (1)
31-31: Prefer constants for label provider values.Centralize
"docker"/"kubernetes"to avoid typos and enable consistent validation across the codebase.♻️ Suggested addition
type Config struct { AppURL string `description:"The base URL where the app is hosted." yaml:"appUrl"` ResourcesDir string `description:"The directory where resources are stored." yaml:"resourcesDir"` @@ LabelProvider string `description:"Label provider to use for ACLs (docker or kubernetes)." yaml:"labelProvider"` Experimental ExperimentalConfig `description:"Experimental features, use with caution." yaml:"experimental"` Log LogConfig `description:"Logging configuration." yaml:"log"` } + +const ( + LabelProviderDocker = "docker" + LabelProviderKubernetes = "kubernetes" +)
Owner
|
@contre95 thank you for the pull request. From a quick look it seems good but I will unfortunately have to hold it until the next version of Tinyauth ( |
Author
|
@steveiliop56 No problem, I bet it can wait. I've been testing |
contre95
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/service/kubernetes_service.go (3)
128-160: Annotation-decoding logic is duplicated betweenupdateFromListand thewatchGVRAdded/Modifiedhandler.Lines 137–158 in
updateFromListand lines 234–255 inwatchGVRare near-identical: both get annotations, decode labels withdecoders.DecodeLabels, build a[]ingressAppslice, and calladdIngressApps/removeIngress. Extract a helper to eliminate the duplication.♻️ Proposed helper
// appsFromIngress decodes tinyauth app annotations from an Ingress object. // Returns the list of ingressApp entries and whether any apps were found. func (k *KubernetesService) appsFromIngress(item *unstructured.Unstructured) ([]ingressApp, bool) { annotations := item.GetAnnotations() if annotations == nil { return nil, false } labels, err := decoders.DecodeLabels[config.Apps](annotations, "apps") if err != nil { tlog.App.Debug().Err(err).Msg("Failed to decode labels from annotations") return nil, false } var apps []ingressApp for appName, appLabels := range labels.Apps { if appLabels.Config.Domain == "" { continue } apps = append(apps, ingressApp{ domain: appLabels.Config.Domain, appName: appName, app: appLabels, }) } return apps, len(apps) > 0 }Then both
updateFromListand the watcher can callappsFromIngress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/kubernetes_service.go` around lines 128 - 160, Duplicate annotation-decoding code in updateFromList and the watchGVR Added/Modified handler should be extracted into a helper; add a method appsFromIngress on KubernetesService that accepts an *unstructured.Unstructured, reads annotations, calls decoders.DecodeLabels[config.Apps](..., "apps"), logs decode errors with tlog.App.Debug().Err(...).Msg(...), builds and returns []ingressApp (filtering out entries with empty Config.Domain) and a bool indicating non-empty result; then replace the decoding blocks in updateFromList and the watch handler to call k.appsFromIngress(item) and call k.addIngressApps(namespace, name, apps) or k.removeIngress(namespace, name) based on the returned bool.
181-184:time.SleepinwatchGVRdoes not respect context cancellation — delays graceful shutdown.Both the initial-resync failure path (line 183, 30s sleep) and the watch-error path (line 201, 10s sleep) use bare
time.Sleep, so a service shutdown that cancelsk.ctxwill still block for up to 30 seconds before the goroutine exits. Replace with a context-aware sleep.♻️ Proposed fix: context-aware backoff
- if err := k.resyncGVR(gvr); err != nil { - tlog.App.Error().Err(err).Str("api", gvr.GroupVersion().String()).Msg("Initial resync failed, retrying in 30 seconds") - time.Sleep(30 * time.Second) - } + if err := k.resyncGVR(gvr); err != nil { + tlog.App.Error().Err(err).Str("api", gvr.GroupVersion().String()).Msg("Initial resync failed, retrying in 30 seconds") + select { + case <-k.ctx.Done(): + return + case <-time.After(30 * time.Second): + } + }And in the watch error path (around line 199–202):
- cancel() - time.Sleep(10 * time.Second) - continue + cancel() + select { + case <-k.ctx.Done(): + return + case <-time.After(10 * time.Second): + continue + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/kubernetes_service.go` around lines 181 - 184, The two bare time.Sleep calls inside watchGVR (after calling k.resyncGVR(gvr) and in the watch-error retry path) block shutdown; replace them with context-aware waits that return immediately if k.ctx is canceled. Concretely, in watchGVR (around k.resyncGVR(gvr) error handling and the watch error handling) swap time.Sleep(d) for a select that waits on either <-k.ctx.Done() (and then return/exit the goroutine) or <-time.After(d) (to perform the retry), so both the initial resync and subsequent retries respect k.ctx cancellation; keep existing logs (tlog.App.Error().Err(...)) and behavior otherwise.
22-22:var _ = unstructured.Unstructured{}is redundant.
unstructured.Unstructuredis already used in type assertions at lines 222 and 257 (*unstructured.Unstructured), so the import is not at risk of being considered unused. This blank-identifier variable can be removed.-var _ = unstructured.Unstructured{} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/kubernetes_service.go` at line 22, Remove the redundant blank-assignment "var _ = unstructured.Unstructured{}" from the file; the unstructured import is already used by type assertions to "*unstructured.Unstructured" in functions that reference those types, so simply delete that var declaration to avoid the unnecessary placeholder while leaving the existing type assertions (references to *unstructured.Unstructured) intact.
🤖 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.go`:
- Line 84: LabelProvider currently accepts any string but only "kubernetes" is
handled and invalid values silently fall back to Docker; update
NewDefaultConfiguration to set LabelProvider's explicit default to "docker" and
modify the code path that switches on LabelProvider (the switch handling in
service_bootstrap.go) to detect unknown values and log a warning via the
existing logger before using the docker fallback. Specifically, set
LabelProvider = "docker" in NewDefaultConfiguration(), and in the switch that
branches on LabelProvider add a default case which logs a clear warning
mentioning the invalid value and the fallback to Docker and then proceeds with
the docker branch.
- Line 76: Remove the orphaned top-level Config.DisableUIWarnings field from the
Config struct in internal/config/config.go (the line with DisableUIWarnings
`yaml:"disableUIWarnings"`), since the canonical field is
UIConfig.DisableWarnings and NewDefaultConfiguration() never initializes the
top-level one; delete that field, search for any remaining references to
DisableUIWarnings and remove or redirect them to UIConfig.DisableWarnings, then
run tests to ensure no consumers remain and update any docs or yaml examples
that referenced disableUIWarnings.
In `@internal/service/kubernetes_service.go`:
- Around line 396-435: GetLabels currently swallows errors from
k.getLabelsFromGVR (called for k.v1GVR and k.v1beta1GVR), logging them at Debug
and then returning an empty app with nil error; change this so any non-nil error
returned by getLabelsFromGVR is propagated to the caller (return config.App{},
err) instead of just logging, while still returning a valid app when
app.Config.Domain is populated; update both v1 and v1beta1 branches to follow
this behavior so callers (e.g., AccessControlsService) can distinguish “no ACL”
from API failures.
---
Duplicate comments:
In `@internal/service/kubernetes_service.go`:
- Around line 276-301: KubernetesService.Init currently logs failures from
rest.InClusterConfig, dynamic.NewForConfig, and
discovery.NewDiscoveryClientForConfig but returns nil error, making upstream
bootstrap error checks unreachable; update Init (function
KubernetesService.Init) to return the actual error (or a wrapped error) on each
failure path instead of nil and still set k.started = false so callers like
service_bootstrap can detect and handle initialization failure; ensure you
return the err variable (or fmt.Errorf(...) wrapping it) in the three failure
branches.
---
Nitpick comments:
In `@internal/service/kubernetes_service.go`:
- Around line 128-160: Duplicate annotation-decoding code in updateFromList and
the watchGVR Added/Modified handler should be extracted into a helper; add a
method appsFromIngress on KubernetesService that accepts an
*unstructured.Unstructured, reads annotations, calls
decoders.DecodeLabels[config.Apps](..., "apps"), logs decode errors with
tlog.App.Debug().Err(...).Msg(...), builds and returns []ingressApp (filtering
out entries with empty Config.Domain) and a bool indicating non-empty result;
then replace the decoding blocks in updateFromList and the watch handler to call
k.appsFromIngress(item) and call k.addIngressApps(namespace, name, apps) or
k.removeIngress(namespace, name) based on the returned bool.
- Around line 181-184: The two bare time.Sleep calls inside watchGVR (after
calling k.resyncGVR(gvr) and in the watch-error retry path) block shutdown;
replace them with context-aware waits that return immediately if k.ctx is
canceled. Concretely, in watchGVR (around k.resyncGVR(gvr) error handling and
the watch error handling) swap time.Sleep(d) for a select that waits on either
<-k.ctx.Done() (and then return/exit the goroutine) or <-time.After(d) (to
perform the retry), so both the initial resync and subsequent retries respect
k.ctx cancellation; keep existing logs (tlog.App.Error().Err(...)) and behavior
otherwise.
- Line 22: Remove the redundant blank-assignment "var _ =
unstructured.Unstructured{}" from the file; the unstructured import is already
used by type assertions to "*unstructured.Unstructured" in functions that
reference those types, so simply delete that var declaration to avoid the
unnecessary placeholder while leaving the existing type assertions (references
to *unstructured.Unstructured) intact.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 30-31: In go.mod there are duplicate direct requirements for the
same module modernc.org/sqlite at v1.45.0 and v1.46.1; remove the older entry
(v1.45.0) so only modernc.org/sqlite v1.46.1 remains, then run go mod tidy to
ensure the module graph is consistent and go.sum is updated.
ℹ️ Review info
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 (1)
go.mod
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/config/config.go (1)
91-91:⚠️ Potential issue | 🟡 MinorSet an explicit default for
LabelProviderto prevent silent typo fallback.Line 91 introduces a free-form selector, but no explicit default is set in
NewDefaultConfiguration(), so mistyped values still silently fall back to Docker in bootstrap.Suggested fix
func NewDefaultConfiguration() *Config { return &Config{ + LabelProvider: "docker", Database: DatabaseConfig{ Path: "./tinyauth.db", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` at line 91, NewDefaultConfiguration currently doesn't set a default for the LabelProvider field so typos silently fallback to Docker; update NewDefaultConfiguration to explicitly initialize LabelProvider (e.g., "docker" or "kubernetes") to a sensible default, ensure the struct field LabelProvider in config.go gets that default value, and adjust any tests or callers that rely on implicit behavior; reference the LabelProvider field and the NewDefaultConfiguration function to locate and apply the change.go.mod (1)
28-29:⚠️ Potential issue | 🔴 CriticalRemove duplicate
modernc.org/sqliterequirements.Lines 28-29 declare the same module twice with different versions; keep only one version (
v1.46.1) to avoid module graph/parsing problems.Suggested fix
- modernc.org/sqlite v1.45.0 modernc.org/sqlite v1.46.1Use this script to verify duplicate module entries in
go.mod:#!/bin/bash python - <<'PY' import re from collections import defaultdict from pathlib import Path mods = defaultdict(list) for i, line in enumerate(Path("go.mod").read_text(encoding="utf-8").splitlines(), 1): m = re.match(r'^\s*([A-Za-z0-9._/\-]+)\s+v[^\s]+(?:\s+//\s+indirect)?\s*$', line) if m: mods[m.group(1)].append((i, line.strip())) dups = {k: v for k, v in mods.items() if len(v) > 1} if not dups: print("OK: no duplicate module entries found.") else: print("Duplicate module entries found:") for mod, entries in sorted(dups.items()): print(f" {mod}") for ln, txt in entries: print(f" line {ln}: {txt}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 28 - 29, go.mod currently lists modernc.org/sqlite twice with different versions; remove the older entry "modernc.org/sqlite v1.45.0" and keep only "modernc.org/sqlite v1.46.1" in the require block, then run go mod tidy to refresh the module graph and ensure no duplicate entries remain; look for the duplicated module name "modernc.org/sqlite" to locate the lines to edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go.mod`:
- Around line 28-29: go.mod currently lists modernc.org/sqlite twice with
different versions; remove the older entry "modernc.org/sqlite v1.45.0" and keep
only "modernc.org/sqlite v1.46.1" in the require block, then run go mod tidy to
refresh the module graph and ensure no duplicate entries remain; look for the
duplicated module name "modernc.org/sqlite" to locate the lines to edit.
In `@internal/config/config.go`:
- Line 91: NewDefaultConfiguration currently doesn't set a default for the
LabelProvider field so typos silently fallback to Docker; update
NewDefaultConfiguration to explicitly initialize LabelProvider (e.g., "docker"
or "kubernetes") to a sensible default, ensure the struct field LabelProvider in
config.go gets that default value, and adjust any tests or callers that rely on
implicit behavior; reference the LabelProvider field and the
NewDefaultConfiguration function to locate and apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15427e9f-0ea3-40d6-9616-705fa06b68a5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modinternal/config/config.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release Notes
New Features
Documentation
LabelProviderconfiguration option to choose between Docker labels and Kubernetes for ACL configuration.Summary by CodeRabbit