From d0a48cbe361f2ac8699ffd5b7ac2cc088a089c51 Mon Sep 17 00:00:00 2001 From: carlos-lopez-vecino Date: Thu, 1 Oct 2026 21:49:39 +0200 Subject: [PATCH] chore(operator): drop the groupRef transition shim PAIINFRA-202 Restore upstream v0.23.1 behaviour now that the string-form groupRef is no longer needed: MCPServer.spec.groupRef is the typed {name} object again, VirtualMCPServer.spec.groupRef is required again, and the shim tests are replaced by a groupRef schema envtest. CRDs regenerated with controller-gen v0.17.3. Co-Authored-By: Claude Opus 5.5 --- .../api/v1beta1/mcpserver_types.go | 30 --------------- .../api/v1beta1/mcpserver_types_test.go | 35 ----------------- .../api/v1beta1/virtualmcpserver_types.go | 6 +-- ...pserver_cel_validation_integration_test.go | 38 +++---------------- .../toolhive.stacklok.dev_mcpservers.yaml | 30 +++++++++------ ...olhive.stacklok.dev_virtualmcpservers.yaml | 6 +-- .../toolhive.stacklok.dev_mcpservers.yaml | 30 +++++++++------ ...olhive.stacklok.dev_virtualmcpservers.yaml | 6 +-- 8 files changed, 47 insertions(+), 134 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpserver_types.go b/cmd/thv-operator/api/v1beta1/mcpserver_types.go index 20f5d686f8..ba3556b34a 100644 --- a/cmd/thv-operator/api/v1beta1/mcpserver_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpserver_types.go @@ -4,8 +4,6 @@ package v1beta1 import ( - "encoding/json" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -319,14 +317,7 @@ type MCPServerSpec struct { // GroupRef references the MCPGroup this server belongs to. // The referenced MCPGroup must be in the same namespace. - // Accepts {name: } or, for objects created before v0.20, a plain - // group name string. Both forms decode to the same MCPGroupRef. The schema - // is untyped to admit both forms, so CEL cannot validate it; an invalid - // value decodes to an empty name and is reported by the GroupRefValidated - // condition. // +optional - // +kubebuilder:validation:Schemaless - // +kubebuilder:pruning:PreserveUnknownFields GroupRef *MCPGroupRef `json:"groupRef,omitempty"` // SessionAffinity controls whether the Service routes repeated client connections to the same pod. @@ -750,27 +741,6 @@ type MCPGroupRef struct { Name string `json:"name"` } -// UnmarshalJSON accepts both the object form {"name": ""} and the -// legacy plain string form "" used by MCPServer before v0.20. -// Any other shape decodes to an empty name instead of failing, so a single -// malformed object cannot break list/watch decoding in the operator cache. -func (r *MCPGroupRef) UnmarshalJSON(data []byte) error { - var name string - if err := json.Unmarshal(data, &name); err == nil { - r.Name = name - return nil - } - var obj struct { - Name any `json:"name"` - } - if err := json.Unmarshal(data, &obj); err != nil { - r.Name = "" - return nil - } - r.Name, _ = obj.Name.(string) - return nil -} - // GetName returns the name, or empty string if the receiver is nil. func (r *MCPGroupRef) GetName() string { if r == nil { diff --git a/cmd/thv-operator/api/v1beta1/mcpserver_types_test.go b/cmd/thv-operator/api/v1beta1/mcpserver_types_test.go index e44b613f6d..0e69f33836 100644 --- a/cmd/thv-operator/api/v1beta1/mcpserver_types_test.go +++ b/cmd/thv-operator/api/v1beta1/mcpserver_types_test.go @@ -180,38 +180,3 @@ func TestMCPServerSpecScalingFieldsJSONRoundtrip(t *testing.T) { }) } } - -func TestMCPGroupRefUnmarshalJSON(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - input string - want *MCPGroupRef - }{ - {name: "object form", input: `{"groupRef":{"name":"platform"}}`, want: &MCPGroupRef{Name: "platform"}}, - {name: "legacy string form", input: `{"groupRef":"platform"}`, want: &MCPGroupRef{Name: "platform"}}, - {name: "absent", input: `{}`, want: nil}, - {name: "number decodes to empty name", input: `{"groupRef":42}`, want: &MCPGroupRef{}}, - {name: "array decodes to empty name", input: `{"groupRef":["platform"]}`, want: &MCPGroupRef{}}, - {name: "non-string name decodes to empty name", input: `{"groupRef":{"name":7}}`, want: &MCPGroupRef{}}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - var spec MCPServerSpec - require.NoError(t, json.Unmarshal([]byte(tt.input), &spec)) - assert.Equal(t, tt.want, spec.GroupRef) - }) - } -} - -func TestMCPGroupRefMarshalsObjectForm(t *testing.T) { - t.Parallel() - - data, err := json.Marshal(MCPServerSpec{Image: "img", GroupRef: &MCPGroupRef{Name: "platform"}}) - require.NoError(t, err) - assert.Contains(t, string(data), `"groupRef":{"name":"platform"}`) -} diff --git a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go index 654f156c15..6a9f72b3d9 100644 --- a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go +++ b/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go @@ -63,10 +63,8 @@ type VirtualMCPServerSpec struct { // GroupRef references the MCPGroup that defines backend workloads. // The referenced MCPGroup must exist in the same namespace. - // Optional in the schema so objects created before groupRef was introduced - // still validate; the controller rejects a VirtualMCPServer without it. - // +optional - GroupRef *MCPGroupRef `json:"groupRef,omitempty"` + // +kubebuilder:validation:Required + GroupRef *MCPGroupRef `json:"groupRef"` // Config is the Virtual MCP server configuration. // The audit config from here is also supported, but not required. diff --git a/cmd/thv-operator/test-integration/mcp-server/mcpserver_cel_validation_integration_test.go b/cmd/thv-operator/test-integration/mcp-server/mcpserver_cel_validation_integration_test.go index 72e7f00b4b..95edd40dc8 100644 --- a/cmd/thv-operator/test-integration/mcp-server/mcpserver_cel_validation_integration_test.go +++ b/cmd/thv-operator/test-integration/mcp-server/mcpserver_cel_validation_integration_test.go @@ -122,23 +122,15 @@ var _ = Describe("CEL Validation for AuthzConfigRef", Label("k8s", "cel", "valid }) }) - Context("groupRef transition shim", func() { - createRaw := func(name string, groupRef any) error { + Context("groupRef schema", func() { + It("should reject the legacy string form", func() { obj := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "toolhive.stacklok.dev/v1beta1", "kind": "MCPServer", - "metadata": map[string]any{"name": name, "namespace": "default"}, - "spec": map[string]any{"image": "example/mcp-server:latest", "groupRef": groupRef}, + "metadata": map[string]any{"name": "groupref-string", "namespace": "default"}, + "spec": map[string]any{"image": "example/mcp-server:latest", "groupRef": "platform"}, }} - return k8sClient.Create(ctx, obj) - } - - It("should accept and decode the legacy string form", func() { - Expect(createRaw("groupref-string", "platform")).To(Succeed()) - - server := &mcpv1beta1.MCPServer{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "groupref-string", Namespace: "default"}, server)).To(Succeed()) - Expect(server.Spec.GroupRef.GetName()).To(Equal("platform")) + Expect(k8sClient.Create(ctx, obj)).NotTo(Succeed()) }) It("should accept the object form", func() { @@ -150,26 +142,6 @@ var _ = Describe("CEL Validation for AuthzConfigRef", Label("k8s", "cel", "valid Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "groupref-object", Namespace: "default"}, fetched)).To(Succeed()) Expect(fetched.Spec.GroupRef.GetName()).To(Equal("platform")) }) - - It("should accept the legacy string form through v1alpha1", func() { - obj := &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": "toolhive.stacklok.dev/v1alpha1", - "kind": "MCPServer", - "metadata": map[string]any{"name": "groupref-string-v1alpha1", "namespace": "default"}, - "spec": map[string]any{"image": "example/mcp-server:latest", "groupRef": "platform"}, - }} - Expect(k8sClient.Create(ctx, obj)).To(Succeed()) - }) - - It("should decode an unexpected groupRef shape to an empty name without failing", func() { - Expect(createRaw("groupref-number", int64(42))).To(Succeed()) - - list := &mcpv1beta1.MCPServerList{} - Expect(k8sClient.List(ctx, list)).To(Succeed()) - server := &mcpv1beta1.MCPServer{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "groupref-number", Namespace: "default"}, server)).To(Succeed()) - Expect(server.Spec.GroupRef.GetName()).To(BeEmpty()) - }) }) Context("SecretRef schema validation", func() { diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml index ef975bb047..c0c6888b34 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml @@ -217,12 +217,15 @@ spec: description: |- GroupRef references the MCPGroup this server belongs to. The referenced MCPGroup must be in the same namespace. - Accepts {name: } or, for objects created before v0.20, a plain - group name string. Both forms decode to the same MCPGroupRef. The schema - is untyped to admit both forms, so CEL cannot validate it; an invalid - value decodes to an empty name and is reported by the GroupRefValidated - condition. - x-kubernetes-preserve-unknown-fields: true + properties: + name: + description: Name is the name of the MCPGroup resource in the + same namespace + minLength: 1 + type: string + required: + - name + type: object image: description: Image is the container image for the MCP server type: string @@ -1089,12 +1092,15 @@ spec: description: |- GroupRef references the MCPGroup this server belongs to. The referenced MCPGroup must be in the same namespace. - Accepts {name: } or, for objects created before v0.20, a plain - group name string. Both forms decode to the same MCPGroupRef. The schema - is untyped to admit both forms, so CEL cannot validate it; an invalid - value decodes to an empty name and is reported by the GroupRefValidated - condition. - x-kubernetes-preserve-unknown-fields: true + properties: + name: + description: Name is the name of the MCPGroup resource in the + same namespace + minLength: 1 + type: string + required: + - name + type: object image: description: Image is the container image for the MCP server type: string diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index fa33181d27..3e6235ed3c 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1844,8 +1844,6 @@ spec: description: |- GroupRef references the MCPGroup that defines backend workloads. The referenced MCPGroup must exist in the same namespace. - Optional in the schema so objects created before groupRef was introduced - still validate; the controller rejects a VirtualMCPServer without it. properties: name: description: Name is the name of the MCPGroup resource in the @@ -2179,6 +2177,7 @@ spec: - name type: object required: + - groupRef - incomingAuth type: object status: @@ -4174,8 +4173,6 @@ spec: description: |- GroupRef references the MCPGroup that defines backend workloads. The referenced MCPGroup must exist in the same namespace. - Optional in the schema so objects created before groupRef was introduced - still validate; the controller rejects a VirtualMCPServer without it. properties: name: description: Name is the name of the MCPGroup resource in the @@ -4509,6 +4506,7 @@ spec: - name type: object required: + - groupRef - incomingAuth type: object status: diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml index da66fd8192..a59df583c4 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml @@ -220,12 +220,15 @@ spec: description: |- GroupRef references the MCPGroup this server belongs to. The referenced MCPGroup must be in the same namespace. - Accepts {name: } or, for objects created before v0.20, a plain - group name string. Both forms decode to the same MCPGroupRef. The schema - is untyped to admit both forms, so CEL cannot validate it; an invalid - value decodes to an empty name and is reported by the GroupRefValidated - condition. - x-kubernetes-preserve-unknown-fields: true + properties: + name: + description: Name is the name of the MCPGroup resource in the + same namespace + minLength: 1 + type: string + required: + - name + type: object image: description: Image is the container image for the MCP server type: string @@ -1092,12 +1095,15 @@ spec: description: |- GroupRef references the MCPGroup this server belongs to. The referenced MCPGroup must be in the same namespace. - Accepts {name: } or, for objects created before v0.20, a plain - group name string. Both forms decode to the same MCPGroupRef. The schema - is untyped to admit both forms, so CEL cannot validate it; an invalid - value decodes to an empty name and is reported by the GroupRefValidated - condition. - x-kubernetes-preserve-unknown-fields: true + properties: + name: + description: Name is the name of the MCPGroup resource in the + same namespace + minLength: 1 + type: string + required: + - name + type: object image: description: Image is the container image for the MCP server type: string diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index eb6ed618a0..c8bfcc6019 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1847,8 +1847,6 @@ spec: description: |- GroupRef references the MCPGroup that defines backend workloads. The referenced MCPGroup must exist in the same namespace. - Optional in the schema so objects created before groupRef was introduced - still validate; the controller rejects a VirtualMCPServer without it. properties: name: description: Name is the name of the MCPGroup resource in the @@ -2182,6 +2180,7 @@ spec: - name type: object required: + - groupRef - incomingAuth type: object status: @@ -4177,8 +4176,6 @@ spec: description: |- GroupRef references the MCPGroup that defines backend workloads. The referenced MCPGroup must exist in the same namespace. - Optional in the schema so objects created before groupRef was introduced - still validate; the controller rejects a VirtualMCPServer without it. properties: name: description: Name is the name of the MCPGroup resource in the @@ -4512,6 +4509,7 @@ spec: - name type: object required: + - groupRef - incomingAuth type: object status: