Preserve external annotations on operator Services (#5731)

A Service co-owned by an external controller (e.g. GKE NEG/Gateway,
which writes `cloud.google.com/*` annotations) made `serviceNeedsUpdate`
fire on every reconcile and the update wholesale-replace those
annotations, hot-looping `Update` against the concurrent writer with
`resourceVersion` conflicts.

* Compare operator-owned labels/annotations with `MapIsSubset` instead
  of `maps.Equal`, so externally-managed keys are not treated as drift
* Merge (not replace) labels/annotations on write via `MergeLabels`/
  `MergeAnnotations`, preserving external keys
* Apply the fix to the VirtualMCPServer, MCPServer, MCPRemoteProxy, and
  EmbeddingServer Service reconcilers
* Add detection and write-path regression tests for all four

Fixes #5730

Author: blkt

cmd/thv-operator/controllers/embeddingserver_controller.go

@@ -287,8 +287,11 @@ func (r *EmbeddingServerReconciler) ensureService(
 	if r.serviceNeedsUpdate(service, embedding) {
 		desiredService := r.serviceForEmbedding(ctx, embedding)
 		service.Spec.Ports = desiredService.Spec.Ports
-		service.Labels = desiredService.Labels
-		service.Annotations = desiredService.Annotations
+		// Merge (not replace) Labels/Annotations so keys written by external controllers
+		// (e.g. GKE NEG's cloud.google.com/* annotations) are preserved while the
+		// operator-owned values are applied.
+		service.Labels = ctrlutil.MergeLabels(desiredService.Labels, service.Labels)
+		service.Annotations = ctrlutil.MergeAnnotations(desiredService.Annotations, service.Annotations)
 		// Preserve ClusterIP as it's immutable
 		if err := r.Update(ctx, service); err != nil {
 			ctxLogger.Error(err, "Failed to update Service",
@@ -330,18 +333,16 @@ func (*EmbeddingServerReconciler) serviceNeedsUpdate(
 		}
 	}
 
-	// Check if expected annotations are present in service
-	for key, value := range expectedAnnotations {
-		if service.Annotations[key] != value {
-			return true
-		}
+	// Subset check rather than exact equality: the Service is co-owned by external
+	// controllers (e.g. GKE NEG/Gateway writes cloud.google.com/* annotations), so only
+	// the operator-owned keys must match. Exact equality would treat those external
+	// annotations as drift and hot-loop Update against the concurrent writer.
+	if !ctrlutil.MapIsSubset(expectedAnnotations, service.Annotations) {
+		return true
 	}
 
-	// Check if expected labels are present in service
-	for key, value := range expectedLabels {
-		if service.Labels[key] != value {
-			return true
-		}
+	if !ctrlutil.MapIsSubset(expectedLabels, service.Labels) {
+		return true
 	}
 
 	return false

cmd/thv-operator/controllers/embeddingserver_controller_test.go

@@ -1161,3 +1161,79 @@ func TestEmbeddingServer_PodTemplateSpec_EmptyObjectIsNoOp(t *testing.T) {
 	assert.Contains(t, c.Args, "--model-id")
 	assert.Contains(t, c.Args, "test-model")
 }
+
+// TestEmbeddingServerServiceNeedsUpdate_ExternalAnnotationsIgnored is a regression test
+// for #5730: external controllers (e.g. GKE NEG/Gateway) write cloud.google.com/*
+// annotations on the Service; these are not operator-owned and must not be treated as
+// drift. A genuine operator-owned change (port) must still be detected.
+func TestEmbeddingServerServiceNeedsUpdate_ExternalAnnotationsIgnored(t *testing.T) {
+	t.Parallel()
+
+	embedding := v1beta1test.NewEmbeddingServer("embed", testNamespaceDefault,
+		v1beta1test.WithEmbeddingPort(9000))
+	r := &EmbeddingServerReconciler{}
+
+	svc := &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				"cloud.google.com/neg-status": `{"network_endpoint_groups":{"9000":"k8s1-abc"}}`,
+			},
+		},
+		Spec: corev1.ServiceSpec{
+			Ports: []corev1.ServicePort{{Name: "http", Port: 9000}},
+		},
+	}
+	assert.False(t, r.serviceNeedsUpdate(svc, embedding),
+		"external annotation must not be treated as drift")
+
+	svc.Spec.Ports[0].Port = 1234
+	assert.True(t, r.serviceNeedsUpdate(svc, embedding),
+		"operator-owned port drift must still be detected")
+}
+
+// TestEmbeddingServerEnsureService_PreservesExternalAnnotations is a regression test for
+// #5730: when a genuine operator-owned change triggers a Service update, annotations
+// written by an external controller (e.g. GKE NEG) must be merged, not stripped.
+func TestEmbeddingServerEnsureService_PreservesExternalAnnotations(t *testing.T) {
+	t.Parallel()
+
+	embedding := v1beta1test.NewEmbeddingServer("embed-ext", testNamespaceDefault,
+		v1beta1test.WithEmbeddingPort(9000))
+
+	// Existing Service co-owned by GKE (external annotation) with a drifted operator-owned
+	// field (wrong port) so serviceNeedsUpdate fires.
+	existing := &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      embedding.Name,
+			Namespace: embedding.Namespace,
+			Annotations: map[string]string{
+				"cloud.google.com/neg-status": `{"network_endpoint_groups":{"9000":"k8s1-abc"}}`,
+			},
+		},
+		Spec: corev1.ServiceSpec{
+			Ports: []corev1.ServicePort{{Name: "http", Port: 1234}},
+		},
+	}
+
+	scheme := testutil.NewScheme(t)
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(embedding, existing).
+		Build()
+	r := &EmbeddingServerReconciler{Client: fakeClient, Scheme: scheme}
+
+	_, err := r.ensureService(context.TODO(), embedding)
+	require.NoError(t, err)
+
+	updated := &corev1.Service{}
+	require.NoError(t, fakeClient.Get(context.TODO(), types.NamespacedName{
+		Name:      embedding.Name,
+		Namespace: embedding.Namespace,
+	}, updated))
+	// External annotation preserved...
+	assert.Equal(t, `{"network_endpoint_groups":{"9000":"k8s1-abc"}}`,
+		updated.Annotations["cloud.google.com/neg-status"])
+	// ...and the operator-owned port corrected.
+	require.Len(t, updated.Spec.Ports, 1)
+	assert.Equal(t, int32(9000), updated.Spec.Ports[0].Port)
+}

cmd/thv-operator/controllers/mcpremoteproxy_controller.go

@@ -376,8 +376,11 @@ func (r *MCPRemoteProxyReconciler) ensureService(
 		}
 		service.Spec.Ports = newService.Spec.Ports
 		service.Spec.SessionAffinity = newService.Spec.SessionAffinity
-		service.Labels = newService.Labels
-		service.Annotations = newService.Annotations
+		// Merge (not replace) Labels/Annotations so keys written by external controllers
+		// (e.g. GKE NEG's cloud.google.com/* annotations) are preserved while the
+		// operator-owned values are applied.
+		service.Labels = ctrlutil.MergeLabels(newService.Labels, service.Labels)
+		service.Annotations = ctrlutil.MergeAnnotations(newService.Annotations, service.Annotations)
 
 		ctxLogger.Info("Updating Service", "Service.Namespace", service.Namespace, "Service.Name", service.Name)
 		if err := r.Update(ctx, service); err != nil {
@@ -1642,11 +1645,15 @@ func (*MCPRemoteProxyReconciler) serviceNeedsUpdate(service *corev1.Service, pro
 		}
 	}
 
-	if !maps.Equal(service.Labels, expectedLabels) {
+	// Subset check rather than exact equality: the Service is co-owned by external
+	// controllers (e.g. GKE NEG/Gateway writes cloud.google.com/* annotations), so only
+	// the operator-owned keys must match. maps.Equal would treat those external
+	// annotations as drift and hot-loop Update against the concurrent writer.
+	if !ctrlutil.MapIsSubset(expectedLabels, service.Labels) {
 		return true
 	}
 
-	if !maps.Equal(service.Annotations, expectedAnnotations) {
+	if !ctrlutil.MapIsSubset(expectedAnnotations, service.Annotations) {
 		return true
 	}
 

cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go

@@ -26,6 +26,7 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
@@ -605,6 +606,55 @@ func TestEnsureService(t *testing.T) {
 	}
 }
 
+// TestMCPRemoteProxyEnsureService_PreservesExternalAnnotations is a regression test for
+// #5730: when a genuine operator-owned change triggers a Service update, annotations
+// written by an external controller (e.g. GKE NEG) must be merged, not stripped.
+func TestMCPRemoteProxyEnsureService_PreservesExternalAnnotations(t *testing.T) {
+	t.Parallel()
+
+	proxy := v1beta1test.NewMCPRemoteProxy("ext-annot-proxy", "default")
+
+	// Existing Service co-owned by GKE (external annotation) with an operator-owned field
+	// drifted (empty session affinity) so serviceNeedsUpdate fires.
+	existing := &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      createProxyServiceName(proxy.Name),
+			Namespace: proxy.Namespace,
+			Labels:    labelsForMCPRemoteProxy(proxy.Name),
+			Annotations: map[string]string{
+				"cloud.google.com/neg-status": `{"network_endpoint_groups":{"8080":"k8s1-abc"}}`,
+			},
+		},
+		Spec: corev1.ServiceSpec{
+			SessionAffinity: "",
+			Ports:           []corev1.ServicePort{{Port: 8080}},
+		},
+	}
+
+	scheme := testutil.NewScheme(t)
+	_ = rbacv1.AddToScheme(scheme)
+	_ = appsv1.AddToScheme(scheme)
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(proxy, existing).
+		Build()
+	reconciler := &MCPRemoteProxyReconciler{Client: fakeClient, Scheme: scheme}
+
+	_, err := reconciler.ensureService(context.TODO(), proxy)
+	require.NoError(t, err)
+
+	updated := &corev1.Service{}
+	require.NoError(t, fakeClient.Get(context.TODO(), types.NamespacedName{
+		Name:      createProxyServiceName(proxy.Name),
+		Namespace: proxy.Namespace,
+	}, updated))
+	// External annotation preserved...
+	assert.Equal(t, `{"network_endpoint_groups":{"8080":"k8s1-abc"}}`,
+		updated.Annotations["cloud.google.com/neg-status"])
+	// ...and the operator-owned field corrected.
+	assert.Equal(t, corev1.ServiceAffinityClientIP, updated.Spec.SessionAffinity)
+}
+
 func TestMCPRemoteProxyDeploymentNeedsUpdate_EmbeddedAuthLegacyEnvStable(t *testing.T) {
 	t.Parallel()
 
@@ -1182,6 +1232,19 @@ func TestMCPRemoteProxyServiceNeedsUpdate(t *testing.T) {
 			}(),
 			needsUpdate: false,
 		},
