refactor: update JWT private key handling and authentication mode val…#204
Closed
avaya09 wants to merge 2 commits into
Closed
refactor: update JWT private key handling and authentication mode val…#204avaya09 wants to merge 2 commits into
avaya09 wants to merge 2 commits into
Conversation
…idation in configuration
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens Helm chart configuration validation and refactors how certain Secrets are referenced, primarily to improve security posture around JWT signing keys and authentication mode selection.
Changes:
- Add Helm template-time validation for MinIO credentials (gateway chart) and for JWT private key + auth mode selection (app chart).
- Refactor app chart env var Secret references to allow overriding Secret names for log storage and Bedrock assumed-role credentials.
- Update app chart defaults (JWT private key empty, no-auth disabled by default, frontend Service type set to ClusterIP).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| charts/portkey-gateway/templates/_helpers.tpl | Adds MinIO auth key validation and wires it into MinIO secret naming. |
| charts/portkey-app/values.yaml | Updates default security/auth-related values and frontend Service type default. |
| charts/portkey-app/templates/_helpers.tpl | Adds config validations and refactors Secret name helpers for log storage/Bedrock credentials. |
Comments suppressed due to low confidence (1)
charts/portkey-app/values.yaml:99
- With the new auth-mode validation (called from
portkey.secretsName), leaving bothconfig.oauth.enabled=falseandconfig.noAuth.enabled=falsecauses Helm rendering to fail. Since this values file now defaults tonoAuth.enabled: false, add an inline note so users know they must enable one of the two modes (or consider adjusting defaults if the chart is expected to render with default values).
onboardingSupportMessage: ""
noAuth:
enabled: false
oauth:
enabled: false
oauthType: ""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+526
to
+535
| {{- define "minio.validateAuthKey" -}} | ||
| {{- if and .Values.minio.enabled .Values.minio.authKey.create (not .Values.minio.authKey.existingSecret) }} | ||
| {{- if not .Values.minio.authKey.accessKey }} | ||
| {{- fail "minio.authKey.accessKey must not be empty when minio.enabled and minio.authKey.create are true. Set it, or provide credentials via minio.authKey.existingSecret." }} | ||
| {{- end }} | ||
| {{- if not .Values.minio.authKey.secretKey }} | ||
| {{- fail "minio.authKey.secretKey must not be empty when minio.enabled and minio.authKey.create are true. Set it, or provide credentials via minio.authKey.existingSecret." }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
Comment on lines
+109
to
+113
| {{- define "portkey.validateAuthMode" -}} | ||
| {{- if and (not .Values.config.oauth.enabled) (not .Values.config.noAuth.enabled) }} | ||
| {{- fail "No authentication mode selected. Set config.oauth.enabled=true to use SSO, or explicitly opt into config.noAuth.enabled=true to run without authentication." }} | ||
| {{- end }} | ||
| {{- end }} |
…ntials in configuration
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
charts/portkey-app/values.yaml:99
portkey.validateAuthModerequires exactly one ofconfig.oauth.enabledorconfig.noAuth.enabledto be true, but the defaults invalues.yamlset both tofalse. That means the chart will fail to render/install with default values. If the intent is to force an explicit opt-in, consider making that requirement very explicit in docs and/or adjusting defaults to a valid mode.
noAuth:
enabled: false
oauth:
enabled: false
oauthType: ""
Comment on lines
+530
to
+535
| {{- if .Values.minio.enabled }} | ||
| {{- if and .Values.minio.authKey.create .Values.minio.authKey.existingSecret }} | ||
| {{- fail "minio.authKey.create and minio.authKey.existingSecret are mutually exclusive. Set create=true to have the chart create the Secret, or provide existingSecret (with create=false) to use your own." }} | ||
| {{- end }} | ||
| {{- if .Values.minio.authKey.create }} | ||
| {{- if not .Values.minio.authKey.accessKey }} |
Comment on lines
+188
to
+211
| {{/* | ||
| Name of the Secret holding log storage credentials. | ||
| Overridable via logStorage.existingSecretName. | ||
| */}} | ||
| {{- define "portkey.logStoreSecretsName" -}} | ||
| {{- if .Values.logStorage.existingSecretName }} | ||
| {{- .Values.logStorage.existingSecretName }} | ||
| {{- else }} | ||
| {{- include "portkey.gatewaySecretsName" . }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
| Name of the Secret holding bedrockAssumed credentials. | ||
| Overridable via bedrockAssumed.existingSecretName; otherwise the keys | ||
| live in the chart-managed gateway Secret. | ||
| */}} | ||
| {{- define "portkey.bedrockSecretsName" -}} | ||
| {{- if .Values.bedrockAssumed.existingSecretName }} | ||
| {{- .Values.bedrockAssumed.existingSecretName }} | ||
| {{- else }} | ||
| {{- include "portkey.gatewaySecretsName" . }} | ||
| {{- end }} | ||
| {{- end }} |
Comment on lines
665
to
667
| - name: ENABLE_PROMETHEUS | ||
|
|
||
| value: {{ if .Values.apm.grafana.prometheus.enabled }} "true" {{ else }} "false" {{ end }} |
Comment on lines
82
to
86
| existingSecretName: "" | ||
| defaultGatewayURL: "" | ||
| defaultGatewayClientAuth: "" | ||
| jwtPrivateKey: "<some-random-string>" | ||
| jwtPrivateKey: "" # Set to a strong secret for the JWT private key used for signing the frontend JWT tokens | ||
| controlPlaneURL: "" |
Comment on lines
541
to
543
| service: | ||
| type: LoadBalancer | ||
| type: ClusterIP | ||
| httpPort: 80 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…idation in configuration