From d59a5e9869ae41ddd8e597e163ea234ba123470d Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Thu, 18 Sep 2025 14:46:11 +0200 Subject: [PATCH 1/7] Add MCPRegistry controller e2e test framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Create Ginkgo-based test suite for operator testing - Add comprehensive test helpers for MCPRegistry operations - Include test fixtures with sample YAML manifests - Set up Kubernetes test environment with envtest support - Add namespace isolation and cleanup utilities 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Daniele Martinoli Co-authored-by: Claude --- test/e2e/operator/doc.go | 3 + test/e2e/operator/fixtures/README.md | 22 ++ .../fixtures/git-credentials-secret.yaml | 9 + .../fixtures/mcpregistry-git-auth.yaml | 19 ++ .../fixtures/mcpregistry-git-basic.yaml | 15 ++ .../fixtures/mcpregistry-manual-sync.yaml | 14 + .../operator/fixtures/test-registry-data.yaml | 33 +++ test/e2e/operator/helpers_test.go | 243 ++++++++++++++++++ test/e2e/operator/suite_test.go | 146 +++++++++++ 9 files changed, 504 insertions(+) create mode 100644 test/e2e/operator/doc.go create mode 100644 test/e2e/operator/fixtures/README.md create mode 100644 test/e2e/operator/fixtures/git-credentials-secret.yaml create mode 100644 test/e2e/operator/fixtures/mcpregistry-git-auth.yaml create mode 100644 test/e2e/operator/fixtures/mcpregistry-git-basic.yaml create mode 100644 test/e2e/operator/fixtures/mcpregistry-manual-sync.yaml create mode 100644 test/e2e/operator/fixtures/test-registry-data.yaml create mode 100644 test/e2e/operator/helpers_test.go create mode 100644 test/e2e/operator/suite_test.go diff --git a/test/e2e/operator/doc.go b/test/e2e/operator/doc.go new file mode 100644 index 000000000..2004d4acf --- /dev/null +++ b/test/e2e/operator/doc.go @@ -0,0 +1,3 @@ +// Package operator_test provides end-to-end tests for the ToolHive operator controllers. +// This package tests MCPRegistry and other operator functionality using Ginkgo and Kubernetes APIs. +package operator_test diff --git a/test/e2e/operator/fixtures/README.md b/test/e2e/operator/fixtures/README.md new file mode 100644 index 000000000..7dc786869 --- /dev/null +++ b/test/e2e/operator/fixtures/README.md @@ -0,0 +1,22 @@ +# Test Fixtures + +This directory contains YAML manifests for testing the MCPRegistry controller. + +## Files + +- **mcpregistry-git-basic.yaml**: Basic MCPRegistry with Git source and automatic sync +- **mcpregistry-git-auth.yaml**: MCPRegistry with Git authentication using secrets +- **mcpregistry-manual-sync.yaml**: MCPRegistry with manual sync only +- **git-credentials-secret.yaml**: Secret containing Git authentication credentials +- **test-registry-data.yaml**: Sample registry data in ConfigMap format + +## Usage + +These fixtures are used by the operator e2e tests to create consistent test scenarios. They can be loaded using the test helpers or applied directly with kubectl for manual testing. + +## Customization + +When using these fixtures in tests: +1. Update the namespace field to match your test namespace +2. Modify resource names to avoid conflicts +3. Adjust Git URLs to point to test repositories as needed \ No newline at end of file diff --git a/test/e2e/operator/fixtures/git-credentials-secret.yaml b/test/e2e/operator/fixtures/git-credentials-secret.yaml new file mode 100644 index 000000000..7285732a0 --- /dev/null +++ b/test/e2e/operator/fixtures/git-credentials-secret.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: Secret +metadata: + name: git-credentials + namespace: test-namespace +type: Opaque +data: + # Base64 encoded "test-token-value" + token: dGVzdC10b2tlbi12YWx1ZQ== \ No newline at end of file diff --git a/test/e2e/operator/fixtures/mcpregistry-git-auth.yaml b/test/e2e/operator/fixtures/mcpregistry-git-auth.yaml new file mode 100644 index 000000000..ea82b7df5 --- /dev/null +++ b/test/e2e/operator/fixtures/mcpregistry-git-auth.yaml @@ -0,0 +1,19 @@ +apiVersion: toolhive.stacklok.dev/v1alpha1 +kind: MCPRegistry +metadata: + name: test-git-auth-registry + namespace: test-namespace +spec: + source: + git: + url: "https://github.com/private/mcp-registry.git" + ref: "main" + auth: + secretRef: + name: git-credentials + key: token + syncPolicy: + interval: "30m" + automatic: true +status: + phase: "Pending" \ No newline at end of file diff --git a/test/e2e/operator/fixtures/mcpregistry-git-basic.yaml b/test/e2e/operator/fixtures/mcpregistry-git-basic.yaml new file mode 100644 index 000000000..3d9150ab7 --- /dev/null +++ b/test/e2e/operator/fixtures/mcpregistry-git-basic.yaml @@ -0,0 +1,15 @@ +apiVersion: toolhive.stacklok.dev/v1alpha1 +kind: MCPRegistry +metadata: + name: test-git-registry + namespace: test-namespace +spec: + source: + git: + url: "https://github.com/modelcontextprotocol/registry.git" + ref: "main" + syncPolicy: + interval: "1h" + automatic: true +status: + phase: "Pending" \ No newline at end of file diff --git a/test/e2e/operator/fixtures/mcpregistry-manual-sync.yaml b/test/e2e/operator/fixtures/mcpregistry-manual-sync.yaml new file mode 100644 index 000000000..f3e50d637 --- /dev/null +++ b/test/e2e/operator/fixtures/mcpregistry-manual-sync.yaml @@ -0,0 +1,14 @@ +apiVersion: toolhive.stacklok.dev/v1alpha1 +kind: MCPRegistry +metadata: + name: test-manual-registry + namespace: test-namespace +spec: + source: + git: + url: "https://github.com/modelcontextprotocol/registry.git" + ref: "main" + syncPolicy: + automatic: false +status: + phase: "Pending" \ No newline at end of file diff --git a/test/e2e/operator/fixtures/test-registry-data.yaml b/test/e2e/operator/fixtures/test-registry-data.yaml new file mode 100644 index 000000000..5dcc2d7b4 --- /dev/null +++ b/test/e2e/operator/fixtures/test-registry-data.yaml @@ -0,0 +1,33 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-registry-data + namespace: test-namespace + labels: + toolhive.stacklok.dev/registry: "test-registry" +data: + registry.json: | + { + "servers": [ + { + "name": "filesystem", + "description": "File system operations for secure file access", + "version": "1.0.0", + "sourceUrl": "https://github.com/modelcontextprotocol/servers/tree/main/src/filesystem", + "transport": { + "type": "stdio" + }, + "tags": ["filesystem", "files"] + }, + { + "name": "fetch", + "description": "Web content fetching with readability processing", + "version": "0.1.0", + "sourceUrl": "https://github.com/modelcontextprotocol/servers/tree/main/src/fetch", + "transport": { + "type": "stdio" + }, + "tags": ["web", "fetch", "readability"] + } + ] + } \ No newline at end of file diff --git a/test/e2e/operator/helpers_test.go b/test/e2e/operator/helpers_test.go new file mode 100644 index 000000000..43dcc78af --- /dev/null +++ b/test/e2e/operator/helpers_test.go @@ -0,0 +1,243 @@ +package operator_test + +import ( + "context" + "fmt" + "time" + + "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// KubernetesTestHelper provides utilities for Kubernetes testing +type KubernetesTestHelper struct { + Client client.Client + Context context.Context + Namespace string +} + +// NewKubernetesTestHelper creates a new test helper for the given namespace +func NewKubernetesTestHelper(namespace string) *KubernetesTestHelper { + return &KubernetesTestHelper{ + Client: k8sClient, + Context: ctx, + Namespace: namespace, + } +} + +// CreateMCPRegistry creates an MCPRegistry with the given spec +func (h *KubernetesTestHelper) CreateMCPRegistry(name string, spec mcpv1alpha1.MCPRegistrySpec) *mcpv1alpha1.MCPRegistry { + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + }, + Spec: spec, + } + + err := h.Client.Create(h.Context, registry) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to create MCPRegistry") + + return registry +} + +// GetMCPRegistry retrieves an MCPRegistry by name +func (h *KubernetesTestHelper) GetMCPRegistry(name string) (*mcpv1alpha1.MCPRegistry, error) { + registry := &mcpv1alpha1.MCPRegistry{} + err := h.Client.Get(h.Context, types.NamespacedName{ + Namespace: h.Namespace, + Name: name, + }, registry) + return registry, err +} + +// UpdateMCPRegistry updates an existing MCPRegistry +func (h *KubernetesTestHelper) UpdateMCPRegistry(registry *mcpv1alpha1.MCPRegistry) error { + return h.Client.Update(h.Context, registry) +} + +// DeleteMCPRegistry deletes an MCPRegistry by name +func (h *KubernetesTestHelper) DeleteMCPRegistry(name string) error { + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + }, + } + return h.Client.Delete(h.Context, registry) +} + +// WaitForMCPRegistryPhase waits for the MCPRegistry to reach the specified phase +func (h *KubernetesTestHelper) WaitForMCPRegistryPhase(name string, phase mcpv1alpha1.MCPRegistryPhase, timeout time.Duration) { + gomega.Eventually(func() mcpv1alpha1.MCPRegistryPhase { + registry, err := h.GetMCPRegistry(name) + if err != nil { + return "" + } + return registry.Status.Phase + }, timeout, time.Second).Should(gomega.Equal(phase), "MCPRegistry should reach phase %s", phase) +} + +// WaitForMCPRegistryCondition waits for a specific condition to be true +func (h *KubernetesTestHelper) WaitForMCPRegistryCondition( + name string, conditionType string, status metav1.ConditionStatus, timeout time.Duration) { + gomega.Eventually(func() metav1.ConditionStatus { + registry, err := h.GetMCPRegistry(name) + if err != nil { + return metav1.ConditionUnknown + } + + for _, condition := range registry.Status.Conditions { + if condition.Type == conditionType { + return condition.Status + } + } + return metav1.ConditionUnknown + }, + timeout, time.Second).Should( + gomega.Equal(status), "MCPRegistry should have condition %s with status %s", conditionType, status) +} + +// WaitForMCPRegistryDeletion waits for the MCPRegistry to be deleted +func (h *KubernetesTestHelper) WaitForMCPRegistryDeletion(name string, timeout time.Duration) { + gomega.Eventually(func() bool { + _, err := h.GetMCPRegistry(name) + return apierrors.IsNotFound(err) + }, timeout, time.Second).Should(gomega.BeTrue(), "MCPRegistry should be deleted") +} + +// CreateConfigMap creates a ConfigMap for testing +func (h *KubernetesTestHelper) CreateConfigMap(name string, data map[string]string) *corev1.ConfigMap { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + }, + Data: data, + } + + err := h.Client.Create(h.Context, cm) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to create ConfigMap") + + return cm +} + +// GetConfigMap retrieves a ConfigMap by name +func (h *KubernetesTestHelper) GetConfigMap(name string) (*corev1.ConfigMap, error) { + cm := &corev1.ConfigMap{} + err := h.Client.Get(h.Context, types.NamespacedName{ + Namespace: h.Namespace, + Name: name, + }, cm) + return cm, err +} + +// WaitForConfigMap waits for a ConfigMap to exist +func (h *KubernetesTestHelper) WaitForConfigMap(name string, timeout time.Duration) *corev1.ConfigMap { + var cm *corev1.ConfigMap + gomega.Eventually(func() error { + var err error + cm, err = h.GetConfigMap(name) + return err + }, timeout, time.Second).Should(gomega.Succeed(), "ConfigMap should be created") + return cm +} + +// WaitForConfigMapData waits for a ConfigMap to contain specific data +func (h *KubernetesTestHelper) WaitForConfigMapData(name, key, expectedValue string, timeout time.Duration) { + gomega.Eventually(func() string { + cm, err := h.GetConfigMap(name) + if err != nil { + return "" + } + return cm.Data[key] + }, timeout, time.Second).Should(gomega.Equal(expectedValue), "ConfigMap should contain expected data") +} + +// CreateSecret creates a Secret for testing +func (h *KubernetesTestHelper) CreateSecret(name string, data map[string][]byte) *corev1.Secret { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + }, + Data: data, + } + + err := h.Client.Create(h.Context, secret) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to create Secret") + + return secret +} + +// CleanupResources removes all test resources in the namespace +func (h *KubernetesTestHelper) CleanupResources() { + // Delete all MCPRegistries + registryList := &mcpv1alpha1.MCPRegistryList{} + err := h.Client.List(h.Context, registryList, client.InNamespace(h.Namespace)) + if err == nil { + for _, registry := range registryList.Items { + _ = h.Client.Delete(h.Context, ®istry) + } + } + + // Delete all ConfigMaps + cmList := &corev1.ConfigMapList{} + err = h.Client.List(h.Context, cmList, client.InNamespace(h.Namespace)) + if err == nil { + for _, cm := range cmList.Items { + _ = h.Client.Delete(h.Context, &cm) + } + } + + // Delete all Secrets + secretList := &corev1.SecretList{} + err = h.Client.List(h.Context, secretList, client.InNamespace(h.Namespace)) + if err == nil { + for _, secret := range secretList.Items { + _ = h.Client.Delete(h.Context, &secret) + } + } +} + +// TriggerManualSync adds an annotation to trigger a manual sync +func (h *KubernetesTestHelper) TriggerManualSync(registryName string) error { + registry, err := h.GetMCPRegistry(registryName) + if err != nil { + return err + } + + if registry.Annotations == nil { + registry.Annotations = make(map[string]string) + } + registry.Annotations["toolhive.stacklok.dev/manual-sync"] = fmt.Sprintf("%d", time.Now().Unix()) + + return h.UpdateMCPRegistry(registry) +} + +// WaitForSyncCompletion waits for a sync operation to complete +func (h *KubernetesTestHelper) WaitForSyncCompletion(registryName string, timeout time.Duration) { + gomega.Eventually(func() bool { + registry, err := h.GetMCPRegistry(registryName) + if err != nil { + return false + } + + // Check if sync is in progress + for _, condition := range registry.Status.Conditions { + if condition.Type == "Syncing" && condition.Status == metav1.ConditionTrue { + return false // Still syncing + } + } + + // Sync should be complete (either success or failure) + return registry.Status.Phase == mcpv1alpha1.MCPRegistryPhaseReady || + registry.Status.Phase == mcpv1alpha1.MCPRegistryPhaseFailed + }, timeout, time.Second).Should(gomega.BeTrue(), "Sync operation should complete") +} diff --git a/test/e2e/operator/suite_test.go b/test/e2e/operator/suite_test.go new file mode 100644 index 000000000..63c786250 --- /dev/null +++ b/test/e2e/operator/suite_test.go @@ -0,0 +1,146 @@ +package operator_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +var ( + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment + ctx context.Context + cancel context.CancelFunc +) + +func TestOperatorE2E(t *testing.T) { //nolint:paralleltest // E2E tests should not run in parallel + RegisterFailHandler(Fail) + RunSpecs(t, "Operator E2E Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + ctx, cancel = context.WithCancel(context.TODO()) + + By("bootstrapping test environment") + + // Check if we should use an existing cluster (for CI/CD) + useExistingCluster := os.Getenv("USE_EXISTING_CLUSTER") == "true" + + testEnv = &envtest.Environment{ + UseExistingCluster: &useExistingCluster, + CRDDirectoryPaths: []string{ + filepath.Join("..", "..", "..", "deploy", "charts", "operator-crds", "crds"), + }, + ErrorIfCRDPathMissing: true, + } + + var err error + cfg, err = testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + // Add MCPRegistry scheme + err = mcpv1alpha1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + + // Create controller-runtime client + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient).NotTo(BeNil()) + + // Verify MCPRegistry CRD is available + By("verifying MCPRegistry CRD is available") + Eventually(func() error { + mcpRegistry := &mcpv1alpha1.MCPRegistry{} + return k8sClient.Get(ctx, client.ObjectKey{ + Namespace: "default", + Name: "test-availability-check", + }, mcpRegistry) + }, time.Minute, time.Second).Should(MatchError(ContainSubstring("not found"))) +}) + +var _ = AfterSuite(func() { + cancel() + By("tearing down the test environment") + err := testEnv.Stop() + Expect(err).NotTo(HaveOccurred()) +}) + +// TestNamespace represents a test namespace with automatic cleanup +type TestNamespace struct { + Name string + Namespace *corev1.Namespace + Client client.Client + ctx context.Context +} + +// NewTestNamespace creates a new test namespace with a unique name +func NewTestNamespace(namePrefix string) *TestNamespace { + timestamp := time.Now().Unix() + name := fmt.Sprintf("%s-%d", namePrefix, timestamp) + + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{ + "test.toolhive.io/suite": "operator-e2e", + "test.toolhive.io/prefix": namePrefix, + }, + }, + } + + return &TestNamespace{ + Name: name, + Namespace: ns, + Client: k8sClient, + ctx: ctx, + } +} + +// Create creates the namespace in the cluster +func (tn *TestNamespace) Create() error { + return tn.Client.Create(tn.ctx, tn.Namespace) +} + +// Delete deletes the namespace and all its resources +func (tn *TestNamespace) Delete() error { + return tn.Client.Delete(tn.ctx, tn.Namespace) +} + +// WaitForDeletion waits for the namespace to be fully deleted +func (tn *TestNamespace) WaitForDeletion(timeout time.Duration) { + Eventually(func() bool { + ns := &corev1.Namespace{} + err := tn.Client.Get(tn.ctx, client.ObjectKey{Name: tn.Name}, ns) + return err != nil + }, timeout, time.Second).Should(BeTrue(), "namespace should be deleted") +} + +// GetClient returns a client scoped to this namespace +func (tn *TestNamespace) GetClient() client.Client { + return tn.Client +} + +// GetContext returns the test context +func (tn *TestNamespace) GetContext() context.Context { + return tn.ctx +} From f4219072eb433fb459c5ec68bef1c6c8ad54cc29 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Thu, 18 Sep 2025 15:18:22 +0200 Subject: [PATCH 2/7] Add comprehensive Ginkgo test framework for MCPRegistry e2e testing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Create test suite with proper Kubernetes environment setup - Add specialized helper utilities for MCPRegistry operations - Implement ConfigMap test helpers for registry data validation - Add status validation helpers for phase and condition checking - Create timing utilities with proper timeout configurations - Add test data factories for generating test resources - Include builder patterns for fluent resource construction - Support both ToolHive and upstream MCP registry formats - Add comprehensive test fixtures and scenarios 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Daniele Martinoli Co-authored-by: Claude --- test/e2e/operator/configmap_helpers.go | 320 ++++++++++++++++++ test/e2e/operator/factories.go | 450 +++++++++++++++++++++++++ test/e2e/operator/registry_helpers.go | 237 +++++++++++++ test/e2e/operator/status_helpers.go | 252 ++++++++++++++ test/e2e/operator/timing_helpers.go | 312 +++++++++++++++++ 5 files changed, 1571 insertions(+) create mode 100644 test/e2e/operator/configmap_helpers.go create mode 100644 test/e2e/operator/factories.go create mode 100644 test/e2e/operator/registry_helpers.go create mode 100644 test/e2e/operator/status_helpers.go create mode 100644 test/e2e/operator/timing_helpers.go diff --git a/test/e2e/operator/configmap_helpers.go b/test/e2e/operator/configmap_helpers.go new file mode 100644 index 000000000..12d9efec7 --- /dev/null +++ b/test/e2e/operator/configmap_helpers.go @@ -0,0 +1,320 @@ +package operator_test + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + // Registry format constants + registryFormatToolHive = "toolhive" + registryFormatUpstream = "upstream" +) + +// ConfigMapTestHelper provides utilities for ConfigMap testing and validation +type ConfigMapTestHelper struct { + Client client.Client + Context context.Context + Namespace string +} + +// NewConfigMapTestHelper creates a new test helper for ConfigMap operations +func NewConfigMapTestHelper(ctx context.Context, k8sClient client.Client, namespace string) *ConfigMapTestHelper { + return &ConfigMapTestHelper{ + Client: k8sClient, + Context: ctx, + Namespace: namespace, + } +} + +// RegistryServer represents a server definition in the registry +type RegistryServer struct { + Name string `json:"name"` + Description string `json:"description,omitempty"` + Version string `json:"version,omitempty"` + SourceURL string `json:"sourceUrl,omitempty"` + Transport map[string]string `json:"transport,omitempty"` + Tags []string `json:"tags,omitempty"` +} + +// ToolHiveRegistryData represents the ToolHive registry format +type ToolHiveRegistryData struct { + Servers []RegistryServer `json:"servers"` +} + +// UpstreamRegistryData represents the upstream MCP registry format +type UpstreamRegistryData struct { + Servers map[string]RegistryServer `json:"servers"` +} + +// ConfigMapBuilder provides a fluent interface for building ConfigMaps +type ConfigMapBuilder struct { + configMap *corev1.ConfigMap +} + +// NewConfigMapBuilder creates a new ConfigMap builder +func (h *ConfigMapTestHelper) NewConfigMapBuilder(name string) *ConfigMapBuilder { + return &ConfigMapBuilder{ + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + Labels: map[string]string{ + "test.toolhive.io/suite": "operator-e2e", + }, + }, + Data: make(map[string]string), + }, + } +} + +// WithLabel adds a label to the ConfigMap +func (cb *ConfigMapBuilder) WithLabel(key, value string) *ConfigMapBuilder { + if cb.configMap.Labels == nil { + cb.configMap.Labels = make(map[string]string) + } + cb.configMap.Labels[key] = value + return cb +} + +// WithData adds arbitrary data to the ConfigMap +func (cb *ConfigMapBuilder) WithData(key, value string) *ConfigMapBuilder { + cb.configMap.Data[key] = value + return cb +} + +// WithToolHiveRegistry adds ToolHive format registry data +func (cb *ConfigMapBuilder) WithToolHiveRegistry(key string, servers []RegistryServer) *ConfigMapBuilder { + registryData := ToolHiveRegistryData{Servers: servers} + jsonData, err := json.MarshalIndent(registryData, "", " ") + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to marshal ToolHive registry data") + cb.configMap.Data[key] = string(jsonData) + return cb +} + +// WithUpstreamRegistry adds upstream MCP format registry data +func (cb *ConfigMapBuilder) WithUpstreamRegistry(key string, servers map[string]RegistryServer) *ConfigMapBuilder { + registryData := UpstreamRegistryData{Servers: servers} + jsonData, err := json.MarshalIndent(registryData, "", " ") + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to marshal upstream registry data") + cb.configMap.Data[key] = string(jsonData) + return cb +} + +// Build returns the constructed ConfigMap +func (cb *ConfigMapBuilder) Build() *corev1.ConfigMap { + return cb.configMap.DeepCopy() +} + +// Create builds and creates the ConfigMap in the cluster +func (cb *ConfigMapBuilder) Create(h *ConfigMapTestHelper) *corev1.ConfigMap { + configMap := cb.Build() + err := h.Client.Create(h.Context, configMap) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to create ConfigMap") + return configMap +} + +// CreateSampleToolHiveRegistry creates a ConfigMap with sample ToolHive registry data +func (h *ConfigMapTestHelper) CreateSampleToolHiveRegistry(name string) *corev1.ConfigMap { + servers := []RegistryServer{ + { + Name: "filesystem", + Description: "File system operations for secure file access", + Version: "1.0.0", + SourceURL: "https://github.com/modelcontextprotocol/servers/tree/main/src/filesystem", + Transport: map[string]string{"type": "stdio"}, + Tags: []string{"filesystem", "files"}, + }, + { + Name: "fetch", + Description: "Web content fetching with readability processing", + Version: "0.1.0", + SourceURL: "https://github.com/modelcontextprotocol/servers/tree/main/src/fetch", + Transport: map[string]string{"type": "stdio"}, + Tags: []string{"web", "fetch", "readability"}, + }, + } + + return h.NewConfigMapBuilder(name). + WithToolHiveRegistry("registry.json", servers). + Create(h) +} + +// CreateSampleUpstreamRegistry creates a ConfigMap with sample upstream registry data +func (h *ConfigMapTestHelper) CreateSampleUpstreamRegistry(name string) *corev1.ConfigMap { + servers := map[string]RegistryServer{ + "filesystem": { + Name: "filesystem", + Description: "File system operations", + Version: "1.0.0", + SourceURL: "https://github.com/modelcontextprotocol/servers/tree/main/src/filesystem", + Transport: map[string]string{"type": "stdio"}, + Tags: []string{"filesystem"}, + }, + } + + return h.NewConfigMapBuilder(name). + WithUpstreamRegistry("registry.json", servers). + Create(h) +} + +// GetConfigMap retrieves a ConfigMap by name +func (h *ConfigMapTestHelper) GetConfigMap(name string) (*corev1.ConfigMap, error) { + cm := &corev1.ConfigMap{} + err := h.Client.Get(h.Context, types.NamespacedName{ + Namespace: h.Namespace, + Name: name, + }, cm) + return cm, err +} + +// UpdateConfigMap updates an existing ConfigMap +func (h *ConfigMapTestHelper) UpdateConfigMap(configMap *corev1.ConfigMap) error { + return h.Client.Update(h.Context, configMap) +} + +// DeleteConfigMap deletes a ConfigMap by name +func (h *ConfigMapTestHelper) DeleteConfigMap(name string) error { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + }, + } + return h.Client.Delete(h.Context, cm) +} + +// ValidateRegistryData validates the structure of registry data in a ConfigMap +func (h *ConfigMapTestHelper) ValidateRegistryData(configMapName, key string, expectedFormat string) error { + cm, err := h.GetConfigMap(configMapName) + if err != nil { + return fmt.Errorf("failed to get ConfigMap: %w", err) + } + + data, exists := cm.Data[key] + if !exists { + return fmt.Errorf("key %s not found in ConfigMap", key) + } + + switch expectedFormat { + case registryFormatToolHive: + var registryData ToolHiveRegistryData + if err := json.Unmarshal([]byte(data), ®istryData); err != nil { + return fmt.Errorf("failed to unmarshal ToolHive registry data: %w", err) + } + if len(registryData.Servers) == 0 { + return fmt.Errorf("no servers found in ToolHive registry data") + } + case registryFormatUpstream: + var registryData UpstreamRegistryData + if err := json.Unmarshal([]byte(data), ®istryData); err != nil { + return fmt.Errorf("failed to unmarshal upstream registry data: %w", err) + } + if len(registryData.Servers) == 0 { + return fmt.Errorf("no servers found in upstream registry data") + } + default: + return fmt.Errorf("unknown registry format: %s", expectedFormat) + } + + return nil +} + +// GetServerCount returns the number of servers in a registry ConfigMap +func (h *ConfigMapTestHelper) GetServerCount(configMapName, key, format string) (int, error) { + cm, err := h.GetConfigMap(configMapName) + if err != nil { + return 0, err + } + + data, exists := cm.Data[key] + if !exists { + return 0, fmt.Errorf("key %s not found in ConfigMap", key) + } + + switch format { + case registryFormatToolHive: + var registryData ToolHiveRegistryData + if err := json.Unmarshal([]byte(data), ®istryData); err != nil { + return 0, err + } + return len(registryData.Servers), nil + case registryFormatUpstream: + var registryData UpstreamRegistryData + if err := json.Unmarshal([]byte(data), ®istryData); err != nil { + return 0, err + } + return len(registryData.Servers), nil + default: + return 0, fmt.Errorf("unknown registry format: %s", format) + } +} + +// ContainsServer checks if a ConfigMap contains a server with the given name +func (h *ConfigMapTestHelper) ContainsServer(configMapName, key, format, serverName string) (bool, error) { + cm, err := h.GetConfigMap(configMapName) + if err != nil { + return false, err + } + + data, exists := cm.Data[key] + if !exists { + return false, fmt.Errorf("key %s not found in ConfigMap", key) + } + + switch format { + case registryFormatToolHive: + var registryData ToolHiveRegistryData + if err := json.Unmarshal([]byte(data), ®istryData); err != nil { + return false, err + } + for _, server := range registryData.Servers { + if server.Name == serverName { + return true, nil + } + } + case registryFormatUpstream: + var registryData UpstreamRegistryData + if err := json.Unmarshal([]byte(data), ®istryData); err != nil { + return false, err + } + _, exists := registryData.Servers[serverName] + return exists, nil + default: + return false, fmt.Errorf("unknown registry format: %s", format) + } + + return false, nil +} + +// ListConfigMaps returns all ConfigMaps in the namespace +func (h *ConfigMapTestHelper) ListConfigMaps() (*corev1.ConfigMapList, error) { + cmList := &corev1.ConfigMapList{} + err := h.Client.List(h.Context, cmList, client.InNamespace(h.Namespace)) + return cmList, err +} + +// CleanupConfigMaps deletes all test ConfigMaps in the namespace +func (h *ConfigMapTestHelper) CleanupConfigMaps() error { + cmList, err := h.ListConfigMaps() + if err != nil { + return err + } + + for _, cm := range cmList.Items { + // Only delete ConfigMaps with our test label + if cm.Labels != nil && cm.Labels["test.toolhive.io/suite"] == "operator-e2e" { + if err := h.Client.Delete(h.Context, &cm); err != nil { + return err + } + } + } + return nil +} diff --git a/test/e2e/operator/factories.go b/test/e2e/operator/factories.go new file mode 100644 index 000000000..4cf4190e3 --- /dev/null +++ b/test/e2e/operator/factories.go @@ -0,0 +1,450 @@ +package operator_test + +import ( + "context" + "crypto/rand" + "fmt" + "math/big" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// TestDataFactory provides utilities for generating test data and resources +type TestDataFactory struct { + Client client.Client + Context context.Context + Namespace string +} + +// NewTestDataFactory creates a new test data factory +func NewTestDataFactory(ctx context.Context, k8sClient client.Client, namespace string) *TestDataFactory { + return &TestDataFactory{ + Client: k8sClient, + Context: ctx, + Namespace: namespace, + } +} + +// MCPRegistryTemplate represents a template for creating MCPRegistry instances +type MCPRegistryTemplate struct { + NamePrefix string + ConfigMapPrefix string + SyncInterval string + Format string + Labels map[string]string + Annotations map[string]string + ServerCount int + WithSyncPolicy bool + WithFilter bool +} + +// DefaultMCPRegistryTemplate returns a default template for MCPRegistry creation +func (*TestDataFactory) DefaultMCPRegistryTemplate() MCPRegistryTemplate { + return MCPRegistryTemplate{ + NamePrefix: "test-registry", + ConfigMapPrefix: "test-data", + SyncInterval: "1h", + Format: mcpv1alpha1.RegistryFormatToolHive, + Labels: map[string]string{ + "test.toolhive.io/suite": "operator-e2e", + }, + Annotations: make(map[string]string), + ServerCount: 2, + WithSyncPolicy: true, + WithFilter: false, + } +} + +// CreateMCPRegistryFromTemplate creates an MCPRegistry based on a template +func (f *TestDataFactory) CreateMCPRegistryFromTemplate(template MCPRegistryTemplate) ( + *mcpv1alpha1.MCPRegistry, *corev1.ConfigMap, error) { + // Generate unique names + registryName := f.GenerateUniqueName(template.NamePrefix) + configMapName := f.GenerateUniqueName(template.ConfigMapPrefix) + + // Create ConfigMap with test data + configMap, err := f.CreateTestConfigMap(configMapName, template.Format, template.ServerCount) + if err != nil { + return nil, nil, fmt.Errorf("failed to create test ConfigMap: %w", err) + } + + // Create MCPRegistry + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: registryName, + Namespace: f.Namespace, + Labels: copyMap(template.Labels), + Annotations: copyMap(template.Annotations), + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Source: mcpv1alpha1.MCPRegistrySource{ + Type: mcpv1alpha1.RegistrySourceTypeConfigMap, + Format: template.Format, + ConfigMap: &mcpv1alpha1.ConfigMapSource{ + Name: configMapName, + Key: "registry.json", + }, + }, + }, + } + + // Add sync policy if requested + if template.WithSyncPolicy { + registry.Spec.SyncPolicy = &mcpv1alpha1.SyncPolicy{ + Interval: template.SyncInterval, + } + } + + // Add filter if requested + if template.WithFilter { + registry.Spec.Filter = &mcpv1alpha1.RegistryFilter{ + NameFilters: &mcpv1alpha1.NameFilter{ + Include: []string{"*"}, + Exclude: []string{"test-*"}, + }, + } + } + + // Create the registry + if err := f.Client.Create(f.Context, registry); err != nil { + // Clean up ConfigMap if registry creation fails + _ = f.Client.Delete(f.Context, configMap) + return nil, nil, fmt.Errorf("failed to create MCPRegistry: %w", err) + } + + return registry, configMap, nil +} + +// CreateTestConfigMap creates a ConfigMap with test registry data +func (f *TestDataFactory) CreateTestConfigMap(name, format string, serverCount int) (*corev1.ConfigMap, error) { + configMapHelper := NewConfigMapTestHelper(f.Context, f.Client, f.Namespace) + + switch format { + case mcpv1alpha1.RegistryFormatToolHive: + servers := f.GenerateTestServers(serverCount) + return configMapHelper.NewConfigMapBuilder(name). + WithToolHiveRegistry("registry.json", servers). + Create(configMapHelper), nil + + case mcpv1alpha1.RegistryFormatUpstream: + servers := f.GenerateTestServersMap(serverCount) + return configMapHelper.NewConfigMapBuilder(name). + WithUpstreamRegistry("registry.json", servers). + Create(configMapHelper), nil + + default: + return nil, fmt.Errorf("unsupported registry format: %s", format) + } +} + +// GenerateTestServers generates a slice of test servers for ToolHive format +func (f *TestDataFactory) GenerateTestServers(count int) []RegistryServer { + servers := make([]RegistryServer, count) + for i := 0; i < count; i++ { + servers[i] = f.GenerateTestServer(i) + } + return servers +} + +// GenerateTestServersMap generates a map of test servers for upstream format +func (f *TestDataFactory) GenerateTestServersMap(count int) map[string]RegistryServer { + servers := make(map[string]RegistryServer) + for i := 0; i < count; i++ { + server := f.GenerateTestServer(i) + servers[server.Name] = server + } + return servers +} + +// GenerateTestServer generates a single test server +func (*TestDataFactory) GenerateTestServer(index int) RegistryServer { + serverTypes := []string{"filesystem", "fetch", "database", "search", "email"} + transports := []string{"stdio", "sse", "http"} + + serverType := serverTypes[index%len(serverTypes)] + transport := transports[index%len(transports)] + + return RegistryServer{ + Name: fmt.Sprintf("%s-server-%d", serverType, index), + Description: fmt.Sprintf("Test %s server for e2e testing", serverType), + Version: fmt.Sprintf("1.%d.0", index), + SourceURL: fmt.Sprintf("https://github.com/test/servers/tree/main/src/%s", serverType), + Transport: map[string]string{"type": transport}, + Tags: []string{serverType, "test", fmt.Sprintf("v1-%d", index)}, + } +} + +// GenerateUniqueName generates a unique name with timestamp and random suffix +func (*TestDataFactory) GenerateUniqueName(prefix string) string { + timestamp := time.Now().Unix() + // Use crypto/rand for secure random number generation + randomBig, _ := rand.Int(rand.Reader, big.NewInt(1000)) + randomSuffix := randomBig.Int64() + return fmt.Sprintf("%s-%d-%d", prefix, timestamp, randomSuffix) +} + +// CreateTestSecret creates a test secret for authentication +func (f *TestDataFactory) CreateTestSecret(name string, data map[string][]byte) (*corev1.Secret, error) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: f.Namespace, + Labels: map[string]string{ + "test.toolhive.io/suite": "operator-e2e", + }, + }, + Data: data, + } + + if err := f.Client.Create(f.Context, secret); err != nil { + return nil, fmt.Errorf("failed to create secret: %w", err) + } + + return secret, nil +} + +// TestScenario represents a complete test scenario with multiple resources +type TestScenario struct { + Name string + Description string + Registries []MCPRegistryTemplate + ConfigMaps []string + Secrets []string +} + +// CommonTestScenarios returns a set of common test scenarios +func (f *TestDataFactory) CommonTestScenarios() map[string]TestScenario { + return map[string]TestScenario{ + "basic-registry": { + Name: "Basic Registry", + Description: "Single registry with ConfigMap source and sync policy", + Registries: []MCPRegistryTemplate{ + f.DefaultMCPRegistryTemplate(), + }, + }, + "manual-sync-registry": { + Name: "Manual Sync Registry", + Description: "Registry without automatic sync policy", + Registries: []MCPRegistryTemplate{ + func() MCPRegistryTemplate { + template := f.DefaultMCPRegistryTemplate() + template.WithSyncPolicy = false + return template + }(), + }, + }, + "upstream-format-registry": { + Name: "Upstream Format Registry", + Description: "Registry using upstream MCP format", + Registries: []MCPRegistryTemplate{ + func() MCPRegistryTemplate { + template := f.DefaultMCPRegistryTemplate() + template.Format = mcpv1alpha1.RegistryFormatUpstream + return template + }(), + }, + }, + "filtered-registry": { + Name: "Filtered Registry", + Description: "Registry with content filtering", + Registries: []MCPRegistryTemplate{ + func() MCPRegistryTemplate { + template := f.DefaultMCPRegistryTemplate() + template.WithFilter = true + return template + }(), + }, + }, + "multiple-registries": { + Name: "Multiple Registries", + Description: "Multiple registries with different configurations", + Registries: []MCPRegistryTemplate{ + f.DefaultMCPRegistryTemplate(), + func() MCPRegistryTemplate { + template := f.DefaultMCPRegistryTemplate() + template.NamePrefix = "secondary-registry" + template.Format = mcpv1alpha1.RegistryFormatUpstream + template.SyncInterval = "30m" + return template + }(), + }, + }, + } +} + +// CreateTestScenario creates all resources for a test scenario +func (f *TestDataFactory) CreateTestScenario(scenario TestScenario) (*TestScenarioResources, error) { + resources := &TestScenarioResources{ + Registries: make([]*mcpv1alpha1.MCPRegistry, 0), + ConfigMaps: make([]*corev1.ConfigMap, 0), + Secrets: make([]*corev1.Secret, 0), + } + + // Create registries + for _, template := range scenario.Registries { + registry, configMap, err := f.CreateMCPRegistryFromTemplate(template) + if err != nil { + // Clean up already created resources + _ = f.CleanupTestScenarioResources(resources) + return nil, fmt.Errorf("failed to create registry from template: %w", err) + } + resources.Registries = append(resources.Registries, registry) + resources.ConfigMaps = append(resources.ConfigMaps, configMap) + } + + return resources, nil +} + +// TestScenarioResources holds all resources created for a test scenario +type TestScenarioResources struct { + Registries []*mcpv1alpha1.MCPRegistry + ConfigMaps []*corev1.ConfigMap + Secrets []*corev1.Secret +} + +// CleanupTestScenarioResources cleans up all resources in a test scenario +func (f *TestDataFactory) CleanupTestScenarioResources(resources *TestScenarioResources) error { + var errors []error + + // Delete registries + for _, registry := range resources.Registries { + if err := f.Client.Delete(f.Context, registry); err != nil { + errors = append(errors, fmt.Errorf("failed to delete registry %s: %w", registry.Name, err)) + } + } + + // Delete ConfigMaps + for _, cm := range resources.ConfigMaps { + if err := f.Client.Delete(f.Context, cm); err != nil { + errors = append(errors, fmt.Errorf("failed to delete ConfigMap %s: %w", cm.Name, err)) + } + } + + // Delete Secrets + for _, secret := range resources.Secrets { + if err := f.Client.Delete(f.Context, secret); err != nil { + errors = append(errors, fmt.Errorf("failed to delete Secret %s: %w", secret.Name, err)) + } + } + + if len(errors) > 0 { + return fmt.Errorf("cleanup errors: %v", errors) + } + + return nil +} + +// RandomRegistryData generates random registry data for stress testing +func (f *TestDataFactory) RandomRegistryData(serverCount int) []RegistryServer { + servers := make([]RegistryServer, serverCount) + + for i := 0; i < serverCount; i++ { + servers[i] = RegistryServer{ + Name: f.randomServerName(), + Description: f.randomDescription(), + Version: f.randomVersion(), + SourceURL: f.randomSourceURL(), + Transport: map[string]string{"type": f.randomTransport()}, + Tags: f.randomTags(), + } + } + + return servers +} + +// Helper functions for random data generation +func (*TestDataFactory) randomServerName() string { + prefixes := []string{"test", "demo", "sample", "mock", "fake"} + suffixes := []string{"server", "service", "tool", "handler", "processor"} + + prefixBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(prefixes)))) + suffixBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(suffixes)))) + numBig, _ := rand.Int(rand.Reader, big.NewInt(1000)) + + prefix := prefixes[prefixBig.Int64()] + suffix := suffixes[suffixBig.Int64()] + + return fmt.Sprintf("%s-%s-%d", prefix, suffix, numBig.Int64()) +} + +func (*TestDataFactory) randomDescription() string { + templates := []string{ + "A test server for %s operations", + "Mock %s implementation for testing", + "Sample %s service with basic functionality", + "Demo %s tool for development purposes", + } + + operations := []string{"file", "network", "database", "authentication", "processing"} + + templateBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(templates)))) + operationBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(operations)))) + + template := templates[templateBig.Int64()] + operation := operations[operationBig.Int64()] + + return fmt.Sprintf(template, operation) +} + +func (*TestDataFactory) randomVersion() string { + majorBig, _ := rand.Int(rand.Reader, big.NewInt(3)) + minorBig, _ := rand.Int(rand.Reader, big.NewInt(10)) + patchBig, _ := rand.Int(rand.Reader, big.NewInt(20)) + + major := majorBig.Int64() + 1 + minor := minorBig.Int64() + patch := patchBig.Int64() + + return fmt.Sprintf("%d.%d.%d", major, minor, patch) +} + +func (*TestDataFactory) randomSourceURL() string { + orgs := []string{"test-org", "demo-company", "sample-corp"} + repos := []string{"servers", "tools", "services", "handlers"} + + orgBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(orgs)))) + repoBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(repos)))) + + org := orgs[orgBig.Int64()] + repo := repos[repoBig.Int64()] + + return fmt.Sprintf("https://github.com/%s/%s", org, repo) +} + +func (*TestDataFactory) randomTransport() string { + transports := []string{"stdio", "sse", "http"} + transportBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(transports)))) + return transports[transportBig.Int64()] +} + +func (*TestDataFactory) randomTags() []string { + allTags := []string{"test", "demo", "sample", "mock", "development", "staging", "production"} + countBig, _ := rand.Int(rand.Reader, big.NewInt(3)) + count := int(countBig.Int64()) + 1 + + tags := make([]string, count) + for i := 0; i < count; i++ { + tagBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(allTags)))) + tags[i] = allTags[tagBig.Int64()] + } + + return tags +} + +// Utility function to copy maps +func copyMap(m map[string]string) map[string]string { + if m == nil { + return nil + } + + result := make(map[string]string) + for k, v := range m { + result[k] = v + } + return result +} diff --git a/test/e2e/operator/registry_helpers.go b/test/e2e/operator/registry_helpers.go new file mode 100644 index 000000000..6ba99970e --- /dev/null +++ b/test/e2e/operator/registry_helpers.go @@ -0,0 +1,237 @@ +package operator_test + +import ( + "context" + "fmt" + "time" + + "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// MCPRegistryTestHelper provides specialized utilities for MCPRegistry testing +type MCPRegistryTestHelper struct { + Client client.Client + Context context.Context + Namespace string +} + +// NewMCPRegistryTestHelper creates a new test helper for MCPRegistry operations +func NewMCPRegistryTestHelper(ctx context.Context, k8sClient client.Client, namespace string) *MCPRegistryTestHelper { + return &MCPRegistryTestHelper{ + Client: k8sClient, + Context: ctx, + Namespace: namespace, + } +} + +// RegistryBuilder provides a fluent interface for building MCPRegistry objects +type RegistryBuilder struct { + registry *mcpv1alpha1.MCPRegistry +} + +// NewRegistryBuilder creates a new MCPRegistry builder +func (h *MCPRegistryTestHelper) NewRegistryBuilder(name string) *RegistryBuilder { + return &RegistryBuilder{ + registry: &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + Labels: map[string]string{ + "test.toolhive.io/suite": "operator-e2e", + }, + }, + Spec: mcpv1alpha1.MCPRegistrySpec{}, + }, + } +} + +// WithConfigMapSource configures the registry with a ConfigMap source +func (rb *RegistryBuilder) WithConfigMapSource(configMapName, key string) *RegistryBuilder { + rb.registry.Spec.Source = mcpv1alpha1.MCPRegistrySource{ + Type: mcpv1alpha1.RegistrySourceTypeConfigMap, + Format: mcpv1alpha1.RegistryFormatToolHive, + ConfigMap: &mcpv1alpha1.ConfigMapSource{ + Name: configMapName, + Key: key, + }, + } + return rb +} + +// WithUpstreamFormat configures the registry to use upstream MCP format +func (rb *RegistryBuilder) WithUpstreamFormat() *RegistryBuilder { + rb.registry.Spec.Source.Format = mcpv1alpha1.RegistryFormatUpstream + return rb +} + +// WithSyncPolicy configures the sync policy +func (rb *RegistryBuilder) WithSyncPolicy(interval string) *RegistryBuilder { + rb.registry.Spec.SyncPolicy = &mcpv1alpha1.SyncPolicy{ + Interval: interval, + } + return rb +} + +// WithAnnotation adds an annotation to the registry +func (rb *RegistryBuilder) WithAnnotation(key, value string) *RegistryBuilder { + if rb.registry.Annotations == nil { + rb.registry.Annotations = make(map[string]string) + } + rb.registry.Annotations[key] = value + return rb +} + +// WithLabel adds a label to the registry +func (rb *RegistryBuilder) WithLabel(key, value string) *RegistryBuilder { + if rb.registry.Labels == nil { + rb.registry.Labels = make(map[string]string) + } + rb.registry.Labels[key] = value + return rb +} + +// Build returns the constructed MCPRegistry +func (rb *RegistryBuilder) Build() *mcpv1alpha1.MCPRegistry { + return rb.registry.DeepCopy() +} + +// Create builds and creates the MCPRegistry in the cluster +func (rb *RegistryBuilder) Create(h *MCPRegistryTestHelper) *mcpv1alpha1.MCPRegistry { + registry := rb.Build() + err := h.Client.Create(h.Context, registry) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to create MCPRegistry") + return registry +} + +// CreateBasicConfigMapRegistry creates a simple MCPRegistry with ConfigMap source +func (h *MCPRegistryTestHelper) CreateBasicConfigMapRegistry(name, configMapName string) *mcpv1alpha1.MCPRegistry { + return h.NewRegistryBuilder(name). + WithConfigMapSource(configMapName, "registry.json"). + WithSyncPolicy("1h"). + Create(h) +} + +// CreateManualSyncRegistry creates an MCPRegistry with manual sync only +func (h *MCPRegistryTestHelper) CreateManualSyncRegistry(name, configMapName string) *mcpv1alpha1.MCPRegistry { + return h.NewRegistryBuilder(name). + WithConfigMapSource(configMapName, "registry.json"). + Create(h) +} + +// CreateUpstreamFormatRegistry creates an MCPRegistry with upstream format +func (h *MCPRegistryTestHelper) CreateUpstreamFormatRegistry(name, configMapName string) *mcpv1alpha1.MCPRegistry { + return h.NewRegistryBuilder(name). + WithConfigMapSource(configMapName, "registry.json"). + WithUpstreamFormat(). + WithSyncPolicy("30m"). + Create(h) +} + +// GetRegistry retrieves an MCPRegistry by name +func (h *MCPRegistryTestHelper) GetRegistry(name string) (*mcpv1alpha1.MCPRegistry, error) { + registry := &mcpv1alpha1.MCPRegistry{} + err := h.Client.Get(h.Context, types.NamespacedName{ + Namespace: h.Namespace, + Name: name, + }, registry) + return registry, err +} + +// UpdateRegistry updates an existing MCPRegistry +func (h *MCPRegistryTestHelper) UpdateRegistry(registry *mcpv1alpha1.MCPRegistry) error { + return h.Client.Update(h.Context, registry) +} + +// PatchRegistry patches an MCPRegistry with the given patch +func (h *MCPRegistryTestHelper) PatchRegistry(name string, patch client.Patch) error { + registry := &mcpv1alpha1.MCPRegistry{} + registry.Name = name + registry.Namespace = h.Namespace + return h.Client.Patch(h.Context, registry, patch) +} + +// DeleteRegistry deletes an MCPRegistry by name +func (h *MCPRegistryTestHelper) DeleteRegistry(name string) error { + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: h.Namespace, + }, + } + return h.Client.Delete(h.Context, registry) +} + +// TriggerManualSync adds the manual sync annotation to trigger a sync +func (h *MCPRegistryTestHelper) TriggerManualSync(name string) error { + registry, err := h.GetRegistry(name) + if err != nil { + return err + } + + if registry.Annotations == nil { + registry.Annotations = make(map[string]string) + } + registry.Annotations["toolhive.stacklok.dev/manual-sync"] = fmt.Sprintf("%d", time.Now().Unix()) + + return h.UpdateRegistry(registry) +} + +// GetRegistryStatus returns the current status of an MCPRegistry +func (h *MCPRegistryTestHelper) GetRegistryStatus(name string) (*mcpv1alpha1.MCPRegistryStatus, error) { + registry, err := h.GetRegistry(name) + if err != nil { + return nil, err + } + return ®istry.Status, nil +} + +// GetRegistryPhase returns the current phase of an MCPRegistry +func (h *MCPRegistryTestHelper) GetRegistryPhase(name string) (mcpv1alpha1.MCPRegistryPhase, error) { + status, err := h.GetRegistryStatus(name) + if err != nil { + return "", err + } + return status.Phase, nil +} + +// GetRegistryCondition returns a specific condition from the registry status +func (h *MCPRegistryTestHelper) GetRegistryCondition(name, conditionType string) (*metav1.Condition, error) { + status, err := h.GetRegistryStatus(name) + if err != nil { + return nil, err + } + + for _, condition := range status.Conditions { + if condition.Type == conditionType { + return &condition, nil + } + } + return nil, fmt.Errorf("condition %s not found", conditionType) +} + +// ListRegistries returns all MCPRegistries in the namespace +func (h *MCPRegistryTestHelper) ListRegistries() (*mcpv1alpha1.MCPRegistryList, error) { + registryList := &mcpv1alpha1.MCPRegistryList{} + err := h.Client.List(h.Context, registryList, client.InNamespace(h.Namespace)) + return registryList, err +} + +// CleanupRegistries deletes all MCPRegistries in the namespace +func (h *MCPRegistryTestHelper) CleanupRegistries() error { + registryList, err := h.ListRegistries() + if err != nil { + return err + } + + for _, registry := range registryList.Items { + if err := h.Client.Delete(h.Context, ®istry); err != nil { + return err + } + } + return nil +} diff --git a/test/e2e/operator/status_helpers.go b/test/e2e/operator/status_helpers.go new file mode 100644 index 000000000..2ecd676ee --- /dev/null +++ b/test/e2e/operator/status_helpers.go @@ -0,0 +1,252 @@ +package operator_test + +import ( + "context" + "fmt" + "time" + + "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// StatusTestHelper provides utilities for MCPRegistry status testing and validation +type StatusTestHelper struct { + registryHelper *MCPRegistryTestHelper +} + +// NewStatusTestHelper creates a new test helper for status operations +func NewStatusTestHelper(ctx context.Context, k8sClient client.Client, namespace string) *StatusTestHelper { + return &StatusTestHelper{ + registryHelper: NewMCPRegistryTestHelper(ctx, k8sClient, namespace), + } +} + +// WaitForPhase waits for an MCPRegistry to reach the specified phase +func (h *StatusTestHelper) WaitForPhase(registryName string, expectedPhase mcpv1alpha1.MCPRegistryPhase, timeout time.Duration) { + gomega.Eventually(func() mcpv1alpha1.MCPRegistryPhase { + registry, err := h.registryHelper.GetRegistry(registryName) + if err != nil { + return "" + } + return registry.Status.Phase + }, timeout, time.Second).Should(gomega.Equal(expectedPhase), + "MCPRegistry %s should reach phase %s", registryName, expectedPhase) +} + +// WaitForCondition waits for a specific condition to have the expected status +func (h *StatusTestHelper) WaitForCondition(registryName, conditionType string, + expectedStatus metav1.ConditionStatus, timeout time.Duration) { + gomega.Eventually(func() metav1.ConditionStatus { + condition, err := h.registryHelper.GetRegistryCondition(registryName, conditionType) + if err != nil { + return metav1.ConditionUnknown + } + return condition.Status + }, timeout, time.Second).Should(gomega.Equal(expectedStatus), + "MCPRegistry %s should have condition %s with status %s", registryName, conditionType, expectedStatus) +} + +// WaitForConditionReason waits for a condition to have a specific reason +func (h *StatusTestHelper) WaitForConditionReason(registryName, conditionType, expectedReason string, timeout time.Duration) { + gomega.Eventually(func() string { + condition, err := h.registryHelper.GetRegistryCondition(registryName, conditionType) + if err != nil { + return "" + } + return condition.Reason + }, timeout, time.Second).Should(gomega.Equal(expectedReason), + "MCPRegistry %s condition %s should have reason %s", registryName, conditionType, expectedReason) +} + +// WaitForServerCount waits for the registry to report a specific server count +func (h *StatusTestHelper) WaitForServerCount(registryName string, expectedCount int, timeout time.Duration) { + gomega.Eventually(func() int { + status, err := h.registryHelper.GetRegistryStatus(registryName) + if err != nil { + return -1 + } + return status.ServerCount + }, timeout, time.Second).Should(gomega.Equal(expectedCount), + "MCPRegistry %s should have server count %d", registryName, expectedCount) +} + +// WaitForDeployedServerCount waits for the registry to report a specific deployed server count +func (h *StatusTestHelper) WaitForDeployedServerCount(registryName string, expectedCount int, timeout time.Duration) { + gomega.Eventually(func() int { + status, err := h.registryHelper.GetRegistryStatus(registryName) + if err != nil { + return -1 + } + return status.DeployedServerCount + }, timeout, time.Second).Should(gomega.Equal(expectedCount), + "MCPRegistry %s should have deployed server count %d", registryName, expectedCount) +} + +// WaitForLastSyncTime waits for the registry to update its last sync time +func (h *StatusTestHelper) WaitForLastSyncTime(registryName string, afterTime time.Time, timeout time.Duration) { + gomega.Eventually(func() bool { + status, err := h.registryHelper.GetRegistryStatus(registryName) + if err != nil || status.LastSyncTime == nil { + return false + } + return status.LastSyncTime.After(afterTime) + }, timeout, time.Second).Should(gomega.BeTrue(), + "MCPRegistry %s should update last sync time after %s", registryName, afterTime) +} + +// WaitForLastSyncHash waits for the registry to have a non-empty last sync hash +func (h *StatusTestHelper) WaitForLastSyncHash(registryName string, timeout time.Duration) { + gomega.Eventually(func() string { + status, err := h.registryHelper.GetRegistryStatus(registryName) + if err != nil { + return "" + } + return status.LastSyncHash + }, timeout, time.Second).ShouldNot(gomega.BeEmpty(), + "MCPRegistry %s should have a last sync hash", registryName) +} + +// WaitForSyncCompletion waits for a sync operation to complete (either success or failure) +func (h *StatusTestHelper) WaitForSyncCompletion(registryName string, timeout time.Duration) { + gomega.Eventually(func() bool { + registry, err := h.registryHelper.GetRegistry(registryName) + if err != nil { + return false + } + + // Check if sync is no longer in progress + phase := registry.Status.Phase + return phase == mcpv1alpha1.MCPRegistryPhaseReady || + phase == mcpv1alpha1.MCPRegistryPhaseFailed + }, timeout, time.Second).Should(gomega.BeTrue(), + "MCPRegistry %s sync operation should complete", registryName) +} + +// WaitForManualSyncProcessed waits for a manual sync annotation to be processed +func (h *StatusTestHelper) WaitForManualSyncProcessed(registryName, triggerValue string, timeout time.Duration) { + gomega.Eventually(func() string { + status, err := h.registryHelper.GetRegistryStatus(registryName) + if err != nil { + return "" + } + return status.LastManualSyncTrigger + }, timeout, time.Second).Should(gomega.Equal(triggerValue), + "MCPRegistry %s should process manual sync trigger %s", registryName, triggerValue) +} + +// AssertPhase asserts that an MCPRegistry is currently in the specified phase +func (h *StatusTestHelper) AssertPhase(registryName string, expectedPhase mcpv1alpha1.MCPRegistryPhase) { + phase, err := h.registryHelper.GetRegistryPhase(registryName) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get registry phase") + gomega.Expect(phase).To(gomega.Equal(expectedPhase), + "MCPRegistry %s should be in phase %s", registryName, expectedPhase) +} + +// AssertCondition asserts that a condition has the expected status +func (h *StatusTestHelper) AssertCondition(registryName, conditionType string, expectedStatus metav1.ConditionStatus) { + condition, err := h.registryHelper.GetRegistryCondition(registryName, conditionType) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get condition %s", conditionType) + gomega.Expect(condition.Status).To(gomega.Equal(expectedStatus), + "Condition %s should have status %s", conditionType, expectedStatus) +} + +// AssertConditionReason asserts that a condition has the expected reason +func (h *StatusTestHelper) AssertConditionReason(registryName, conditionType, expectedReason string) { + condition, err := h.registryHelper.GetRegistryCondition(registryName, conditionType) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get condition %s", conditionType) + gomega.Expect(condition.Reason).To(gomega.Equal(expectedReason), + "Condition %s should have reason %s", conditionType, expectedReason) +} + +// AssertServerCount asserts that the registry has the expected server count +func (h *StatusTestHelper) AssertServerCount(registryName string, expectedCount int) { + status, err := h.registryHelper.GetRegistryStatus(registryName) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get registry status") + gomega.Expect(status.ServerCount).To(gomega.Equal(expectedCount), + "MCPRegistry %s should have server count %d", registryName, expectedCount) +} + +// AssertHasConditions asserts that the registry has all expected condition types +func (h *StatusTestHelper) AssertHasConditions(registryName string, expectedConditions []string) { + status, err := h.registryHelper.GetRegistryStatus(registryName) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get registry status") + + actualConditions := make(map[string]bool) + for _, condition := range status.Conditions { + actualConditions[condition.Type] = true + } + + for _, expectedCondition := range expectedConditions { + gomega.Expect(actualConditions[expectedCondition]).To(gomega.BeTrue(), + "MCPRegistry %s should have condition %s", registryName, expectedCondition) + } +} + +// AssertStorageRef asserts that the registry has a storage reference configured +func (h *StatusTestHelper) AssertStorageRef(registryName, expectedType string) { + status, err := h.registryHelper.GetRegistryStatus(registryName) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get registry status") + gomega.Expect(status.StorageRef).NotTo(gomega.BeNil(), "Storage reference should be set") + gomega.Expect(status.StorageRef.Type).To(gomega.Equal(expectedType), + "Storage reference type should be %s", expectedType) +} + +// AssertAPIEndpoint asserts that the registry has an API endpoint configured +func (h *StatusTestHelper) AssertAPIEndpoint(registryName string) { + status, err := h.registryHelper.GetRegistryStatus(registryName) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get registry status") + gomega.Expect(status.APIEndpoint).NotTo(gomega.BeEmpty(), "API endpoint should be set") +} + +// GetConditionMessage returns the message of a specific condition +func (h *StatusTestHelper) GetConditionMessage(registryName, conditionType string) (string, error) { + condition, err := h.registryHelper.GetRegistryCondition(registryName, conditionType) + if err != nil { + return "", err + } + return condition.Message, nil +} + +// GetStatusMessage returns the current status message +func (h *StatusTestHelper) GetStatusMessage(registryName string) (string, error) { + status, err := h.registryHelper.GetRegistryStatus(registryName) + if err != nil { + return "", err + } + return status.Message, nil +} + +// PrintStatus prints the current status for debugging purposes +func (h *StatusTestHelper) PrintStatus(registryName string) { + registry, err := h.registryHelper.GetRegistry(registryName) + if err != nil { + fmt.Printf("Failed to get registry %s: %v\n", registryName, err) + return + } + + fmt.Printf("=== MCPRegistry %s Status ===\n", registryName) + fmt.Printf("Phase: %s\n", registry.Status.Phase) + fmt.Printf("Message: %s\n", registry.Status.Message) + fmt.Printf("Server Count: %d\n", registry.Status.ServerCount) + fmt.Printf("Deployed Server Count: %d\n", registry.Status.DeployedServerCount) + if registry.Status.LastSyncTime != nil { + fmt.Printf("Last Sync Time: %s\n", registry.Status.LastSyncTime.Format(time.RFC3339)) + } + fmt.Printf("Last Sync Hash: %s\n", registry.Status.LastSyncHash) + fmt.Printf("Sync Attempts: %d\n", registry.Status.SyncAttempts) + + if len(registry.Status.Conditions) > 0 { + fmt.Printf("Conditions:\n") + for _, condition := range registry.Status.Conditions { + fmt.Printf(" - Type: %s, Status: %s, Reason: %s\n", + condition.Type, condition.Status, condition.Reason) + if condition.Message != "" { + fmt.Printf(" Message: %s\n", condition.Message) + } + } + } + fmt.Printf("==============================\n") +} diff --git a/test/e2e/operator/timing_helpers.go b/test/e2e/operator/timing_helpers.go new file mode 100644 index 000000000..f6d6833aa --- /dev/null +++ b/test/e2e/operator/timing_helpers.go @@ -0,0 +1,312 @@ +package operator_test + +import ( + "context" + "fmt" + "time" + + "github.com/onsi/gomega" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// TimingTestHelper provides utilities for timing and synchronization in async operations +type TimingTestHelper struct { + Client client.Client + Context context.Context +} + +// NewTimingTestHelper creates a new test helper for timing operations +func NewTimingTestHelper(ctx context.Context, k8sClient client.Client) *TimingTestHelper { + return &TimingTestHelper{ + Client: k8sClient, + Context: ctx, + } +} + +// Common timeout values for different types of operations +const ( + // QuickTimeout for operations that should complete quickly (e.g., resource creation) + QuickTimeout = 10 * time.Second + + // MediumTimeout for operations that may take some time (e.g., controller reconciliation) + MediumTimeout = 30 * time.Second + + // LongTimeout for operations that may take a while (e.g., sync operations) + LongTimeout = 2 * time.Minute + + // ExtraLongTimeout for operations that may take very long (e.g., complex e2e scenarios) + ExtraLongTimeout = 5 * time.Minute + + // DefaultPollingInterval for Eventually/Consistently checks + DefaultPollingInterval = 1 * time.Second + + // FastPollingInterval for operations that need frequent checks + FastPollingInterval = 200 * time.Millisecond + + // SlowPollingInterval for operations that don't need frequent checks + SlowPollingInterval = 5 * time.Second +) + +// EventuallyWithTimeout runs an Eventually check with custom timeout and polling +func (*TimingTestHelper) EventuallyWithTimeout(assertion func() interface{}, + timeout, polling time.Duration) gomega.AsyncAssertion { + return gomega.Eventually(assertion, timeout, polling) +} + +// ConsistentlyWithTimeout runs a Consistently check with custom timeout and polling +func (*TimingTestHelper) ConsistentlyWithTimeout(assertion func() interface{}, + duration, polling time.Duration) gomega.AsyncAssertion { + return gomega.Consistently(assertion, duration, polling) +} + +// WaitForResourceCreation waits for a resource to be created with quick timeout +func (*TimingTestHelper) WaitForResourceCreation(assertion func() interface{}) gomega.AsyncAssertion { + return gomega.Eventually(assertion, QuickTimeout, FastPollingInterval) +} + +// WaitForControllerReconciliation waits for controller to reconcile changes +func (*TimingTestHelper) WaitForControllerReconciliation(assertion func() interface{}) gomega.AsyncAssertion { + return gomega.Eventually(assertion, MediumTimeout, DefaultPollingInterval) +} + +// WaitForSyncOperation waits for a sync operation to complete +func (*TimingTestHelper) WaitForSyncOperation(assertion func() interface{}) gomega.AsyncAssertion { + return gomega.Eventually(assertion, LongTimeout, DefaultPollingInterval) +} + +// WaitForComplexOperation waits for complex multi-step operations +func (*TimingTestHelper) WaitForComplexOperation(assertion func() interface{}) gomega.AsyncAssertion { + return gomega.Eventually(assertion, ExtraLongTimeout, SlowPollingInterval) +} + +// EnsureStableState ensures a condition remains stable for a period +func (*TimingTestHelper) EnsureStableState(assertion func() interface{}, duration time.Duration) gomega.AsyncAssertion { + return gomega.Consistently(assertion, duration, DefaultPollingInterval) +} + +// EnsureQuickStability ensures a condition remains stable for a short period +func (h *TimingTestHelper) EnsureQuickStability(assertion func() interface{}) gomega.AsyncAssertion { + return h.EnsureStableState(assertion, 5*time.Second) +} + +// TimeoutConfig represents timeout configuration for different scenarios +type TimeoutConfig struct { + Timeout time.Duration + PollingInterval time.Duration + Description string +} + +// GetTimeoutForOperation returns appropriate timeout configuration for different operation types +func (*TimingTestHelper) GetTimeoutForOperation(operationType string) TimeoutConfig { + switch operationType { + case "create": + return TimeoutConfig{ + Timeout: QuickTimeout, + PollingInterval: FastPollingInterval, + Description: "Resource creation", + } + case "reconcile": + return TimeoutConfig{ + Timeout: MediumTimeout, + PollingInterval: DefaultPollingInterval, + Description: "Controller reconciliation", + } + case "sync": + return TimeoutConfig{ + Timeout: LongTimeout, + PollingInterval: DefaultPollingInterval, + Description: "Sync operation", + } + case "complex": + return TimeoutConfig{ + Timeout: ExtraLongTimeout, + PollingInterval: SlowPollingInterval, + Description: "Complex operation", + } + case "delete": + return TimeoutConfig{ + Timeout: MediumTimeout, + PollingInterval: DefaultPollingInterval, + Description: "Resource deletion", + } + case "status-update": + return TimeoutConfig{ + Timeout: MediumTimeout, + PollingInterval: FastPollingInterval, + Description: "Status update", + } + default: + return TimeoutConfig{ + Timeout: MediumTimeout, + PollingInterval: DefaultPollingInterval, + Description: "Default operation", + } + } +} + +// WaitWithCustomTimeout waits with custom timeout configuration +func (*TimingTestHelper) WaitWithCustomTimeout(assertion func() interface{}, config TimeoutConfig) gomega.AsyncAssertion { + return gomega.Eventually(assertion, config.Timeout, config.PollingInterval) +} + +// MeasureOperationTime measures how long an operation takes to complete +func (*TimingTestHelper) MeasureOperationTime(operation func()) time.Duration { + start := time.Now() + operation() + return time.Since(start) +} + +// WaitForConditionWithRetry waits for a condition with exponential backoff retry +func (*TimingTestHelper) WaitForConditionWithRetry( + condition func() (bool, error), + maxTimeout time.Duration, + initialDelay time.Duration, +) error { + deadline := time.Now().Add(maxTimeout) + delay := initialDelay + + for time.Now().Before(deadline) { + if ok, err := condition(); err != nil { + return err + } else if ok { + return nil + } + + time.Sleep(delay) + delay = delay * 2 + if delay > time.Minute { + delay = time.Minute + } + } + + return context.DeadlineExceeded +} + +// SyncPoint represents a synchronization point for coordinating multiple operations +type SyncPoint struct { + name string + ready chan struct{} + finished chan struct{} +} + +// NewSyncPoint creates a new synchronization point +func (*TimingTestHelper) NewSyncPoint(name string) *SyncPoint { + return &SyncPoint{ + name: name, + ready: make(chan struct{}), + finished: make(chan struct{}), + } +} + +// SignalReady signals that this point is ready +func (sp *SyncPoint) SignalReady() { + close(sp.ready) +} + +// WaitForReady waits for this sync point to be ready +func (sp *SyncPoint) WaitForReady(timeout time.Duration) error { + select { + case <-sp.ready: + return nil + case <-time.After(timeout): + return context.DeadlineExceeded + } +} + +// SignalFinished signals that this point is finished +func (sp *SyncPoint) SignalFinished() { + close(sp.finished) +} + +// WaitForFinished waits for this sync point to be finished +func (sp *SyncPoint) WaitForFinished(timeout time.Duration) error { + select { + case <-sp.finished: + return nil + case <-time.After(timeout): + return context.DeadlineExceeded + } +} + +// MultiSyncCoordinator coordinates multiple sync points +type MultiSyncCoordinator struct { + syncPoints map[string]*SyncPoint +} + +// NewMultiSyncCoordinator creates a new multi-sync coordinator +func (*TimingTestHelper) NewMultiSyncCoordinator() *MultiSyncCoordinator { + return &MultiSyncCoordinator{ + syncPoints: make(map[string]*SyncPoint), + } +} + +// AddSyncPoint adds a new sync point +func (msc *MultiSyncCoordinator) AddSyncPoint(name string) *SyncPoint { + sp := &SyncPoint{ + name: name, + ready: make(chan struct{}), + finished: make(chan struct{}), + } + msc.syncPoints[name] = sp + return sp +} + +// WaitForAllReady waits for all sync points to be ready +func (msc *MultiSyncCoordinator) WaitForAllReady(timeout time.Duration) error { + deadline := time.Now().Add(timeout) + + for name, sp := range msc.syncPoints { + remaining := time.Until(deadline) + if remaining <= 0 { + return context.DeadlineExceeded + } + + if err := sp.WaitForReady(remaining); err != nil { + return err + } + + // Signal that this sync point completed + select { + case <-sp.ready: + // Already ready + default: + return fmt.Errorf("sync point %s not ready", name) + } + } + + return nil +} + +// DelayedExecution executes a function after a specified delay +func (*TimingTestHelper) DelayedExecution(delay time.Duration, fn func()) { + go func() { + time.Sleep(delay) + fn() + }() +} + +// PeriodicExecution executes a function periodically until context is cancelled +func (h *TimingTestHelper) PeriodicExecution(interval time.Duration, fn func()) context.CancelFunc { + ctx, cancel := context.WithCancel(h.Context) + + go func() { + ticker := time.NewTicker(interval) + defer ticker.Stop() + + for { + select { + case <-ticker.C: + fn() + case <-ctx.Done(): + return + } + } + }() + + return cancel +} + +// TimeoutWithContext creates a context with timeout +func (h *TimingTestHelper) TimeoutWithContext(timeout time.Duration) (context.Context, context.CancelFunc) { + return context.WithTimeout(h.Context, timeout) +} From 00b3085f7d4ee8d81322028d2258217599434c45 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Fri, 19 Sep 2025 09:29:55 +0200 Subject: [PATCH 3/7] Fix MCPRegistry e2e test timeouts and finalizer handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix finalizer removal using Patch instead of Update to avoid resource conflicts - Update registry data structure to match expected schema (add required fields: tier, status, tools, image) - Add proper registry deletion waiting in cleanup to prevent namespace deletion issues - Fix lint errors by removing dot imports from non-test files - Add comprehensive MCPRegistry lifecycle test coverage - Improve error handling and logging in test helpers Signed-off-by: Daniele Martinoli Co-authored-by: Claude 🤖 Generated with [Claude Code](https://claude.ai/code) --- .../controllers/mcpregistry_controller.go | 6 +- test/e2e/operator/configmap_helpers.go | 66 ++- test/e2e/operator/factories.go | 38 +- test/e2e/operator/registry_helpers.go | 9 + test/e2e/operator/registry_lifecycle_test.go | 444 ++++++++++++++++++ test/e2e/operator/status_helpers.go | 7 + test/e2e/operator/suite_test.go | 42 +- 7 files changed, 566 insertions(+), 46 deletions(-) create mode 100644 test/e2e/operator/registry_lifecycle_test.go diff --git a/cmd/thv-operator/controllers/mcpregistry_controller.go b/cmd/thv-operator/controllers/mcpregistry_controller.go index 0aad6c965..c9663a1c9 100644 --- a/cmd/thv-operator/controllers/mcpregistry_controller.go +++ b/cmd/thv-operator/controllers/mcpregistry_controller.go @@ -62,7 +62,7 @@ func NewMCPRegistryReconciler(k8sClient client.Client, scheme *runtime.Scheme) * // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries/finalizers,verbs=update +// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries/finalizers,verbs=update;delete // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // @@ -120,8 +120,10 @@ func (r *MCPRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) } // Remove the finalizer. Once all finalizers have been removed, the object will be deleted. + original := mcpRegistry.DeepCopy() controllerutil.RemoveFinalizer(mcpRegistry, "mcpregistry.toolhive.stacklok.dev/finalizer") - err := r.Update(ctx, mcpRegistry) + patch := client.MergeFrom(original) + err := r.Patch(ctx, mcpRegistry, patch) if err != nil { ctxLogger.Error(err, "Reconciliation completed with error while removing finalizer", "MCPRegistry.Name", mcpRegistry.Name) diff --git a/test/e2e/operator/configmap_helpers.go b/test/e2e/operator/configmap_helpers.go index 12d9efec7..9e131d34d 100644 --- a/test/e2e/operator/configmap_helpers.go +++ b/test/e2e/operator/configmap_helpers.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" + ginkgo "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -36,17 +37,22 @@ func NewConfigMapTestHelper(ctx context.Context, k8sClient client.Client, namesp // RegistryServer represents a server definition in the registry type RegistryServer struct { - Name string `json:"name"` - Description string `json:"description,omitempty"` - Version string `json:"version,omitempty"` - SourceURL string `json:"sourceUrl,omitempty"` - Transport map[string]string `json:"transport,omitempty"` - Tags []string `json:"tags,omitempty"` + Name string `json:"name"` + Description string `json:"description,omitempty"` + Tier string `json:"tier"` + Status string `json:"status"` + Transport string `json:"transport"` + Tools []string `json:"tools"` + Image string `json:"image"` + Tags []string `json:"tags,omitempty"` } // ToolHiveRegistryData represents the ToolHive registry format type ToolHiveRegistryData struct { - Servers []RegistryServer `json:"servers"` + Version string `json:"version"` + LastUpdated string `json:"last_updated"` + Servers map[string]RegistryServer `json:"servers"` + RemoteServers map[string]RegistryServer `json:"remoteServers"` } // UpstreamRegistryData represents the upstream MCP registry format @@ -92,7 +98,18 @@ func (cb *ConfigMapBuilder) WithData(key, value string) *ConfigMapBuilder { // WithToolHiveRegistry adds ToolHive format registry data func (cb *ConfigMapBuilder) WithToolHiveRegistry(key string, servers []RegistryServer) *ConfigMapBuilder { - registryData := ToolHiveRegistryData{Servers: servers} + // Convert slice to map using server names as keys + serverMap := make(map[string]RegistryServer) + for _, server := range servers { + serverMap[server.Name] = server + } + + registryData := ToolHiveRegistryData{ + Version: "1.0.0", + LastUpdated: "2025-01-15T10:30:00Z", + Servers: serverMap, + RemoteServers: make(map[string]RegistryServer), + } jsonData, err := json.MarshalIndent(registryData, "", " ") gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to marshal ToolHive registry data") cb.configMap.Data[key] = string(jsonData) @@ -127,17 +144,21 @@ func (h *ConfigMapTestHelper) CreateSampleToolHiveRegistry(name string) *corev1. { Name: "filesystem", Description: "File system operations for secure file access", - Version: "1.0.0", - SourceURL: "https://github.com/modelcontextprotocol/servers/tree/main/src/filesystem", - Transport: map[string]string{"type": "stdio"}, + Tier: "Community", + Status: "Active", + Transport: "stdio", + Tools: []string{"filesystem_tool"}, + Image: "filesystem/server:latest", Tags: []string{"filesystem", "files"}, }, { Name: "fetch", Description: "Web content fetching with readability processing", - Version: "0.1.0", - SourceURL: "https://github.com/modelcontextprotocol/servers/tree/main/src/fetch", - Transport: map[string]string{"type": "stdio"}, + Tier: "Community", + Status: "Active", + Transport: "stdio", + Tools: []string{"fetch_tool"}, + Image: "fetch/server:latest", Tags: []string{"web", "fetch", "readability"}, }, } @@ -153,9 +174,11 @@ func (h *ConfigMapTestHelper) CreateSampleUpstreamRegistry(name string) *corev1. "filesystem": { Name: "filesystem", Description: "File system operations", - Version: "1.0.0", - SourceURL: "https://github.com/modelcontextprotocol/servers/tree/main/src/filesystem", - Transport: map[string]string{"type": "stdio"}, + Tier: "Community", + Status: "Active", + Transport: "stdio", + Tools: []string{"filesystem_tool"}, + Image: "filesystem/server:latest", Tags: []string{"filesystem"}, }, } @@ -275,11 +298,8 @@ func (h *ConfigMapTestHelper) ContainsServer(configMapName, key, format, serverN if err := json.Unmarshal([]byte(data), ®istryData); err != nil { return false, err } - for _, server := range registryData.Servers { - if server.Name == serverName { - return true, nil - } - } + _, exists := registryData.Servers[serverName] + return exists, nil case registryFormatUpstream: var registryData UpstreamRegistryData if err := json.Unmarshal([]byte(data), ®istryData); err != nil { @@ -291,7 +311,6 @@ func (h *ConfigMapTestHelper) ContainsServer(configMapName, key, format, serverN return false, fmt.Errorf("unknown registry format: %s", format) } - return false, nil } // ListConfigMaps returns all ConfigMaps in the namespace @@ -311,6 +330,7 @@ func (h *ConfigMapTestHelper) CleanupConfigMaps() error { for _, cm := range cmList.Items { // Only delete ConfigMaps with our test label if cm.Labels != nil && cm.Labels["test.toolhive.io/suite"] == "operator-e2e" { + ginkgo.By(fmt.Sprintf("deleting ConfigMap %s", cm.Name)) if err := h.Client.Delete(h.Context, &cm); err != nil { return err } diff --git a/test/e2e/operator/factories.go b/test/e2e/operator/factories.go index 4cf4190e3..df608ae85 100644 --- a/test/e2e/operator/factories.go +++ b/test/e2e/operator/factories.go @@ -172,9 +172,11 @@ func (*TestDataFactory) GenerateTestServer(index int) RegistryServer { return RegistryServer{ Name: fmt.Sprintf("%s-server-%d", serverType, index), Description: fmt.Sprintf("Test %s server for e2e testing", serverType), - Version: fmt.Sprintf("1.%d.0", index), - SourceURL: fmt.Sprintf("https://github.com/test/servers/tree/main/src/%s", serverType), - Transport: map[string]string{"type": transport}, + Tier: "Community", + Status: "Active", + Transport: transport, + Tools: []string{fmt.Sprintf("%s_tool", serverType)}, + Image: fmt.Sprintf("%s/server:1.%d.0", serverType, index), Tags: []string{serverType, "test", fmt.Sprintf("v1-%d", index)}, } } @@ -344,12 +346,15 @@ func (f *TestDataFactory) RandomRegistryData(serverCount int) []RegistryServer { servers := make([]RegistryServer, serverCount) for i := 0; i < serverCount; i++ { + serverName := f.randomServerName() servers[i] = RegistryServer{ - Name: f.randomServerName(), + Name: serverName, Description: f.randomDescription(), - Version: f.randomVersion(), - SourceURL: f.randomSourceURL(), - Transport: map[string]string{"type": f.randomTransport()}, + Tier: f.randomTier(), + Status: "Active", + Transport: f.randomTransport(), + Tools: []string{fmt.Sprintf("%s_tool", serverName)}, + Image: fmt.Sprintf("%s/server:%s", serverName, f.randomVersion()), Tags: f.randomTags(), } } @@ -403,25 +408,18 @@ func (*TestDataFactory) randomVersion() string { return fmt.Sprintf("%d.%d.%d", major, minor, patch) } -func (*TestDataFactory) randomSourceURL() string { - orgs := []string{"test-org", "demo-company", "sample-corp"} - repos := []string{"servers", "tools", "services", "handlers"} - - orgBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(orgs)))) - repoBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(repos)))) - - org := orgs[orgBig.Int64()] - repo := repos[repoBig.Int64()] - - return fmt.Sprintf("https://github.com/%s/%s", org, repo) -} - func (*TestDataFactory) randomTransport() string { transports := []string{"stdio", "sse", "http"} transportBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(transports)))) return transports[transportBig.Int64()] } +func (*TestDataFactory) randomTier() string { + tiers := []string{"Community", "Official", "Enterprise"} + tierBig, _ := rand.Int(rand.Reader, big.NewInt(int64(len(tiers)))) + return tiers[tierBig.Int64()] +} + func (*TestDataFactory) randomTags() []string { allTags := []string{"test", "demo", "sample", "mock", "development", "staging", "production"} countBig, _ := rand.Int(rand.Reader, big.NewInt(3)) diff --git a/test/e2e/operator/registry_helpers.go b/test/e2e/operator/registry_helpers.go index 6ba99970e..eadd93193 100644 --- a/test/e2e/operator/registry_helpers.go +++ b/test/e2e/operator/registry_helpers.go @@ -5,7 +5,9 @@ import ( "fmt" "time" + "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -232,6 +234,13 @@ func (h *MCPRegistryTestHelper) CleanupRegistries() error { if err := h.Client.Delete(h.Context, ®istry); err != nil { return err } + + // Wait for registry to be actually deleted + ginkgo.By(fmt.Sprintf("waiting for registry %s to be deleted", registry.Name)) + gomega.Eventually(func() bool { + _, err := h.GetRegistry(registry.Name) + return err != nil && errors.IsNotFound(err) + }, LongTimeout, DefaultPollingInterval).Should(gomega.BeTrue()) } return nil } diff --git a/test/e2e/operator/registry_lifecycle_test.go b/test/e2e/operator/registry_lifecycle_test.go new file mode 100644 index 000000000..bcc901f3b --- /dev/null +++ b/test/e2e/operator/registry_lifecycle_test.go @@ -0,0 +1,444 @@ +package operator_test + +import ( + "context" + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +const ( + registryFinalizerName = "mcpregistry.toolhive.stacklok.dev/finalizer" +) + +var _ = Describe("MCPRegistry Lifecycle Management", func() { + var ( + ctx context.Context + registryHelper *MCPRegistryTestHelper + configMapHelper *ConfigMapTestHelper + statusHelper *StatusTestHelper + timingHelper *TimingTestHelper + testNamespace string + ) + + BeforeEach(func() { + ctx = context.Background() + testNamespace = createTestNamespace(ctx) + + // Initialize helpers + registryHelper = NewMCPRegistryTestHelper(ctx, k8sClient, testNamespace) + configMapHelper = NewConfigMapTestHelper(ctx, k8sClient, testNamespace) + statusHelper = NewStatusTestHelper(ctx, k8sClient, testNamespace) + timingHelper = NewTimingTestHelper(ctx, k8sClient) + }) + + AfterEach(func() { + // Clean up test resources + Expect(registryHelper.CleanupRegistries()).To(Succeed()) + Expect(configMapHelper.CleanupConfigMaps()).To(Succeed()) + deleteTestNamespace(ctx, testNamespace) + }) + + Context("Basic Registry Creation", func() { + It("should create MCPRegistry with correct initial status", func() { + // Create test ConfigMap + configMap := configMapHelper.CreateSampleToolHiveRegistry("test-config") + + // Create MCPRegistry + registry := registryHelper.NewRegistryBuilder("test-registry"). + WithConfigMapSource(configMap.Name, "registry.json"). + WithSyncPolicy("1h"). + Create(registryHelper) + + // Verify registry was created + Expect(registry.Name).To(Equal("test-registry")) + Expect(registry.Namespace).To(Equal(testNamespace)) + + // Verify initial spec + Expect(registry.Spec.Source.Type).To(Equal(mcpv1alpha1.RegistrySourceTypeConfigMap)) + Expect(registry.Spec.Source.ConfigMap.Name).To(Equal(configMap.Name)) + Expect(registry.Spec.SyncPolicy.Interval).To(Equal("1h")) + + // Wait for controller to process and verify initial status + By("waiting for controller to process and verify initial status") + timingHelper.WaitForControllerReconciliation(func() interface{} { + phase, err := registryHelper.GetRegistryPhase(registry.Name) + if err != nil { + return "" + } + return phase + }).Should(BeElementOf( + mcpv1alpha1.MCPRegistryPhasePending, + mcpv1alpha1.MCPRegistryPhaseReady, + mcpv1alpha1.MCPRegistryPhaseSyncing, + )) + + // Verify finalizer was added + By("waiting for finalizer to be added") + timingHelper.WaitForControllerReconciliation(func() interface{} { + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + if err != nil { + return false + } + return containsFinalizer(updatedRegistry.Finalizers, registryFinalizerName) + }).Should(BeTrue()) + + By("verifying registry status") + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + Expect(err).NotTo(HaveOccurred()) + Expect(updatedRegistry.Status.Phase).To(Equal(mcpv1alpha1.MCPRegistryPhaseReady)) + By("BYE") + }) + + It("should handle registry with minimal configuration", func() { + // Create minimal ConfigMap + configMap := configMapHelper.NewConfigMapBuilder("minimal-config"). + WithToolHiveRegistry("registry.json", []RegistryServer{ + { + Name: "test-server", + Description: "Test server", + Tier: "Community", + Status: "Active", + Transport: "stdio", + Tools: []string{"test_tool"}, + Image: "test/server:1.0.0", + }, + }). + Create(configMapHelper) + + // Create minimal registry (no sync policy) + registry := registryHelper.NewRegistryBuilder("minimal-registry"). + WithConfigMapSource(configMap.Name, "registry.json"). + Create(registryHelper) + + // Verify creation + Expect(registry.Spec.SyncPolicy).To(BeNil()) + + // Should still become ready for manual sync + statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + }) + + It("should set correct metadata labels and annotations", func() { + configMap := configMapHelper.CreateSampleToolHiveRegistry("labeled-config") + + registry := registryHelper.NewRegistryBuilder("labeled-registry"). + WithConfigMapSource(configMap.Name, "registry.json"). + WithLabel("app", "test"). + WithLabel("version", "1.0"). + WithAnnotation("description", "Test registry"). + Create(registryHelper) + + // Verify labels and annotations + Expect(registry.Labels).To(HaveKeyWithValue("app", "test")) + Expect(registry.Labels).To(HaveKeyWithValue("version", "1.0")) + Expect(registry.Annotations).To(HaveKeyWithValue("description", "Test registry")) + }) + }) + + Context("Finalizer Management", func() { + It("should add finalizer on creation", func() { + configMap := configMapHelper.CreateSampleToolHiveRegistry("finalizer-config") + + registry := registryHelper.NewRegistryBuilder("finalizer-test"). + WithConfigMapSource(configMap.Name, "registry.json"). + Create(registryHelper) + + // Wait for finalizer to be added + timingHelper.WaitForControllerReconciliation(func() interface{} { + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + if err != nil { + return false + } + return containsFinalizer(updatedRegistry.Finalizers, registryFinalizerName) + }).Should(BeTrue()) + }) + + It("should remove finalizer during deletion", func() { + configMap := configMapHelper.CreateSampleToolHiveRegistry("deletion-config") + + registry := registryHelper.NewRegistryBuilder("deletion-test"). + WithConfigMapSource(configMap.Name, "registry.json"). + Create(registryHelper) + + // Wait for finalizer to be added + timingHelper.WaitForControllerReconciliation(func() interface{} { + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + if err != nil { + return false + } + return containsFinalizer(updatedRegistry.Finalizers, registryFinalizerName) + }).Should(BeTrue()) + + // Delete the registry + Expect(registryHelper.DeleteRegistry(registry.Name)).To(Succeed()) + + // Verify registry enters terminating phase + statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseTerminating, MediumTimeout) + + // Verify registry is eventually deleted (finalizer removed) + timingHelper.WaitForControllerReconciliation(func() interface{} { + _, err := registryHelper.GetRegistry(registry.Name) + return errors.IsNotFound(err) + }).Should(BeTrue()) + }) + }) + + Context("Deletion Handling", func() { + It("should perform graceful deletion with cleanup", func() { + configMap := configMapHelper.CreateSampleToolHiveRegistry("cleanup-config") + + registry := registryHelper.NewRegistryBuilder("cleanup-test"). + WithConfigMapSource(configMap.Name, "registry.json"). + WithSyncPolicy("30m"). + Create(registryHelper) + + // Wait for registry to be ready + statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + + // Store initial storage reference for cleanup verification + status, err := registryHelper.GetRegistryStatus(registry.Name) + Expect(err).NotTo(HaveOccurred()) + initialStorageRef := status.StorageRef + + // Delete the registry + Expect(registryHelper.DeleteRegistry(registry.Name)).To(Succeed()) + + // Verify graceful deletion process + statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseTerminating, QuickTimeout) + + // Verify cleanup of associated resources (if any storage was created) + if initialStorageRef != nil && initialStorageRef.ConfigMapRef != nil { + timingHelper.WaitForControllerReconciliation(func() interface{} { + _, err := configMapHelper.GetConfigMap(initialStorageRef.ConfigMapRef.Name) + // Storage ConfigMap should be cleaned up or marked for deletion + return errors.IsNotFound(err) + }).Should(BeTrue()) + } + + // Verify complete deletion + timingHelper.WaitForControllerReconciliation(func() interface{} { + _, err := registryHelper.GetRegistry(registry.Name) + return errors.IsNotFound(err) + }).Should(BeTrue()) + }) + + It("should handle deletion when source ConfigMap is missing", func() { + configMap := configMapHelper.CreateSampleToolHiveRegistry("missing-config") + + registry := registryHelper.NewRegistryBuilder("missing-source-test"). + WithConfigMapSource(configMap.Name, "registry.json"). + Create(registryHelper) + + // Delete the source ConfigMap first + Expect(configMapHelper.DeleteConfigMap(configMap.Name)).To(Succeed()) + + // Now delete the registry - should still succeed + Expect(registryHelper.DeleteRegistry(registry.Name)).To(Succeed()) + + // Verify deletion completes despite missing source + timingHelper.WaitForControllerReconciliation(func() interface{} { + _, err := registryHelper.GetRegistry(registry.Name) + return errors.IsNotFound(err) + }).Should(BeTrue()) + }) + }) + + Context("Spec Validation", func() { + It("should reject invalid source configuration", func() { + // Try to create registry with missing ConfigMap reference + invalidRegistry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalid-registry", + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Source: mcpv1alpha1.MCPRegistrySource{ + Type: mcpv1alpha1.RegistrySourceTypeConfigMap, + // Missing ConfigMap field + }, + }, + } + + // Should fail validation + err := k8sClient.Create(ctx, invalidRegistry) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("configMap field is required")) + }) + + It("should reject invalid sync interval", func() { + configMap := configMapHelper.CreateSampleToolHiveRegistry("interval-config") + + invalidRegistry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalid-interval", + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Source: mcpv1alpha1.MCPRegistrySource{ + Type: mcpv1alpha1.RegistrySourceTypeConfigMap, + ConfigMap: &mcpv1alpha1.ConfigMapSource{ + Name: configMap.Name, + Key: "registry.json", + }, + }, + SyncPolicy: &mcpv1alpha1.SyncPolicy{ + Interval: "invalid-duration", + }, + }, + } + + // Should fail validation + err := k8sClient.Create(ctx, invalidRegistry) + Expect(err).To(HaveOccurred()) + }) + + It("should handle missing source ConfigMap gracefully", func() { + registry := registryHelper.NewRegistryBuilder("missing-configmap"). + WithConfigMapSource("nonexistent-configmap", "registry.json"). + Create(registryHelper) + + // Should enter failed state due to missing source + statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseFailed, MediumTimeout) + + // Check condition reflects the problem + statusHelper.WaitForCondition(registry.Name, mcpv1alpha1.ConditionSourceAvailable, + metav1.ConditionFalse, MediumTimeout) + }) + }) + + Context("Multiple Registry Management", func() { + It("should handle multiple registries in same namespace", func() { + // Create multiple ConfigMaps + configMap1 := configMapHelper.CreateSampleToolHiveRegistry("config-1") + configMap2 := configMapHelper.CreateSampleUpstreamRegistry("config-2") + + // Create multiple registries + registry1 := registryHelper.NewRegistryBuilder("registry-1"). + WithConfigMapSource(configMap1.Name, "registry.json"). + WithSyncPolicy("1h"). + Create(registryHelper) + + registry2 := registryHelper.NewRegistryBuilder("registry-2"). + WithConfigMapSource(configMap2.Name, "registry.json"). + // WithUpstreamFormat(). + WithSyncPolicy("30m"). + Create(registryHelper) + + // Both should become ready independently + statusHelper.WaitForPhase(registry1.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + statusHelper.WaitForPhase(registry2.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + + // Verify they operate independently + Expect(registry1.Spec.SyncPolicy.Interval).To(Equal("1h")) + Expect(registry2.Spec.SyncPolicy.Interval).To(Equal("30m")) + Expect(registry2.Spec.Source.Format).To(Equal(mcpv1alpha1.RegistryFormatUpstream)) + }) + + It("should allow multiple registries with same ConfigMap source", func() { + // Create shared ConfigMap + sharedConfigMap := configMapHelper.CreateSampleToolHiveRegistry("shared-config") + + // Create multiple registries using same source + registry1 := registryHelper.NewRegistryBuilder("shared-registry-1"). + WithConfigMapSource(sharedConfigMap.Name, "registry.json"). + WithSyncPolicy("1h"). + Create(registryHelper) + + registry2 := registryHelper.NewRegistryBuilder("shared-registry-2"). + WithConfigMapSource(sharedConfigMap.Name, "registry.json"). + WithSyncPolicy("2h"). + Create(registryHelper) + + // Both should become ready + statusHelper.WaitForPhase(registry1.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + statusHelper.WaitForPhase(registry2.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + + // Both should have same server count from shared source + statusHelper.WaitForServerCount(registry1.Name, 2, MediumTimeout) + statusHelper.WaitForServerCount(registry2.Name, 2, MediumTimeout) + }) + + It("should handle registry name conflicts gracefully", func() { + configMap := configMapHelper.CreateSampleToolHiveRegistry("conflict-config") + + // Create first registry + registry1 := registryHelper.NewRegistryBuilder("conflict-registry"). + WithConfigMapSource(configMap.Name, "registry.json"). + Create(registryHelper) + + // Try to create second registry with same name - should fail + duplicateRegistry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "conflict-registry", + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Source: mcpv1alpha1.MCPRegistrySource{ + Type: mcpv1alpha1.RegistrySourceTypeConfigMap, + ConfigMap: &mcpv1alpha1.ConfigMapSource{ + Name: configMap.Name, + Key: "registry.json", + }, + }, + }, + } + + err := k8sClient.Create(ctx, duplicateRegistry) + Expect(err).To(HaveOccurred()) + Expect(errors.IsAlreadyExists(err)).To(BeTrue()) + + // Original registry should still be functional + statusHelper.WaitForPhase(registry1.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + }) + }) +}) + +// Helper function to check if a finalizer exists in the list +func containsFinalizer(finalizers []string, finalizer string) bool { + for _, f := range finalizers { + if f == finalizer { + return true + } + } + return false +} + +// Helper function to create test namespace +func createTestNamespace(ctx context.Context) string { + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-registry-lifecycle-", + Labels: map[string]string{ + "test.toolhive.io/suite": "operator-e2e", + }, + }, + } + + Expect(k8sClient.Create(ctx, namespace)).To(Succeed()) + return namespace.Name +} + +// Helper function to delete test namespace +func deleteTestNamespace(ctx context.Context, name string) { + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + } + + By(fmt.Sprintf("deleting namespace %s", name)) + _ = k8sClient.Delete(ctx, namespace) + By(fmt.Sprintf("deleted namespace %s", name)) + + // Wait for namespace deletion + // Eventually(func() bool { + // err := k8sClient.Get(ctx, types.NamespacedName{Name: name}, namespace) + // return errors.IsNotFound(err) + // }, LongTimeout, DefaultPollingInterval).Should(BeTrue()) +} diff --git a/test/e2e/operator/status_helpers.go b/test/e2e/operator/status_helpers.go index 2ecd676ee..7b8c75579 100644 --- a/test/e2e/operator/status_helpers.go +++ b/test/e2e/operator/status_helpers.go @@ -5,7 +5,9 @@ import ( "fmt" "time" + "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -27,8 +29,13 @@ func NewStatusTestHelper(ctx context.Context, k8sClient client.Client, namespace // WaitForPhase waits for an MCPRegistry to reach the specified phase func (h *StatusTestHelper) WaitForPhase(registryName string, expectedPhase mcpv1alpha1.MCPRegistryPhase, timeout time.Duration) { gomega.Eventually(func() mcpv1alpha1.MCPRegistryPhase { + ginkgo.By(fmt.Sprintf("waiting for registry %s to reach phase %s", registryName, expectedPhase)) registry, err := h.registryHelper.GetRegistry(registryName) if err != nil { + if errors.IsNotFound(err) { + ginkgo.By(fmt.Sprintf("registry %s not found", registryName)) + return mcpv1alpha1.MCPRegistryPhaseTerminating + } return "" } return registry.Status.Phase diff --git a/test/e2e/operator/suite_test.go b/test/e2e/operator/suite_test.go index 63c786250..3640eb900 100644 --- a/test/e2e/operator/suite_test.go +++ b/test/e2e/operator/suite_test.go @@ -14,18 +14,22 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/cmd/thv-operator/controllers" ) var ( cfg *rest.Config k8sClient client.Client testEnv *envtest.Environment + testMgr ctrl.Manager ctx context.Context cancel context.CancelFunc ) @@ -40,6 +44,11 @@ var _ = BeforeSuite(func() { ctx, cancel = context.WithCancel(context.TODO()) + // Enable experimental features for MCPRegistry controller + By("enabling experimental features") + err := os.Setenv("ENABLE_EXPERIMENTAL_FEATURES", "true") + Expect(err).NotTo(HaveOccurred()) + By("bootstrapping test environment") // Check if we should use an existing cluster (for CI/CD) @@ -53,7 +62,6 @@ var _ = BeforeSuite(func() { ErrorIfCRDPathMissing: true, } - var err error cfg, err = testEnv.Start() Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) @@ -76,6 +84,38 @@ var _ = BeforeSuite(func() { Name: "test-availability-check", }, mcpRegistry) }, time.Minute, time.Second).Should(MatchError(ContainSubstring("not found"))) + + // Set up the manager for controllers (only for envtest, not existing cluster) + if !useExistingCluster { + By("setting up controller manager for envtest") + testMgr, err = ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme.Scheme, + Metrics: metricsserver.Options{ + BindAddress: "0", // Disable metrics server for tests + }, + HealthProbeBindAddress: "0", // Disable health probe for tests + }) + Expect(err).NotTo(HaveOccurred()) + + // Set up MCPRegistry controller + By("setting up MCPRegistry controller") + err = controllers.NewMCPRegistryReconciler(testMgr.GetClient(), testMgr.GetScheme()).SetupWithManager(testMgr) + Expect(err).NotTo(HaveOccurred()) + + // Start the manager in the background + By("starting controller manager") + go func() { + defer GinkgoRecover() + err = testMgr.Start(ctx) + Expect(err).NotTo(HaveOccurred(), "failed to run manager") + }() + + // Wait for the manager to be ready + By("waiting for controller manager to be ready") + Eventually(func() bool { + return testMgr.GetCache().WaitForCacheSync(ctx) + }, time.Minute, time.Second).Should(BeTrue()) + } }) var _ = AfterSuite(func() { From ca586805ad040ad80a499e23669d8d19538fe9a0 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Thu, 25 Sep 2025 12:01:10 +0200 Subject: [PATCH 4/7] reviewed finalization logic to avoid unnecessary attempts (and logged errors) Signed-off-by: Daniele Martinoli --- .../controllers/mcpregistry_controller.go | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpregistry_controller.go b/cmd/thv-operator/controllers/mcpregistry_controller.go index c9663a1c9..d72e66cb5 100644 --- a/cmd/thv-operator/controllers/mcpregistry_controller.go +++ b/cmd/thv-operator/controllers/mcpregistry_controller.go @@ -111,23 +111,25 @@ func (r *MCPRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) if mcpRegistry.GetDeletionTimestamp() != nil { // The object is being deleted if controllerutil.ContainsFinalizer(mcpRegistry, "mcpregistry.toolhive.stacklok.dev/finalizer") { - // Run finalization logic. If the finalization logic fails, - // don't remove the finalizer so that we can retry during the next reconciliation. - if err := r.finalizeMCPRegistry(ctx, mcpRegistry); err != nil { - ctxLogger.Error(err, "Reconciliation completed with error while finalizing MCPRegistry", - "MCPRegistry.Name", mcpRegistry.Name) - return ctrl.Result{}, err - } - - // Remove the finalizer. Once all finalizers have been removed, the object will be deleted. - original := mcpRegistry.DeepCopy() - controllerutil.RemoveFinalizer(mcpRegistry, "mcpregistry.toolhive.stacklok.dev/finalizer") - patch := client.MergeFrom(original) - err := r.Patch(ctx, mcpRegistry, patch) - if err != nil { - ctxLogger.Error(err, "Reconciliation completed with error while removing finalizer", - "MCPRegistry.Name", mcpRegistry.Name) - return ctrl.Result{}, err + // Run finalization logic only if not already terminating to avoid redundant work + if mcpRegistry.Status.Phase != mcpv1alpha1.MCPRegistryPhaseTerminating { + // Run finalization logic. If the finalization logic fails, + // don't remove the finalizer so that we can retry during the next reconciliation. + if err := r.finalizeMCPRegistry(ctx, mcpRegistry); err != nil { + ctxLogger.Error(err, "Reconciliation completed with error while finalizing MCPRegistry", + "MCPRegistry.Name", mcpRegistry.Name) + return ctrl.Result{}, err + } + // Remove the finalizer. Once all finalizers have been removed, the object will be deleted. + original := mcpRegistry.DeepCopy() + controllerutil.RemoveFinalizer(mcpRegistry, "mcpregistry.toolhive.stacklok.dev/finalizer") + patch := client.MergeFrom(original) + err := r.Patch(ctx, mcpRegistry, patch) + if err != nil { + ctxLogger.Error(err, "Reconciliation completed with error while removing finalizer", + "MCPRegistry.Name", mcpRegistry.Name) + return ctrl.Result{}, err + } } } ctxLogger.Info("Reconciliation of deleted MCPRegistry completed successfully", From 46ce322d9aa551f2e45f5d0143adeb3d99833504 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Thu, 25 Sep 2025 12:02:32 +0200 Subject: [PATCH 5/7] extended and successful validation of "should create MCPRegistry with correct initial status" Signed-off-by: Daniele Martinoli --- test/e2e/operator/configmap_helpers.go | 8 +- test/e2e/operator/k8s_helpers.go | 134 +++++++++++++++++++ test/e2e/operator/registry_lifecycle_test.go | 124 +++++++++++++---- test/e2e/operator/status_helpers.go | 35 ++--- 4 files changed, 250 insertions(+), 51 deletions(-) create mode 100644 test/e2e/operator/k8s_helpers.go diff --git a/test/e2e/operator/configmap_helpers.go b/test/e2e/operator/configmap_helpers.go index 9e131d34d..4cfde14fb 100644 --- a/test/e2e/operator/configmap_helpers.go +++ b/test/e2e/operator/configmap_helpers.go @@ -139,7 +139,7 @@ func (cb *ConfigMapBuilder) Create(h *ConfigMapTestHelper) *corev1.ConfigMap { } // CreateSampleToolHiveRegistry creates a ConfigMap with sample ToolHive registry data -func (h *ConfigMapTestHelper) CreateSampleToolHiveRegistry(name string) *corev1.ConfigMap { +func (h *ConfigMapTestHelper) CreateSampleToolHiveRegistry(name string) (*corev1.ConfigMap, int) { servers := []RegistryServer{ { Name: "filesystem", @@ -165,11 +165,11 @@ func (h *ConfigMapTestHelper) CreateSampleToolHiveRegistry(name string) *corev1. return h.NewConfigMapBuilder(name). WithToolHiveRegistry("registry.json", servers). - Create(h) + Create(h), len(servers) } // CreateSampleUpstreamRegistry creates a ConfigMap with sample upstream registry data -func (h *ConfigMapTestHelper) CreateSampleUpstreamRegistry(name string) *corev1.ConfigMap { +func (h *ConfigMapTestHelper) CreateSampleUpstreamRegistry(name string) (*corev1.ConfigMap, int) { servers := map[string]RegistryServer{ "filesystem": { Name: "filesystem", @@ -185,7 +185,7 @@ func (h *ConfigMapTestHelper) CreateSampleUpstreamRegistry(name string) *corev1. return h.NewConfigMapBuilder(name). WithUpstreamRegistry("registry.json", servers). - Create(h) + Create(h), len(servers) } // GetConfigMap retrieves a ConfigMap by name diff --git a/test/e2e/operator/k8s_helpers.go b/test/e2e/operator/k8s_helpers.go new file mode 100644 index 000000000..4233ed095 --- /dev/null +++ b/test/e2e/operator/k8s_helpers.go @@ -0,0 +1,134 @@ +package operator_test + +import ( + "context" + "fmt" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// K8sResourceTestHelper provides utilities for testing Kubernetes resources +type K8sResourceTestHelper struct { + ctx context.Context + k8sClient client.Client + namespace string +} + +// NewK8sResourceTestHelper creates a new test helper for Kubernetes resources +func NewK8sResourceTestHelper(ctx context.Context, k8sClient client.Client, namespace string) *K8sResourceTestHelper { + return &K8sResourceTestHelper{ + ctx: ctx, + k8sClient: k8sClient, + namespace: namespace, + } +} + +// GetDeployment retrieves a deployment by name +func (h *K8sResourceTestHelper) GetDeployment(name string) (*appsv1.Deployment, error) { + deployment := &appsv1.Deployment{} + err := h.k8sClient.Get(h.ctx, types.NamespacedName{ + Namespace: h.namespace, + Name: name, + }, deployment) + return deployment, err +} + +// GetService retrieves a service by name +func (h *K8sResourceTestHelper) GetService(name string) (*corev1.Service, error) { + service := &corev1.Service{} + err := h.k8sClient.Get(h.ctx, types.NamespacedName{ + Namespace: h.namespace, + Name: name, + }, service) + return service, err +} + +// GetConfigMap retrieves a configmap by name +func (h *K8sResourceTestHelper) GetConfigMap(name string) (*corev1.ConfigMap, error) { + configMap := &corev1.ConfigMap{} + err := h.k8sClient.Get(h.ctx, types.NamespacedName{ + Namespace: h.namespace, + Name: name, + }, configMap) + return configMap, err +} + +// DeploymentExists checks if a deployment exists +func (h *K8sResourceTestHelper) DeploymentExists(name string) bool { + _, err := h.GetDeployment(name) + return err == nil +} + +// ServiceExists checks if a service exists +func (h *K8sResourceTestHelper) ServiceExists(name string) bool { + _, err := h.GetService(name) + return err == nil +} + +// IsDeploymentReady checks if a deployment is ready (all replicas available) +func (h *K8sResourceTestHelper) IsDeploymentReady(name string) bool { + deployment, err := h.GetDeployment(name) + if err != nil { + return false + } + + // Check if deployment has at least one replica and all are available + if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas == 0 { + return false + } + + return deployment.Status.ReadyReplicas == *deployment.Spec.Replicas +} + +// GetDeploymentOwnerReferences returns the owner references of a deployment +func (h *K8sResourceTestHelper) GetDeploymentOwnerReferences(name string) ([]metav1.OwnerReference, error) { + deployment, err := h.GetDeployment(name) + if err != nil { + return nil, err + } + return deployment.OwnerReferences, nil +} + +// GetServiceOwnerReferences returns the owner references of a service +func (h *K8sResourceTestHelper) GetServiceOwnerReferences(name string) ([]metav1.OwnerReference, error) { + service, err := h.GetService(name) + if err != nil { + return nil, err + } + return service.OwnerReferences, nil +} + +// GetServiceEndpoint returns the service endpoint (cluster DNS name) +func (h *K8sResourceTestHelper) GetServiceEndpoint(name string) (string, error) { + service, err := h.GetService(name) + if err != nil { + return "", err + } + + // Return cluster-internal endpoint + if len(service.Spec.Ports) > 0 { + port := service.Spec.Ports[0].Port + return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", name, h.namespace, port), nil + } + + return "", fmt.Errorf("service has no ports defined") +} + +// WaitForResourceDeletion waits for a resource to be deleted +func (h *K8sResourceTestHelper) WaitForResourceDeletion(resourceType, name string) bool { + switch resourceType { + case "deployment": + _, err := h.GetDeployment(name) + return errors.IsNotFound(err) + case "service": + _, err := h.GetService(name) + return errors.IsNotFound(err) + default: + return false + } +} \ No newline at end of file diff --git a/test/e2e/operator/registry_lifecycle_test.go b/test/e2e/operator/registry_lifecycle_test.go index bcc901f3b..19c8ac259 100644 --- a/test/e2e/operator/registry_lifecycle_test.go +++ b/test/e2e/operator/registry_lifecycle_test.go @@ -24,6 +24,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { configMapHelper *ConfigMapTestHelper statusHelper *StatusTestHelper timingHelper *TimingTestHelper + k8sHelper *K8sResourceTestHelper testNamespace string ) @@ -36,6 +37,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { configMapHelper = NewConfigMapTestHelper(ctx, k8sClient, testNamespace) statusHelper = NewStatusTestHelper(ctx, k8sClient, testNamespace) timingHelper = NewTimingTestHelper(ctx, k8sClient) + k8sHelper = NewK8sResourceTestHelper(ctx, k8sClient, testNamespace) }) AfterEach(func() { @@ -48,7 +50,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Context("Basic Registry Creation", func() { It("should create MCPRegistry with correct initial status", func() { // Create test ConfigMap - configMap := configMapHelper.CreateSampleToolHiveRegistry("test-config") + configMap, numServers := configMapHelper.CreateSampleToolHiveRegistry("test-config") // Create MCPRegistry registry := registryHelper.NewRegistryBuilder("test-registry"). @@ -65,6 +67,16 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Expect(registry.Spec.Source.ConfigMap.Name).To(Equal(configMap.Name)) Expect(registry.Spec.SyncPolicy.Interval).To(Equal("1h")) + // Verify finalizer was added + By("waiting for finalizer to be added") + timingHelper.WaitForControllerReconciliation(func() interface{} { + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + if err != nil { + return false + } + return containsFinalizer(updatedRegistry.Finalizers, registryFinalizerName) + }).Should(BeTrue()) + // Wait for controller to process and verify initial status By("waiting for controller to process and verify initial status") timingHelper.WaitForControllerReconciliation(func() interface{} { @@ -79,20 +91,84 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { mcpv1alpha1.MCPRegistryPhaseSyncing, )) - // Verify finalizer was added - By("waiting for finalizer to be added") + By("verifying storage ConfigMap is defined in status and exists") + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + Expect(err).NotTo(HaveOccurred()) + + // Verify storage reference is set in status + Expect(updatedRegistry.Status.StorageRef).NotTo(BeNil()) + Expect(updatedRegistry.Status.StorageRef.Type).To(Equal("configmap")) + Expect(updatedRegistry.Status.StorageRef.ConfigMapRef).NotTo(BeNil()) + Expect(updatedRegistry.Status.StorageRef.ConfigMapRef.Name).NotTo(BeEmpty()) + + // Verify the storage ConfigMap actually exists + storageConfigMapName := updatedRegistry.Status.StorageRef.ConfigMapRef.Name + storageConfigMap, err := k8sHelper.GetConfigMap(storageConfigMapName) + Expect(err).NotTo(HaveOccurred()) + Expect(storageConfigMap.Name).To(Equal(storageConfigMapName)) + Expect(storageConfigMap.Namespace).To(Equal(testNamespace)) + + // Verify it has the registry.json key + Expect(storageConfigMap.Data).To(HaveKey("registry.json")) + Expect(storageConfigMap.Data["registry.json"]).NotTo(BeEmpty()) + + By("verifying Registry API Service and Deployment exist") + apiResourceName := updatedRegistry.GetAPIResourceName() + + // Wait for Service to be created timingHelper.WaitForControllerReconciliation(func() interface{} { - updatedRegistry, err := registryHelper.GetRegistry(registry.Name) - if err != nil { - return false - } - return containsFinalizer(updatedRegistry.Finalizers, registryFinalizerName) - }).Should(BeTrue()) + return k8sHelper.ServiceExists(apiResourceName) + }).Should(BeTrue(), "Registry API Service should exist") + + // Wait for Deployment to be created + timingHelper.WaitForControllerReconciliation(func() interface{} { + return k8sHelper.DeploymentExists(apiResourceName) + }).Should(BeTrue(), "Registry API Deployment should exist") + + // Verify the Service has correct configuration + service, err := k8sHelper.GetService(apiResourceName) + Expect(err).NotTo(HaveOccurred()) + Expect(service.Name).To(Equal(apiResourceName)) + Expect(service.Namespace).To(Equal(testNamespace)) + Expect(service.Spec.Ports).To(HaveLen(1)) + Expect(service.Spec.Ports[0].Name).To(Equal("http")) + + // Verify the Deployment has correct configuration + deployment, err := k8sHelper.GetDeployment(apiResourceName) + Expect(err).NotTo(HaveOccurred()) + Expect(deployment.Name).To(Equal(apiResourceName)) + Expect(deployment.Namespace).To(Equal(testNamespace)) + Expect(deployment.Spec.Template.Spec.Containers).To(HaveLen(1)) + Expect(deployment.Spec.Template.Spec.Containers[0].Name).To(Equal("registry-api")) + + By("verifying deployment has proper ownership") + Expect(deployment.OwnerReferences).To(HaveLen(1)) + Expect(deployment.OwnerReferences[0].Kind).To(Equal("MCPRegistry")) + Expect(deployment.OwnerReferences[0].Name).To(Equal(registry.Name)) By("verifying registry status") - updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + updatedRegistry, err = registryHelper.GetRegistry(registry.Name) Expect(err).NotTo(HaveOccurred()) - Expect(updatedRegistry.Status.Phase).To(Equal(mcpv1alpha1.MCPRegistryPhaseReady)) + // In envtest, the deployment won't actually be ready, so expect Pending phase + // but verify that sync is complete and API deployment is in progress + Expect(updatedRegistry.Status.Phase).To(BeElementOf( + mcpv1alpha1.MCPRegistryPhasePending, // API deployment in progress + mcpv1alpha1.MCPRegistryPhaseReady, // If somehow API becomes ready + )) + + // Verify sync is complete + Expect(updatedRegistry.Status.SyncStatus).NotTo(BeNil()) + Expect(updatedRegistry.Status.SyncStatus.Phase).To(BeElementOf(mcpv1alpha1.SyncPhaseComplete, mcpv1alpha1.SyncPhaseIdle)) + Expect(updatedRegistry.Status.SyncStatus.AttemptCount).To(Equal(0)) + Expect(updatedRegistry.Status.SyncStatus.ServerCount).To(Equal(numServers)) + + // Verify API status exists and shows deployment + Expect(updatedRegistry.Status.APIStatus).NotTo(BeNil()) + Expect(updatedRegistry.Status.APIStatus.Phase).To(BeElementOf( + mcpv1alpha1.APIPhaseDeploying, // Deployment created but not ready + mcpv1alpha1.APIPhaseReady, // If somehow becomes ready + )) + Expect(updatedRegistry.Status.APIStatus.Endpoint).To(Equal(fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", apiResourceName, testNamespace))) By("BYE") }) @@ -125,7 +201,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { }) It("should set correct metadata labels and annotations", func() { - configMap := configMapHelper.CreateSampleToolHiveRegistry("labeled-config") + configMap, _ := configMapHelper.CreateSampleToolHiveRegistry("labeled-config") registry := registryHelper.NewRegistryBuilder("labeled-registry"). WithConfigMapSource(configMap.Name, "registry.json"). @@ -143,7 +219,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Context("Finalizer Management", func() { It("should add finalizer on creation", func() { - configMap := configMapHelper.CreateSampleToolHiveRegistry("finalizer-config") + configMap, _ := configMapHelper.CreateSampleToolHiveRegistry("finalizer-config") registry := registryHelper.NewRegistryBuilder("finalizer-test"). WithConfigMapSource(configMap.Name, "registry.json"). @@ -160,7 +236,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { }) It("should remove finalizer during deletion", func() { - configMap := configMapHelper.CreateSampleToolHiveRegistry("deletion-config") + configMap, _ := configMapHelper.CreateSampleToolHiveRegistry("deletion-config") registry := registryHelper.NewRegistryBuilder("deletion-test"). WithConfigMapSource(configMap.Name, "registry.json"). @@ -191,7 +267,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Context("Deletion Handling", func() { It("should perform graceful deletion with cleanup", func() { - configMap := configMapHelper.CreateSampleToolHiveRegistry("cleanup-config") + configMap, _ := configMapHelper.CreateSampleToolHiveRegistry("cleanup-config") registry := registryHelper.NewRegistryBuilder("cleanup-test"). WithConfigMapSource(configMap.Name, "registry.json"). @@ -229,7 +305,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { }) It("should handle deletion when source ConfigMap is missing", func() { - configMap := configMapHelper.CreateSampleToolHiveRegistry("missing-config") + configMap, _ := configMapHelper.CreateSampleToolHiveRegistry("missing-config") registry := registryHelper.NewRegistryBuilder("missing-source-test"). WithConfigMapSource(configMap.Name, "registry.json"). @@ -272,7 +348,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { }) It("should reject invalid sync interval", func() { - configMap := configMapHelper.CreateSampleToolHiveRegistry("interval-config") + configMap, _ := configMapHelper.CreateSampleToolHiveRegistry("interval-config") invalidRegistry := &mcpv1alpha1.MCPRegistry{ ObjectMeta: metav1.ObjectMeta{ @@ -313,10 +389,12 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { }) Context("Multiple Registry Management", func() { + var numServers1, numServers2 int + var configMap1, configMap2 *corev1.ConfigMap It("should handle multiple registries in same namespace", func() { // Create multiple ConfigMaps - configMap1 := configMapHelper.CreateSampleToolHiveRegistry("config-1") - configMap2 := configMapHelper.CreateSampleUpstreamRegistry("config-2") + configMap1, numServers1 = configMapHelper.CreateSampleToolHiveRegistry("config-1") + configMap2, numServers2 = configMapHelper.CreateSampleUpstreamRegistry("config-2") // Create multiple registries registry1 := registryHelper.NewRegistryBuilder("registry-1"). @@ -342,7 +420,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { It("should allow multiple registries with same ConfigMap source", func() { // Create shared ConfigMap - sharedConfigMap := configMapHelper.CreateSampleToolHiveRegistry("shared-config") + sharedConfigMap, _ := configMapHelper.CreateSampleToolHiveRegistry("shared-config") // Create multiple registries using same source registry1 := registryHelper.NewRegistryBuilder("shared-registry-1"). @@ -360,12 +438,12 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { statusHelper.WaitForPhase(registry2.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) // Both should have same server count from shared source - statusHelper.WaitForServerCount(registry1.Name, 2, MediumTimeout) - statusHelper.WaitForServerCount(registry2.Name, 2, MediumTimeout) + statusHelper.WaitForServerCount(registry1.Name, numServers1, MediumTimeout) + statusHelper.WaitForServerCount(registry2.Name, numServers2, MediumTimeout) }) It("should handle registry name conflicts gracefully", func() { - configMap := configMapHelper.CreateSampleToolHiveRegistry("conflict-config") + configMap, _ := configMapHelper.CreateSampleToolHiveRegistry("conflict-config") // Create first registry registry1 := registryHelper.NewRegistryBuilder("conflict-registry"). diff --git a/test/e2e/operator/status_helpers.go b/test/e2e/operator/status_helpers.go index 7b8c75579..269f21019 100644 --- a/test/e2e/operator/status_helpers.go +++ b/test/e2e/operator/status_helpers.go @@ -75,31 +75,19 @@ func (h *StatusTestHelper) WaitForServerCount(registryName string, expectedCount if err != nil { return -1 } - return status.ServerCount + return status.SyncStatus.ServerCount }, timeout, time.Second).Should(gomega.Equal(expectedCount), "MCPRegistry %s should have server count %d", registryName, expectedCount) } -// WaitForDeployedServerCount waits for the registry to report a specific deployed server count -func (h *StatusTestHelper) WaitForDeployedServerCount(registryName string, expectedCount int, timeout time.Duration) { - gomega.Eventually(func() int { - status, err := h.registryHelper.GetRegistryStatus(registryName) - if err != nil { - return -1 - } - return status.DeployedServerCount - }, timeout, time.Second).Should(gomega.Equal(expectedCount), - "MCPRegistry %s should have deployed server count %d", registryName, expectedCount) -} - // WaitForLastSyncTime waits for the registry to update its last sync time func (h *StatusTestHelper) WaitForLastSyncTime(registryName string, afterTime time.Time, timeout time.Duration) { gomega.Eventually(func() bool { status, err := h.registryHelper.GetRegistryStatus(registryName) - if err != nil || status.LastSyncTime == nil { + if err != nil || status.SyncStatus.LastSyncTime == nil { return false } - return status.LastSyncTime.After(afterTime) + return status.SyncStatus.LastSyncTime.After(afterTime) }, timeout, time.Second).Should(gomega.BeTrue(), "MCPRegistry %s should update last sync time after %s", registryName, afterTime) } @@ -111,7 +99,7 @@ func (h *StatusTestHelper) WaitForLastSyncHash(registryName string, timeout time if err != nil { return "" } - return status.LastSyncHash + return status.SyncStatus.LastSyncHash }, timeout, time.Second).ShouldNot(gomega.BeEmpty(), "MCPRegistry %s should have a last sync hash", registryName) } @@ -172,7 +160,7 @@ func (h *StatusTestHelper) AssertConditionReason(registryName, conditionType, ex func (h *StatusTestHelper) AssertServerCount(registryName string, expectedCount int) { status, err := h.registryHelper.GetRegistryStatus(registryName) gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get registry status") - gomega.Expect(status.ServerCount).To(gomega.Equal(expectedCount), + gomega.Expect(status.SyncStatus.ServerCount).To(gomega.Equal(expectedCount), "MCPRegistry %s should have server count %d", registryName, expectedCount) } @@ -205,7 +193,7 @@ func (h *StatusTestHelper) AssertStorageRef(registryName, expectedType string) { func (h *StatusTestHelper) AssertAPIEndpoint(registryName string) { status, err := h.registryHelper.GetRegistryStatus(registryName) gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to get registry status") - gomega.Expect(status.APIEndpoint).NotTo(gomega.BeEmpty(), "API endpoint should be set") + gomega.Expect(status.APIStatus.Endpoint).NotTo(gomega.BeEmpty(), "API endpoint should be set") } // GetConditionMessage returns the message of a specific condition @@ -237,13 +225,12 @@ func (h *StatusTestHelper) PrintStatus(registryName string) { fmt.Printf("=== MCPRegistry %s Status ===\n", registryName) fmt.Printf("Phase: %s\n", registry.Status.Phase) fmt.Printf("Message: %s\n", registry.Status.Message) - fmt.Printf("Server Count: %d\n", registry.Status.ServerCount) - fmt.Printf("Deployed Server Count: %d\n", registry.Status.DeployedServerCount) - if registry.Status.LastSyncTime != nil { - fmt.Printf("Last Sync Time: %s\n", registry.Status.LastSyncTime.Format(time.RFC3339)) + fmt.Printf("Server Count: %d\n", registry.Status.SyncStatus.ServerCount) + if registry.Status.SyncStatus.LastSyncTime != nil { + fmt.Printf("Last Sync Time: %s\n", registry.Status.SyncStatus.LastSyncTime.Format(time.RFC3339)) } - fmt.Printf("Last Sync Hash: %s\n", registry.Status.LastSyncHash) - fmt.Printf("Sync Attempts: %d\n", registry.Status.SyncAttempts) + fmt.Printf("Last Sync Hash: %s\n", registry.Status.SyncStatus.LastSyncHash) + fmt.Printf("Sync Attempts: %d\n", registry.Status.SyncStatus.AttemptCount) if len(registry.Status.Conditions) > 0 { fmt.Printf("Conditions:\n") From 148acff208a6c9477508151d5ae6033ef18d772f Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Thu, 25 Sep 2025 12:05:49 +0200 Subject: [PATCH 6/7] Enhance e2e test setup by adding support for kubebuilder assets - Introduced environment variable handling for KUBEBUILDER_ASSETS - Added warning for missing kubebuilder assets to improve test reliability - Updated test environment configuration to include BinaryAssetsDirectory This change aims to streamline the e2e testing process and provide clearer feedback on asset availability. Signed-off-by: Daniele Martinoli --- test/e2e/operator/suite_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/e2e/operator/suite_test.go b/test/e2e/operator/suite_test.go index 3640eb900..563a30d4c 100644 --- a/test/e2e/operator/suite_test.go +++ b/test/e2e/operator/suite_test.go @@ -54,12 +54,23 @@ var _ = BeforeSuite(func() { // Check if we should use an existing cluster (for CI/CD) useExistingCluster := os.Getenv("USE_EXISTING_CLUSTER") == "true" + // // Get kubebuilder assets path + kubebuilderAssets := os.Getenv("KUBEBUILDER_ASSETS") + + if !useExistingCluster { + By(fmt.Sprintf("using kubebuilder assets from: %s", kubebuilderAssets)) + if kubebuilderAssets == "" { + By("WARNING: no kubebuilder assets found, test may fail") + } + } + testEnv = &envtest.Environment{ UseExistingCluster: &useExistingCluster, CRDDirectoryPaths: []string{ filepath.Join("..", "..", "..", "deploy", "charts", "operator-crds", "crds"), }, ErrorIfCRDPathMissing: true, + BinaryAssetsDirectory: kubebuilderAssets, } cfg, err = testEnv.Start() From 12ec9f6d6b696d4b7e28fbbfae1287138775c897 Mon Sep 17 00:00:00 2001 From: Daniele Martinoli Date: Fri, 26 Sep 2025 17:19:16 +0200 Subject: [PATCH 7/7] - reviewed controller logic to avoid reconciliaton loops - initial draft of e2e tests Signed-off-by: Daniele Martinoli --- .codespellrc | 2 +- .../api/v1alpha1/mcpregistry_types.go | 43 +++ .../controllers/mcpregistry_controller.go | 178 ++++++------ .../pkg/mcpregistrystatus/collector.go | 129 +++++++-- .../pkg/mcpregistrystatus/collector_test.go | 196 +++++++++++-- .../pkg/mcpregistrystatus/deriver.go | 55 ++++ .../pkg/mcpregistrystatus/deriver_test.go | 266 ++++++++++++++++++ .../mcpregistrystatus/mocks/mock_collector.go | 216 +++++++++++--- .../pkg/mcpregistrystatus/types.go | 66 ++++- .../pkg/mcpregistrystatus/types_test.go | 168 +++++++++++ cmd/thv-operator/pkg/registryapi/manager.go | 90 ++---- .../pkg/registryapi/manager_test.go | 90 ------ .../pkg/registryapi/mocks/mock_manager.go | 10 +- cmd/thv-operator/pkg/registryapi/types.go | 2 +- cmd/thv-operator/pkg/sync/manager.go | 111 +++----- cmd/thv-operator/pkg/sync/manager_test.go | 107 +------ test/e2e/operator/k8s_helpers.go | 2 +- test/e2e/operator/registry_helpers.go | 35 +++ test/e2e/operator/registry_lifecycle_test.go | 104 +++---- test/e2e/operator/status_helpers.go | 12 +- 20 files changed, 1307 insertions(+), 575 deletions(-) create mode 100644 cmd/thv-operator/pkg/mcpregistrystatus/deriver.go create mode 100644 cmd/thv-operator/pkg/mcpregistrystatus/deriver_test.go create mode 100644 cmd/thv-operator/pkg/mcpregistrystatus/types_test.go diff --git a/.codespellrc b/.codespellrc index 0e785ac5c..e83793750 100644 --- a/.codespellrc +++ b/.codespellrc @@ -1,3 +1,3 @@ [codespell] -ignore-words-list = NotIn,notin,AfterAll,ND,aks +ignore-words-list = NotIn,notin,AfterAll,ND,aks,deriver skip = *.svg,*.mod,*.sum diff --git a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go index 5147b7e70..d9ceb1094 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go @@ -1,10 +1,13 @@ package v1alpha1 import ( + "context" "fmt" + "reflect" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/log" ) const ( @@ -425,6 +428,46 @@ func (r *MCPRegistry) DeriveOverallPhase() MCPRegistryPhase { return MCPRegistryPhasePending } +// IsEqualTo checks if the current status is equal to the new status +// This is used to avoid unnecessary status updates +func (r *MCPRegistryStatus) IsEqualTo(ctx context.Context, newStatus MCPRegistryStatus) bool { + // Do not use DeepEqual but checks only the fields that are erlevant for status changes + // This is used to avoid unnecessary status updates + ctxLogger := log.FromContext(ctx) + + if r.Phase != newStatus.Phase { + ctxLogger.V(1).Info("Phase difference", "current", r.Phase, "updated", newStatus.Phase) + return false + } + if r.Message != newStatus.Message { + ctxLogger.V(1).Info("Message difference", "current", r.Message, "updated", newStatus.Message) + return false + } + if r.SyncStatus != nil && newStatus.SyncStatus != nil { + if r.SyncStatus.Phase != newStatus.SyncStatus.Phase { + ctxLogger.V(1).Info("SyncStatus.Phase difference", "current", r.SyncStatus.Phase, "updated", newStatus.SyncStatus.Phase) + return false + } + } + if r.APIStatus != nil && newStatus.APIStatus != nil { + if r.APIStatus.Phase != newStatus.APIStatus.Phase { + ctxLogger.V(1).Info("APIStatus.Phase difference", "current", r.APIStatus.Phase, "updated", newStatus.APIStatus.Phase) + return false + } + } + + if !reflect.DeepEqual(r.StorageRef, newStatus.StorageRef) { + ctxLogger.V(1).Info("StorageRef difference", "current", r.StorageRef, "updated", newStatus.StorageRef) + return false + } + if !reflect.DeepEqual(r.Conditions, newStatus.Conditions) { + ctxLogger.V(1).Info("Conditions difference", "current", r.Conditions, "updated", newStatus.Conditions) + return false + } + + return true +} + func init() { SchemeBuilder.Register(&MCPRegistry{}, &MCPRegistryList{}) } diff --git a/cmd/thv-operator/controllers/mcpregistry_controller.go b/cmd/thv-operator/controllers/mcpregistry_controller.go index d72e66cb5..df89b41b5 100644 --- a/cmd/thv-operator/controllers/mcpregistry_controller.go +++ b/cmd/thv-operator/controllers/mcpregistry_controller.go @@ -7,7 +7,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -62,7 +62,7 @@ func NewMCPRegistryReconciler(k8sClient client.Client, scheme *runtime.Scheme) * // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries/finalizers,verbs=update;delete +// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpregistries/finalizers,verbs=update // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // @@ -81,7 +81,7 @@ func (r *MCPRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) mcpRegistry := &mcpv1alpha1.MCPRegistry{} err := r.Get(ctx, req.NamespacedName, mcpRegistry) if err != nil { - if errors.IsNotFound(err) { + if kerrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. // Return and don't requeue ctxLogger.Info("MCPRegistry resource not found. Ignoring since object must be deleted") @@ -111,25 +111,21 @@ func (r *MCPRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) if mcpRegistry.GetDeletionTimestamp() != nil { // The object is being deleted if controllerutil.ContainsFinalizer(mcpRegistry, "mcpregistry.toolhive.stacklok.dev/finalizer") { - // Run finalization logic only if not already terminating to avoid redundant work - if mcpRegistry.Status.Phase != mcpv1alpha1.MCPRegistryPhaseTerminating { - // Run finalization logic. If the finalization logic fails, - // don't remove the finalizer so that we can retry during the next reconciliation. - if err := r.finalizeMCPRegistry(ctx, mcpRegistry); err != nil { - ctxLogger.Error(err, "Reconciliation completed with error while finalizing MCPRegistry", - "MCPRegistry.Name", mcpRegistry.Name) - return ctrl.Result{}, err - } - // Remove the finalizer. Once all finalizers have been removed, the object will be deleted. - original := mcpRegistry.DeepCopy() - controllerutil.RemoveFinalizer(mcpRegistry, "mcpregistry.toolhive.stacklok.dev/finalizer") - patch := client.MergeFrom(original) - err := r.Patch(ctx, mcpRegistry, patch) - if err != nil { - ctxLogger.Error(err, "Reconciliation completed with error while removing finalizer", - "MCPRegistry.Name", mcpRegistry.Name) - return ctrl.Result{}, err - } + // Run finalization logic. If the finalization logic fails, + // don't remove the finalizer so that we can retry during the next reconciliation. + if err := r.finalizeMCPRegistry(ctx, mcpRegistry); err != nil { + ctxLogger.Error(err, "Reconciliation completed with error while finalizing MCPRegistry", + "MCPRegistry.Name", mcpRegistry.Name) + return ctrl.Result{}, err + } + + // Remove the finalizer. Once all finalizers have been removed, the object will be deleted. + controllerutil.RemoveFinalizer(mcpRegistry, "mcpregistry.toolhive.stacklok.dev/finalizer") + err := r.Update(ctx, mcpRegistry) + if err != nil { + ctxLogger.Error(err, "Reconciliation completed with error while removing finalizer", + "MCPRegistry.Name", mcpRegistry.Name) + return ctrl.Result{}, err } } ctxLogger.Info("Reconciliation of deleted MCPRegistry completed successfully", @@ -152,17 +148,38 @@ func (r *MCPRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } - // 3. Create status collector for batched updates - statusCollector := mcpregistrystatus.NewCollector(mcpRegistry) + // 3. Create status manager for batched updates with separation of concerns + statusManager := mcpregistrystatus.NewStatusManager(mcpRegistry) + statusDeriver := mcpregistrystatus.NewDefaultStatusDeriver() // 4. Reconcile sync operation - result, syncErr := r.reconcileSync(ctx, mcpRegistry, statusCollector) + result, syncErr := r.reconcileSync(ctx, mcpRegistry, statusManager) // 5. Reconcile API service (deployment and service, independent of sync status) if syncErr == nil { - if apiErr := r.registryAPIManager.ReconcileAPIService(ctx, mcpRegistry, statusCollector); apiErr != nil { + if apiErr := r.registryAPIManager.ReconcileAPIService(ctx, mcpRegistry); apiErr != nil { ctxLogger.Error(apiErr, "Failed to reconcile API service") + // Set API status with detailed error message from structured error + statusManager.API().SetAPIStatus(mcpv1alpha1.APIPhaseError, apiErr.Message, "") + statusManager.API().SetAPIReadyCondition(apiErr.ConditionReason, apiErr.Message, metav1.ConditionFalse) err = apiErr + } else { + // API reconciliation successful - check readiness and set appropriate status + isReady := r.registryAPIManager.IsAPIReady(ctx, mcpRegistry) + if isReady { + // TODO: Get actual service endpoint - for now, construct it + endpoint := fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", + mcpRegistry.GetAPIResourceName(), mcpRegistry.Namespace) + statusManager.API().SetAPIStatus(mcpv1alpha1.APIPhaseReady, + "Registry API is ready and serving requests", endpoint) + statusManager.API().SetAPIReadyCondition("APIReady", + "Registry API is ready and serving requests", metav1.ConditionTrue) + } else { + statusManager.API().SetAPIStatus(mcpv1alpha1.APIPhaseDeploying, + "Registry API deployment is not ready yet", "") + statusManager.API().SetAPIReadyCondition("APINotReady", + "Registry API deployment is not ready yet", metav1.ConditionFalse) + } } } @@ -175,10 +192,10 @@ func (r *MCPRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) } // 7. Derive overall phase and message from sync and API status - r.deriveOverallStatus(ctx, mcpRegistry, statusCollector) + r.deriveOverallStatus(ctx, mcpRegistry, statusManager, statusDeriver) // 8. Apply all status changes in a single batch update - if statusUpdateErr := statusCollector.Apply(ctx, r.Client); statusUpdateErr != nil { + if statusUpdateErr := statusManager.Apply(ctx, r.Client); statusUpdateErr != nil { ctxLogger.Error(statusUpdateErr, "Failed to apply batched status update") // Return the status update error only if there was no main reconciliation error if syncErr == nil { @@ -211,6 +228,10 @@ func (r *MCPRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) "requeueAfter", result.RequeueAfter) } + if result.RequeueAfter > 0 { + ctxLogger.Info("Resetting error to nil because of requeue") + err = nil + } return result, err } @@ -231,7 +252,7 @@ func (*MCPRegistryReconciler) preserveExistingSyncData(mcpRegistry *mcpv1alpha1. // //nolint:gocyclo // Complex reconciliation logic requires multiple conditions func (r *MCPRegistryReconciler) reconcileSync( - ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, statusCollector mcpregistrystatus.Collector, + ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, statusManager mcpregistrystatus.StatusManager, ) (ctrl.Result, error) { ctxLogger := log.FromContext(ctx) @@ -259,7 +280,7 @@ func (r *MCPRegistryReconciler) reconcileSync( if currentSyncPhase != mcpv1alpha1.SyncPhaseIdle || currentMessage != "No sync required" { // Preserve existing sync data when no sync is needed lastSyncTime, lastSyncHash, serverCount := r.preserveExistingSyncData(mcpRegistry) - statusCollector.SetSyncStatus(mcpv1alpha1.SyncPhaseIdle, "No sync required", 0, lastSyncTime, lastSyncHash, serverCount) + statusManager.Sync().SetSyncStatus(mcpv1alpha1.SyncPhaseIdle, "No sync required", 0, lastSyncTime, lastSyncHash, serverCount) } // Schedule next reconciliation if we have a sync policy @@ -277,7 +298,7 @@ func (r *MCPRegistryReconciler) reconcileSync( if syncReason == sync.ReasonManualNoChanges { // Preserve existing sync data for manual sync with no changes lastSyncTime, lastSyncHash, serverCount := r.preserveExistingSyncData(mcpRegistry) - statusCollector.SetSyncStatus( + statusManager.Sync().SetSyncStatus( mcpv1alpha1.SyncPhaseComplete, "Manual sync completed (no data changes)", 0, lastSyncTime, lastSyncHash, serverCount) return r.syncManager.UpdateManualSyncTriggerOnly(ctx, mcpRegistry) @@ -285,32 +306,43 @@ func (r *MCPRegistryReconciler) reconcileSync( // Set sync status to syncing before starting the operation // Clear sync data when starting sync operation - statusCollector.SetSyncStatus( + statusManager.Sync().SetSyncStatus( mcpv1alpha1.SyncPhaseSyncing, "Synchronizing registry data", getCurrentAttemptCount(mcpRegistry)+1, nil, "", 0) // Perform the sync - the sync manager will handle core registry field updates - result, syncResult, err := r.syncManager.PerformSync(ctx, mcpRegistry) + result, syncResult, syncErr := r.syncManager.PerformSync(ctx, mcpRegistry) - if err != nil { + if syncErr != nil { // Sync failed - set sync status to failed - ctxLogger.Error(err, "Sync failed, scheduling retry") + ctxLogger.Info("Sync failed, scheduling retry", "error", syncErr.Error()) // Preserve existing sync data when sync fails lastSyncTime, lastSyncHash, serverCount := r.preserveExistingSyncData(mcpRegistry) - statusCollector.SetSyncStatus(mcpv1alpha1.SyncPhaseFailed, - fmt.Sprintf("Sync failed: %v", err), getCurrentAttemptCount(mcpRegistry)+1, lastSyncTime, lastSyncHash, serverCount) + + // Set sync status with detailed error message from SyncError + statusManager.Sync().SetSyncStatus(mcpv1alpha1.SyncPhaseFailed, + syncErr.Message, getCurrentAttemptCount(mcpRegistry)+1, lastSyncTime, lastSyncHash, serverCount) + // Set the appropriate condition based on the error type + statusManager.Sync().SetSyncCondition(metav1.Condition{ + Type: syncErr.ConditionType, + Status: metav1.ConditionFalse, + Reason: syncErr.ConditionReason, + Message: syncErr.Message, + LastTransitionTime: metav1.Now(), + }) + // Use a shorter retry interval instead of the full sync interval retryAfter := time.Minute * 5 // Default retry interval if result.RequeueAfter > 0 { // If PerformSync already set a retry interval, use it retryAfter = result.RequeueAfter } - return ctrl.Result{RequeueAfter: retryAfter}, err + return ctrl.Result{RequeueAfter: retryAfter}, syncErr } // Sync successful - set sync status to complete using data from sync result now := metav1.Now() - statusCollector.SetSyncStatus(mcpv1alpha1.SyncPhaseComplete, "Registry data synchronized successfully", 0, + statusManager.Sync().SetSyncStatus(mcpv1alpha1.SyncPhaseComplete, "Registry data synchronized successfully", 0, &now, syncResult.Hash, syncResult.ServerCount) ctxLogger.Info("Registry data sync completed successfully") @@ -330,7 +362,7 @@ func (r *MCPRegistryReconciler) reconcileSync( ctxLogger.Info("Sync successful, no automatic sync policy configured") } - return result, err + return result, nil } // finalizeMCPRegistry performs the finalizer logic for the MCPRegistry @@ -360,61 +392,25 @@ func (r *MCPRegistryReconciler) finalizeMCPRegistry(ctx context.Context, registr } // deriveOverallStatus determines the overall MCPRegistry phase and message based on sync and API status -func (r *MCPRegistryReconciler) deriveOverallStatus( - ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, statusCollector mcpregistrystatus.Collector) { +func (*MCPRegistryReconciler) deriveOverallStatus( + ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, + statusManager mcpregistrystatus.StatusManager, statusDeriver mcpregistrystatus.StatusDeriver) { ctxLogger := log.FromContext(ctx) - // Create a temporary copy with current collected status to derive phase - tempRegistry := mcpRegistry.DeepCopy() - - // Apply the collected status changes to temp registry for phase calculation - // Note: This is a simulation - we can't actually access the collected values directly - // Instead, we'll use the DeriveOverallPhase method which works with current status - // The controller will need to be smart about when sync/API status get updated - - // For now, let's derive phase based on current MCPRegistry status since - // the status collector changes haven't been applied yet - derivedPhase := tempRegistry.DeriveOverallPhase() - derivedMessage := r.deriveMessage(derivedPhase, tempRegistry) + // Use the StatusDeriver to determine the overall phase and message + // based on current sync and API statuses + derivedPhase, derivedMessage := statusDeriver.DeriveOverallStatus( + statusManager.Sync().Status(), + statusManager.API().Status(), + ) // Only update phase and message if they've changed - if mcpRegistry.Status.Phase != derivedPhase { - statusCollector.SetPhase(derivedPhase) - ctxLogger.Info("Updated overall phase", "oldPhase", mcpRegistry.Status.Phase, "newPhase", derivedPhase) - } - - if mcpRegistry.Status.Message != derivedMessage { - statusCollector.SetMessage(derivedMessage) - ctxLogger.Info("Updated overall message", "message", derivedMessage) - } -} - -// deriveMessage creates an appropriate message based on the overall phase and registry state -func (*MCPRegistryReconciler) deriveMessage(phase mcpv1alpha1.MCPRegistryPhase, mcpRegistry *mcpv1alpha1.MCPRegistry) string { - switch phase { - case mcpv1alpha1.MCPRegistryPhasePending: - if mcpRegistry.Status.SyncStatus != nil && mcpRegistry.Status.SyncStatus.Phase == mcpv1alpha1.SyncPhaseComplete { - return "Registry data synced, API deployment in progress" - } - return "Registry initialization in progress" - case mcpv1alpha1.MCPRegistryPhaseReady: - return "Registry is ready and API is serving requests" - case mcpv1alpha1.MCPRegistryPhaseFailed: - // Return more specific error message if available - if mcpRegistry.Status.SyncStatus != nil && mcpRegistry.Status.SyncStatus.Phase == mcpv1alpha1.SyncPhaseFailed { - return fmt.Sprintf("Sync failed: %s", mcpRegistry.Status.SyncStatus.Message) - } - if mcpRegistry.Status.APIStatus != nil && mcpRegistry.Status.APIStatus.Phase == mcpv1alpha1.APIPhaseError { - return fmt.Sprintf("API deployment failed: %s", mcpRegistry.Status.APIStatus.Message) - } - return "Registry operation failed" - case mcpv1alpha1.MCPRegistryPhaseSyncing: - return "Registry data synchronization in progress" - case mcpv1alpha1.MCPRegistryPhaseTerminating: - return "Registry is being terminated" - default: - return "Registry status unknown" - } + statusManager.SetOverallStatus(derivedPhase, derivedMessage) + ctxLogger.Info("Updated overall status", + "oldPhase", mcpRegistry.Status.Phase, + "newPhase", derivedPhase, + "oldMessage", mcpRegistry.Status.Message, + "newMessage", derivedMessage) } // SetupWithManager sets up the controller with the Manager. diff --git a/cmd/thv-operator/pkg/mcpregistrystatus/collector.go b/cmd/thv-operator/pkg/mcpregistrystatus/collector.go index 7bd607619..03c190ff6 100644 --- a/cmd/thv-operator/pkg/mcpregistrystatus/collector.go +++ b/cmd/thv-operator/pkg/mcpregistrystatus/collector.go @@ -15,7 +15,7 @@ import ( // StatusCollector collects status changes during reconciliation // and applies them in a single batch update at the end. -// It implements the Collector interface. +// It implements the StatusManager interface. type StatusCollector struct { mcpRegistry *mcpv1alpha1.MCPRegistry hasChanges bool @@ -24,14 +24,36 @@ type StatusCollector struct { syncStatus *mcpv1alpha1.SyncStatus apiStatus *mcpv1alpha1.APIStatus conditions map[string]metav1.Condition + + // Component collectors + syncCollector *syncStatusCollector + apiCollector *apiStatusCollector +} + +// syncStatusCollector implements SyncStatusCollector +type syncStatusCollector struct { + parent *StatusCollector +} + +// apiStatusCollector implements APIStatusCollector +type apiStatusCollector struct { + parent *StatusCollector } -// NewCollector creates a new status update collector for the given MCPRegistry resource. -func NewCollector(mcpRegistry *mcpv1alpha1.MCPRegistry) Collector { - return &StatusCollector{ +// NewStatusManager creates a new StatusManager for the given MCPRegistry resource. +func NewStatusManager(mcpRegistry *mcpv1alpha1.MCPRegistry) StatusManager { + return newStatusCollector(mcpRegistry) +} + +// newStatusCollector creates the internal StatusCollector implementation +func newStatusCollector(mcpRegistry *mcpv1alpha1.MCPRegistry) *StatusCollector { + collector := &StatusCollector{ mcpRegistry: mcpRegistry, conditions: make(map[string]metav1.Condition), } + collector.syncCollector = &syncStatusCollector{parent: collector} + collector.apiCollector = &apiStatusCollector{parent: collector} + return collector } // SetPhase sets the phase to be updated. @@ -46,10 +68,10 @@ func (s *StatusCollector) SetMessage(message string) { s.hasChanges = true } -// SetAPIReadyCondition adds or updates the API ready condition. -func (s *StatusCollector) SetAPIReadyCondition(reason, message string, status metav1.ConditionStatus) { - s.conditions[mcpv1alpha1.ConditionAPIReady] = metav1.Condition{ - Type: mcpv1alpha1.ConditionAPIReady, +// SetCondition sets a general condition with the specified type, reason, message, and status +func (s *StatusCollector) SetCondition(conditionType, reason, message string, status metav1.ConditionStatus) { + s.conditions[conditionType] = metav1.Condition{ + Type: conditionType, Status: status, Reason: reason, Message: message, @@ -57,6 +79,11 @@ func (s *StatusCollector) SetAPIReadyCondition(reason, message string, status me s.hasChanges = true } +// SetAPIReadyCondition adds or updates the API ready condition. +func (s *StatusCollector) SetAPIReadyCondition(reason, message string, status metav1.ConditionStatus) { + s.SetCondition(mcpv1alpha1.ConditionAPIReady, reason, message, status) +} + // SetSyncStatus sets the detailed sync status. func (s *StatusCollector) SetSyncStatus( phase mcpv1alpha1.SyncPhase, message string, attemptCount int, @@ -110,41 +137,99 @@ func (s *StatusCollector) Apply(ctx context.Context, k8sClient client.Client) er return fmt.Errorf("failed to fetch latest MCPRegistry version: %w", err) } + updatedStatus := latestRegistry.Status.DeepCopy() // Apply phase change if s.phase != nil { - latestRegistry.Status.Phase = *s.phase + updatedStatus.Phase = *s.phase } // Apply message change if s.message != nil { - latestRegistry.Status.Message = *s.message + updatedStatus.Message = *s.message } // Apply sync status change if s.syncStatus != nil { - latestRegistry.Status.SyncStatus = s.syncStatus + updatedStatus.SyncStatus = s.syncStatus } // Apply API status change if s.apiStatus != nil { - latestRegistry.Status.APIStatus = s.apiStatus + updatedStatus.APIStatus = s.apiStatus } // Apply condition changes for _, condition := range s.conditions { - meta.SetStatusCondition(&latestRegistry.Status.Conditions, condition) + meta.SetStatusCondition(&updatedStatus.Conditions, condition) } - // Single status update using the latest version - if err := k8sClient.Status().Update(ctx, latestRegistry); err != nil { - ctxLogger.Error(err, "Failed to apply batched status update") - return fmt.Errorf("failed to apply batched status update: %w", err) + if !latestRegistry.Status.IsEqualTo(ctx, *updatedStatus) { + latestRegistry.Status = *updatedStatus + // Single status update using the latest version + if err := k8sClient.Status().Update(ctx, latestRegistry); err != nil { + ctxLogger.Error(err, "Failed to apply batched status update") + return fmt.Errorf("failed to apply batched status update: %w", err) + } + + ctxLogger.V(1).Info("Applied batched status update", + "phase", s.phase, + "message", s.message, + "conditionsCount", len(s.conditions)) + } else { + ctxLogger.V(1).Info("No changes to apply to MCPRegistry status") } + return nil +} - ctxLogger.V(1).Info("Applied batched status update", - "phase", s.phase, - "message", s.message, - "conditionsCount", len(s.conditions)) +// StatusManager interface methods - return nil +// Sync returns the sync status collector +func (s *StatusCollector) Sync() SyncStatusCollector { + return s.syncCollector +} + +// API returns the API status collector +func (s *StatusCollector) API() APIStatusCollector { + return s.apiCollector +} + +// SetOverallStatus sets the overall phase and message explicitly (for special cases) +func (s *StatusCollector) SetOverallStatus(phase mcpv1alpha1.MCPRegistryPhase, message string) { + s.SetPhase(phase) + s.SetMessage(message) +} + +// SyncStatusCollector implementation + +// Status returns the current status +func (sc *syncStatusCollector) Status() *mcpv1alpha1.SyncStatus { + return sc.parent.syncStatus +} + +// SetSyncCondition sets a sync-related condition +func (sc *syncStatusCollector) SetSyncCondition(condition metav1.Condition) { + sc.parent.conditions[condition.Type] = condition + sc.parent.hasChanges = true +} + +// SetSyncStatus delegates to the parent's SetSyncStatus method +func (sc *syncStatusCollector) SetSyncStatus(phase mcpv1alpha1.SyncPhase, message string, attemptCount int, + lastSyncTime *metav1.Time, lastSyncHash string, serverCount int) { + sc.parent.SetSyncStatus(phase, message, attemptCount, lastSyncTime, lastSyncHash, serverCount) +} + +// APIStatusCollector implementation +// Status returns the current status +func (ac *apiStatusCollector) Status() *mcpv1alpha1.APIStatus { + return ac.parent.apiStatus +} + +// SetAPIStatus delegates to the parent's SetAPIStatus method +func (ac *apiStatusCollector) SetAPIStatus(phase mcpv1alpha1.APIPhase, message string, endpoint string) { + ac.parent.SetAPIStatus(phase, message, endpoint) +} + +// SetAPIReadyCondition delegates to the parent's SetAPIReadyCondition method +func (ac *apiStatusCollector) SetAPIReadyCondition(reason, message string, status metav1.ConditionStatus) { + ac.parent.SetAPIReadyCondition(reason, message, status) } diff --git a/cmd/thv-operator/pkg/mcpregistrystatus/collector_test.go b/cmd/thv-operator/pkg/mcpregistrystatus/collector_test.go index 872c9dc80..d3a7d431c 100644 --- a/cmd/thv-operator/pkg/mcpregistrystatus/collector_test.go +++ b/cmd/thv-operator/pkg/mcpregistrystatus/collector_test.go @@ -13,7 +13,7 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" ) -func TestNewCollector(t *testing.T) { +func TestNewStatusManager(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{ @@ -23,10 +23,10 @@ func TestNewCollector(t *testing.T) { }, } - collector := NewCollector(registry) + statusManager := NewStatusManager(registry) - assert.NotNil(t, collector) - sc := collector.(*StatusCollector) + assert.NotNil(t, statusManager) + sc := statusManager.(*StatusCollector) assert.Equal(t, registry, sc.mcpRegistry) assert.False(t, sc.hasChanges) assert.Empty(t, sc.conditions) @@ -58,7 +58,7 @@ func TestStatusCollector_SetPhase(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) collector.SetPhase(tt.phase) @@ -73,7 +73,7 @@ func TestStatusCollector_SetMessage(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) testMessage := "Test message" collector.SetMessage(testMessage) @@ -114,7 +114,7 @@ func TestStatusCollector_SetAPIReadyCondition(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) collector.SetAPIReadyCondition(tt.reason, tt.message, tt.status) @@ -176,7 +176,7 @@ func TestStatusCollector_SetSyncStatus(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) collector.SetSyncStatus(tt.phase, tt.message, tt.attemptCount, tt.lastSyncTime, tt.lastSyncHash, tt.serverCount) @@ -230,7 +230,7 @@ func TestStatusCollector_SetAPIStatus(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) collector.SetAPIStatus(tt.phase, tt.message, tt.endpoint) @@ -257,7 +257,7 @@ func TestStatusCollector_SetAPIStatus_ReadySince(t *testing.T) { }, }, } - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) collector.SetAPIStatus(mcpv1alpha1.APIPhaseReady, "API is ready", "http://test.com") @@ -275,7 +275,7 @@ func TestStatusCollector_SetAPIStatus_ReadySince(t *testing.T) { }, }, } - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) collector.SetAPIStatus(mcpv1alpha1.APIPhaseReady, "API is ready", "http://test.com") @@ -285,7 +285,7 @@ func TestStatusCollector_SetAPIStatus_ReadySince(t *testing.T) { t.Run("clears ReadySince when not ready", func(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) collector.SetAPIStatus(mcpv1alpha1.APIPhaseError, "API failed", "") @@ -320,7 +320,7 @@ func TestStatusCollector_Apply(t *testing.T) { t.Run("applies no changes when hasChanges is false", func(t *testing.T) { t.Parallel() - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) err := collector.Apply(ctx, k8sClient) @@ -329,7 +329,7 @@ func TestStatusCollector_Apply(t *testing.T) { t.Run("verifies hasChanges behavior", func(t *testing.T) { t.Parallel() - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) // Initially no changes assert.False(t, collector.hasChanges) @@ -341,7 +341,7 @@ func TestStatusCollector_Apply(t *testing.T) { t.Run("verifies status field collection", func(t *testing.T) { t.Parallel() - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) // Set various status fields collector.SetPhase(mcpv1alpha1.MCPRegistryPhaseReady) @@ -370,7 +370,7 @@ func TestStatusCollector_NoChanges(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) // Initially no changes assert.False(t, collector.hasChanges) @@ -384,7 +384,7 @@ func TestStatusCollector_MultipleConditions(t *testing.T) { t.Parallel() registry := &mcpv1alpha1.MCPRegistry{} - collector := NewCollector(registry).(*StatusCollector) + collector := NewStatusManager(registry).(*StatusCollector) // Add condition collector.SetAPIReadyCondition("APIReady", "API is ready", metav1.ConditionTrue) @@ -393,3 +393,165 @@ func TestStatusCollector_MultipleConditions(t *testing.T) { assert.Len(t, collector.conditions, 1) assert.Contains(t, collector.conditions, mcpv1alpha1.ConditionAPIReady) } + +func TestStatusCollector_ApplyErrors(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + // Create scheme + scheme := runtime.NewScheme() + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + + t.Run("error fetching latest registry", func(t *testing.T) { + t.Parallel() + + // Create client that will fail on Get + k8sClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + // Create collector with registry that doesn't exist in client + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nonexistent-registry", + Namespace: "default", + }, + } + + collector := newStatusCollector(registry) + collector.SetPhase(mcpv1alpha1.MCPRegistryPhaseReady) // Make some changes + + err := collector.Apply(ctx, k8sClient) + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to fetch latest MCPRegistry version") + }) + +} + +func TestStatusCollector_InterfaceMethods(t *testing.T) { + t.Parallel() + + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "default", + }, + } + + collector := newStatusCollector(registry) + + t.Run("Sync method returns sync collector", func(t *testing.T) { + t.Parallel() + syncCollector := collector.Sync() + assert.NotNil(t, syncCollector) + assert.IsType(t, &syncStatusCollector{}, syncCollector) + }) + + t.Run("API method returns API collector", func(t *testing.T) { + t.Parallel() + apiCollector := collector.API() + assert.NotNil(t, apiCollector) + assert.IsType(t, &apiStatusCollector{}, apiCollector) + }) + + t.Run("SetOverallStatus delegates correctly", func(t *testing.T) { + t.Parallel() + collector.SetOverallStatus(mcpv1alpha1.MCPRegistryPhaseReady, "Test message") + + assert.True(t, collector.hasChanges) + assert.Equal(t, mcpv1alpha1.MCPRegistryPhaseReady, *collector.phase) + assert.Equal(t, "Test message", *collector.message) + }) +} + +func TestSyncStatusCollector_Methods(t *testing.T) { + t.Parallel() + + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "default", + }, + } + + collector := newStatusCollector(registry) + syncCollector := collector.Sync() + + t.Run("SetSyncCondition delegates correctly", func(t *testing.T) { + t.Parallel() + condition := metav1.Condition{ + Type: "TestCondition", + Status: metav1.ConditionTrue, + Reason: "TestReason", + Message: "Test message", + } + + syncCollector.SetSyncCondition(condition) + + assert.True(t, collector.hasChanges) + assert.Contains(t, collector.conditions, "TestCondition") + assert.Equal(t, condition, collector.conditions["TestCondition"]) + }) + + t.Run("SetSyncStatus delegates correctly", func(t *testing.T) { + t.Parallel() + now := metav1.Now() + syncCollector.SetSyncStatus( + mcpv1alpha1.SyncPhaseComplete, + "Sync completed", + 1, + &now, + "hash123", + 5, + ) + + assert.True(t, collector.hasChanges) + assert.NotNil(t, collector.syncStatus) + assert.Equal(t, mcpv1alpha1.SyncPhaseComplete, collector.syncStatus.Phase) + assert.Equal(t, "Sync completed", collector.syncStatus.Message) + assert.Equal(t, 1, collector.syncStatus.AttemptCount) + assert.Equal(t, &now, collector.syncStatus.LastSyncTime) + assert.Equal(t, "hash123", collector.syncStatus.LastSyncHash) + assert.Equal(t, 5, collector.syncStatus.ServerCount) + }) +} + +func TestAPIStatusCollector_Methods(t *testing.T) { + t.Parallel() + + registry := &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "default", + }, + } + + collector := newStatusCollector(registry) + apiCollector := collector.API() + + t.Run("SetAPIStatus delegates correctly", func(t *testing.T) { + t.Parallel() + apiCollector.SetAPIStatus( + mcpv1alpha1.APIPhaseReady, + "API is ready", + "http://example.com", + ) + + assert.True(t, collector.hasChanges) + assert.NotNil(t, collector.apiStatus) + assert.Equal(t, mcpv1alpha1.APIPhaseReady, collector.apiStatus.Phase) + assert.Equal(t, "API is ready", collector.apiStatus.Message) + assert.Equal(t, "http://example.com", collector.apiStatus.Endpoint) + }) + + t.Run("SetAPIReadyCondition delegates correctly", func(t *testing.T) { + t.Parallel() + apiCollector.SetAPIReadyCondition("APIReady", "API is ready", metav1.ConditionTrue) + + assert.True(t, collector.hasChanges) + assert.Contains(t, collector.conditions, mcpv1alpha1.ConditionAPIReady) + condition := collector.conditions[mcpv1alpha1.ConditionAPIReady] + assert.Equal(t, metav1.ConditionTrue, condition.Status) + assert.Equal(t, "APIReady", condition.Reason) + assert.Equal(t, "API is ready", condition.Message) + }) +} diff --git a/cmd/thv-operator/pkg/mcpregistrystatus/deriver.go b/cmd/thv-operator/pkg/mcpregistrystatus/deriver.go new file mode 100644 index 000000000..92b1aacee --- /dev/null +++ b/cmd/thv-operator/pkg/mcpregistrystatus/deriver.go @@ -0,0 +1,55 @@ +// Package mcpregistrystatus provides status management for MCPRegistry resources. +package mcpregistrystatus + +import ( + "fmt" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// DefaultStatusDeriver implements the StatusDeriver interface +type DefaultStatusDeriver struct{} + +// NewDefaultStatusDeriver creates a new DefaultStatusDeriver +func NewDefaultStatusDeriver() StatusDeriver { + return &DefaultStatusDeriver{} +} + +// DeriveOverallStatus derives the overall MCPRegistry phase and message from component statuses +func (*DefaultStatusDeriver) DeriveOverallStatus( + syncStatus *mcpv1alpha1.SyncStatus, apiStatus *mcpv1alpha1.APIStatus) (mcpv1alpha1.MCPRegistryPhase, string) { + // Handle sync failures first (highest priority) + if syncStatus != nil && syncStatus.Phase == mcpv1alpha1.SyncPhaseFailed { + return mcpv1alpha1.MCPRegistryPhaseFailed, fmt.Sprintf("Sync failed: %s", syncStatus.Message) + } + + // Handle API failures + if apiStatus != nil && apiStatus.Phase == mcpv1alpha1.APIPhaseError { + return mcpv1alpha1.MCPRegistryPhaseFailed, fmt.Sprintf("API deployment failed: %s", apiStatus.Message) + } + + // Handle sync in progress + if syncStatus != nil && syncStatus.Phase == mcpv1alpha1.SyncPhaseSyncing { + return mcpv1alpha1.MCPRegistryPhaseSyncing, "Registry data synchronization in progress" + } + + // Check if both sync and API are ready + syncReady := syncStatus != nil && + (syncStatus.Phase == mcpv1alpha1.SyncPhaseComplete || syncStatus.Phase == mcpv1alpha1.SyncPhaseIdle) + apiReady := apiStatus != nil && apiStatus.Phase == mcpv1alpha1.APIPhaseReady + + if syncReady && apiReady { + return mcpv1alpha1.MCPRegistryPhaseReady, "Registry is ready and API is serving requests" + } + + // If sync is complete but API is not ready yet + if syncReady { + if apiStatus != nil && apiStatus.Phase == mcpv1alpha1.APIPhaseDeploying { + return mcpv1alpha1.MCPRegistryPhasePending, "Registry data synced, API deployment in progress" + } + return mcpv1alpha1.MCPRegistryPhasePending, "Registry data synced, API deployment pending" + } + + // Default to pending for initial state or unknown combinations + return mcpv1alpha1.MCPRegistryPhasePending, "Registry initialization in progress" +} diff --git a/cmd/thv-operator/pkg/mcpregistrystatus/deriver_test.go b/cmd/thv-operator/pkg/mcpregistrystatus/deriver_test.go new file mode 100644 index 000000000..c27095409 --- /dev/null +++ b/cmd/thv-operator/pkg/mcpregistrystatus/deriver_test.go @@ -0,0 +1,266 @@ +package mcpregistrystatus + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +func TestNewDefaultStatusDeriver(t *testing.T) { + t.Parallel() + + deriver := NewDefaultStatusDeriver() + assert.NotNil(t, deriver) + assert.IsType(t, &DefaultStatusDeriver{}, deriver) +} + +func TestDeriveOverallStatus(t *testing.T) { + t.Parallel() + + deriver := &DefaultStatusDeriver{} + + tests := []struct { + name string + syncStatus *mcpv1alpha1.SyncStatus + apiStatus *mcpv1alpha1.APIStatus + expectedPhase mcpv1alpha1.MCPRegistryPhase + expectedMessage string + description string + }{ + { + name: "sync failed - highest priority", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseFailed, + Message: "source unreachable", + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseReady, + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhaseFailed, + expectedMessage: "Sync failed: source unreachable", + description: "Sync failure should take precedence over API ready state", + }, + { + name: "API error when sync is complete", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseComplete, + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseError, + Message: "deployment failed", + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhaseFailed, + expectedMessage: "API deployment failed: deployment failed", + description: "API error should result in failed phase", + }, + { + name: "sync in progress", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseSyncing, + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseDeploying, + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhaseSyncing, + expectedMessage: "Registry data synchronization in progress", + description: "Syncing phase should be shown when sync is in progress", + }, + { + name: "both sync and API ready", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseComplete, + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseReady, + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhaseReady, + expectedMessage: "Registry is ready and API is serving requests", + description: "Both components ready should result in ready phase", + }, + { + name: "sync idle and API ready", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseIdle, + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseReady, + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhaseReady, + expectedMessage: "Registry is ready and API is serving requests", + description: "Idle sync with ready API should result in ready phase", + }, + { + name: "sync complete, API deploying", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseComplete, + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseDeploying, + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, + expectedMessage: "Registry data synced, API deployment in progress", + description: "Complete sync with deploying API should be pending", + }, + { + name: "sync complete, API status missing", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseComplete, + }, + apiStatus: nil, + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, + expectedMessage: "Registry data synced, API deployment pending", + description: "Complete sync without API status should be pending", + }, + { + name: "sync idle, API status missing", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseIdle, + }, + apiStatus: nil, + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, + expectedMessage: "Registry data synced, API deployment pending", + description: "Idle sync without API status should be pending", + }, + { + name: "both statuses nil", + syncStatus: nil, + apiStatus: nil, + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, + expectedMessage: "Registry initialization in progress", + description: "No status information should default to pending", + }, + { + name: "sync nil, API ready", + syncStatus: nil, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseReady, + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, + expectedMessage: "Registry initialization in progress", + description: "Missing sync status should default to pending even with ready API", + }, + { + name: "sync complete, API unknown phase", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseComplete, + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: "UnknownPhase", + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, + expectedMessage: "Registry data synced, API deployment pending", + description: "Unknown API phase should be treated as not ready", + }, + { + name: "sync with unknown phase", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: "UnknownSyncPhase", + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseReady, + }, + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, + expectedMessage: "Registry initialization in progress", + description: "Unknown sync phase should default to pending", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + phase, message := deriver.DeriveOverallStatus(tt.syncStatus, tt.apiStatus) + + assert.Equal(t, tt.expectedPhase, phase, tt.description) + assert.Equal(t, tt.expectedMessage, message, tt.description) + }) + } +} + +func TestDeriveOverallStatus_PriorityOrdering(t *testing.T) { + t.Parallel() + + deriver := &DefaultStatusDeriver{} + + // Test that sync failures take precedence over API errors + syncStatus := &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseFailed, + Message: "sync failed", + } + apiStatus := &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseError, + Message: "api failed", + } + + phase, message := deriver.DeriveOverallStatus(syncStatus, apiStatus) + + assert.Equal(t, mcpv1alpha1.MCPRegistryPhaseFailed, phase) + assert.Contains(t, message, "Sync failed") + assert.NotContains(t, message, "API deployment failed") +} + +func TestDeriveOverallStatus_SyncingTakesPrecedence(t *testing.T) { + t.Parallel() + + deriver := &DefaultStatusDeriver{} + + // Test that syncing takes precedence over API ready state + syncStatus := &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseSyncing, + } + apiStatus := &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseReady, + } + + phase, message := deriver.DeriveOverallStatus(syncStatus, apiStatus) + + assert.Equal(t, mcpv1alpha1.MCPRegistryPhaseSyncing, phase) + assert.Equal(t, "Registry data synchronization in progress", message) +} + +func TestDeriveOverallStatus_EdgeCases(t *testing.T) { + t.Parallel() + + deriver := &DefaultStatusDeriver{} + + tests := []struct { + name string + syncStatus *mcpv1alpha1.SyncStatus + apiStatus *mcpv1alpha1.APIStatus + description string + }{ + { + name: "empty sync status with empty phase", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: "", + }, + apiStatus: &mcpv1alpha1.APIStatus{ + Phase: mcpv1alpha1.APIPhaseReady, + }, + description: "Empty sync phase should be handled gracefully", + }, + { + name: "sync status with whitespace message", + syncStatus: &mcpv1alpha1.SyncStatus{ + Phase: mcpv1alpha1.SyncPhaseFailed, + Message: " ", + }, + apiStatus: nil, + description: "Whitespace in error message should be preserved", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Should not panic and should return valid phase/message + phase, message := deriver.DeriveOverallStatus(tt.syncStatus, tt.apiStatus) + + assert.NotEmpty(t, phase, tt.description) + assert.NotEmpty(t, message, tt.description) + }) + } +} diff --git a/cmd/thv-operator/pkg/mcpregistrystatus/mocks/mock_collector.go b/cmd/thv-operator/pkg/mcpregistrystatus/mocks/mock_collector.go index 4e163e987..031248f88 100644 --- a/cmd/thv-operator/pkg/mcpregistrystatus/mocks/mock_collector.go +++ b/cmd/thv-operator/pkg/mcpregistrystatus/mocks/mock_collector.go @@ -3,7 +3,7 @@ // // Generated by this command: // -// mockgen -destination=mocks/mock_collector.go -package=mocks -source=types.go Collector +// mockgen -destination=mocks/mock_collector.go -package=mocks -source=types.go SyncStatusCollector,APIStatusCollector,StatusDeriver,StatusManager // // Package mocks is a generated GoMock package. @@ -14,105 +14,233 @@ import ( reflect "reflect" v1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + mcpregistrystatus "github.com/stacklok/toolhive/cmd/thv-operator/pkg/mcpregistrystatus" gomock "go.uber.org/mock/gomock" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" client "sigs.k8s.io/controller-runtime/pkg/client" ) -// MockCollector is a mock of Collector interface. -type MockCollector struct { +// MockSyncStatusCollector is a mock of SyncStatusCollector interface. +type MockSyncStatusCollector struct { ctrl *gomock.Controller - recorder *MockCollectorMockRecorder + recorder *MockSyncStatusCollectorMockRecorder isgomock struct{} } -// MockCollectorMockRecorder is the mock recorder for MockCollector. -type MockCollectorMockRecorder struct { - mock *MockCollector +// MockSyncStatusCollectorMockRecorder is the mock recorder for MockSyncStatusCollector. +type MockSyncStatusCollectorMockRecorder struct { + mock *MockSyncStatusCollector } -// NewMockCollector creates a new mock instance. -func NewMockCollector(ctrl *gomock.Controller) *MockCollector { - mock := &MockCollector{ctrl: ctrl} - mock.recorder = &MockCollectorMockRecorder{mock} +// NewMockSyncStatusCollector creates a new mock instance. +func NewMockSyncStatusCollector(ctrl *gomock.Controller) *MockSyncStatusCollector { + mock := &MockSyncStatusCollector{ctrl: ctrl} + mock.recorder = &MockSyncStatusCollectorMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockCollector) EXPECT() *MockCollectorMockRecorder { +func (m *MockSyncStatusCollector) EXPECT() *MockSyncStatusCollectorMockRecorder { return m.recorder } -// Apply mocks base method. -func (m *MockCollector) Apply(ctx context.Context, k8sClient client.Client) error { +// SetSyncCondition mocks base method. +func (m *MockSyncStatusCollector) SetSyncCondition(condition v1.Condition) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Apply", ctx, k8sClient) - ret0, _ := ret[0].(error) - return ret0 + m.ctrl.Call(m, "SetSyncCondition", condition) } -// Apply indicates an expected call of Apply. -func (mr *MockCollectorMockRecorder) Apply(ctx, k8sClient any) *gomock.Call { +// SetSyncCondition indicates an expected call of SetSyncCondition. +func (mr *MockSyncStatusCollectorMockRecorder) SetSyncCondition(condition any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Apply", reflect.TypeOf((*MockCollector)(nil).Apply), ctx, k8sClient) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSyncCondition", reflect.TypeOf((*MockSyncStatusCollector)(nil).SetSyncCondition), condition) +} + +// SetSyncStatus mocks base method. +func (m *MockSyncStatusCollector) SetSyncStatus(phase v1alpha1.SyncPhase, message string, attemptCount int, lastSyncTime *v1.Time, lastSyncHash string, serverCount int) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetSyncStatus", phase, message, attemptCount, lastSyncTime, lastSyncHash, serverCount) +} + +// SetSyncStatus indicates an expected call of SetSyncStatus. +func (mr *MockSyncStatusCollectorMockRecorder) SetSyncStatus(phase, message, attemptCount, lastSyncTime, lastSyncHash, serverCount any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSyncStatus", reflect.TypeOf((*MockSyncStatusCollector)(nil).SetSyncStatus), phase, message, attemptCount, lastSyncTime, lastSyncHash, serverCount) +} + +// MockAPIStatusCollector is a mock of APIStatusCollector interface. +type MockAPIStatusCollector struct { + ctrl *gomock.Controller + recorder *MockAPIStatusCollectorMockRecorder + isgomock struct{} +} + +// MockAPIStatusCollectorMockRecorder is the mock recorder for MockAPIStatusCollector. +type MockAPIStatusCollectorMockRecorder struct { + mock *MockAPIStatusCollector +} + +// NewMockAPIStatusCollector creates a new mock instance. +func NewMockAPIStatusCollector(ctrl *gomock.Controller) *MockAPIStatusCollector { + mock := &MockAPIStatusCollector{ctrl: ctrl} + mock.recorder = &MockAPIStatusCollectorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockAPIStatusCollector) EXPECT() *MockAPIStatusCollectorMockRecorder { + return m.recorder } // SetAPIReadyCondition mocks base method. -func (m *MockCollector) SetAPIReadyCondition(reason, message string, status v1.ConditionStatus) { +func (m *MockAPIStatusCollector) SetAPIReadyCondition(reason, message string, status v1.ConditionStatus) { m.ctrl.T.Helper() m.ctrl.Call(m, "SetAPIReadyCondition", reason, message, status) } // SetAPIReadyCondition indicates an expected call of SetAPIReadyCondition. -func (mr *MockCollectorMockRecorder) SetAPIReadyCondition(reason, message, status any) *gomock.Call { +func (mr *MockAPIStatusCollectorMockRecorder) SetAPIReadyCondition(reason, message, status any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetAPIReadyCondition", reflect.TypeOf((*MockCollector)(nil).SetAPIReadyCondition), reason, message, status) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetAPIReadyCondition", reflect.TypeOf((*MockAPIStatusCollector)(nil).SetAPIReadyCondition), reason, message, status) } // SetAPIStatus mocks base method. -func (m *MockCollector) SetAPIStatus(phase v1alpha1.APIPhase, message, endpoint string) { +func (m *MockAPIStatusCollector) SetAPIStatus(phase v1alpha1.APIPhase, message, endpoint string) { m.ctrl.T.Helper() m.ctrl.Call(m, "SetAPIStatus", phase, message, endpoint) } // SetAPIStatus indicates an expected call of SetAPIStatus. -func (mr *MockCollectorMockRecorder) SetAPIStatus(phase, message, endpoint any) *gomock.Call { +func (mr *MockAPIStatusCollectorMockRecorder) SetAPIStatus(phase, message, endpoint any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetAPIStatus", reflect.TypeOf((*MockCollector)(nil).SetAPIStatus), phase, message, endpoint) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetAPIStatus", reflect.TypeOf((*MockAPIStatusCollector)(nil).SetAPIStatus), phase, message, endpoint) +} + +// MockStatusDeriver is a mock of StatusDeriver interface. +type MockStatusDeriver struct { + ctrl *gomock.Controller + recorder *MockStatusDeriverMockRecorder + isgomock struct{} } -// SetMessage mocks base method. -func (m *MockCollector) SetMessage(message string) { +// MockStatusDeriverMockRecorder is the mock recorder for MockStatusDeriver. +type MockStatusDeriverMockRecorder struct { + mock *MockStatusDeriver +} + +// NewMockStatusDeriver creates a new mock instance. +func NewMockStatusDeriver(ctrl *gomock.Controller) *MockStatusDeriver { + mock := &MockStatusDeriver{ctrl: ctrl} + mock.recorder = &MockStatusDeriverMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockStatusDeriver) EXPECT() *MockStatusDeriverMockRecorder { + return m.recorder +} + +// DeriveOverallStatus mocks base method. +func (m *MockStatusDeriver) DeriveOverallStatus(syncStatus *v1alpha1.SyncStatus, apiStatus *v1alpha1.APIStatus) (v1alpha1.MCPRegistryPhase, string) { m.ctrl.T.Helper() - m.ctrl.Call(m, "SetMessage", message) + ret := m.ctrl.Call(m, "DeriveOverallStatus", syncStatus, apiStatus) + ret0, _ := ret[0].(v1alpha1.MCPRegistryPhase) + ret1, _ := ret[1].(string) + return ret0, ret1 } -// SetMessage indicates an expected call of SetMessage. -func (mr *MockCollectorMockRecorder) SetMessage(message any) *gomock.Call { +// DeriveOverallStatus indicates an expected call of DeriveOverallStatus. +func (mr *MockStatusDeriverMockRecorder) DeriveOverallStatus(syncStatus, apiStatus any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetMessage", reflect.TypeOf((*MockCollector)(nil).SetMessage), message) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeriveOverallStatus", reflect.TypeOf((*MockStatusDeriver)(nil).DeriveOverallStatus), syncStatus, apiStatus) } -// SetPhase mocks base method. -func (m *MockCollector) SetPhase(phase v1alpha1.MCPRegistryPhase) { +// MockStatusManager is a mock of StatusManager interface. +type MockStatusManager struct { + ctrl *gomock.Controller + recorder *MockStatusManagerMockRecorder + isgomock struct{} +} + +// MockStatusManagerMockRecorder is the mock recorder for MockStatusManager. +type MockStatusManagerMockRecorder struct { + mock *MockStatusManager +} + +// NewMockStatusManager creates a new mock instance. +func NewMockStatusManager(ctrl *gomock.Controller) *MockStatusManager { + mock := &MockStatusManager{ctrl: ctrl} + mock.recorder = &MockStatusManagerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockStatusManager) EXPECT() *MockStatusManagerMockRecorder { + return m.recorder +} + +// API mocks base method. +func (m *MockStatusManager) API() mcpregistrystatus.APIStatusCollector { m.ctrl.T.Helper() - m.ctrl.Call(m, "SetPhase", phase) + ret := m.ctrl.Call(m, "API") + ret0, _ := ret[0].(mcpregistrystatus.APIStatusCollector) + return ret0 } -// SetPhase indicates an expected call of SetPhase. -func (mr *MockCollectorMockRecorder) SetPhase(phase any) *gomock.Call { +// API indicates an expected call of API. +func (mr *MockStatusManagerMockRecorder) API() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetPhase", reflect.TypeOf((*MockCollector)(nil).SetPhase), phase) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "API", reflect.TypeOf((*MockStatusManager)(nil).API)) } -// SetSyncStatus mocks base method. -func (m *MockCollector) SetSyncStatus(phase v1alpha1.SyncPhase, message string, attemptCount int, lastSyncTime *v1.Time, lastSyncHash string, serverCount int) { +// Apply mocks base method. +func (m *MockStatusManager) Apply(ctx context.Context, k8sClient client.Client) error { m.ctrl.T.Helper() - m.ctrl.Call(m, "SetSyncStatus", phase, message, attemptCount, lastSyncTime, lastSyncHash, serverCount) + ret := m.ctrl.Call(m, "Apply", ctx, k8sClient) + ret0, _ := ret[0].(error) + return ret0 } -// SetSyncStatus indicates an expected call of SetSyncStatus. -func (mr *MockCollectorMockRecorder) SetSyncStatus(phase, message, attemptCount, lastSyncTime, lastSyncHash, serverCount any) *gomock.Call { +// Apply indicates an expected call of Apply. +func (mr *MockStatusManagerMockRecorder) Apply(ctx, k8sClient any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Apply", reflect.TypeOf((*MockStatusManager)(nil).Apply), ctx, k8sClient) +} + +// SetCondition mocks base method. +func (m *MockStatusManager) SetCondition(conditionType, reason, message string, status v1.ConditionStatus) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetCondition", conditionType, reason, message, status) +} + +// SetCondition indicates an expected call of SetCondition. +func (mr *MockStatusManagerMockRecorder) SetCondition(conditionType, reason, message, status any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetCondition", reflect.TypeOf((*MockStatusManager)(nil).SetCondition), conditionType, reason, message, status) +} + +// SetOverallStatus mocks base method. +func (m *MockStatusManager) SetOverallStatus(phase v1alpha1.MCPRegistryPhase, message string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetOverallStatus", phase, message) +} + +// SetOverallStatus indicates an expected call of SetOverallStatus. +func (mr *MockStatusManagerMockRecorder) SetOverallStatus(phase, message any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetOverallStatus", reflect.TypeOf((*MockStatusManager)(nil).SetOverallStatus), phase, message) +} + +// Sync mocks base method. +func (m *MockStatusManager) Sync() mcpregistrystatus.SyncStatusCollector { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Sync") + ret0, _ := ret[0].(mcpregistrystatus.SyncStatusCollector) + return ret0 +} + +// Sync indicates an expected call of Sync. +func (mr *MockStatusManagerMockRecorder) Sync() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSyncStatus", reflect.TypeOf((*MockCollector)(nil).SetSyncStatus), phase, message, attemptCount, lastSyncTime, lastSyncHash, serverCount) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Sync", reflect.TypeOf((*MockStatusManager)(nil).Sync)) } diff --git a/cmd/thv-operator/pkg/mcpregistrystatus/types.go b/cmd/thv-operator/pkg/mcpregistrystatus/types.go index 858e9520b..b9d990e4c 100644 --- a/cmd/thv-operator/pkg/mcpregistrystatus/types.go +++ b/cmd/thv-operator/pkg/mcpregistrystatus/types.go @@ -10,29 +10,69 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" ) -//go:generate mockgen -destination=mocks/mock_collector.go -package=mocks -source=types.go Collector +// Error represents a structured error with condition information for operator components +type Error struct { + Err error + Message string + ConditionType string + ConditionReason string +} -// Collector defines the interface for collecting MCPRegistry status updates. -// It provides methods to collect status changes during reconciliation -// and apply them in a single batch update at the end. -type Collector interface { - // SetAPIReadyCondition sets the API ready condition with the specified reason, message, and status - SetAPIReadyCondition(reason, message string, status metav1.ConditionStatus) +func (e *Error) Error() string { + return e.Message +} + +func (e *Error) Unwrap() error { + return e.Err +} - // SetPhase sets the MCPRegistry phase in the status (overall phase) - SetPhase(phase mcpv1alpha1.MCPRegistryPhase) +//go:generate mockgen -destination=mocks/mock_collector.go -package=mocks -source=types.go SyncStatusCollector,APIStatusCollector,StatusDeriver,StatusManager - // SetMessage sets the status message (overall message) - SetMessage(message string) +// SyncStatusCollector handles sync-related status updates +type SyncStatusCollector interface { + // Status returns the current sync status + Status() *mcpv1alpha1.SyncStatus // SetSyncStatus sets the detailed sync status - SetSyncStatus( - phase mcpv1alpha1.SyncPhase, message string, attemptCount int, + SetSyncStatus(phase mcpv1alpha1.SyncPhase, message string, attemptCount int, lastSyncTime *metav1.Time, lastSyncHash string, serverCount int) + // SetSyncCondition sets a sync-related condition + SetSyncCondition(condition metav1.Condition) +} + +// APIStatusCollector handles API-related status updates +type APIStatusCollector interface { + // Status returns the current status + Status() *mcpv1alpha1.APIStatus + // SetAPIStatus sets the detailed API status SetAPIStatus(phase mcpv1alpha1.APIPhase, message string, endpoint string) + // SetAPIReadyCondition sets the API ready condition with the specified reason, message, and status + SetAPIReadyCondition(reason, message string, status metav1.ConditionStatus) +} + +// StatusDeriver handles overall status derivation logic +type StatusDeriver interface { + // DeriveOverallStatus derives the overall MCPRegistry phase and message from component statuses + DeriveOverallStatus(syncStatus *mcpv1alpha1.SyncStatus, apiStatus *mcpv1alpha1.APIStatus) (mcpv1alpha1.MCPRegistryPhase, string) +} + +// StatusManager orchestrates all status updates and provides access to domain-specific collectors +type StatusManager interface { + // Sync returns the sync status collector + Sync() SyncStatusCollector + + // API returns the API status collector + API() APIStatusCollector + + // SetOverallStatus sets the overall phase and message explicitly (for special cases) + SetOverallStatus(phase mcpv1alpha1.MCPRegistryPhase, message string) + + // SetCondition sets a general condition + SetCondition(conditionType, reason, message string, status metav1.ConditionStatus) + // Apply applies all collected status changes in a single batch update Apply(ctx context.Context, k8sClient client.Client) error } diff --git a/cmd/thv-operator/pkg/mcpregistrystatus/types_test.go b/cmd/thv-operator/pkg/mcpregistrystatus/types_test.go new file mode 100644 index 000000000..90d4aeb9e --- /dev/null +++ b/cmd/thv-operator/pkg/mcpregistrystatus/types_test.go @@ -0,0 +1,168 @@ +package mcpregistrystatus + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestError_Error(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err *Error + expected string + }{ + { + name: "normal message", + err: &Error{ + Err: errors.New("underlying error"), + Message: "custom error message", + ConditionType: "TestCondition", + ConditionReason: "TestReason", + }, + expected: "custom error message", + }, + { + name: "empty message", + err: &Error{ + Err: errors.New("underlying error"), + Message: "", + ConditionType: "TestCondition", + ConditionReason: "TestReason", + }, + expected: "", + }, + { + name: "message with special characters", + err: &Error{ + Err: errors.New("underlying error"), + Message: "Error: 50% of deployments failed\nRetry needed", + ConditionType: "TestCondition", + ConditionReason: "TestReason", + }, + expected: "Error: 50% of deployments failed\nRetry needed", + }, + { + name: "nil underlying error", + err: &Error{ + Err: nil, + Message: "custom message without underlying error", + ConditionType: "TestCondition", + ConditionReason: "TestReason", + }, + expected: "custom message without underlying error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result := tt.err.Error() + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestError_Unwrap(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err *Error + expected error + }{ + { + name: "normal underlying error", + err: &Error{ + Err: errors.New("underlying error"), + Message: "custom error message", + ConditionType: "TestCondition", + ConditionReason: "TestReason", + }, + expected: errors.New("underlying error"), + }, + { + name: "nil underlying error", + err: &Error{ + Err: nil, + Message: "custom error message", + ConditionType: "TestCondition", + ConditionReason: "TestReason", + }, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result := tt.err.Unwrap() + if tt.expected == nil { + assert.Nil(t, result) + } else { + assert.Equal(t, tt.expected.Error(), result.Error()) + } + }) + } +} + +func TestError_Interface(t *testing.T) { + t.Parallel() + + // Test that Error implements the error interface + var _ error = &Error{} + + // Test error chaining with errors.Is and errors.As + originalErr := errors.New("original error") + wrappedErr := &Error{ + Err: originalErr, + Message: "wrapped error", + ConditionType: "TestCondition", + ConditionReason: "TestReason", + } + + // Test errors.Is + assert.True(t, errors.Is(wrappedErr, originalErr)) + + // Test errors.As + var targetErr *Error + assert.True(t, errors.As(wrappedErr, &targetErr)) + assert.Equal(t, "wrapped error", targetErr.Message) + assert.Equal(t, "TestCondition", targetErr.ConditionType) + assert.Equal(t, "TestReason", targetErr.ConditionReason) +} + +func TestError_Fields(t *testing.T) { + t.Parallel() + + originalErr := errors.New("original error") + err := &Error{ + Err: originalErr, + Message: "custom message", + ConditionType: "SyncFailed", + ConditionReason: "NetworkError", + } + + // Test that all fields are accessible and correct + assert.Equal(t, originalErr, err.Err) + assert.Equal(t, "custom message", err.Message) + assert.Equal(t, "SyncFailed", err.ConditionType) + assert.Equal(t, "NetworkError", err.ConditionReason) +} + +func TestError_ZeroValue(t *testing.T) { + t.Parallel() + + // Test zero value behavior + var err Error + + assert.Equal(t, "", err.Error()) + assert.Nil(t, err.Unwrap()) + assert.Equal(t, "", err.ConditionType) + assert.Equal(t, "", err.ConditionReason) +} diff --git a/cmd/thv-operator/pkg/registryapi/manager.go b/cmd/thv-operator/pkg/registryapi/manager.go index 807eb4253..7cfc0440f 100644 --- a/cmd/thv-operator/pkg/registryapi/manager.go +++ b/cmd/thv-operator/pkg/registryapi/manager.go @@ -6,7 +6,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -43,8 +42,8 @@ func NewManager( // This method coordinates all aspects of API service including creating/updating the deployment and service, // checking readiness, and updating the MCPRegistry status with deployment references and endpoint information. func (m *manager) ReconcileAPIService( - ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, statusCollector mcpregistrystatus.Collector, -) error { + ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, +) *mcpregistrystatus.Error { ctxLogger := log.FromContext(ctx).WithValues("mcpregistry", mcpRegistry.Name) ctxLogger.Info("Reconciling API service") @@ -52,33 +51,33 @@ func (m *manager) ReconcileAPIService( deployment, err := m.ensureDeployment(ctx, mcpRegistry) if err != nil { ctxLogger.Error(err, "Failed to ensure deployment") - // Update status with failure condition - statusCollector.SetAPIStatus(mcpv1alpha1.APIPhaseError, - fmt.Sprintf("Failed to ensure deployment: %v", err), "") - statusCollector.SetAPIReadyCondition("DeploymentFailed", - fmt.Sprintf("Failed to ensure deployment: %v", err), metav1.ConditionFalse) - return fmt.Errorf("failed to ensure deployment: %w", err) + return &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Failed to ensure deployment: %v", err), + ConditionType: mcpv1alpha1.ConditionAPIReady, + ConditionReason: "DeploymentFailed", + } } // Step 2: Ensure service exists and is configured correctly - service, err := m.ensureService(ctx, mcpRegistry) + _, err = m.ensureService(ctx, mcpRegistry) if err != nil { ctxLogger.Error(err, "Failed to ensure service") - // Update status with failure condition - statusCollector.SetAPIStatus(mcpv1alpha1.APIPhaseError, - fmt.Sprintf("Failed to ensure service: %v", err), "") - statusCollector.SetAPIReadyCondition("ServiceFailed", - fmt.Sprintf("Failed to ensure service: %v", err), metav1.ConditionFalse) - return fmt.Errorf("failed to ensure service: %w", err) + return &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Failed to ensure service: %v", err), + ConditionType: mcpv1alpha1.ConditionAPIReady, + ConditionReason: "ServiceFailed", + } } // Step 3: Check API readiness isReady := m.CheckAPIReadiness(ctx, deployment) - // Step 4: Update MCPRegistry status with deployment and service references - m.updateAPIStatus(ctx, mcpRegistry, deployment, service, isReady, statusCollector) + // Note: Status updates are now handled by the controller + // The controller can call IsAPIReady to check readiness and update status accordingly - // Step 5: Log completion status + // Step 4: Log completion status if isReady { ctxLogger.Info("API service reconciliation completed successfully - API is ready") } else { @@ -109,59 +108,6 @@ func (m *manager) IsAPIReady(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRe return m.CheckAPIReadiness(ctx, deployment) } -// updateAPIStatus updates the MCPRegistry status with deployment and service references and API endpoint information -func (*manager) updateAPIStatus(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, - _ *appsv1.Deployment, service *corev1.Service, isReady bool, statusCollector mcpregistrystatus.Collector) { - ctxLogger := log.FromContext(ctx) - - // Determine API endpoint - var endpoint string - if service != nil { - // Construct internal URL from service information - endpoint = fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", - service.Name, service.Namespace, service.Spec.Ports[0].Port) - } - - // Set detailed API status - var apiPhase mcpv1alpha1.APIPhase - var reason, message string - - if isReady { - apiPhase = mcpv1alpha1.APIPhaseReady - reason = "APIReady" - message = "Registry API is ready and serving requests" - } else { - apiPhase = mcpv1alpha1.APIPhaseDeploying - reason = "APINotReady" - message = "Registry API deployment is not ready yet" - } - - // Only update API status if it has changed - currentAPIPhase := mcpv1alpha1.APIPhaseNotStarted // default - currentMessage := "" - currentEndpoint := "" - if mcpRegistry.Status.APIStatus != nil { - currentAPIPhase = mcpRegistry.Status.APIStatus.Phase - currentMessage = mcpRegistry.Status.APIStatus.Message - currentEndpoint = mcpRegistry.Status.APIStatus.Endpoint - } - - // Set API status only if it has changed - if currentAPIPhase != apiPhase || currentMessage != message || currentEndpoint != endpoint { - statusCollector.SetAPIStatus(apiPhase, message, endpoint) - statusCollector.SetAPIReadyCondition(reason, message, - func() metav1.ConditionStatus { - if isReady { - return metav1.ConditionTrue - } - return metav1.ConditionFalse - }()) - } - - ctxLogger.V(1).Info("Prepared API status update for batching", - "apiPhase", apiPhase, "apiReady", isReady) -} - // ConfigureDeploymentStorage configures a deployment with storage-specific requirements. // This method inverts the dependency by having the deployment manager configure itself // based on the storage manager type, following the dependency inversion principle. diff --git a/cmd/thv-operator/pkg/registryapi/manager_test.go b/cmd/thv-operator/pkg/registryapi/manager_test.go index d251bd4c8..55b9502e2 100644 --- a/cmd/thv-operator/pkg/registryapi/manager_test.go +++ b/cmd/thv-operator/pkg/registryapi/manager_test.go @@ -13,7 +13,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" - mcpregistrystatusmocks "github.com/stacklok/toolhive/cmd/thv-operator/pkg/mcpregistrystatus/mocks" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/sources" sourcesmocks "github.com/stacklok/toolhive/cmd/thv-operator/pkg/sources/mocks" ) @@ -390,92 +389,3 @@ func TestManagerCheckAPIReadiness(t *testing.T) { }) } } - -func TestManagerUpdateAPIStatus(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - service *corev1.Service - isReady bool - setupMocks func(*mcpregistrystatusmocks.MockCollector) - description string - }{ - { - name: "ready API with service", - service: &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-service", - Namespace: "test-namespace", - }, - Spec: corev1.ServiceSpec{ - Ports: []corev1.ServicePort{ - { - Port: 8080, - }, - }, - }, - }, - isReady: true, - setupMocks: func(m *mcpregistrystatusmocks.MockCollector) { - m.EXPECT().SetAPIStatus(mcpv1alpha1.APIPhaseReady, "Registry API is ready and serving requests", "http://test-service.test-namespace.svc.cluster.local:8080") - m.EXPECT().SetAPIReadyCondition("APIReady", "Registry API is ready and serving requests", metav1.ConditionTrue) - }, - description: "Should set endpoint and ready condition when API is ready", - }, - { - name: "not ready API with service", - service: &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-service", - Namespace: "test-namespace", - }, - Spec: corev1.ServiceSpec{ - Ports: []corev1.ServicePort{ - { - Port: 8080, - }, - }, - }, - }, - isReady: false, - setupMocks: func(m *mcpregistrystatusmocks.MockCollector) { - m.EXPECT().SetAPIStatus(mcpv1alpha1.APIPhaseDeploying, "Registry API deployment is not ready yet", "http://test-service.test-namespace.svc.cluster.local:8080") - m.EXPECT().SetAPIReadyCondition("APINotReady", "Registry API deployment is not ready yet", metav1.ConditionFalse) - }, - description: "Should set endpoint and not ready condition when API is not ready", - }, - { - name: "no service provided", - service: nil, - isReady: false, - setupMocks: func(m *mcpregistrystatusmocks.MockCollector) { - m.EXPECT().SetAPIStatus(mcpv1alpha1.APIPhaseDeploying, "Registry API deployment is not ready yet", "") - m.EXPECT().SetAPIReadyCondition("APINotReady", "Registry API deployment is not ready yet", metav1.ConditionFalse) - }, - description: "Should only set condition when no service is provided", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockCollector := mcpregistrystatusmocks.NewMockCollector(ctrl) - tt.setupMocks(mockCollector) - - manager := &manager{} - ctx := context.Background() - mcpRegistry := &mcpv1alpha1.MCPRegistry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-registry", - Namespace: "test-namespace", - }, - } - - manager.updateAPIStatus(ctx, mcpRegistry, nil, tt.service, tt.isReady, mockCollector) - }) - } -} diff --git a/cmd/thv-operator/pkg/registryapi/mocks/mock_manager.go b/cmd/thv-operator/pkg/registryapi/mocks/mock_manager.go index 7ae3963cc..18c0ae40b 100644 --- a/cmd/thv-operator/pkg/registryapi/mocks/mock_manager.go +++ b/cmd/thv-operator/pkg/registryapi/mocks/mock_manager.go @@ -72,15 +72,15 @@ func (mr *MockManagerMockRecorder) IsAPIReady(ctx, mcpRegistry any) *gomock.Call } // ReconcileAPIService mocks base method. -func (m *MockManager) ReconcileAPIService(ctx context.Context, mcpRegistry *v1alpha1.MCPRegistry, statusCollector mcpregistrystatus.Collector) error { +func (m *MockManager) ReconcileAPIService(ctx context.Context, mcpRegistry *v1alpha1.MCPRegistry) *mcpregistrystatus.Error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ReconcileAPIService", ctx, mcpRegistry, statusCollector) - ret0, _ := ret[0].(error) + ret := m.ctrl.Call(m, "ReconcileAPIService", ctx, mcpRegistry) + ret0, _ := ret[0].(*mcpregistrystatus.Error) return ret0 } // ReconcileAPIService indicates an expected call of ReconcileAPIService. -func (mr *MockManagerMockRecorder) ReconcileAPIService(ctx, mcpRegistry, statusCollector any) *gomock.Call { +func (mr *MockManagerMockRecorder) ReconcileAPIService(ctx, mcpRegistry any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReconcileAPIService", reflect.TypeOf((*MockManager)(nil).ReconcileAPIService), ctx, mcpRegistry, statusCollector) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReconcileAPIService", reflect.TypeOf((*MockManager)(nil).ReconcileAPIService), ctx, mcpRegistry) } diff --git a/cmd/thv-operator/pkg/registryapi/types.go b/cmd/thv-operator/pkg/registryapi/types.go index 20ec7ff9e..265b46f4d 100644 --- a/cmd/thv-operator/pkg/registryapi/types.go +++ b/cmd/thv-operator/pkg/registryapi/types.go @@ -59,7 +59,7 @@ const ( // Manager handles registry API deployment operations type Manager interface { // ReconcileAPIService orchestrates the deployment, service creation, and readiness checking for the registry API - ReconcileAPIService(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, statusCollector mcpregistrystatus.Collector) error + ReconcileAPIService(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry) *mcpregistrystatus.Error // CheckAPIReadiness verifies that the deployed registry-API Deployment is ready CheckAPIReadiness(ctx context.Context, deployment *appsv1.Deployment) bool diff --git a/cmd/thv-operator/pkg/sync/manager.go b/cmd/thv-operator/pkg/sync/manager.go index f1f1f596d..a5a91227b 100644 --- a/cmd/thv-operator/pkg/sync/manager.go +++ b/cmd/thv-operator/pkg/sync/manager.go @@ -5,7 +5,6 @@ import ( "fmt" "time" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -14,6 +13,7 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/filtering" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/mcpregistrystatus" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/sources" ) @@ -69,7 +69,7 @@ type Manager interface { ShouldSync(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry) (bool, string, *time.Time, error) // PerformSync executes the complete sync operation - PerformSync(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry) (ctrl.Result, *Result, error) + PerformSync(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry) (ctrl.Result, *Result, *mcpregistrystatus.Error) // UpdateManualSyncTriggerOnly updates manual sync trigger tracking without performing actual sync UpdateManualSyncTriggerOnly(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry) (ctrl.Result, error) @@ -211,7 +211,7 @@ func (*DefaultSyncManager) isSyncNeededForState(mcpRegistry *mcpv1alpha1.MCPRegi // The controller is responsible for setting sync status via the status collector func (s *DefaultSyncManager) PerformSync( ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, -) (ctrl.Result, *Result, error) { +) (ctrl.Result, *Result, *mcpregistrystatus.Error) { // Fetch and process registry data fetchResult, err := s.fetchAndProcessRegistryData(ctx, mcpRegistry) if err != nil { @@ -271,77 +271,46 @@ func (s *DefaultSyncManager) Delete(ctx context.Context, mcpRegistry *mcpv1alpha return s.storageManager.Delete(ctx, mcpRegistry) } -// updatePhase updates the MCPRegistry phase and message -func (s *DefaultSyncManager) updatePhase(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, - phase mcpv1alpha1.MCPRegistryPhase, message string) error { - mcpRegistry.Status.Phase = phase - mcpRegistry.Status.Message = message - return s.client.Status().Update(ctx, mcpRegistry) -} - -// updatePhaseFailedWithCondition updates phase, message and sets a condition -func (s *DefaultSyncManager) updatePhaseFailedWithCondition(ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, - message string, conditionType string, reason, conditionMessage string) error { - - // Refresh object to get latest resourceVersion - if err := s.client.Get(ctx, client.ObjectKeyFromObject(mcpRegistry), mcpRegistry); err != nil { - return err - } - - mcpRegistry.Status.Phase = mcpv1alpha1.MCPRegistryPhaseFailed - mcpRegistry.Status.Message = message - - // Set condition - meta.SetStatusCondition(&mcpRegistry.Status.Conditions, metav1.Condition{ - Type: conditionType, - Status: metav1.ConditionFalse, - Reason: reason, - Message: conditionMessage, - }) - - return s.client.Status().Update(ctx, mcpRegistry) -} - // fetchAndProcessRegistryData handles source handler creation, validation, fetch, and filtering func (s *DefaultSyncManager) fetchAndProcessRegistryData( ctx context.Context, - mcpRegistry *mcpv1alpha1.MCPRegistry) (*sources.FetchResult, error) { + mcpRegistry *mcpv1alpha1.MCPRegistry) (*sources.FetchResult, *mcpregistrystatus.Error) { ctxLogger := log.FromContext(ctx) // Get source handler sourceHandler, err := s.sourceHandlerFactory.CreateHandler(mcpRegistry.Spec.Source.Type) if err != nil { ctxLogger.Error(err, "Failed to create source handler") - if updateErr := s.updatePhaseFailedWithCondition(ctx, mcpRegistry, - fmt.Sprintf("Failed to create source handler: %v", err), - mcpv1alpha1.ConditionSourceAvailable, conditionReasonHandlerCreationFailed, err.Error()); updateErr != nil { - ctxLogger.Error(updateErr, "Failed to update status after handler creation failure") + return nil, &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Failed to create source handler: %v", err), + ConditionType: mcpv1alpha1.ConditionSourceAvailable, + ConditionReason: conditionReasonHandlerCreationFailed, } - return nil, err } // Validate source configuration if err := sourceHandler.Validate(&mcpRegistry.Spec.Source); err != nil { ctxLogger.Error(err, "Source validation failed") - if updateErr := s.updatePhaseFailedWithCondition(ctx, mcpRegistry, - fmt.Sprintf("Source validation failed: %v", err), - mcpv1alpha1.ConditionSourceAvailable, conditionReasonValidationFailed, err.Error()); updateErr != nil { - ctxLogger.Error(updateErr, "Failed to update status after validation failure") + return nil, &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Source validation failed: %v", err), + ConditionType: mcpv1alpha1.ConditionSourceAvailable, + ConditionReason: conditionReasonValidationFailed, } - return nil, err } // Execute fetch operation fetchResult, err := sourceHandler.FetchRegistry(ctx, mcpRegistry) if err != nil { - ctxLogger.Error(err, "Fetch operation failed") + ctxLogger.Info("Fetch operation failed", "error", err.Error()) // Sync attempt counting is now handled by the controller via status collector - if updateErr := s.updatePhaseFailedWithCondition(ctx, mcpRegistry, - fmt.Sprintf("Fetch failed: %v", err), - mcpv1alpha1.ConditionSyncSuccessful, conditionReasonFetchFailed, err.Error()); updateErr != nil { - ctxLogger.Error(updateErr, "Failed to update status after fetch failure") + return nil, &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Fetch failed: %v", err), + ConditionType: mcpv1alpha1.ConditionSyncSuccessful, + ConditionReason: conditionReasonFetchFailed, } - return nil, err } ctxLogger.Info("Registry data fetched successfully from source", @@ -361,7 +330,7 @@ func (s *DefaultSyncManager) fetchAndProcessRegistryData( func (s *DefaultSyncManager) applyFilteringIfConfigured( ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, - fetchResult *sources.FetchResult) error { + fetchResult *sources.FetchResult) *mcpregistrystatus.Error { ctxLogger := log.FromContext(ctx) if mcpRegistry.Spec.Filter != nil { @@ -372,12 +341,12 @@ func (s *DefaultSyncManager) applyFilteringIfConfigured( filteredRegistry, err := s.filterService.ApplyFilters(ctx, fetchResult.Registry, mcpRegistry.Spec.Filter) if err != nil { ctxLogger.Error(err, "Registry filtering failed") - if updateErr := s.updatePhaseFailedWithCondition(ctx, mcpRegistry, - fmt.Sprintf("Filtering failed: %v", err), - mcpv1alpha1.ConditionSyncSuccessful, conditionReasonFetchFailed, err.Error()); updateErr != nil { - ctxLogger.Error(updateErr, "Failed to update status after filtering failure") + return &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Filtering failed: %v", err), + ConditionType: mcpv1alpha1.ConditionSyncSuccessful, + ConditionReason: conditionReasonFetchFailed, } - return err } // Update fetch result with filtered data @@ -400,17 +369,17 @@ func (s *DefaultSyncManager) applyFilteringIfConfigured( func (s *DefaultSyncManager) storeRegistryData( ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, - fetchResult *sources.FetchResult) error { + fetchResult *sources.FetchResult) *mcpregistrystatus.Error { ctxLogger := log.FromContext(ctx) if err := s.storageManager.Store(ctx, mcpRegistry, fetchResult.Registry); err != nil { ctxLogger.Error(err, "Failed to store registry data") - if updateErr := s.updatePhaseFailedWithCondition(ctx, mcpRegistry, - fmt.Sprintf("Storage failed: %v", err), - mcpv1alpha1.ConditionSyncSuccessful, conditionReasonStorageFailed, err.Error()); updateErr != nil { - ctxLogger.Error(updateErr, "Failed to update status after storage failure") + return &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Storage failed: %v", err), + ConditionType: mcpv1alpha1.ConditionSyncSuccessful, + ConditionReason: conditionReasonStorageFailed, } - return err } ctxLogger.Info("Registry data stored successfully", @@ -425,13 +394,18 @@ func (s *DefaultSyncManager) storeRegistryData( func (s *DefaultSyncManager) updateCoreRegistryFields( ctx context.Context, mcpRegistry *mcpv1alpha1.MCPRegistry, - fetchResult *sources.FetchResult) error { + fetchResult *sources.FetchResult) *mcpregistrystatus.Error { ctxLogger := log.FromContext(ctx) // Refresh the object to get latest resourceVersion before final update if err := s.client.Get(ctx, client.ObjectKeyFromObject(mcpRegistry), mcpRegistry); err != nil { ctxLogger.Error(err, "Failed to refresh MCPRegistry object") - return err + return &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Failed to refresh MCPRegistry object: %v", err), + ConditionType: mcpv1alpha1.ConditionSyncSuccessful, + ConditionReason: "ObjectRefreshFailed", + } } // Get storage reference @@ -453,7 +427,12 @@ func (s *DefaultSyncManager) updateCoreRegistryFields( // Single final status update if err := s.client.Status().Update(ctx, mcpRegistry); err != nil { ctxLogger.Error(err, "Failed to update core registry fields") - return err + return &mcpregistrystatus.Error{ + Err: err, + Message: fmt.Sprintf("Failed to update core registry fields: %v", err), + ConditionType: mcpv1alpha1.ConditionSyncSuccessful, + ConditionReason: "StatusUpdateFailed", + } } ctxLogger.Info("MCPRegistry sync completed successfully", diff --git a/cmd/thv-operator/pkg/sync/manager_test.go b/cmd/thv-operator/pkg/sync/manager_test.go index 3e475e12c..a6d4548ee 100644 --- a/cmd/thv-operator/pkg/sync/manager_test.go +++ b/cmd/thv-operator/pkg/sync/manager_test.go @@ -294,9 +294,9 @@ func TestDefaultSyncManager_PerformSync(t *testing.T) { }, }, sourceConfigMap: nil, - expectedError: true, // PerformSync now returns errors for controller to handle - expectedPhase: mcpv1alpha1.MCPRegistryPhaseFailed, - expectedServerCount: nil, // Don't validate server count for failed sync + expectedError: true, // PerformSync now returns errors for controller to handle + expectedPhase: mcpv1alpha1.MCPRegistryPhasePending, // Phase is not changed by PerformSync, only by controller + expectedServerCount: nil, // Don't validate server count for failed sync errorContains: "", validateConditions: false, }, @@ -495,15 +495,15 @@ func TestDefaultSyncManager_PerformSync(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - result, syncResult, err := syncManager.PerformSync(ctx, tt.mcpRegistry) + result, syncResult, syncErr := syncManager.PerformSync(ctx, tt.mcpRegistry) if tt.expectedError { - assert.Error(t, err) + assert.NotNil(t, syncErr) if tt.errorContains != "" { - assert.Contains(t, err.Error(), tt.errorContains) + assert.Contains(t, syncErr.Error(), tt.errorContains) } } else { - assert.NoError(t, err) + assert.Nil(t, syncErr) } // Verify the result @@ -715,99 +715,6 @@ func TestDefaultSyncManager_Delete(t *testing.T) { } } -func TestDefaultSyncManager_updatePhase(t *testing.T) { - t.Parallel() - - scheme := runtime.NewScheme() - require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) - - mcpRegistry := &mcpv1alpha1.MCPRegistry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-registry", - Namespace: "test-namespace", - UID: types.UID("test-uid"), - }, - Status: mcpv1alpha1.MCPRegistryStatus{ - Phase: mcpv1alpha1.MCPRegistryPhasePending, - }, - } - - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithRuntimeObjects(mcpRegistry). - WithStatusSubresource(&mcpv1alpha1.MCPRegistry{}). - Build() - - sourceHandlerFactory := sources.NewSourceHandlerFactory(fakeClient) - storageManager := sources.NewConfigMapStorageManager(fakeClient, scheme) - syncManager := NewDefaultSyncManager(fakeClient, scheme, sourceHandlerFactory, storageManager) - - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - err := syncManager.updatePhase(ctx, mcpRegistry, mcpv1alpha1.MCPRegistryPhaseSyncing, "Test message") - assert.NoError(t, err) - - // Verify the phase was updated - check the modified object directly - // since the method modifies in place - assert.Equal(t, mcpv1alpha1.MCPRegistryPhaseSyncing, mcpRegistry.Status.Phase) - assert.Equal(t, "Test message", mcpRegistry.Status.Message) -} - -func TestDefaultSyncManager_updatePhaseFailedWithCondition(t *testing.T) { - t.Parallel() - - scheme := runtime.NewScheme() - require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) - - mcpRegistry := &mcpv1alpha1.MCPRegistry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-registry", - Namespace: "test-namespace", - UID: types.UID("test-uid"), - }, - Status: mcpv1alpha1.MCPRegistryStatus{ - Phase: mcpv1alpha1.MCPRegistryPhasePending, - }, - } - - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithRuntimeObjects(mcpRegistry). - WithStatusSubresource(&mcpv1alpha1.MCPRegistry{}). - Build() - - sourceHandlerFactory := sources.NewSourceHandlerFactory(fakeClient) - storageManager := sources.NewConfigMapStorageManager(fakeClient, scheme) - syncManager := NewDefaultSyncManager(fakeClient, scheme, sourceHandlerFactory, storageManager) - - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - err := syncManager.updatePhaseFailedWithCondition( - ctx, - mcpRegistry, - "Test failure message", - mcpv1alpha1.ConditionSourceAvailable, - "TestFailure", - "Test condition message", - ) - assert.NoError(t, err) - - // Verify the phase and condition were updated - check the modified object directly - // since the method modifies in place after refreshing from client - assert.Equal(t, mcpv1alpha1.MCPRegistryPhaseFailed, mcpRegistry.Status.Phase) - assert.Equal(t, "Test failure message", mcpRegistry.Status.Message) - - // Check condition was set - require.Len(t, mcpRegistry.Status.Conditions, 1) - condition := mcpRegistry.Status.Conditions[0] - assert.Equal(t, mcpv1alpha1.ConditionSourceAvailable, condition.Type) - assert.Equal(t, metav1.ConditionFalse, condition.Status) - assert.Equal(t, "TestFailure", condition.Reason) - assert.Equal(t, "Test condition message", condition.Message) -} - func TestIsManualSync(t *testing.T) { t.Parallel() diff --git a/test/e2e/operator/k8s_helpers.go b/test/e2e/operator/k8s_helpers.go index 4233ed095..505759218 100644 --- a/test/e2e/operator/k8s_helpers.go +++ b/test/e2e/operator/k8s_helpers.go @@ -131,4 +131,4 @@ func (h *K8sResourceTestHelper) WaitForResourceDeletion(resourceType, name strin default: return false } -} \ No newline at end of file +} diff --git a/test/e2e/operator/registry_helpers.go b/test/e2e/operator/registry_helpers.go index eadd93193..178816f1f 100644 --- a/test/e2e/operator/registry_helpers.go +++ b/test/e2e/operator/registry_helpers.go @@ -244,3 +244,38 @@ func (h *MCPRegistryTestHelper) CleanupRegistries() error { } return nil } + +// WaitForRegistryInitialization waits for common initialization steps after registry creation: +// 1. Wait for finalizer to be added +// 2. Wait for controller to process the registry into an acceptable initial phase +func (h *MCPRegistryTestHelper) WaitForRegistryInitialization(registryName string, + timingHelper *TimingTestHelper, statusHelper *StatusTestHelper) { + // Wait for finalizer to be added + ginkgo.By("waiting for finalizer to be added") + timingHelper.WaitForControllerReconciliation(func() interface{} { + updatedRegistry, err := h.GetRegistry(registryName) + if err != nil { + return false + } + return containsFinalizer(updatedRegistry.Finalizers, "mcpregistry.toolhive.stacklok.dev/finalizer") + }).Should(gomega.BeTrue()) + + // Wait for controller to process and verify initial status + ginkgo.By("waiting for controller to process and verify initial status") + statusHelper.WaitForPhaseAny(registryName, []mcpv1alpha1.MCPRegistryPhase{ + mcpv1alpha1.MCPRegistryPhasePending, + mcpv1alpha1.MCPRegistryPhaseReady, + mcpv1alpha1.MCPRegistryPhaseSyncing, + }, MediumTimeout) +} + +// containsFinalizer checks if the registry finalizer exists in the list +func containsFinalizer(finalizers []string, _ string) bool { + const registryFinalizer = "mcpregistry.toolhive.stacklok.dev/finalizer" + for _, f := range finalizers { + if f == registryFinalizer { + return true + } + } + return false +} diff --git a/test/e2e/operator/registry_lifecycle_test.go b/test/e2e/operator/registry_lifecycle_test.go index 19c8ac259..58e8f6e0b 100644 --- a/test/e2e/operator/registry_lifecycle_test.go +++ b/test/e2e/operator/registry_lifecycle_test.go @@ -67,29 +67,8 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Expect(registry.Spec.Source.ConfigMap.Name).To(Equal(configMap.Name)) Expect(registry.Spec.SyncPolicy.Interval).To(Equal("1h")) - // Verify finalizer was added - By("waiting for finalizer to be added") - timingHelper.WaitForControllerReconciliation(func() interface{} { - updatedRegistry, err := registryHelper.GetRegistry(registry.Name) - if err != nil { - return false - } - return containsFinalizer(updatedRegistry.Finalizers, registryFinalizerName) - }).Should(BeTrue()) - - // Wait for controller to process and verify initial status - By("waiting for controller to process and verify initial status") - timingHelper.WaitForControllerReconciliation(func() interface{} { - phase, err := registryHelper.GetRegistryPhase(registry.Name) - if err != nil { - return "" - } - return phase - }).Should(BeElementOf( - mcpv1alpha1.MCPRegistryPhasePending, - mcpv1alpha1.MCPRegistryPhaseReady, - mcpv1alpha1.MCPRegistryPhaseSyncing, - )) + // Wait for registry initialization (finalizer + initial status) + registryHelper.WaitForRegistryInitialization(registry.Name, timingHelper, statusHelper) By("verifying storage ConfigMap is defined in status and exists") updatedRegistry, err := registryHelper.GetRegistry(registry.Name) @@ -168,7 +147,9 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { mcpv1alpha1.APIPhaseDeploying, // Deployment created but not ready mcpv1alpha1.APIPhaseReady, // If somehow becomes ready )) - Expect(updatedRegistry.Status.APIStatus.Endpoint).To(Equal(fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", apiResourceName, testNamespace))) + if updatedRegistry.Status.APIStatus.Phase == mcpv1alpha1.APIPhaseReady { + Expect(updatedRegistry.Status.APIStatus.Endpoint).To(Equal(fmt.Sprintf("http://%s.%s.svc.cluster.local:8080", apiResourceName, testNamespace))) + } By("BYE") }) @@ -196,8 +177,16 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { // Verify creation Expect(registry.Spec.SyncPolicy).To(BeNil()) - // Should still become ready for manual sync - statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + // Wait for registry initialization (finalizer + initial status) + registryHelper.WaitForRegistryInitialization(registry.Name, timingHelper, statusHelper) + + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + Expect(err).NotTo(HaveOccurred()) + + // Verify sync status is idle or complete + Expect(updatedRegistry.Status.SyncStatus).NotTo(BeNil()) + Expect(updatedRegistry.Status.SyncStatus.Phase).To(BeElementOf(mcpv1alpha1.SyncPhaseIdle, mcpv1alpha1.SyncPhaseComplete)) + Expect(updatedRegistry.Status.SyncStatus.ServerCount).To(Equal(1)) }) It("should set correct metadata labels and annotations", func() { @@ -210,6 +199,8 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { WithAnnotation("description", "Test registry"). Create(registryHelper) + registryHelper.WaitForRegistryInitialization(registry.Name, timingHelper, statusHelper) + // Verify labels and annotations Expect(registry.Labels).To(HaveKeyWithValue("app", "test")) Expect(registry.Labels).To(HaveKeyWithValue("version", "1.0")) @@ -255,9 +246,20 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Expect(registryHelper.DeleteRegistry(registry.Name)).To(Succeed()) // Verify registry enters terminating phase + By("waiting for registry to enter terminating phase") statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseTerminating, MediumTimeout) + By("waiting for finalizer to be removed") + timingHelper.WaitForControllerReconciliation(func() interface{} { + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + if err != nil { + return true // Registry might be deleted, which means finalizer was removed + } + return !containsFinalizer(updatedRegistry.Finalizers, registryFinalizerName) + }).Should(BeTrue()) + // Verify registry is eventually deleted (finalizer removed) + By("waiting for registry to be deleted") timingHelper.WaitForControllerReconciliation(func() interface{} { _, err := registryHelper.GetRegistry(registry.Name) return errors.IsNotFound(err) @@ -275,7 +277,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Create(registryHelper) // Wait for registry to be ready - statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + statusHelper.WaitForPhaseAny(registry.Name, []mcpv1alpha1.MCPRegistryPhase{mcpv1alpha1.MCPRegistryPhaseReady, mcpv1alpha1.MCPRegistryPhasePending}, MediumTimeout) // Store initial storage reference for cleanup verification status, err := registryHelper.GetRegistryStatus(registry.Name) @@ -343,6 +345,7 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { // Should fail validation err := k8sClient.Create(ctx, invalidRegistry) + By("verifying validation error") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("configMap field is required")) }) @@ -379,22 +382,33 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { WithConfigMapSource("nonexistent-configmap", "registry.json"). Create(registryHelper) + By("waiting for registry to enter failed state") // Should enter failed state due to missing source statusHelper.WaitForPhase(registry.Name, mcpv1alpha1.MCPRegistryPhaseFailed, MediumTimeout) // Check condition reflects the problem - statusHelper.WaitForCondition(registry.Name, mcpv1alpha1.ConditionSourceAvailable, + statusHelper.WaitForCondition(registry.Name, mcpv1alpha1.ConditionSyncSuccessful, metav1.ConditionFalse, MediumTimeout) + + updatedRegistry, err := registryHelper.GetRegistry(registry.Name) + Expect(err).NotTo(HaveOccurred()) + + By("verifying sync status") + Expect(updatedRegistry.Status.SyncStatus).NotTo(BeNil()) + Expect(updatedRegistry.Status.SyncStatus.Phase).To(Equal(mcpv1alpha1.SyncPhaseFailed)) + Expect(updatedRegistry.Status.SyncStatus.AttemptCount).To(Equal(1)) + + By("verifying API status") + Expect(updatedRegistry.Status.APIStatus).To(BeNil()) }) }) Context("Multiple Registry Management", func() { - var numServers1, numServers2 int var configMap1, configMap2 *corev1.ConfigMap It("should handle multiple registries in same namespace", func() { // Create multiple ConfigMaps - configMap1, numServers1 = configMapHelper.CreateSampleToolHiveRegistry("config-1") - configMap2, numServers2 = configMapHelper.CreateSampleUpstreamRegistry("config-2") + configMap1, _ = configMapHelper.CreateSampleToolHiveRegistry("config-1") + configMap2, _ = configMapHelper.CreateSampleToolHiveRegistry("config-2") // Create multiple registries registry1 := registryHelper.NewRegistryBuilder("registry-1"). @@ -409,13 +423,14 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Create(registryHelper) // Both should become ready independently - statusHelper.WaitForPhase(registry1.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) - statusHelper.WaitForPhase(registry2.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + statusHelper.WaitForPhaseAny(registry1.Name, []mcpv1alpha1.MCPRegistryPhase{mcpv1alpha1.MCPRegistryPhaseReady, mcpv1alpha1.MCPRegistryPhasePending}, MediumTimeout) + statusHelper.WaitForPhaseAny(registry2.Name, []mcpv1alpha1.MCPRegistryPhase{mcpv1alpha1.MCPRegistryPhaseReady, mcpv1alpha1.MCPRegistryPhasePending}, MediumTimeout) // Verify they operate independently Expect(registry1.Spec.SyncPolicy.Interval).To(Equal("1h")) Expect(registry2.Spec.SyncPolicy.Interval).To(Equal("30m")) - Expect(registry2.Spec.Source.Format).To(Equal(mcpv1alpha1.RegistryFormatUpstream)) + Expect(registry1.Spec.Source.Format).To(Equal(mcpv1alpha1.RegistryFormatToolHive)) + Expect(registry2.Spec.Source.Format).To(Equal(mcpv1alpha1.RegistryFormatToolHive)) }) It("should allow multiple registries with same ConfigMap source", func() { @@ -434,12 +449,13 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Create(registryHelper) // Both should become ready - statusHelper.WaitForPhase(registry1.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) - statusHelper.WaitForPhase(registry2.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + statusHelper.WaitForPhaseAny(registry1.Name, []mcpv1alpha1.MCPRegistryPhase{mcpv1alpha1.MCPRegistryPhaseReady, mcpv1alpha1.MCPRegistryPhasePending}, MediumTimeout) + statusHelper.WaitForPhaseAny(registry2.Name, []mcpv1alpha1.MCPRegistryPhase{mcpv1alpha1.MCPRegistryPhaseReady, mcpv1alpha1.MCPRegistryPhasePending}, MediumTimeout) // Both should have same server count from shared source - statusHelper.WaitForServerCount(registry1.Name, numServers1, MediumTimeout) - statusHelper.WaitForServerCount(registry2.Name, numServers2, MediumTimeout) + sharedNumServers := 2 // Sample ToolHive registry has 2 servers + statusHelper.WaitForServerCount(registry1.Name, sharedNumServers, MediumTimeout) + statusHelper.WaitForServerCount(registry2.Name, sharedNumServers, MediumTimeout) }) It("should handle registry name conflicts gracefully", func() { @@ -472,21 +488,11 @@ var _ = Describe("MCPRegistry Lifecycle Management", func() { Expect(errors.IsAlreadyExists(err)).To(BeTrue()) // Original registry should still be functional - statusHelper.WaitForPhase(registry1.Name, mcpv1alpha1.MCPRegistryPhaseReady, MediumTimeout) + statusHelper.WaitForPhaseAny(registry1.Name, []mcpv1alpha1.MCPRegistryPhase{mcpv1alpha1.MCPRegistryPhaseReady, mcpv1alpha1.MCPRegistryPhasePending}, MediumTimeout) }) }) }) -// Helper function to check if a finalizer exists in the list -func containsFinalizer(finalizers []string, finalizer string) bool { - for _, f := range finalizers { - if f == finalizer { - return true - } - } - return false -} - // Helper function to create test namespace func createTestNamespace(ctx context.Context) string { namespace := &corev1.Namespace{ diff --git a/test/e2e/operator/status_helpers.go b/test/e2e/operator/status_helpers.go index 269f21019..c9e463e02 100644 --- a/test/e2e/operator/status_helpers.go +++ b/test/e2e/operator/status_helpers.go @@ -28,8 +28,14 @@ func NewStatusTestHelper(ctx context.Context, k8sClient client.Client, namespace // WaitForPhase waits for an MCPRegistry to reach the specified phase func (h *StatusTestHelper) WaitForPhase(registryName string, expectedPhase mcpv1alpha1.MCPRegistryPhase, timeout time.Duration) { + h.WaitForPhaseAny(registryName, []mcpv1alpha1.MCPRegistryPhase{expectedPhase}, timeout) +} + +// WaitForPhaseAny waits for an MCPRegistry to reach any of the specified phases +func (h *StatusTestHelper) WaitForPhaseAny(registryName string, + expectedPhases []mcpv1alpha1.MCPRegistryPhase, timeout time.Duration) { gomega.Eventually(func() mcpv1alpha1.MCPRegistryPhase { - ginkgo.By(fmt.Sprintf("waiting for registry %s to reach phase %s", registryName, expectedPhase)) + ginkgo.By(fmt.Sprintf("waiting for registry %s to reach one of phases %v", registryName, expectedPhases)) registry, err := h.registryHelper.GetRegistry(registryName) if err != nil { if errors.IsNotFound(err) { @@ -39,8 +45,8 @@ func (h *StatusTestHelper) WaitForPhase(registryName string, expectedPhase mcpv1 return "" } return registry.Status.Phase - }, timeout, time.Second).Should(gomega.Equal(expectedPhase), - "MCPRegistry %s should reach phase %s", registryName, expectedPhase) + }, timeout, time.Second).Should(gomega.BeElementOf(expectedPhases), + "MCPRegistry %s should reach one of phases %v", registryName, expectedPhases) } // WaitForCondition waits for a specific condition to have the expected status