Skip to content

Commit b36ed41

Browse files
JAORMXclaude
JAORMX
and
claude
committed
Add operator-level defaultImagePullSecrets plumbing
Cluster operators frequently need a registry pull secret applied to every workload the operator spawns (proxy runners, registry API, vMCP servers, embedding servers). Today the chart only exposes imagePullSecrets for the operator's own pod, forcing users to set the secret on every CR or to mutate the namespace-default ServiceAccount. This change introduces a chart value, operator.defaultImagePullSecrets, that the operator picks up at startup via THV_DEFAULT_IMAGE_PULL_SECRETS and applies as a default to every workload it spawns. All five workload-spawning reconcilers consume the shared imagepullsecrets.Defaults value and merge it with the per-CR list at workload-construction time: MCPServer, MCPRemoteProxy, MCPRegistry (via registryapi.manager), VirtualMCPServer, and EmbeddingServer. Precedence rule: per-CR imagePullSecrets take priority on name collisions; chart-level entries are appended additively and deduped by Name. The CR-level slice is never mutated. EmbeddingServer places the chart defaults on the base PodSpec and lets strategic-merge-patch additively union the user's PodTemplateSpec entries (PodSpec.ImagePullSecrets is declared with patchStrategy:"merge",patchMergeKey:"name"). Drift detection on every controller routes through the same merge helper as the construction site so chart defaults do not flag perpetual reconcile loops. The Helm template renders operator.env before chart-managed env vars so a user-supplied entry cannot silently override a reserved name like THV_DEFAULT_IMAGE_PULL_SECRETS — Kubernetes keeps the last entry on a duplicate-named env. The startup parser logs a diagnostic when the env var is set but parses to nothing (typos like " , " or ",,,") so the misconfiguration is visible. Part of #5102 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 4c23ade commit b36ed41

22 files changed

Lines changed: 1116 additions & 103 deletions

‎cmd/thv-operator/controllers/embeddingserver_controller.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929

3030
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
3131
ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
32+
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/imagepullsecrets"
3233
)
3334

3435
// EmbeddingServerReconciler reconciles a EmbeddingServer object
@@ -37,6 +38,13 @@ type EmbeddingServerReconciler struct {
3738
Scheme *runtime.Scheme
3839
Recorder events.EventRecorder
3940
PlatformDetector *ctrlutil.SharedPlatformDetector
41+
// ImagePullSecretsDefaults are cluster-wide defaults sourced from the
42+
// operator chart, applied to the StatefulSet's PodSpec before the
43+
// user-provided PodTemplateSpec strategic-merge patch runs. The strategic
44+
// merge with the user PTS continues to additively merge the user's
45+
// imagePullSecrets entries on top, with the user's entries winning on
46+
// name collisions per Kubernetes' strategic-merge semantics.
47+
ImagePullSecretsDefaults imagepullsecrets.Defaults
4048
}
4149

4250
const (
@@ -644,7 +652,12 @@ func (*EmbeddingServerReconciler) applyResourceRequirements(embedding *mcpv1beta
644652
// buildPodTemplate builds the pod template for the statefulset.
645653
// User-provided PodTemplateSpec customizations are applied later in
646654
// statefulSetForEmbedding via strategic merge patch.
647-
func (*EmbeddingServerReconciler) buildPodTemplate(
655+
//
656+
// Cluster-wide chart defaults for imagePullSecrets are placed on the base
657+
// PodSpec here so that a subsequent strategic-merge with the user PTS
658+
// additively unions the lists (Kubernetes treats PodSpec.ImagePullSecrets
659+
// as a merge list keyed on Name; user entries win on name collisions).
660+
func (r *EmbeddingServerReconciler) buildPodTemplate(
648661
labels map[string]string,
649662
container corev1.Container,
650663
) corev1.PodTemplateSpec {
@@ -655,7 +668,8 @@ func (*EmbeddingServerReconciler) buildPodTemplate(
655668
Labels: labels,
656669
},
657670
Spec: corev1.PodSpec{
658-
Containers: []corev1.Container{container},
671+
ImagePullSecrets: r.ImagePullSecretsDefaults.List(),
672+
Containers: []corev1.Container{container},
659673
},
660674
}
661675
}

‎cmd/thv-operator/controllers/embeddingserver_default_imagepullsecrets_test.go‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package controllers
5+
6+
import (
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
corev1 "k8s.io/api/core/v1"
12+
13+
ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
14+
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/imagepullsecrets"
15+
)
16+
17+
// TestEmbeddingServer_DefaultImagePullSecrets verifies that cluster-wide
18+
// chart defaults reach the StatefulSet's PodSpec.ImagePullSecrets.
19+
//
20+
// EmbeddingServer has no per-CR imagePullSecrets field; users add their own
21+
// entries via spec.podTemplateSpec.spec.imagePullSecrets, which is
22+
// strategic-merged on top of this base list. The strategic-merge behavior
23+
// (additive union keyed by Name) is exercised by integration tests against a
24+
// real K8s API; here we only assert the chart defaults reach the base PodSpec.
25+
func TestEmbeddingServer_DefaultImagePullSecrets(t *testing.T) {
26+
t.Parallel()
27+
28+
tests := []struct {
29+
name string
30+
defaults []string
31+
wantSecrets []corev1.LocalObjectReference
32+
}{
33+
{
34+
name: "chart defaults reach base PodSpec",
35+
defaults: []string{"chart-default", "second-default"},
36+
wantSecrets: []corev1.LocalObjectReference{
37+
{Name: "chart-default"},
38+
{Name: "second-default"},
39+
},
40+
},
41+
{
42+
name: "no defaults yields nil ImagePullSecrets",
43+
defaults: nil,
44+
wantSecrets: nil,
45+
},
46+
}
47+
48+
for _, tt := range tests {
49+
t.Run(tt.name, func(t *testing.T) {
50+
t.Parallel()
51+
52+
embedding := createTestEmbeddingServer(
53+
"default-pullsecrets-embed",
54+
testNamespaceDefault,
55+
"image:latest",
56+
"model",
57+
)
58+
59+
scheme := createEmbeddingServerTestScheme()
60+
reconciler := &EmbeddingServerReconciler{
61+
Scheme: scheme,
62+
PlatformDetector: ctrlutil.NewSharedPlatformDetector(),
63+
ImagePullSecretsDefaults: imagepullsecrets.NewDefaults(tt.defaults),
64+
}
65+
66+
sts := reconciler.statefulSetForEmbedding(t.Context(), embedding)
67+
require.NotNil(t, sts)
68+
assert.Equal(t, tt.wantSecrets, sts.Spec.Template.Spec.ImagePullSecrets,
69+
"StatefulSet PodSpec ImagePullSecrets must reflect chart defaults")
70+
})
71+
}
72+
}

‎cmd/thv-operator/controllers/mcpregistry_controller.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"sigs.k8s.io/controller-runtime/pkg/log"
2323

2424
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
25+
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/imagepullsecrets"
2526
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/registryapi"
2627
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/registryapi/config"
2728
)
@@ -47,9 +48,16 @@ type MCPRegistryReconciler struct {
4748
registryAPIManager registryapi.Manager
4849
}
4950

50-
// NewMCPRegistryReconciler creates a new MCPRegistryReconciler with required dependencies
51-
func NewMCPRegistryReconciler(k8sClient client.Client, scheme *runtime.Scheme) *MCPRegistryReconciler {
52-
registryAPIManager := registryapi.NewManager(k8sClient, scheme)
51+
// NewMCPRegistryReconciler creates a new MCPRegistryReconciler with required
52+
// dependencies. imagePullSecretsDefaults are cluster-wide pull-secret defaults
53+
// from the operator chart that are merged with the per-CR list at registry-api
54+
// workload-construction time.
55+
func NewMCPRegistryReconciler(
56+
k8sClient client.Client,
57+
scheme *runtime.Scheme,
58+
imagePullSecretsDefaults imagepullsecrets.Defaults,
59+
) *MCPRegistryReconciler {
60+
registryAPIManager := registryapi.NewManager(k8sClient, scheme, imagePullSecretsDefaults)
5361
return &MCPRegistryReconciler{
5462
Client: k8sClient,
5563
Scheme: scheme,

‎cmd/thv-operator/controllers/mcpremoteproxy_controller.go‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030

3131
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
3232
ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
33+
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/imagepullsecrets"
3334
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/kubernetes/rbac"
3435
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum"
3536
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation"
@@ -41,6 +42,10 @@ type MCPRemoteProxyReconciler struct {
4142
Scheme *runtime.Scheme
4243
Recorder events.EventRecorder
4344
PlatformDetector *ctrlutil.SharedPlatformDetector
45+
// ImagePullSecretsDefaults are cluster-wide defaults sourced from the
46+
// operator chart that are merged with the per-CR imagePullSecrets when
47+
// constructing workloads. The zero value is a usable empty Defaults.
48+
ImagePullSecretsDefaults imagepullsecrets.Defaults
4449
}
4550

4651
// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpremoteproxies,verbs=get;list;watch;create;update;patch;delete
@@ -1092,18 +1097,25 @@ func (r *MCPRemoteProxyReconciler) ensureRBACResources(ctx context.Context, prox
10921097
Namespace: proxy.Namespace,
10931098
Rules: remoteProxyRBACRules,
10941099
Owner: proxy,
1095-
ImagePullSecrets: imagePullSecretsForRemoteProxy(proxy),
1100+
ImagePullSecrets: r.imagePullSecretsForRemoteProxy(proxy),
10961101
})
10971102
return err
10981103
}
10991104

1100-
// imagePullSecretsForRemoteProxy returns the image pull secrets configured via
1101-
// spec.resourceOverrides.proxyDeployment.imagePullSecrets, or nil if unset.
1102-
func imagePullSecretsForRemoteProxy(proxy *mcpv1beta1.MCPRemoteProxy) []corev1.LocalObjectReference {
1103-
if proxy.Spec.ResourceOverrides == nil || proxy.Spec.ResourceOverrides.ProxyDeployment == nil {
1104-
return nil
1105+
// imagePullSecretsForRemoteProxy returns the image pull secrets the operator
1106+
// will set on the workload's PodSpec and ServiceAccount. The list is the merge
1107+
// of cluster-wide chart defaults (from r.ImagePullSecretsDefaults) with the
1108+
// per-CR list from spec.resourceOverrides.proxyDeployment.imagePullSecrets.
1109+
// CR-level entries win on name collisions; chart-level entries are appended
1110+
// additively. Returns nil when both inputs are empty.
1111+
func (r *MCPRemoteProxyReconciler) imagePullSecretsForRemoteProxy(
1112+
proxy *mcpv1beta1.MCPRemoteProxy,
1113+
) []corev1.LocalObjectReference {
1114+
var crLevel []corev1.LocalObjectReference
1115+
if proxy.Spec.ResourceOverrides != nil && proxy.Spec.ResourceOverrides.ProxyDeployment != nil {
1116+
crLevel = proxy.Spec.ResourceOverrides.ProxyDeployment.ImagePullSecrets
11051117
}
1106-
return proxy.Spec.ResourceOverrides.ProxyDeployment.ImagePullSecrets
1118+
return r.ImagePullSecretsDefaults.Merge(crLevel)
11071119
}
11081120

11091121
// updateMCPRemoteProxyStatus updates the status of the MCPRemoteProxy
@@ -1390,14 +1402,15 @@ func (r *MCPRemoteProxyReconciler) podTemplateMetadataNeedsUpdate(
13901402

13911403
// podSpecNeedsUpdate checks if pod-level fields (not container fields) have drifted.
13921404
//
1393-
// Currently compares ImagePullSecrets sourced from spec.resourceOverrides.proxyDeployment.
1394-
// Uses equality.Semantic.DeepEqual so nil and empty slices are treated as equal,
1405+
// Currently compares ImagePullSecrets — the merge of cluster-wide chart
1406+
// defaults with spec.resourceOverrides.proxyDeployment.imagePullSecrets. Uses
1407+
// equality.Semantic.DeepEqual so nil and empty slices are treated as equal,
13951408
// which matches Kubernetes' own serialization semantics.
1396-
func (*MCPRemoteProxyReconciler) podSpecNeedsUpdate(
1409+
func (r *MCPRemoteProxyReconciler) podSpecNeedsUpdate(
13971410
deployment *appsv1.Deployment,
13981411
proxy *mcpv1beta1.MCPRemoteProxy,
13991412
) bool {
1400-
expected := imagePullSecretsForRemoteProxy(proxy)
1413+
expected := r.imagePullSecretsForRemoteProxy(proxy)
14011414
current := deployment.Spec.Template.Spec.ImagePullSecrets
14021415
return !equality.Semantic.DeepEqual(current, expected)
14031416
}

‎cmd/thv-operator/controllers/mcpremoteproxy_default_imagepullsecrets_test.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package controllers
5+
6+
import (
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
corev1 "k8s.io/api/core/v1"
12+
rbacv1 "k8s.io/api/rbac/v1"
13+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
14+
"k8s.io/apimachinery/pkg/types"
15+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
16+
17+
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
18+
ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
19+
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/imagepullsecrets"
20+
)
21+
22+
// TestMCPRemoteProxy_DefaultImagePullSecrets verifies that the merge of
23+
// cluster-wide chart defaults with spec.resourceOverrides.proxyDeployment.imagePullSecrets
24+
// reaches both the proxy-runner ServiceAccount and the Deployment PodSpec.
25+
//
26+
// The Merge precedence rule is exhaustively covered in
27+
// imagepullsecrets/defaults_test.go::TestDefaultsMerge; the cases here exist
28+
// only to prove the wiring is correct end-to-end.
29+
func TestMCPRemoteProxy_DefaultImagePullSecrets(t *testing.T) {
30+
t.Parallel()
31+
32+
tests := []struct {
33+
name string
34+
defaults []string
35+
crSecrets []corev1.LocalObjectReference
36+
wantSecrets []corev1.LocalObjectReference
37+
}{
38+
{
39+
name: "merged defaults+CR with name collision reach SA and Deployment",
40+
defaults: []string{"shared", "chart-only"},
41+
crSecrets: []corev1.LocalObjectReference{
42+
{Name: "shared"},
43+
},
44+
wantSecrets: []corev1.LocalObjectReference{
45+
{Name: "shared"},
46+
{Name: "chart-only"},
47+
},
48+
},
49+
{
50+
name: "no defaults and no CR yields empty fields",
51+
defaults: nil,
52+
crSecrets: nil,
53+
wantSecrets: nil,
54+
},
55+
}
56+
57+
for _, tt := range tests {
58+
t.Run(tt.name, func(t *testing.T) {
59+
t.Parallel()
60+
61+
proxy := &mcpv1beta1.MCPRemoteProxy{
62+
ObjectMeta: metav1.ObjectMeta{
63+
Name: "default-pullsecrets-proxy",
64+
Namespace: "default",
65+
},
66+
Spec: mcpv1beta1.MCPRemoteProxySpec{
67+
RemoteURL: "https://mcp.example.com",
68+
ProxyPort: 8080,
69+
},
70+
}
71+
if tt.crSecrets != nil {
72+
proxy.Spec.ResourceOverrides = &mcpv1beta1.ResourceOverrides{
73+
ProxyDeployment: &mcpv1beta1.ProxyDeploymentOverrides{
74+
ImagePullSecrets: tt.crSecrets,
75+
},
76+
}
77+
}
78+
79+
scheme := createRunConfigTestScheme()
80+
_ = rbacv1.AddToScheme(scheme)
81+
82+
fakeClient := fake.NewClientBuilder().
83+
WithScheme(scheme).
84+
WithRuntimeObjects(proxy).
85+
Build()
86+
87+
reconciler := &MCPRemoteProxyReconciler{
88+
Client: fakeClient,
89+
Scheme: scheme,
90+
PlatformDetector: ctrlutil.NewSharedPlatformDetector(),
91+
ImagePullSecretsDefaults: imagepullsecrets.NewDefaults(tt.defaults),
92+
}
93+
94+
require.NoError(t, reconciler.ensureRBACResources(t.Context(), proxy))
95+
96+
sa := &corev1.ServiceAccount{}
97+
require.NoError(t, fakeClient.Get(t.Context(), types.NamespacedName{
98+
Name: proxyRunnerServiceAccountNameForRemoteProxy(proxy.Name),
99+
Namespace: proxy.Namespace,
100+
}, sa))
101+
assert.Equal(t, tt.wantSecrets, sa.ImagePullSecrets,
102+
"proxy runner SA ImagePullSecrets must reflect merged defaults+CR")
103+
104+
dep := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum")
105+
require.NotNil(t, dep)
106+
assert.Equal(t, tt.wantSecrets, dep.Spec.Template.Spec.ImagePullSecrets,
107+
"proxy runner Deployment ImagePullSecrets must reflect merged defaults+CR")
108+
})
109+
}
110+
}

‎cmd/thv-operator/controllers/mcpremoteproxy_deployment.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy(
7272
},
7373
Spec: corev1.PodSpec{
7474
ServiceAccountName: serviceAccountNameForRemoteProxy(proxy),
75-
ImagePullSecrets: imagePullSecretsForRemoteProxy(proxy),
75+
ImagePullSecrets: r.imagePullSecretsForRemoteProxy(proxy),
7676
Containers: []corev1.Container{{
7777
Image: getToolhiveRunnerImage(),
7878
Name: "toolhive",

0 commit comments

Comments
 (0)