Conversation
Add optional ingress for Keycloak to enable OIDC browser flow when deployed in Kubernetes. When keycloak.ingress.enabled is true, the coordinator will use the public Keycloak URL instead of the internal service URL. Changes: - Add templates/ingress-keycloak.yaml for Keycloak ingress - Add keycloak.ingress.* values (enabled, className, annotations, host, tls) - Update deployment.yaml to use public URL when ingress is enabled - Update values.schema.json with ingress schema
…erving Add support for configuring Keycloak identity providers (GitHub, Google, etc.) through Helm values. Includes schema validation and realm import configuration. Also improves coordinator stability and frontend experience: - Add retry logic for ACL policy initialization on startup - Enhance static file serving with explicit content-type headers - Fix post-login redirect path to /ui/ instead of / - Update join token command example with full coordinator URL Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
Debian includes /etc/mime.types by default, allowing us to simplify the static file handler by using http.FileServer instead of manually setting Content-Type headers.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Use window.location.origin for dynamic coordinator URL in join token example - Fix TLS check to use tls.secretName instead of tls object (empty object is truthy)
Show the full ready-to-use command with actual token value and add a copy button for the complete command.
The builder was using Alpine (musl libc) but runtime uses Debian (glibc). This caused "exec: no such file or directory" because the dynamic linker was missing.
- Update test expectations to use /ui/ as default redirect - Fix HandleLogout to use defaultPostLoginRedirect constant
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # OR the user must manually set coordinator.oidc.url to a public URL and keycloak.service.type/Ingress accordingly. | ||
| # Here we default to the service DNS for backend communication. | ||
| {{- if .Values.keycloak.ingress.enabled }} | ||
| {{- if .Values.keycloak.ingress.tls.secretName }} |
There was a problem hiding this comment.
The deployment template is accessing .Values.keycloak.ingress.tls.secretName directly, but in values.yaml, tls is defined as an empty object (line 70). This will work in Helm templates when the field is not set (returns empty string), but the inconsistency between checking for .secretName existence here versus treating tls as a simple object in values.yaml could be confusing. Consider either:
- Keeping tls as an object with a secretName field (tls: {secretName: ""})
- Or checking if .Values.keycloak.ingress.tls is not empty before accessing .secretName
The same pattern is used in ingress-keycloak.yaml line 19.
| {{- if .Values.keycloak.ingress.tls.secretName }} | |
| {{- if and .Values.keycloak.ingress.tls .Values.keycloak.ingress.tls.secretName }} |
| {{- if .Values.keycloak.ingress.tls.secretName }} | ||
| tls: | ||
| - hosts: | ||
| - {{ .Values.keycloak.ingress.host | quote }} | ||
| secretName: {{ .Values.keycloak.ingress.tls.secretName }} | ||
| {{- end }} |
There was a problem hiding this comment.
The template is accessing .Values.keycloak.ingress.tls.secretName, but in values.yaml line 70, tls is defined as an empty object. While this works in Helm templates (returns empty string when the field doesn't exist), it would be more consistent and clear to either:
- Document that secretName should be set within the tls object (e.g., tls: {secretName: ""})
- Or add a comment in values.yaml showing the expected structure
This makes it easier for users to understand how to configure TLS for the Keycloak ingress.
| {{- if .Values.keycloak.ingress.tls.secretName }} | |
| tls: | |
| - hosts: | |
| - {{ .Values.keycloak.ingress.host | quote }} | |
| secretName: {{ .Values.keycloak.ingress.tls.secretName }} | |
| {{- end }} | |
| {{- with .Values.keycloak.ingress.tls }} | |
| {{- if .secretName }} | |
| tls: | |
| - hosts: | |
| - {{ $.Values.keycloak.ingress.host | quote }} | |
| secretName: {{ .secretName }} | |
| {{- end }} | |
| {{- end }} |
laike9m
left a comment
laike9m
left a comment
There was a problem hiding this comment.
The pull request introduces several improvements to the Helm charts, including support for Keycloak ingress and identity providers, as well as some backend refinements like a retry loop for ACL policy initialization.
I have identified a few issues in the Helm templates and the backend startup logic that should be addressed.
| { | ||
| "alias": {{ $idp.alias | quote }}, | ||
| "providerId": {{ $idp.providerId | quote }}, | ||
| "enabled": {{ $idp.enabled | default true }}, |
There was a problem hiding this comment.
In Helm templates, the default function treats false as an empty value. If .Values.keycloak.identityProviders[].enabled is explicitly set to false, it will be overridden by true. Since the schema already provides a default value of true, you can safely remove | default true or use a conditional check like {{ if hasKey $idp "enabled" }}{{ $idp.enabled }}{{ else }}true{{ end }}.
| # Here we default to the service DNS for backend communication. | ||
| {{- if .Values.keycloak.ingress.enabled }} | ||
| {{- if .Values.keycloak.ingress.tls.secretName }} | ||
| value: "https://{{ .Values.keycloak.ingress.host }}" |
There was a problem hiding this comment.
If keycloak.ingress.host is not provided, this will result in an invalid URL (https://). Consider adding a check to ensure the host is set when ingress is enabled.
| for i := 0; i < 10; i++ { | ||
| if err := s.wonderNetService.InitializeACLPolicy(ctx); err != nil { | ||
| aclErr = err | ||
| slog.Warn("initialize ACL policy, retrying", "error", err, "attempt", i+1) | ||
| time.Sleep(time.Duration(i+1) * time.Second) | ||
| continue | ||
| } | ||
| slog.Info("ACL policy initialized successfully") | ||
| aclErr = nil | ||
| break | ||
| } |
There was a problem hiding this comment.
This retry loop can block the server startup for up to 55 seconds. If the coordinator is running in Kubernetes, this might cause liveness or readiness probes to fail and restart the pod before it can successfully initialize. Consider using a shorter total timeout or ensuring that health checks can still pass during this initialization phase.
No description provided.