Skip to content

feat: enhance serverMode retrieval in helper template#162

Open
avaya09 wants to merge 1 commit intomainfrom
fix/servermode-condition
Open

feat: enhance serverMode retrieval in helper template#162
avaya09 wants to merge 1 commit intomainfrom
fix/servermode-condition

Conversation

@avaya09
Copy link
Contributor

@avaya09 avaya09 commented Mar 5, 2026

  • Updated the mcp.serverMode function to read SERVER_MODE from a Kubernetes Secret when using vault injection or an existing secret.
  • Added fallback logic to retrieve SERVER_MODE from environment data if the secret is not available.
  • Improved handling of empty serverMode values to ensure proper trimming and conversion.

- Updated the mcp.serverMode function to read SERVER_MODE from a Kubernetes Secret when using vault injection or an existing secret.
- Added fallback logic to retrieve SERVER_MODE from environment data if the secret is not available.
- Improved handling of empty serverMode values to ensure proper trimming and conversion.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the mcp.serverMode Helm template helper to correctly determine the SERVER_MODE value at render time when the value is stored in an external Kubernetes Secret (either via Vault injection or an existing secret reference). Previously, the function used portkeyenterprise.commonEnvMap which returns valueFrom references rather than decoded values — making it unsuitable for conditional logic. The new implementation uses lookup to read and decode the actual secret value, with a fallback to environment.data when the secret is unavailable.

Changes:

  • Rewrote mcp.serverMode to use Kubernetes secret lookup for vault and existingSecret scenarios instead of relying on commonEnvMap.
  • Added base64 decoding (b64dec) and trimming pipeline for values read directly from secret data.
  • Added fallback to environment.data["SERVER_MODE"] when neither the vault secret nor the existing secret is available or contains the key.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{{- end -}}
{{- else -}}
{{- $serverMode = (index .Values.environment.data "SERVER_MODE") | default "" | toString -}}
{{- else if and (not .Values.environment.create) .Values.environment.existingSecret -}}
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existingSecret branch (line 375) uses a stricter condition — and (not .Values.environment.create) .Values.environment.existingSecret — compared to the existing portkeyenterprise.commonEnvMap function which uses simply else if .Values.environment.existingSecret (line 256). While the resulting behavior is functionally equivalent for SERVER_MODE (since when create=true, the chart-managed secret's value equals environment.data["SERVER_MODE"]), the inconsistency could cause confusion during maintenance. More importantly, this means when someone sets environment.create: true as an override (i.e., the default) together with useVaultInjection: false, the lookup of the chart-managed secret is skipped entirely. Since that managed secret's SERVER_MODE value is sourced directly from environment.data["SERVER_MODE"], the fallback is semantically correct — but the approach is asymmetric with how other functions handle the same conditions. Consider simplifying the condition to just else if .Values.environment.existingSecret (removing the not .Values.environment.create guard) to be consistent with the rest of the helpers.

Suggested change
{{- else if and (not .Values.environment.create) .Values.environment.existingSecret -}}
{{- else if .Values.environment.existingSecret -}}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants