Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add keycloak #475

Merged
merged 3 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/core/platform/bundles/distro-full.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,9 @@ releases:
namespace: cozy-external-secrets-operator
optional: true
dependsOn: [cilium]

- name: keycloak
releaseName: keycloak
chart: cozy-keycloak
namespace: cozy-keycloak
dependsOn: [postgres-operator]
6 changes: 6 additions & 0 deletions packages/core/platform/bundles/distro-hosted.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,9 @@ releases:
namespace: cozy-external-secrets-operator
optional: true
dependsOn: []

- name: keycloak
releaseName: keycloak
chart: cozy-keycloak
namespace: cozy-keycloak
dependsOn: [postgres-operator]
6 changes: 6 additions & 0 deletions packages/core/platform/bundles/paas-full.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,9 @@ releases:
namespace: cozy-external-secrets-operator
optional: true
dependsOn: [cilium,kubeovn]

- name: keycloak
releaseName: keycloak
chart: cozy-keycloak
namespace: cozy-keycloak
dependsOn: [postgres-operator]
8 changes: 7 additions & 1 deletion packages/core/platform/bundles/paas-hosted.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ releases:
chart: cozy-cert-manager-crds
namespace: cozy-cert-manager
dependsOn: []

- name: cozystack-api
releaseName: cozystack-api
chart: cozy-cozystack-api
Expand Down Expand Up @@ -145,3 +145,9 @@ releases:
{{- end }}
{{- end }}
{{- end }}

- name: keycloak
releaseName: keycloak
chart: cozy-keycloak
namespace: cozy-keycloak
dependsOn: [postgres-operator]
3 changes: 3 additions & 0 deletions packages/system/keycloak/Chart.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
apiVersion: v2
name: cozy-keycloak
version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process
12 changes: 12 additions & 0 deletions packages/system/keycloak/templates/db.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
apiVersion: postgresql.cnpg.io/v1
kind: Cluster
metadata:
name: keycloak-db
spec:
instances: 2
storage:
size: 20Gi

