[keycloak] Enable injecting themes#2142
[keycloak] Enable injecting themes#2142
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Keycloak deployment within Cozystack by enabling the injection of custom user-provided themes. It achieves this by introducing a mechanism to define initContainers that copy theme files from specified container images into the Keycloak application's theme directory, thereby allowing for UI customization. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis PR introduces optional theming support to the Keycloak Helm deployment. When themes are configured, initContainers copy theme artifacts into a shared emptyDir volume that the main Keycloak container accesses at runtime. The feature is conditionally applied and does not affect existing deployments without theme configuration. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Code Review
The pull request introduces a mechanism to inject custom themes into Keycloak deployments using Kubernetes initContainers. This allows administrators to specify theme images that will copy their contents into the Keycloak themes directory on startup. The changes include adding initContainers, volume mounts for the main Keycloak container, and an emptyDir volume for themes, all conditionally enabled by a new .Values.themes parameter. A corresponding entry has been added to values.yaml with an example.
| {{- range .Values.themes }} | ||
| - name: theme-{{ .name }} | ||
| image: {{ .image }} | ||
| command: ["sh", "-c", "cp -r /themes/* /opt/keycloak/themes/"] |
There was a problem hiding this comment.
The cp -r /themes/* /opt/keycloak/themes/ command can be problematic if the /themes directory in the image is empty, as the shell's wildcard expansion might lead to an invalid cp command (e.g., cp -r /opt/keycloak/themes/).
For more robust copying of directory contents, especially when the source directory might be empty, it's generally safer to use cp -a /themes/. /opt/keycloak/themes/. The . after /themes ensures that the contents of /themes are copied, not the directory itself, and handles empty directories gracefully without error.
command: ["sh", "-c", "cp -a /themes/. /opt/keycloak/themes/"]| {{- if .Values.themes }} | ||
| initContainers: | ||
| {{- range .Values.themes }} | ||
| - name: theme-{{ .name }} | ||
| image: {{ .image }} | ||
| command: ["sh", "-c", "cp -r /themes/* /opt/keycloak/themes/"] | ||
| volumeMounts: | ||
| - name: themes | ||
| mountPath: /opt/keycloak/themes | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
The initContainers are currently missing a securityContext. It's a good practice to define a securityContext for initContainers to ensure they run with appropriate privileges, similar to the main keycloak container. This helps maintain a consistent security posture across all containers in the pod.
Consider adding runAsNonRoot: true and allowPrivilegeEscalation: false to enhance security. readOnlyRootFilesystem: false would also be necessary since the initContainer performs a write operation.
{{- if .Values.themes }}
initContainers:
{{- range .Values.themes }}
- name: theme-{{ .name }}
image: {{ .image }}
command: ["sh", "-c", "cp -a /themes/. /opt/keycloak/themes/"]
securityContext:
runAsNonRoot: true
runAsUser: 1000 # Assuming 1000 is a non-root user in the theme image
readOnlyRootFilesystem: false
allowPrivilegeEscalation: false
volumeMounts:
- name: themes
mountPath: /opt/keycloak/themes
{{- end }}
{{- end }}There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/keycloak/templates/sts.yaml`:
- Line 48: Replace the fragile glob cp command in the theme initContainer so it
won’t fail on empty or hidden-only /themes; use a form that copies the directory
contents safely (preserve hidden files and handle empty sources) instead of "cp
-r /themes/* /opt/keycloak/themes/". Also sanitize and constrain the
user-derived container name "theme-{{ .name }}" to a valid Kubernetes DNS-1123
label (convert/replace invalid characters, lower-case, and truncate to <=57
chars) before using it in the template to prevent pod creation failures from
invalid or too-long theme names.
- Line 46: Sanitize the initContainer name built from theme name ("theme-{{
.name }}"): transform .name to RFC1123 DNS_LABEL by lowercasing it, replacing
any chars not in [a-z0-9-] with hyphens, truncating to 63 chars, and trimming
leading/trailing hyphens; implement this either inline with Helm functions
(lower, replaceRE/regexReplaceAll, trunc, trimPrefix, trimSuffix) or as a helper
template (e.g., include "sanitizeName" .Values.themes.X.name) and use the
sanitized result in the "theme-{{ ... }}" expression; also ensure if the
sanitized string becomes empty you fallback to a safe default (e.g., "default")
so the final initContainer name is valid and starts/ends with an alphanumeric
character.
| {{- if .Values.themes }} | ||
| initContainers: | ||
| {{- range .Values.themes }} | ||
| - name: theme-{{ .name }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes Pod container name validation rules allowed characters maximum length
💡 Result:
Kubernetes Pod spec.containers[].name (also initContainers[] / ephemeralContainers[]) must be a DNS_LABEL and unique within the Pod. The effective validation is the RFC1123 “DNS label” rule:
- Max length: 63 characters. [1][2]
- Allowed characters: lowercase letters
a-z, digits0-9, and hyphen-. [2] - Must start and end with an alphanumeric character (
[a-z0-9]) (so no leading/trailing-). [2] - No dots (
.) (those are valid for DNS subdomains, not labels). [2] - Regex (as implemented):
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$(equivalently:[a-z0-9]([-a-z0-9]*[a-z0-9])?). [2]
Sources: Kubernetes API reference for the container name field (DNS_LABEL) [1], and Kubernetes’ IsDNS1123Label validator / DNS1123LabelMaxLength=63 implementation in apimachinery. [2]
References
- Kubernetes API Reference (Container
name: “DNS_LABEL”, unique in Pod) https://kubernetes.io/docs/reference/generated/kubernetes-api/ - Kubernetes
apimachineryDNS1123 label validation (regex, max length 63, no dots) https://raw.githubusercontent.com/kubernetes/apimachinery/master/pkg/util/validation/validation.go
🏁 Script executed:
# Check if the file exists and examine the context around line 46
fd "sts.yaml" packages/system/Repository: cozystack/cozystack
Length of output: 819
🏁 Script executed:
# Read the sts.yaml file to see line 46 and context
wc -l packages/system/keycloak/templates/sts.yaml
cat -n packages/system/keycloak/templates/sts.yaml | sed -n '40,55p'Repository: cozystack/cozystack
Length of output: 697
🏁 Script executed:
# Check for other uses of .name or similar patterns in the sts.yaml file
cat -n packages/system/keycloak/templates/sts.yaml | grep -E "\.name|{{ "Repository: cozystack/cozystack
Length of output: 567
Sanitize initContainer names derived from user input.
The .name field from .Values.themes at line 46 is directly embedded in the initContainer name without validation. Kubernetes requires container names to follow RFC1123 DNS_LABEL rules: lowercase letters (a-z), digits (0-9), and hyphens only, with a maximum of 63 characters and must start and end with an alphanumeric character. Invalid values will cause Pod creation to fail at apply time.
Suggested fix
- - name: theme-{{ .name }}
+ - name: {{ printf "theme-%s" .name | lower | regexReplaceAll "[^a-z0-9-]" "-" | trunc 63 | trimSuffix "-" }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: theme-{{ .name }} | |
| - name: {{ printf "theme-%s" .name | lower | regexReplaceAll "[^a-z0-9-]" "-" | trunc 63 | trimSuffix "-" }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/keycloak/templates/sts.yaml` at line 46, Sanitize the
initContainer name built from theme name ("theme-{{ .name }}"): transform .name
to RFC1123 DNS_LABEL by lowercasing it, replacing any chars not in [a-z0-9-]
with hyphens, truncating to 63 chars, and trimming leading/trailing hyphens;
implement this either inline with Helm functions (lower,
replaceRE/regexReplaceAll, trunc, trimPrefix, trimSuffix) or as a helper
template (e.g., include "sanitizeName" .Values.themes.X.name) and use the
sanitized result in the "theme-{{ ... }}" expression; also ensure if the
sanitized string becomes empty you fallback to a safe default (e.g., "default")
so the final initContainer name is valid and starts/ends with an alphanumeric
character.
| {{- range .Values.themes }} | ||
| - name: theme-{{ .name }} | ||
| image: {{ .image }} | ||
| command: ["sh", "-c", "cp -r /themes/* /opt/keycloak/themes/"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "sts.yaml" -o -name "*keycloak*" | head -20Repository: cozystack/cozystack
Length of output: 1693
🏁 Script executed:
cat -n packages/system/keycloak/templates/sts.yaml | sed -n '40,55p'Repository: cozystack/cozystack
Length of output: 649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
mkdir -p "$tmp/src-empty" "$tmp/src-hidden" "$tmp/dst"
touch "$tmp/src-hidden/.hidden-theme-file"
echo "=== Test 1: cp -r with empty source glob ==="
set +e
sh -c "cp -r $tmp/src-empty/* $tmp/dst/"
echo "exit code for 'cp -r src/* dst/' with empty source: $?"
set -e
echo ""
echo "=== Test 2: cp -a with /. notation ==="
rm -rf "$tmp/dst"/*
mkdir -p "$tmp/src-with-files"
touch "$tmp/src-with-files/file1" "$tmp/src-with-files/.hidden"
mkdir -p "$tmp/src-with-files/subdir"
sh -c "cp -a $tmp/src-with-files/. $tmp/dst/"
echo "exit code: $?"
echo "contents copied:"
find "$tmp/dst" -type f | sort
echo ""
echo "=== Test 3: cp -a with empty source /. ==="
rm -rf "$tmp/dst"/*
sh -c "cp -a $tmp/src-empty/. $tmp/dst/"
echo "exit code for 'cp -a src/. dst/' with empty source: $?"
echo "dst contents:"
ls -la "$tmp/dst" || echo "directory is empty"
rm -rf "$tmp"Repository: cozystack/cozystack
Length of output: 664
🏁 Script executed:
# Check Kubernetes container naming constraints
# Container names must be DNS-1123 compliant: alphanumeric and hyphens only, max 63 chars
cat <<'EOF'
# Kubernetes container naming rules (from API validation):
# - Must be DNS-1123 compliant: [a-z0-9]([-a-z0-9]*[a-z0-9])?
# - Max length: 63 characters
# - No underscores, dots, or other special characters
# The template generates: theme-{{ .name }}
# This means:
# - If .name contains invalid chars (e.g., _, ., uppercase), container name becomes invalid
# - If .name exceeds 58 chars, the full name exceeds 63 char limit (6 chars for "theme-" prefix)
# - No validation happens at helm template time; failure occurs at pod creation
EOFRepository: cozystack/cozystack
Length of output: 595
Make the theme copy command resilient to empty sources.
Line 48's glob expansion cp -r /themes/* fails with exit code 1 when /themes is empty or contains only hidden files, causing initContainer failure and blocking pod startup. The suggested fix using cp -a /themes/. /opt/keycloak/themes/ handles empty directories gracefully and properly preserves hidden files.
Additionally, the container name theme-{{ .name }} on line 46 is user-derived from .Values.themes[].name and is not validated. Invalid characters or names exceeding 57 characters will cause Kubernetes pod validation to fail at creation time.
Suggested fix
- command: ["sh", "-c", "cp -r /themes/* /opt/keycloak/themes/"]
+ command: ["sh", "-ec", "cp -a /themes/. /opt/keycloak/themes/"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| command: ["sh", "-c", "cp -r /themes/* /opt/keycloak/themes/"] | |
| command: ["sh", "-ec", "cp -a /themes/. /opt/keycloak/themes/"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/keycloak/templates/sts.yaml` at line 48, Replace the fragile
glob cp command in the theme initContainer so it won’t fail on empty or
hidden-only /themes; use a form that copies the directory contents safely
(preserve hidden files and handle empty sources) instead of "cp -r /themes/*
/opt/keycloak/themes/". Also sanitize and constrain the user-derived container
name "theme-{{ .name }}" to a valid Kubernetes DNS-1123 label (convert/replace
invalid characters, lower-case, and truncate to <=57 chars) before using it in
the template to prevent pod creation failures from invalid or too-long theme
names.
| {{- range .Values.themes }} | ||
| - name: theme-{{ .name }} | ||
| image: {{ .image }} | ||
| command: ["sh", "-c", "cp -r /themes/* /opt/keycloak/themes/"] |
There was a problem hiding this comment.
When multiple themes are specified, they can get unexpectedly merged
This patch lets Cozystack admins specify initContainers that will run `cp -r /themes/ /opt/keycloak/themes/` on startup, effectively providing an interface for operators to inject custom themes into the keycloak deployment to customize the UI. ```release-note [keycloak] Enable injection of user-provided themes for Keycloak via initContainers. ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
dfc4192 to
7f38b67
Compare
What this PR does
This patch lets Cozystack admins specify initContainers that will run
cp -r /themes/ /opt/keycloak/themes/on startup, effectively providing an interface for operators to inject custom themes into the keycloak deployment to customize the UI.Release note
Summary by CodeRabbit