refactor: move to traefik paerser for label parsing#197
Conversation
|
Warning Rate limit exceeded@steveiliop56 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis update refactors the handling and structure of Docker container labels in the codebase. It introduces new structured types for labels, replaces manual label parsing with a declarative decoding approach, updates related function signatures, improves error handling, and updates Dockerfile base images. Associated tests and constants are adjusted or removed to align with the new label structure. Changes
Sequence Diagram(s)sequenceDiagram
participant Docker as Docker Engine
participant App as Application
participant Utils as utils.GetLabels
participant Parser as parser.Decode
App->>Docker: Get container labels
Docker-->>App: Return label map
App->>Utils: GetLabels(label map)
Utils->>Parser: Decode(label map, Labels struct)
Parser-->>Utils: Labels struct or error
Utils-->>App: Labels struct or error
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
frontend/Dockerfile.dev (1)
1-1: Consider pinning the image by digest for reproducible builds.Bumping to
oven/bun:1.2.16-alpineis fine, but using a digest (@sha256:<hash>) guarantees the exact layer set and guards against unexpected retags.Dockerfile (1)
2-2: Same reproducibility concern asDockerfile.dev.Pin the Bun builder stage to a digest so CI / local builds always run with the exact same bits.
-FROM oven/bun:1.2.16-alpine AS frontend-builder +FROM oven/bun:1.2.16-alpine@sha256:<expected-hash> AS frontend-builderinternal/handlers/handlers.go (1)
240-244: Duplicate logic – extract into a helper to avoid divergence.The block is identical to lines 117-121. Factor it into a small helper (e.g.
setExtraHeaders(c *gin.Context, hdrs []string)) to keep behaviour in sync and shrink this very large handler.internal/types/config.go (1)
96-108: Add explicit struct tags to future-proof label decoding.
github.com/traefik/paerserrelies on struct tags (label:"...") or field names to map keys. Relying on bare field names assumes label keys will always match the Go identifier case-insensitively (e.g.Headers). Explicit tags make the mapping obvious and resilient to renames:-type OAuthLabels struct { - Whitelist string - Groups string +type OAuthLabels struct { + Whitelist string `label:"oauth.whitelist"` + Groups string `label:"oauth.groups"` } -type Labels struct { - Users string - Allowed string - Headers []string - OAuth OAuthLabels +type Labels struct { + Users string `label:"users"` + Allowed string `label:"allowed"` + Headers []string `label:"headers"` + OAuth OAuthLabels `label:"oauth"` }Tag names are illustrative—adjust to your actual label keys.
internal/utils/utils.go (1)
201-207: Hard-coding every key defeats the purpose of declarative decodingPassing both
"tinyauth"and every concrete key toparser.Decodemakes maintenance harder – any new sub-key will require touching this list in multiple places.If you just want to allow anything under
tinyauth.*, provide only the root prefix:- err := parser.Decode(labels, &labelsParsed, - "tinyauth", "tinyauth.users", "tinyauth.allowed", - "tinyauth.headers", "tinyauth.oauth") + err := parser.Decode(labels, &labelsParsed, "tinyauth")This keeps the decoder future-proof.
internal/docker/docker.go (1)
114-121: Duplicate log + error propagationThe error from
utils.GetLabelsis already logged inside that function.
Logging it again here adds noise without extra signal.Either drop the local log line or change the inner function to not log.
internal/utils/utils_test.go (1)
282-316: Slice order in tests can introduce flakiness
reflect.DeepEqualon slices is order-sensitive.
parser.Decodeiterates over a map, so"tinyauth.headers"➔[]stringorder is not guaranteed across Go versions.Prefer a set/element comparison:
- if !reflect.DeepEqual(expected, result) { + if !cmp.Equal(expected, result, cmpopts.SortSlices(func(a, b string) bool { return a < b })) { t.Fatalf("Expected %v, got %v", expected, result) }(uses
github.com/google/go-cmp/cmp)Prevents sporadic CI failures.
internal/auth/auth.go (2)
267-278: Minor readability: early-return instead of nestedif- if context.OAuth { - log.Debug().Msg("Checking OAuth whitelist") - return utils.CheckWhitelist(labels.OAuth.Whitelist, context.Email) - } - - // Check users - log.Debug().Msg("Checking users") - - return utils.CheckWhitelist(labels.Users, context.Username) + if context.OAuth { + log.Debug().Msg("Checking OAuth whitelist") + return utils.CheckWhitelist(labels.OAuth.Whitelist, context.Email) + } + // User/password path + return utils.CheckWhitelist(labels.Users, context.Username)Same logic, flatter structure.
310-336: Compiling regex for every request is unnecessary overhead
AuthEnabledrecompileslabels.Allowedon each call.
Caching the compiled pattern when labels are first parsed avoids per-request CPU and GC churn.Consider adding a
*regexp.Regexpfield totypes.Labelsor storing it in a small in-memory cache keyed by the pattern string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
Dockerfile(1 hunks)frontend/Dockerfile.dev(1 hunks)go.mod(1 hunks)internal/auth/auth.go(4 hunks)internal/constants/constants.go(0 hunks)internal/docker/docker.go(4 hunks)internal/handlers/handlers.go(2 hunks)internal/types/config.go(1 hunks)internal/types/types.go(0 hunks)internal/utils/utils.go(2 hunks)internal/utils/utils_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/constants/constants.go
- internal/types/types.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/handlers/handlers.go (1)
internal/utils/utils.go (1)
ParseHeaders(178-194)
internal/auth/auth.go (3)
internal/types/types.go (1)
UserContext(36-46)internal/types/config.go (1)
Labels(103-108)internal/utils/utils.go (1)
CheckWhitelist(288-324)
internal/docker/docker.go (2)
internal/utils/utils.go (1)
GetLabels(197-212)internal/types/config.go (1)
Labels(103-108)
internal/utils/utils.go (1)
internal/types/config.go (1)
Labels(103-108)
🔇 Additional comments (1)
go.mod (1)
27-27: Mark thepaerserdependency as direct (not indirect).The codebase now imports
github.com/traefik/paerser/parser, sopaerseris a first-class dependency.go mod tidywill automatically drop the// indirectsuffix, but checking it in as direct avoids noisy diffs later.-github.com/traefik/paerser v0.2.2 // indirect +github.com/traefik/paerser v0.2.2Run
go mod tidybefore merging to ensure the module graph is consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/utils/utils.go (1)
183-190:⚠️ Potential issueStill splits on every “=” and keeps stray whitespace – bug resurfaced
Previous review already pointed this out:
strings.Split(header, "=")breaks as soon as a header value contains an=(e.g.X-Auth=a=b) and you also keep leading / trailing spaces in both key and value. The implementation hasn’t been updated, so the parser still discards valid headers silently.- headerSplit := strings.Split(header, "=") + headerSplit := strings.SplitN(header, "=", 2) if len(headerSplit) != 2 { log.Warn().Str("header", header).Msg("Invalid header format, skipping") continue } - headerMap[headerSplit[0]] = SanitizeHeader(headerSplit[1]) + key := strings.TrimSpace(headerSplit[0]) + val := strings.TrimSpace(headerSplit[1]) + headerMap[key] = SanitizeHeader(val)This single change fixes both the “extra =” and “space pollution” issues without impacting the happy path.
🧹 Nitpick comments (1)
internal/utils/utils.go (1)
201-208:parser.Decodecall can be simplifiedYou already scope decoding to the
tinyauthprefix, so listing every sub-path (tinyauth.users,tinyauth.allowed, …) is redundant and brittle when new fields are added. A simpler, future-proof call:err := parser.Decode(labels, &labelsParsed, "tinyauth")Less to maintain, same result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/utils/utils.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/utils/utils.go (1)
internal/types/config.go (1)
Labels(103-108)
1 participant
Summary by CodeRabbit
Chores
Refactor
Tests