inheritedMetadata:
labels:
policy.cozystack.io/allow-to-apiserver: "true"
36 changes: 36 additions & 0 deletions packages/system/keycloak/templates/ingress.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{{- $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" }}

Comment on lines +1 to +7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
{{- $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)

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: keycloak-ingress
{{- with .Values.ingress.annotations }}
annotations:
{{- if ne $issuerType "cloudflare" }}
acme.cert-manager.io/http01-ingress-class: {{ $ingress }}
{{- end }}
cert-manager.io/cluster-issuer: letsencrypt-prod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
cert-manager.io/cluster-issuer: letsencrypt-prod
cert-manager.io/cluster-issuer: {{ .Values.ingress.certManager.issuer | default "letsencrypt-prod" }}

{{- toYaml . | nindent 4 }}
{{- end }}
spec:
ingressClassName: {{ $ingress }}
tls:
- hosts:
- keycloak.{{ $host }}
secretName: web-tls
rules:
- host: keycloak.{{ $host }}
http:
paths:
- path: /
pathType: Prefix
backend:
service:
name: keycloak-http
port:
name: http
9 changes: 9 additions & 0 deletions packages/system/keycloak/templates/service-headless.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
apiVersion: v1
kind: Service
metadata:
name: keycloak-headless
spec:
type: ClusterIP
clusterIP: None
selector:
app: keycloak-ha
Comment on lines +1 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add essential service configurations

The headless service definition needs the following essential improvements:

  1. Port definitions for the Keycloak service
  2. 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.

Suggested change
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

12 changes: 12 additions & 0 deletions packages/system/keycloak/templates/service.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
apiVersion: v1
kind: Service
metadata:
name: keycloak-http
spec:
type: ClusterIP
ports:
- name: http
port: 8080
protocol: TCP
selector:
app: keycloak-ha
135 changes: 135 additions & 0 deletions packages/system/keycloak/templates/sts.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
{{- $host := index $cozyConfig.data "root-host" }}
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for ConfigMap lookup

The ConfigMap lookup could fail if cozystack ConfigMap doesn't exist or if root-host key 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.

Suggested change
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
{{- $host := index $cozyConfig.data "root-host" }}
{{- $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)

{{- $password := randAlphaNum 16 -}}

apiVersion: v1
kind: Secret
metadata:
name: {{ .Release.Name }}-credentials
stringData:
admin: {{ $password }}

---

apiVersion: apps/v1
kind: StatefulSet
metadata:
name: keycloak
labels:
app: keycloak-ha
spec:
selector:
matchLabels:
app: keycloak-ha
replicas: 2
Comment on lines +17 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve high availability configuration

  1. Add recommended Kubernetes labels
  2. 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/hostname

Committable suggestion skipped: line range outside the PR's diff.

serviceName: keycloak-headless
podManagementPolicy: Parallel
updateStrategy:
type: RollingUpdate
template:
metadata:
labels:
app: keycloak-ha
spec:
restartPolicy: Always
securityContext:
fsGroup: 1000
containers:
- name: keycloak
image: {{ .Values.image }}
imagePullPolicy: Always
Comment on lines +39 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Specify explicit image tag

Using implicit latest tag or no tag can lead to unintended updates and inconsistent deployments. Always specify an explicit image tag:

-          image: {{ .Values.image }}
+          image: {{ .Values.image }}:{{ .Values.imageTag | default "latest" }}
📝 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.

Suggested change
image: {{ .Values.image }}
imagePullPolicy: Always
image: {{ .Values.image }}:{{ .Values.imageTag | default "latest" }}
imagePullPolicy: Always

{{- if or .Values.resources.requests .Values.resources.limits }}
resources:
{{- if .Values.resources.limits }}
limits:
{{- toYaml .Values.resources.limits | nindent 14 }}
{{- end }}
{{- if .Values.resources.requests }}
requests:
{{- toYaml .Values.resources.requests | nindent 14 }}
{{- end }}
{{- end }}
Comment on lines +41 to +51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix YAML indentation in resources block

The resources block has incorrect indentation which could cause issues. Fix the indentation to match Helm's standard:

          {{- if or .Values.resources.requests .Values.resources.limits }}
          resources:
            {{- if .Values.resources.limits }}
            limits:
-              {{- toYaml .Values.resources.limits | nindent 14 }}
+              {{- toYaml .Values.resources.limits | nindent 16 }}
            {{- end }}
            {{- if .Values.resources.requests }}
            requests:
-              {{- toYaml .Values.resources.requests | nindent 14 }}
+              {{- toYaml .Values.resources.requests | nindent 16 }}
            {{- end }}
          {{- 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.

Suggested change
{{- if or .Values.resources.requests .Values.resources.limits }}
resources:
{{- if .Values.resources.limits }}
limits:
{{- toYaml .Values.resources.limits | nindent 14 }}
{{- end }}
{{- if .Values.resources.requests }}
requests:
{{- toYaml .Values.resources.requests | nindent 14 }}
{{- end }}
{{- end }}
{{- if or .Values.resources.requests .Values.resources.limits }}
resources:
{{- if .Values.resources.limits }}
limits:
{{- toYaml .Values.resources.limits | nindent 16 }}
{{- end }}
{{- if .Values.resources.requests }}
requests:
{{- toYaml .Values.resources.requests | nindent 16 }}
{{- end }}
{{- end }}
🧰 Tools
🪛 yamllint

[warning] 44-44: wrong indentation: expected 10 but found 12

(indentation)

securityContext:
runAsNonRoot: true
runAsUser: 1000
capabilities:
drop:
- ALL
- CAP_NET_RAW
readOnlyRootFilesystem: false
allowPrivilegeEscalation: false
args:
- start
env:
- name: KC_METRICS_ENABLED
value: "true"
- name: KC_LOG_LEVEL
value: "info"
- name: KC_CACHE
value: "ispn"
- name: KC_CACHE_STACK
value: "kubernetes"
- name: KC_PROXY
value: "edge"
- name: KEYCLOAK_ADMIN
value: admin
- name: KEYCLOAK_ADMIN_PASSWORD
value: {{ $password }}
- name: KC_DB
value: "postgres"
- name: KC_DB_URL_HOST
valueFrom:
secretKeyRef:
name: keycloak-db-app
key: "host"
- name: KC_DB_URL_PORT
valueFrom:
secretKeyRef:
name: keycloak-db-app
key: "port"
- name: KC_DB_USERNAME
valueFrom:
secretKeyRef:
name: keycloak-db-app
key: "username"
- name: KC_DB_PASSWORD
valueFrom:
secretKeyRef:
name: keycloak-db-app
key: "password"
- name: KC_DB_URL_DATABASE
valueFrom:
secretKeyRef:
name: keycloak-db-app
key: "dbname"
- name: KC_FEATURES
value: "docker"
- name: KC_HOSTNAME
value: https://keycloak.{{ $host }}
- name: JAVA_OPTS_APPEND
value: "-Djgroups.dns.query=keycloak-headless.cozy-keycloak.svc.cozy.local"
ports:
- name: http
containerPort: 8080
protocol: TCP
livenessProbe:
httpGet:
path: /
port: http
initialDelaySeconds: 120
timeoutSeconds: 5
readinessProbe:
httpGet:
path: /realms/master
port: http
initialDelaySeconds: 60
timeoutSeconds: 1
volumes:
- name: init-script
configMap:
name: keycloak-init-script
volumeMounts:
- name: init-script
mountPath: /scripts
readOnly: true
Comment on lines +131 to +134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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

terminationGracePeriodSeconds: 60
14 changes: 14 additions & 0 deletions packages/system/keycloak/values.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
image: quay.io/keycloak/keycloak:26.0.4

ingress:
annotations:
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"
Comment on lines +5 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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"

resources:
limits:
memory: 1500Mi
requests:
memory: 500Mi
cpu: 100m
2 changes: 1 addition & 1 deletion scripts/installer.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ set -o pipefail
set -e

BUNDLE=$(set -x; kubectl get configmap -n cozy-system cozystack -o 'go-template={{index .data "bundle-name"}}')
VERSION=7
VERSION=8

run_migrations() {
if ! kubectl get configmap -n cozy-system cozystack-version; then
Expand Down
9 changes: 9 additions & 0 deletions scripts/migrations/7
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/bin/sh
# Migration 7 --> 8


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\"}}"

# 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-
Loading