feat: replace Bitnami Keycloak dependency with cloudpirates chart#46
feat: replace Bitnami Keycloak dependency with cloudpirates chart#46aaronzi merged 3 commits intoeclipse-basyx:mainfrom
Conversation
Relates: eclipse-basyx#44 - Replace bitnami/keycloak@24.4.11 with cloudpirates/keycloak@0.20.0 - Slim BaSyx-realm.json to minimal config: admin role, basyx-client-api client roles, basyx-technical-user service account with admin role - Add all RBAC roles from values.yaml to the realm; remove all -two and -three suffixed roles from realm and rbac rules across all services - Set proxyHeaders to xforwarded and ingress className to nginx in values.yaml - Bump chart version to 2.2.0
1574c66 to
c4651c8
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates the umbrella Helm chart’s Keycloak dependency away from Bitnami to the CloudPirates Keycloak chart (addressing the Bitnami artifact/dependency availability concerns in #44), and updates the chart’s default Keycloak configuration and realm/RBAC examples accordingly.
Changes:
- Replace the
bitnami/keycloakchart dependency withcloudpirates/keycloak(OCI) and bump the umbrella chart version. - Update
values.yamlKeycloak settings to match the new chart structure (image, postgres subchart key, ingress structure, realm import settings). - Slim down
BaSyx-realm.jsonand example RBAC blocks; adjust APIRule port and CI repo-add logic for OCI deps.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
charts/basyx/Chart.yaml |
Switch Keycloak dependency to CloudPirates OCI chart and bump chart version. |
charts/basyx/values.yaml |
Update Keycloak values structure (image, postgres key, ingress, service port) and slim RBAC examples. |
charts/basyx/templates/keycloak/basyx-keycloak-apirule.yaml |
Update APIRule to target the Keycloak service port used by the new setup. |
charts/basyx/config/BaSyx-realm.json |
Replace large realm export with a minimal realm/client/role setup for BaSyx. |
.github/workflows/tests.yml |
Prevent helm repo add from attempting to add OCI repositories during CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
charts/basyx/values.yaml
Outdated
| @@ -106,13 +145,17 @@ keycloak: | |||
| enabled: false | |||
| ## @param keycloak.ingress.ingressClassName IngressClass for the ingress | |||
There was a problem hiding this comment.
keycloak.ingress now uses the field className, but the docs still reference keycloak.ingress.ingressClassName. This makes the values file misleading and can break generated documentation. Update the @param annotation (and optionally the label text) to match the actual key (keycloak.ingress.className).
| ## @param keycloak.ingress.ingressClassName IngressClass for the ingress | |
| ## @param keycloak.ingress.className IngressClass for the ingress |
| extraVolumes: | ||
| - name: basyx-keycloak-realm | ||
| configMap: | ||
| name: basyx-keycloak-realm |
There was a problem hiding this comment.
keycloak.fullnameOverride controls the realm ConfigMap name ({{ .Values.keycloak.fullnameOverride }}-realm), but the default extraVolumes[].configMap.name is hard-coded to basyx-keycloak-realm. If a user overrides fullnameOverride, the pod will try to mount a non-existent ConfigMap. Consider making the ConfigMap name stable (not derived from fullnameOverride), or generate the default volume reference in templates so it always matches the rendered ConfigMap name.
There was a problem hiding this comment.
made a constant
| service: | ||
| name: {{ .Values.keycloak.fullnameOverride }} | ||
| port: 80 | ||
| port: 8080 |
There was a problem hiding this comment.
The APIRule service port is hard-coded to 8080, even though values.yaml now defines keycloak.service.httpPort. Hard-coding risks drift if the Keycloak service port is changed via values. Use the configured value (or derive it from the Keycloak subchart service port) instead of a constant.
| port: 8080 | |
| port: {{ .Values.keycloak.service.httpPort | default 8080 }} |
| "clientId": "basyx-technical-user", | ||
| "name": "BaSyx Technical User", | ||
| "description": "Technical service account client for machine-to-machine access with admin role", | ||
| "enabled": true, | ||
| "clientAuthenticatorType": "client-secret", | ||
| "secret": "change-me-pls", | ||
| "standardFlowEnabled": false, | ||
| "implicitFlowEnabled": false, | ||
| "directAccessGrantsEnabled": false, | ||
| "serviceAccountsEnabled": true, | ||
| "publicClient": false, |
There was a problem hiding this comment.
The realm import JSON hard-codes the basyx-technical-user client secret ("change-me-pls"). This makes the default deployment insecure (anyone with chart access can mint admin tokens) and forces users to edit a tracked file to rotate secrets. Please make this secret configurable via values and source it from a Kubernetes Secret (or generate it at install time) rather than committing a fixed credential into the realm file.
There was a problem hiding this comment.
added secret parameterized via values.yaml.
| service: | ||
| name: {{ .Values.keycloak.fullnameOverride }} | ||
| port: 80 | ||
| port: 8080 |
There was a problem hiding this comment.
that's taken care of above - it's configurable now. The defaults exposed by the image changed.
| - name: keycloak | ||
| version: "0.20.0" | ||
| repository: oci://registry-1.docker.io/cloudpirates |
There was a problem hiding this comment.
Is this the recommended image to use? Is it maintained and are current vulnerabilities considered?
There was a problem hiding this comment.
The chart just wraps the image. The image the chart defaults to is the official postgres image [1]. Cloudpirates has been the go-to distribution in the Tractus-X context [2] since bitnami refusing to deliver free updates to their self-hosted images. So we're arguably way better off.
[1] image: docker.io/keycloak/keycloak:26.5.6@sha256:8d44614c7479
[2] https://eclipse-tractusx.github.io/docs/release/trg-5/trg-5-07/
| run: | | ||
| for dir in $(ls -d charts/*/); do | ||
| helm dependency list $dir 2> /dev/null | tail +2 | head -n -1 | awk '$3 !~ /^file:/ { print "helm repo add " $1 " " $3 }' | while read cmd; do $cmd; done | ||
| helm dependency list $dir 2> /dev/null | tail +2 | head -n -1 | awk '$3 !~ /^file:/ && $3 !~ /^oci:/ { print "helm repo add " $1 " " $3 }' | while read cmd; do $cmd; done |
There was a problem hiding this comment.
What is the impact of this change?
There was a problem hiding this comment.
The pipeline not crashing, see https://github.com/eclipse-basyx/charts/actions/runs/24389821173/job/71232645058. OCI-repos don't have to be helm add'ed
| httpEnabled: true | ||
| ## @param keycloak.keycloak.production false = start-dev mode, true = production start | ||
| ## | ||
| production: false |
There was a problem hiding this comment.
Is this intended to be set to false?
|
Thanks for the answers and the small fixes @arnoweiss. Looks good to me now. Is this PR ready to merge or do you still want to add/change something? |
|
Done from my perspective. |
Summary
bitnami/keycloak@24.4.11withcloudpirates/keycloak@0.20.0to resolve Bitnami dependency issuesBaSyx-realm.jsonand rbac configRelates: #44
Reviewer Tasks
I've tested these on an internal cluster with slightly different values. deployed and integrated successfully.
helm installdeploys without errorsbasyx-technical-userand token carriesadminrealm role