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 9 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 2 commits
Commits
Show all changes
9 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
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
4 changes: 2 additions & 2 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 @@ -193,7 +193,7 @@ type MCPServerSpec struct {

// Env are environment variables to set in the MCP server container
// +optional
Env []EnvVar `json:"env,omitempty"`
Env []corev1.EnvVar `json:"env,omitempty"`

// Volumes are volumes to mount in the MCP server container
// +optional
Expand Down Expand Up @@ -348,7 +348,7 @@ type ProxyDeploymentOverrides struct {
// These affect the toolhive proxy itself, not the MCP server it manages
// Use TOOLHIVE_DEBUG=true to enable debug logging in the proxy
// +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 Down
6 changes: 3 additions & 3 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: 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 @@ -551,7 +551,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

if len(envs) == 0 {
return nil
}
Expand Down
12 changes: 6 additions & 6 deletions cmd/thv-operator/controllers/mcpserver_runconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func createRunConfigTestScheme() *runtime.Scheme {
return testScheme
}

func createTestMCPServerWithConfig(name, namespace, image string, envVars []mcpv1alpha1.EnvVar) *mcpv1alpha1.MCPServer {
func createTestMCPServerWithConfig(name, namespace, image string, envVars []corev1.EnvVar) *mcpv1alpha1.MCPServer {
return &mcpv1alpha1.MCPServer{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand Down Expand Up @@ -96,7 +96,7 @@ func TestCreateRunConfigFromMCPServer(t *testing.T) {
Image: "env-image:latest",
Transport: "sse",
ProxyPort: 9090,
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "VAR1", Value: "value1"},
{Name: "VAR2", Value: "value2"},
},
Expand Down Expand Up @@ -322,7 +322,7 @@ func TestCreateRunConfigFromMCPServer(t *testing.T) {
McpPort: 8080,
ProxyMode: "streamable-http",
Args: []string{"--comprehensive", "--test"},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "ENV1", Value: "value1"},
{Name: "ENV2", Value: "value2"},
{Name: "EMPTY_VALUE", Value: ""},
Expand Down Expand Up @@ -611,7 +611,7 @@ func TestDeterministicConfigMapGeneration(t *testing.T) {
ProxyPort: 9090,
McpPort: 8080,
Args: []string{"--arg1", "--arg2", "--complex-flag=value"},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "VAR_C", Value: "value_c"},
{Name: "VAR_A", Value: "value_a"},
{Name: "VAR_B", Value: "value_b"},
Expand Down Expand Up @@ -1324,7 +1324,7 @@ func TestEnsureRunConfigConfigMapCompleteFlow(t *testing.T) {
}

// Step 1: Create initial MCPServer and ConfigMap
mcpServer := createTestMCPServerWithConfig("flow-server", "flow-ns", "test:v1", []mcpv1alpha1.EnvVar{
mcpServer := createTestMCPServerWithConfig("flow-server", "flow-ns", "test:v1", []corev1.EnvVar{
{Name: "ENV1", Value: "value1"},
})

Expand Down Expand Up @@ -1355,7 +1355,7 @@ func TestEnsureRunConfigConfigMapCompleteFlow(t *testing.T) {
// The checksum will automatically change when content changes

mcpServer.Spec.Image = "test:v2"
mcpServer.Spec.Env = []mcpv1alpha1.EnvVar{
mcpServer.Spec.Env = []corev1.EnvVar{
{Name: "ENV1", Value: "value1"},
{Name: "ENV2", Value: "value2"},
}
Expand Down
10 changes: 1 addition & 9 deletions cmd/thv-operator/test-integration/mcp-server/mcpserver_controller_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ var _ = Describe("MCPServer Controller Integration Tests", func() {
ProxyPort: 8080,
McpPort: 8080,
Args: []string{"--verbose"},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{
Name: "DEBUG",
Value: "true",
Expand Down Expand Up @@ -113,7 +113,6 @@ var _ = Describe("MCPServer Controller Integration Tests", func() {
})

It("Should create a Deployment with proper configuration", func() {

// Wait for Deployment to be created
deployment := &appsv1.Deployment{}
Eventually(func() error {
Expand Down Expand Up @@ -229,11 +228,9 @@ var _ = Describe("MCPServer Controller Integration Tests", func() {
Expect(container.ReadinessProbe.ProbeHandler.HTTPGet.Port).To(Equal(intstr.FromString("http")))
Expect(container.ReadinessProbe.InitialDelaySeconds).To(Equal(int32(5)))
Expect(container.ReadinessProbe.PeriodSeconds).To(Equal(int32(5)))

})

It("Should create the RunConfig ConfigMap", func() {

// Wait for Service to be created (using the correct naming pattern)
configMap := &corev1.ConfigMap{}
configMapName := mcpServerName + "-runconfig"
Expand All @@ -253,7 +250,6 @@ var _ = Describe("MCPServer Controller Integration Tests", func() {
})

It("Should create a Service for the MCPServer Proxy", func() {

// Wait for Service to be created (using the correct naming pattern)
service := &corev1.Service{}
serviceName := "mcp-" + mcpServerName + "-proxy"
Expand All @@ -271,11 +267,9 @@ var _ = Describe("MCPServer Controller Integration Tests", func() {
Expect(service.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP))
Expect(service.Spec.Ports).To(HaveLen(1))
Expect(service.Spec.Ports[0].Port).To(Equal(int32(8080)))

})

It("Should create RBAC resources when ServiceAccount is not specified", func() {

// Wait for ServiceAccount to be created
serviceAccountName := mcpServerName + "-proxy-runner"
serviceAccount := &corev1.ServiceAccount{}
Expand Down Expand Up @@ -320,7 +314,6 @@ var _ = Describe("MCPServer Controller Integration Tests", func() {
Expect(roleBinding.Subjects).To(HaveLen(1))
Expect(roleBinding.Subjects[0].Name).To(Equal(serviceAccountName))
Expect(roleBinding.RoleRef.Name).To(Equal(serviceAccountName))

})

It("Should set ObservedGeneration in status after reconciliation", func() {
Expand Down Expand Up @@ -359,7 +352,6 @@ var _ = Describe("MCPServer Controller Integration Tests", func() {
})

It("Should update Deployment when MCPServer spec changes", func() {

// Wait for Deployment to be created
deployment := &appsv1.Deployment{}
Eventually(func() error {
Expand Down
6 changes: 3 additions & 3 deletions cmd/thv-operator/test-integration/mcp-server/mcpserver_runconfig_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ var _ = Describe("RunConfig ConfigMap Integration Tests", func() {
ProxyPort: 8080,
McpPort: 8081,
Args: []string{"--verbose", "--debug"},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{
Name: "DEBUG",
Value: "true",
Expand Down Expand Up @@ -300,7 +300,7 @@ var _ = Describe("RunConfig ConfigMap Integration Tests", func() {
// Update multiple fields
mcpServer.Spec.Image = "example/mcp-server:v2.0.0"
mcpServer.Spec.ProxyPort = 9090
mcpServer.Spec.Env = append(mcpServer.Spec.Env, mcpv1alpha1.EnvVar{
mcpServer.Spec.Env = append(mcpServer.Spec.Env, corev1.EnvVar{
Name: "NEW_VAR",
Value: "new_value",
})
Expand Down Expand Up @@ -777,7 +777,7 @@ var _ = Describe("RunConfig ConfigMap Integration Tests", func() {
ProxyPort: 9090,
McpPort: 8080,
Args: []string{"--arg1", "--arg2", "--arg3"},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "VAR_C", Value: "value_c"},
{Name: "VAR_A", Value: "value_a"},
{Name: "VAR_B", Value: "value_b"},
Expand Down
6 changes: 2 additions & 4 deletions docs/operator/crd-api.md
View file
Open in desktop

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

5 changes: 3 additions & 2 deletions pkg/export/k8s.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"
"strings"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/yaml"

Expand Down Expand Up @@ -94,9 +95,9 @@ func runConfigToMCPServer(config *runner.RunConfig) (*v1alpha1.MCPServer, error)

// Convert environment variables
if len(config.EnvVars) > 0 {
mcpServer.Spec.Env = make([]v1alpha1.EnvVar, 0, len(config.EnvVars))
mcpServer.Spec.Env = make([]corev1.EnvVar, 0, len(config.EnvVars))
for key, value := range config.EnvVars {
mcpServer.Spec.Env = append(mcpServer.Spec.Env, v1alpha1.EnvVar{
mcpServer.Spec.Env = append(mcpServer.Spec.Env, corev1.EnvVar{
Name: key,
Value: value,
})
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/thv-operator/virtualmcp/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ func CreateMCPServerAndWait(
ProxyPort: 8080,
McpPort: 8080,
Resources: defaultMCPServerResources(),
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TRANSPORT", Value: "streamable-http"},
},
},
Expand Down Expand Up @@ -806,7 +806,7 @@ type BackendConfig struct {
Transport string // defaults to "streamable-http" if empty
ExternalAuthConfigRef *mcpv1alpha1.ExternalAuthConfigRef
Secrets []mcpv1alpha1.SecretRef
Env []mcpv1alpha1.EnvVar // additional env vars beyond TRANSPORT
Env []corev1.EnvVar // additional env vars beyond TRANSPORT
// Resources overrides the default resource requests/limits. When nil,
// defaultMCPServerResources() is used to ensure containers are scheduled
// with reasonable resource guarantees and do not compete excessively.
Expand Down Expand Up @@ -864,7 +864,7 @@ func CreateMultipleMCPServersInParallel(
ExternalAuthConfigRef: backends[idx].ExternalAuthConfigRef,
Secrets: backends[idx].Secrets,
Resources: resources,
Env: append([]mcpv1alpha1.EnvVar{
Env: append([]corev1.EnvVar{
{Name: "TRANSPORT", Value: backendTransport},
}, backends[idx].Env...),
},
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/thv-operator/virtualmcp/virtualmcp_auth_discovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1454,7 +1454,7 @@ var _ = Describe("Auth Config Error Handling", Ordered, func() {
ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{
Name: workingAuthConfigName,
},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TRANSPORT", Value: "streamable-http"},
},
},
Expand All @@ -1477,7 +1477,7 @@ var _ = Describe("Auth Config Error Handling", Ordered, func() {
ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{
Name: missingAuthConfigName,
},
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TRANSPORT", Value: "streamable-http"},
},
},
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/thv-operator/virtualmcp/virtualmcp_circuit_breaker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ var _ = Describe("VirtualMCPServer Circuit Breaker Lifecycle", Ordered, func() {
Transport: "streamable-http",
ProxyPort: 8080,
McpPort: 8080,
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TRANSPORT", Value: "streamable-http"},
},
},
Expand All @@ -76,7 +76,7 @@ var _ = Describe("VirtualMCPServer Circuit Breaker Lifecycle", Ordered, func() {
Transport: "streamable-http",
ProxyPort: 8080,
McpPort: 8080,
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TRANSPORT", Value: "streamable-http"},
},
},
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/thv-operator/virtualmcp/virtualmcp_external_auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ var _ = Describe("VirtualMCPServer Health Check with HeaderInjection Auth", Orde
Transport: "streamable-http",
ProxyPort: 8080,
McpPort: 8080,
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TRANSPORT", Value: "streamable-http"},
},
ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{
Expand Down Expand Up @@ -1051,7 +1051,7 @@ var _ = Describe("VirtualMCPServer Health Check with TokenExchange Auth", Ordere
Transport: "streamable-http",
ProxyPort: 8080,
McpPort: 8080,
Env: []mcpv1alpha1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TRANSPORT", Value: "streamable-http"},
},
ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{
Expand Down
Loading