Skip to content

Replace custom EnvVar with corev1.EnvVar #4570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anveshthakur wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Replace custom EnvVar with corev1.EnvVar #4570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5cc6965
Replace custom EnvVar with corev1.EnvVar
anveshthakur Apr 6, 2026
cfac2af
Merge branch 'main' into issue-4547
anveshthakur Apr 6, 2026
345f1ca
Merge branch 'main' into issue-4547
anveshthakur Apr 7, 2026
b29cd5e
updates controller code
anveshthakur Apr 7, 2026
ca9dc98
Merge branch 'main' into issue-4547
anveshthakur Apr 7, 2026
7a502e8
Replace custom EnvVar with corev1.EnvVar
anveshthakur Apr 6, 2026
c1388bc
updates controller code
anveshthakur Apr 7, 2026
6b4cd67
Merge branch 'issue-4547' of https://github.com/anveshthakur/toolhive…
anveshthakur Apr 9, 2026
8dcfd7b
Merge branch 'main' into issue-4547
anveshthakur Apr 9, 2026
82fd867
Merge branch 'main' of https://github.com/stacklok/toolhive into issu…
anveshthakur Apr 11, 2026
0486ace
Merge branch 'issue-4547' of https://github.com/anveshthakur/toolhive…
anveshthakur Apr 11, 2026
d37b279
changes implementation to corev1.EnvVar
anveshthakur Apr 11, 2026
b42ab61
removes EnvVar completely
anveshthakur Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/thv-operator/api/v1alpha1/embeddingserver_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package v1alpha1

import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
)
Expand Down Expand Up @@ -77,7 +78,7 @@ type EmbeddingServerSpec struct {
// +listType=map
// +listMapKey=name
// +optional
Env []EnvVar `json:"env,omitempty"`
Env []corev1.EnvVar `json:"env,omitempty"`
Copy link
Copy Markdown
Collaborator

@ChrisJBurns ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

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

[HIGH] F3: EmbeddingServer controller buildEnvVars also drops ValueFrom (Consensus: 9/10)

The EmbeddingServerReconciler.buildEnvVars function at embeddingserver_controller.go:640-644 manually copies only Name/Value into new corev1.EnvVar structs, discarding ValueFrom. Since this function already returns []corev1.EnvVar, the fix is straightforward: replace the conversion loop with envVars = append(envVars, embedding.Spec.Env...) to pass through the full struct, matching the pattern used in the MCPServer proxy deployment code.

Raised by: kubernetes-expert, code-reviewer


// Resources defines compute resources for the embedding server
// +optional
Expand Down
22 changes: 11 additions & 11 deletions cmd/thv-operator/api/v1alpha1/mcpserver_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ type MCPServerSpec struct {
// +listType=map
// +listMapKey=name
// +optional
Env []EnvVar `json:"env,omitempty"`
Env []corev1.EnvVar `json:"env,omitempty"`

// Volumes are volumes to mount in the MCP server container
// +listType=map
Expand Down Expand Up @@ -408,7 +408,7 @@ type ProxyDeploymentOverrides struct {
// +listType=map
// +listMapKey=name
// +optional
Env []EnvVar `json:"env,omitempty"`
Env []corev1.EnvVar `json:"env,omitempty"`

// ImagePullSecrets allows specifying image pull secrets for the proxy runner
// These are applied to both the Deployment and the ServiceAccount
Expand All @@ -429,15 +429,15 @@ type ResourceMetadataOverrides struct {
}

// EnvVar represents an environment variable in a container
type EnvVar struct {
// Name of the environment variable
// +kubebuilder:validation:Required
Name string `json:"name"`

// Value of the environment variable
// +kubebuilder:validation:Required
Value string `json:"value"`
}
// type EnvVar struct {
// // Name of the environment variable
// // +kubebuilder:validation:Required
// Name string `json:"name"`

// // Value of the environment variable
// // +kubebuilder:validation:Required
// Value string `json:"value"`
// }
Comment on lines 431 to +440
Copy link
Copy Markdown
Collaborator

@ChrisJBurns ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

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

[MEDIUM] F4: Custom EnvVar type commented out, not deleted (Consensus: 9/10)

Dead commented-out code should be deleted entirely. Git history preserves the original type definition if it's ever needed for reference. The zz_generated.deepcopy.go already removed the EnvVar.DeepCopyInto and EnvVar.DeepCopy methods, confirming the type is no longer used anywhere.

Delete this entire block (lines 431-440).

Raised by: kubernetes-expert, code-reviewer, toolhive-expert


// Volume represents a volume to mount in a container
type Volume struct {
Expand Down
67 changes: 29 additions & 38 deletions cmd/thv-operator/api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func TestDeploymentForMCPRemoteProxy(t *testing.T) {
"custom-annotation": "custom-annotation-value",
},
},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "CUSTOM_ENV", Value: "custom-value"},
{Name: "TOOLHIVE_DEBUG", Value: "true"},
},
Expand Down
14 changes: 2 additions & 12 deletions cmd/thv-operator/controllers/mcpserver_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,12 +1149,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(

// Add user-specified proxy environment variables from ResourceOverrides
if m.Spec.ResourceOverrides != nil && m.Spec.ResourceOverrides.ProxyDeployment != nil {
for _, envVar := range m.Spec.ResourceOverrides.ProxyDeployment.Env {
env = append(env, corev1.EnvVar{
Name: envVar.Name,
Value: envVar.Value,
})
}
env = append(env, m.Spec.ResourceOverrides.ProxyDeployment.Env...)
Copy link
Copy Markdown
Collaborator

@ChrisJBurns ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

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

[MEDIUM] F6: Direct pass-through now includes FieldRef/ResourceFieldRef (Consensus: 7/10)

Nice simplification, but the full corev1.EnvVar type also includes ValueFrom.FieldRef and ValueFrom.ResourceFieldRef, which can leak node/pod metadata (hostIP, nodeName, etc.) to MCP server containers. Consider whether this expanded attack surface is acceptable, or whether validation should restrict ValueFrom to only SecretKeyRef and ConfigMapKeyRef.

Raised by: go-security-reviewer

}

// Add volume mounts for user-defined volumes
Expand Down Expand Up @@ -1762,12 +1757,7 @@ func (r *MCPServerReconciler) deploymentNeedsUpdate(

// Add user-specified environment variables
if mcpServer.Spec.ResourceOverrides != nil && mcpServer.Spec.ResourceOverrides.ProxyDeployment != nil {
for _, envVar := range mcpServer.Spec.ResourceOverrides.ProxyDeployment.Env {
expectedProxyEnv = append(expectedProxyEnv, corev1.EnvVar{
Name: envVar.Name,
Value: envVar.Value,
})
}
expectedProxyEnv = append(expectedProxyEnv, mcpServer.Spec.ResourceOverrides.ProxyDeployment.Env...)
}
// Add default environment variables that are always injected
expectedProxyEnv = ctrlutil.EnsureRequiredEnvVars(ctx, expectedProxyEnv)
Expand Down
14 changes: 7 additions & 7 deletions cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func TestResourceOverrides(t *testing.T) {
"environment": "test",
},
},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{
Name: "HTTP_PROXY",
Value: "http://proxy.example.com:8080",
Expand Down Expand Up @@ -203,7 +203,7 @@ func TestResourceOverrides(t *testing.T) {
ProxyPort: 8080,
ResourceOverrides: &mcpv1alpha1.ResourceOverrides{
ProxyDeployment: &mcpv1alpha1.ProxyDeploymentOverrides{
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TOOLHIVE_DEBUG", Value: "true"},
},
},
Expand Down Expand Up @@ -249,7 +249,7 @@ func TestResourceOverrides(t *testing.T) {
"version": "v1.2.3",
},
},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{
Name: "LOG_LEVEL",
Value: "debug",
Expand Down Expand Up @@ -483,7 +483,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) {
ProxyPort: 8080,
ResourceOverrides: &mcpv1alpha1.ResourceOverrides{
ProxyDeployment: &mcpv1alpha1.ProxyDeploymentOverrides{
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://proxy.example.com:8080"},
{Name: "NO_PROXY", Value: "localhost,127.0.0.1"},
},
Expand All @@ -509,7 +509,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) {
ProxyPort: 8080,
ResourceOverrides: &mcpv1alpha1.ResourceOverrides{
ProxyDeployment: &mcpv1alpha1.ProxyDeploymentOverrides{
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://new-proxy.example.com:8080"},
{Name: "NO_PROXY", Value: "localhost,127.0.0.1"},
},
Expand All @@ -535,7 +535,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) {
ProxyPort: 8080,
ResourceOverrides: &mcpv1alpha1.ResourceOverrides{
ProxyDeployment: &mcpv1alpha1.ProxyDeploymentOverrides{
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://proxy.example.com:8080"},
{Name: "NO_PROXY", Value: "localhost,127.0.0.1"},
{Name: "CUSTOM_ENV", Value: "custom-value"},
Expand All @@ -562,7 +562,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) {
ProxyPort: 8080,
ResourceOverrides: &mcpv1alpha1.ResourceOverrides{
ProxyDeployment: &mcpv1alpha1.ProxyDeploymentOverrides{
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://proxy.example.com:8080"},
},
},
Expand Down
2 changes: 1 addition & 1 deletion cmd/thv-operator/controllers/mcpserver_runconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ func (*MCPServerReconciler) validateToolsFilter(config *runner.RunConfig) error
}

// convertEnvVarsFromMCPServer converts MCPServer environment variables to builder format
func convertEnvVarsFromMCPServer(envs []mcpv1alpha1.EnvVar) map[string]string {
func convertEnvVarsFromMCPServer(envs []corev1.EnvVar) map[string]string {
Copy link
Copy Markdown
Contributor

@jerm-dro jerm-dro Apr 6, 2026

Choose a reason for hiding this comment

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

blocker: The issue (#4547) notes that controller conversion logic should be simplified now that the types match directly. This function converts to map[string]string which is fine for the RunConfig path (JSON config file). But the same Name/Value-only copy pattern exists at mcpserver_controller.go:1142 and :1734, where it builds a new corev1.EnvVar{Name: envVar.Name, Value: envVar.Value} from what is already a corev1.EnvVar — silently dropping ValueFrom when setting env vars on the proxy Deployment pod spec.

Those two locations should be simplified to:

env = append(env, m.Spec.ResourceOverrides.ProxyDeployment.Env...)

Without this, valueFrom references on proxy env vars won't propagate to the pod spec.

Copy link
Copy Markdown
Author

@anveshthakur anveshthakur Apr 7, 2026

Choose a reason for hiding this comment

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

Good Catch ! Totally missed these
Made changes for this.

@jerm-dro

Copy link
Copy Markdown
Collaborator

@ChrisJBurns ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

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

[HIGH] F2: convertEnvVarsFromMCPServer silently drops ValueFrom (Consensus: 10/10)

This function now accepts []corev1.EnvVar but only reads .Name and .Value. If a user specifies valueFrom: secretKeyRef: ..., the value will silently be set to an empty string. The downstream runner.RunConfig uses map[string]string and cannot support ValueFrom references.

Add validation that detects and rejects ValueFrom entries with a clear error rather than silently dropping the data.

Raised by: kubernetes-expert, code-reviewer, toolhive-expert, go-security-reviewer

if len(envs) == 0 {
return nil
}
Expand Down
Loading