Skip to content

Commit 145a068

Browse files
SanskarzzSanskarzz
committed
Improved coverage and fixed CI lint error
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
1 parent 48d2dca commit 145a068

7 files changed

Lines changed: 214 additions & 44 deletions

‎cmd/thv-operator/api/v1alpha1/types.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ type MCPToolConfigList struct {
341341
//+kubebuilder:printcolumn:name="Workflow",type="string",JSONPath=".spec.name",description="Workflow name"
342342
//+kubebuilder:printcolumn:name="Steps",type="integer",JSONPath=".status.stepCount",description="Number of steps"
343343
//+kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.validationStatus",description="Validation status"
344-
//+kubebuilder:printcolumn:name="Refs",type="integer",JSONPath=".status.refCount",description="Number of referencing VirtualMCPServers"
344+
//+kubebuilder:printcolumn:name="Refs",type="integer",JSONPath=".status.refCount",description="Referencing servers"
345345
//+kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Age"
346346
//+kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"
347347

‎cmd/thv-operator/api/v1beta1/virtualmcpcompositetooldefinition_types.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ const (
112112
//+kubebuilder:printcolumn:name="Workflow",type="string",JSONPath=".spec.name",description="Workflow name"
113113
//+kubebuilder:printcolumn:name="Steps",type="integer",JSONPath=".status.stepCount",description="Number of steps"
114114
//+kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.validationStatus",description="Validation status"
115-
//+kubebuilder:printcolumn:name="Refs",type="integer",JSONPath=".status.refCount",description="Number of referencing VirtualMCPServers"
115+
//+kubebuilder:printcolumn:name="Refs",type="integer",JSONPath=".status.refCount",description="Referencing servers"
116116
//+kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Age"
117117
//+kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"
118118

‎cmd/thv-operator/app/app.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"k8s.io/apimachinery/pkg/runtime"
1919
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
2020
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
21-
2221
// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
2322
// to ensure that exec-entrypoint and run can make use of them.
2423
_ "k8s.io/client-go/plugin/pkg/client/auth"

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

Lines changed: 42 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -89,47 +89,52 @@ func compositeToolDefinitionItemCount(length int) int32 {
8989
return int32(length) //nolint:gosec // Kubernetes object size limits keep CRD list lengths within int32.
9090
}
9191

92-
// SetupWithManager configures reconciliation of definitions and the virtual
93-
// servers whose references determine the Refs column.
94-
func (r *VirtualMCPCompositeToolDefinitionReconciler) SetupWithManager(mgr ctrl.Manager) error {
95-
virtualMCPServerHandler := handler.EnqueueRequestsFromMapFunc(
96-
func(ctx context.Context, obj client.Object) []reconcile.Request {
97-
virtualMCPServer, ok := obj.(*mcpv1beta1.VirtualMCPServer)
98-
if !ok {
99-
return nil
100-
}
92+
// mapVirtualMCPServerToCompositeToolDefinitions enqueues both definitions a server
93+
// currently references and definitions that still contain a stale reference to it.
94+
func (r *VirtualMCPCompositeToolDefinitionReconciler) mapVirtualMCPServerToCompositeToolDefinitions(
95+
ctx context.Context,
96+
obj client.Object,
97+
) []reconcile.Request {
98+
virtualMCPServer, ok := obj.(*mcpv1beta1.VirtualMCPServer)
99+
if !ok {
100+
return nil
101+
}
101102

102-
requests := make([]reconcile.Request, 0, len(virtualMCPServer.Spec.Config.CompositeToolRefs))
103-
seen := make(map[types.NamespacedName]struct{})
104-
for _, ref := range virtualMCPServer.Spec.Config.CompositeToolRefs {
105-
name := types.NamespacedName{Name: ref.Name, Namespace: virtualMCPServer.Namespace}
106-
if _, exists := seen[name]; exists {
107-
continue
108-
}
109-
seen[name] = struct{}{}
103+
requests := make([]reconcile.Request, 0, len(virtualMCPServer.Spec.Config.CompositeToolRefs))
104+
seen := make(map[types.NamespacedName]struct{})
105+
for _, ref := range virtualMCPServer.Spec.Config.CompositeToolRefs {
106+
name := types.NamespacedName{Name: ref.Name, Namespace: virtualMCPServer.Namespace}
107+
if _, exists := seen[name]; exists {
108+
continue
109+
}
110+
seen[name] = struct{}{}
111+
requests = append(requests, reconcile.Request{NamespacedName: name})
112+
}
113+
114+
definitions := &mcpv1beta1.VirtualMCPCompositeToolDefinitionList{}
115+
if err := r.List(ctx, definitions, client.InNamespace(virtualMCPServer.Namespace)); err != nil {
116+
log.FromContext(ctx).Error(err, "Failed to list VirtualMCPCompositeToolDefinitions for VirtualMCPServer watch")
117+
return requests
118+
}
119+
for _, definition := range definitions.Items {
120+
name := types.NamespacedName{Name: definition.Name, Namespace: definition.Namespace}
121+
if _, exists := seen[name]; exists {
122+
continue
123+
}
124+
for _, referencingVirtualServer := range definition.Status.ReferencingVirtualServers {
125+
if referencingVirtualServer == virtualMCPServer.Name {
110126
requests = append(requests, reconcile.Request{NamespacedName: name})
127+
break
111128
}
129+
}
130+
}
131+
return requests
132+
}
112133

113-
definitions := &mcpv1beta1.VirtualMCPCompositeToolDefinitionList{}
114-
if err := r.List(ctx, definitions, client.InNamespace(virtualMCPServer.Namespace)); err != nil {
115-
log.FromContext(ctx).Error(err, "Failed to list VirtualMCPCompositeToolDefinitions for VirtualMCPServer watch")
116-
return requests
117-
}
118-
for _, definition := range definitions.Items {
119-
name := types.NamespacedName{Name: definition.Name, Namespace: definition.Namespace}
120-
if _, exists := seen[name]; exists {
121-
continue
122-
}
123-
for _, referencingVirtualServer := range definition.Status.ReferencingVirtualServers {
124-
if referencingVirtualServer == virtualMCPServer.Name {
125-
requests = append(requests, reconcile.Request{NamespacedName: name})
126-
break
127-
}
128-
}
129-
}
130-
return requests
131-
},
132-
)
134+
// SetupWithManager configures reconciliation of definitions and the virtual
135+
// servers whose references determine the Refs column.
136+
func (r *VirtualMCPCompositeToolDefinitionReconciler) SetupWithManager(mgr ctrl.Manager) error {
137+
virtualMCPServerHandler := handler.EnqueueRequestsFromMapFunc(r.mapVirtualMCPServerToCompositeToolDefinitions)
133138

134139
return ctrl.NewControllerManagedBy(mgr).
135140
For(&mcpv1beta1.VirtualMCPCompositeToolDefinition{}).

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

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,19 @@
44
package controllers
55

66
import (
7+
"context"
8+
"errors"
79
"testing"
810

911
"github.com/stretchr/testify/assert"
1012
"github.com/stretchr/testify/require"
13+
corev1 "k8s.io/api/core/v1"
1114
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1215
"k8s.io/apimachinery/pkg/runtime"
1316
"k8s.io/apimachinery/pkg/types"
17+
"sigs.k8s.io/controller-runtime/pkg/client"
1418
"sigs.k8s.io/controller-runtime/pkg/client/fake"
19+
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
1520
"sigs.k8s.io/controller-runtime/pkg/reconcile"
1621

1722
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
@@ -97,6 +102,167 @@ func TestVirtualMCPCompositeToolDefinitionReconciler_Reconcile(t *testing.T) {
97102
}
98103
}
99104

105+
func TestVirtualMCPCompositeToolDefinitionReconciler_ReconcileNotFound(t *testing.T) {
106+
t.Parallel()
107+
108+
scheme := runtime.NewScheme()
109+
require.NoError(t, mcpv1beta1.AddToScheme(scheme))
110+
111+
reconciler := &VirtualMCPCompositeToolDefinitionReconciler{
112+
Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
113+
}
114+
115+
result, err := reconciler.Reconcile(t.Context(), reconcile.Request{
116+
NamespacedName: types.NamespacedName{Name: "missing", Namespace: "default"},
117+
})
118+
119+
require.NoError(t, err)
120+
assert.Equal(t, reconcile.Result{}, result)
121+
}
122+
123+
func TestVirtualMCPCompositeToolDefinitionReconciler_ReconcileErrors(t *testing.T) {
124+
t.Parallel()
125+
126+
scheme := runtime.NewScheme()
127+
require.NoError(t, mcpv1beta1.AddToScheme(scheme))
128+
129+
definition := &mcpv1beta1.VirtualMCPCompositeToolDefinition{
130+
ObjectMeta: metav1.ObjectMeta{Name: "workflow", Namespace: "default"},
131+
}
132+
getErr := errors.New("simulated get failure")
133+
listErr := errors.New("simulated list failure")
134+
135+
t.Run("returns get failure", func(t *testing.T) {
136+
t.Parallel()
137+
138+
fakeClient := fake.NewClientBuilder().
139+
WithScheme(scheme).
140+
WithInterceptorFuncs(interceptor.Funcs{
141+
Get: func(
142+
_ context.Context,
143+
_ client.WithWatch,
144+
_ client.ObjectKey,
145+
_ client.Object,
146+
_ ...client.GetOption,
147+
) error {
148+
return getErr
149+
},
150+
}).
151+
Build()
152+
reconciler := &VirtualMCPCompositeToolDefinitionReconciler{Client: fakeClient}
153+
154+
_, err := reconciler.Reconcile(t.Context(), reconcile.Request{
155+
NamespacedName: types.NamespacedName{Name: "workflow", Namespace: "default"},
156+
})
157+
require.ErrorIs(t, err, getErr)
158+
})
159+
160+
t.Run("returns reference list failure", func(t *testing.T) {
161+
t.Parallel()
162+
163+
fakeClient := fake.NewClientBuilder().
164+
WithScheme(scheme).
165+
WithObjects(definition.DeepCopy()).
166+
WithInterceptorFuncs(interceptor.Funcs{
167+
List: func(
168+
_ context.Context,
169+
_ client.WithWatch,
170+
list client.ObjectList,
171+
_ ...client.ListOption,
172+
) error {
173+
if _, ok := list.(*mcpv1beta1.VirtualMCPServerList); ok {
174+
return listErr
175+
}
176+
return nil
177+
},
178+
}).
179+
Build()
180+
reconciler := &VirtualMCPCompositeToolDefinitionReconciler{Client: fakeClient}
181+
182+
_, err := reconciler.Reconcile(t.Context(), reconcile.Request{
183+
NamespacedName: types.NamespacedName{Name: definition.Name, Namespace: definition.Namespace},
184+
})
185+
require.ErrorIs(t, err, listErr)
186+
})
187+
}
188+
189+
func TestVirtualMCPCompositeToolDefinitionReconciler_MapVirtualMCPServer(t *testing.T) {
190+
t.Parallel()
191+
192+
scheme := runtime.NewScheme()
193+
require.NoError(t, mcpv1beta1.AddToScheme(scheme))
194+
require.NoError(t, corev1.AddToScheme(scheme))
195+
196+
server := virtualMCPServerWithCompositeToolRef("server", "current")
197+
server.Spec.Config.CompositeToolRefs = append(server.Spec.Config.CompositeToolRefs,
198+
vmcpconfig.CompositeToolRef{Name: "current"})
199+
current := &mcpv1beta1.VirtualMCPCompositeToolDefinition{
200+
ObjectMeta: metav1.ObjectMeta{Name: "current", Namespace: "default"},
201+
Status: mcpv1beta1.VirtualMCPCompositeToolDefinitionStatus{
202+
ReferencingVirtualServers: []string{"server"},
203+
},
204+
}
205+
stale := &mcpv1beta1.VirtualMCPCompositeToolDefinition{
206+
ObjectMeta: metav1.ObjectMeta{Name: "stale", Namespace: "default"},
207+
Status: mcpv1beta1.VirtualMCPCompositeToolDefinitionStatus{
208+
ReferencingVirtualServers: []string{"server"},
209+
},
210+
}
211+
212+
t.Run("returns current and stale definitions without duplicates", func(t *testing.T) {
213+
t.Parallel()
214+
215+
fakeClient := fake.NewClientBuilder().
216+
WithScheme(scheme).
217+
WithObjects(current.DeepCopy(), stale.DeepCopy()).
218+
Build()
219+
reconciler := &VirtualMCPCompositeToolDefinitionReconciler{Client: fakeClient}
220+
221+
assert.ElementsMatch(t, []reconcile.Request{
222+
{NamespacedName: types.NamespacedName{Name: "current", Namespace: "default"}},
223+
{NamespacedName: types.NamespacedName{Name: "stale", Namespace: "default"}},
224+
}, reconciler.mapVirtualMCPServerToCompositeToolDefinitions(t.Context(), server.DeepCopy()))
225+
})
226+
227+
t.Run("returns nil for unrelated object type", func(t *testing.T) {
228+
t.Parallel()
229+
230+
reconciler := &VirtualMCPCompositeToolDefinitionReconciler{
231+
Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
232+
}
233+
pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod", Namespace: "default"}}
234+
235+
assert.Nil(t, reconciler.mapVirtualMCPServerToCompositeToolDefinitions(t.Context(), pod))
236+
})
237+
238+
t.Run("returns current definition if stale lookup fails", func(t *testing.T) {
239+
t.Parallel()
240+
241+
listErr := errors.New("simulated definition list failure")
242+
fakeClient := fake.NewClientBuilder().
243+
WithScheme(scheme).
244+
WithInterceptorFuncs(interceptor.Funcs{
245+
List: func(
246+
_ context.Context,
247+
_ client.WithWatch,
248+
list client.ObjectList,
249+
_ ...client.ListOption,
250+
) error {
251+
if _, ok := list.(*mcpv1beta1.VirtualMCPCompositeToolDefinitionList); ok {
252+
return listErr
253+
}
254+
return nil
255+
},
256+
}).
257+
Build()
258+
reconciler := &VirtualMCPCompositeToolDefinitionReconciler{Client: fakeClient}
259+
260+
assert.Equal(t, []reconcile.Request{{
261+
NamespacedName: types.NamespacedName{Name: "current", Namespace: "default"},
262+
}}, reconciler.mapVirtualMCPServerToCompositeToolDefinitions(t.Context(), server.DeepCopy()))
263+
})
264+
}
265+
100266
func virtualMCPServerWithCompositeToolRef(name, definitionName string) mcpv1beta1.VirtualMCPServer {
101267
return mcpv1beta1.VirtualMCPServer{
102268
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"},

‎deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpcompositetooldefinitions.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ spec:
3232
jsonPath: .status.validationStatus
3333
name: Status
3434
type: string
35-
- description: Number of referencing VirtualMCPServers
35+
- description: Referencing servers
3636
jsonPath: .status.refCount
3737
name: Refs
3838
type: integer
@@ -449,7 +449,7 @@ spec:
449449
jsonPath: .status.validationStatus
450450
name: Status
451451
type: string
452-
- description: Number of referencing VirtualMCPServers
452+
- description: Referencing servers
453453
jsonPath: .status.refCount
454454
name: Refs
455455
type: integer

‎deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpcompositetooldefinitions.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ spec:
3535
jsonPath: .status.validationStatus
3636
name: Status
3737
type: string
38-
- description: Number of referencing VirtualMCPServers
38+
- description: Referencing servers
3939
jsonPath: .status.refCount
4040
name: Refs
4141
type: integer
@@ -452,7 +452,7 @@ spec:
452452
jsonPath: .status.validationStatus
453453
name: Status
454454
type: string
455-
- description: Number of referencing VirtualMCPServers
455+
- description: Referencing servers
456456
jsonPath: .status.refCount
457457
name: Refs
458458
type: integer

0 commit comments

Comments
 (0)