add keycloak #475
add keycloak #475
Conversation
dosubot
bot
added
size:L
WalkthroughThe changes in this pull request introduce a new release entry for Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 23
🧹 Outside diff range and nitpick comments (15)
packages/system/postgres-instance/values.yaml (2)
4-5: Review password generation mechanismThe password generation uses Helm's built-in functions which is good, but consider:
- The base64 encoding might not be necessary if the value is used in a Kubernetes Secret (Kubernetes automatically handles base64 encoding)
- 16 characters provide good entropy, but you might want to ensure the generated password meets any specific password policies
Consider this alternative if the value is used in a Secret:
database: - password: "{{ randAlphaNum 16 | b64enc }}" + password: "{{ randAlphaNum 16 }}"
1-5: Consider adding additional PostgreSQL configuration parametersThe current configuration is minimal. Consider adding common PostgreSQL parameters such as:
- Resource limits (CPU/Memory)
- Backup configuration
- Monitoring integration
- PostgreSQL version specification
These parameters will help ensure stable operation and maintainability of the database instance.
packages/system/keycloak/templates/service-headless.yaml (1)
3-9: Enhance service maintainability with metadataConsider adding standard Kubernetes labels and annotations for better service discovery and maintenance.
Apply this diff:
metadata: name: keycloak-headless + labels: + app.kubernetes.io/name: keycloak + app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/component: auth + app.kubernetes.io/part-of: {{ .Chart.Name }} + app.kubernetes.io/managed-by: {{ .Release.Service }} + annotations: + service.alpha.kubernetes.io/tolerate-unready-endpoints: "true" spec: type: ClusterIP clusterIP: None selector: - app: keycloak-ha + app.kubernetes.io/name: keycloak + app.kubernetes.io/instance: {{ .Release.Name }}packages/system/keycloak/templates/service.yaml (1)
7-10: Follow K8s naming conventions for protocol-specific ports.For better integration with service mesh and monitoring tools, consider using the standardized port naming convention:
<protocol>[-<suffix>].ports: - - name: http + - name: http-web port: 8080 protocol: TCPpackages/system/postgres-instance/templates/cluster.yaml (1)
1-5: LGTM! Consider explicitly specifying the namespace.The resource definition looks good. While the release namespace will be used by default, consider explicitly specifying the namespace for better clarity and to prevent potential issues in multi-namespace deployments.
metadata: name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }}packages/system/keycloak/templates/ingress.yaml (2)
1-8: Consider adding recommended Ingress annotationsWhile the template supports custom annotations, consider adding some recommended ones by default:
nginx.ingress.kubernetes.io/proxy-buffer-size: Keycloak responses can be largenginx.ingress.kubernetes.io/proxy-read-timeout: Keycloak operations might need extended timeoutsAdd these default annotations while still allowing overrides:
annotations: + nginx.ingress.kubernetes.io/proxy-buffer-size: "128k" + nginx.ingress.kubernetes.io/proxy-read-timeout: "120" {{- toYaml . | nindent 4 }}🧰 Tools
🪛 yamllint
[error] 6-6: syntax error: could not find expected ':'
(syntax)
1-25: Add health check annotationsConsider adding health check annotations to ensure proper load balancer behavior.
Add these annotations to the metadata section:
annotations: + nginx.ingress.kubernetes.io/healthcheck-path: /health/ready + nginx.ingress.kubernetes.io/healthcheck-interval: "10s" {{- toYaml . | nindent 4 }}🧰 Tools
🪛 yamllint
[error] 6-6: syntax error: could not find expected ':'
(syntax)
packages/system/postgres-instance/templates/init-script.yaml (2)
1-6: Consider enhancing secret security measuresWhile the implementation is functional, consider these security improvements:
- Add immutability annotation to prevent accidental updates
- Consider using Kubernetes Secrets Store CSI Driver for better secrets management
- Add labels for better resource management
apiVersion: v1 kind: Secret metadata: name: {{ .Release.Name }}-credentials + annotations: + immutable: "true" + labels: + app.kubernetes.io/name: postgres-instance + app.kubernetes.io/component: database stringData: keycloak: {{ .Values.database.password | b64dec }}🧰 Tools
🪛 yamllint
[error] 4-4: syntax error: expected , but found ''
(syntax)
8-12: Consider using ConfigMap for non-sensitive initialization scriptThe initialization script doesn't contain sensitive data and could be stored in a ConfigMap instead of a Secret. This follows the principle of least privilege and preserves Secret quota for sensitive data.
-apiVersion: v1 -kind: Secret +apiVersion: v1 +kind: ConfigMap metadata: name: {{ .Release.Name }}-init-script -stringData: + labels: + app.kubernetes.io/name: postgres-instance + app.kubernetes.io/component: database +data:packages/system/keycloak/templates/init-script.yaml (2)
1-4: Consider adding namespace specificationThe ConfigMap should explicitly specify a namespace to avoid potential conflicts and ensure proper isolation.
apiVersion: v1 kind: ConfigMap metadata: name: keycloak-init-script + namespace: {{ .Release.Namespace }}
1-30: Consider operational aspects of Keycloak initializationSeveral architectural considerations:
- Consider implementing a readiness probe that checks realm availability
- Add backup/restore procedures for realm configuration
- Consider implementing idempotency to handle script reruns
- Add monitoring for realm availability and token validity
packages/core/platform/bundles/distro-hosted.yaml (1)
134-138: Consider adding health checks and resource configurationsThe Keycloak deployment might benefit from explicit configuration parameters for:
- Health check endpoints
- Resource limits and requests
- Replicas configuration
These could be added as values in the chart, but it would be beneficial to make critical ones visible here for easier maintenance.
packages/system/keycloak/templates/sts.yaml (1)
39-47: Consider enabling read-only root filesystemThe security context follows best practices by running as non-root and dropping capabilities. However, consider enabling
readOnlyRootFilesystem: truefor enhanced security if Keycloak doesn't require writing to the root filesystem.securityContext: runAsNonRoot: true runAsUser: 1000 capabilities: drop: - ALL - CAP_NET_RAW - readOnlyRootFilesystem: false + readOnlyRootFilesystem: true allowPrivilegeEscalation: falsepackages/core/platform/bundles/paas-hosted.yaml (1)
148-159: Consider adding descriptive comments.To improve maintainability, consider adding comments explaining:
- The purpose of the Keycloak deployment
- Any specific configuration requirements
- Links to relevant documentation
Example addition:
+# PostgreSQL instance dedicated to Keycloak - name: postgres-instance releaseName: postgres-instance chart: cozy-postgres-instance namespace: cozy-keycloak dependsOn: [postgres-operator] +# Keycloak identity and access management service +# Documentation: https://www.keycloak.org/documentation - name: keycloak releaseName: keycloak chart: cozy-keycloak namespace: cozy-keycloak dependsOn: [postgres-instance]packages/core/platform/bundles/distro-full.yaml (1)
177-188: LGTM! Consider adding documentationThe additions follow the existing patterns and establish correct dependencies. The changes are well-structured and properly integrated into the platform configuration.
Consider adding documentation about:
- The purpose of adding Keycloak to the platform
- Configuration requirements for different environments
- Integration points with other platform components
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
.pre-commit-config.yaml(0 hunks)packages/apps/tenant/templates/tenant.yaml(1 hunks)packages/core/platform/bundles/distro-full.yaml(1 hunks)packages/core/platform/bundles/distro-hosted.yaml(1 hunks)packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(2 hunks)packages/system/keycloak/Chart.yaml(1 hunks)packages/system/keycloak/templates/ingress.yaml(1 hunks)packages/system/keycloak/templates/init-script.yaml(1 hunks)packages/system/keycloak/templates/service-headless.yaml(1 hunks)packages/system/keycloak/templates/service.yaml(1 hunks)packages/system/keycloak/templates/sts.yaml(1 hunks)packages/system/keycloak/values.yaml(1 hunks)packages/system/postgres-instance/Chart.yaml(1 hunks)packages/system/postgres-instance/templates/cluster.yaml(1 hunks)packages/system/postgres-instance/templates/init-job.yaml(1 hunks)packages/system/postgres-instance/templates/init-script.yaml(1 hunks)packages/system/postgres-instance/values.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
✅ Files skipped from review due to trivial changes (2)
- packages/system/keycloak/Chart.yaml
- packages/system/postgres-instance/Chart.yaml
🧰 Additional context used
🪛 yamllint
packages/system/keycloak/templates/ingress.yaml
[error] 6-6: syntax error: could not find expected ':'
(syntax)
packages/system/keycloak/templates/sts.yaml
[error] 28-28: syntax error: expected , but found '{'
(syntax)
[warning] 31-31: wrong indentation: expected 10 but found 12
(indentation)
packages/system/postgres-instance/templates/cluster.yaml
[error] 14-14: syntax error: could not find expected ':'
(syntax)
packages/system/postgres-instance/templates/init-job.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
packages/system/postgres-instance/templates/init-script.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (20)
packages/system/postgres-instance/values.yaml (2)
2-2: Verify PostgreSQL replication configuration
Setting replicas: 2 indicates a high-availability setup. Ensure that:
- The PostgreSQL CRD/operator supports this replication configuration
- Network policies and services are properly configured for primary-replica communication
#!/bin/bash
# Check for PostgreSQL operator configuration and network policies
rg -l 'kind:\s*NetworkPolicy.*postgres|postgres.*NetworkPolicy'
# Look for service definitions that handle primary/replica routing
rg -l 'kind:\s*Service.*postgres|postgres.*Service'1-1: Review storage configuration settings
The storage configuration needs attention:
- Consider if 10Gi is sufficient for your use case, especially with Keycloak's data
- Empty storageClass might default to the cluster's default storage class, which may not be optimal for database workloads
Also applies to: 3-3
packages/system/keycloak/templates/service.yaml (2)
11-12: Verify selector consistency with StatefulSet/Deployment labels.
The selector app: keycloak-ha suggests a high-availability setup. Ensure this label matches the ones defined in your StatefulSet or Deployment manifest.
#!/bin/bash
# Check if the StatefulSet/Deployment uses matching labels
rg -A 5 'kind:\s*(StatefulSet|Deployment).*keycloak' packages/system/keycloak/templates/ | rg -A 5 'labels:'1-6: Consider using LoadBalancer or NodePort type for external access.
The Service is configured with ClusterIP type, which only allows internal cluster access. Since Keycloak typically needs to be accessed by external clients, consider using LoadBalancer or NodePort type depending on your infrastructure requirements.
packages/system/postgres-instance/templates/cluster.yaml (2)
11-18: Verify the implications of API server access label.
The label policy.cozystack.io/allow-to-apiserver: "true" grants API server access. This requires careful consideration:
- Ensure this aligns with your security policies
- Verify if this level of access is necessary for the PostgreSQL cluster
The storage configuration looks good with proper validation for required fields.
Let's check how this label is used across the cluster:
#!/bin/bash
# Check for other resources using this label and related RBAC policies
rg -l "policy\.cozystack\.io/allow-to-apiserver"
rg -l "ClusterRole|Role|RoleBinding|ClusterRoleBinding" | xargs rg "apiserver"🧰 Tools
🪛 yamllint
[error] 14-14: syntax error: could not find expected ':'
(syntax)
7-10:
Security: Review superuser access configuration.
While enableSuperuserAccess: true might be required for initial setup or maintenance, it poses security risks:
- It creates a superuser role with full database privileges
- Consider restricting this in production environments
Let's check if there are any security controls or documentation addressing this:
Consider the following improvements:
- Add conditional enabling based on environment:
- enableSuperuserAccess: true
+ enableSuperuserAccess: {{ .Values.enableSuperuser | default false }}- Document the security implications in values.yaml
packages/system/keycloak/templates/ingress.yaml (1)
19-25: Verify backend service configuration
The backend service configuration points to keycloak-http service. Please ensure:
- The service name matches the Keycloak service template
- The HTTP port name is correctly defined in the service
✅ Verification successful
Backend service configuration is correctly defined
The verification confirms that:
- The service name
keycloak-httpmatches between the ingress and service templates - The HTTP port is correctly defined in the service template with:
- Port name:
http - Port number: 8080
- Protocol: TCP
- Port name:
The configuration is consistent across all related templates (ingress, service, and statefulset).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify service template configuration
# Check if service template exists and uses matching port name
echo "Checking service template configuration..."
grep -r "name: keycloak-http" packages/system/keycloak/templates/
grep -r "port.*name: http" packages/system/keycloak/templates/
# Check for potential service name template variables
echo "Checking for service name variables..."
grep -r "name:.*keycloak.*service" packages/system/keycloak/templates/
Length of output: 537
Script:
#!/bin/bash
# Let's check the service.yaml file content to verify port configuration
echo "Checking service.yaml content..."
cat packages/system/keycloak/templates/service.yaml
# Also check if there are any other port configurations in the templates
echo -e "\nChecking other port configurations..."
grep -r "port:" packages/system/keycloak/templates/ -A 3
Length of output: 1449
packages/system/keycloak/values.yaml (2)
18-23: Review resource allocation configuration
Several concerns with the current resource configuration:
- Missing CPU limit could lead to resource abuse
- The memory limit to request ratio (1500Mi:500Mi) is quite high
- These values should be verified against Keycloak's recommended specifications
Consider adding CPU limits and adjusting memory configuration:
resources:
limits:
memory: 1500Mi
+ cpu: 500m
requests:
memory: 500Mi
cpu: 100m1-1: Verify Keycloak version for security updates
The current version is 26.0.4. Let's verify if this is the latest stable version.
packages/system/postgres-instance/templates/init-job.yaml (4)
10-14: LGTM! Good use of checksum annotation
The checksum annotation ensures the Job pod is recreated when the initialization script changes, which is a good practice for configuration management.
40-59: LGTM! Excellent security configuration
The security contexts are well-configured with:
- Non-root execution
- Read-only root filesystem
- Dropped capabilities
- Runtime default seccomp profile
60-66: Verify the referenced secrets and initialization script
The Job depends on two secrets:
{{ .Release.Name }}-superuser{{ .Release.Name }}-init-script
Let's verify their configuration and the content of the initialization script.
✅ Verification successful
Both referenced secrets are properly defined and configured
The verification shows that both secrets referenced in the volume mounts are correctly defined:
{{ .Release.Name }}-superuser- This is a built-in secret automatically created by the CloudNativePG operator for superuser access{{ .Release.Name }}-init-script- This secret is defined in the same directory (init-script.yaml) and contains:- A safe initialization script that creates the database and user with proper permissions
- No sensitive operations beyond the expected database setup
- Proper security practices (using
ON_ERROR_STOP=1, parameterized queries)
The volume mounts and their usage in the init job follow security best practices:
- Read-only mounts
- Non-root execution
- Strict security context
- Limited capabilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for secret definitions and init script content
# Look for secret definitions and any sensitive operations in the init script
# Check for secret definitions
echo "Checking for superuser secret definition..."
rg -l "kind:\s+Secret.*{{ .Release.Name }}-superuser" --type yaml
echo "Checking for init-script secret definition..."
rg -l "kind:\s+Secret.*{{ .Release.Name }}-init-script" --type yaml
# Look for init script content
echo "Checking init script content for sensitive operations..."
rg -A 5 "data:.*init.sh" --type yaml
Length of output: 789
Script:
#!/bin/bash
# Description: Check for secret definitions and init script content with escaped template variables
# Look for secret definitions and any sensitive operations in the init script
# Check for secret definitions
echo "Checking for superuser secret definition..."
rg -l "kind:\s+Secret.*superuser" --type yaml
echo "Checking for init-script secret definition..."
rg -l "kind:\s+Secret.*init-script" --type yaml
# Look for init script content and related configurations
echo "Checking init script and secret configurations..."
rg -A 5 "kind:\s+Secret" --type yaml
# Look specifically in the postgres-instance directory
echo "Checking postgres-instance directory..."
fd . packages/system/postgres-instance -t f -e yaml -x cat {}
Length of output: 64929
6-7: Verify the hook weight value
The hook weight of -5 determines the execution order relative to other hooks. Please ensure this weight allows proper sequencing with other components, especially considering dependencies with Keycloak.
✅ Verification successful
Hook weight of -5 is consistent with other initialization jobs
The hook weight of -5 is appropriate as it aligns with similar initialization jobs across the codebase. Several other components use the same weight for their post-install/upgrade initialization jobs:
- Other database initialization jobs (postgres, ferretdb)
- Telepresence hooks
- SeaweedFS bucket initialization
- Cert-manager initialization
The weight ensures this initialization runs before other components (positive weights) but after critical infrastructure setup (lower negative weights), which is the desired sequencing for database initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other Helm hooks and their weights
# Look for other hook weights to ensure proper ordering
rg -A 2 'helm.sh/hook-weight' --type yaml
Length of output: 18069
packages/system/keycloak/templates/sts.yaml (3)
91-102: Verify probe timing configuration
The initial delay seconds (120s for liveness, 60s for readiness) are quite high. While this might be necessary for Keycloak startup, please verify these values are optimal for your environment to ensure faster pod availability.
#!/bin/bash
# Search for similar Keycloak deployments in the codebase for probe timing reference
rg "initialDelaySeconds" packages/system/103-114: Verify init-realm.sh script and ConfigMap
The StatefulSet depends on a ConfigMap named keycloak-init-script containing an initialization script. Ensure this ConfigMap is properly defined and the script is idempotent since it runs on pod restarts.
#!/bin/bash
# Check for the ConfigMap definition
rg -l "kind: ConfigMap.*keycloak-init-script" packages/system/keycloak/templates/5-10: Verify label consistency across related resources
The StatefulSet uses the label app: keycloak-ha. Ensure this label is consistently used across related resources (Service, Ingress, etc.) for proper service discovery and networking.
✅ Verification successful
Labels are consistent across all Keycloak resources
The label app: keycloak-ha is consistently used across all related resources:
- StatefulSet (both in metadata and selector)
- Headless Service (selector)
- HTTP Service (selector)
The Ingress doesn't require the label as it connects to the service by name. The ConfigMap doesn't need the label as it's mounted directly by name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check label consistency across related Keycloak resources
rg -l "app: keycloak" packages/system/keycloak/templates/
Length of output: 176
Script:
#!/bin/bash
# Check for all YAML files in the keycloak templates directory and their content
fd -e yaml -e yml . packages/system/keycloak/templates/ --exec cat {}
Length of output: 5229
packages/core/platform/bundles/paas-hosted.yaml (1)
155-159: Consider additional dependencies for Keycloak.
While the dependency on postgres-instance is correct, Keycloak typically requires additional components for production deployment:
- cert-manager for TLS certificates
- external-dns if automatic DNS management is needed (marked as optional in the bundle)
Let's verify the Keycloak chart's ingress and TLS configurations:
✅ Verification successful
Let's verify if the Keycloak chart has proper ingress and TLS configurations by checking the chart's values and templates:
The Keycloak configuration already includes proper TLS and cert-manager integration
The review comment's concerns about cert-manager dependencies are already addressed in the configuration:
-
TLS certificates: The Keycloak chart is properly configured with cert-manager integration through:
- cert-manager.io/cluster-issuer: letsencrypt-prod annotation
- acme.cert-manager.io/http01-ingress-class annotation
- TLS configuration in the ingress template with secretName: web-tls
-
DNS management: While external-dns was mentioned as optional in the review comment, it's not a strict requirement and can be handled through manual DNS configuration if needed.
The current configuration follows best practices for securing Keycloak with TLS certificates managed by cert-manager.
[incorrect_review_comment]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Keycloak chart includes proper ingress and TLS configurations
# Look for ingress and certificate configurations
fd -e yaml -e tpl . | xargs rg -l "ingress:|tls:|certificate:" | while read -r file; do
echo "=== $file ==="
rg "ingress:|tls:|certificate:" -A 10 "$file"
done
# Check if cert-manager is referenced in the chart
rg -l "cert-manager.io"
Length of output: 151175
Script:
#!/bin/bash
# Check Keycloak chart's ingress and TLS configurations
fd -e yaml . packages/system/keycloak/ | xargs rg -A 10 "ingress:|tls:|certificate:|cert-manager"
Length of output: 2008
packages/core/platform/bundles/distro-full.yaml (2)
178-182: Verify PostgreSQL instance configuration for production readiness
While the basic configuration is correct, consider verifying if additional production-ready configurations are needed, such as:
- Resource requests and limits
- Backup configurations
- High availability settings
- Storage class specifications
Let's check if these configurations are defined in the chart:
184-188: Verify Keycloak security and ingress configurations
As Keycloak is a critical authentication component, please ensure:
- TLS is properly configured
- Ingress is secured
- Authentication and authorization policies are defined
- Resource limits are set to prevent DoS
Let's verify these configurations:
✅ Verification successful
Let me gather more specific information about the Keycloak configuration.
Let me try a different approach to locate and analyze the Keycloak configurations.
Let me check the specific Keycloak chart templates and values to verify the security configurations.
Security configurations are properly implemented
The Keycloak deployment has appropriate security measures in place:
- TLS is configured via ingress with Let's Encrypt certificates
- Security context is properly set:
- Non-root user (UID 1000)
- Dropped capabilities
- Read-only root filesystem
- Privilege escalation disabled
- Resource limits are defined to prevent DoS
- Session affinity is configured with cookie-based persistence
- Proper health checks are implemented
Additional security features:
- Random admin password generation
- Secure database credentials handling via secrets
- Edge proxy configuration
- HTTPS redirect enabled via ingress annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security-related configurations in the Keycloak chart
echo "Checking for security configurations in Keycloak chart..."
fd -t f "values.yaml" -x grep -l "tls\|ingress\|auth\|resources" {}
# Check for any security-related Kubernetes resources
echo "Checking for associated security resources..."
rg -l "kind: (NetworkPolicy|PodSecurityPolicy|SecurityContextConstraints)"
Length of output: 8812
Script:
#!/bin/bash
# Check for cozy-keycloak chart and its configurations
echo "Looking for cozy-keycloak chart..."
fd -t f -e yaml -e yml "cozy-keycloak"
# Check values.yaml in keycloak related directories
echo "Checking keycloak values and templates..."
fd -t f "values.yaml" -p ".*keycloak.*" -x cat {}
# Check for keycloak ingress configurations
echo "Checking for Keycloak ingress configurations..."
rg -A 10 "kind: Ingress" -g "*keycloak*"
# Check for keycloak security configurations
echo "Checking for Keycloak security configurations..."
rg "securityContext|resources|tls|auth" -g "*keycloak*"
Length of output: 973
Script:
#!/bin/bash
# Search for any keycloak related files
echo "Looking for Keycloak related files..."
fd -t f "keycloak"
# Search in the platform bundles directory for keycloak configurations
echo "Checking platform bundles for Keycloak configs..."
rg -C 5 "keycloak" "packages/core/platform/bundles/"
# Search for chart configurations
echo "Looking for chart configurations..."
fd -t f "Chart.yaml" -x grep -l "keycloak" {}
# Check for any helm templates related to keycloak
echo "Checking for Helm templates..."
fd -t f -e yaml -e yml -p "templates" -x grep -l "keycloak" {}
Length of output: 4625
Script:
#!/bin/bash
# Check Keycloak values
echo "Checking Keycloak values.yaml..."
cat packages/system/keycloak/values.yaml
# Check Keycloak templates
echo "Checking Keycloak ingress configuration..."
cat packages/system/keycloak/templates/ingress.yaml
echo "Checking Keycloak statefulset configuration..."
cat packages/system/keycloak/templates/sts.yaml
# Check Chart.yaml for dependencies
echo "Checking Chart.yaml..."
cat packages/system/keycloak/Chart.yaml
Length of output: 5298
packages/core/platform/bundles/paas-full.yaml (1)
259-263: LGTM! Keycloak configuration looks correct.
The keycloak release is properly configured with the correct dependency chain through postgres-instance.
Let's verify the chart and dependency setup:
✅ Verification successful
Keycloak configuration and dependencies are correctly set up
The verification confirms:
- The
cozy-keycloakchart exists and is properly defined - The postgres-instance dependency is correctly configured in the same namespace (cozy-keycloak)
- Keycloak is properly configured to use postgres with the correct connection details
- No circular dependencies were found
- The postgres-instance chart exists across multiple bundle configurations, maintaining consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify chart existence and dependencies
# Check if the chart exists
fd -t f "Chart.yaml" -x grep -l "name: cozy-keycloak" {}
# Look for any potential circular dependencies
rg "dependsOn.*keycloak"
Length of output: 217
Script:
#!/bin/bash
# Let's verify the chart details and dependencies more thoroughly
# Check the chart content
cat ./packages/system/keycloak/Chart.yaml
# Check for any references to postgres in the chart or values
rg -A 3 "postgres" ./packages/system/keycloak/
# Look for circular dependencies with proper quoting
rg 'dependsOn:.*keycloak'
# Check if postgres-instance exists as a chart or release
fd -t f "Chart.yaml" -x grep -l "name: postgres" {}
rg 'name: postgres-instance' -A 3
Length of output: 2512
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: keycloak-headless | ||
| spec: | ||
| type: ClusterIP | ||
| clusterIP: None | ||
| selector: | ||
| app: keycloak-ha |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential service configurations
The headless service definition needs the following essential improvements:
- Port definitions for the Keycloak service
- Namespace specification
Apply this diff:
apiVersion: v1
kind: Service
metadata:
name: keycloak-headless
+ namespace: {{ .Release.Namespace }}
spec:
type: ClusterIP
clusterIP: None
selector:
app: keycloak-ha
+ ports:
+ - name: http
+ port: 8080
+ targetPort: http
+ - name: https
+ port: 8443
+ targetPort: https📝 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.
| apiVersion: v1 | |
| kind: Service | |
| metadata: | |
| name: keycloak-headless | |
| spec: | |
| type: ClusterIP | |
| clusterIP: None | |
| selector: | |
| app: keycloak-ha | |
| apiVersion: v1 | |
| kind: Service | |
| metadata: | |
| name: keycloak-headless | |
| namespace: {{ .Release.Namespace }} | |
| spec: | |
| type: ClusterIP | |
| clusterIP: None | |
| selector: | |
| app: keycloak-ha | |
| ports: | |
| - name: http | |
| port: 8080 | |
| targetPort: http | |
| - name: https | |
| port: 8443 | |
| targetPort: https |
| tls: | ||
| - hosts: | ||
| - {{ .Values.ingress.host }} | ||
| secretName: web-tls |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding the TLS secret name
The TLS secret name web-tls is hardcoded. This reduces flexibility and may cause issues in different environments.
Make the TLS secret name configurable:
tls:
- hosts:
- {{ .Values.ingress.host }}
- secretName: web-tls
+ secretName: {{ .Values.ingress.tls.secretName | default "web-tls" }}📝 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.
| tls: | |
| - hosts: | |
| - {{ .Values.ingress.host }} | |
| secretName: web-tls | |
| tls: | |
| - hosts: | |
| - {{ .Values.ingress.host }} | |
| secretName: {{ .Values.ingress.tls.secretName | default "web-tls" }} |
packages/system/keycloak/values.yaml
Outdated
| @@ -0,0 +1,23 @@ | |||
| image: quay.io/keycloak/keycloak:26.0.4 | |||
| host: keycloak.infra.aenix.org | |||
There was a problem hiding this comment.
Security concern: Remove production domain from version control
The host value keycloak.infra.aenix.org appears to be a production domain. This should be parameterized and moved to a secure configuration management system.
-host: keycloak.infra.aenix.org
+host: "${KEYCLOAK_HOST}"📝 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.
| host: keycloak.infra.aenix.org | |
| host: "${KEYCLOAK_HOST}" |
packages/system/keycloak/values.yaml
Outdated
| host: keycloak.example.com | ||
| issuer: letsencrypt-prod | ||
| ingressClassName: tenent-root |
There was a problem hiding this comment.
Fix configuration issues in ingress setup
- The ingress host uses example.com domain
- There's a typo in
ingressClassName: "tenent-root" should be "tenant-root"
- host: keycloak.example.com
+ host: "${KEYCLOAK_INGRESS_HOST}"
- ingressClassName: tenent-root
+ ingressClassName: tenant-root📝 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.
| host: keycloak.example.com | |
| issuer: letsencrypt-prod | |
| ingressClassName: tenent-root | |
| host: "${KEYCLOAK_INGRESS_HOST}" | |
| issuer: letsencrypt-prod | |
| ingressClassName: tenant-root |
| nginx.ingress.kubernetes.io/affinity: "cookie" | ||
| nginx.ingress.kubernetes.io/session-cookie-expires: "86400" | ||
| nginx.ingress.kubernetes.io/session-cookie-max-age: "86400" | ||
| nginx.ingress.kubernetes.io/session-cookie-name: "keycloak-cookie" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cookie security configuration
The current cookie configuration lacks security-related settings. Consider adding secure flags and SameSite policy.
nginx.ingress.kubernetes.io/affinity: "cookie"
nginx.ingress.kubernetes.io/session-cookie-expires: "86400"
nginx.ingress.kubernetes.io/session-cookie-max-age: "86400"
nginx.ingress.kubernetes.io/session-cookie-name: "keycloak-cookie"
+ nginx.ingress.kubernetes.io/session-cookie-secure: "true"
+ nginx.ingress.kubernetes.io/session-cookie-samesite: "Strict"📝 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.
| nginx.ingress.kubernetes.io/affinity: "cookie" | |
| nginx.ingress.kubernetes.io/session-cookie-expires: "86400" | |
| nginx.ingress.kubernetes.io/session-cookie-max-age: "86400" | |
| nginx.ingress.kubernetes.io/session-cookie-name: "keycloak-cookie" | |
| nginx.ingress.kubernetes.io/affinity: "cookie" | |
| nginx.ingress.kubernetes.io/session-cookie-expires: "86400" | |
| nginx.ingress.kubernetes.io/session-cookie-max-age: "86400" | |
| nginx.ingress.kubernetes.io/session-cookie-name: "keycloak-cookie" | |
| nginx.ingress.kubernetes.io/session-cookie-secure: "true" | |
| nginx.ingress.kubernetes.io/session-cookie-samesite: "Strict" |
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: Role | ||
| metadata: | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| namespace: cozy-public | ||
| rules: | ||
| - apiGroups: ["apps.cozystack.io/v1alpha1"] | ||
| resources: ["*"] | ||
| verbs: ["get", "list", "watch"] | ||
| - apiGroups: ["source.toolkit.fluxcd.io"] | ||
| resources: ["helm.toolkit.fluxcd.io/v2"] | ||
| verbs: ["get", "list", "watch"] | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| namespace: cozy-public | ||
| subjects: | ||
| - kind: Group | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| apiGroup: rbac.authorization.k8s.io | ||
| roleRef: | ||
| kind: Role | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency with read-write role changes
While the read-only permissions are appropriately restricted, the role configuration should be consistent with the changes suggested for the read-write role.
Apply these changes:
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: cozy:{{ include "tenant.name" . }}-ro
namespace: cozy-public
rules:
- apiGroups: ["apps.cozystack.io/v1alpha1"]
- resources: ["*"]
+ resources: ["keycloaks", "postgresqls"]
verbs: ["get", "list", "watch"]
- apiGroups: ["source.toolkit.fluxcd.io"]
- resources: ["helm.toolkit.fluxcd.io/v2"]
+ resources: ["helmcharts", "helmrepositories"]
verbs: ["get", "list", "watch"]
+- apiGroups: ["helm.toolkit.fluxcd.io"]
+ resources: ["helmreleases"]
+ verbs: ["get", "list", "watch"]📝 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.
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: Role | |
| metadata: | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| namespace: cozy-public | |
| rules: | |
| - apiGroups: ["apps.cozystack.io/v1alpha1"] | |
| resources: ["*"] | |
| verbs: ["get", "list", "watch"] | |
| - apiGroups: ["source.toolkit.fluxcd.io"] | |
| resources: ["helm.toolkit.fluxcd.io/v2"] | |
| verbs: ["get", "list", "watch"] | |
| --- | |
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: RoleBinding | |
| metadata: | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| namespace: cozy-public | |
| subjects: | |
| - kind: Group | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| apiGroup: rbac.authorization.k8s.io | |
| roleRef: | |
| kind: Role | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| apiGroup: rbac.authorization.k8s.io | |
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: Role | |
| metadata: | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| namespace: cozy-public | |
| rules: | |
| - apiGroups: ["apps.cozystack.io/v1alpha1"] | |
| resources: ["keycloaks", "postgresqls"] | |
| verbs: ["get", "list", "watch"] | |
| - apiGroups: ["source.toolkit.fluxcd.io"] | |
| resources: ["helmcharts", "helmrepositories"] | |
| verbs: ["get", "list", "watch"] | |
| - apiGroups: ["helm.toolkit.fluxcd.io"] | |
| resources: ["helmreleases"] | |
| verbs: ["get", "list", "watch"] | |
| --- | |
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: RoleBinding | |
| metadata: | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| namespace: cozy-public | |
| subjects: | |
| - kind: Group | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| apiGroup: rbac.authorization.k8s.io | |
| roleRef: | |
| kind: Role | |
| name: cozy:{{ include "tenant.name" . }}-ro | |
| apiGroup: rbac.authorization.k8s.io |
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: Role | ||
| metadata: | ||
| name: cozy:{{ include "tenant.name" . }}-rw | ||
| namespace: cozy-public | ||
| rules: | ||
| - apiGroups: ["apps.cozystack.io/v1alpha1"] | ||
| resources: ["*"] | ||
| verbs: ["*"] | ||
| - apiGroups: ["source.toolkit.fluxcd.io"] | ||
| resources: ["helm.toolkit.fluxcd.io/v2"] | ||
| verbs: ["*"] | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: cozy:{{ include "tenant.name" . }}-rw | ||
| namespace: cozy-public | ||
| subjects: | ||
| - kind: Group | ||
| name: cozy:{{ include "tenant.name" . }}-rw | ||
| apiGroup: rbac.authorization.k8s.io | ||
| roleRef: | ||
| kind: Role | ||
| name: cozy:{{ include "tenant.name" . }}-rw | ||
| apiGroup: rbac.authorization.k8s.io | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: Role | ||
| metadata: | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| namespace: cozy-public | ||
| rules: | ||
| - apiGroups: ["apps.cozystack.io/v1alpha1"] | ||
| resources: ["*"] | ||
| verbs: ["get", "list", "watch"] | ||
| - apiGroups: ["source.toolkit.fluxcd.io"] | ||
| resources: ["helm.toolkit.fluxcd.io/v2"] | ||
| verbs: ["get", "list", "watch"] | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| namespace: cozy-public | ||
| subjects: | ||
| - kind: Group | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| apiGroup: rbac.authorization.k8s.io | ||
| roleRef: | ||
| kind: Role | ||
| name: cozy:{{ include "tenant.name" . }}-ro | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a definitive response about the RBAC configuration:
RBAC configuration needs correction for FluxCD resources
The current RBAC rules have two issues:
-
The API group
apps.cozystack.io/v1alpha1is not defined in any CRD in the codebase and should be removed. -
The FluxCD related rule is incorrectly specified:
- apiGroups: ["source.toolkit.fluxcd.io"]
resources: ["helm.toolkit.fluxcd.io/v2"] # incorrect
verbs: ["*"]Should be split into two separate rules based on actual FluxCD API groups used in the codebase:
- apiGroups: ["source.toolkit.fluxcd.io"]
resources: ["helmrepositories", "helmcharts"]
verbs: ["*"]
- apiGroups: ["helm.toolkit.fluxcd.io"]
resources: ["helmreleases"]
verbs: ["*"]🔗 Analysis chain
Verify RBAC requirements for Keycloak deployment
Let's verify if these RBAC configurations align with typical Keycloak deployment patterns and if all API groups/resources exist in the cluster.
Let me analyze the results from the shell scripts to verify the RBAC configuration.
The search results show that:
-
The API group
apps.cozystack.io/v1alpha1is only referenced in the tenant.yaml file and no corresponding CRD definition was found in the codebase. -
The API group
source.toolkit.fluxcd.iowith resourcehelm.toolkit.fluxcd.io/v2also appears to be incorrectly specified - these seem to be mixing two different API groups.
Let me verify the correct FluxCD API groups with another search:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify RBAC configuration for Keycloak deployment
# Check for similar RBAC patterns in other Keycloak-related files
echo "Checking for similar RBAC patterns..."
rg -l "apps.cozystack.io/v1alpha1" --type yaml
# Check for API group usage
echo "Checking API group usage..."
rg "apps.cozystack.io/v1alpha1" --type yaml -A 5 -B 5
# Look for CRD definitions to verify resources
echo "Checking for CRD definitions..."
fd -e yaml | xargs rg "kind: CustomResourceDefinition" -A 10
Length of output: 202688
Script:
#!/bin/bash
# Check for FluxCD related CRDs and API groups
echo "Checking for FluxCD API groups..."
rg "source.toolkit.fluxcd.io|helm.toolkit.fluxcd.io" --type yaml -A 5 -B 5
Length of output: 39191
| - name: postgres-instance | ||
| releaseName: postgres-instance | ||
| chart: cozy-postgres-instance | ||
| namespace: cozy-keycloak | ||
| dependsOn: [postgres-operator] |
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the code and configuration files, I can now provide a final response:
PostgreSQL instance lacks resource requirements specification
The PostgreSQL instance configuration in postgres-instance chart only specifies storage size (10Gi) and replicas (2), but lacks CPU and memory resource requirements. While the operator allows setting resource limits and requests, no defaults are configured. This could lead to:
- Unbounded resource consumption
- Potential performance issues under load
- Resource contention with other workloads
Recommendations:
- Define resource requests and limits for the PostgreSQL instances
- Consider adding the following to the cluster spec:
resources:
requests:
cpu: 200m
memory: 256Mi
limits:
cpu: 1000m
memory: 1Gi🔗 Analysis chain
LGTM! Verify PostgreSQL resource requirements.
The postgres-instance configuration follows the established patterns and has correct dependencies. The placement in the cozy-keycloak namespace maintains good resource isolation.
Let's verify the PostgreSQL chart's resource definitions:
Let me analyze the review comment and the results from the shell scripts.
The review comment suggests verifying PostgreSQL resource requirements for the postgres-instance release. From the search results, I can see:
- The
postgres-instancechart inpackages/core/platform/bundles/paas-hosted.yamlreferencescozy-postgres-instancechart - The
packages/system/postgres-instance/templates/cluster.yamlshows that only storage size and storage class are configurable parameters - No explicit resource requirements are defined in the chart
Let me verify if there are any default resource requirements defined in the PostgreSQL operator that would apply to this instance.
Let me check the actual PostgreSQL instance configuration to verify resource requirements.
Let me try a different approach to find the PostgreSQL instance configuration.
Let me check the default resource requirements from the PostgreSQL operator's default configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the PostgreSQL chart defines appropriate resource requests/limits
# and storage configurations that are compatible with the cluster's capabilities
# Look for resource specifications in the chart
fd -e yaml -e tpl . | xargs rg -l "resources:|storage:|volumeClaimTemplates:" | while read -r file; do
echo "=== $file ==="
rg "resources:|storage:|volumeClaimTemplates:" -A 5 "$file"
done
Length of output: 370488
Script:
#!/bin/bash
# Check for default resource requirements in the PostgreSQL operator configuration
rg -A 10 "resources:|defaultResources:" packages/system/postgres-operator/
Length of output: 61207
Script:
#!/bin/bash
# Check the postgres-instance chart and cluster configuration
rg -A 10 "resources:|limits:|requests:" packages/system/postgres-instance/
Length of output: 76
Script:
#!/bin/bash
# Look for the postgres instance chart and its values
fd -t f . packages/system/postgres-instance/ -x cat {}
Length of output: 3287
Script:
#!/bin/bash
# Check the PostgreSQL operator's default configuration for resource requirements
rg -A 5 "defaultResources:|resources:" packages/system/postgres-operator/charts/cloudnative-pg/values.yaml
Length of output: 335
| - name: postgres-instance | ||
| releaseName: postgres-instance | ||
| chart: cozy-postgres-instance | ||
| namespace: cozy-keycloak | ||
| dependsOn: [postgres-operator] | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Chart name mismatch detected in postgres-instance configuration
The chart name in the bundle (cozy-postgres-instance) doesn't match the actual chart name in the repository (cozy-postgres-keycloak). This needs to be corrected to avoid deployment failures.
- In
packages/core/platform/bundles/paas-full.yaml: Changechart: cozy-postgres-instancetochart: cozy-postgres-keycloak
🔗 Analysis chain
LGTM! Postgres instance configuration looks correct.
The postgres-instance release is properly configured with the correct dependency on postgres-operator and appropriate namespace placement.
Let's verify the chart and namespace setup:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify chart existence and namespace configuration
# Check if the chart exists
fd -t f "Chart.yaml" -x grep -l "name: cozy-postgres-instance" {}
# Check for namespace-related resources
rg -l "namespace: cozy-keycloak"
Length of output: 217
Script:
#!/bin/bash
# Let's verify with more specific checks
# Check for postgres-instance chart
fd -t f "Chart.yaml" -x grep -l "name: cozy-postgres-instance" {}
# Look for any postgres-instance related files
fd "postgres-instance"
# Check for namespace definitions in platform bundles
rg "namespace: cozy-keycloak" packages/core/platform/bundles/
# Look for any keycloak namespace references in k8s resources
fd -e yaml -e yml | xargs grep -l "namespace: cozy-keycloak"
Length of output: 1044
Script:
#!/bin/bash
# Let's check the postgres-instance chart structure and configuration
# Check if postgres-instance chart exists and its structure
ls -R packages/system/postgres-instance/
# Check for any values files that might contain default configurations
fd -t f "values.yaml" packages/system/postgres-instance/
# Check for any Chart.yaml in postgres-instance directory
cat packages/system/postgres-instance/Chart.yaml 2>/dev/null
# Check if postgres-operator is properly defined in the bundle
rg "name: postgres-operator" packages/core/platform/bundles/paas-full.yaml -B 2 -A 2
Length of output: 747
|
|
||
| - name: postgres-instance | ||
| releaseName: postgres-instance | ||
| chart: cozy-postgres-instance | ||
| namespace: cozy-keycloak | ||
| dependsOn: [postgres-operator] | ||
|
|
||
| - name: keycloak | ||
| releaseName: keycloak | ||
| chart: cozy-keycloak | ||
| namespace: cozy-keycloak | ||
| dependsOn: [postgres-instance] |
There was a problem hiding this comment.
💡 Codebase verification
Namespace and RBAC resources need to be defined for cozy-keycloak namespace
The verification shows that neither the cozy-keycloak nor cozy-postgres-instance charts are present in the repository, and there's no explicit namespace initialization or RBAC setup for the cozy-keycloak namespace. To ensure proper operation:
- Create a namespace manifest for
cozy-keycloak - Define ServiceAccount, Role, and RoleBinding for both services
- Consider adding NetworkPolicy resources to control traffic
Example locations to add these resources:
packages/core/platform/templates/namespaces.yamlfor namespace definition- Chart-specific templates for RBAC resources
- Chart-specific templates for network policies
🔗 Analysis chain
Consider namespace initialization and RBAC setup.
Both services are configured in the new 'cozy-keycloak' namespace. Ensure that:
- Namespace initialization is handled (typically via helm pre-install hooks)
- Appropriate RBAC resources (ServiceAccount, Role, RoleBinding) are defined
- Network policies are configured to control traffic to/from this namespace
Let's check for namespace and RBAC setup:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify namespace and RBAC configuration
# Look for namespace definition
rg "kind: Namespace.*cozy-keycloak"
# Check for RBAC resources
rg "kind: (Role|RoleBinding|ServiceAccount).*namespace: cozy-keycloak"
# Look for NetworkPolicy resources
rg "kind: NetworkPolicy.*namespace: cozy-keycloak"
Length of output: 163
Script:
#!/bin/bash
# Let's check the chart contents for namespace, RBAC, and network policies
# Check if the charts exist and their contents
fd "cozy-keycloak|cozy-postgres-instance" --type f
# Look for any helm templates or yaml files that might contain namespace definitions
fd "\.ya?ml$|\.tpl$" --type f | xargs rg -l "namespace:|kind: Namespace"
# Look for RBAC-related resources in yaml files
fd "\.ya?ml$|\.tpl$" --type f | xargs rg -l "kind: (Role|RoleBinding|ServiceAccount|NetworkPolicy)"
Length of output: 70070
Script:
#!/bin/bash
# Let's check the specific charts mentioned in the review for namespace and RBAC setup
# Check cozy-keycloak chart contents
fd "cozy-keycloak" --type f
# Check cozy-postgres-instance chart contents
fd "cozy-postgres-instance" --type f
# Check if there are any namespace initialization or RBAC resources in the platform bundle
rg -A 10 "cozy-keycloak" packages/core/platform/bundles/paas-full.yaml
Length of output: 320
| @@ -174,3 +174,15 @@ releases: | |||
| namespace: cozy-external-secrets-operator | |||
| optional: true | |||
| dependsOn: [cilium] | |||
|
|
|||
| - name: postgres-instance | |||
There was a problem hiding this comment.
Let's move it into the same chart with keycloak or renable to keycloak-db
There was a problem hiding this comment.
Let's use default CNPG resource, without custom chart. Here is example for grafana:
| kind: Ingress | ||
| metadata: | ||
| name: keycloak-ingress | ||
| {{- with .Values.ingress.Annotations }} |
There was a problem hiding this comment.
| {{- with .Values.ingress.Annotations }} | |
| {{- with .Values.ingress.annotations }} |
packages/system/keycloak/values.yaml
Outdated
| ingressClassName: tenent-root | ||
| annotations: | ||
| acme.cert-manager.io/http01-ingress-class: tenant-root | ||
| cert-manager.io/cluster-issuer: letsencrypt-prod |
There was a problem hiding this comment.
use common logic from cozystack-config
cozystack/packages/extra/monitoring/templates/alerta/alerta.yaml
Lines 151 to 154 in 47bd46c
packages/system/keycloak/values.yaml
Outdated
| image: quay.io/keycloak/keycloak:26.0.4 | ||
| host: keycloak.infra.aenix.org | ||
| user: admin | ||
| password: "{{ randAlphaNum 16 }}" |
There was a problem hiding this comment.
Let's use common secret generated by CNPG
| lifecycle: | ||
| postStart: | ||
| exec: | ||
| command: ["/bin/bash", "-c", "if [[ $(HOSTNAME) == *-0 ]]; then /scripts/init-realm.sh; fi"] |
There was a problem hiding this comment.
Let's move keycloak configuration to separate PR
There was a problem hiding this comment.
Let's move keycloak configuration to separate PR
klinch0
force-pushed
the
feature/add-sso
branch
from
0747acc
to
7e7e90e
Compare
klinch0
force-pushed
the
feature/add-sso
branch
from
7e7e90e
to
3d2d365
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (8)
packages/system/keycloak/templates/db.yaml (2)
6-8: Consider environment-specific storage configurationThe current configuration might not be suitable for all environments:
- Storage class is not specified, which might cause issues in different Kubernetes clusters
- Fixed size of 20Gi might be too large for dev/test or too small for production
- No backup strategy is defined
Consider adding:
storage: size: {{ .Values.database.storage.size | default "20Gi" }} storageClass: {{ .Values.database.storage.class | default "default" }} backup: barmanObjectStore: destinationPath: {{ .Values.database.backup.destination }} endpointURL: {{ .Values.database.backup.endpoint }} s3Credentials: inheritFromIAMRole: true
6-6: Enhance high availability configurationWhile having 2 instances provides basic HA, consider adding topology spread constraints and anti-affinity rules to ensure proper distribution across nodes.
Consider adding:
topologySpreadConstraints: - maxSkew: 1 topologyKey: kubernetes.io/hostname whenUnsatisfiable: DoNotSchedule affinity: podAntiAffinity: requiredDuringSchedulingIgnoredDuringExecution: - labelSelector: matchLabels: postgresql: keycloak-db topologyKey: kubernetes.io/hostnamepackages/system/keycloak/templates/ingress.yaml (1)
11-19: Consider adding security headers and rate limitingFor enhanced security, consider adding the following annotations:
- nginx.ingress.kubernetes.io/rate-limit-* for rate limiting
- nginx.ingress.kubernetes.io/configuration-snippet for security headers (X-Frame-Options, HSTS, etc.)
Example configuration to add to
.Values.ingress.annotations:nginx.ingress.kubernetes.io/rate-limit-rpm: "60" nginx.ingress.kubernetes.io/configuration-snippet: | add_header X-Frame-Options DENY; add_header X-Content-Type-Options nosniff; add_header X-XSS-Protection "1; mode=block";scripts/installer.sh (2)
Line range hint
9-20: Verify migration process robustnessThe migration process looks solid with proper version tracking and sequential execution. However, consider adding these improvements:
- Backup mechanism before migrations
- Rollback capability if migration fails
- Logging of migration steps
run_migrations() { if ! kubectl get configmap -n cozy-system cozystack-version; then kubectl create configmap -n cozy-system cozystack-version --from-literal=version="$VERSION" --dry-run=client -o yaml | kubectl create -f- return fi current_version=$(kubectl get configmap -n cozy-system cozystack-version -o jsonpath='{.data.version}') || true + # Log migration start + echo "Starting migration from version $current_version to $VERSION" + + # Create backup + kubectl get configmap -n cozy-system cozystack -o yaml > "backup-cozystack-v$current_version.yaml" + until [ "$current_version" = "$VERSION" ]; do echo "run migration: $current_version --> $VERSION" chmod +x scripts/migrations/$current_version - scripts/migrations/$current_version + if ! scripts/migrations/$current_version; then + echo "Migration failed, check logs for details" + exit 1 + fi current_version=$(kubectl get configmap -n cozy-system cozystack-version -o jsonpath='{.data.version}') done }
Line range hint
95-103: Consider adding health checks for KeycloakThe continuous reconciliation loop could benefit from additional health checks for the Keycloak service.
# Reconcile platform chart trap 'exit' INT TERM while true; do sleep 60 & wait + # Check Keycloak health + if ! kubectl get deployment -n cozy-public keycloak >/dev/null 2>&1; then + echo "WARNING: Keycloak deployment not found" + fi make -C packages/core/platform apply donepackages/apps/tenant/templates/tenant.yaml (1)
91-144: Well-structured RBAC with clear separation of concernsThe overall RBAC structure demonstrates good design principles:
- Clear separation between read-write and read-only access
- Consistent use of Groups for access management
- Proper restriction of verbs for read-only role following the principle of least privilege
Once the API group and resource configurations are fixed (as noted in previous reviews), this will provide a solid foundation for access control.
packages/system/keycloak/templates/sts.yaml (2)
14-28: Add PodDisruptionBudget for high availabilitySince this is an HA deployment with multiple replicas, consider adding a PodDisruptionBudget to ensure service availability during voluntary disruptions:
apiVersion: policy/v1 kind: PodDisruptionBudget metadata: name: keycloak-pdb spec: minAvailable: 1 selector: matchLabels: app: keycloak-ha
115-126: Optimize probe timingsThe initial delays seem excessive. Consider:
- Reducing initialDelaySeconds for faster pod availability
- Adding periodSeconds and failureThreshold for better control
livenessProbe: httpGet: path: / port: http - initialDelaySeconds: 120 + initialDelaySeconds: 60 timeoutSeconds: 5 + periodSeconds: 10 + failureThreshold: 3 readinessProbe: httpGet: path: /realms/master port: http - initialDelaySeconds: 60 + initialDelaySeconds: 30 timeoutSeconds: 1 + periodSeconds: 10 + failureThreshold: 3
🛑 Comments failed to post (10)
packages/system/keycloak/templates/db.yaml (1)
1-12: 🛠️ Refactor suggestion
Add resource management configuration
The cluster lacks resource requests and limits, which could lead to resource contention issues.
Consider adding:
resources: requests: cpu: "1" memory: "2Gi" limits: cpu: "2" memory: "4Gi"Also, consider adding monitoring configuration:
monitoring: enablePodMonitor: truescripts/migrations/7 (3)
1-3: 🛠️ Refactor suggestion
Add script documentation and prerequisites
The script should include:
- Description of what the migration does
- Prerequisites and requirements
- Impact on the system
- Rollback procedure
Add a comment block like this:
#!/bin/sh # Migration 7 --> 8 +# +# Description: +# Updates the root-host configuration and bumps version to 8 +# +# Prerequisites: +# - kubectl with admin access to the cluster +# - tenant-root HelmRelease must exist +# +# Impact: +# - Updates cozystack ConfigMap +# - Updates version to 8 +# +# Rollback: +# - Restore cozystack ConfigMap from backup +# - Set version back to 7📝 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.#!/bin/sh # Migration 7 --> 8 # # Description: # Updates the root-host configuration and bumps version to 8 # # Prerequisites: # - kubectl with admin access to the cluster # - tenant-root HelmRelease must exist # # Impact: # - Updates cozystack ConfigMap # - Updates version to 8 # # Rollback: # - Restore cozystack ConfigMap from backup # - Set version back to 7
8-9:
⚠️ Potential issueAdd version check and proper error handling
The version update lacks a check if migration is already completed and proper error handling.
Apply this improvement:
-# Write version to cozystack-version config -kubectl create configmap -n cozy-system cozystack-version --from-literal=version=8 --dry-run=client -o yaml | kubectl apply -f- +# Check current version +current_version=$(kubectl get configmap -n cozy-system cozystack-version -o jsonpath='{.data.version}' 2>/dev/null || echo "7") +if [ "$current_version" = "8" ]; then + echo "Migration already completed (version 8)" + exit 0 +elif [ "$current_version" != "7" ]; then + echo "Error: Unexpected version $current_version (expected 7)" + exit 1 +fi + +# Update version +if ! kubectl create configmap -n cozy-system cozystack-version --from-literal=version=8 --dry-run=client -o yaml | kubectl apply -f-; then + echo "Error: Failed to update version" + exit 1 +fi + +echo "Successfully migrated to version 8"📝 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.# Check current version current_version=$(kubectl get configmap -n cozy-system cozystack-version -o jsonpath='{.data.version}' 2>/dev/null || echo "7") if [ "$current_version" = "8" ]; then echo "Migration already completed (version 8)" exit 0 elif [ "$current_version" != "7" ]; then echo "Error: Unexpected version $current_version (expected 7)" exit 1 fi # Update version if ! kubectl create configmap -n cozy-system cozystack-version --from-literal=version=8 --dry-run=client -o yaml | kubectl apply -f-; then echo "Error: Failed to update version" exit 1 fi echo "Successfully migrated to version 8"
5-6:
⚠️ Potential issueAdd error handling and validation
The host extraction and ConfigMap update lack error handling and validation. A failure in these commands could leave the system in an inconsistent state.
Apply this improvement:
-host=$(kubectl get hr tenant-root -n tenant-root -o yaml | grep 'host:' | awk '{print $2}') -kubectl patch configmap -n cozy-system cozystack --type merge -p "{\"data\":{\"root-host\":\"$host\"}}" +# Extract host value +if ! host=$(kubectl get hr tenant-root -n tenant-root -o jsonpath='{.spec.values.host}'); then + echo "Error: Failed to extract host from tenant-root HelmRelease" + exit 1 +fi + +# Validate host is not empty +if [ -z "$host" ]; then + echo "Error: Extracted host is empty" + exit 1 +fi + +# Backup existing ConfigMap +if ! kubectl get configmap -n cozy-system cozystack -o yaml > cozystack-backup.yaml; then + echo "Error: Failed to backup existing ConfigMap" + exit 1 +fi + +# Update ConfigMap +if ! kubectl patch configmap -n cozy-system cozystack --type merge -p "{\"data\":{\"root-host\":\"$host\"}}"; then + echo "Error: Failed to update cozystack ConfigMap" + echo "Restoring from backup..." + kubectl apply -f cozystack-backup.yaml + exit 1 +fi📝 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.# Extract host value if ! host=$(kubectl get hr tenant-root -n tenant-root -o jsonpath='{.spec.values.host}'); then echo "Error: Failed to extract host from tenant-root HelmRelease" exit 1 fi # Validate host is not empty if [ -z "$host" ]; then echo "Error: Extracted host is empty" exit 1 fi # Backup existing ConfigMap if ! kubectl get configmap -n cozy-system cozystack -o yaml > cozystack-backup.yaml; then echo "Error: Failed to backup existing ConfigMap" exit 1 fi # Update ConfigMap if ! kubectl patch configmap -n cozy-system cozystack --type merge -p "{\"data\":{\"root-host\":\"$host\"}}"; then echo "Error: Failed to update cozystack ConfigMap" echo "Restoring from backup..." kubectl apply -f cozystack-backup.yaml exit 1 fipackages/system/keycloak/templates/ingress.yaml (2)
14-18: 🛠️ Refactor suggestion
Parameterize cert-manager configuration
The cert-manager configuration is hardcoded to use 'letsencrypt-prod', which may not be suitable for all environments (e.g., staging/development).
Make the cert-manager configuration configurable:
{{- if ne $issuerType "cloudflare" }} acme.cert-manager.io/http01-ingress-class: {{ $ingress }} {{- end }} - cert-manager.io/cluster-issuer: letsencrypt-prod + cert-manager.io/cluster-issuer: {{ .Values.certManager.issuer | default "letsencrypt-prod" }} {{- toYaml . | nindent 4 }}📝 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.{{- if ne $issuerType "cloudflare" }} acme.cert-manager.io/http01-ingress-class: {{ $ingress }} {{- end }} cert-manager.io/cluster-issuer: {{ .Values.certManager.issuer | default "letsencrypt-prod" }} {{- toYaml . | nindent 4 }}
1-7: 🛠️ Refactor suggestion
Add error handling for configuration lookups
The template should handle cases where the ConfigMap or Namespace lookups fail, or when required data fields are missing.
Add validation and error handling:
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} +{{- if not $cozyConfig }} +{{- fail "ConfigMap 'cozystack' not found in namespace 'cozy-system'" }} +{{- end }} +{{- if not (index $cozyConfig.data "root-host") }} +{{- fail "Required field 'root-host' not found in ConfigMap" }} +{{- end }} {{- $host := index $cozyConfig.data "root-host" }} {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} {{- $rootns := lookup "v1" "Namespace" "" "tenant-root" }} +{{- if not $rootns }} +{{- fail "Namespace 'tenant-root' not found" }} +{{- end }} +{{- if not (index $rootns.metadata.annotations "namespace.cozystack.io/ingress") }} +{{- fail "Required annotation 'namespace.cozystack.io/ingress' not found in tenant-root namespace" }} +{{- end }} {{- $ingress := index $rootns.metadata.annotations "namespace.cozystack.io/ingress" }}📝 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.{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} {{- if not $cozyConfig }} {{- fail "ConfigMap 'cozystack' not found in namespace 'cozy-system'" }} {{- end }} {{- if not (index $cozyConfig.data "root-host") }} {{- fail "Required field 'root-host' not found in ConfigMap" }} {{- end }} {{- $host := index $cozyConfig.data "root-host" }} {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} {{- $rootns := lookup "v1" "Namespace" "" "tenant-root" }} {{- if not $rootns }} {{- fail "Namespace 'tenant-root' not found" }} {{- end }} {{- if not (index $rootns.metadata.annotations "namespace.cozystack.io/ingress") }} {{- fail "Required annotation 'namespace.cozystack.io/ingress' not found in tenant-root namespace" }} {{- end }} {{- $ingress := index $rootns.metadata.annotations "namespace.cozystack.io/ingress" }}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/keycloak/templates/sts.yaml (4)
56-59: 🛠️ Refactor suggestion
Remove redundant capability and consider stricter security settings
CAP_NET_RAWdrop is redundant as it's already included inALL.- Consider enabling
readOnlyRootFilesystemfor better security.capabilities: drop: - ALL - - CAP_NET_RAW readOnlyRootFilesystem: false + readOnlyRootFilesystem: true📝 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.drop: - ALL readOnlyRootFilesystem: true
74-77: 🛠️ Refactor suggestion
Add validation for required values
Add validation for required values like
.Values.user:+{{- if not .Values.user }} + {{- fail "user is required in values" }} +{{- end }} - name: KEYCLOAK_ADMIN value: {{ .Values.user }}Committable suggestion skipped: line range outside the PR's diff.
1-2:
⚠️ Potential issueAdd error handling for ConfigMap lookup
The ConfigMap lookup could fail if
cozystackConfigMap doesn't exist incozy-systemnamespace or ifroot-hostkey is missing. Consider adding error handling:{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} +{{- if not $cozyConfig }} + {{- fail "ConfigMap 'cozystack' not found in 'cozy-system' namespace" }} +{{- end }} {{- $host := index $cozyConfig.data "root-host" }} +{{- if not $host }} + {{- fail "root-host key not found in cozystack ConfigMap" }} +{{- end }}📝 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.{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} {{- if not $cozyConfig }} {{- fail "ConfigMap 'cozystack' not found in 'cozy-system' namespace" }} {{- end }} {{- $host := index $cozyConfig.data "root-host" }} {{- if not $host }} {{- fail "root-host key not found in cozystack ConfigMap" }} {{- end }}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
127-134:
⚠️ Potential issueFix volumeMounts placement
The
volumeMountssection should be under the container spec, not at the pod level:volumes: - name: init-script configMap: name: keycloak-init-script - volumeMounts: - - name: init-script - mountPath: /scripts - readOnly: true + containers: + - name: keycloak + volumeMounts: + - name: init-script + mountPath: /scripts + readOnly: trueCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
packages/system/keycloak/templates/ingress.yaml (1)
29-36: Add health check annotationsConsider adding health check annotations to ensure proper monitoring of the Keycloak service.
Add these annotations in the metadata section:
nginx.ingress.kubernetes.io/backend-protocol: "HTTP" nginx.ingress.kubernetes.io/proxy-buffer-size: "128k" nginx.ingress.kubernetes.io/proxy-connect-timeout: "30" nginx.ingress.kubernetes.io/proxy-read-timeout: "30"packages/system/keycloak/templates/sts.yaml (3)
3-10: Enhance secret management practicesConsider improving the secret configuration:
- Add metadata labels for better secret management
- Implement stronger password requirements (minimum length, complexity)
kind: Secret metadata: name: {{ .Release.Name }}-credentials + labels: + app.kubernetes.io/name: keycloak + app.kubernetes.io/instance: {{ .Release.Name }} stringData: - admin: {{ $password }} + admin: {{ randAscii 32 | b64enc }}
56-59: Remove redundant capability drop
CAP_NET_RAWis already included inALL, making its explicit drop redundant.capabilities: drop: - ALL - - CAP_NET_RAW
115-126: Optimize probe timing for faster recoveryThe initial delays (120s for liveness, 60s for readiness) seem excessive and could delay recovery in case of restarts.
livenessProbe: httpGet: path: / port: http - initialDelaySeconds: 120 + initialDelaySeconds: 60 timeoutSeconds: 5 readinessProbe: httpGet: path: /realms/master port: http - initialDelaySeconds: 60 + initialDelaySeconds: 30 timeoutSeconds: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/core/platform/bundles/distro-full.yaml(1 hunks)packages/core/platform/bundles/distro-hosted.yaml(1 hunks)packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(2 hunks)packages/system/keycloak/Chart.yaml(1 hunks)packages/system/keycloak/templates/db.yaml(1 hunks)packages/system/keycloak/templates/ingress.yaml(1 hunks)packages/system/keycloak/templates/service-headless.yaml(1 hunks)packages/system/keycloak/templates/service.yaml(1 hunks)packages/system/keycloak/templates/sts.yaml(1 hunks)packages/system/keycloak/values.yaml(1 hunks)scripts/installer.sh(1 hunks)scripts/migrations/7(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/core/platform/bundles/distro-full.yaml
- packages/core/platform/bundles/distro-hosted.yaml
- packages/core/platform/bundles/paas-full.yaml
- packages/core/platform/bundles/paas-hosted.yaml
- packages/system/keycloak/Chart.yaml
- packages/system/keycloak/templates/db.yaml
- packages/system/keycloak/templates/service-headless.yaml
- packages/system/keycloak/templates/service.yaml
- packages/system/keycloak/values.yaml
- scripts/installer.sh
- scripts/migrations/7
🧰 Additional context used
🪛 yamllint
packages/system/keycloak/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/keycloak/templates/sts.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 44-44: wrong indentation: expected 10 but found 12
(indentation)
🔇 Additional comments (3)
packages/system/keycloak/templates/ingress.yaml (2)
25-25: Previous review comment about hardcoded TLS secret is still applicable
The TLS secret name is still hardcoded as 'web-tls', which was previously identified as an issue.
18-18: Prevent override of critical annotations
The current annotation merge could potentially override important annotations like the cert-manager configuration.
packages/system/keycloak/templates/sts.yaml (1)
109-110: Make DNS suffix configurable
| {{- if ne $issuerType "cloudflare" }} | ||
| acme.cert-manager.io/http01-ingress-class: {{ $ingress }} | ||
| {{- end }} | ||
| cert-manager.io/cluster-issuer: letsencrypt-prod |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make cluster-issuer configurable
The cert-manager cluster-issuer is hardcoded to 'letsencrypt-prod', which reduces flexibility for different environments.
Make it configurable:
- cert-manager.io/cluster-issuer: letsencrypt-prod
+ cert-manager.io/cluster-issuer: {{ .Values.ingress.certManager.issuer | default "letsencrypt-prod" }}📝 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.
| cert-manager.io/cluster-issuer: letsencrypt-prod | |
| cert-manager.io/cluster-issuer: {{ .Values.ingress.certManager.issuer | default "letsencrypt-prod" }} |
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $host := index $cozyConfig.data "root-host" }} | ||
| {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} | ||
|
|
||
| {{- $rootns := lookup "v1" "Namespace" "" "tenant-root" }} | ||
| {{- $ingress := index $rootns.metadata.annotations "namespace.cozystack.io/ingress" }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for configuration lookups
The template assumes the ConfigMap and Namespace exist and contain the required data. Consider adding validation to handle missing configurations gracefully.
Add error handling:
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
+{{- if not $cozyConfig }}
+ {{- fail "ConfigMap 'cozystack' not found in namespace 'cozy-system'" }}
+{{- end }}
{{- $host := index $cozyConfig.data "root-host" }}
+{{- if not $host }}
+ {{- fail "root-host configuration is required in cozystack ConfigMap" }}
+{{- end }}📝 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.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- $host := index $cozyConfig.data "root-host" }} | |
| {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} | |
| {{- $rootns := lookup "v1" "Namespace" "" "tenant-root" }} | |
| {{- $ingress := index $rootns.metadata.annotations "namespace.cozystack.io/ingress" }} | |
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- if not $cozyConfig }} | |
| {{- fail "ConfigMap 'cozystack' not found in namespace 'cozy-system'" }} | |
| {{- end }} | |
| {{- $host := index $cozyConfig.data "root-host" }} | |
| {{- if not $host }} | |
| {{- fail "root-host configuration is required in cozystack ConfigMap" }} | |
| {{- end }} | |
| {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} | |
| {{- $rootns := lookup "v1" "Namespace" "" "tenant-root" }} | |
| {{- $ingress := index $rootns.metadata.annotations "namespace.cozystack.io/ingress" }} |
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| name: keycloak | ||
| labels: | ||
| app: keycloak-ha | ||
| spec: | ||
| selector: | ||
| matchLabels: | ||
| app: keycloak-ha | ||
| replicas: 2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve high availability configuration
- Add recommended Kubernetes labels
- Configure pod anti-affinity for better HA
labels:
- app: keycloak-ha
+ app.kubernetes.io/name: keycloak
+ app.kubernetes.io/instance: {{ .Release.Name }}
+ app.kubernetes.io/component: auth
spec:
selector:
matchLabels:
- app: keycloak-ha
+ app.kubernetes.io/name: keycloak
+ app.kubernetes.io/instance: {{ .Release.Name }}
+ template:
+ spec:
+ affinity:
+ podAntiAffinity:
+ requiredDuringSchedulingIgnoredDuringExecution:
+ - labelSelector:
+ matchExpressions:
+ - key: app.kubernetes.io/name
+ operator: In
+ values:
+ - keycloak
+ topologyKey: kubernetes.io/hostnameCommittable suggestion skipped: line range outside the PR's diff.
| volumeMounts: | ||
| - name: init-script | ||
| mountPath: /scripts | ||
| readOnly: true |
There was a problem hiding this comment.
Fix volume mounts placement
The volumeMounts section should be inside the container spec, not at the pod level.
volumes:
- name: init-script
configMap:
name: keycloak-init-script
- volumeMounts:
- - name: init-script
- mountPath: /scripts
- readOnly: true
terminationGracePeriodSeconds: 60
+ containers:
+ - name: keycloak
+ volumeMounts:
+ - name: init-script
+ mountPath: /scripts
+ readOnly: true📝 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.
| volumeMounts: | |
| - name: init-script | |
| mountPath: /scripts | |
| readOnly: true | |
| volumes: | |
| - name: init-script | |
| configMap: | |
| name: keycloak-init-script | |
| terminationGracePeriodSeconds: 60 | |
| containers: | |
| - name: keycloak | |
| volumeMounts: | |
| - name: init-script | |
| mountPath: /scripts | |
| readOnly: true |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation