diff --git a/.gitignore b/.gitignore index b1602e0..34897d4 100644 --- a/.gitignore +++ b/.gitignore @@ -38,4 +38,7 @@ go.work.sum # Build output /bin/ /dist/ -/coverage/ \ No newline at end of file +/coverage/ + +# data folder created by local examples +data/ diff --git a/CLAUDE.md b/CLAUDE.md index b32bb7b..8cf05e0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,9 +37,8 @@ The codebase follows clean architecture with three layers: - Provider abstraction with factory pattern 3. **Provider Layer** (backends for registry data) - - `FileRegistryDataProvider` - Local file backend - - `K8sRegistryDataProvider` - Kubernetes ConfigMap backend - - `K8sDeploymentProvider` - Queries deployed instances + - `FileRegistryDataProvider` - Local file backend (reads synced data) + - `K8sDeploymentProvider` - Queries deployed MCP server instances ### Key Patterns for AI Development diff --git a/README.md b/README.md index cc0995e..29db0ed 100644 --- a/README.md +++ b/README.md @@ -26,10 +26,11 @@ The ToolHive Registry API (`thv-registry-api`) implements the official [Model Co ## Features - **Standards-compliant**: Implements the official MCP Registry API specification -- **Multiple backends**: Supports Kubernetes ConfigMaps and file-based registry data +- **Multiple data sources**: Git repositories, Kubernetes ConfigMaps, API endpoints, and local files +- **Automatic synchronization**: Background sync with configurable intervals and retry logic - **Container-ready**: Designed for deployment in Kubernetes clusters - **Flexible deployment**: Works standalone or as part of ToolHive infrastructure -- **Production-ready**: Built-in health checks, graceful shutdown, and basic observability +- **Production-ready**: Built-in health checks, graceful shutdown, and sync status persistence ## Quick Start @@ -37,7 +38,7 @@ The ToolHive Registry API (`thv-registry-api`) implements the official [Model Co - Go 1.23 or later - [Task](https://taskfile.dev) for build automation -- Access to a Kubernetes cluster (for ConfigMap backend) +- Access to a Kubernetes cluster (for ConfigMap data source) ### Building the binary @@ -48,22 +49,33 @@ task build ### Running the Server -**From a Kubernetes ConfigMap:** +All configuration is done via YAML configuration files. See the [examples/](examples/) directory for sample configurations. + +**Quick start with Git source:** ```bash -thv-registry-api serve \ - --from-configmap my-registry-cm \ - --registry-name my-registry +thv-registry-api serve --config examples/config-git.yaml ``` -**From a local file:** +**With Kubernetes ConfigMap:** ```bash -thv-registry-api serve \ - --from-file /path/to/registry.json \ - --registry-name my-registry +thv-registry-api serve --config examples/config-configmap.yaml +``` + +**With local file:** +```bash +thv-registry-api serve --config examples/config-file.yaml ``` The server starts on port 8080 by default. Use `--address :PORT` to customize. +**What happens when the server starts:** +1. Loads configuration from the specified YAML file +2. Immediately fetches registry data from the configured source +3. Starts background sync coordinator for automatic updates +4. Serves MCP Registry API endpoints on the configured address + +For detailed configuration options and examples, see the [examples/README.md](examples/README.md). + ## API Endpoints The server implements the standard MCP Registry API: @@ -78,32 +90,75 @@ See the [MCP Registry API specification](https://github.com/modelcontextprotocol ## Configuration -### Command-line Flags +All configuration is done via YAML files. The server requires a `--config` flag pointing to a YAML configuration file. + +### Configuration File Structure + +```yaml +# Registry name/identifier (optional, defaults to "default") +registryName: my-registry + +# Data source configuration (required) +source: + # Source type: git, configmap, api, or file + type: git + + # Data format: toolhive (native) or upstream (MCP registry format) + format: toolhive + + # Source-specific configuration + git: + repository: https://github.com/stacklok/toolhive.git + branch: main + path: pkg/registry/data/registry.json + +# Automatic sync policy (required) +syncPolicy: + # Sync interval (e.g., "30m", "1h", "24h") + interval: "30m" + +# Optional: Server filtering +filter: + names: + include: ["official/*"] + exclude: ["*/deprecated"] + tags: + include: ["production"] + exclude: ["experimental"] +``` -The `thv-registry-api serve` command supports the following flags: +### Command-line Flags | Flag | Description | Required | Default | |------|-------------|----------|---------| +| `--config` | Path to YAML configuration file | Yes | - | | `--address` | Server listen address | No | `:8080` | -| `--from-configmap` | ConfigMap name containing registry data | Yes* | - | -| `--from-file` | Path to registry.json file | Yes* | - | -| `--registry-name` | Registry identifier | Yes | - | -*One of `--from-configmap` or `--from-file` must be specified (mutually exclusive) +### Data Sources + +The server supports four data source types: + +1. **Git Repository** - Clone and sync from Git repositories + - Supports branch, tag, or commit pinning + - Ideal for version-controlled registries + - Example: [config-git.yaml](examples/config-git.yaml) + +2. **Kubernetes ConfigMap** - Read from ConfigMaps in the cluster + - Requires Kubernetes API access and RBAC permissions + - Ideal for Kubernetes-native deployments + - Example: [config-configmap.yaml](examples/config-configmap.yaml) -### Backend Options +3. **API Endpoint** - Sync from upstream MCP Registry APIs + - Supports federation and aggregation scenarios + - Format conversion from upstream to ToolHive format + - Example: [config-api.yaml](examples/config-api.yaml) -#### ConfigMap Backend -Fetches registry data from a Kubernetes ConfigMap. Requires: -- Kubernetes API access (in-cluster or via kubeconfig) -- ConfigMap with a `registry.json` key containing MCP registry data -- Appropriate RBAC permissions to read ConfigMaps +4. **Local File** - Read from filesystem + - Ideal for local development and testing + - Supports mounted volumes in containers + - Example: [config-file.yaml](examples/config-file.yaml) -#### File Backend -Reads registry data from a local file. Useful for: -- Mounting ConfigMaps as volumes in Kubernetes -- Local development and testing -- Static registry deployments +For complete configuration examples and advanced options, see [examples/README.md](examples/README.md). ## Development @@ -133,25 +188,47 @@ task build-image ``` cmd/thv-registry-api/ -├── api/v1/ # REST API handlers and routes +├── api/ # REST API implementation +│ └── v1/ # API v1 handlers and routes ├── app/ # CLI commands and application setup -├── internal/service/ # Business logic and data providers -│ ├── file_provider.go # File-based registry backend -│ ├── k8s_provider.go # Kubernetes ConfigMap backend -│ ├── provider.go # Provider interfaces +├── internal/service/ # Legacy service layer (being refactored) +│ ├── file_provider.go # File-based registry provider +│ ├── k8s_provider.go # Kubernetes provider │ └── service.go # Core service implementation └── main.go # Application entry point + +pkg/ +├── config/ # Configuration loading and validation +├── sources/ # Data source handlers +│ ├── git.go # Git repository source +│ ├── configmap.go # Kubernetes ConfigMap source +│ ├── api.go # API endpoint source +│ ├── file.go # File system source +│ ├── factory.go # Source handler factory +│ └── storage_manager.go # Storage abstraction +├── sync/ # Sync manager and coordination +│ └── manager.go # Background sync logic +└── status/ # Sync status tracking + └── persistence.go # Status file persistence + +examples/ # Example configurations ``` ### Architecture -The server follows a clean architecture pattern: +The server follows a clean architecture pattern with the following layers: -1. **API Layer** (`api/v1`): HTTP handlers implementing the MCP Registry API -2. **Service Layer** (`internal/service`): Business logic for registry operations -3. **Provider Layer**: Pluggable backends for registry data sources - - `FileRegistryDataProvider`: Reads from local files - - `K8sRegistryDataProvider`: Fetches from Kubernetes ConfigMaps +1. **API Layer** (`cmd/thv-registry-api/api`): HTTP handlers implementing the MCP Registry API +2. **Service Layer** (`cmd/thv-registry-api/internal/service`): Legacy business logic (being refactored) +3. **Configuration Layer** (`pkg/config`): YAML configuration loading and validation +4. **Source Handler Layer** (`pkg/sources`): Pluggable data source implementations + - `GitSourceHandler`: Clones Git repositories and extracts registry files + - `ConfigMapSourceHandler`: Reads from Kubernetes ConfigMaps + - `APISourceHandler`: Fetches from upstream MCP Registry APIs + - `FileSourceHandler`: Reads from local filesystem +5. **Sync Manager** (`pkg/sync`): Coordinates automatic registry synchronization +6. **Storage Layer** (`pkg/sources`): Persists registry data to local storage +7. **Status Tracking** (`pkg/status`): Tracks and persists sync status ### Testing @@ -184,10 +261,33 @@ spec: image: ghcr.io/stacklok/toolhive/thv-registry-api:latest args: - serve - - --from-configmap=my-registry - - --registry-name=my-registry + - --config=/etc/registry/config.yaml ports: - containerPort: 8080 + volumeMounts: + - name: config + mountPath: /etc/registry + volumes: + - name: config + configMap: + name: registry-api-config +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: registry-api-config +data: + config.yaml: | + registryName: my-registry + source: + type: configmap + format: toolhive + configmap: + namespace: toolhive-system + name: my-registry-data + key: registry.json + syncPolicy: + interval: "15m" ``` ### Docker @@ -196,10 +296,16 @@ spec: # Build the image task build-image -# Run with file backend -docker run -v /path/to/registry.json:/data/registry.json \ +# Run with Git source +docker run -v $(pwd)/examples:/config \ + ghcr.io/stacklok/toolhive/thv-registry-api:latest \ + serve --config /config/config-git.yaml + +# Run with file source (mount local registry file) +docker run -v $(pwd)/examples:/config \ + -v /path/to/registry.json:/data/registry.json \ ghcr.io/stacklok/toolhive/thv-registry-api:latest \ - serve --from-file /data/registry.json --registry-name my-registry + serve --config /config/config-file.yaml ``` ## Integration with ToolHive diff --git a/cmd/thv-registry-api/app/serve.go b/cmd/thv-registry-api/app/serve.go index 4d78b28..f86562e 100644 --- a/cmd/thv-registry-api/app/serve.go +++ b/cmd/thv-registry-api/app/serve.go @@ -2,26 +2,19 @@ package app import ( "context" - "errors" "fmt" - "net/http" "os" "os/signal" "syscall" "time" - "github.com/go-chi/chi/v5/middleware" "github.com/spf13/cobra" "github.com/spf13/viper" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + registryapp "github.com/stacklok/toolhive-registry-server/pkg/app" "github.com/stacklok/toolhive-registry-server/pkg/config" - - "github.com/stacklok/toolhive-registry-server/internal/api" - "github.com/stacklok/toolhive-registry-server/internal/service" - thvk8scli "github.com/stacklok/toolhive/pkg/container/kubernetes" "github.com/stacklok/toolhive/pkg/logger" ) @@ -29,29 +22,23 @@ var serveCmd = &cobra.Command{ Use: "serve", Short: "Start the registry API server", Long: `Start the registry API server to serve MCP registry data. -The server can read registry data from either: -- ConfigMaps using --from-configmap flag (requires Kubernetes API access) -- Local files using --from-file flag (for mounted ConfigMaps) -Both options require --registry-name to specify the registry identifier. -One of --from-configmap or --from-file must be specified.`, +The server requires a configuration file (--config) that specifies: +- Registry name and data source (Git, ConfigMap, API, or File) +- Sync policy and filtering rules +- All other operational settings + +See examples/ directory for sample configurations.`, RunE: runServe, } const ( defaultGracefulTimeout = 30 * time.Second // Kubernetes-friendly shutdown time - serverRequestTimeout = 10 * time.Second // Registry API should respond quickly - serverReadTimeout = 10 * time.Second // Enough for headers and small requests - serverWriteTimeout = 15 * time.Second // Must be > serverRequestTimeout to let middleware handle timeout - serverIdleTimeout = 60 * time.Second // Keep connections alive for reuse ) func init() { serveCmd.Flags().String("address", ":8080", "Address to listen on") - serveCmd.Flags().String("config", "", "Path to configuration file (YAML format)") - serveCmd.Flags().String("from-configmap", "", "ConfigMap name containing registry data (mutually exclusive with --from-file)") - serveCmd.Flags().String("from-file", "", "File path to registry.json (mutually exclusive with --from-configmap)") - serveCmd.Flags().String("registry-name", "", "Registry name identifier (required)") + serveCmd.Flags().String("config", "", "Path to configuration file (YAML format, required)") err := viper.BindPFlag("address", serveCmd.Flags().Lookup("address")) if err != nil { @@ -61,170 +48,45 @@ func init() { if err != nil { logger.Fatalf("Failed to bind config flag: %v", err) } - err = viper.BindPFlag("from-configmap", serveCmd.Flags().Lookup("from-configmap")) - if err != nil { - logger.Fatalf("Failed to bind from-configmap flag: %v", err) - } - err = viper.BindPFlag("from-file", serveCmd.Flags().Lookup("from-file")) - if err != nil { - logger.Fatalf("Failed to bind from-file flag: %v", err) - } - err = viper.BindPFlag("registry-name", serveCmd.Flags().Lookup("registry-name")) - if err != nil { - logger.Fatalf("Failed to bind registry-name flag: %v", err) - } -} -// getKubernetesConfig returns a Kubernetes REST config -func getKubernetesConfig() (*rest.Config, error) { - // Try in-cluster config first - config, err := rest.InClusterConfig() - if err == nil { - return config, nil + // Mark config as required + if err := serveCmd.MarkFlagRequired("config"); err != nil { + logger.Fatalf("Failed to mark config flag as required: %v", err) } - - // Fall back to kubeconfig - loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() - configOverrides := &clientcmd.ConfigOverrides{} - kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides) - return kubeConfig.ClientConfig() -} - -// buildProviderConfig creates provider configuration based on command-line flags -func buildProviderConfig() (*service.RegistryProviderConfig, error) { - configMapName := viper.GetString("from-configmap") - filePath := viper.GetString("from-file") - registryName := viper.GetString("registry-name") - - // Validate mutual exclusivity - if configMapName != "" && filePath != "" { - return nil, fmt.Errorf("--from-configmap and --from-file flags are mutually exclusive") - } - - // Require one of the flags - if configMapName == "" && filePath == "" { - return nil, fmt.Errorf("either --from-configmap or --from-file flag is required") - } - - // Require registry name - if registryName == "" { - return nil, fmt.Errorf("--registry-name flag is required") - } - - if configMapName != "" { - config, err := getKubernetesConfig() - if err != nil { - return nil, fmt.Errorf("failed to create kubernetes config: %w", err) - } - - clientset, err := kubernetes.NewForConfig(config) - if err != nil { - return nil, fmt.Errorf("failed to create kubernetes client: %w", err) - } - - return &service.RegistryProviderConfig{ - Type: service.RegistryProviderTypeConfigMap, - ConfigMap: &service.ConfigMapProviderConfig{ - Name: configMapName, - Namespace: thvk8scli.GetCurrentNamespace(), - Clientset: clientset, - RegistryName: registryName, - }, - }, nil - } - - return &service.RegistryProviderConfig{ - Type: service.RegistryProviderTypeFile, - File: &service.FileProviderConfig{ - FilePath: filePath, - RegistryName: registryName, - }, - }, nil } func runServe(_ *cobra.Command, _ []string) error { ctx := context.Background() - address := viper.GetString("address") - logger.Infof("Starting registry API server on %s", address) + // Initialize controller-runtime logger to suppress warnings + log.SetLogger(zap.New(zap.UseDevMode(false))) + // Load and validate configuration configPath := viper.GetString("config") - if configPath != "" { - // TODO: Use the configuration - // TODO: Validate the path to avoid path traversal issues - c, err := config.NewConfigLoader().LoadConfig(configPath) - if err := c.Validate(); err != nil { - return fmt.Errorf("invalid configuration: %w", err) - } - if err != nil { - return fmt.Errorf("failed to load configuration: %w", err) - } - logger.Infof("Loaded configuration from %s", configPath) - } - - providerConfig, err := buildProviderConfig() - if err != nil { - return fmt.Errorf("failed to build provider configuration: %w", err) - } - - if err := providerConfig.Validate(); err != nil { - return fmt.Errorf("invalid provider configuration: %w", err) - } - - factory := service.NewRegistryProviderFactory() - registryProvider, err := factory.CreateProvider(providerConfig) - if err != nil { - return fmt.Errorf("failed to create registry provider: %w", err) - } - - logger.Infof("Created registry data provider: %s", registryProvider.GetSource()) - - var deploymentProvider service.DeploymentProvider - config, err := getKubernetesConfig() + // TODO: Validate the path to avoid path traversal issues + cfg, err := config.NewConfigLoader().LoadConfig(configPath) if err != nil { - return fmt.Errorf("failed to create kubernetes config for deployment provider: %w", err) + return fmt.Errorf("failed to load configuration: %w", err) } - // Use registry name from provider - registryName := registryProvider.GetRegistryName() - - deploymentProvider, err = service.NewK8sDeploymentProvider(config, registryName) - if err != nil { - return fmt.Errorf("failed to create kubernetes deployment provider: %w", err) - } - logger.Infof("Created Kubernetes deployment provider for registry: %s", registryName) + logger.Infof("Loaded configuration from %s (registry: %s, source: %s)", + configPath, cfg.GetRegistryName(), cfg.Source.Type) - // Create the registry service - svc, err := service.NewService(ctx, registryProvider, deploymentProvider) + // Build application using the builder pattern + address := viper.GetString("address") + app, err := registryapp.NewRegistryAppBuilder(cfg). + WithAddress(address). + Build(ctx) if err != nil { - return fmt.Errorf("failed to create registry service: %w", err) + return fmt.Errorf("failed to build application: %w", err) } - // Create the registry server with middleware - router := api.NewServer(svc, - api.WithMiddlewares( - middleware.RequestID, - middleware.RealIP, - middleware.Recoverer, - middleware.Timeout(serverRequestTimeout), - api.LoggingMiddleware, - ), - ) - - // Create HTTP server - server := &http.Server{ - Addr: address, - Handler: router, - ReadTimeout: serverReadTimeout, - WriteTimeout: serverWriteTimeout, - IdleTimeout: serverIdleTimeout, - } + logger.Infof("Starting registry API server on %s", address) - // Start server in goroutine + // Start application in goroutine go func() { - logger.Infof("Server listening on %s", address) - if err := server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { - logger.Fatalf("Failed to start server: %v", err) + if err := app.Start(); err != nil { + logger.Fatalf("Application failed: %v", err) } }() @@ -232,17 +94,7 @@ func runServe(_ *cobra.Command, _ []string) error { quit := make(chan os.Signal, 1) signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM) <-quit - logger.Info("Shutting down server...") - - // Graceful shutdown with timeout - ctx, cancel := context.WithTimeout(context.Background(), defaultGracefulTimeout) - defer cancel() - - if err := server.Shutdown(ctx); err != nil { - logger.Errorf("Server forced to shutdown: %v", err) - return err - } - logger.Info("Server shutdown complete") - return nil + // Graceful shutdown + return app.Stop(defaultGracefulTimeout) } diff --git a/docs/cli/thv-registry-api_serve.md b/docs/cli/thv-registry-api_serve.md index 4f18ce8..9550208 100644 --- a/docs/cli/thv-registry-api_serve.md +++ b/docs/cli/thv-registry-api_serve.md @@ -16,12 +16,13 @@ Start the registry API server ### Synopsis Start the registry API server to serve MCP registry data. -The server can read registry data from either: -- ConfigMaps using --from-configmap flag (requires Kubernetes API access) -- Local files using --from-file flag (for mounted ConfigMaps) -Both options require --registry-name to specify the registry identifier. -One of --from-configmap or --from-file must be specified. +The server requires a configuration file (--config) that specifies: +- Registry name and data source (Git, ConfigMap, API, or File) +- Sync policy and filtering rules +- All other operational settings + +See examples/ directory for sample configurations. ``` thv-registry-api serve [flags] @@ -30,12 +31,9 @@ thv-registry-api serve [flags] ### Options ``` - --address string Address to listen on (default ":8080") - --config string Path to configuration file (YAML format) - --from-configmap string ConfigMap name containing registry data (mutually exclusive with --from-file) - --from-file string File path to registry.json (mutually exclusive with --from-configmap) - -h, --help help for serve - --registry-name string Registry name identifier (required) + --address string Address to listen on (default ":8080") + --config string Path to configuration file (YAML format, required) + -h, --help help for serve ``` ### Options inherited from parent commands diff --git a/examples/README.md b/examples/README.md new file mode 100644 index 0000000..35be8fb --- /dev/null +++ b/examples/README.md @@ -0,0 +1,589 @@ +# ToolHive Registry API - Configuration Examples + +This directory contains sample configuration files demonstrating automatic registry synchronization from different data sources. + +## Quick Start + +**Choose your data source:** + +| Source | Use Case | Config File | Sync Interval | +|--------|----------|-------------|---------------| +| **Git** | Official registries, version control | [config-git.yaml](config-git.yaml) | 30m | +| **ConfigMap** | Kubernetes-native, internal data | [config-configmap.yaml](config-configmap.yaml) | 15m | +| **API** | Upstream aggregation, federation | [config-api.yaml](config-api.yaml) | 1h | +| **File** | Local development, testing | [config-file.yaml](config-file.yaml) | 5m | + +**Start the server with sync:** + +```bash +# Git source (recommended for getting started) +thv-registry-api serve --config examples/config-git.yaml + +# ConfigMap source (Kubernetes) +thv-registry-api serve --config examples/config-configmap.yaml + +# API source (upstream MCP registry) +thv-registry-api serve --config examples/config-api.yaml + +# File source (local development) +thv-registry-api serve --config examples/config-file.yaml +``` + +**Verify sync is working:** + +```bash +# Check sync status +cat ./data/status.json | jq '.phase, .serverCount, .lastSyncTime' + +# View synced registry data +cat ./data/registry.json | jq '.servers | keys' + +# Query the API +curl http://localhost:8080/api/v0/servers | jq +``` + +--- + +## Configuration Files + +### 1. Git Repository Source + +**File:** [config-git.yaml](config-git.yaml) + +Syncs from the official ToolHive Git repository. + +**Configuration:** +```yaml +source: + type: git + format: toolhive + git: + repository: https://github.com/stacklok/toolhive.git + branch: main + path: pkg/registry/data/registry.json + +syncPolicy: + interval: "30m" +``` + +**What happens when you start:** +1. Background sync coordinator starts immediately +2. Clones `https://github.com/stacklok/toolhive.git` (shallow, depth=1) +3. Extracts `pkg/registry/data/registry.json` from the `main` branch +4. Saves synced data to `./data/registry.json` +5. Saves sync status to `./data/status.json` +6. Repeats every 30 minutes + +**Best for:** +- Using official ToolHive registry data +- Version-controlled registry sources +- Multi-environment deployments (use different branches) +- Pinning to specific tags/commits for stability + +**Options:** +- Use `tag: v1.0.0` instead of `branch` to pin to a release +- Use `commit: abc123` to pin to exact commit +- Change `interval` to control sync frequency + +--- + +### 2. Kubernetes ConfigMap Source + +**File:** [config-configmap.yaml](config-configmap.yaml) + +Syncs from a Kubernetes ConfigMap in the cluster. + +**Configuration:** +```yaml +registryName: rh-partners + +source: + type: configmap + format: toolhive + configmap: + namespace: toolhive-system + name: rh-partners-mcp + key: registry.json + +syncPolicy: + interval: "15m" +``` + +**Prerequisites:** +```bash +# Create a sample ConfigMap (or use existing) +kubectl create configmap rh-partners-mcp \ + -n toolhive-system \ + --from-file=registry.json=./my-registry.json + +# Verify it exists +kubectl get configmap rh-partners-mcp -n toolhive-system + +# Check you have permissions +kubectl auth can-i get configmaps -n toolhive-system +``` + +**What happens when you start:** +1. Connects to Kubernetes API (in-cluster or via kubeconfig) +2. Reads ConfigMap `rh-partners-mcp` from namespace `toolhive-system` +3. Extracts data from the `registry.json` key +4. Saves to `./data/registry.json` +5. Repeats every 15 minutes + +**Best for:** +- Kubernetes-native deployments +- Internal/private registry data +- Partner or organization-specific servers +- Dynamic updates without redeploying the server + +**RBAC Requirements:** +The ServiceAccount needs permission to read ConfigMaps: +```yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: registry-reader + namespace: toolhive-system +rules: +- apiGroups: [""] + resources: ["configmaps"] + verbs: ["get", "list", "watch"] +``` + +--- + +### 3. API Endpoint Source + +**File:** [config-api.yaml](config-api.yaml) + +Syncs from another MCP Registry API endpoint (like the official upstream registry). + +**Configuration:** +```yaml +registryName: mcp-registry + +source: + type: api + format: upstream + api: + endpoint: https://registry.modelcontextprotocol.io/api + +syncPolicy: + interval: "1h" +``` + +**What happens when you start:** +1. Makes HTTP GET to `https://registry.modelcontextprotocol.io/api/v0/servers` +2. Converts from upstream MCP format to ToolHive format +3. Saves to `./data/registry.json` +4. Repeats every hour (less frequent to be respectful of external APIs) + +**Best for:** +- Aggregating multiple registry sources +- Consuming official MCP registry data +- Creating curated/filtered subsets +- Registry federation scenarios + +**Note:** Upstream format conversion is not yet fully implemented. Use Git or ConfigMap sources for production. + +--- + +### 4. File Source + +**File:** [config-file.yaml](config-file.yaml) + +Reads registry data from a local file on the filesystem. + +**Configuration:** +```yaml +registryName: toolhive + +source: + type: file + format: toolhive + file: + path: ./data/registry.json + +syncPolicy: + interval: "5m" +``` + +**What happens when you start:** +1. Reads registry data from the specified file path +2. Validates the JSON data structure +3. Saves validated data to `./data/registry.json` (if different from source) +4. Repeats every 5 minutes to detect file changes + +**Best for:** +- Local development and testing +- Reading from mounted volumes in containers +- Using pre-generated registry files +- Quick prototyping without external dependencies + +**Note:** For file source, the source file and storage location can be the same path. The sync manager will detect if the file has changed by comparing content hashes. + +--- + +### 5. Complete Reference + +**File:** [config-complete.yaml](config-complete.yaml) + +Comprehensive example showing **all** available configuration options with detailed comments. + +Use this as a reference when you need to: +- Understand all available options +- Configure advanced filtering +- See examples of every source type +- Learn about data formats + +--- + +## Configuration Structure + +All config files follow this structure: + +```yaml +# Registry name/identifier (optional, defaults to "default") +registryName: + +# Data source configuration (required) +source: + type: + format: + + # Source-specific config (one of): + git: + repository: + branch: # OR tag: OR commit: + path: + + configmap: + namespace: + name: + key: # optional, defaults to "registry.json" + + api: + endpoint: + + file: + path: + +# Automatic sync policy (required) +syncPolicy: + interval: # e.g., "30m", "1h", "24h" + +# Optional: Filter servers +filter: + names: + include: [] + exclude: [] + tags: + include: [] + exclude: [] +``` + +--- + +## Customization Guide + +### Sync Frequency + +Choose based on your source and needs: + +```yaml +syncPolicy: + interval: "5m" # Development/testing - very frequent + interval: "15m" # ConfigMap sources - local, low latency + interval: "30m" # Git sources - balance freshness vs load + interval: "1h" # API sources - respectful rate limiting + interval: "6h" # Stable sources - infrequent updates +``` + +### Filtering Servers + +Include/exclude specific servers: + +```yaml +filter: + # Name-based filtering (glob patterns) + names: + include: + - "official/*" # Only official namespace + - "myorg/*" # Your organization + exclude: + - "*/deprecated" # Skip deprecated + - "*/internal" # Skip internal-only + - "*/test" # Skip test servers + + # Tag-based filtering (exact matches) + tags: + include: + - "production" # Only production-ready + - "verified" # Only verified servers + exclude: + - "experimental" # Skip experimental + - "beta" # Skip beta versions +``` + +**Filter logic:** +1. Name include patterns (empty = include all) +2. Name exclude patterns +3. Tag include (empty = include all) +4. Tag exclude + +### Git Version Pinning + +```yaml +source: + git: + repository: https://github.com/stacklok/toolhive.git + + # Option 1: Track latest on a branch + branch: main + + # Option 2: Pin to a release tag + tag: v1.2.3 + + # Option 3: Pin to exact commit + commit: abc123def456 + + path: pkg/registry/data/registry.json +``` + +--- + +## Monitoring & Troubleshooting + +### Check Sync Status + +```bash +# View current status +cat ./data/status.json | jq + +# Expected output: +{ + "phase": "Complete", # Syncing, Complete, or Failed + "message": "Sync completed successfully", + "lastAttempt": "2024-11-05T12:30:00Z", + "attemptCount": 0, # Resets to 0 on success + "lastSyncTime": "2024-11-05T12:30:00Z", + "lastSyncHash": "abc123...", # Hash of synced data + "serverCount": 42 # Number of servers synced +} +``` + +**Status phases:** +- `Syncing`: Sync operation in progress +- `Complete`: Last sync successful +- `Failed`: Last sync failed (will auto-retry at next sync interval) + +### Server Logs + +Look for these log messages: + +```bash +# Successful initialization +"Initializing sync manager for automatic registry synchronization" +"Loaded sync status: last sync at 2024-11-05T12:00:00Z, 42 servers" +"Starting background sync coordinator" + +# Successful sync +"Starting sync operation (attempt 1)" +"Registry data fetched successfully from source" +"Sync completed successfully: 42 servers, hash=abc123de" + +# Sync failures +"Sync failed: Fetch failed: ..." +``` + +### Common Issues + +#### Sync Not Starting + +**Symptom:** No `./data/status.json` file + +**Solution:** +1. Verify `--config` flag is provided: + ```bash + thv-registry-api serve --config examples/config-git.yaml + ``` +2. Check logs for "Loaded configuration from..." +3. Ensure `syncPolicy` is defined in config + +#### Git Clone Failed + +**Symptom:** Status shows `Failed` with git error + +**Solutions:** +- Check repository URL is accessible: `git ls-remote ` +- For private repos, configure git credentials +- Verify branch/tag/commit exists +- Check network connectivity + +#### ConfigMap Not Found + +**Symptom:** Status shows `Failed` with "not found" + +**Solutions:** +```bash +# Check ConfigMap exists +kubectl get configmap rh-partners-mcp -n toolhive-system + +# Verify namespace is correct in config +grep namespace examples/config-configmap.yaml + +# Check RBAC permissions +kubectl auth can-i get configmaps -n toolhive-system + +# Describe ConfigMap to see its keys +kubectl describe configmap rh-partners-mcp -n toolhive-system +``` + +#### Permission Denied (File System) + +**Symptom:** Can't write to `./data/` + +**Solutions:** +```bash +# Create data directory +mkdir -p ./data + +# Fix permissions +chmod 755 ./data + +# Check disk space +df -h . +``` + +#### API Endpoint Unreachable + +**Symptom:** Status shows `Failed` with connection error + +**Solutions:** +- Verify endpoint URL: `curl /v0/servers` +- Check network connectivity +- Look for rate limiting (increase interval) +- Verify API is MCP-compatible + +--- + +## Storage Locations + +Synced data is stored in: + +- **Registry data**: `./data/registry.json` +- **Sync status**: `./data/status.json` + +**Note:** The `./data` directory is currently hardcoded but will be configurable via CLI flags in a future release. + +--- + +## Advanced Usage + +### Development Setup + +Fast updates for local development: + +```yaml +source: + type: git + git: + repository: https://github.com/stacklok/toolhive.git + branch: develop # Use dev branch + path: pkg/registry/data/registry.json + +syncPolicy: + interval: "1m" # Very frequent for testing +``` + +### Production Setup + +Conservative config with filtering: + +```yaml +source: + type: configmap + configmap: + namespace: toolhive-production + name: prod-registry + key: registry.json + +syncPolicy: + interval: "30m" + +filter: + tags: + include: ["production", "stable"] + exclude: ["experimental", "deprecated"] +``` + +### Multi-Source Aggregation + +Run multiple instances and aggregate at the application level: + +```bash +# Instance 1: Official ToolHive (port 8081) +thv-registry-api serve \ + --config examples/config-git.yaml \ + --address :8081 & + +# Instance 2: Partners (port 8082) +thv-registry-api serve \ + --config examples/config-configmap.yaml \ + --address :8082 & + +# Instance 3: Upstream MCP (port 8083) +thv-registry-api serve \ + --config examples/config-api.yaml \ + --address :8083 & +``` + +**Note:** Each instance will use its own `./data/` directory for storage. If you need separate storage locations, start each instance from a different working directory or modify the configuration to support custom storage paths (planned feature). + +--- + +## Command Reference + +```bash +# Validate configs before using +./examples/validate-configs.sh + +# Start with Git sync +thv-registry-api serve --config examples/config-git.yaml + +# Start with ConfigMap sync (Kubernetes) +thv-registry-api serve --config examples/config-configmap.yaml + +# Start with API sync +thv-registry-api serve --config examples/config-api.yaml + +# Start with File sync (local development) +thv-registry-api serve --config examples/config-file.yaml + +# Start with custom address +thv-registry-api serve --config examples/config-git.yaml --address :9090 + +# Check sync status +cat ./data/status.json | jq + +# View synced servers +cat ./data/registry.json | jq '.servers | keys' + +# Test API endpoint +curl http://localhost:8080/api/v0/servers | jq + +# Watch logs +tail -f /var/log/thv-registry-api.log | grep -i sync + +# Manually trigger sync (future feature) +# curl -X POST http://localhost:8080/api/v0/sync +``` + +--- + +## See Also + +- [Main README](../README.md) - Full project documentation +- [Architecture](../README.md#architecture) - System design +- [API Reference](../README.md#api-endpoints) - REST endpoints +- [CLAUDE.md](../CLAUDE.md) - Development guide diff --git a/examples/config-api.yaml b/examples/config-api.yaml new file mode 100644 index 0000000..30594b7 --- /dev/null +++ b/examples/config-api.yaml @@ -0,0 +1,64 @@ +# Example configuration for syncing from an API endpoint +# +# This configuration pulls registry data from the official MCP Registry API +# and syncs it to the local storage every hour. +# +# The API source type is useful for: +# - Aggregating data from multiple upstream registries +# - Consuming data from other ToolHive Registry API instances +# - Integrating with third-party MCP registry services +# +# Usage: +# thv-registry-api serve --config examples/config-api.yaml + +# Registry name/identifier (optional, defaults to "default") +registryName: mcp-registry + +# Data source configuration +source: + # Source type: git, configmap, or api + type: api + + # Data format: toolhive (native) or upstream (MCP registry format) + # Use 'upstream' for the official MCP registry format + format: upstream + + # API endpoint configuration + api: + # Base API URL (without path) + # The sync manager will append the appropriate API paths: + # - /v0/servers - List all servers (single response, no pagination) + # - /v0/servers/{name} - Get specific server (future) + # - /v0/info - Get registry metadata (future) + endpoint: https://registry.modelcontextprotocol.io/api + +# Automatic synchronization policy +syncPolicy: + # Sync interval (valid duration: 1m, 5m, 30m, 1h, 24h, etc.) + # Less frequent for upstream API to be respectful of external services + interval: "1h" + +# Optional: Filter configuration to include/exclude specific servers +# This is especially useful when syncing from upstream to curate +# a subset of servers for your organization +# filter: +# # Name-based filtering +# names: +# # Only include servers matching these patterns (glob syntax) +# include: +# - "@modelcontextprotocol/*" +# - "anthropic/*" +# # Exclude servers matching these patterns +# exclude: +# - "*/experimental" +# +# # Tag-based filtering +# tags: +# # Only include servers with these tags +# include: +# - "official" +# - "verified" +# # Exclude servers with these tags +# exclude: +# - "alpha" +# - "unmaintained" diff --git a/examples/config-complete.yaml b/examples/config-complete.yaml new file mode 100644 index 0000000..153e41f --- /dev/null +++ b/examples/config-complete.yaml @@ -0,0 +1,208 @@ +# Complete configuration example showing all available options +# +# This is a comprehensive reference showing every supported configuration option. +# Most deployments will only need a subset of these settings. +# +# For practical examples, see: +# - config-git.yaml: Git repository source +# - config-configmap.yaml: Kubernetes ConfigMap source +# - config-api.yaml: API endpoint source +# - config-file.yaml: Local file source + +# ============================================================================== +# REGISTRY IDENTIFICATION (Optional) +# ============================================================================== + +# Registry name/identifier +# This name is used for: +# - Identifying this registry instance in logs +# - Naming Kubernetes resources (if deployed) +# - API responses and metadata +# Defaults to "default" if not specified +registryName: my-registry + +# ============================================================================== +# DATA SOURCE CONFIGURATION (Required) +# ============================================================================== + +source: + # Source type (Required) + # Options: git, configmap, api, file + type: git + + # Data format (Required) + # Options: + # - toolhive: Native ToolHive registry format (recommended) + # - upstream: Official MCP registry format (not yet fully supported) + format: toolhive + + # ------------------------------------------------------------------------------ + # GIT SOURCE OPTIONS (used when type: git) + # ------------------------------------------------------------------------------ + git: + # Repository URL (Required for git source) + # Supports HTTP, HTTPS, and SSH protocols + repository: https://github.com/stacklok/toolhive.git + + # Version selection (Choose ONE of: branch, tag, or commit) + + # Option 1: Use a branch (tracks latest commits) + branch: main + + # Option 2: Use a specific tag (pinned version) + # tag: v1.0.0 + + # Option 3: Use a specific commit SHA (exact version) + # commit: abc123def456789 + + # Path to registry file within repository (Required) + path: pkg/registry/data/registry.json + + # ------------------------------------------------------------------------------ + # KUBERNETES CONFIGMAP SOURCE OPTIONS (used when type: configmap) + # ------------------------------------------------------------------------------ + # configmap: + # # Kubernetes namespace (Required) + # namespace: toolhive-system + # + # # ConfigMap name (Required) + # name: registry-data + # + # # Key containing registry data (Optional, defaults to "registry.json") + # key: registry.json + + # ------------------------------------------------------------------------------ + # API ENDPOINT SOURCE OPTIONS (used when type: api) + # ------------------------------------------------------------------------------ + # api: + # # Base API URL without path (Required) + # # The sync manager appends standard MCP registry API paths: + # # - /v0/servers (list all servers) + # # - /v0/servers/{name} (get specific server, future) + # # - /v0/info (registry metadata, future) + # endpoint: https://registry.modelcontextprotocol.io/api + + # ------------------------------------------------------------------------------ + # LOCAL FILE SOURCE OPTIONS (used when type: file) + # ------------------------------------------------------------------------------ + # file: + # # Path to registry.json file (Required) + # # Can be absolute or relative to the working directory + # # Useful for: + # # - Reading from mounted volumes (e.g., ConfigMaps as files) + # # - Pre-downloaded or static registry data + # # - Development and testing + # path: ./data/registry.json + +# ============================================================================== +# SYNCHRONIZATION POLICY (Required) +# ============================================================================== + +syncPolicy: + # Sync interval (Required) + # How often to check for and sync updates from the source + # Format: Go duration string (e.g., "30s", "5m", "1h", "24h") + # + # Recommended values: + # - Development: 1m-5m (fast iteration) + # - ConfigMap: 15m-30m (local source, low latency) + # - Git: 30m-1h (balance freshness vs. load) + # - API: 1h-6h (respectful of external services) + # - Stable: 24h (infrequent updates) + interval: "30m" + +# ============================================================================== +# FILTERING CONFIGURATION (Optional) +# ============================================================================== + +# Filter configuration allows you to include/exclude servers from the synced registry +# This is useful for: +# - Creating curated subsets of larger registries +# - Removing deprecated or internal-only servers +# - Enforcing organizational policies +# - Reducing registry size for specific use cases + +filter: + # ------------------------------------------------------------------------------ + # NAME-BASED FILTERING (Optional) + # ------------------------------------------------------------------------------ + names: + # Include patterns (Optional) + # Only servers matching these glob patterns will be included + # Patterns are evaluated before exclude patterns + include: + - "official/*" # Include all official servers + - "stacklok/*" # Include all Stacklok servers + - "@modelcontextprotocol/*" # Include all MCP organization servers + + # Exclude patterns (Optional) + # Servers matching these patterns will be removed + # Applied after include patterns + exclude: + - "*/deprecated" # Exclude deprecated servers + - "*/internal" # Exclude internal-only servers + - "*/test" # Exclude test servers + - "experimental/*" # Exclude experimental namespace + + # ------------------------------------------------------------------------------ + # TAG-BASED FILTERING (Optional) + # ------------------------------------------------------------------------------ + tags: + # Include tags (Optional) + # Only servers with at least one of these tags will be included + include: + - "production" # Only production-ready servers + - "stable" # Only stable releases + - "verified" # Only verified servers + - "certified" # Only certified servers + + # Exclude tags (Optional) + # Servers with any of these tags will be removed + # Applied after include tags + exclude: + - "experimental" # Exclude experimental servers + - "beta" # Exclude beta versions + - "alpha" # Exclude alpha versions + - "deprecated" # Exclude deprecated servers + - "unmaintained" # Exclude unmaintained servers + +# ============================================================================== +# NOTES +# ============================================================================== + +# Storage Locations: +# - Registry data: ./data/registry.json +# - Sync status: ./data/status.json +# +# The storage location is currently hardcoded but will be configurable +# via CLI flags in a future release. + +# Authentication: +# - Git: Uses default git credential helper (SSH keys, .netrc, etc.) +# - ConfigMap: Uses in-cluster service account or kubeconfig +# - API: Currently no authentication (will be added in future) + +# Error Handling: +# - Failed syncs are retried at the next configured sync interval +# - Sync interval is configured via syncPolicy.interval (e.g., "5m", "30m", "1h") +# - Server continues running even if sync fails +# - Status is persisted to track failures and attempts + +# Filter Behavior: +# - Filters are applied in order: name include → name exclude → tag include → tag exclude +# - Empty include list means "include all" +# - Empty exclude list means "exclude none" +# - Patterns use glob syntax: * (any chars), ? (single char) +# - Tag matching is exact (no glob patterns) + +# Performance: +# - Sync runs in background, doesn't block server startup +# - Registry data is cached in memory (refreshes every 30s by default) +# - Sync status is written atomically to prevent corruption +# - Git clones are shallow (depth=1) to minimize bandwidth + +# Monitoring: +# - Check ./data/status.json for sync health +# - Watch server logs for sync messages +# - Use GET /api/v0/info endpoint for registry metadata +# - Monitor serverCount in status for changes diff --git a/examples/config-configmap.yaml b/examples/config-configmap.yaml new file mode 100644 index 0000000..af7b7f1 --- /dev/null +++ b/examples/config-configmap.yaml @@ -0,0 +1,65 @@ +# Example configuration for syncing from a Kubernetes ConfigMap +# +# This configuration pulls registry data from a ConfigMap in the Kubernetes cluster +# and syncs it to the local storage every 15 minutes. +# +# Prerequisites: +# - Running in a Kubernetes cluster or have kubeconfig configured +# - ConfigMap 'rh-partners-mcp' exists in 'toolhive-system' namespace +# - ConfigMap has a 'registry.json' key with MCP registry data +# - Appropriate RBAC permissions to read ConfigMaps +# +# Usage: +# thv-registry-api serve --config examples/config-configmap.yaml + +# Registry name/identifier (optional, defaults to "default") +registryName: rh-partners + +# Data source configuration +source: + # Source type: git, configmap, or api + type: configmap + + # Data format: toolhive (native) or upstream (MCP registry format) + format: toolhive + + # Kubernetes ConfigMap configuration + configmap: + # Namespace where the ConfigMap is located + namespace: toolhive-system + + # ConfigMap name + name: rh-partners-mcp + + # Key within the ConfigMap containing the registry data + # Defaults to 'registry.json' if not specified + key: registry.json + +# Automatic synchronization policy +syncPolicy: + # Sync interval (valid duration: 1m, 5m, 30m, 1h, 24h, etc.) + # More frequent checks for ConfigMap sources since they're local + interval: "15m" + +# Optional: Filter configuration to include/exclude specific servers +# filter: +# # Name-based filtering +# names: +# # Only include servers matching these patterns (glob syntax) +# include: +# - "redhat/*" +# - "partners/*" +# # Exclude servers matching these patterns +# exclude: +# - "*/internal" +# - "*/test" +# +# # Tag-based filtering +# tags: +# # Only include servers with these tags +# include: +# - "partner" +# - "certified" +# # Exclude servers with these tags +# exclude: +# - "deprecated" diff --git a/examples/config-file.yaml b/examples/config-file.yaml new file mode 100644 index 0000000..b35a02c --- /dev/null +++ b/examples/config-file.yaml @@ -0,0 +1,51 @@ +# Example configuration for reading from a local file +# +# This configuration reads registry data from a local JSON file on the filesystem. +# This is useful for: +# - Testing and development +# - Reading from mounted volumes (e.g., ConfigMaps mounted as files) +# - Pre-downloaded or statically provided registry data +# +# Usage: +# thv-registry-api serve --config examples/config-file.yaml + +# Registry name/identifier (optional, defaults to "default") +registryName: toolhive + +# Data source configuration +source: + # Source type: file, git, configmap, or api + type: file + + # Data format: toolhive (native) or upstream (MCP registry format) + format: toolhive + + # Local file configuration + file: + # Path to the registry.json file (absolute or relative) + # Can be a mounted ConfigMap, downloaded file, or any JSON file + path: ./data/registry.json + +# Automatic synchronization policy +syncPolicy: + # Sync interval - how often to check if the file has changed + # For local files, you can use shorter intervals + interval: "5m" + +# Optional: Filter configuration to include/exclude specific servers +# filter: +# # Name-based filtering +# names: +# include: +# - "official/*" +# - "stacklok/*" +# exclude: +# - "*/deprecated" +# +# # Tag-based filtering +# tags: +# include: +# - "production" +# - "stable" +# exclude: +# - "experimental" diff --git a/examples/config-git.yaml b/examples/config-git.yaml new file mode 100644 index 0000000..07dd86d --- /dev/null +++ b/examples/config-git.yaml @@ -0,0 +1,63 @@ +# Example configuration for syncing from the ToolHive Git repository +# +# This configuration pulls registry data from the official ToolHive repository +# and syncs it to the local storage every 30 minutes. +# +# Usage: +# thv-registry-api serve --config examples/config-git.yaml + +# Registry name/identifier (optional, defaults to "default") +registryName: toolhive + +# Data source configuration +source: + # Source type: git, configmap, or api + type: git + + # Data format: toolhive (native) or upstream (MCP registry format) + format: toolhive + + # Git repository configuration + git: + # Repository URL (HTTP/HTTPS/SSH) + repository: https://github.com/stacklok/toolhive.git + + # Branch to use (mutually exclusive with tag and commit) + branch: main + + # Alternative: Use a specific tag + # tag: v1.0.0 + + # Alternative: Use a specific commit SHA + # commit: abc123def456 + + # Path to registry file within the repository + path: pkg/registry/data/registry.json + +# Automatic synchronization policy +syncPolicy: + # Sync interval (valid duration: 1m, 5m, 30m, 1h, 24h, etc.) + interval: "30m" + +# Optional: Filter configuration to include/exclude specific servers +# filter: +# # Name-based filtering +# names: +# # Only include servers matching these patterns (glob syntax) +# include: +# - "stacklok/*" +# - "official/*" +# # Exclude servers matching these patterns +# exclude: +# - "*/deprecated" +# +# # Tag-based filtering +# tags: +# # Only include servers with these tags +# include: +# - "production" +# - "stable" +# # Exclude servers with these tags +# exclude: +# - "experimental" +# - "beta" diff --git a/examples/validate-configs.sh b/examples/validate-configs.sh new file mode 100755 index 0000000..2569ad0 --- /dev/null +++ b/examples/validate-configs.sh @@ -0,0 +1,47 @@ +#!/bin/bash +# Validate example configuration files + +# Get the directory where this script is located +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +echo "Validating example configurations..." +echo + +for config in "$SCRIPT_DIR"/config-*.yaml; do + echo "Checking $config..." + + # Basic YAML syntax check using yq (if available) or python + if command -v yq &> /dev/null; then + if yq eval '.' "$config" > /dev/null 2>&1; then + echo " ✓ Valid YAML syntax" + else + echo " ✗ Invalid YAML syntax" + exit 1 + fi + elif command -v python3 &> /dev/null; then + if python3 -c "import yaml; yaml.safe_load(open('$config'))" 2>/dev/null; then + echo " ✓ Valid YAML syntax" + else + echo " ✗ Invalid YAML syntax" + exit 1 + fi + else + echo " ⚠ No YAML validator found (install yq or python3+pyyaml)" + fi + + # Check required fields + if grep -q "source:" "$config" && \ + grep -q "type:" "$config" && \ + grep -q "format:" "$config" && \ + grep -q "syncPolicy:" "$config" && \ + grep -q "interval:" "$config"; then + echo " ✓ Required fields present" + else + echo " ✗ Missing required fields" + exit 1 + fi + + echo +done + +echo "✓ All configurations validated successfully!" diff --git a/internal/service/file_provider.go b/internal/service/file_provider.go index a23c1d3..60fd9ae 100644 --- a/internal/service/file_provider.go +++ b/internal/service/file_provider.go @@ -3,72 +3,48 @@ package service import ( "context" - "encoding/json" "fmt" - "os" - "path/filepath" + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stacklok/toolhive-registry-server/pkg/sources" "github.com/stacklok/toolhive/pkg/registry" ) -// FileRegistryDataProvider implements RegistryDataProvider using local file system. -// This implementation reads registry data from a mounted file instead of calling the Kubernetes API. -// It is designed to work with ConfigMaps mounted as volumes in Kubernetes deployments. +// FileRegistryDataProvider implements RegistryDataProvider by delegating to StorageManager. +// This implementation uses the adapter pattern to reuse storage infrastructure instead of +// duplicating file reading logic. It delegates to StorageManager for all file operations. type FileRegistryDataProvider struct { - filePath string - registryName string + storageManager sources.StorageManager + config *config.Config + registryName string } // NewFileRegistryDataProvider creates a new file-based registry data provider. -// The filePath parameter should point to the registry.json file, typically mounted from a ConfigMap. -// The registryName parameter specifies the registry identifier for business logic purposes. -func NewFileRegistryDataProvider(filePath, registryName string) *FileRegistryDataProvider { +// It accepts a StorageManager to delegate file operations and a Config for registry metadata. +// This design eliminates code duplication and improves testability through dependency injection. +func NewFileRegistryDataProvider(storageManager sources.StorageManager, cfg *config.Config) *FileRegistryDataProvider { return &FileRegistryDataProvider{ - filePath: filePath, - registryName: registryName, + storageManager: storageManager, + config: cfg, + registryName: cfg.GetRegistryName(), } } // GetRegistryData implements RegistryDataProvider.GetRegistryData. -// It reads the registry.json file from the local filesystem and parses it into a Registry struct. -func (p *FileRegistryDataProvider) GetRegistryData(_ context.Context) (*registry.Registry, error) { - if p.filePath == "" { - return nil, fmt.Errorf("file path not configured") - } - - // Check if the file exists and is readable - if _, err := os.Stat(p.filePath); err != nil { - if os.IsNotExist(err) { - return nil, fmt.Errorf("registry file not found at %s: %w", p.filePath, err) - } - return nil, fmt.Errorf("cannot access registry file at %s: %w", p.filePath, err) - } - - // Read the file contents - data, err := os.ReadFile(p.filePath) - if err != nil { - return nil, fmt.Errorf("failed to read registry file at %s: %w", p.filePath, err) - } - - // Parse the JSON data - var reg registry.Registry - if err := json.Unmarshal(data, ®); err != nil { - return nil, fmt.Errorf("failed to parse registry data from file %s: %w", p.filePath, err) - } - - return ®, nil +// It delegates to the StorageManager to retrieve and parse registry data. +// This eliminates code duplication and provides a single source of truth for file operations. +func (p *FileRegistryDataProvider) GetRegistryData(ctx context.Context) (*registry.Registry, error) { + // Delegate to storage manager - all file reading logic is centralized there + return p.storageManager.Get(ctx, p.config) } // GetSource implements RegistryDataProvider.GetSource. -// It returns a descriptive string indicating the file source. +// It returns a descriptive string indicating the file source from the configuration. func (p *FileRegistryDataProvider) GetSource() string { - if p.filePath == "" { + if p.config.Source.File == nil || p.config.Source.File.Path == "" { return "file:" } - - // Clean the path for consistent display - cleanPath := filepath.Clean(p.filePath) - return fmt.Sprintf("file:%s", cleanPath) + return fmt.Sprintf("file:%s", p.config.Source.File.Path) } // GetRegistryName implements RegistryDataProvider.GetRegistryName. diff --git a/internal/service/file_provider_test.go b/internal/service/file_provider_test.go index 84f39a7..25f2b5d 100644 --- a/internal/service/file_provider_test.go +++ b/internal/service/file_provider_test.go @@ -2,13 +2,15 @@ package service import ( "context" - "os" - "path/filepath" + "errors" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "github.com/stacklok/toolhive-registry-server/pkg/config" + sourcesmocks "github.com/stacklok/toolhive-registry-server/pkg/sources/mocks" "github.com/stacklok/toolhive/pkg/registry" ) @@ -18,25 +20,32 @@ func TestFileRegistryDataProvider_GetRegistryData(t *testing.T) { tests := []struct { name string - fileContent string + setupMock func(*sourcesmocks.MockStorageManager) wantErr bool errContains string validate func(*testing.T, *registry.Registry) }{ { - name: "valid registry file", - fileContent: `{ - "version": "1.0", - "last_updated": "2024-01-01T00:00:00Z", - "servers": { - "test-server": { - "name": "test-server", - "description": "A test server", - "image": "test:latest" - } - }, - "remote_servers": {} - }`, + name: "successful retrieval", + setupMock: func(m *sourcesmocks.MockStorageManager) { + expectedRegistry := ®istry.Registry{ + Version: "1.0", + LastUpdated: "2024-01-01T00:00:00Z", + Servers: map[string]*registry.ImageMetadata{ + "test-server": { + BaseServerMetadata: registry.BaseServerMetadata{ + Name: "test-server", + Description: "A test server", + }, + Image: "test:latest", + }, + }, + RemoteServers: map[string]*registry.RemoteServerMetadata{}, + } + m.EXPECT(). + Get(gomock.Any(), gomock.Any()). + Return(expectedRegistry, nil) + }, wantErr: false, validate: func(t *testing.T, reg *registry.Registry) { t.Helper() @@ -44,55 +53,59 @@ func TestFileRegistryDataProvider_GetRegistryData(t *testing.T) { assert.Equal(t, "2024-01-01T00:00:00Z", reg.LastUpdated) assert.Len(t, reg.Servers, 1) assert.Contains(t, reg.Servers, "test-server") - assert.Equal(t, "test-server", reg.Servers["test-server"].Name) - assert.Equal(t, "A test server", reg.Servers["test-server"].Description) - assert.Equal(t, "test:latest", reg.Servers["test-server"].Image) }, }, { - name: "empty registry file", - fileContent: `{ - "version": "1.0", - "last_updated": "", - "servers": {}, - "remote_servers": {} - }`, + name: "empty registry", + setupMock: func(m *sourcesmocks.MockStorageManager) { + expectedRegistry := ®istry.Registry{ + Version: "1.0", + Servers: map[string]*registry.ImageMetadata{}, + RemoteServers: map[string]*registry.RemoteServerMetadata{}, + } + m.EXPECT(). + Get(gomock.Any(), gomock.Any()). + Return(expectedRegistry, nil) + }, wantErr: false, validate: func(t *testing.T, reg *registry.Registry) { t.Helper() assert.Equal(t, "1.0", reg.Version) assert.Len(t, reg.Servers, 0) - assert.Len(t, reg.RemoteServers, 0) }, }, { - name: "invalid JSON", - fileContent: `{invalid json}`, - wantErr: true, - errContains: "failed to parse registry data", - }, - { - name: "empty file", - fileContent: "", + name: "storage manager returns error", + setupMock: func(m *sourcesmocks.MockStorageManager) { + m.EXPECT(). + Get(gomock.Any(), gomock.Any()). + Return(nil, errors.New("registry file not found")) + }, wantErr: true, - errContains: "failed to parse registry data", + errContains: "registry file not found", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - // Create temporary file - tmpDir := t.TempDir() - tmpFile := filepath.Join(tmpDir, "registry.json") + ctrl := gomock.NewController(t) + defer ctrl.Finish() - err := os.WriteFile(tmpFile, []byte(tt.fileContent), 0644) - require.NoError(t, err) + mockStorageManager := sourcesmocks.NewMockStorageManager(ctrl) + tt.setupMock(mockStorageManager) - // Create provider - provider := NewFileRegistryDataProvider(tmpFile, "test-registry") + cfg := &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{Path: "/tmp/registry.json"}, + }, + } + + provider := NewFileRegistryDataProvider(mockStorageManager, cfg) - // Test GetRegistryData result, err := provider.GetRegistryData(ctx) if tt.wantErr { @@ -112,76 +125,64 @@ func TestFileRegistryDataProvider_GetRegistryData(t *testing.T) { } } -func TestFileRegistryDataProvider_GetRegistryData_FileErrors(t *testing.T) { +func TestFileRegistryDataProvider_GetSource(t *testing.T) { t.Parallel() - ctx := context.Background() - - tests := []struct { - name string - filePath string - wantErr bool - errContains string - }{ - { - name: "empty file path", - filePath: "", - wantErr: true, - errContains: "file path not configured", - }, - { - name: "non-existent file", - filePath: "/non/existent/file.json", - wantErr: true, - errContains: "registry file not found", - }, - { - name: "directory instead of file", - filePath: t.TempDir(), - wantErr: true, - errContains: "failed to read registry file", - }, - } + ctrl := gomock.NewController(t) + defer ctrl.Finish() - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - provider := NewFileRegistryDataProvider(tt.filePath, "test-registry") - result, err := provider.GetRegistryData(ctx) + mockStorageManager := sourcesmocks.NewMockStorageManager(ctrl) - if tt.wantErr { - assert.Error(t, err) - if tt.errContains != "" { - assert.Contains(t, err.Error(), tt.errContains) - } - assert.Nil(t, result) - } else { - assert.NoError(t, err) - assert.NotNil(t, result) - } - }) - } -} - -func TestFileRegistryDataProvider_GetSource(t *testing.T) { - t.Parallel() tests := []struct { name string - filePath string + config *config.Config expected string }{ { - name: "absolute path", - filePath: "/data/registry/registry.json", + name: "absolute path", + config: &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{Path: "/data/registry/registry.json"}, + }, + }, expected: "file:/data/registry/registry.json", }, { - name: "relative path", - filePath: "./registry.json", - expected: "file:registry.json", // filepath.Clean removes ./ + name: "relative path", + config: &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{Path: "./registry.json"}, + }, + }, + expected: "file:./registry.json", }, { - name: "empty path", - filePath: "", + name: "empty path", + config: &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{Path: ""}, + }, + }, + expected: "file:", + }, + { + name: "nil file config", + config: &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: nil, + }, + }, expected: "file:", }, } @@ -189,7 +190,7 @@ func TestFileRegistryDataProvider_GetSource(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - provider := NewFileRegistryDataProvider(tt.filePath, "test-registry") + provider := NewFileRegistryDataProvider(mockStorageManager, tt.config) result := provider.GetSource() assert.Equal(t, tt.expected, result) }) @@ -198,35 +199,48 @@ func TestFileRegistryDataProvider_GetSource(t *testing.T) { func TestNewFileRegistryDataProvider(t *testing.T) { t.Parallel() - filePath := "/test/path/registry.json" - registryName := "test-registry" - provider := NewFileRegistryDataProvider(filePath, registryName) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStorageManager := sourcesmocks.NewMockStorageManager(ctrl) + + cfg := &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{Path: "/test/path/registry.json"}, + }, + } + + provider := NewFileRegistryDataProvider(mockStorageManager, cfg) - assert.NotNil(t, provider) + require.NotNil(t, provider) assert.Equal(t, "file:/test/path/registry.json", provider.GetSource()) - assert.Equal(t, registryName, provider.GetRegistryName()) + assert.Equal(t, "test-registry", provider.GetRegistryName()) } func TestFileRegistryDataProvider_GetRegistryName(t *testing.T) { t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStorageManager := sourcesmocks.NewMockStorageManager(ctrl) + tests := []struct { name string - filePath string registryName string }{ { name: "normal registry name", - filePath: "/data/registry/registry.json", registryName: "my-custom-registry", }, { name: "production registry", - filePath: "/path/to/file.json", registryName: "production-registry", }, { - name: "empty registry name", - filePath: "/some/path.json", + name: "empty registry name defaults to default", registryName: "", }, } @@ -234,9 +248,24 @@ func TestFileRegistryDataProvider_GetRegistryName(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - provider := NewFileRegistryDataProvider(tt.filePath, tt.registryName) + cfg := &config.Config{ + RegistryName: tt.registryName, + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{Path: "/path/to/file.json"}, + }, + } + + provider := NewFileRegistryDataProvider(mockStorageManager, cfg) result := provider.GetRegistryName() - assert.Equal(t, tt.registryName, result) + + // Config.GetRegistryName() returns "default" for empty names + expectedName := tt.registryName + if expectedName == "" { + expectedName = "default" + } + assert.Equal(t, expectedName, result) }) } } diff --git a/internal/service/k8s_provider.go b/internal/service/k8s_provider.go index 8b2e6f8..6001ee2 100644 --- a/internal/service/k8s_provider.go +++ b/internal/service/k8s_provider.go @@ -3,17 +3,14 @@ package service import ( "context" - "encoding/json" "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" thvv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" - "github.com/stacklok/toolhive/pkg/registry" ) // Label constants for deployed server identification @@ -35,69 +32,6 @@ func (p *K8sDeploymentProvider) getDeployedServerLabelSelector() string { return fmt.Sprintf("%s=%s,%s", LabelRegistryName, p.registryName, LabelRegistryNamespace) } -// K8sRegistryDataProvider implements RegistryDataProvider using Kubernetes ConfigMaps. -// This implementation fetches registry data from a ConfigMap in a Kubernetes cluster. -type K8sRegistryDataProvider struct { - client kubernetes.Interface - configMapName string - namespace string - registryName string -} - -// NewK8sRegistryDataProvider creates a new Kubernetes-based registry data provider. -// It requires a Kubernetes client, the ConfigMap details where registry data is stored, -// and the registry name identifier for business logic purposes. -func NewK8sRegistryDataProvider( - kubeClient kubernetes.Interface, - configMapName, namespace, registryName string, -) *K8sRegistryDataProvider { - return &K8sRegistryDataProvider{ - client: kubeClient, - configMapName: configMapName, - namespace: namespace, - registryName: registryName, - } -} - -// GetRegistryData implements RegistryDataProvider.GetRegistryData. -// It fetches the ConfigMap from Kubernetes and extracts the registry.json data. -func (p *K8sRegistryDataProvider) GetRegistryData(ctx context.Context) (*registry.Registry, error) { - if p.client == nil { - return nil, fmt.Errorf("kubernetes client not initialized") - } - - // Get the ConfigMap - configMap, err := p.client.CoreV1().ConfigMaps(p.namespace).Get(ctx, p.configMapName, metav1.GetOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to get configmap %s/%s: %w", p.namespace, p.configMapName, err) - } - - // Parse registry data from ConfigMap - registryJSON, exists := configMap.Data["registry.json"] - if !exists { - return nil, fmt.Errorf("registry.json not found in configmap %s/%s", p.namespace, p.configMapName) - } - - var data registry.Registry - if err := json.Unmarshal([]byte(registryJSON), &data); err != nil { - return nil, fmt.Errorf("failed to parse registry data from configmap %s/%s: %w", p.namespace, p.configMapName, err) - } - - return &data, nil -} - -// GetSource implements RegistryDataProvider.GetSource. -// It returns a descriptive string indicating the ConfigMap source. -func (p *K8sRegistryDataProvider) GetSource() string { - return fmt.Sprintf("configmap:%s/%s", p.namespace, p.configMapName) -} - -// GetRegistryName implements RegistryDataProvider.GetRegistryName. -// It returns the injected registry name identifier. -func (p *K8sRegistryDataProvider) GetRegistryName() string { - return p.registryName -} - // K8sDeploymentProvider implements DeploymentProvider using Kubernetes API. // This implementation queries Kubernetes MCPServer custom resources to find deployed MCP servers. type K8sDeploymentProvider struct { diff --git a/internal/service/mocks/mock_provider_factory.go b/internal/service/mocks/mock_provider_factory.go index ed55771..48f329f 100644 --- a/internal/service/mocks/mock_provider_factory.go +++ b/internal/service/mocks/mock_provider_factory.go @@ -13,6 +13,7 @@ import ( reflect "reflect" service "github.com/stacklok/toolhive-registry-server/internal/service" + config "github.com/stacklok/toolhive-registry-server/pkg/config" gomock "go.uber.org/mock/gomock" ) @@ -41,16 +42,16 @@ func (m *MockRegistryProviderFactory) EXPECT() *MockRegistryProviderFactoryMockR } // CreateProvider mocks base method. -func (m *MockRegistryProviderFactory) CreateProvider(config *service.RegistryProviderConfig) (service.RegistryDataProvider, error) { +func (m *MockRegistryProviderFactory) CreateProvider(cfg *config.Config) (service.RegistryDataProvider, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateProvider", config) + ret := m.ctrl.Call(m, "CreateProvider", cfg) ret0, _ := ret[0].(service.RegistryDataProvider) ret1, _ := ret[1].(error) return ret0, ret1 } // CreateProvider indicates an expected call of CreateProvider. -func (mr *MockRegistryProviderFactoryMockRecorder) CreateProvider(config any) *gomock.Call { +func (mr *MockRegistryProviderFactoryMockRecorder) CreateProvider(cfg any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateProvider", reflect.TypeOf((*MockRegistryProviderFactory)(nil).CreateProvider), config) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateProvider", reflect.TypeOf((*MockRegistryProviderFactory)(nil).CreateProvider), cfg) } diff --git a/internal/service/provider_factory.go b/internal/service/provider_factory.go index 1e6dcee..44226bb 100644 --- a/internal/service/provider_factory.go +++ b/internal/service/provider_factory.go @@ -4,156 +4,39 @@ package service import ( "fmt" - "k8s.io/client-go/kubernetes" + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stacklok/toolhive-registry-server/pkg/sources" ) -// RegistryProviderType represents the type of registry data provider -type RegistryProviderType string - -const ( - // RegistryProviderTypeConfigMap represents a ConfigMap-based registry provider - RegistryProviderTypeConfigMap RegistryProviderType = "configmap" - // RegistryProviderTypeFile represents a file-based registry provider - RegistryProviderTypeFile RegistryProviderType = "file" -) - -// RegistryProviderConfig holds configuration for creating a registry data provider -type RegistryProviderConfig struct { - Type RegistryProviderType - ConfigMap *ConfigMapProviderConfig - File *FileProviderConfig -} - -// ConfigMapProviderConfig holds configuration for ConfigMap-based registry provider -type ConfigMapProviderConfig struct { - Name string - Namespace string - Clientset kubernetes.Interface - RegistryName string -} - -// FileProviderConfig holds configuration for file-based registry provider -type FileProviderConfig struct { - FilePath string - RegistryName string -} - -// Validate validates the RegistryProviderConfig -func (c *RegistryProviderConfig) Validate() error { - if c.Type == "" { - return fmt.Errorf("provider type is required") - } - - switch c.Type { - case RegistryProviderTypeConfigMap: - if c.ConfigMap == nil { - return fmt.Errorf("configmap configuration required for configmap provider") - } - return c.ConfigMap.Validate() - case RegistryProviderTypeFile: - if c.File == nil { - return fmt.Errorf("file configuration required for file provider") - } - return c.File.Validate() - default: - return fmt.Errorf("unsupported provider type: %s", c.Type) - } -} - -// Validate validates the ConfigMapProviderConfig -func (c *ConfigMapProviderConfig) Validate() error { - if c.Clientset == nil { - return fmt.Errorf("kubernetes clientset is required") - } - if c.Name == "" { - return fmt.Errorf("configmap name is required") - } - if c.Namespace == "" { - return fmt.Errorf("configmap namespace is required") - } - if c.RegistryName == "" { - return fmt.Errorf("registry name is required") - } - return nil -} - -// Validate validates the FileProviderConfig -func (c *FileProviderConfig) Validate() error { - if c.FilePath == "" { - return fmt.Errorf("file path is required") - } - if c.RegistryName == "" { - return fmt.Errorf("registry name is required") - } - return nil -} - //go:generate mockgen -destination=mocks/mock_provider_factory.go -package=mocks -source=provider_factory.go RegistryProviderFactory // RegistryProviderFactory creates registry data providers based on configuration type RegistryProviderFactory interface { // CreateProvider creates a registry data provider based on the provided configuration - CreateProvider(config *RegistryProviderConfig) (RegistryDataProvider, error) + CreateProvider(cfg *config.Config) (RegistryDataProvider, error) } // DefaultRegistryProviderFactory is the default implementation of RegistryProviderFactory -type DefaultRegistryProviderFactory struct{} - -// NewRegistryProviderFactory creates a new default registry provider factory -func NewRegistryProviderFactory() RegistryProviderFactory { - return &DefaultRegistryProviderFactory{} +type DefaultRegistryProviderFactory struct { + storageManager sources.StorageManager } -// CreateProvider implements RegistryProviderFactory.CreateProvider -func (f *DefaultRegistryProviderFactory) CreateProvider(config *RegistryProviderConfig) (RegistryDataProvider, error) { - if config == nil { - return nil, fmt.Errorf("registry provider config cannot be nil") - } - - switch config.Type { - case RegistryProviderTypeConfigMap: - if config.ConfigMap == nil { - return nil, fmt.Errorf("configmap configuration required for configmap provider type") - } - return f.createConfigMapProvider(config.ConfigMap) - - case RegistryProviderTypeFile: - if config.File == nil { - return nil, fmt.Errorf("file configuration required for file provider type") - } - return f.createFileProvider(config.File) - - default: - return nil, fmt.Errorf("unsupported registry provider type: %s", config.Type) +// NewRegistryProviderFactory creates a new default registry provider factory +func NewRegistryProviderFactory(storageManager sources.StorageManager) RegistryProviderFactory { + return &DefaultRegistryProviderFactory{ + storageManager: storageManager, } } -// createConfigMapProvider creates a ConfigMap-based registry data provider -func (*DefaultRegistryProviderFactory) createConfigMapProvider(config *ConfigMapProviderConfig) (RegistryDataProvider, error) { - if config.Clientset == nil { - return nil, fmt.Errorf("kubernetes clientset is required for configmap provider") - } - if config.Name == "" { - return nil, fmt.Errorf("configmap name is required") - } - if config.Namespace == "" { - return nil, fmt.Errorf("configmap namespace is required") - } - if config.RegistryName == "" { - return nil, fmt.Errorf("registry name is required for configmap provider") +// CreateProvider implements RegistryProviderFactory.CreateProvider +func (f *DefaultRegistryProviderFactory) CreateProvider(cfg *config.Config) (RegistryDataProvider, error) { + if cfg == nil { + return nil, fmt.Errorf("config cannot be nil") } - return NewK8sRegistryDataProvider(config.Clientset, config.Name, config.Namespace, config.RegistryName), nil -} - -// createFileProvider creates a file-based registry data provider -func (*DefaultRegistryProviderFactory) createFileProvider(config *FileProviderConfig) (RegistryDataProvider, error) { - if config.FilePath == "" { - return nil, fmt.Errorf("file path is required for file provider") - } - if config.RegistryName == "" { - return nil, fmt.Errorf("registry name is required for file provider") + if err := cfg.Validate(); err != nil { + return nil, fmt.Errorf("invalid config: %w", err) } - return NewFileRegistryDataProvider(config.FilePath, config.RegistryName), nil + return NewFileRegistryDataProvider(f.storageManager, cfg), nil } diff --git a/internal/service/provider_factory_test.go b/internal/service/provider_factory_test.go index 21148fc..44cf10d 100644 --- a/internal/service/provider_factory_test.go +++ b/internal/service/provider_factory_test.go @@ -1,204 +1,42 @@ package service import ( - "os" - "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "k8s.io/client-go/kubernetes/fake" -) - -func TestRegistryProviderConfig_Validate(t *testing.T) { - t.Parallel() - tests := []struct { - name string - config *RegistryProviderConfig - wantErr bool - errContains string - }{ - { - name: "valid configmap provider", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: &ConfigMapProviderConfig{ - Name: "test-cm", - Namespace: "test-ns", - Clientset: fake.NewSimpleClientset(), - RegistryName: "test-registry", - }, - }, - wantErr: false, - }, - { - name: "valid file provider", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeFile, - File: &FileProviderConfig{ - FilePath: "/data/registry.json", - RegistryName: "test-registry", - }, - }, - wantErr: false, - }, - { - name: "empty provider type", - config: &RegistryProviderConfig{ - Type: "", - }, - wantErr: true, - errContains: "provider type is required", - }, - { - name: "configmap provider without config", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: nil, - }, - wantErr: true, - errContains: "configmap configuration required", - }, - { - name: "file provider without config", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeFile, - File: nil, - }, - wantErr: true, - errContains: "file configuration required", - }, - { - name: "unsupported provider type", - config: &RegistryProviderConfig{ - Type: "unsupported", - }, - wantErr: true, - errContains: "unsupported provider type", - }, - { - name: "configmap with missing clientset", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: &ConfigMapProviderConfig{ - Name: "test-cm", - Namespace: "test-ns", - Clientset: nil, - }, - }, - wantErr: true, - errContains: "kubernetes clientset is required", - }, - { - name: "configmap with empty name", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: &ConfigMapProviderConfig{ - Name: "", - Namespace: "test-ns", - Clientset: fake.NewSimpleClientset(), - }, - }, - wantErr: true, - errContains: "configmap name is required", - }, - { - name: "configmap with empty namespace", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: &ConfigMapProviderConfig{ - Name: "test-cm", - Namespace: "", - Clientset: fake.NewSimpleClientset(), - }, - }, - wantErr: true, - errContains: "configmap namespace is required", - }, - { - name: "file with empty path", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeFile, - File: &FileProviderConfig{ - FilePath: "", - }, - }, - wantErr: true, - errContains: "file path is required", - }, - } + "go.uber.org/mock/gomock" - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - err := tt.config.Validate() - - if tt.wantErr { - assert.Error(t, err) - if tt.errContains != "" { - assert.Contains(t, err.Error(), tt.errContains) - } - } else { - assert.NoError(t, err) - } - }) - } -} + "github.com/stacklok/toolhive-registry-server/pkg/config" + sourcesmocks "github.com/stacklok/toolhive-registry-server/pkg/sources/mocks" +) func TestDefaultRegistryProviderFactory_CreateProvider(t *testing.T) { t.Parallel() - tmpDir := t.TempDir() - tmpFile := filepath.Join(tmpDir, "test-registry.json") - - // Create a test file - err := os.WriteFile(tmpFile, []byte(`{ - "version": "1.0", - "last_updated": "", - "servers": {}, - "remote_servers": {} - }`), 0644) - require.NoError(t, err) tests := []struct { name string - config *RegistryProviderConfig + config *config.Config wantErr bool errContains string checkType func(*testing.T, RegistryDataProvider) }{ { - name: "create configmap provider", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: &ConfigMapProviderConfig{ - Name: "test-cm", - Namespace: "test-ns", - Clientset: fake.NewSimpleClientset(), - RegistryName: "test-registry", - }, - }, - wantErr: false, - checkType: func(t *testing.T, provider RegistryDataProvider) { - t.Helper() - assert.IsType(t, &K8sRegistryDataProvider{}, provider) - assert.Equal(t, "configmap:test-ns/test-cm", provider.GetSource()) - assert.Equal(t, "test-registry", provider.GetRegistryName()) - }, - }, - { - name: "create file provider", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeFile, - File: &FileProviderConfig{ - FilePath: tmpFile, - RegistryName: "test-file-registry", + name: "create file provider with valid config", + config: &config.Config{ + RegistryName: "test-file-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{Path: "/data/registry.json"}, }, + SyncPolicy: &config.SyncPolicyConfig{Interval: "30m"}, }, wantErr: false, checkType: func(t *testing.T, provider RegistryDataProvider) { t.Helper() assert.IsType(t, &FileRegistryDataProvider{}, provider) - assert.Equal(t, "file:"+tmpFile, provider.GetSource()) + assert.Equal(t, "file:/data/registry.json", provider.GetSource()) assert.Equal(t, "test-file-registry", provider.GetRegistryName()) }, }, @@ -206,65 +44,31 @@ func TestDefaultRegistryProviderFactory_CreateProvider(t *testing.T) { name: "nil config", config: nil, wantErr: true, - errContains: "registry provider config cannot be nil", - }, - { - name: "invalid config - missing configmap", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: nil, - }, - wantErr: true, - errContains: "configmap configuration required", - }, - { - name: "invalid config - missing file", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeFile, - File: nil, - }, - wantErr: true, - errContains: "file configuration required", - }, - { - name: "invalid config - unsupported type", - config: &RegistryProviderConfig{ - Type: "unknown", - }, - wantErr: true, - errContains: "unsupported registry provider type", - }, - { - name: "configmap with missing clientset", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeConfigMap, - ConfigMap: &ConfigMapProviderConfig{ - Name: "test-cm", - Namespace: "test-ns", - Clientset: nil, - }, - }, - wantErr: true, - errContains: "kubernetes clientset is required", + errContains: "config cannot be nil", }, { - name: "file with empty path", - config: &RegistryProviderConfig{ - Type: RegistryProviderTypeFile, - File: &FileProviderConfig{ - FilePath: "", + name: "invalid config - missing source type", + config: &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: "", + Format: config.SourceFormatToolHive, }, }, wantErr: true, - errContains: "file path is required", + errContains: "invalid config", }, } - factory := NewRegistryProviderFactory() - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStorageManager := sourcesmocks.NewMockStorageManager(ctrl) + factory := NewRegistryProviderFactory(mockStorageManager) + provider, err := factory.CreateProvider(tt.config) if tt.wantErr { @@ -286,51 +90,17 @@ func TestDefaultRegistryProviderFactory_CreateProvider(t *testing.T) { func TestNewRegistryProviderFactory(t *testing.T) { t.Parallel() - factory := NewRegistryProviderFactory() - assert.NotNil(t, factory) - assert.IsType(t, &DefaultRegistryProviderFactory{}, factory) -} + ctrl := gomock.NewController(t) + defer ctrl.Finish() -func TestProviderTypes(t *testing.T) { - t.Parallel() - assert.Equal(t, "configmap", string(RegistryProviderTypeConfigMap)) - assert.Equal(t, "file", string(RegistryProviderTypeFile)) -} + mockStorageManager := sourcesmocks.NewMockStorageManager(ctrl) + factory := NewRegistryProviderFactory(mockStorageManager) -func TestK8sRegistryDataProvider_GetRegistryName(t *testing.T) { - t.Parallel() - tests := []struct { - name string - configMapName string - namespace string - registryName string - }{ - { - name: "registry name independent of configmap", - configMapName: "my-configmap", - namespace: "default", - registryName: "production-registry", - }, - { - name: "different registry name", - configMapName: "some-configmap", - namespace: "test-namespace", - registryName: "custom-registry", - }, - { - name: "empty registry name", - configMapName: "test-cm", - namespace: "default", - registryName: "", - }, - } + require.NotNil(t, factory) + assert.IsType(t, &DefaultRegistryProviderFactory{}, factory) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - provider := NewK8sRegistryDataProvider(fake.NewSimpleClientset(), tt.configMapName, tt.namespace, tt.registryName) - result := provider.GetRegistryName() - assert.Equal(t, tt.registryName, result) - }) - } + // Verify that the factory has the storage manager injected + concreteFactory, ok := factory.(*DefaultRegistryProviderFactory) + require.True(t, ok) + assert.NotNil(t, concreteFactory.storageManager) } diff --git a/pkg/app/app.go b/pkg/app/app.go new file mode 100644 index 0000000..d4eb327 --- /dev/null +++ b/pkg/app/app.go @@ -0,0 +1,80 @@ +package app + +import ( + "context" + "errors" + "fmt" + "net/http" + "time" + + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stacklok/toolhive/pkg/logger" +) + +// RegistryApp encapsulates all components needed to run the registry API server +// It provides lifecycle management and graceful shutdown capabilities +type RegistryApp struct { + config *config.Config + components *AppComponents + httpServer *http.Server + + // Lifecycle management + ctx context.Context + cancelFunc context.CancelFunc +} + +// Start starts the application components (HTTP server and background sync) +// This method blocks until the HTTP server stops or encounters an error +func (app *RegistryApp) Start() error { + // Start sync coordinator in background + go func() { + if err := app.components.SyncCoordinator.Start(app.ctx); err != nil { + logger.Errorf("Sync coordinator failed: %v", err) + } + }() + + // Start HTTP server (blocks until stopped) + logger.Infof("Server listening on %s", app.httpServer.Addr) + if err := app.httpServer.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { + return fmt.Errorf("HTTP server failed: %w", err) + } + + return nil +} + +// Stop gracefully stops the application with the given timeout +// It stops the sync coordinator and then shuts down the HTTP server +func (app *RegistryApp) Stop(timeout time.Duration) error { + logger.Info("Shutting down server...") + + // Stop sync coordinator first + if err := app.components.SyncCoordinator.Stop(); err != nil { + logger.Errorf("Failed to stop sync coordinator: %v", err) + } + + // Cancel the application context + if app.cancelFunc != nil { + app.cancelFunc() + } + + // Graceful HTTP server shutdown + shutdownCtx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + if err := app.httpServer.Shutdown(shutdownCtx); err != nil { + return fmt.Errorf("server forced to shutdown: %w", err) + } + + logger.Info("Server shutdown complete") + return nil +} + +// GetConfig returns the application configuration +func (app *RegistryApp) GetConfig() *config.Config { + return app.config +} + +// GetHTTPServer returns the HTTP server (useful for testing to get the actual port) +func (app *RegistryApp) GetHTTPServer() *http.Server { + return app.httpServer +} diff --git a/pkg/app/builder.go b/pkg/app/builder.go new file mode 100644 index 0000000..d5d393b --- /dev/null +++ b/pkg/app/builder.go @@ -0,0 +1,346 @@ +package app + +import ( + "context" + "fmt" + "net/http" + "os" + "time" + + "github.com/go-chi/chi/v5/middleware" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/stacklok/toolhive-registry-server/internal/api" + "github.com/stacklok/toolhive-registry-server/internal/service" + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stacklok/toolhive-registry-server/pkg/sources" + "github.com/stacklok/toolhive-registry-server/pkg/status" + pkgsync "github.com/stacklok/toolhive-registry-server/pkg/sync" + "github.com/stacklok/toolhive-registry-server/pkg/sync/coordinator" + "github.com/stacklok/toolhive/pkg/logger" +) + +const ( + defaultDataDir = "./data" + defaultRegistryFile = "./data/registry.json" + defaultStatusFile = "./data/status.json" + defaultHTTPAddress = ":8080" + defaultRequestTimeout = 10 * time.Second + defaultReadTimeout = 10 * time.Second + defaultWriteTimeout = 15 * time.Second + defaultIdleTimeout = 60 * time.Second +) + +// RegistryAppBuilder builds a RegistryApp using the builder pattern +// It supports dependency injection for testing while providing sensible defaults for production +type RegistryAppBuilder struct { + config *config.Config + + // Optional component overrides (primarily for testing) + sourceHandlerFactory sources.SourceHandlerFactory + storageManager sources.StorageManager + statusPersistence status.StatusPersistence + syncManager pkgsync.Manager + registryProvider service.RegistryDataProvider + deploymentProvider service.DeploymentProvider + + // Kubernetes components (optional) + kubeConfig *rest.Config + kubeClient client.Client + kubeScheme *runtime.Scheme + + // HTTP server options + address string + middlewares []func(http.Handler) http.Handler + requestTimeout time.Duration + readTimeout time.Duration + writeTimeout time.Duration + idleTimeout time.Duration + + // Data directories + dataDir string + registryFile string + statusFile string +} + +// NewRegistryAppBuilder creates a new builder with the given configuration +func NewRegistryAppBuilder(cfg *config.Config) *RegistryAppBuilder { + return &RegistryAppBuilder{ + config: cfg, + address: defaultHTTPAddress, + requestTimeout: defaultRequestTimeout, + readTimeout: defaultReadTimeout, + writeTimeout: defaultWriteTimeout, + idleTimeout: defaultIdleTimeout, + dataDir: defaultDataDir, + registryFile: defaultRegistryFile, + statusFile: defaultStatusFile, + } +} + +// WithAddress sets the HTTP server address +func (b *RegistryAppBuilder) WithAddress(addr string) *RegistryAppBuilder { + b.address = addr + return b +} + +// WithMiddlewares sets custom HTTP middlewares +func (b *RegistryAppBuilder) WithMiddlewares(mw ...func(http.Handler) http.Handler) *RegistryAppBuilder { + b.middlewares = mw + return b +} + +// WithDataDirectory sets the data directory for storage and status files +func (b *RegistryAppBuilder) WithDataDirectory(dir string) *RegistryAppBuilder { + b.dataDir = dir + b.registryFile = dir + "/registry.json" + b.statusFile = dir + "/status.json" + return b +} + +// WithKubernetesConfig sets the Kubernetes REST configuration +func (b *RegistryAppBuilder) WithKubernetesConfig(cfg *rest.Config) *RegistryAppBuilder { + b.kubeConfig = cfg + return b +} + +// WithKubernetesClient sets the Kubernetes client (for testing) +func (b *RegistryAppBuilder) WithKubernetesClient(client client.Client, scheme *runtime.Scheme) *RegistryAppBuilder { + b.kubeClient = client + b.kubeScheme = scheme + return b +} + +// WithSourceHandlerFactory allows injecting a custom source handler factory (for testing) +func (b *RegistryAppBuilder) WithSourceHandlerFactory(factory sources.SourceHandlerFactory) *RegistryAppBuilder { + b.sourceHandlerFactory = factory + return b +} + +// WithStorageManager allows injecting a custom storage manager (for testing) +func (b *RegistryAppBuilder) WithStorageManager(sm sources.StorageManager) *RegistryAppBuilder { + b.storageManager = sm + return b +} + +// WithStatusPersistence allows injecting custom status persistence (for testing) +func (b *RegistryAppBuilder) WithStatusPersistence(sp status.StatusPersistence) *RegistryAppBuilder { + b.statusPersistence = sp + return b +} + +// WithSyncManager allows injecting a custom sync manager (for testing) +func (b *RegistryAppBuilder) WithSyncManager(sm pkgsync.Manager) *RegistryAppBuilder { + b.syncManager = sm + return b +} + +// WithRegistryProvider allows injecting a custom registry provider (for testing) +func (b *RegistryAppBuilder) WithRegistryProvider(provider service.RegistryDataProvider) *RegistryAppBuilder { + b.registryProvider = provider + return b +} + +// WithDeploymentProvider allows injecting a custom deployment provider (for testing) +func (b *RegistryAppBuilder) WithDeploymentProvider(provider service.DeploymentProvider) *RegistryAppBuilder { + b.deploymentProvider = provider + return b +} + +// Build constructs the RegistryApp from the builder configuration +func (b *RegistryAppBuilder) Build(ctx context.Context) (*RegistryApp, error) { + // Validate config + if err := b.config.Validate(); err != nil { + return nil, fmt.Errorf("invalid configuration: %w", err) + } + + // Build Kubernetes components if needed (non-fatal if unavailable) + kubeComponents, err := b.buildKubernetesComponents() + if err != nil { + logger.Warnf("Kubernetes components unavailable: %v", err) + logger.Warn("ConfigMap source and deployment provider will not be available") + } + + // Build sync components + syncCoordinator, err := b.buildSyncComponents(kubeComponents) + if err != nil { + return nil, fmt.Errorf("failed to build sync components: %w", err) + } + + // Build service components + registryService, err := b.buildServiceComponents(ctx, kubeComponents) + if err != nil { + return nil, fmt.Errorf("failed to build service components: %w", err) + } + + // Build HTTP server + httpServer, err := b.buildHTTPServer(registryService) + if err != nil { + return nil, fmt.Errorf("failed to build HTTP server: %w", err) + } + + // Create application context + appCtx, cancel := context.WithCancel(context.Background()) + + return &RegistryApp{ + config: b.config, + components: &AppComponents{ + SyncCoordinator: syncCoordinator, + RegistryService: registryService, + KubeComponents: kubeComponents, + }, + httpServer: httpServer, + ctx: appCtx, + cancelFunc: cancel, + }, nil +} + +// buildKubernetesComponents builds Kubernetes client, scheme, etc. +// Returns nil if Kubernetes is not available (non-fatal) +func (b *RegistryAppBuilder) buildKubernetesComponents() (*KubernetesComponents, error) { + // Use injected config if provided (for testing) + if b.kubeConfig == nil { + cfg, err := getKubernetesConfig() + if err != nil { + return nil, fmt.Errorf("kubernetes config unavailable: %w", err) + } + b.kubeConfig = cfg + } + + // Use injected client if provided (for testing) + if b.kubeClient == nil { + client, scheme, err := createKubernetesClient(b.kubeConfig) + if err != nil { + return nil, fmt.Errorf("failed to create kubernetes client: %w", err) + } + b.kubeClient = client + b.kubeScheme = scheme + } + + logger.Info("Kubernetes components initialized successfully") + return &KubernetesComponents{ + RestConfig: b.kubeConfig, + Client: b.kubeClient, + Scheme: b.kubeScheme, + }, nil +} + +// buildSyncComponents builds sync manager, coordinator, and related components +func (b *RegistryAppBuilder) buildSyncComponents(kube *KubernetesComponents) (coordinator.Coordinator, error) { + logger.Info("Initializing sync components") + + // Build source handler factory + if b.sourceHandlerFactory == nil { + var k8sClient client.Client + if kube != nil { + k8sClient = kube.Client + } + b.sourceHandlerFactory = sources.NewSourceHandlerFactory(k8sClient) + } + + // Build storage manager + if b.storageManager == nil { + // Ensure data directory exists + if err := os.MkdirAll(b.dataDir, 0755); err != nil { + return nil, fmt.Errorf("failed to create data directory %s: %w", b.dataDir, err) + } + b.storageManager = sources.NewFileStorageManager(b.dataDir) + } + + // Build status persistence + if b.statusPersistence == nil { + b.statusPersistence = status.NewFileStatusPersistence(b.statusFile) + } + + // Build sync manager + if b.syncManager == nil { + var k8sClient client.Client + var scheme *runtime.Scheme + if kube != nil { + k8sClient = kube.Client + scheme = kube.Scheme + } + b.syncManager = pkgsync.NewDefaultSyncManager( + k8sClient, + scheme, + b.sourceHandlerFactory, + b.storageManager, + ) + } + + // Create coordinator + syncCoordinator := coordinator.New(b.syncManager, b.statusPersistence, b.config) + logger.Info("Sync components initialized successfully") + + return syncCoordinator, nil +} + +// buildServiceComponents builds registry service and providers +func (b *RegistryAppBuilder) buildServiceComponents(ctx context.Context, kube *KubernetesComponents) (service.RegistryService, error) { + logger.Info("Initializing service components") + + // Build registry provider (reads from synced data via StorageManager) + if b.registryProvider == nil { + // StorageManager was already built in buildSyncComponents + factory := service.NewRegistryProviderFactory(b.storageManager) + provider, err := factory.CreateProvider(b.config) + if err != nil { + return nil, fmt.Errorf("failed to create registry provider: %w", err) + } + b.registryProvider = provider + logger.Infof("Created registry data provider using storage manager") + } + + // Build deployment provider (optional, requires Kubernetes) + if b.deploymentProvider == nil && kube != nil { + provider, err := service.NewK8sDeploymentProvider(kube.RestConfig, b.config.GetRegistryName()) + if err != nil { + logger.Warnf("Failed to create deployment provider: %v", err) + } else { + b.deploymentProvider = provider + logger.Infof("Created Kubernetes deployment provider for registry: %s", b.config.GetRegistryName()) + } + } + + // Create service + svc, err := service.NewService(ctx, b.registryProvider, b.deploymentProvider) + if err != nil { + return nil, fmt.Errorf("failed to create registry service: %w", err) + } + + logger.Info("Service components initialized successfully") + return svc, nil +} + +// buildHTTPServer builds the HTTP server with router and middleware +func (b *RegistryAppBuilder) buildHTTPServer(svc service.RegistryService) (*http.Server, error) { + logger.Info("Initializing HTTP server") + + // Use default middlewares if not provided + if b.middlewares == nil { + b.middlewares = []func(http.Handler) http.Handler{ + middleware.RequestID, + middleware.RealIP, + middleware.Recoverer, + middleware.Timeout(b.requestTimeout), + api.LoggingMiddleware, + } + } + + // Create router with middlewares + router := api.NewServer(svc, api.WithMiddlewares(b.middlewares...)) + + // Create HTTP server + server := &http.Server{ + Addr: b.address, + Handler: router, + ReadTimeout: b.readTimeout, + WriteTimeout: b.writeTimeout, + IdleTimeout: b.idleTimeout, + } + + logger.Infof("HTTP server configured on %s", b.address) + return server, nil +} diff --git a/pkg/app/builder_test.go b/pkg/app/builder_test.go new file mode 100644 index 0000000..7f94e2e --- /dev/null +++ b/pkg/app/builder_test.go @@ -0,0 +1,93 @@ +package app + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-registry-server/pkg/config" +) + +func TestNewRegistryAppBuilder(t *testing.T) { + cfg := &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{ + Path: "/tmp/test-registry.json", + }, + }, + SyncPolicy: &config.SyncPolicyConfig{ + Interval: "30m", + }, + } + + builder := NewRegistryAppBuilder(cfg) + require.NotNil(t, builder) + assert.Equal(t, cfg, builder.config) + assert.Equal(t, defaultHTTPAddress, builder.address) + assert.Equal(t, defaultDataDir, builder.dataDir) +} + +func TestRegistryAppBuilder_WithAddress(t *testing.T) { + cfg := createValidTestConfig() + builder := NewRegistryAppBuilder(cfg).WithAddress(":9090") + + assert.Equal(t, ":9090", builder.address) +} + +func TestRegistryAppBuilder_WithDataDirectory(t *testing.T) { + cfg := createValidTestConfig() + builder := NewRegistryAppBuilder(cfg).WithDataDirectory("/custom/data") + + assert.Equal(t, "/custom/data", builder.dataDir) + assert.Equal(t, "/custom/data/registry.json", builder.registryFile) + assert.Equal(t, "/custom/data/status.json", builder.statusFile) +} + +func TestRegistryAppBuilder_Build_InvalidConfig(t *testing.T) { + // Config with no source type + cfg := &config.Config{ + SyncPolicy: &config.SyncPolicyConfig{ + Interval: "30m", + }, + } + + builder := NewRegistryAppBuilder(cfg) + _, err := builder.Build(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid configuration") +} + +func TestRegistryAppBuilder_ChainedBuilder(t *testing.T) { + cfg := createValidTestConfig() + + builder := NewRegistryAppBuilder(cfg). + WithAddress(":8888"). + WithDataDirectory("/tmp/test-data") + + require.NotNil(t, builder) + assert.Equal(t, ":8888", builder.address) + assert.Equal(t, "/tmp/test-data", builder.dataDir) +} + +// createValidTestConfig creates a minimal valid config for testing +func createValidTestConfig() *config.Config { + return &config.Config{ + RegistryName: "test-registry", + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{ + Path: "/tmp/test-registry.json", + }, + }, + SyncPolicy: &config.SyncPolicyConfig{ + Interval: "30m", + }, + } +} diff --git a/pkg/app/components.go b/pkg/app/components.go new file mode 100644 index 0000000..ee11f85 --- /dev/null +++ b/pkg/app/components.go @@ -0,0 +1,35 @@ +package app + +import ( + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/stacklok/toolhive-registry-server/internal/service" + "github.com/stacklok/toolhive-registry-server/pkg/sync/coordinator" +) + +// AppComponents groups all application components +type AppComponents struct { + // SyncCoordinator manages background synchronization + SyncCoordinator coordinator.Coordinator + + // RegistryService provides registry business logic + RegistryService service.RegistryService + + // KubeComponents contains optional Kubernetes resources + KubeComponents *KubernetesComponents +} + +// KubernetesComponents groups Kubernetes-related components +// These are optional and may be nil if Kubernetes is not available +type KubernetesComponents struct { + // RestConfig is the Kubernetes REST configuration + RestConfig *rest.Config + + // Client is the controller-runtime Kubernetes client + Client client.Client + + // Scheme contains registered Kubernetes types + Scheme *runtime.Scheme +} diff --git a/pkg/app/kubernetes.go b/pkg/app/kubernetes.go new file mode 100644 index 0000000..e3cee12 --- /dev/null +++ b/pkg/app/kubernetes.go @@ -0,0 +1,45 @@ +package app + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/clientcmd" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// getKubernetesConfig returns a Kubernetes REST config +// It tries in-cluster config first, then falls back to kubeconfig +func getKubernetesConfig() (*rest.Config, error) { + // Try in-cluster config first + cfg, err := rest.InClusterConfig() + if err == nil { + return cfg, nil + } + + // Fall back to kubeconfig + loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() + configOverrides := &clientcmd.ConfigOverrides{} + kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides) + return kubeConfig.ClientConfig() +} + +// createKubernetesClient creates a controller-runtime Kubernetes client +// with the necessary scheme registered +func createKubernetesClient(restConfig *rest.Config) (client.Client, *runtime.Scheme, error) { + // Create and register scheme + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + return nil, nil, fmt.Errorf("failed to add Kubernetes core types to scheme: %w", err) + } + + // Create client + k8sClient, err := client.New(restConfig, client.Options{Scheme: scheme}) + if err != nil { + return nil, nil, fmt.Errorf("failed to create Kubernetes client: %w", err) + } + + return k8sClient, scheme, nil +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 28bb839..49059b9 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -9,14 +9,17 @@ import ( ) const ( - // RegistrySourceTypeConfigMap is the type for registry data stored in ConfigMaps + // SourceTypeConfigMap is the type for registry data stored in ConfigMaps SourceTypeConfigMap = "configmap" - // RegistrySourceTypeGit is the type for registry data stored in Git repositories + // SourceTypeGit is the type for registry data stored in Git repositories SourceTypeGit = "git" // SourceTypeAPI is the type for registry data fetched from API endpoints SourceTypeAPI = "api" + + // SourceTypeFile is the type for registry data stored in local files + SourceTypeFile = "file" ) const ( @@ -34,9 +37,12 @@ type ConfigLoader interface { // Config represents the root configuration structure type Config struct { - Source SourceConfig `yaml:"source"` - SyncPolicy *SyncPolicyConfig `yaml:"syncPolicy,omitempty"` - Filter *FilterConfig `yaml:"filter,omitempty"` + // RegistryName is the name/identifier for this registry instance + // Defaults to "default" if not specified + RegistryName string `yaml:"registryName,omitempty"` + Source SourceConfig `yaml:"source"` + SyncPolicy *SyncPolicyConfig `yaml:"syncPolicy,omitempty"` + Filter *FilterConfig `yaml:"filter,omitempty"` } // SourceConfig defines the data source configuration @@ -46,6 +52,7 @@ type SourceConfig struct { ConfigMap *ConfigMapConfig `yaml:"configmap,omitempty"` Git *GitConfig `yaml:"git,omitempty"` API *APIConfig `yaml:"api,omitempty"` + File *FileConfig `yaml:"file,omitempty"` } // ConfigMapConfig defines Kubernetes ConfigMap source settings @@ -84,6 +91,13 @@ type APIConfig struct { Endpoint string `yaml:"endpoint"` } +// FileConfig defines local file source configuration +type FileConfig struct { + // Path is the path to the registry.json file on the local filesystem + // Can be absolute or relative to the working directory + Path string `yaml:"path"` +} + // SyncPolicyConfig defines synchronization settings type SyncPolicyConfig struct { Interval string `yaml:"interval"` @@ -132,6 +146,14 @@ func (c *configLoader) LoadConfig(path string) (*Config, error) { return &config, nil } +// GetRegistryName returns the registry name, using "default" if not specified +func (c *Config) GetRegistryName() string { + if c.RegistryName == "" { + return "default" + } + return c.RegistryName +} + // Validate performs validation on the configuration func (c *Config) Validate() error { if c == nil { @@ -143,18 +165,46 @@ func (c *Config) Validate() error { return fmt.Errorf("source.type is required") } - // Validate ConfigMap settings if type is configmap - if c.Source.Type == "configmap" { + // Validate source-specific settings + switch c.Source.Type { + case SourceTypeConfigMap: if c.Source.ConfigMap == nil { return fmt.Errorf("source.configmap is required when type is configmap") } if c.Source.ConfigMap.Name == "" { return fmt.Errorf("source.configmap.name is required") } + + case SourceTypeGit: + if c.Source.Git == nil { + return fmt.Errorf("source.git is required when type is git") + } + if c.Source.Git.Repository == "" { + return fmt.Errorf("source.git.repository is required") + } + + case SourceTypeAPI: + if c.Source.API == nil { + return fmt.Errorf("source.api is required when type is api") + } + if c.Source.API.Endpoint == "" { + return fmt.Errorf("source.api.endpoint is required") + } + + case SourceTypeFile: + if c.Source.File == nil { + return fmt.Errorf("source.file is required when type is file") + } + if c.Source.File.Path == "" { + return fmt.Errorf("source.file.path is required") + } + + default: + return fmt.Errorf("unsupported source type: %s", c.Source.Type) } // Validate sync policy - if c.SyncPolicy.Interval == "" { + if c.SyncPolicy == nil || c.SyncPolicy.Interval == "" { return fmt.Errorf("syncPolicy.interval is required") } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index e3e5b26..4f14fe0 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -173,7 +173,7 @@ filter: // Create test config file configPath := filepath.Join(tmpDir, "config.yaml") - err := os.WriteFile(configPath, []byte(tt.yamlContent), 0644) + err := os.WriteFile(configPath, []byte(tt.yamlContent), 0600) require.NoError(t, err) // Load the config @@ -229,7 +229,7 @@ filter: include: ["prod", "stable"] exclude: ["beta", "alpha"]` - err := os.WriteFile(configPath, []byte(yamlContent), 0644) + err := os.WriteFile(configPath, []byte(yamlContent), 0600) require.NoError(t, err) // Load it back @@ -341,6 +341,62 @@ func TestConfigValidate(t *testing.T) { wantErr: true, errMsg: "config cannot be nil", }, + { + name: "valid_file_source", + config: &Config{ + RegistryName: "test-registry", + Source: SourceConfig{ + Type: "file", + File: &FileConfig{ + Path: "/tmp/registry.json", + }, + }, + SyncPolicy: &SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: false, + }, + { + name: "missing_file_config", + config: &Config{ + Source: SourceConfig{ + Type: "file", + }, + SyncPolicy: &SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.file is required", + }, + { + name: "missing_file_path", + config: &Config{ + Source: SourceConfig{ + Type: "file", + File: &FileConfig{}, + }, + SyncPolicy: &SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.file.path is required", + }, + { + name: "unsupported_source_type", + config: &Config{ + Source: SourceConfig{ + Type: "unknown", + }, + SyncPolicy: &SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "unsupported source type", + }, } for _, tt := range tests { @@ -358,3 +414,38 @@ func TestConfigValidate(t *testing.T) { }) } } + +func TestGetRegistryName(t *testing.T) { + tests := []struct { + name string + config *Config + expected string + }{ + { + name: "with_registry_name", + config: &Config{ + RegistryName: "my-registry", + }, + expected: "my-registry", + }, + { + name: "without_registry_name", + config: &Config{}, + expected: "default", + }, + { + name: "empty_registry_name", + config: &Config{ + RegistryName: "", + }, + expected: "default", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.config.GetRegistryName() + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/pkg/sources/doc.go b/pkg/sources/doc.go index bc4b673..2c2f467 100644 --- a/pkg/sources/doc.go +++ b/pkg/sources/doc.go @@ -8,7 +8,7 @@ // // Architecture: // - SourceHandler: Interface for fetching and validating registry data -// - StorageManager: Interface for persisting registry data to ConfigMaps +// - StorageManager: Interface for persisting registry data to local storage // - SourceDataValidator: Validates and parses registry data in different formats // - FetchResult: Strongly-typed result containing Registry instances with metadata // @@ -19,7 +19,7 @@ // Supports public repos via HTTPS with branch/tag/commit checkout // - APISourceHandler: Retrieves registry data from HTTP/HTTPS endpoints // Delegates to format-specific handlers (ToolHiveAPIHandler, UpstreamAPIHandler) -// - ConfigMapStorageManager: Persists Registry data to Kubernetes ConfigMaps +// - FileStorageManager: Persists Registry data to local file storage for serving // // The package provides a factory pattern for creating appropriate // source handlers based on the source type configuration, and uses diff --git a/pkg/sources/factory.go b/pkg/sources/factory.go index 364495a..96f7c7d 100644 --- a/pkg/sources/factory.go +++ b/pkg/sources/factory.go @@ -28,6 +28,8 @@ func (f *DefaultSourceHandlerFactory) CreateHandler(sourceType string) (SourceHa return NewGitSourceHandler(), nil case config.SourceTypeAPI: return NewAPISourceHandler(), nil + case config.SourceTypeFile: + return NewFileSourceHandler(), nil default: return nil, fmt.Errorf("unsupported source type: %s", sourceType) } diff --git a/pkg/sources/file.go b/pkg/sources/file.go new file mode 100644 index 0000000..df1b2a9 --- /dev/null +++ b/pkg/sources/file.go @@ -0,0 +1,95 @@ +package sources + +import ( + "context" + "crypto/sha256" + "fmt" + "os" + + "github.com/stacklok/toolhive-registry-server/pkg/config" +) + +// FileSourceHandler handles registry data from local files +type FileSourceHandler struct { + validator SourceDataValidator +} + +// NewFileSourceHandler creates a new file source handler +func NewFileSourceHandler() *FileSourceHandler { + return &FileSourceHandler{ + validator: NewSourceDataValidator(), + } +} + +// Validate validates the file source configuration +func (*FileSourceHandler) Validate(source *config.SourceConfig) error { + if source.Type != config.SourceTypeFile { + return fmt.Errorf("invalid source type: expected %s, got %s", + config.SourceTypeFile, source.Type) + } + + if source.File == nil { + return fmt.Errorf("file configuration is required for source type %s", + config.SourceTypeFile) + } + + if source.File.Path == "" { + return fmt.Errorf("file path cannot be empty") + } + + return nil +} + +// FetchRegistry retrieves registry data from the local file +func (h *FileSourceHandler) FetchRegistry(ctx context.Context, registryConfig *config.Config) (*FetchResult, error) { + // Fetch file data + data, hash, err := h.fetchFileData(ctx, registryConfig) + if err != nil { + return nil, fmt.Errorf("failed to fetch file data: %w", err) + } + + // Validate and parse data + reg, err := h.validator.ValidateData(data, registryConfig.Source.Format) + if err != nil { + return nil, fmt.Errorf("validation failed: %w", err) + } + + // Return result + return NewFetchResult(reg, hash, registryConfig.Source.Format), nil +} + +// fetchFileData reads the file and calculates its hash +func (h *FileSourceHandler) fetchFileData(ctx context.Context, registryConfig *config.Config) ([]byte, string, error) { + // Validate source configuration + if err := h.Validate(®istryConfig.Source); err != nil { + return nil, "", fmt.Errorf("source validation failed: %w", err) + } + + filePath := registryConfig.Source.File.Path + + // Read the file + data, err := os.ReadFile(filePath) + if err != nil { + if os.IsNotExist(err) { + return nil, "", fmt.Errorf("file not found: %s", filePath) + } + return nil, "", fmt.Errorf("failed to read file %s: %w", filePath, err) + } + + // Calculate hash + hash := fmt.Sprintf("%x", sha256.Sum256(data)) + + return data, hash, nil +} + +// CurrentHash returns the current hash of the file without performing a full parse +func (h *FileSourceHandler) CurrentHash(ctx context.Context, registryConfig *config.Config) (string, error) { + // For file sources, we read and hash the file + // This is nearly as expensive as a full fetch, but maintains the interface + _, hash, err := h.fetchFileData(ctx, registryConfig) + if err != nil { + return "", err + } + + return hash, nil +} diff --git a/pkg/sources/file_test.go b/pkg/sources/file_test.go new file mode 100644 index 0000000..c15ace3 --- /dev/null +++ b/pkg/sources/file_test.go @@ -0,0 +1,221 @@ +package sources + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewFileSourceHandler(t *testing.T) { + t.Parallel() + + handler := NewFileSourceHandler() + assert.NotNil(t, handler) + assert.NotNil(t, handler.validator) +} + +func TestFileSourceHandler_Validate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + source *config.SourceConfig + wantErr bool + errMsg string + }{ + { + name: "valid_file_source", + source: &config.SourceConfig{ + Type: config.SourceTypeFile, + File: &config.FileConfig{ + Path: "/tmp/registry.json", + }, + }, + wantErr: false, + }, + { + name: "invalid_source_type", + source: &config.SourceConfig{ + Type: config.SourceTypeGit, + File: &config.FileConfig{ + Path: "/tmp/registry.json", + }, + }, + wantErr: true, + errMsg: "invalid source type", + }, + { + name: "missing_file_configuration", + source: &config.SourceConfig{ + Type: config.SourceTypeFile, + }, + wantErr: true, + errMsg: "file configuration is required", + }, + { + name: "empty_file_path", + source: &config.SourceConfig{ + Type: config.SourceTypeFile, + File: &config.FileConfig{ + Path: "", + }, + }, + wantErr: true, + errMsg: "file path cannot be empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handler := NewFileSourceHandler() + err := handler.Validate(tt.source) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + } else { + require.NoError(t, err) + } + }) + } +} + +func TestFileSourceHandler_FetchRegistry(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setupFile func(t *testing.T) string + config *config.Config + wantErr bool + errMsg string + checkResult func(t *testing.T, result *FetchResult) + }{ + { + name: "successful_fetch_toolhive_format", + setupFile: func(t *testing.T) string { + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, "registry.json") + registryData := `{ + "version": "1.0.0", + "last_updated": "2024-01-01T00:00:00Z", + "servers": {} + }` + err := os.WriteFile(filePath, []byte(registryData), 0644) + require.NoError(t, err) + return filePath + }, + config: &config.Config{ + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + }, + }, + wantErr: false, + checkResult: func(t *testing.T, result *FetchResult) { + assert.NotNil(t, result) + assert.NotNil(t, result.Registry) + assert.NotEmpty(t, result.Hash) + assert.Equal(t, config.SourceFormatToolHive, result.Format) + }, + }, + { + name: "file_not_found", + setupFile: func(t *testing.T) string { + return "/nonexistent/path/registry.json" + }, + config: &config.Config{ + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + }, + }, + wantErr: true, + errMsg: "file not found", + }, + { + name: "invalid_json", + setupFile: func(t *testing.T) string { + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, "invalid.json") + err := os.WriteFile(filePath, []byte("invalid json {"), 0644) + require.NoError(t, err) + return filePath + }, + config: &config.Config{ + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + }, + }, + wantErr: true, + errMsg: "validation failed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + filePath := tt.setupFile(t) + tt.config.Source.File = &config.FileConfig{ + Path: filePath, + } + + handler := NewFileSourceHandler() + result, err := handler.FetchRegistry(context.Background(), tt.config) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + } else { + require.NoError(t, err) + if tt.checkResult != nil { + tt.checkResult(t, result) + } + } + }) + } +} + +func TestFileSourceHandler_CurrentHash(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, "registry.json") + registryData := `{ + "version": "1.0.0", + "last_updated": "2024-01-01T00:00:00Z", + "servers": {} + }` + err := os.WriteFile(filePath, []byte(registryData), 0644) + require.NoError(t, err) + + cfg := &config.Config{ + Source: config.SourceConfig{ + Type: config.SourceTypeFile, + Format: config.SourceFormatToolHive, + File: &config.FileConfig{ + Path: filePath, + }, + }, + } + + handler := NewFileSourceHandler() + hash, err := handler.CurrentHash(context.Background(), cfg) + + require.NoError(t, err) + assert.NotEmpty(t, hash) + + // Hash should be consistent + hash2, err := handler.CurrentHash(context.Background(), cfg) + require.NoError(t, err) + assert.Equal(t, hash, hash2) +} diff --git a/pkg/sources/storage_manager.go b/pkg/sources/storage_manager.go index 41b2bb1..1f1eea6 100644 --- a/pkg/sources/storage_manager.go +++ b/pkg/sources/storage_manager.go @@ -2,7 +2,10 @@ package sources import ( "context" + "encoding/json" "fmt" + "os" + "path/filepath" "github.com/stacklok/toolhive-registry-server/pkg/config" "github.com/stacklok/toolhive/pkg/registry" @@ -14,8 +17,8 @@ const ( // RegistryStorageComponent is the component label for the registry storage RegistryStorageComponent = "registry-storage" - // StorageTypeConfigMap identifies the ConfigMap storage manager implementation - StorageTypeConfigMap = "configmap" + // RegistryFileName is the name of the registry data file + RegistryFileName = "registry.json" ) //go:generate mockgen -destination=mocks/mock_storage_manager.go -package=mocks -source=storage_manager.go StorageManager @@ -32,7 +35,82 @@ type StorageManager interface { Delete(ctx context.Context, config *config.Config) error } -// NewStorageManager creates a new storage manager -func NewStorageManager() (StorageManager, error) { - return nil, fmt.Errorf("storage manager not yet implemented") +// FileStorageManager implements StorageManager using local filesystem +type FileStorageManager struct { + basePath string +} + +// NewFileStorageManager creates a new file-based storage manager +func NewFileStorageManager(basePath string) StorageManager { + return &FileStorageManager{ + basePath: basePath, + } +} + +// Store saves the registry data to a JSON file +func (f *FileStorageManager) Store(ctx context.Context, config *config.Config, reg *registry.Registry) error { + // Create base directory if it doesn't exist + if err := os.MkdirAll(f.basePath, 0755); err != nil { + return fmt.Errorf("failed to create storage directory: %w", err) + } + + filePath := filepath.Join(f.basePath, RegistryFileName) + + // Marshal registry to JSON with pretty printing for readability + data, err := json.MarshalIndent(reg, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal registry data: %w", err) + } + + // Write to temporary file first for atomic operation + tempPath := filePath + ".tmp" + if err := os.WriteFile(tempPath, data, 0600); err != nil { + return fmt.Errorf("failed to write temporary registry file: %w", err) + } + + // Atomic rename + if err := os.Rename(tempPath, filePath); err != nil { + // Clean up temp file on error + _ = os.Remove(tempPath) + return fmt.Errorf("failed to rename registry file: %w", err) + } + + return nil +} + +// Get retrieves and parses registry data from the JSON file +func (f *FileStorageManager) Get(ctx context.Context, config *config.Config) (*registry.Registry, error) { + filePath := filepath.Join(f.basePath, RegistryFileName) + + // Read file + data, err := os.ReadFile(filePath) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("registry file not found: %w", err) + } + return nil, fmt.Errorf("failed to read registry file: %w", err) + } + + // Unmarshal JSON + var reg registry.Registry + if err := json.Unmarshal(data, ®); err != nil { + return nil, fmt.Errorf("failed to unmarshal registry data: %w", err) + } + + return ®, nil +} + +// Delete removes the registry data file +func (f *FileStorageManager) Delete(ctx context.Context, config *config.Config) error { + filePath := filepath.Join(f.basePath, RegistryFileName) + + if err := os.Remove(filePath); err != nil { + if os.IsNotExist(err) { + // File doesn't exist, nothing to delete + return nil + } + return fmt.Errorf("failed to delete registry file: %w", err) + } + + return nil } diff --git a/pkg/sources/storage_manager_test.go b/pkg/sources/storage_manager_test.go index 740c993..2c14cb2 100644 --- a/pkg/sources/storage_manager_test.go +++ b/pkg/sources/storage_manager_test.go @@ -1,15 +1,126 @@ package sources import ( + "context" + "os" + "path/filepath" "testing" + "github.com/stacklok/toolhive/pkg/registry" "github.com/stretchr/testify/require" ) -func TestNewConfigMapStorageManager(t *testing.T) { +func TestFileStorageManager_StoreAndGet(t *testing.T) { t.Parallel() - manager, err := NewStorageManager() + // Create temporary directory for test + tmpDir := t.TempDir() + + manager := NewFileStorageManager(tmpDir) + require.NotNil(t, manager) + + // Create a test registry + testRegistry := ®istry.Registry{ + Version: "1.0.0", + LastUpdated: "2024-01-01T00:00:00Z", + Servers: make(map[string]*registry.ImageMetadata), + } + + // Store the registry + ctx := context.Background() + err := manager.Store(ctx, nil, testRegistry) + require.NoError(t, err) + + // Verify file was created + filePath := filepath.Join(tmpDir, RegistryFileName) + _, err = os.Stat(filePath) + require.NoError(t, err) + + // Get the registry back + retrieved, err := manager.Get(ctx, nil) + require.NoError(t, err) + require.NotNil(t, retrieved) + require.Equal(t, testRegistry.Version, retrieved.Version) + require.Equal(t, testRegistry.LastUpdated, retrieved.LastUpdated) +} + +func TestFileStorageManager_Delete(t *testing.T) { + t.Parallel() + + // Create temporary directory for test + tmpDir := t.TempDir() + + manager := NewFileStorageManager(tmpDir) + require.NotNil(t, manager) + + // Create and store a test registry + testRegistry := ®istry.Registry{ + Version: "1.0.0", + } + + ctx := context.Background() + err := manager.Store(ctx, nil, testRegistry) + require.NoError(t, err) + + // Delete the registry + err = manager.Delete(ctx, nil) + require.NoError(t, err) + + // Verify file was deleted + filePath := filepath.Join(tmpDir, RegistryFileName) + _, err = os.Stat(filePath) + require.True(t, os.IsNotExist(err)) +} + +func TestFileStorageManager_GetNonExistent(t *testing.T) { + t.Parallel() + + // Create temporary directory for test + tmpDir := t.TempDir() + + manager := NewFileStorageManager(tmpDir) + require.NotNil(t, manager) + + // Try to get non-existent registry + ctx := context.Background() + _, err := manager.Get(ctx, nil) require.Error(t, err) - require.Nil(t, manager) +} + +func TestFileStorageManager_Delete_PermissionDenied(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + manager := NewFileStorageManager(tmpDir) + + // Create and store a file + testRegistry := ®istry.Registry{Version: "1.0.0"} + ctx := context.Background() + err := manager.Store(ctx, nil, testRegistry) + require.NoError(t, err) + + // Make directory read-only to prevent deletion + err = os.Chmod(tmpDir, 0555) // Read + execute only, no write + require.NoError(t, err) + + // Try to delete - should fail with permission error + err = manager.Delete(ctx, nil) + require.Error(t, err) // + require.Contains(t, err.Error(), "failed to delete registry file") + + _ = os.Chmod(tmpDir, 0755) // Restore permissions +} + +func TestFileStorageManager_Delete_NonExistent(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + manager := NewFileStorageManager(tmpDir) + + // Try to delete file that doesn't exist + ctx := context.Background() + err := manager.Delete(ctx, nil) + + // Should succeed (idempotent operation) + require.NoError(t, err) } diff --git a/pkg/status/persistence.go b/pkg/status/persistence.go new file mode 100644 index 0000000..dad93db --- /dev/null +++ b/pkg/status/persistence.go @@ -0,0 +1,88 @@ +package status + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" +) + +const ( + // StatusFileName is the name of the status file + StatusFileName = "status.json" +) + +// StatusPersistence defines the interface for sync status persistence +type StatusPersistence interface { + // SaveStatus saves the sync status to persistent storage + SaveStatus(ctx context.Context, status *SyncStatus) error + + // LoadStatus loads the sync status from persistent storage + // Returns an empty SyncStatus if the file doesn't exist (first run) + LoadStatus(ctx context.Context) (*SyncStatus, error) +} + +// FileStatusPersistence implements StatusPersistence using local filesystem +type FileStatusPersistence struct { + filePath string +} + +// NewFileStatusPersistence creates a new file-based status persistence +func NewFileStatusPersistence(filePath string) StatusPersistence { + return &FileStatusPersistence{ + filePath: filePath, + } +} + +// SaveStatus saves the sync status to a JSON file +func (f *FileStatusPersistence) SaveStatus(ctx context.Context, status *SyncStatus) error { + // Create directory if it doesn't exist + dir := filepath.Dir(f.filePath) + if err := os.MkdirAll(dir, 0755); err != nil { + return fmt.Errorf("failed to create status directory: %w", err) + } + + // Marshal status to JSON with pretty printing for readability + data, err := json.MarshalIndent(status, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal status data: %w", err) + } + + // Write to temporary file first for atomic operation + tempPath := f.filePath + ".tmp" + if err := os.WriteFile(tempPath, data, 0600); err != nil { + return fmt.Errorf("failed to write temporary status file: %w", err) + } + + // Atomic rename + if err := os.Rename(tempPath, f.filePath); err != nil { + // Clean up temp file on error + _ = os.Remove(tempPath) + return fmt.Errorf("failed to rename status file: %w", err) + } + + return nil +} + +// LoadStatus loads the sync status from a JSON file +// Returns an empty SyncStatus if the file doesn't exist +func (f *FileStatusPersistence) LoadStatus(ctx context.Context) (*SyncStatus, error) { + // Read file + data, err := os.ReadFile(f.filePath) + if err != nil { + if os.IsNotExist(err) { + // File doesn't exist - this is OK for first run + return &SyncStatus{}, nil + } + return nil, fmt.Errorf("failed to read status file: %w", err) + } + + // Unmarshal JSON + var status SyncStatus + if err := json.Unmarshal(data, &status); err != nil { + return nil, fmt.Errorf("failed to unmarshal status data: %w", err) + } + + return &status, nil +} diff --git a/pkg/status/persistence_test.go b/pkg/status/persistence_test.go new file mode 100644 index 0000000..88bac14 --- /dev/null +++ b/pkg/status/persistence_test.go @@ -0,0 +1,147 @@ +package status + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestFileStatusPersistence_SaveAndLoad(t *testing.T) { + t.Parallel() + + // Create temporary directory for test + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, StatusFileName) + + persistence := NewFileStatusPersistence(filePath) + require.NotNil(t, persistence) + + // Create a test status + now := time.Now() + testStatus := &SyncStatus{ + Phase: SyncPhaseComplete, + Message: "Test sync completed", + LastAttempt: &now, + AttemptCount: 1, + LastSyncTime: &now, + LastSyncHash: "abc123", + ServerCount: 5, + } + + // Save the status + ctx := context.Background() + err := persistence.SaveStatus(ctx, testStatus) + require.NoError(t, err) + + // Verify file was created + _, err = os.Stat(filePath) + require.NoError(t, err) + + // Load the status back + loaded, err := persistence.LoadStatus(ctx) + require.NoError(t, err) + require.NotNil(t, loaded) + require.Equal(t, testStatus.Phase, loaded.Phase) + require.Equal(t, testStatus.Message, loaded.Message) + require.Equal(t, testStatus.AttemptCount, loaded.AttemptCount) + require.Equal(t, testStatus.LastSyncHash, loaded.LastSyncHash) + require.Equal(t, testStatus.ServerCount, loaded.ServerCount) +} + +func TestFileStatusPersistence_LoadNonExistent(t *testing.T) { + t.Parallel() + + // Create temporary directory for test + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, StatusFileName) + + persistence := NewFileStatusPersistence(filePath) + require.NotNil(t, persistence) + + // Load non-existent status should return empty status + ctx := context.Background() + loaded, err := persistence.LoadStatus(ctx) + require.NoError(t, err) + require.NotNil(t, loaded) + require.Equal(t, SyncPhase(""), loaded.Phase) + require.Equal(t, "", loaded.Message) +} + +func TestFileStatusPersistence_UpdateStatus(t *testing.T) { + t.Parallel() + + // Create temporary directory for test + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, StatusFileName) + + persistence := NewFileStatusPersistence(filePath) + require.NotNil(t, persistence) + + ctx := context.Background() + + // Save initial status + now1 := time.Now() + initialStatus := &SyncStatus{ + Phase: SyncPhaseSyncing, + Message: "Syncing...", + LastAttempt: &now1, + AttemptCount: 1, + } + err := persistence.SaveStatus(ctx, initialStatus) + require.NoError(t, err) + + // Update status + now2 := time.Now() + updatedStatus := &SyncStatus{ + Phase: SyncPhaseComplete, + Message: "Sync completed", + LastAttempt: &now2, + AttemptCount: 0, + LastSyncTime: &now2, + LastSyncHash: "xyz789", + ServerCount: 10, + } + err = persistence.SaveStatus(ctx, updatedStatus) + require.NoError(t, err) + + // Load and verify it was updated + loaded, err := persistence.LoadStatus(ctx) + require.NoError(t, err) + require.NotNil(t, loaded) + require.Equal(t, SyncPhaseComplete, loaded.Phase) + require.Equal(t, "Sync completed", loaded.Message) + require.Equal(t, 0, loaded.AttemptCount) + require.Equal(t, "xyz789", loaded.LastSyncHash) + require.Equal(t, 10, loaded.ServerCount) +} + +func TestFileStatusPersistence_AtomicWrite(t *testing.T) { + t.Parallel() + + // Create temporary directory for test + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, StatusFileName) + + persistence := NewFileStatusPersistence(filePath) + require.NotNil(t, persistence) + + ctx := context.Background() + + // Save status + now := time.Now() + testStatus := &SyncStatus{ + Phase: SyncPhaseComplete, + LastAttempt: &now, + } + err := persistence.SaveStatus(ctx, testStatus) + require.NoError(t, err) + + // Verify temporary file was cleaned up + tempPath := filePath + ".tmp" + _, err = os.Stat(tempPath) + require.True(t, os.IsNotExist(err), "Temporary file should not exist after save") +} diff --git a/pkg/sync/coordinator/config.go b/pkg/sync/coordinator/config.go new file mode 100644 index 0000000..7b00e1c --- /dev/null +++ b/pkg/sync/coordinator/config.go @@ -0,0 +1,22 @@ +package coordinator + +import ( + "time" + + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stacklok/toolhive/pkg/logger" +) + +// getSyncInterval extracts the sync interval from the policy configuration +func getSyncInterval(policy *config.SyncPolicyConfig) time.Duration { + // Use policy interval if configured + if policy != nil && policy.Interval != "" { + if interval, err := time.ParseDuration(policy.Interval); err == nil { + return interval + } + logger.Warnf("Invalid sync interval '%s', using default: 1m", policy.Interval) + } + + // Default to 1 minute if no valid interval + return time.Minute +} diff --git a/pkg/sync/coordinator/coordinator.go b/pkg/sync/coordinator/coordinator.go new file mode 100644 index 0000000..45b3db9 --- /dev/null +++ b/pkg/sync/coordinator/coordinator.go @@ -0,0 +1,173 @@ +package coordinator + +import ( + "context" + "fmt" + "sync" + "time" + + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stacklok/toolhive-registry-server/pkg/status" + pkgsync "github.com/stacklok/toolhive-registry-server/pkg/sync" + "github.com/stacklok/toolhive/pkg/logger" +) + +// Coordinator manages background synchronization scheduling and execution +type Coordinator interface { + // Start begins background sync coordination + // Blocks until context is cancelled or an unrecoverable error occurs + Start(ctx context.Context) error + + // Stop gracefully stops the coordinator + Stop() error + + // GetStatus returns current sync status (thread-safe) + GetStatus() *status.SyncStatus +} + +// DefaultCoordinator is the default implementation of Coordinator +type DefaultCoordinator struct { + manager pkgsync.Manager + statusPersistence status.StatusPersistence + config *config.Config + + // Thread-safe status management + mu sync.Mutex + cachedStatus *status.SyncStatus + + // Lifecycle management + cancelFunc context.CancelFunc + done chan struct{} +} + +// New creates a new coordinator with injected dependencies +func New( + manager pkgsync.Manager, + statusPersistence status.StatusPersistence, + config *config.Config, +) Coordinator { + return &DefaultCoordinator{ + manager: manager, + statusPersistence: statusPersistence, + config: config, + done: make(chan struct{}), + } +} + +// Start begins background sync coordination +func (c *DefaultCoordinator) Start(ctx context.Context) error { + logger.Info("Starting background sync coordinator") + + // Create cancellable context for this coordinator + coordCtx, cancel := context.WithCancel(ctx) + c.cancelFunc = cancel + defer func() { + close(c.done) + logger.Info("Background sync coordinator shutting down") + }() + + // Load or initialize sync status + if err := c.loadOrInitializeStatus(coordCtx); err != nil { + return fmt.Errorf("failed to initialize sync status: %w", err) + } + + // Get sync interval from policy + interval := getSyncInterval(c.config.SyncPolicy) + logger.Infof("Configured sync interval: %v", interval) + + // Create ticker for periodic sync + ticker := time.NewTicker(interval) + defer ticker.Stop() + + // Perform initial sync check + c.checkSync(coordCtx, "initial") + + // Continue with periodic sync + for { + select { + case <-ticker.C: + c.checkSync(coordCtx, "periodic") + case <-coordCtx.Done(): + return nil + } + } +} + +// Stop gracefully stops the coordinator +func (c *DefaultCoordinator) Stop() error { + if c.cancelFunc != nil { + logger.Info("Stopping sync coordinator...") + c.cancelFunc() + // Wait for coordinator to finish + <-c.done + } + return nil +} + +// GetStatus returns current sync status (thread-safe) +func (c *DefaultCoordinator) GetStatus() *status.SyncStatus { + c.mu.Lock() + defer c.mu.Unlock() + + // Return a copy to prevent external modification + if c.cachedStatus == nil { + return nil + } + statusCopy := *c.cachedStatus + return &statusCopy +} + +// loadOrInitializeStatus loads existing status or creates default +func (c *DefaultCoordinator) loadOrInitializeStatus(ctx context.Context) error { + syncStatus, err := c.statusPersistence.LoadStatus(ctx) + if err != nil { + logger.Warnf("Failed to load sync status, initializing with defaults: %v", err) + syncStatus = &status.SyncStatus{ + Phase: status.SyncPhaseFailed, + Message: "No previous sync status found", + } + } + + // Check if this is a brand new status (no file existed) + if syncStatus.Phase == "" && syncStatus.LastSyncTime == nil { + logger.Info("No previous sync status found, initializing with defaults") + syncStatus.Phase = status.SyncPhaseFailed + syncStatus.Message = "No previous sync status found" + // Persist the default status immediately + if err := c.statusPersistence.SaveStatus(ctx, syncStatus); err != nil { + logger.Warnf("Failed to persist default sync status: %v", err) + } + } else if syncStatus.Phase == status.SyncPhaseSyncing { + // If status was left in Syncing state, it means the previous run was interrupted + // Reset it to Failed so the sync will be triggered + logger.Warn("Previous sync was interrupted (status=Syncing), resetting to Failed") + syncStatus.Phase = status.SyncPhaseFailed + syncStatus.Message = "Previous sync was interrupted" + // Persist the corrected status + if err := c.statusPersistence.SaveStatus(ctx, syncStatus); err != nil { + logger.Warnf("Failed to persist corrected sync status: %v", err) + } + } + + // Log the loaded/initialized status + if syncStatus.LastSyncTime != nil { + logger.Infof("Loaded sync status: phase=%s, last sync at %s, %d servers", + syncStatus.Phase, syncStatus.LastSyncTime.Format(time.RFC3339), syncStatus.ServerCount) + } else { + logger.Infof("Sync status: phase=%s, no previous sync", syncStatus.Phase) + } + + // Store in cached status + c.mu.Lock() + c.cachedStatus = syncStatus + c.mu.Unlock() + + return nil +} + +// withStatus executes a function while holding the status lock +func (c *DefaultCoordinator) withStatus(fn func(*status.SyncStatus)) { + c.mu.Lock() + defer c.mu.Unlock() + fn(c.cachedStatus) +} diff --git a/pkg/sync/coordinator/coordinator_test.go b/pkg/sync/coordinator/coordinator_test.go new file mode 100644 index 0000000..40e9e84 --- /dev/null +++ b/pkg/sync/coordinator/coordinator_test.go @@ -0,0 +1,407 @@ +package coordinator + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/stacklok/toolhive-registry-server/pkg/config" + "github.com/stacklok/toolhive-registry-server/pkg/status" + "github.com/stacklok/toolhive-registry-server/pkg/sync" + syncmocks "github.com/stacklok/toolhive-registry-server/pkg/sync/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" +) + +func TestPerformSync_StatusPersistence(t *testing.T) { + tests := []struct { + name string + syncError *sync.Error + expectedPhase status.SyncPhase + expectedMsg string + shouldHaveHash bool + }{ + { + name: "successful sync updates status to Complete", + syncError: nil, + expectedPhase: status.SyncPhaseComplete, + expectedMsg: "Sync completed successfully", + shouldHaveHash: true, + }, + { + name: "failed sync updates status to Failed", + syncError: &sync.Error{ + Message: "sync failed due to network error", + }, + expectedPhase: status.SyncPhaseFailed, + expectedMsg: "sync failed due to network error", + shouldHaveHash: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + // Create temporary directory for status file + tempDir := t.TempDir() + statusFile := filepath.Join(tempDir, "status.json") + + // Create mock sync manager + mockSyncMgr := syncmocks.NewMockManager(mockCtrl) + + // Setup mock behavior + if tt.syncError == nil { + mockSyncMgr.EXPECT(). + PerformSync(gomock.Any(), gomock.Any()). + Return(&sync.Result{ + Hash: "test-hash-123", + ServerCount: 42, + }, nil) + } else { + mockSyncMgr.EXPECT(). + PerformSync(gomock.Any(), gomock.Any()). + Return(nil, tt.syncError) + } + + // Create status persistence + statusPersistence := status.NewFileStatusPersistence(statusFile) + + // Create coordinator with initial status + cfg := &config.Config{ + RegistryName: "test-registry", + } + + coord := &DefaultCoordinator{ + manager: mockSyncMgr, + statusPersistence: statusPersistence, + config: cfg, + cachedStatus: &status.SyncStatus{ + Phase: status.SyncPhaseFailed, + Message: "Initial state", + }, + } + + // Execute performSync + ctx := context.Background() + coord.performSync(ctx) + + // Verify the status object was updated correctly + assert.Equal(t, tt.expectedPhase, coord.cachedStatus.Phase, "Phase should be updated") + assert.Equal(t, tt.expectedMsg, coord.cachedStatus.Message, "Message should be updated") + + if tt.shouldHaveHash { + assert.Equal(t, "test-hash-123", coord.cachedStatus.LastSyncHash, "Hash should be set") + assert.Equal(t, 42, coord.cachedStatus.ServerCount, "Server count should be set") + assert.NotNil(t, coord.cachedStatus.LastSyncTime, "Last sync time should be set") + } + + // Verify the status was persisted to disk + require.FileExists(t, statusFile, "Status file should be created") + + // Load the persisted status and verify + loadedStatus, err := statusPersistence.LoadStatus(ctx) + require.NoError(t, err, "Should load status from disk") + assert.Equal(t, tt.expectedPhase, loadedStatus.Phase, "Persisted phase should match") + assert.Equal(t, tt.expectedMsg, loadedStatus.Message, "Persisted message should match") + + if tt.shouldHaveHash { + assert.Equal(t, "test-hash-123", loadedStatus.LastSyncHash, "Persisted hash should match") + assert.Equal(t, 42, loadedStatus.ServerCount, "Persisted server count should match") + } + }) + } +} + +func TestPerformSync_AlwaysPersists(t *testing.T) { + t.Run("status is persisted even if sync succeeds", func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + tempDir := t.TempDir() + statusFile := filepath.Join(tempDir, "status.json") + + mockSyncMgr := syncmocks.NewMockManager(mockCtrl) + mockSyncMgr.EXPECT(). + PerformSync(gomock.Any(), gomock.Any()). + Return(&sync.Result{Hash: "hash", ServerCount: 1}, nil) + + statusPersistence := status.NewFileStatusPersistence(statusFile) + cfg := &config.Config{RegistryName: "test"} + + coord := &DefaultCoordinator{ + manager: mockSyncMgr, + statusPersistence: statusPersistence, + config: cfg, + cachedStatus: &status.SyncStatus{}, + } + + coord.performSync(context.Background()) + + // Verify file exists (proves defer executed) + assert.FileExists(t, statusFile) + }) +} + +func TestPerformSync_SyncingPhasePersistedImmediately(t *testing.T) { + t.Run("Syncing phase is persisted before sync starts", func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + tempDir := t.TempDir() + statusFile := filepath.Join(tempDir, "status.json") + statusPersistence := status.NewFileStatusPersistence(statusFile) + + // Create a channel to coordinate the test + syncStarted := make(chan struct{}) + + mockSyncMgr := syncmocks.NewMockManager(mockCtrl) + mockSyncMgr.EXPECT(). + PerformSync(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, cfg *config.Config) (*sync.Result, *sync.Error) { + // Signal that sync has started + close(syncStarted) + + // At this point, the Syncing status should already be persisted + // We'll verify this after the function returns + + // Simulate a long-running sync + time.Sleep(100 * time.Millisecond) + + return &sync.Result{Hash: "test-hash", ServerCount: 10}, nil + }) + + cfg := &config.Config{RegistryName: "test"} + coord := &DefaultCoordinator{ + manager: mockSyncMgr, + statusPersistence: statusPersistence, + config: cfg, + cachedStatus: &status.SyncStatus{ + Phase: status.SyncPhaseFailed, + }, + } + + // Run performSync in a goroutine so we can check the status while it's running + done := make(chan struct{}) + go func() { + coord.performSync(context.Background()) + close(done) + }() + + // Wait for sync to start + <-syncStarted + + // Now verify that the status file has Syncing phase already persisted + loadedStatus, err := statusPersistence.LoadStatus(context.Background()) + assert.NoError(t, err) + assert.Equal(t, status.SyncPhaseSyncing, loadedStatus.Phase, "Syncing phase should be persisted immediately") + assert.Equal(t, "Sync in progress", loadedStatus.Message) + + // Wait for performSync to complete + <-done + + // Final status should be Complete + finalStatus, err := statusPersistence.LoadStatus(context.Background()) + assert.NoError(t, err) + assert.Equal(t, status.SyncPhaseComplete, finalStatus.Phase) + }) +} + +func TestPerformSync_PhaseTransitions(t *testing.T) { + tests := []struct { + name string + initialPhase status.SyncPhase + syncResult *sync.Result + syncError *sync.Error + expectedPhase status.SyncPhase + }{ + { + name: "Failed -> Syncing -> Complete on successful sync", + initialPhase: status.SyncPhaseFailed, + syncResult: &sync.Result{Hash: "abc123", ServerCount: 5}, + syncError: nil, + expectedPhase: status.SyncPhaseComplete, + }, + { + name: "Failed -> Syncing -> Failed on failed sync", + initialPhase: status.SyncPhaseFailed, + syncResult: nil, + syncError: &sync.Error{ + Message: "network timeout", + }, + expectedPhase: status.SyncPhaseFailed, + }, + { + name: "Complete -> Syncing -> Complete on successful resync", + initialPhase: status.SyncPhaseComplete, + syncResult: &sync.Result{Hash: "xyz789", ServerCount: 10}, + syncError: nil, + expectedPhase: status.SyncPhaseComplete, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + tempDir := t.TempDir() + statusFile := filepath.Join(tempDir, "status.json") + statusPersistence := status.NewFileStatusPersistence(statusFile) + + // Track observed phases + observedPhases := []status.SyncPhase{tt.initialPhase} + syncStarted := make(chan struct{}) + + mockSyncMgr := syncmocks.NewMockManager(mockCtrl) + mockSyncMgr.EXPECT(). + PerformSync(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, cfg *config.Config) (*sync.Result, *sync.Error) { + close(syncStarted) + time.Sleep(50 * time.Millisecond) // Simulate work + return tt.syncResult, tt.syncError + }) + + cfg := &config.Config{RegistryName: "test"} + coord := &DefaultCoordinator{ + manager: mockSyncMgr, + statusPersistence: statusPersistence, + config: cfg, + cachedStatus: &status.SyncStatus{Phase: tt.initialPhase}, + } + + // Run performSync in goroutine + done := make(chan struct{}) + go func() { + coord.performSync(context.Background()) + close(done) + }() + + // Wait for sync to start and capture Syncing phase + <-syncStarted + loadedStatus, _ := statusPersistence.LoadStatus(context.Background()) + if loadedStatus != nil { + observedPhases = append(observedPhases, loadedStatus.Phase) + } + + // Wait for completion and capture final phase + <-done + finalStatus, err := statusPersistence.LoadStatus(context.Background()) + require.NoError(t, err) + observedPhases = append(observedPhases, finalStatus.Phase) + + // Verify phase transitions + assert.Contains(t, observedPhases, status.SyncPhaseSyncing, "Should transition through Syncing") + assert.Equal(t, tt.expectedPhase, finalStatus.Phase, "Should end in expected phase") + }) + } +} + +func TestUpdateStatusForSkippedSync(t *testing.T) { + tests := []struct { + name string + initialPhase status.SyncPhase + initialMsg string + reason string + expectedMsg string + }{ + { + name: "updates message when phase is Complete", + initialPhase: status.SyncPhaseComplete, + initialMsg: "Sync completed successfully", + reason: "up-to-date-no-policy", + expectedMsg: "Sync skipped: up-to-date-no-policy", + }, + { + name: "does update message when phase is Failed", + initialPhase: status.SyncPhaseFailed, + initialMsg: "Previous sync failed", + reason: "up-to-date-no-policy", + expectedMsg: "Sync skipped: up-to-date-no-policy", + }, + { + name: "does update message when phase is Syncing", + initialPhase: status.SyncPhaseSyncing, + initialMsg: "Sync in progress", + reason: "already-in-progress", + expectedMsg: "Sync skipped: already-in-progress", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir := t.TempDir() + statusFile := filepath.Join(tempDir, "status.json") + statusPersistence := status.NewFileStatusPersistence(statusFile) + + coord := &DefaultCoordinator{ + statusPersistence: statusPersistence, + cachedStatus: &status.SyncStatus{ + Phase: tt.initialPhase, + Message: tt.initialMsg, + }, + } + + // Save initial status + err := statusPersistence.SaveStatus(context.Background(), coord.cachedStatus) + require.NoError(t, err) + + // Call updateStatusForSkippedSync + coord.updateStatusForSkippedSync(context.Background(), tt.reason) + + // Verify the status + assert.Equal(t, tt.expectedMsg, coord.cachedStatus.Message, "Message should be updated correctly") + + // Verify it was persisted (or not) + loadedStatus, err := statusPersistence.LoadStatus(context.Background()) + require.NoError(t, err) + assert.Equal(t, tt.expectedMsg, loadedStatus.Message, "Persisted message should match") + assert.Equal(t, status.SyncPhaseComplete, loadedStatus.Phase, "Phase should not change") + }) + } +} + +func TestGetSyncInterval(t *testing.T) { + tests := []struct { + name string + policy *config.SyncPolicyConfig + expected time.Duration + }{ + { + name: "nil policy returns default", + policy: nil, + expected: time.Minute, + }, + { + name: "empty interval returns default", + policy: &config.SyncPolicyConfig{ + Interval: "", + }, + expected: time.Minute, + }, + { + name: "valid interval is parsed correctly", + policy: &config.SyncPolicyConfig{ + Interval: "5m", + }, + expected: 5 * time.Minute, + }, + { + name: "invalid interval returns default", + policy: &config.SyncPolicyConfig{ + Interval: "invalid", + }, + expected: time.Minute, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getSyncInterval(tt.policy) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/pkg/sync/coordinator/doc.go b/pkg/sync/coordinator/doc.go new file mode 100644 index 0000000..a5d6ff5 --- /dev/null +++ b/pkg/sync/coordinator/doc.go @@ -0,0 +1,96 @@ +// Package coordinator provides background synchronization coordination for registry resources. +// +// This package implements the orchestration layer that schedules and executes periodic +// sync operations. It sits on top of pkg/sync.Manager and handles: +// +// - Background sync scheduling using time.Ticker +// - Initial sync on startup +// - Status persistence and thread-safe access +// - Graceful shutdown +// +// # Architecture +// +// The coordinator separates concerns between: +// +// - pkg/sync: Domain logic (what/when to sync, how to detect changes) +// - pkg/sync/coordinator: Orchestration (scheduling, lifecycle, state management) +// - cmd/app/serve: HTTP server lifecycle (just starts/stops coordinator) +// +// # Core Interface +// +// The Coordinator interface provides a simple lifecycle API: +// +// type Coordinator interface { +// Start(ctx context.Context) error // Begin background sync loop +// Stop() error // Graceful shutdown +// GetStatus() *status.SyncStatus // Thread-safe status access +// } +// +// # Usage Example +// +// // Create dependencies +// syncManager := sync.NewDefaultSyncManager(...) +// statusPersistence := status.NewFileStatusPersistence("./data/status.json") +// +// // Create coordinator with injected dependencies +// coordinator := coordinator.New(syncManager, statusPersistence, config) +// +// // Start background sync +// ctx, cancel := context.WithCancel(context.Background()) +// defer cancel() +// +// go coordinator.Start(ctx) +// +// // ... run server ... +// +// // Stop on shutdown +// coordinator.Stop() +// +// # Thread Safety +// +// The coordinator maintains an in-memory cache of sync status that is accessed by +// multiple goroutines (ticker goroutine and sync execution). All access to the cached +// status is protected by an internal mutex, eliminating the need for external +// synchronization wrappers. +// +// # Status Persistence +// +// The coordinator accepts a StatusPersistence interface, making it easy to swap +// implementations (file, database, etc.) without changing coordinator code: +// +// // File-based (current): +// persistence := status.NewFileStatusPersistence("./data/status.json") +// +// // Future database-based: +// persistence := status.NewDatabaseStatusPersistence(db, registryID) +// +// // Same coordinator works with both: +// coordinator := coordinator.New(manager, persistence, config) +// +// # Sync Decision Flow +// +// 1. Ticker fires (based on configured interval) +// 2. Coordinator calls checkSync() +// 3. checkSync() calls Manager.ShouldSync() to decide +// 4. If needed, performSync() executes the sync +// 5. Status is updated and persisted at each phase transition +// +// # Error Handling +// +// The coordinator handles errors gracefully: +// +// - Failed syncs are logged and status updated to "Failed" +// - Coordinator continues running even after failures +// - Next sync attempt occurs on next ticker interval +// - Status persistence errors are logged but don't stop sync +// +// # Integration with pkg/sync +// +// The coordinator delegates all sync logic to pkg/sync.Manager: +// +// - ShouldSync(): Determines if sync is needed +// - PerformSync(): Executes the actual sync operation +// +// This keeps the coordinator focused on scheduling and state management, +// while sync business logic stays in pkg/sync. +package coordinator diff --git a/pkg/sync/coordinator/sync.go b/pkg/sync/coordinator/sync.go new file mode 100644 index 0000000..d40a97e --- /dev/null +++ b/pkg/sync/coordinator/sync.go @@ -0,0 +1,96 @@ +package coordinator + +import ( + "context" + "fmt" + "time" + + "github.com/stacklok/toolhive-registry-server/pkg/status" + "github.com/stacklok/toolhive/pkg/logger" +) + +// checkSync performs a sync check and updates status accordingly +func (c *DefaultCoordinator) checkSync(ctx context.Context, checkType string) { + // Check if sync is needed (read status under lock) + // ShouldSync will return false with ReasonAlreadyInProgress if Phase == SyncPhaseSyncing + var shouldSync bool + var reason string + c.withStatus(func(syncStatus *status.SyncStatus) { + shouldSync, reason, _ = c.manager.ShouldSync(ctx, c.config, syncStatus, false) + }) + logger.Infof("%s sync check: shouldSync=%v, reason=%s", checkType, shouldSync, reason) + + if shouldSync { + c.performSync(ctx) + } else { + c.updateStatusForSkippedSync(ctx, reason) + } +} + +// performSync executes a sync operation and updates status +func (c *DefaultCoordinator) performSync(ctx context.Context) { + // Ensure status is persisted at the end, whatever the result + defer func() { + c.withStatus(func(syncStatus *status.SyncStatus) { + if err := c.statusPersistence.SaveStatus(ctx, syncStatus); err != nil { + logger.Errorf("Failed to persist final sync status: %v", err) + } + }) + }() + + // Update status: Syncing (under lock) + var attemptCount int + c.withStatus(func(syncStatus *status.SyncStatus) { + syncStatus.Phase = status.SyncPhaseSyncing + syncStatus.Message = "Sync in progress" + now := time.Now() + syncStatus.LastAttempt = &now + syncStatus.AttemptCount++ + attemptCount = syncStatus.AttemptCount + + // Persist the "Syncing" state immediately so it's visible + if err := c.statusPersistence.SaveStatus(ctx, syncStatus); err != nil { + logger.Warnf("Failed to persist syncing status: %v", err) + } + }) + + logger.Infof("Starting sync operation (attempt %d)", attemptCount) + + // Perform sync (outside lock - this can take a long time) + result, err := c.manager.PerformSync(ctx, c.config) + + // Update status based on result (under lock) + now := time.Now() + c.withStatus(func(syncStatus *status.SyncStatus) { + if err != nil { + syncStatus.Phase = status.SyncPhaseFailed + syncStatus.Message = err.Message + logger.Errorf("Sync failed: %v", err) + } else { + syncStatus.Phase = status.SyncPhaseComplete + syncStatus.Message = "Sync completed successfully" + syncStatus.LastSyncTime = &now + syncStatus.LastSyncHash = result.Hash + syncStatus.ServerCount = result.ServerCount + syncStatus.AttemptCount = 0 + hashPreview := result.Hash + if len(hashPreview) > 8 { + hashPreview = hashPreview[:8] + } + logger.Infof("Sync completed successfully: %d servers, hash=%s", result.ServerCount, hashPreview) + } + }) +} + +// updateStatusForSkippedSync updates the status when a sync check determines sync is not needed +func (c *DefaultCoordinator) updateStatusForSkippedSync(ctx context.Context, reason string) { + // Only update if we have a previous successful sync + // Don't overwrite Failed/Syncing states with "skipped" messages + c.withStatus(func(syncStatus *status.SyncStatus) { + syncStatus.Phase = status.SyncPhaseComplete + syncStatus.Message = fmt.Sprintf("Sync skipped: %s", reason) + if err := c.statusPersistence.SaveStatus(ctx, syncStatus); err != nil { + logger.Warnf("Failed to persist skipped sync status: %v", err) + } + }) +} diff --git a/pkg/sync/detectors.go b/pkg/sync/detectors.go index f1493c8..cf1d6cc 100644 --- a/pkg/sync/detectors.go +++ b/pkg/sync/detectors.go @@ -48,8 +48,12 @@ type DefaultAutomaticSyncChecker struct{} // IsIntervalSyncNeeded checks if sync is needed based on time interval // Returns: (syncNeeded, nextSyncTime, error) -// nextSyncTime is always a future time when the next sync should occur +// nextSyncTime is a future time when the next sync should occur, or zero time if no policy configured func (*DefaultAutomaticSyncChecker) IsIntervalSyncNeeded(config *config.Config, syncStatus *status.SyncStatus) (bool, time.Time, error) { + if config.SyncPolicy == nil || config.SyncPolicy.Interval == "" { + return false, time.Time{}, nil + } + // Parse the sync interval interval, err := time.ParseDuration(config.SyncPolicy.Interval) if err != nil { diff --git a/pkg/sync/doc.go b/pkg/sync/doc.go index 77f572d..cb0508a 100644 --- a/pkg/sync/doc.go +++ b/pkg/sync/doc.go @@ -2,14 +2,20 @@ // for registry resources in the ToolHive Registry Server. // // This package implements a clean separation of concerns by extracting all -// sync-related logic from the controller into dedicated interfaces and types: +// sync-related logic into dedicated interfaces and types: // // # Core Interfaces // -// - Manager: Main interface for orchestrating sync operations +// - Manager: Main interface for orchestrating sync operations (domain logic) // - DataChangeDetector: Detects changes in source data using hash comparison // - AutomaticSyncChecker: Manages time-based automatic sync scheduling // +// # Coordinator Package +// +// The sync/coordinator subpackage provides the orchestration layer that schedules +// and executes background sync operations. It handles ticker-based periodic syncs, +// status persistence, and lifecycle management. See pkg/sync/coordinator for details. +// // # Result Types // // - Result: Contains the outcome of successful sync operations (hash, server count) @@ -18,14 +24,16 @@ // # Sync Decision Making // // The Manager.ShouldSync method evaluates multiple factors to determine if a sync -// is needed, returning a decision (bool), reason (string), and next sync time: +// is needed, returning a decision (bool) and reason (string): // // - Registry state (failed, not ready, complete) // - Filter configuration changes (via hash comparison) // - Source data changes (via hash comparison) -// - Manual sync requests (via annotations) -// - Automatic sync intervals (time-based policies) -// - Requeue timing (minimum time between sync attempts) +// - Sync interval elapsed (time-based automatic sync) +// - Manual sync requests +// +// The coordinator package (pkg/sync/coordinator) handles periodic sync scheduling +// using the configured sync interval from server configuration. // // # Sync Reasons // @@ -37,9 +45,8 @@ // - ReasonFilterChanged: Filter configuration modified // - ReasonManualWithChanges: Manual sync requested with detected changes // - ReasonManualNoChanges: Manual sync requested but no changes detected -// - ReasonUpToDateWithPolicy: No sync needed, automatic policy active +// - ReasonUpToDateWithPolicy: No sync needed, data is current // - ReasonUpToDateNoPolicy: No sync needed, no automatic policy -// - ReasonRequeueTimeNotElapsed: Too soon to retry after last attempt // - ReasonErrorCheckingChanges: Error during change detection // // # Kubernetes Status Integration @@ -56,23 +63,15 @@ // - ConditionReason: Reason code for the condition // - Message: Human-readable error message // -// # Timing Configuration -// -// Default timing constants control sync behavior: -// -// - DefaultSyncRequeueAfter: Minimum time between sync attempts (5 minutes) -// This variable can be modified in tests for faster iteration -// // # Key Features // // - Hash-based change detection to avoid unnecessary syncs // - Filter change detection via hash comparison -// - Manual sync support via Kubernetes annotations -// - Automatic sync scheduling with configurable intervals +// - Manual sync support +// - Configurable sync intervals via server configuration // - Comprehensive error handling with structured Error types // - Detailed sync reason tracking for observability -// - Clean integration with Kubernetes controller patterns -// - Resource cleanup on Registry deletion +// - Resource cleanup on deletion // // # Implementation // diff --git a/pkg/sync/manager.go b/pkg/sync/manager.go index 293b54b..1599695 100644 --- a/pkg/sync/manager.go +++ b/pkg/sync/manager.go @@ -9,7 +9,6 @@ import ( "time" "k8s.io/apimachinery/pkg/runtime" - ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -28,9 +27,8 @@ type Result struct { // Sync reason constants const ( // Registry state related reasons - ReasonAlreadyInProgress = "sync-already-in-progress" - ReasonRegistryNotReady = "registry-not-ready" - ReasonRequeueTimeNotElapsed = "requeue-time-not-elapsed" + ReasonAlreadyInProgress = "sync-already-in-progress" + ReasonRegistryNotReady = "registry-not-ready" // Filter change related reasons ReasonFilterChanged = "filter-changed" @@ -69,19 +67,6 @@ const ( conditionReasonStorageFailed = "StorageFailed" ) -// Default timing constants for the sync manager -const ( - // DefaultSyncRequeueAfterConstant is the constant default requeue interval for sync operations - DefaultSyncRequeueAfterConstant = time.Minute * 5 -) - -// Configurable timing variables for testing -var ( - // DefaultSyncRequeueAfter is the configurable default requeue interval for sync operations - // This can be modified in tests to speed up requeue behavior - DefaultSyncRequeueAfter = DefaultSyncRequeueAfterConstant -) - // Condition types for Config const ( // ConditionSourceAvailable indicates whether the source is available and accessible @@ -114,12 +99,14 @@ func (e *Error) Unwrap() error { } // Manager manages synchronization operations for Registry resources +// +//go:generate mockgen -destination=mocks/mock_manager.go -package=mocks github.com/stacklok/toolhive-registry-server/pkg/sync Manager type Manager interface { // ShouldSync determines if a sync operation is needed - ShouldSync(ctx context.Context, config *config.Config, syncStatus *status.SyncStatus) (bool, string, *time.Time) + ShouldSync(ctx context.Context, config *config.Config, syncStatus *status.SyncStatus, manualSyncRequested bool) (bool, string, *time.Time) // PerformSync executes the complete sync operation - PerformSync(ctx context.Context, config *config.Config) (ctrl.Result, *Result, *Error) + PerformSync(ctx context.Context, config *config.Config) (*Result, *Error) // Delete cleans up storage resources for the Registry Delete(ctx context.Context, config *config.Config) error @@ -163,8 +150,9 @@ func NewDefaultSyncManager(k8sClient client.Client, scheme *runtime.Scheme, } } -// ShouldSync determines if a sync operation is needed and when the next sync should occur -// nolint:gocyclo +// ShouldSync determines if a sync operation is needed +// Returns: (shouldSync bool, reason string, nextSyncTime *time.Time) +// nextSyncTime is always nil - timing is controlled by the configured sync interval func (s *DefaultSyncManager) ShouldSync( ctx context.Context, config *config.Config, @@ -178,39 +166,24 @@ func (s *DefaultSyncManager) ShouldSync( return false, ReasonAlreadyInProgress, nil } - // Check if requeue time has elapsed and pre-compute next sync time - requeueElapsed, nextSyncTime := s.calculateNextSyncTime(ctx, config, syncStatus) - // Check if sync is needed based on registry state syncNeededForState := s.isSyncNeededForState(syncStatus) // Check if filter has changed filterChanged := s.isFilterChanged(ctx, config, syncStatus) + // Check if interval has elapsed + checkIntervalElapsed, _, err := s.automaticSyncChecker.IsIntervalSyncNeeded(config, syncStatus) + if err != nil { + ctxLogger.Error(err, "Failed to determine if interval has elapsed") + return false, ReasonErrorCheckingSyncNeed, nil + } shouldSync := false reason := ReasonUpToDateNoPolicy - if syncNeededForState { - if !requeueElapsed { - ctxLogger.Info("Sync not needed because requeue time not elapsed", - "requeueTime", DefaultSyncRequeueAfter, "lastAttempt", syncStatus.LastAttempt) - reason = ReasonRequeueTimeNotElapsed - } else { - shouldSync = true - } - } - - if !shouldSync && manualSyncRequested { - // Manual sync requested - shouldSync = true - nextSyncTime = nil - } - - if !shouldSync && filterChanged { - // Filter changed - shouldSync = true - reason = ReasonFilterChanged - } else if shouldSync || requeueElapsed { - // Check if source data has changed by comparing hash + // Check if update is needed for state, manual sync, or filter change + dataChangedString := "N/A" + if syncNeededForState || manualSyncRequested || filterChanged || checkIntervalElapsed { + // Check if source data has changed dataChanged, err := s.dataChangeDetector.IsDataChanged(ctx, config, syncStatus) if err != nil { ctxLogger.Error(err, "Failed to determine if data has changed") @@ -224,29 +197,26 @@ func (s *DefaultSyncManager) ShouldSync( reason = ReasonRegistryNotReady } else if manualSyncRequested { reason = ReasonManualWithChanges + } else if filterChanged { + reason = ReasonFilterChanged } else { reason = ReasonSourceDataChanged } } else { shouldSync = false - if syncNeededForState { - reason = ReasonUpToDateWithPolicy - } else { + if manualSyncRequested { reason = ReasonManualNoChanges } } } + dataChangedString = fmt.Sprintf("%t", dataChanged) } ctxLogger.Info("ShouldSync", "syncNeededForState", syncNeededForState, "filterChanged", filterChanged, - "requeueElapsed", requeueElapsed, "manualSyncRequested", manualSyncRequested, "nextSyncTime", - nextSyncTime) - ctxLogger.Info("ShouldSync returning", "shouldSync", shouldSync, "reason", reason, "nextSyncTime", nextSyncTime) + "manualSyncRequested", manualSyncRequested, "checkIntervalElapsed", checkIntervalElapsed, "dataChanged", dataChangedString) + ctxLogger.Info("ShouldSync returning", "shouldSync", shouldSync, "reason", reason) - if shouldSync { - return shouldSync, reason, nil - } - return shouldSync, reason, nextSyncTime + return shouldSync, reason, nil } // isSyncNeededForState checks if sync is needed based on the registry's current state @@ -294,50 +264,20 @@ func (*DefaultSyncManager) isFilterChanged(ctx context.Context, config *config.C return currentHashStr != lastHash } -// calculateNextSyncTime checks if the requeue or sync policy time has elapsed and calculates the next requeue time -func (s *DefaultSyncManager) calculateNextSyncTime(ctx context.Context, config *config.Config, syncStatus *status.SyncStatus) (bool, *time.Time) { - ctxLogger := log.FromContext(ctx) - - // First consider the requeue time - requeueElapsed := false - var nextSyncTime time.Time - if syncStatus != nil { - if syncStatus.LastAttempt != nil { - nextSyncTime = syncStatus.LastAttempt.Add(DefaultSyncRequeueAfter) - } - } - - // If we have a sync policy, check if the next automatic sync time is sooner than the next requeue time - if config != nil && config.SyncPolicy != nil { - autoSyncNeeded, nextAutomaticSyncTime, err := s.automaticSyncChecker.IsIntervalSyncNeeded(config, syncStatus) - if err != nil { - ctxLogger.Error(err, "Failed to determine if interval sync is needed") - } - - // Resync at the earlier time between the next sync time and the next automatic sync time - if autoSyncNeeded && nextSyncTime.After(nextAutomaticSyncTime) { - nextSyncTime = nextAutomaticSyncTime - } - } - - requeueElapsed = time.Now().After(nextSyncTime) - return requeueElapsed, &nextSyncTime -} - // PerformSync performs the complete sync operation for the Registry -// The controller is responsible for setting sync status via the status collector +// Returns sync result on success, or error on failure func (s *DefaultSyncManager) PerformSync( ctx context.Context, config *config.Config, -) (ctrl.Result, *Result, *Error) { +) (*Result, *Error) { // Fetch and process registry data fetchResult, err := s.fetchAndProcessRegistryData(ctx, config) if err != nil { - return ctrl.Result{RequeueAfter: DefaultSyncRequeueAfter}, nil, err + return nil, err } // Store the processed registry data if err := s.storeRegistryData(ctx, config, fetchResult); err != nil { - return ctrl.Result{RequeueAfter: DefaultSyncRequeueAfter}, nil, err + return nil, err } // Return sync result with data for status collector @@ -346,7 +286,7 @@ func (s *DefaultSyncManager) PerformSync( ServerCount: fetchResult.ServerCount, } - return ctrl.Result{}, syncResult, nil + return syncResult, nil } // Delete cleans up storage resources for the Registry diff --git a/pkg/sync/manager_test.go b/pkg/sync/manager_test.go index 3e58b21..6cc6696 100644 --- a/pkg/sync/manager_test.go +++ b/pkg/sync/manager_test.go @@ -28,8 +28,7 @@ func TestNewDefaultSyncManager(t *testing.T) { Build() sourceHandlerFactory := sources.NewSourceHandlerFactory(fakeClient) - storageManager, err := sources.NewStorageManager() - require.Error(t, err) + storageManager := sources.NewFileStorageManager("/tmp/test-storage") syncManager := NewDefaultSyncManager(fakeClient, scheme, sourceHandlerFactory, storageManager) @@ -92,7 +91,7 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { expectedNextTime: false, }, { - name: "sync needed when no last sync hash", + name: "sync needed when registry is in failed state", manualSyncRequested: false, config: &config.Config{ Source: config.SourceConfig{ @@ -104,11 +103,11 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { }, }, syncStatus: &status.SyncStatus{ - Phase: status.SyncPhaseComplete, + Phase: status.SyncPhaseFailed, LastSyncHash: "", }, expectedSyncNeeded: true, - expectedReason: ReasonSourceDataChanged, + expectedReason: ReasonRegistryNotReady, expectedNextTime: false, }, { @@ -158,8 +157,7 @@ func TestDefaultSyncManager_ShouldSync(t *testing.T) { Build() sourceHandlerFactory := sources.NewSourceHandlerFactory(fakeClient) - storageManager, err := sources.NewStorageManager() - require.Error(t, err) + storageManager := sources.NewFileStorageManager("/tmp/test-storage") syncManager := NewDefaultSyncManager(fakeClient, scheme, sourceHandlerFactory, storageManager) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -404,7 +402,7 @@ func TestDefaultSyncManager_PerformSync(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - result, syncResult, syncErr := syncManager.PerformSync(ctx, tt.config) + syncResult, syncErr := syncManager.PerformSync(ctx, tt.config) if tt.expectedError { assert.NotNil(t, syncErr) @@ -415,9 +413,6 @@ func TestDefaultSyncManager_PerformSync(t *testing.T) { assert.Nil(t, syncErr) } - // Verify the result - assert.NotNil(t, result) - // Validate server count if expected if tt.expectedServerCount != nil && syncResult != nil { assert.Equal(t, *tt.expectedServerCount, syncResult.ServerCount, "ServerCount should match expected value after sync") diff --git a/pkg/sync/mocks/mock_manager.go b/pkg/sync/mocks/mock_manager.go new file mode 100644 index 0000000..5ed8add --- /dev/null +++ b/pkg/sync/mocks/mock_manager.go @@ -0,0 +1,90 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/stacklok/toolhive-registry-server/pkg/sync (interfaces: Manager) +// +// Generated by this command: +// +// mockgen -destination=mocks/mock_manager.go -package=mocks github.com/stacklok/toolhive-registry-server/pkg/sync Manager +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + reflect "reflect" + time "time" + + config "github.com/stacklok/toolhive-registry-server/pkg/config" + status "github.com/stacklok/toolhive-registry-server/pkg/status" + sync "github.com/stacklok/toolhive-registry-server/pkg/sync" + gomock "go.uber.org/mock/gomock" +) + +// MockManager is a mock of Manager interface. +type MockManager struct { + ctrl *gomock.Controller + recorder *MockManagerMockRecorder + isgomock struct{} +} + +// MockManagerMockRecorder is the mock recorder for MockManager. +type MockManagerMockRecorder struct { + mock *MockManager +} + +// NewMockManager creates a new mock instance. +func NewMockManager(ctrl *gomock.Controller) *MockManager { + mock := &MockManager{ctrl: ctrl} + mock.recorder = &MockManagerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockManager) EXPECT() *MockManagerMockRecorder { + return m.recorder +} + +// Delete mocks base method. +func (m *MockManager) Delete(ctx context.Context, arg1 *config.Config) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Delete", ctx, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// Delete indicates an expected call of Delete. +func (mr *MockManagerMockRecorder) Delete(ctx, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockManager)(nil).Delete), ctx, arg1) +} + +// PerformSync mocks base method. +func (m *MockManager) PerformSync(ctx context.Context, arg1 *config.Config) (*sync.Result, *sync.Error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PerformSync", ctx, arg1) + ret0, _ := ret[0].(*sync.Result) + ret1, _ := ret[1].(*sync.Error) + return ret0, ret1 +} + +// PerformSync indicates an expected call of PerformSync. +func (mr *MockManagerMockRecorder) PerformSync(ctx, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PerformSync", reflect.TypeOf((*MockManager)(nil).PerformSync), ctx, arg1) +} + +// ShouldSync mocks base method. +func (m *MockManager) ShouldSync(ctx context.Context, arg1 *config.Config, syncStatus *status.SyncStatus, manualSyncRequested bool) (bool, string, *time.Time) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ShouldSync", ctx, arg1, syncStatus, manualSyncRequested) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(*time.Time) + return ret0, ret1, ret2 +} + +// ShouldSync indicates an expected call of ShouldSync. +func (mr *MockManagerMockRecorder) ShouldSync(ctx, arg1, syncStatus, manualSyncRequested any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ShouldSync", reflect.TypeOf((*MockManager)(nil).ShouldSync), ctx, arg1, syncStatus, manualSyncRequested) +}