From 1e3e22b58fcc1b670ae07255649cb58975e89b22 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 19 Sep 2025 16:26:16 +0100 Subject: [PATCH 1/9] Extend the CRD --- cmd/thv-operator/api/v1alpha1/mcpregistry_types.go | 7 +++++++ .../crds/toolhive.stacklok.dev_mcpregistries.yaml | 7 +++++++ docs/operator/crd-api.md | 1 + 3 files changed, 15 insertions(+) diff --git a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go index 84095f943..c8b276d75 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go @@ -44,6 +44,13 @@ type MCPRegistrySpec struct { // Filter defines include/exclude patterns for registry content // +optional Filter *RegistryFilter `json:"filter,omitempty"` + + // Enforce indicates whether MCPServers in this namespace must exist in this registry. + // When true, MCPServers that are not found in this registry will be rejected. + // When false (default), MCPServers can be deployed regardless of registry presence. + // +kubebuilder:default=false + // +optional + Enforce bool `json:"enforce,omitempty"` } // MCPRegistrySource defines the source configuration for registry data diff --git a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml index 2b165a815..624d05f4e 100644 --- a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml +++ b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml @@ -65,6 +65,13 @@ spec: displayName: description: DisplayName is a human-readable name for the registry type: string + enforce: + default: false + description: |- + Enforce indicates whether MCPServers in this namespace must exist in this registry. + When true, MCPServers that are not found in this registry will be rejected. + When false (default), MCPServers can be deployed regardless of registry presence. + type: boolean filter: description: Filter defines include/exclude patterns for registry content diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index c46ab89b3..80555aa7f 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -344,6 +344,7 @@ _Appears in:_ | `source` _[MCPRegistrySource](#mcpregistrysource)_ | Source defines the configuration for the registry data source | | Required: \{\}
| | `syncPolicy` _[SyncPolicy](#syncpolicy)_ | SyncPolicy defines the automatic synchronization behavior for the registry.
If specified, enables automatic synchronization at the given interval.
Manual synchronization is always supported via annotation-based triggers
regardless of this setting. | | | | `filter` _[RegistryFilter](#registryfilter)_ | Filter defines include/exclude patterns for registry content | | | +| `enforce` _boolean_ | Enforce indicates whether MCPServers in this namespace must exist in this registry.
When true, MCPServers that are not found in this registry will be rejected.
When false (default), MCPServers can be deployed regardless of registry presence. | false | | #### MCPRegistryStatus From ca60f59436caedae20763c49b96e580bb10c282a Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 22 Sep 2025 08:47:03 +0100 Subject: [PATCH 2/9] Optional enforcement of images in registries --- .../api/v1alpha1/mcpserver_types.go | 17 + .../controllers/mcpserver_controller.go | 57 ++ cmd/thv-operator/main.go | 14 +- .../pkg/validation/image_validation.go | 188 +++++ .../pkg/validation/image_validation_test.go | 786 ++++++++++++++++++ .../mcp-registries/mcpregistry-enforcing.yaml | 156 ++++ 6 files changed, 1214 insertions(+), 4 deletions(-) create mode 100644 cmd/thv-operator/pkg/validation/image_validation.go create mode 100644 cmd/thv-operator/pkg/validation/image_validation_test.go create mode 100644 examples/operator/mcp-registries/mcpregistry-enforcing.yaml diff --git a/cmd/thv-operator/api/v1alpha1/mcpserver_types.go b/cmd/thv-operator/api/v1alpha1/mcpserver_types.go index ee4075502..9cad574b5 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpserver_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpserver_types.go @@ -5,6 +5,23 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// Condition types for MCPServer +const ( + // ConditionImageValidated indicates whether this image is fine to be used + ConditionImageValidated = "ImageValidated" +) + +const ( + // ConditionReasonImageValidationFailed indicates image validation failed + ConditionReasonImageValidationFailed = "ImageValidationFailed" + // ConditionReasonImageValidationSuccess indicates image validation succeeded + ConditionReasonImageValidationSuccess = "ImageValidationSuccess" + // ConditionReasonImageValidationError indicates an error occurred during validation + ConditionReasonImageValidationError = "ImageValidationError" + // ConditionReasonImageValidationSkipped indicates image validation was skipped + ConditionReasonImageValidationSkipped = "ImageValidationSkipped" +) + // MCPServerSpec defines the desired state of MCPServer type MCPServerSpec struct { // Image is the container image for the MCP server diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 34a29547e..ccc845649 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -5,6 +5,7 @@ package controllers import ( "context" "encoding/json" + goerr "errors" "fmt" "maps" "os" @@ -18,6 +19,7 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -30,6 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" "github.com/stacklok/toolhive/pkg/container/kubernetes" ) @@ -40,6 +43,7 @@ type MCPServerReconciler struct { platformDetector kubernetes.PlatformDetector detectedPlatform kubernetes.Platform platformOnce sync.Once + ImageValidation validation.ImageValidation } // defaultRBACRules are the default RBAC rules that the @@ -193,6 +197,59 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } + // Validate MCPServer image against enforcing registries + imageValidator := validation.NewImageValidator(r.Client, mcpServer.Namespace, r.ImageValidation) + err = imageValidator.ValidateImage(ctx, mcpServer.Spec.Image) + if goerr.Is(err, validation.ErrImageNotChecked) { + ctxLogger.Info("Image validation skipped - no enforcement configured") + // Set condition to indicate validation was skipped + meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionImageValidated, + Status: metav1.ConditionTrue, + Reason: mcpv1alpha1.ConditionReasonImageValidationSkipped, + Message: "Image validation was not performed (no enforcement configured)", + }) + } else if goerr.Is(err, validation.ErrImageInvalid) { + ctxLogger.Error(err, "MCPServer image validation failed", "image", mcpServer.Spec.Image) + // Update status to reflect validation failure + mcpServer.Status.Phase = mcpv1alpha1.MCPServerPhaseFailed + mcpServer.Status.Message = err.Error() // Gets the specific validation failure reason + meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionImageValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1alpha1.ConditionReasonImageValidationFailed, + Message: err.Error(), // This will include the wrapped error context with specific reason + }) + if statusErr := r.Status().Update(ctx, mcpServer); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update MCPServer status after validation error") + } + // Requeue after 5 minutes to retry validation + return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil + } else if err != nil { + // Other system/infrastructure errors + ctxLogger.Error(err, "MCPServer image validation system error", "image", mcpServer.Spec.Image) + meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionImageValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1alpha1.ConditionReasonImageValidationError, + Message: fmt.Sprintf("Error checking image validity: %v", err), + }) + if statusErr := r.Status().Update(ctx, mcpServer); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update MCPServer status after validation error") + } + // Requeue after 5 minutes to retry validation + return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil + } else { + // Validation passed + ctxLogger.Info("Image validation passed", "image", mcpServer.Spec.Image) + meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionImageValidated, + Status: metav1.ConditionTrue, + Reason: mcpv1alpha1.ConditionReasonImageValidationSuccess, + Message: "Image validation passed - image found in enforced registries", + }) + } + // Check if the MCPServer instance is marked to be deleted if mcpServer.GetDeletionTimestamp() != nil { // The object is being deleted diff --git a/cmd/thv-operator/main.go b/cmd/thv-operator/main.go index 2795f2c94..06eb2a1fe 100644 --- a/cmd/thv-operator/main.go +++ b/cmd/thv-operator/main.go @@ -22,6 +22,7 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/cmd/thv-operator/controllers" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" "github.com/stacklok/toolhive/pkg/logger" "github.com/stacklok/toolhive/pkg/operator/telemetry" ) @@ -74,16 +75,21 @@ func main() { os.Exit(1) } - if err = (&controllers.MCPServerReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - }).SetupWithManager(mgr); err != nil { + rec := &controllers.MCPServerReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + ImageValidation: validation.ImageValidationAlwaysAllow, + } + + if err = rec.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MCPServer") os.Exit(1) } // Only register MCPRegistry controller if feature flag is enabled if os.Getenv("ENABLE_EXPERIMENTAL_FEATURES") == "true" { + rec.ImageValidation = validation.ImageValidationRegistryEnforcing + if err = (controllers.NewMCPRegistryReconciler(mgr.GetClient(), mgr.GetScheme())).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MCPRegistry") os.Exit(1) diff --git a/cmd/thv-operator/pkg/validation/image_validation.go b/cmd/thv-operator/pkg/validation/image_validation.go new file mode 100644 index 000000000..59d1548de --- /dev/null +++ b/cmd/thv-operator/pkg/validation/image_validation.go @@ -0,0 +1,188 @@ +// Package validation provides image validation functionality for the ToolHive operator. +package validation + +import ( + "context" + "encoding/json" + "errors" + "fmt" + + corev1 "k8s.io/api/core/v1" + k8serr "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/pkg/registry" +) + +// Sentinel errors for image validation. +// These errors can be checked using errors.Is() to determine the specific validation failure. +var ( + // ErrImageInvalid indicates that the image failed validation for any reason. + // The wrapped error and message provide specific details about the validation failure. + // This is the generic error that controllers should check for to handle any validation failure. + ErrImageInvalid = errors.New("image validation failed") + + // ErrImageNotChecked indicates that no validation was performed on the image + ErrImageNotChecked = errors.New("image validation was not performed") +) + +// ImageValidation represents the type of image validation to perform. +type ImageValidation string + +const ( + // ImageValidationAlwaysAllow indicates that all images are allowed + ImageValidationAlwaysAllow ImageValidation = "always-allow" + // ImageValidationRegistryEnforcing indicates that images must be validated against MCPRegistry resources + ImageValidationRegistryEnforcing ImageValidation = "registry-enforcing" +) + +// ImageValidator defines the interface for validating container images +type ImageValidator interface { + // ValidateImage checks if an image is valid for use. + // Returns: + // - nil if validation passes + // - ErrImageNotChecked if no validation was performed + // - wrapped ErrImageInvalid if image fails validation (with specific reason in error message) + // - other errors for system/infrastructure failures + ValidateImage(ctx context.Context, image string) error +} + +// AlwaysAllowValidator is a no-op validator that always allows images +type AlwaysAllowValidator struct{} + +// ValidateImage always returns ErrImageNotChecked, indicating no validation was performed +func (*AlwaysAllowValidator) ValidateImage(_ context.Context, _ string) error { + return ErrImageNotChecked +} + +// NewImageValidator creates an appropriate ImageValidator based on configuration +func NewImageValidator(k8sClient client.Client, namespace string, validation ImageValidation) ImageValidator { + if validation == ImageValidationRegistryEnforcing { + return &RegistryEnforcingValidator{ + client: k8sClient, + namespace: namespace, + } + } + return &AlwaysAllowValidator{} +} + +// RegistryEnforcingValidator provides validation against MCPRegistry resources +type RegistryEnforcingValidator struct { + client client.Client + namespace string +} + +// ValidateImage checks if an image should be validated and if it exists in registries +func (v *RegistryEnforcingValidator) ValidateImage(ctx context.Context, image string) error { + // List all MCPRegistry resources in the namespace + mcpRegistryList := &mcpv1alpha1.MCPRegistryList{} + if err := v.client.List(ctx, mcpRegistryList, client.InNamespace(v.namespace)); err != nil { + return fmt.Errorf("failed to list MCPRegistry resources: %w", err) + } + + // Check if any registry enforces validation + // If no enforcement required, return ErrImageNotChecked to indicate no validation was performed + hasEnforcement := v.hasEnforcingRegistry(mcpRegistryList) + if !hasEnforcement { + return ErrImageNotChecked + } + + // Enforcement is required, check each registry for the image + var registryErrors []string + for _, mcpRegistry := range mcpRegistryList.Items { + found, err := v.checkImageInRegistry(ctx, &mcpRegistry, image) + if err != nil { + // Collect errors but continue checking other registries + registryErrors = append(registryErrors, fmt.Sprintf("registry %s: %v", mcpRegistry.Name, err)) + continue + } + if found { + // Image found, validation passes + return nil + } + } + + // Image not found in any registry and enforcement is required + // Wrap the generic validation error with context about the specific image and any registry errors + if len(registryErrors) > 0 { + return fmt.Errorf("image %q not found in enforced registries (errors: %v): %w", + image, registryErrors, ErrImageInvalid) + } + return fmt.Errorf("image %q not found in enforced registries: %w", image, ErrImageInvalid) +} + +func (*RegistryEnforcingValidator) hasEnforcingRegistry(mcpRegistryList *mcpv1alpha1.MCPRegistryList) bool { + for _, mcpRegistry := range mcpRegistryList.Items { + if mcpRegistry.Spec.Enforce { + return true + } + } + + return false +} + +// checkImageInRegistry checks if an image exists in a specific MCPRegistry +func (v *RegistryEnforcingValidator) checkImageInRegistry( + ctx context.Context, + mcpRegistry *mcpv1alpha1.MCPRegistry, + image string, +) (bool, error) { + // Only check registries that are ready + if mcpRegistry.Status.Phase != mcpv1alpha1.MCPRegistryPhaseReady { + return false, nil + } + + // Get the ConfigMap containing the registry data + configMapName := mcpRegistry.GetStorageName() + configMap := &corev1.ConfigMap{} + if err := v.client.Get(ctx, client.ObjectKey{ + Name: configMapName, + Namespace: v.namespace, + }, configMap); err != nil { + if k8serr.IsNotFound(err) { + // ConfigMap not found, registry data not available + return false, nil + } + return false, fmt.Errorf("failed to get ConfigMap %s: %w", configMapName, err) + } + + // Get the registry data from the ConfigMap + registryData, exists := configMap.Data["registry.json"] + if !exists { + // No registry data in ConfigMap + return false, nil + } + + // Parse the registry data + var reg registry.Registry + if err := json.Unmarshal([]byte(registryData), ®); err != nil { + // Invalid registry data + return false, fmt.Errorf("failed to parse registry data: %w", err) + } + + // Search for the image in this registry + return findImageInRegistry(®, image), nil +} + +// findImageInRegistry searches for an image in a registry +func findImageInRegistry(reg *registry.Registry, image string) bool { + // Check top-level servers + for _, server := range reg.Servers { + if server.Image == image { + return true + } + } + + // Check servers in groups + // TODO: check with Rado or Ria, is this needed? + for _, group := range reg.Groups { + for _, server := range group.Servers { + if server.Image == image { + return true + } + } + } + + return false +} diff --git a/cmd/thv-operator/pkg/validation/image_validation_test.go b/cmd/thv-operator/pkg/validation/image_validation_test.go new file mode 100644 index 000000000..4a6c98239 --- /dev/null +++ b/cmd/thv-operator/pkg/validation/image_validation_test.go @@ -0,0 +1,786 @@ +package validation + +import ( + "context" + "fmt" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + 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/fake" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/pkg/registry" +) + +func TestAlwaysAllowValidator(t *testing.T) { + t.Parallel() + + validator := &AlwaysAllowValidator{} + ctx := context.Background() + + tests := []struct { + name string + image string + }{ + { + name: "allows any image", + image: "docker.io/example/image:latest", + }, + { + name: "allows empty image", + image: "", + }, + { + name: "allows invalid image format", + image: "not-a-valid-image!!!", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := validator.ValidateImage(ctx, tt.image) + assert.ErrorIs(t, err, ErrImageNotChecked) + }) + } +} + +func TestNewImageValidator(t *testing.T) { + t.Parallel() + scheme := runtime.NewScheme() + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + tests := []struct { + name string + envValue string + expectedType string + setupEnv bool + }{ + { + name: "returns AlwaysAllowValidator when env not set", + envValue: "", + expectedType: "*validation.AlwaysAllowValidator", + setupEnv: false, + }, + { + name: "returns AlwaysAllowValidator when env is false", + envValue: "false", + expectedType: "*validation.AlwaysAllowValidator", + setupEnv: true, + }, + { + name: "returns RegistryEnforcingValidator when env is true", + envValue: "true", + expectedType: "*validation.RegistryEnforcingValidator", + setupEnv: true, + }, + { + name: "returns AlwaysAllowValidator for any other value", + envValue: "yes", + expectedType: "*validation.AlwaysAllowValidator", + setupEnv: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // Save original env value + originalValue := os.Getenv("TOOLHIVE_EXPERIMENTAL_REGISTRY_ENFORCEMENT") + defer func() { + os.Setenv("TOOLHIVE_EXPERIMENTAL_REGISTRY_ENFORCEMENT", originalValue) + }() + + // Set test env value + if tt.setupEnv { + os.Setenv("TOOLHIVE_EXPERIMENTAL_REGISTRY_ENFORCEMENT", tt.envValue) + } else { + os.Unsetenv("TOOLHIVE_EXPERIMENTAL_REGISTRY_ENFORCEMENT") + } + + var validationType ImageValidation + if tt.envValue == "true" { + validationType = ImageValidationRegistryEnforcing + } else { + validationType = ImageValidationAlwaysAllow + } + + validator := NewImageValidator(fakeClient, "test-namespace", validationType) + assert.NotNil(t, validator) + assert.Equal(t, tt.expectedType, fmt.Sprintf("%T", validator)) + }) + } +} + +func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + ctx := context.Background() + + // Test registry data + registryDataWithImage := `{ + "version": "1.0", + "servers": { + "test-server": { + "name": "test-server", + "image": "docker.io/toolhive/test:v1.0.0", + "description": "Test server" + }, + "another-server": { + "name": "another-server", + "image": "docker.io/toolhive/another:latest", + "description": "Another server" + } + }, + "groups": [ + { + "name": "group1", + "description": "Test group", + "servers": { + "group-server": { + "name": "group-server", + "image": "docker.io/toolhive/group:v2.0.0", + "description": "Group server" + } + } + } + ] + }` + + emptyRegistryData := `{ + "version": "1.0", + "servers": {} + }` + + tests := []struct { + name string + namespace string + image string + registries []runtime.Object + configMaps []runtime.Object + expectedValid bool + expectedError bool + expectedErrorMsg string + }{ + { + name: "no registries - validation passes", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + expectedValid: true, + }, + { + name: "registry without enforce - validation passes", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: false, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + expectedValid: true, + }, + { + name: "enforcing registry with image present - validation passes", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: true, + }, + { + name: "enforcing registry with image in group - validation passes", + namespace: "test-namespace", + image: "docker.io/toolhive/group:v2.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: true, + }, + { + name: "enforcing registry with image not present - validation fails", + namespace: "test-namespace", + image: "docker.io/toolhive/missing:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in enforced registries", + }, + { + name: "enforcing registry with empty registry data - validation fails", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": emptyRegistryData, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in enforced registries", + }, + { + name: "enforcing registry not ready - skips validation", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhasePending, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in enforced registries", + }, + { + name: "multiple registries with mixed enforce - image only in non-enforcing should fail", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "enforcing-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-enforcing-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: false, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "enforcing-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": emptyRegistryData, + }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-enforcing-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in enforced registries", + }, + { + name: "missing ConfigMap - enforcing registry without ConfigMap should fail", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-without-configmap", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-with-configmap", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: false, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-with-configmap-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in enforced registries", + }, + { + name: "invalid JSON in ConfigMap - enforcing registry with invalid JSON should fail", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-invalid-json", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-valid-json", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + Enforce: false, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-invalid-json-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": "not-valid-json{", + }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-valid-json-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in enforced registries", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // Build fake client with test objects + var objs []runtime.Object + objs = append(objs, tt.registries...) + objs = append(objs, tt.configMaps...) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(objs...). + Build() + + validator := &RegistryEnforcingValidator{ + client: fakeClient, + namespace: tt.namespace, + } + + err := validator.ValidateImage(ctx, tt.image) + + if tt.expectedValid { + // Validation should pass (no error or ErrImageNotChecked) + if err != nil { + assert.ErrorIs(t, err, ErrImageNotChecked) + } + } else { + // Validation should fail + if tt.expectedError { + assert.Error(t, err) + if tt.expectedErrorMsg != "" { + assert.Contains(t, err.Error(), tt.expectedErrorMsg) + } + } else { + assert.NoError(t, err) + } + } + }) + } +} + +func TestCheckImageInRegistry(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + ctx := context.Background() + + registryData := `{ + "version": "1.0", + "servers": { + "test-server": { + "name": "test-server", + "image": "docker.io/toolhive/test:v1.0.0", + "description": "Test server" + } + } + }` + + tests := []struct { + name string + mcpRegistry *mcpv1alpha1.MCPRegistry + configMap *corev1.ConfigMap + image string + expectedFound bool + }{ + { + name: "registry not ready - returns false", + mcpRegistry: &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhasePending, + }, + }, + image: "docker.io/toolhive/test:v1.0.0", + expectedFound: false, + }, + { + name: "ConfigMap not found - returns false", + mcpRegistry: &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + image: "docker.io/toolhive/test:v1.0.0", + expectedFound: false, + }, + { + name: "registry data not in ConfigMap - returns false", + mcpRegistry: &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "other-key": "some-data", + }, + }, + image: "docker.io/toolhive/test:v1.0.0", + expectedFound: false, + }, + { + name: "image found in registry - returns true", + mcpRegistry: &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryData, + }, + }, + image: "docker.io/toolhive/test:v1.0.0", + expectedFound: true, + }, + { + name: "image not found in registry - returns false", + mcpRegistry: &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry", + Namespace: "test-namespace", + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryData, + }, + }, + image: "docker.io/toolhive/missing:v1.0.0", + expectedFound: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var objs []runtime.Object + if tt.configMap != nil { + objs = append(objs, tt.configMap) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(objs...). + Build() + + validator := &RegistryEnforcingValidator{ + client: fakeClient, + namespace: "test-namespace", + } + + found, err := validator.checkImageInRegistry(ctx, tt.mcpRegistry, tt.image) + assert.NoError(t, err) + assert.Equal(t, tt.expectedFound, found) + }) + } +} + +func TestFindImageInRegistry(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + registry *registry.Registry + image string + expected bool + }{ + { + name: "finds image in top-level servers", + registry: ®istry.Registry{ + Servers: map[string]*registry.ImageMetadata{ + "server1": { + Image: "docker.io/toolhive/test:v1.0.0", + }, + "server2": { + Image: "docker.io/toolhive/other:v2.0.0", + }, + }, + }, + image: "docker.io/toolhive/test:v1.0.0", + expected: true, + }, + { + name: "finds image in group servers", + registry: ®istry.Registry{ + Servers: map[string]*registry.ImageMetadata{}, + Groups: []*registry.Group{ + { + Name: "group1", + Servers: map[string]*registry.ImageMetadata{ + "group-server": { + Image: "docker.io/toolhive/group:v1.0.0", + }, + }, + }, + }, + }, + image: "docker.io/toolhive/group:v1.0.0", + expected: true, + }, + { + name: "does not find missing image", + registry: ®istry.Registry{ + Servers: map[string]*registry.ImageMetadata{ + "server1": { + Image: "docker.io/toolhive/test:v1.0.0", + }, + }, + Groups: []*registry.Group{ + { + Name: "group1", + Servers: map[string]*registry.ImageMetadata{ + "group-server": { + Image: "docker.io/toolhive/group:v1.0.0", + }, + }, + }, + }, + }, + image: "docker.io/toolhive/missing:v1.0.0", + expected: false, + }, + { + name: "handles empty registry", + registry: ®istry.Registry{ + Servers: map[string]*registry.ImageMetadata{}, + }, + image: "docker.io/toolhive/test:v1.0.0", + expected: false, + }, + { + name: "handles nil maps", + registry: ®istry.Registry{}, + image: "docker.io/toolhive/test:v1.0.0", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := findImageInRegistry(tt.registry, tt.image) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/examples/operator/mcp-registries/mcpregistry-enforcing.yaml b/examples/operator/mcp-registries/mcpregistry-enforcing.yaml new file mode 100644 index 000000000..4936b6e4a --- /dev/null +++ b/examples/operator/mcp-registries/mcpregistry-enforcing.yaml @@ -0,0 +1,156 @@ +# Example MCPRegistry with enforcement enabled +# This demonstrates how to create a registry that enforces image validation +# When enforce: true, MCPServers in this namespace must exist in the registry +apiVersion: v1 +kind: ConfigMap +metadata: + name: enforcing-registry-data + namespace: toolhive-system +data: + registry.json: | + { + "$schema": "https://raw.githubusercontent.com/stacklok/toolhive/main/pkg/registry/data/schema.json", + "version": "1.0.0", + "last_updated": "2025-09-08T12:00:00Z", + "servers": { + "filesystem": { + "description": "Allows you to do filesystem operations", + "tier": "Official", + "status": "Active", + "transport": "stdio", + "tools": [ + "read_file", + "write_file", + "list_directory", + "create_directory" + ], + "metadata": { + "stars": 1500, + "pulls": 500, + "last_updated": "2025-09-08T12:00:00Z" + }, + "repository_url": "https://github.com/modelcontextprotocol/servers", + "tags": [ + "filesystem", + "approved" + ], + "image": "docker.io/mcp/filesystem:latest", + "permissions": { + "network": { + "outbound": {} + } + } + }, + "github": { + "description": "Provides integration with GitHub's APIs", + "tier": "Official", + "status": "Active", + "transport": "stdio", + "tools": [ + "create_issue", + "create_pull_request", + "search_repositories" + ], + "metadata": { + "stars": 2200, + "pulls": 800, + "last_updated": "2025-09-08T12:00:00Z" + }, + "repository_url": "https://github.com/github/github-mcp-server", + "tags": [ + "github", + "approved" + ], + "image": "ghcr.io/github/github-mcp-server:latest", + "permissions": { + "network": { + "outbound": { + "allow_host": [ + ".github.com", + ".githubusercontent.com" + ], + "allow_port": [ + 443 + ] + } + } + } + }, + "anthropic-tools": { + "description": "Anthropic's official MCP tools", + "tier": "Official", + "status": "Active", + "transport": "stdio", + "tools": [ + "analyze", + "generate", + "transform" + ], + "metadata": { + "stars": 5000, + "pulls": 2000, + "last_updated": "2025-09-08T12:00:00Z" + }, + "repository_url": "https://github.com/anthropics/mcp-tools", + "tags": [ + "anthropic", + "approved", + "official" + ], + "image": "docker.io/anthropic/mcp-tools:v1.0.0", + "permissions": { + "network": { + "outbound": {} + } + } + } + }, + "groups": [ + { + "name": "development-tools", + "description": "Development and coding tools", + "servers": { + "code-analyzer": { + "description": "Static code analysis tools", + "tier": "Community", + "status": "Active", + "transport": "stdio", + "tools": [ + "analyze_python", + "analyze_go", + "analyze_javascript" + ], + "tags": [ + "development", + "approved" + ], + "image": "docker.io/mcp/code-analyzer:v2.1.0", + "permissions": { + "network": { + "outbound": {} + } + } + } + } + } + ] + } +--- +apiVersion: toolhive.stacklok.dev/v1alpha1 +kind: MCPRegistry +metadata: + name: enforcing-registry + namespace: toolhive-system +spec: + displayName: "Approved MCP Servers (Enforcing)" + # When enforce is true, MCPServers in this namespace must exist in this registry + # Images allowed by this registry: + # - docker.io/mcp/filesystem:latest + # - ghcr.io/github/github-mcp-server:latest + # - docker.io/anthropic/mcp-tools:v1.0.0 + # - docker.io/mcp/code-analyzer:v2.1.0 (from the development-tools group) + enforce: true + source: + type: configmap + configmap: + name: enforcing-registry-data \ No newline at end of file From 4cb65456034544ee89cc726d8c92f07c52521b3d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 23 Sep 2025 18:13:09 +0200 Subject: [PATCH 3/9] Rename enforce to enforceServers --- .../api/v1alpha1/mcpregistry_types.go | 4 ++-- .../pkg/validation/image_validation.go | 2 +- .../pkg/validation/image_validation_test.go | 24 +++++++++---------- .../toolhive.stacklok.dev_mcpregistries.yaml | 4 ++-- docs/operator/crd-api.md | 2 +- .../mcp-registries/mcpregistry-enforcing.yaml | 6 ++--- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go index c8b276d75..6a568ba49 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go @@ -45,12 +45,12 @@ type MCPRegistrySpec struct { // +optional Filter *RegistryFilter `json:"filter,omitempty"` - // Enforce indicates whether MCPServers in this namespace must exist in this registry. + // EnforceServers indicates whether MCPServers in this namespace must exist in this registry. // When true, MCPServers that are not found in this registry will be rejected. // When false (default), MCPServers can be deployed regardless of registry presence. // +kubebuilder:default=false // +optional - Enforce bool `json:"enforce,omitempty"` + EnforceServers bool `json:"enforceServers,omitempty"` } // MCPRegistrySource defines the source configuration for registry data diff --git a/cmd/thv-operator/pkg/validation/image_validation.go b/cmd/thv-operator/pkg/validation/image_validation.go index 59d1548de..1f2a8fb1d 100644 --- a/cmd/thv-operator/pkg/validation/image_validation.go +++ b/cmd/thv-operator/pkg/validation/image_validation.go @@ -114,7 +114,7 @@ func (v *RegistryEnforcingValidator) ValidateImage(ctx context.Context, image st func (*RegistryEnforcingValidator) hasEnforcingRegistry(mcpRegistryList *mcpv1alpha1.MCPRegistryList) bool { for _, mcpRegistry := range mcpRegistryList.Items { - if mcpRegistry.Spec.Enforce { + if mcpRegistry.Spec.EnforceServers { return true } } diff --git a/cmd/thv-operator/pkg/validation/image_validation_test.go b/cmd/thv-operator/pkg/validation/image_validation_test.go index 4a6c98239..5e694d87d 100644 --- a/cmd/thv-operator/pkg/validation/image_validation_test.go +++ b/cmd/thv-operator/pkg/validation/image_validation_test.go @@ -191,7 +191,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: false, + EnforceServers: false, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -211,7 +211,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -242,7 +242,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -273,7 +273,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -306,7 +306,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -339,7 +339,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhasePending, @@ -361,7 +361,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -373,7 +373,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: false, + EnforceServers: false, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -415,7 +415,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -427,7 +427,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: false, + EnforceServers: false, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -460,7 +460,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: true, + EnforceServers: true, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, @@ -472,7 +472,7 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { Namespace: "test-namespace", }, Spec: mcpv1alpha1.MCPRegistrySpec{ - Enforce: false, + EnforceServers: false, }, Status: mcpv1alpha1.MCPRegistryStatus{ Phase: mcpv1alpha1.MCPRegistryPhaseReady, diff --git a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml index 624d05f4e..6aa30bb18 100644 --- a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml +++ b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml @@ -65,10 +65,10 @@ spec: displayName: description: DisplayName is a human-readable name for the registry type: string - enforce: + enforceServers: default: false description: |- - Enforce indicates whether MCPServers in this namespace must exist in this registry. + EnforceServers indicates whether MCPServers in this namespace must exist in this registry. When true, MCPServers that are not found in this registry will be rejected. When false (default), MCPServers can be deployed regardless of registry presence. type: boolean diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 80555aa7f..ec447e294 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -344,7 +344,7 @@ _Appears in:_ | `source` _[MCPRegistrySource](#mcpregistrysource)_ | Source defines the configuration for the registry data source | | Required: \{\}
| | `syncPolicy` _[SyncPolicy](#syncpolicy)_ | SyncPolicy defines the automatic synchronization behavior for the registry.
If specified, enables automatic synchronization at the given interval.
Manual synchronization is always supported via annotation-based triggers
regardless of this setting. | | | | `filter` _[RegistryFilter](#registryfilter)_ | Filter defines include/exclude patterns for registry content | | | -| `enforce` _boolean_ | Enforce indicates whether MCPServers in this namespace must exist in this registry.
When true, MCPServers that are not found in this registry will be rejected.
When false (default), MCPServers can be deployed regardless of registry presence. | false | | +| `enforceServers` _boolean_ | EnforceServers indicates whether MCPServers in this namespace must exist in this registry.
When true, MCPServers that are not found in this registry will be rejected.
When false (default), MCPServers can be deployed regardless of registry presence. | false | | #### MCPRegistryStatus diff --git a/examples/operator/mcp-registries/mcpregistry-enforcing.yaml b/examples/operator/mcp-registries/mcpregistry-enforcing.yaml index 4936b6e4a..798591000 100644 --- a/examples/operator/mcp-registries/mcpregistry-enforcing.yaml +++ b/examples/operator/mcp-registries/mcpregistry-enforcing.yaml @@ -1,6 +1,6 @@ # Example MCPRegistry with enforcement enabled # This demonstrates how to create a registry that enforces image validation -# When enforce: true, MCPServers in this namespace must exist in the registry +# When enforceServers: true, MCPServers in this namespace must exist in the registry apiVersion: v1 kind: ConfigMap metadata: @@ -143,13 +143,13 @@ metadata: namespace: toolhive-system spec: displayName: "Approved MCP Servers (Enforcing)" - # When enforce is true, MCPServers in this namespace must exist in this registry + # When enforceServers is true, MCPServers in this namespace must exist in this registry # Images allowed by this registry: # - docker.io/mcp/filesystem:latest # - ghcr.io/github/github-mcp-server:latest # - docker.io/anthropic/mcp-tools:v1.0.0 # - docker.io/mcp/code-analyzer:v2.1.0 (from the development-tools group) - enforce: true + enforceServers: true source: type: configmap configmap: From 749fbcbf9188fc0cf2efb478f05ab68c915e739a Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 23 Sep 2025 18:19:53 +0200 Subject: [PATCH 4/9] Clarify the documentation of the EnforceServers parameter --- cmd/thv-operator/api/v1alpha1/mcpregistry_types.go | 6 ++++-- .../crds/toolhive.stacklok.dev_mcpregistries.yaml | 6 ++++-- docs/operator/crd-api.md | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go index 6a568ba49..5147b7e70 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpregistry_types.go @@ -45,8 +45,10 @@ type MCPRegistrySpec struct { // +optional Filter *RegistryFilter `json:"filter,omitempty"` - // EnforceServers indicates whether MCPServers in this namespace must exist in this registry. - // When true, MCPServers that are not found in this registry will be rejected. + // EnforceServers indicates whether MCPServers in this namespace must have their images + // present in at least one registry in the namespace. When any registry in the namespace + // has this field set to true, enforcement is enabled for the entire namespace. + // MCPServers with images not found in any registry will be rejected. // When false (default), MCPServers can be deployed regardless of registry presence. // +kubebuilder:default=false // +optional diff --git a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml index 6aa30bb18..92f202439 100644 --- a/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml +++ b/deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpregistries.yaml @@ -68,8 +68,10 @@ spec: enforceServers: default: false description: |- - EnforceServers indicates whether MCPServers in this namespace must exist in this registry. - When true, MCPServers that are not found in this registry will be rejected. + EnforceServers indicates whether MCPServers in this namespace must have their images + present in at least one registry in the namespace. When any registry in the namespace + has this field set to true, enforcement is enabled for the entire namespace. + MCPServers with images not found in any registry will be rejected. When false (default), MCPServers can be deployed regardless of registry presence. type: boolean filter: diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index ec447e294..e637d3dcf 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -344,7 +344,7 @@ _Appears in:_ | `source` _[MCPRegistrySource](#mcpregistrysource)_ | Source defines the configuration for the registry data source | | Required: \{\}
| | `syncPolicy` _[SyncPolicy](#syncpolicy)_ | SyncPolicy defines the automatic synchronization behavior for the registry.
If specified, enables automatic synchronization at the given interval.
Manual synchronization is always supported via annotation-based triggers
regardless of this setting. | | | | `filter` _[RegistryFilter](#registryfilter)_ | Filter defines include/exclude patterns for registry content | | | -| `enforceServers` _boolean_ | EnforceServers indicates whether MCPServers in this namespace must exist in this registry.
When true, MCPServers that are not found in this registry will be rejected.
When false (default), MCPServers can be deployed regardless of registry presence. | false | | +| `enforceServers` _boolean_ | EnforceServers indicates whether MCPServers in this namespace must have their images
present in at least one registry in the namespace. When any registry in the namespace
has this field set to true, enforcement is enabled for the entire namespace.
MCPServers with images not found in any registry will be rejected.
When false (default), MCPServers can be deployed regardless of registry presence. | false | | #### MCPRegistryStatus From 164af652c232cea082733a5ecd0520f6bd2fbe06 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 23 Sep 2025 18:38:53 +0200 Subject: [PATCH 5/9] Allow instantiating MCPServer from a specific registry --- .../controllers/mcpserver_controller.go | 2 +- .../pkg/validation/image_validation.go | 71 +++- .../pkg/validation/image_validation_test.go | 332 +++++++++++++++++- 3 files changed, 399 insertions(+), 6 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index ccc845649..bd5343224 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -199,7 +199,7 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Validate MCPServer image against enforcing registries imageValidator := validation.NewImageValidator(r.Client, mcpServer.Namespace, r.ImageValidation) - err = imageValidator.ValidateImage(ctx, mcpServer.Spec.Image) + err = imageValidator.ValidateImage(ctx, mcpServer.Spec.Image, mcpServer.ObjectMeta) if goerr.Is(err, validation.ErrImageNotChecked) { ctxLogger.Info("Image validation skipped - no enforcement configured") // Set condition to indicate validation was skipped diff --git a/cmd/thv-operator/pkg/validation/image_validation.go b/cmd/thv-operator/pkg/validation/image_validation.go index 1f2a8fb1d..0511d8866 100644 --- a/cmd/thv-operator/pkg/validation/image_validation.go +++ b/cmd/thv-operator/pkg/validation/image_validation.go @@ -9,6 +9,7 @@ import ( corev1 "k8s.io/api/core/v1" k8serr "k8s.io/apimachinery/pkg/api/errors" + 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" @@ -35,24 +36,28 @@ const ( ImageValidationAlwaysAllow ImageValidation = "always-allow" // ImageValidationRegistryEnforcing indicates that images must be validated against MCPRegistry resources ImageValidationRegistryEnforcing ImageValidation = "registry-enforcing" + + // RegistryNameLabel is the label key used to specify which registry an MCPServer should use + RegistryNameLabel = "toolhive.stacklok.io/registry-name" ) // ImageValidator defines the interface for validating container images type ImageValidator interface { // ValidateImage checks if an image is valid for use. + // The metadata parameter contains MCPServer metadata (labels, annotations) that may affect validation. // Returns: // - nil if validation passes // - ErrImageNotChecked if no validation was performed // - wrapped ErrImageInvalid if image fails validation (with specific reason in error message) // - other errors for system/infrastructure failures - ValidateImage(ctx context.Context, image string) error + ValidateImage(ctx context.Context, image string, metadata metav1.ObjectMeta) error } // AlwaysAllowValidator is a no-op validator that always allows images type AlwaysAllowValidator struct{} // ValidateImage always returns ErrImageNotChecked, indicating no validation was performed -func (*AlwaysAllowValidator) ValidateImage(_ context.Context, _ string) error { +func (*AlwaysAllowValidator) ValidateImage(_ context.Context, _ string, _ metav1.ObjectMeta) error { return ErrImageNotChecked } @@ -74,13 +79,73 @@ type RegistryEnforcingValidator struct { } // ValidateImage checks if an image should be validated and if it exists in registries -func (v *RegistryEnforcingValidator) ValidateImage(ctx context.Context, image string) error { +// If the MCPServer has a registry-name label, validation is restricted to that specific registry. +// Otherwise, all registries are checked according to the original behavior. +func (v *RegistryEnforcingValidator) ValidateImage(ctx context.Context, image string, metadata metav1.ObjectMeta) error { + // Check if MCPServer specifies a specific registry to use + registryName, hasRegistryLabel := metadata.Labels[RegistryNameLabel] + // List all MCPRegistry resources in the namespace mcpRegistryList := &mcpv1alpha1.MCPRegistryList{} if err := v.client.List(ctx, mcpRegistryList, client.InNamespace(v.namespace)); err != nil { return fmt.Errorf("failed to list MCPRegistry resources: %w", err) } + if hasRegistryLabel { + // MCPServer specifies a specific registry - validate against that registry only + return v.validateAgainstSpecificRegistry(ctx, image, registryName, mcpRegistryList) + } + + // No specific registry specified - use original behavior (check all registries) + return v.validateAgainstAllRegistries(ctx, image, mcpRegistryList) +} + +// validateAgainstSpecificRegistry validates an image against a specific registry +func (v *RegistryEnforcingValidator) validateAgainstSpecificRegistry( + ctx context.Context, + image string, + registryName string, + mcpRegistryList *mcpv1alpha1.MCPRegistryList, +) error { + // Find the specified registry + var targetRegistry *mcpv1alpha1.MCPRegistry + for i := range mcpRegistryList.Items { + if mcpRegistryList.Items[i].Name == registryName { + targetRegistry = &mcpRegistryList.Items[i] + break + } + } + + if targetRegistry == nil { + return fmt.Errorf("specified registry %q not found: %w", registryName, ErrImageInvalid) + } + + // Check if the specified registry enforces validation + if !targetRegistry.Spec.EnforceServers { + // Registry exists but doesn't enforce - validation not performed + return ErrImageNotChecked + } + + // Check if image exists in the specified registry + found, err := v.checkImageInRegistry(ctx, targetRegistry, image) + if err != nil { + return fmt.Errorf("error checking image in registry %q: %v: %w", registryName, err, ErrImageInvalid) + } + + if !found { + return fmt.Errorf("image %q not found in specified registry %q: %w", image, registryName, ErrImageInvalid) + } + + // Image found in specified registry + return nil +} + +// validateAgainstAllRegistries validates an image against all registries (original behavior) +func (v *RegistryEnforcingValidator) validateAgainstAllRegistries( + ctx context.Context, + image string, + mcpRegistryList *mcpv1alpha1.MCPRegistryList, +) error { // Check if any registry enforces validation // If no enforcement required, return ErrImageNotChecked to indicate no validation was performed hasEnforcement := v.hasEnforcingRegistry(mcpRegistryList) diff --git a/cmd/thv-operator/pkg/validation/image_validation_test.go b/cmd/thv-operator/pkg/validation/image_validation_test.go index 5e694d87d..6ac473838 100644 --- a/cmd/thv-operator/pkg/validation/image_validation_test.go +++ b/cmd/thv-operator/pkg/validation/image_validation_test.go @@ -44,7 +44,9 @@ func TestAlwaysAllowValidator(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := validator.ValidateImage(ctx, tt.image) + // Create empty metadata for test + metadata := metav1.ObjectMeta{} + err := validator.ValidateImage(ctx, tt.image, metadata) assert.ErrorIs(t, err, ErrImageNotChecked) }) } @@ -523,7 +525,9 @@ func TestRegistryEnforcingValidator_ValidateImage(t *testing.T) { namespace: tt.namespace, } - err := validator.ValidateImage(ctx, tt.image) + // Create empty metadata for test (original behavior) + metadata := metav1.ObjectMeta{} + err := validator.ValidateImage(ctx, tt.image, metadata) if tt.expectedValid { // Validation should pass (no error or ErrImageNotChecked) @@ -784,3 +788,327 @@ func TestFindImageInRegistry(t *testing.T) { }) } } + +func TestRegistryEnforcingValidator_ValidateImageWithRegistryLabel(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + ctx := context.Background() + + // Test registry data + registryDataWithImage := `{ + "version": "1.0", + "servers": { + "test-server": { + "name": "test-server", + "image": "docker.io/toolhive/test:v1.0.0", + "description": "Test server" + } + } + }` + + tests := []struct { + name string + namespace string + image string + metadata metav1.ObjectMeta + registries []runtime.Object + configMaps []runtime.Object + expectedValid bool + expectedError bool + expectedErrorMsg string + }{ + { + name: "registry label points to enforcing registry with image - validation passes", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryNameLabel: "target-registry", + }, + }, + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "target-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "target-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: true, + }, + { + name: "registry label points to non-enforcing registry - validation skipped", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryNameLabel: "target-registry", + }, + }, + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "target-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: false, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + expectedValid: true, + }, + { + name: "registry label points to enforcing registry without image - validation fails", + namespace: "test-namespace", + image: "docker.io/toolhive/missing:v1.0.0", + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryNameLabel: "target-registry", + }, + }, + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "target-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "target-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in specified registry", + }, + { + name: "registry label points to non-existent registry - validation fails", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryNameLabel: "non-existent-registry", + }, + }, + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "different-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "specified registry \"non-existent-registry\" not found", + }, + { + name: "registry label with enforcing registry but image in different registry - validation fails", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + metadata: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryNameLabel: "empty-registry", + }, + }, + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-registry", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-with-image", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-registry-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": `{"version": "1.0", "servers": {}}`, + }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry-with-image-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: false, + expectedError: true, + expectedErrorMsg: "not found in specified registry \"empty-registry\"", + }, + { + name: "no registry label - falls back to original behavior (all registries)", + namespace: "test-namespace", + image: "docker.io/toolhive/test:v1.0.0", + metadata: metav1.ObjectMeta{}, // No registry label + registries: []runtime.Object{ + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry1", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + &mcpv1alpha1.MCPRegistry{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry2", + Namespace: "test-namespace", + }, + Spec: mcpv1alpha1.MCPRegistrySpec{ + EnforceServers: true, + }, + Status: mcpv1alpha1.MCPRegistryStatus{ + Phase: mcpv1alpha1.MCPRegistryPhaseReady, + }, + }, + }, + configMaps: []runtime.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry1-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": `{"version": "1.0", "servers": {}}`, + }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry2-registry-storage", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "registry.json": registryDataWithImage, + }, + }, + }, + expectedValid: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // Build fake client with test objects + var objs []runtime.Object + objs = append(objs, tt.registries...) + objs = append(objs, tt.configMaps...) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(objs...). + Build() + + validator := &RegistryEnforcingValidator{ + client: fakeClient, + namespace: tt.namespace, + } + + err := validator.ValidateImage(ctx, tt.image, tt.metadata) + + if tt.expectedValid { + // Validation should pass (no error or ErrImageNotChecked) + if err != nil { + assert.ErrorIs(t, err, ErrImageNotChecked) + } + } else { + // Validation should fail + if tt.expectedError { + assert.Error(t, err) + if tt.expectedErrorMsg != "" { + assert.Contains(t, err.Error(), tt.expectedErrorMsg) + } + } else { + assert.NoError(t, err) + } + } + }) + } +} From 9d2732e8d9832660e93cc2fc22727297dbb768aa Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 23 Sep 2025 18:47:42 +0200 Subject: [PATCH 6/9] Reduce the code duplication somewhat --- .../controllers/mcpserver_controller.go | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index bd5343224..706cd230b 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -203,23 +203,17 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if goerr.Is(err, validation.ErrImageNotChecked) { ctxLogger.Info("Image validation skipped - no enforcement configured") // Set condition to indicate validation was skipped - meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ - Type: mcpv1alpha1.ConditionImageValidated, - Status: metav1.ConditionTrue, - Reason: mcpv1alpha1.ConditionReasonImageValidationSkipped, - Message: "Image validation was not performed (no enforcement configured)", - }) + setImageValidationCondition(mcpServer, metav1.ConditionTrue, + mcpv1alpha1.ConditionReasonImageValidationSkipped, + "Image validation was not performed (no enforcement configured)") } else if goerr.Is(err, validation.ErrImageInvalid) { ctxLogger.Error(err, "MCPServer image validation failed", "image", mcpServer.Spec.Image) // Update status to reflect validation failure mcpServer.Status.Phase = mcpv1alpha1.MCPServerPhaseFailed mcpServer.Status.Message = err.Error() // Gets the specific validation failure reason - meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ - Type: mcpv1alpha1.ConditionImageValidated, - Status: metav1.ConditionFalse, - Reason: mcpv1alpha1.ConditionReasonImageValidationFailed, - Message: err.Error(), // This will include the wrapped error context with specific reason - }) + setImageValidationCondition(mcpServer, metav1.ConditionFalse, + mcpv1alpha1.ConditionReasonImageValidationFailed, + err.Error()) // This will include the wrapped error context with specific reason if statusErr := r.Status().Update(ctx, mcpServer); statusErr != nil { ctxLogger.Error(statusErr, "Failed to update MCPServer status after validation error") } @@ -228,12 +222,9 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } else if err != nil { // Other system/infrastructure errors ctxLogger.Error(err, "MCPServer image validation system error", "image", mcpServer.Spec.Image) - meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ - Type: mcpv1alpha1.ConditionImageValidated, - Status: metav1.ConditionFalse, - Reason: mcpv1alpha1.ConditionReasonImageValidationError, - Message: fmt.Sprintf("Error checking image validity: %v", err), - }) + setImageValidationCondition(mcpServer, metav1.ConditionFalse, + mcpv1alpha1.ConditionReasonImageValidationError, + fmt.Sprintf("Error checking image validity: %v", err)) if statusErr := r.Status().Update(ctx, mcpServer); statusErr != nil { ctxLogger.Error(statusErr, "Failed to update MCPServer status after validation error") } @@ -242,12 +233,9 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } else { // Validation passed ctxLogger.Info("Image validation passed", "image", mcpServer.Spec.Image) - meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ - Type: mcpv1alpha1.ConditionImageValidated, - Status: metav1.ConditionTrue, - Reason: mcpv1alpha1.ConditionReasonImageValidationSuccess, - Message: "Image validation passed - image found in enforced registries", - }) + setImageValidationCondition(mcpServer, metav1.ConditionTrue, + mcpv1alpha1.ConditionReasonImageValidationSuccess, + "Image validation passed - image found in enforced registries") } // Check if the MCPServer instance is marked to be deleted @@ -407,6 +395,17 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, nil } +// setImageValidationCondition is a helper function to set the image validation status condition +// This reduces code duplication in the image validation logic +func setImageValidationCondition(mcpServer *mcpv1alpha1.MCPServer, status metav1.ConditionStatus, reason, message string) { + meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionImageValidated, + Status: status, + Reason: reason, + Message: message, + }) +} + // handleRestartAnnotation checks if the restart annotation has been updated and triggers a restart if needed // Returns true if a restart was triggered and the reconciliation should be requeued func (r *MCPServerReconciler) handleRestartAnnotation(ctx context.Context, mcpServer *mcpv1alpha1.MCPServer) (bool, error) { From 3f50de8cbd05d4efa11030fad774445f3c391df2 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 23 Sep 2025 18:53:05 +0200 Subject: [PATCH 7/9] Filter the list of non-enforcing registries --- .../pkg/validation/image_validation.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/cmd/thv-operator/pkg/validation/image_validation.go b/cmd/thv-operator/pkg/validation/image_validation.go index 0511d8866..c623cb594 100644 --- a/cmd/thv-operator/pkg/validation/image_validation.go +++ b/cmd/thv-operator/pkg/validation/image_validation.go @@ -146,16 +146,15 @@ func (v *RegistryEnforcingValidator) validateAgainstAllRegistries( image string, mcpRegistryList *mcpv1alpha1.MCPRegistryList, ) error { - // Check if any registry enforces validation - // If no enforcement required, return ErrImageNotChecked to indicate no validation was performed - hasEnforcement := v.hasEnforcingRegistry(mcpRegistryList) - if !hasEnforcement { + // Get only enforcing registries for efficient processing + enforcingRegistries := v.getEnforcingRegistries(mcpRegistryList) + if len(enforcingRegistries) == 0 { return ErrImageNotChecked } - // Enforcement is required, check each registry for the image + // Enforcement is required, check each enforcing registry for the image var registryErrors []string - for _, mcpRegistry := range mcpRegistryList.Items { + for _, mcpRegistry := range enforcingRegistries { found, err := v.checkImageInRegistry(ctx, &mcpRegistry, image) if err != nil { // Collect errors but continue checking other registries @@ -177,14 +176,14 @@ func (v *RegistryEnforcingValidator) validateAgainstAllRegistries( return fmt.Errorf("image %q not found in enforced registries: %w", image, ErrImageInvalid) } -func (*RegistryEnforcingValidator) hasEnforcingRegistry(mcpRegistryList *mcpv1alpha1.MCPRegistryList) bool { +func (*RegistryEnforcingValidator) getEnforcingRegistries(mcpRegistryList *mcpv1alpha1.MCPRegistryList) []mcpv1alpha1.MCPRegistry { + var enforcingRegistries []mcpv1alpha1.MCPRegistry for _, mcpRegistry := range mcpRegistryList.Items { if mcpRegistry.Spec.EnforceServers { - return true + enforcingRegistries = append(enforcingRegistries, mcpRegistry) } } - - return false + return enforcingRegistries } // checkImageInRegistry checks if an image exists in a specific MCPRegistry From e4325975e7ca04d89e2e949856a7b10622f637a3 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 24 Sep 2025 15:58:23 +0200 Subject: [PATCH 8/9] Bump charts --- deploy/charts/operator-crds/Chart.yaml | 2 +- deploy/charts/operator-crds/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/deploy/charts/operator-crds/Chart.yaml b/deploy/charts/operator-crds/Chart.yaml index ae38f7d51..dc9b59297 100644 --- a/deploy/charts/operator-crds/Chart.yaml +++ b/deploy/charts/operator-crds/Chart.yaml @@ -2,5 +2,5 @@ apiVersion: v2 name: toolhive-operator-crds description: A Helm chart for installing the ToolHive Operator CRDs into Kubernetes. type: application -version: 0.0.29 +version: 0.0.30 appVersion: "0.0.1" diff --git a/deploy/charts/operator-crds/README.md b/deploy/charts/operator-crds/README.md index 9acb175c4..87dc98e92 100644 --- a/deploy/charts/operator-crds/README.md +++ b/deploy/charts/operator-crds/README.md @@ -1,7 +1,7 @@ # ToolHive Operator CRDs Helm Chart -![Version: 0.0.29](https://img.shields.io/badge/Version-0.0.29-informational?style=flat-square) +![Version: 0.0.30](https://img.shields.io/badge/Version-0.0.30-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) A Helm chart for installing the ToolHive Operator CRDs into Kubernetes. From c6c0efb9c62387c7bc89b48ebd9ba87b5168f3f2 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 24 Sep 2025 16:39:18 +0200 Subject: [PATCH 9/9] fix lint --- cmd/thv-operator/pkg/validation/image_validation.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/thv-operator/pkg/validation/image_validation.go b/cmd/thv-operator/pkg/validation/image_validation.go index c623cb594..e11c4521c 100644 --- a/cmd/thv-operator/pkg/validation/image_validation.go +++ b/cmd/thv-operator/pkg/validation/image_validation.go @@ -176,7 +176,9 @@ func (v *RegistryEnforcingValidator) validateAgainstAllRegistries( return fmt.Errorf("image %q not found in enforced registries: %w", image, ErrImageInvalid) } -func (*RegistryEnforcingValidator) getEnforcingRegistries(mcpRegistryList *mcpv1alpha1.MCPRegistryList) []mcpv1alpha1.MCPRegistry { +func (*RegistryEnforcingValidator) getEnforcingRegistries( + mcpRegistryList *mcpv1alpha1.MCPRegistryList, +) []mcpv1alpha1.MCPRegistry { var enforcingRegistries []mcpv1alpha1.MCPRegistry for _, mcpRegistry := range mcpRegistryList.Items { if mcpRegistry.Spec.EnforceServers {