+		{
+			// Regression for #5730: external controllers (e.g. GKE NEG/Gateway) write
+			// cloud.google.com/* annotations on the Service. These are not operator-owned
+			// and must not be treated as drift, or the operator hot-loops Update.
+			name: "external cloud annotations ignored",
+			service: func() *corev1.Service {
+				s := baseService.DeepCopy()
+				s.Annotations["cloud.google.com/neg-status"] = `{"network_endpoint_groups":{"8080":"k8s1-abc"}}`
+				return s
+			}(),
+			proxy:       baseProxy.DeepCopy(),
+			needsUpdate: false,
+		},
 	}
 
 	for _, tt := range tests {

cmd/thv-operator/controllers/mcpserver_controller.go

@@ -526,8 +526,11 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		newService := r.serviceForMCPServer(ctx, mcpServer)
 		service.Spec.Ports = newService.Spec.Ports
 		service.Spec.SessionAffinity = newService.Spec.SessionAffinity
-		service.Labels = newService.Labels
-		service.Annotations = newService.Annotations
+		// Merge (not replace) Labels/Annotations so keys written by external controllers
+		// (e.g. GKE NEG's cloud.google.com/* annotations) are preserved while the
+		// operator-owned values are applied.
+		service.Labels = ctrlutil.MergeLabels(newService.Labels, service.Labels)
+		service.Annotations = ctrlutil.MergeAnnotations(newService.Annotations, service.Annotations)
 		err = r.Update(ctx, service)
 		if err != nil {
 			ctxLogger.Error(err, "Failed to update Service", "Service.Namespace", service.Namespace, "Service.Name", service.Name)
@@ -2062,11 +2065,15 @@ func serviceNeedsUpdate(service *corev1.Service, mcpServer *mcpv1beta1.MCPServer
 		}
 	}
 
-	if !maps.Equal(service.Labels, expectedLabels) {
+	// Subset check rather than exact equality: the Service is co-owned by external
+	// controllers (e.g. GKE NEG/Gateway writes cloud.google.com/* annotations), so only
+	// the operator-owned keys must match. maps.Equal would treat those external
+	// annotations as drift and hot-loop Update against the concurrent writer.
+	if !ctrlutil.MapIsSubset(expectedLabels, service.Labels) {
 		return true
 	}
 
-	if !maps.Equal(service.Annotations, expectedAnnotations) {
+	if !ctrlutil.MapIsSubset(expectedAnnotations, service.Annotations) {
 		return true
 	}
 

cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go

@@ -1135,6 +1135,19 @@ func TestMCPServerServiceNeedsUpdate(t *testing.T) {
 			}(),
 			needsUpdate: false,
 		},
+		{
+			// Regression for #5730: external controllers (e.g. GKE NEG/Gateway) write
+			// cloud.google.com/* annotations on the Service. These are not operator-owned
+			// and must not be treated as drift, or the operator hot-loops Update.
+			name: "external cloud annotations ignored",
+			service: func() *corev1.Service {
+				s := baseService.DeepCopy()
+				s.Annotations["cloud.google.com/neg-status"] = `{"network_endpoint_groups":{"8080":"k8s1-abc"}}`
+				return s
+			}(),
+			mcpServer:   baseMCPServer.DeepCopy(),
+			needsUpdate: false,
+		},
 	}
 
 	for _, tt := range tests {

cmd/thv-operator/controllers/mcpserver_service_annotations_test.go

@@ -0,0 +1,80 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package controllers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
+	"github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1/v1beta1test"
+	"github.com/stacklok/toolhive/cmd/thv-operator/internal/testutil"
+	ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
+	"github.com/stacklok/toolhive/pkg/container/kubernetes"
+)
+
+// TestMCPServerReconcile_PreservesExternalServiceAnnotations is a regression test for
+// #5730: when a genuine operator-owned change triggers the inline Service update in
+// Reconcile, annotations written by an external controller (e.g. GKE NEG) must be
+// merged, not stripped, otherwise the operator hot-loops Update against the concurrent
+// writer.
+func TestMCPServerReconcile_PreservesExternalServiceAnnotations(t *testing.T) {
+	t.Parallel()
+
+	const name = "ext-annot-server"
+	namespace := testNamespaceDefault
+
+	mcpServer := v1beta1test.NewMCPServer(name, namespace, v1beta1test.WithTransport("sse"))
+
+	scheme := testutil.NewScheme(t)
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(mcpServer).
+		WithStatusSubresource(&mcpv1beta1.MCPServer{}).
+		Build()
+	r := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes)
+
+	req := ctrl.Request{NamespacedName: types.NamespacedName{Name: name, Namespace: namespace}}
+	// Drive to steady state; successive passes create the Deployment and Service.
+	for range 6 {
+		_, err := r.Reconcile(t.Context(), req)
+		require.NoError(t, err)
+	}
+
+	svcName := ctrlutil.CreateProxyServiceName(name)
+	svcKey := types.NamespacedName{Name: svcName, Namespace: namespace}
+
+	// An external controller (GKE NEG) writes an annotation, and an operator-owned field
+	// drifts (empty session affinity) so serviceNeedsUpdate fires on the next reconcile.
+	svc := &corev1.Service{}
+	require.NoError(t, fakeClient.Get(t.Context(), svcKey, svc))
+	if svc.Annotations == nil {
+		svc.Annotations = map[string]string{}
+	}
+	svc.Annotations["cloud.google.com/neg-status"] = `{"network_endpoint_groups":{"8080":"k8s1-abc"}}`
+	svc.Spec.SessionAffinity = ""
+	require.NoError(t, fakeClient.Update(t.Context(), svc))
+
+	// Reconcile again; the inline Service update path merges rather than replaces.
+	for range 3 {
+		_, err := r.Reconcile(t.Context(), req)
+		require.NoError(t, err)
+	}
+
+	updated := &corev1.Service{}
+	require.NoError(t, fakeClient.Get(t.Context(), svcKey, updated))
+	// External annotation preserved...
+	assert.Equal(t, `{"network_endpoint_groups":{"8080":"k8s1-abc"}}`,
+		updated.Annotations["cloud.google.com/neg-status"],
+		"external annotation must survive the operator Service update")
+	// ...and the operator-owned field corrected.
+	assert.Equal(t, corev1.ServiceAffinityClientIP, updated.Spec.SessionAffinity,
+		"operator-owned session affinity should be reconciled back to the default")
+}