refactor: streamline OAuth configuration in templates by using defaul…#174
refactor: streamline OAuth configuration in templates by using defaul…#174
Conversation
…t values for secrets
There was a problem hiding this comment.
Pull request overview
This PR refactors the Helm chart’s OAuth/SSO configuration templating to reduce conditional blocks by always rendering OAuth-related Secret keys (with defaults) and always wiring the corresponding environment variables when OAuth is enabled.
Changes:
- Render OAuth secret data keys unconditionally (within
config.oauth.enabled) usingdefault ""fallbacks. - Render OAuth-related environment variables unconditionally (within
config.oauth.enabled) by always referencing the Secret keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| charts/portkey-app/templates/secrets.yaml | Always emits OAuth-related Secret data entries (defaulting to empty) when OAuth is enabled. |
| charts/portkey-app/templates/_helpers.tpl | Always injects OAuth/SSO env vars when OAuth is enabled, removing per-field conditional blocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.config.oauth.enabled }} | ||
| - name: AUTH_MODE | ||
| value: "SSO" | ||
| {{- if .Values.config.oauth.oauthType }} | ||
| - name: AUTH_SSO_TYPE | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthType | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthIssuerUrl }} | ||
| - name: OIDC_ISSUER | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthIssuerUrl | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthClientId }} | ||
| - name: OIDC_CLIENTID | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthClientId | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthClientSecret }} | ||
| - name: OIDC_CLIENT_SECRET | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthClientSecret | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthRedirectURI }} | ||
| - name: OIDC_REDIRECT_URI | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthRedirectURI | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthMetadataXml }} | ||
| - name: SAML_METADATA_XML | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthMetadataXml |
There was a problem hiding this comment.
OAuth env vars are now always injected whenever config.oauth.enabled is true. This is a breaking behavior change for installs using config.existingSecretName: the referenced Secret must now contain all keys (oauthType, oauthIssuerUrl, oauthClientId, oauthClientSecret, oauthRedirectURI, oauthMetadataXml) or pods will fail with CreateContainerConfigError due to missing secret keys. Consider adding optional: true on these secretKeyRefs or restoring per-field conditionals (or gating OIDC vs SAML keys by oauthType) to preserve backwards compatibility.
…proved flexibility
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.config.oauth.enabled }} | ||
| - name: AUTH_MODE | ||
| value: "SSO" | ||
| {{- if .Values.config.oauth.oauthType }} | ||
| - name: AUTH_SSO_TYPE | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthType | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthIssuerUrl }} | ||
| optional: true | ||
| - name: OIDC_ISSUER | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthIssuerUrl | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthClientId }} | ||
| optional: true |
There was a problem hiding this comment.
All SSO env vars are now sourced from secret keys marked optional: true. This allows pods to start even if the referenced key is missing, which can silently deploy an unusable SSO setup (docs show oauthType/issuer/client details are required when SSO is enabled). Consider making optional conditional (e.g., only true when config.existingSecretName is set), and/or using Helm validation (fail/required) to enforce required values when the chart is managing the Secret.
… including fields
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: {{ include "portkey.secretsName" . }} | ||
| key: oauthType | ||
| {{- end }} | ||
| {{- if .Values.config.oauth.oauthIssuerUrl }} | ||
| optional: true | ||
| - name: OIDC_ISSUER | ||
| valueFrom: |
There was a problem hiding this comment.
Setting optional: true on these OAuth secretKeyRefs means the pod will start even when the referenced key is missing from an existingSecretName secret. Previously, if a user set the corresponding .Values.config.oauth.* value (thereby indicating the key should exist), Kubernetes would fail fast when the key was absent. Consider making optional conditional (e.g., only true when the matching .Values.config.oauth.* value is empty) so misconfigurations are still caught early while still allowing configs that are provided solely via an existing Secret.
…t values for secrets