feat: add sso cookie domain to helm values #2444
Conversation
WalkthroughAdds a configurable SSO cookie domain to the controlplane Helm chart: a new Helm value Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-15T10:21:45.838ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
helm/cosmo/charts/controlplane/templates/deployment.yaml (1)
107-111: Verify if this environment variable should be conditional.The
AUTH_SSO_COOKIE_DOMAINenvironment variable is always set, unlike optional environment variables such asOPENAI_API_KEY(lines 367-373) orCDN_BASE_URL(lines 374-380) which are wrapped in conditionals.If SSO authentication is an optional feature, consider making this conditional to match the pattern:
♻️ Suggested conditional pattern
- name: AUTH_JWT_SECRET valueFrom: secretKeyRef: name: {{ include "controlplane.secretName" . }} key: jwtSessionSecret + {{- if .Values.configuration.authSsoCookieDomain }} - name: AUTH_SSO_COOKIE_DOMAIN valueFrom: secretKeyRef: name: {{ include "controlplane.secretName" . }} key: authSsoCookieDomain + {{- end }} - name: DB_URL valueFrom:However, if SSO is a core required feature and the application expects this environment variable to always be present, then the current implementation is correct. The naming and placement are appropriate.
This verification relates to the same concern flagged in
secret.yaml. Please run the verification script provided in that review comment to confirm the application's expectations.helm/cosmo/charts/controlplane/templates/secret.yaml (1)
24-24: WrapauthSsoCookieDomainin a conditional to match the pattern for optional fields.The code treats
AUTH_SSO_COOKIE_DOMAINas optional (it's marked.optional()inenv.schema.tsand the application handles undefined values gracefully with a nullish coalescing operator). However, the Helm template unconditionally sets it in the secret, which is inconsistent with how other optional fields likesmtpUsernameands3AccessKeyIdare handled.♻️ Suggested fix
jwtSessionSecret: "{{ .Values.global.controlplane.jwtSecret }}" + {{- if .Values.configuration.authSsoCookieDomain }} authSsoCookieDomain: "{{ .Values.configuration.authSsoCookieDomain }}" + {{- end }} keycloakAdminUser: "{{ .Values.global.keycloak.adminUser }}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
helm/cosmo/charts/controlplane/templates/deployment.yamlhelm/cosmo/charts/controlplane/templates/secret.yamlhelm/cosmo/charts/controlplane/values.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2142
File: helm/cosmo/Chart.yaml:0-0
Timestamp: 2025-08-15T10:21:45.838Z
Learning: In the WunderGraph Cosmo project, helm chart version upgrades and README badge synchronization are handled in separate helm release PRs, not in the initial version bump PRs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
🔇 Additional comments (1)
helm/cosmo/charts/controlplane/values.yaml (1)
204-205: LGTM! Consider documenting production configuration.The new
authSsoCookieDomainconfiguration is well-placed and the comment clearly explains its purpose. The leading dot in the default value.wundergraph.localcorrectly allows the cookie to be shared across subdomains.Note that users will need to override this default value for production deployments to match their actual domain (e.g.,
.yourdomain.com).
|
@wilsonrivera did you test it end to end? |
…ould-show-sso-options
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist