Skip to content

Commit 4c23ade

Browse files
jhrozekclaude
jhrozek
and
claude
authored
Detect imagePullSecrets drift on proxy Deployments (#5113)
The MCPServer and MCPRemoteProxy controllers correctly include spec.resourceOverrides.proxyDeployment.imagePullSecrets in newly built proxy Deployments, but their deploymentNeedsUpdate drift checks never compare Spec.Template.Spec.ImagePullSecrets. As a result, editing the field on a live CR did not roll the existing Deployment: the proxy- runner ServiceAccount was refreshed via EnsureRBACResources (so kubelet recovered for new pods through the SA path), but the pod-spec field on the live Deployment remained stale. In the user-managed- ServiceAccount branch, where RBAC reconciliation is skipped, the pod- spec field is the only mechanism — so that path stayed broken. Add a comparison in both controllers using equality.Semantic.DeepEqual so nil and empty slices are treated identically (matching Kubernetes' own serialization semantics) and reorder/add/remove on populated slices triggers a rollout. MCPRemoteProxy gets a new podSpecNeedsUpdate sub-helper alongside the existing containerNeedsUpdate / deploymentMetadataNeedsUpdate / podTemplateMetadataNeedsUpdate trio; MCPServer gets an inline check that mirrors the surrounding style of its single deploymentNeedsUpdate function. Each controller gets a table-driven unit test covering seven cases: the no-override baseline, the bug repro, the removal flow, the populated match, the nil-vs-empty asymmetry from both sides, and the slice-reorder case (which equality.Semantic.DeepEqual treats as drift). Two envtest integration tests per controller exercise the full reconcile-update path: create CR, assert Deployment, mutate imagePullSecrets, assert Deployment rolls. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 09d8047 commit 4c23ade

6 files changed

Lines changed: 522 additions & 0 deletions

File tree

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515

1616
appsv1 "k8s.io/api/apps/v1"
1717
corev1 "k8s.io/api/core/v1"
18+
"k8s.io/apimachinery/pkg/api/equality"
1819
"k8s.io/apimachinery/pkg/api/errors"
1920
"k8s.io/apimachinery/pkg/api/meta"
2021
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1254,6 +1255,10 @@ func (r *MCPRemoteProxyReconciler) deploymentNeedsUpdate(
12541255
return true
12551256
}
12561257

1258+
if r.podSpecNeedsUpdate(deployment, proxy) {
1259+
return true
1260+
}
1261+
12571262
return false
12581263
}
12591264

@@ -1383,6 +1388,20 @@ func (r *MCPRemoteProxyReconciler) podTemplateMetadataNeedsUpdate(
13831388
return false
13841389
}
13851390

1391+
// podSpecNeedsUpdate checks if pod-level fields (not container fields) have drifted.
1392+
//
1393+
// Currently compares ImagePullSecrets sourced from spec.resourceOverrides.proxyDeployment.
1394+
// Uses equality.Semantic.DeepEqual so nil and empty slices are treated as equal,
1395+
// which matches Kubernetes' own serialization semantics.
1396+
func (*MCPRemoteProxyReconciler) podSpecNeedsUpdate(
1397+
deployment *appsv1.Deployment,
1398+
proxy *mcpv1beta1.MCPRemoteProxy,
1399+
) bool {
1400+
expected := imagePullSecretsForRemoteProxy(proxy)
1401+
current := deployment.Spec.Template.Spec.ImagePullSecrets
1402+
return !equality.Semantic.DeepEqual(current, expected)
1403+
}
1404+
13861405
// serviceNeedsUpdate checks if the service needs to be updated
13871406
func (*MCPRemoteProxyReconciler) serviceNeedsUpdate(service *corev1.Service, proxy *mcpv1beta1.MCPRemoteProxy) bool {
13881407
// Check if port has changed

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -880,6 +880,104 @@ func TestMCPRemoteProxyDeploymentNeedsUpdate_TokenExchangeDoesNotDrift(t *testin
880880
assert.False(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum"))
881881
}
882882

883+
func TestMCPRemoteProxyDeploymentNeedsUpdate_ImagePullSecretsDrift(t *testing.T) {
884+
t.Parallel()
885+
886+
tests := []struct {
887+
name string
888+
specSecrets []corev1.LocalObjectReference // set on proxy.Spec.ResourceOverrides
889+
deploymentSecrets []corev1.LocalObjectReference // overrides deployment after build
890+
expectNeedsUpdate bool
891+
}{
892+
{
893+
name: "both empty - no update",
894+
specSecrets: nil,
895+
deploymentSecrets: nil,
896+
expectNeedsUpdate: false,
897+
},
898+
{
899+
name: "spec has secrets, deployment has nil - needs update",
900+
specSecrets: []corev1.LocalObjectReference{{Name: "regsec"}},
901+
deploymentSecrets: nil,
902+
expectNeedsUpdate: true,
903+
},
904+
{
905+
name: "spec cleared, deployment has stale - needs update",
906+
specSecrets: nil,
907+
deploymentSecrets: []corev1.LocalObjectReference{{Name: "old-regsec"}},
908+
expectNeedsUpdate: true,
909+
},
910+
{
911+
name: "match - no update",
912+
specSecrets: []corev1.LocalObjectReference{{Name: "regsec"}},
913+
deploymentSecrets: []corev1.LocalObjectReference{{Name: "regsec"}},
914+
expectNeedsUpdate: false,
915+
},
916+
{
917+
name: "spec nil vs deployment empty slice - no update",
918+
specSecrets: nil,
919+
deploymentSecrets: []corev1.LocalObjectReference{},
920+
expectNeedsUpdate: false,
921+
},
922+
{
923+
name: "spec empty slice vs deployment empty slice - no update",
924+
specSecrets: []corev1.LocalObjectReference{},
925+
deploymentSecrets: []corev1.LocalObjectReference{},
926+
expectNeedsUpdate: false,
927+
},
928+
{
929+
name: "reorder triggers update",
930+
specSecrets: []corev1.LocalObjectReference{{Name: "a"}, {Name: "b"}},
931+
deploymentSecrets: []corev1.LocalObjectReference{{Name: "b"}, {Name: "a"}},
932+
expectNeedsUpdate: true,
933+
},
934+
}
935+
936+
for _, tt := range tests {
937+
t.Run(tt.name, func(t *testing.T) {
938+
t.Parallel()
939+
940+
scheme := createRunConfigTestScheme()
941+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
942+
943+
proxy := &mcpv1beta1.MCPRemoteProxy{
944+
ObjectMeta: metav1.ObjectMeta{
945+
Name: "test-proxy",
946+
Namespace: "default",
947+
},
948+
Spec: mcpv1beta1.MCPRemoteProxySpec{
949+
RemoteURL: "https://mcp.example.com",
950+
ProxyPort: 8080,
951+
},
952+
}
953+
if tt.specSecrets != nil {
954+
proxy.Spec.ResourceOverrides = &mcpv1beta1.ResourceOverrides{
955+
ProxyDeployment: &mcpv1beta1.ProxyDeploymentOverrides{
956+
ImagePullSecrets: tt.specSecrets,
957+
},
958+
}
959+
}
960+
961+
reconciler := &MCPRemoteProxyReconciler{
962+
Client: fakeClient,
963+
Scheme: scheme,
964+
PlatformDetector: ctrlutil.NewSharedPlatformDetector(),
965+
}
966+
967+
deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum")
968+
require.NotNil(t, deployment)
969+
970+
// Simulate the "stored" state by overwriting ImagePullSecrets only.
971+
// The freshly built deployment is otherwise fully aligned with the proxy spec,
972+
// so any detected drift is caused solely by this field.
973+
deployment.Spec.Template.Spec.ImagePullSecrets = tt.deploymentSecrets
974+
975+
needsUpdate := reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")
976+
assert.Equal(t, tt.expectNeedsUpdate, needsUpdate, "ImagePullSecrets drift detection mismatch")
977+
})
978+
}
979+
}
980+
883981
// TestBuildEnvVarsForProxy tests environment variable building
884982
func TestBuildEnvVarsForProxy(t *testing.T) {
885983
t.Parallel()

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1723,6 +1723,18 @@ func (r *MCPServerReconciler) deploymentNeedsUpdate(
17231723
return true
17241724
}
17251725

1726+
// Check if image pull secrets have changed.
1727+
// Sourced from spec.resourceOverrides.proxyDeployment.imagePullSecrets.
1728+
// Uses equality.Semantic.DeepEqual so nil and empty slices are treated as equal.
1729+
var expectedPullSecrets []corev1.LocalObjectReference
1730+
if mcpServer.Spec.ResourceOverrides != nil &&
1731+
mcpServer.Spec.ResourceOverrides.ProxyDeployment != nil {
1732+
expectedPullSecrets = mcpServer.Spec.ResourceOverrides.ProxyDeployment.ImagePullSecrets
1733+
}
1734+
if !equality.Semantic.DeepEqual(deployment.Spec.Template.Spec.ImagePullSecrets, expectedPullSecrets) {
1735+
return true
1736+
}
1737+
17261738
// Check if the resource requirements have changed
17271739
if !equality.Semantic.DeepEqual(container.Resources, resourceRequirementsForMCPServer(mcpServer)) {
17281740
return true

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

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -824,6 +824,102 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) {
824824
}
825825
}
826826

827+
func TestMCPServerDeploymentNeedsUpdate_ImagePullSecretsDrift(t *testing.T) {
828+
t.Parallel()
829+
830+
tests := []struct {
831+
name string
832+
specSecrets []corev1.LocalObjectReference // set on mcpServer.Spec.ResourceOverrides
833+
deploymentSecrets []corev1.LocalObjectReference // overrides deployment after build
834+
expectNeedsUpdate bool
835+
}{
836+
{
837+
name: "both empty - no update",
838+
specSecrets: nil,
839+
deploymentSecrets: nil,
840+
expectNeedsUpdate: false,
841+
},
842+
{
843+
name: "spec has secrets, deployment has nil - needs update",
844+
specSecrets: []corev1.LocalObjectReference{{Name: "regsec"}},
845+
deploymentSecrets: nil,
846+
expectNeedsUpdate: true,
847+
},
848+
{
849+
name: "spec cleared, deployment has stale - needs update",
850+
specSecrets: nil,
851+
deploymentSecrets: []corev1.LocalObjectReference{{Name: "old-regsec"}},
852+
expectNeedsUpdate: true,
853+
},
854+
{
855+
name: "match - no update",
856+
specSecrets: []corev1.LocalObjectReference{{Name: "regsec"}},
857+
deploymentSecrets: []corev1.LocalObjectReference{{Name: "regsec"}},
858+
expectNeedsUpdate: false,
859+
},
860+
{
861+
name: "spec nil vs deployment empty slice - no update",
862+
specSecrets: nil,
863+
deploymentSecrets: []corev1.LocalObjectReference{},
864+
expectNeedsUpdate: false,
865+
},
866+
{
867+
name: "spec empty slice vs deployment empty slice - no update",
868+
specSecrets: []corev1.LocalObjectReference{},
869+
deploymentSecrets: []corev1.LocalObjectReference{},
870+
expectNeedsUpdate: false,
871+
},
872+
{
873+
name: "reorder triggers update",
874+
specSecrets: []corev1.LocalObjectReference{{Name: "a"}, {Name: "b"}},
875+
deploymentSecrets: []corev1.LocalObjectReference{{Name: "b"}, {Name: "a"}},
876+
expectNeedsUpdate: true,
877+
},
878+
}
879+
880+
for _, tt := range tests {
881+
t.Run(tt.name, func(t *testing.T) {
882+
t.Parallel()
883+
884+
scheme := runtime.NewScheme()
885+
require.NoError(t, mcpv1beta1.AddToScheme(scheme))
886+
887+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
888+
r := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes)
889+
890+
mcpServer := &mcpv1beta1.MCPServer{
891+
ObjectMeta: metav1.ObjectMeta{
892+
Name: "test-server",
893+
Namespace: "default",
894+
},
895+
Spec: mcpv1beta1.MCPServerSpec{
896+
Image: "test-image",
897+
ProxyPort: 8080,
898+
},
899+
}
900+
if tt.specSecrets != nil {
901+
mcpServer.Spec.ResourceOverrides = &mcpv1beta1.ResourceOverrides{
902+
ProxyDeployment: &mcpv1beta1.ProxyDeploymentOverrides{
903+
ImagePullSecrets: tt.specSecrets,
904+
},
905+
}
906+
}
907+
908+
ctx := t.Context()
909+
deployment := r.deploymentForMCPServer(ctx, mcpServer, "test-checksum")
910+
require.NotNil(t, deployment)
911+
912+
// Simulate the "stored" state by overwriting ImagePullSecrets only.
913+
// The freshly built deployment is otherwise fully aligned with the mcpServer spec,
914+
// so any detected drift is caused solely by this field.
915+
deployment.Spec.Template.Spec.ImagePullSecrets = tt.deploymentSecrets
916+
917+
needsUpdate := r.deploymentNeedsUpdate(ctx, deployment, mcpServer, "test-checksum")
918+
assert.Equal(t, tt.expectNeedsUpdate, needsUpdate, "ImagePullSecrets drift detection mismatch")
919+
})
920+
}
921+
}
922+
827923
func TestMCPServerSessionAffinityNone(t *testing.T) {
828924
t.Parallel()
829925

0 commit comments

Comments
 (0)