From 2a75dbdc355796c6bb83b18745686bb5c5d2b318 Mon Sep 17 00:00:00 2001 From: Brian Kane Date: Mon, 19 Jan 2026 11:22:28 +0000 Subject: [PATCH] Add a minimal, interface based provider registry to break complex import cycle (#7021) Signed-off-by: Brian Kane --- cmd/core/app/bootstrap.go | 51 ++++ cmd/core/app/server.go | 4 + pkg/registry/README.md | 115 +++++++++ pkg/registry/registry.go | 177 +++++++++++++ pkg/registry/registry_test.go | 452 ++++++++++++++++++++++++++++++++++ 5 files changed, 799 insertions(+) create mode 100644 cmd/core/app/bootstrap.go create mode 100644 pkg/registry/README.md create mode 100644 pkg/registry/registry.go create mode 100644 pkg/registry/registry_test.go diff --git a/cmd/core/app/bootstrap.go b/cmd/core/app/bootstrap.go new file mode 100644 index 000000000..816e25c1e --- /dev/null +++ b/cmd/core/app/bootstrap.go @@ -0,0 +1,51 @@ +/* +Copyright 2025 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package app + +import ( + "k8s.io/klog/v2" +) + +// bootstrapProviderRegistry registers framework-level providers that need +// to be available globally. This is called early in prepareRun before +// controllers are initialized. +// +// The provider registry is a fallback mechanism for breaking import cycles +// that block development. Providers registered here enable immediate feature +// work while longer-term refactoring efforts can be planned. +// +// Best practices when adding a provider: +// 1. Document which packages have the cycle (in comments below) +// 2. Define narrow, focused interfaces (< 5 methods) +// 3. Consider opportunities for future refactoring to eliminate the cycle +// 4. Prefer constructor injection for new code without cycles +// +// See pkg/registry/README.md for feature overview and pkg/registry package docs for guidelines. +func bootstrapProviderRegistry() { + klog.V(2).InfoS("Bootstrapping provider registry") + + // ──────────────────────────────────────────────────────────────────── + // Add providers below following this pattern: + // ──────────────────────────────────────────────────────────────────── + // + // ProviderInterface - Brief description + // Cycle: pkg/foo ↔ pkg/bar (explain the circular dependency) + // Note: Consider refactoring to extract shared interfaces + // registry.RegisterAs[ProviderInterface](implementation) + + klog.V(2).InfoS("Provider registry bootstrap complete") +} diff --git a/cmd/core/app/server.go b/cmd/core/app/server.go index 8573a62fb..33e29d43c 100644 --- a/cmd/core/app/server.go +++ b/cmd/core/app/server.go @@ -451,6 +451,10 @@ func prepareRunInShardingMode(ctx context.Context, manager manager.Manager, core // - Runs pre-start validation hooks to ensure system readiness // This function is used in single-instance mode or by the master shard in sharding mode. func prepareRun(ctx context.Context, manager manager.Manager, coreOptions *options.CoreOptions) error { + // Bootstrap provider registry early before other initialization + klog.V(2).InfoS("Initializing provider registry") + bootstrapProviderRegistry() + if coreOptions.Webhook.UseWebhook { klog.InfoS("Webhook enabled, registering OAM webhooks", "port", coreOptions.Webhook.WebhookPort, diff --git a/pkg/registry/README.md b/pkg/registry/README.md new file mode 100644 index 000000000..a9f596800 --- /dev/null +++ b/pkg/registry/README.md @@ -0,0 +1,115 @@ +# Provider Registry + +A minimal, interface-based provider registry for breaking import cycles in the codebase. + +## Purpose + +This registry is a **fallback mechanism** for situations where import cycles block development work. It provides runtime indirection through interface-based contracts while maintaining type safety. + +## Why It Exists + +Large, mature codebases sometimes develop import cycles between packages: +- `pkg/appfile` needs types from `pkg/controller` +- `pkg/controller` needs functionality from `pkg/appfile` +- Result: Import cycle prevents compilation + +While the ideal solution is restructuring packages with clear boundaries, this isn't always practical in the short term. The registry unblocks development while allowing refactoring efforts to be planned appropriately. + +## Design Philosophy + +**Use as a fallback, not a default:** +- New code should use well-designed package boundaries and constructor injection +- Existing code can use the registry when cycles genuinely block work +- Services registered here are candidates for future refactoring + +**Keep it simple:** +- Interface-only registration (enforced) +- Thread-safe operations +- No complex lifecycle management +- Stdlib dependencies only + +## How It Works + +### 1. Define an Interface + +```go +// In cmd/core/app/bootstrap.go or appropriate location +type MyProvider interface { + DoSomething(ctx context.Context) error +} +``` + +### 2. Register During Bootstrap + +```go +// In cmd/core/app/bootstrap.go +func bootstrapProviderRegistry() { + // MyProvider - Brief description of what it does + // Cycle: pkg/foo ↔ pkg/bar + // Note: Consider refactoring to extract shared interfaces + provider := foo.NewMyProvider() + registry.RegisterAs[MyProvider](provider) +} +``` + +### 3. Retrieve Where Needed + +```go +// In any package that needs it +provider, ok := registry.Get[MyProvider]() +if !ok { + return fmt.Errorf("MyProvider not registered") +} +err := provider.DoSomething(ctx) +``` + +## API + +- **`RegisterAs[T any](impl T)`** - Register an implementation for interface T +- **`Get[T any]() (T, bool)`** - Retrieve registered implementation +- **`Snapshot() RegistrySnapshot`** - Save current state (for testing) +- **`Restore(snapshot RegistrySnapshot)`** - Restore saved state (for testing) + +## Testing + +Use Snapshot/Restore to isolate tests: + +```go +func TestSomething(t *testing.T) { + snapshot := registry.Snapshot() + defer registry.Restore(snapshot) + + // Override with mock + registry.RegisterAs[MyProvider](mockImpl) + + // Test code using mock +} +// Original providers restored automatically +``` + +## When NOT to Use + +Prefer constructor injection when: +- Writing new code with clean package boundaries +- No import cycle exists between packages +- Extracting interfaces to a neutral package is straightforward +- Dependencies can be passed explicitly through constructors + +## Trade-offs + +**Benefits:** +- Unblocks development immediately +- Breaks cycles without major refactoring +- Type-safe through generics +- Easy to mock in tests + +**Costs:** +- Less visible than constructor injection +- Runtime lookups instead of compile-time +- Can hide architectural issues if overused + +## Guidance + +Use this registry judiciously as a pragmatic tool. Each provider registered represents an opportunity to improve package structure in the future. The goal is to keep usage minimal while unblocking important development work. + +For detailed implementation guidelines, see the package documentation in `registry.go`. diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go new file mode 100644 index 000000000..8effd143c --- /dev/null +++ b/pkg/registry/registry.go @@ -0,0 +1,177 @@ +/* +Copyright 2025 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package registry provides a minimal interface-based provider registry for +// breaking import cycles in the codebase. +// +// This is a fallback mechanism for situations where import cycles block development. +// +// See README.md in this directory for detailed rationale, usage guidelines, and examples. +package registry + +import ( + "fmt" + "reflect" + "sync" +) + +var globalRegistry = ®istry{ + providers: make(map[reflect.Type]interface{}), +} + +type registry struct { + mu sync.RWMutex + providers map[reflect.Type]interface{} +} + +// RegisterAs registers an implementation for an interface type T. +// +// The type parameter T must be an interface type, and impl must be a non-nil +// implementation of that interface. If T is not an interface or impl is nil, +// this function panics with a descriptive error message. +// +// If a provider is registered multiple times, the last registration wins. +// This is intentional to allow test code to override providers. +// +// Example: +// +// type MyProvider interface { +// DoSomething() error +// } +// +// type myImpl struct{} +// func (m *myImpl) DoSomething() error { return nil } +// +// registry.RegisterAs[MyProvider](&myImpl{}) +func RegisterAs[T any](impl T) { + interfaceType := reflect.TypeOf((*T)(nil)).Elem() + + // Validate T is an interface + if interfaceType.Kind() != reflect.Interface { + panic(fmt.Sprintf("registry.RegisterAs: type parameter T must be an interface type, got %s", interfaceType)) + } + + // Validate impl is not nil + implValue := reflect.ValueOf(impl) + if !implValue.IsValid() { + panic(fmt.Sprintf("registry.RegisterAs: cannot register nil implementation for interface %s", interfaceType)) + } + + // Check for nil only on types that support IsNil() to avoid panics + // IsNil() only works on: chan, func, interface, map, pointer, slice + //nolint:exhaustive // Default case intentionally handles all other reflect.Kind values + switch implValue.Kind() { + case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice: + if implValue.IsNil() { + panic(fmt.Sprintf("registry.RegisterAs: cannot register nil implementation for interface %s", interfaceType)) + } + default: + // Other types (Invalid, Bool, Int*, Uint*, Float*, Complex*, Array, String, Struct, UnsafePointer) + // either cannot be nil or are already caught by the IsValid() check above + } + + globalRegistry.mu.Lock() + defer globalRegistry.mu.Unlock() + globalRegistry.providers[interfaceType] = impl +} + +// Get retrieves the registered implementation for interface type T. +// +// Returns (implementation, true) if a provider is registered for type T, +// or (zero value, false) if no provider is registered. +// +// This function is thread-safe and optimized for concurrent access. +// +// Example: +// +// if provider, ok := registry.Get[MyProvider](); ok { +// provider.DoSomething() +// } else { +// // handle missing provider +// } +func Get[T any]() (T, bool) { + interfaceType := reflect.TypeOf((*T)(nil)).Elem() + + globalRegistry.mu.RLock() + defer globalRegistry.mu.RUnlock() + + if impl, ok := globalRegistry.providers[interfaceType]; ok { + return impl.(T), true + } + + var zero T + return zero, false +} + +// RegistrySnapshot represents a saved state of the registry. +// Use with Restore() to save and restore registry state in tests. +type RegistrySnapshot struct { + providers map[reflect.Type]interface{} +} + +// Snapshot creates a copy of the current registry state. +// +// This is useful in tests to save the registry state (including bootstrap providers), +// make temporary changes, and then restore the original state. +// +// Example in tests: +// +// func TestSomething(t *testing.T) { +// snapshot := registry.Snapshot() +// defer registry.Restore(snapshot) +// +// registry.RegisterAs[MyProvider](mockImpl) +// // ... test code that uses mockImpl +// +// } // Restore() brings back all original providers including bootstrap ones +func Snapshot() RegistrySnapshot { + globalRegistry.mu.RLock() + defer globalRegistry.mu.RUnlock() + + // Create a deep copy of the providers map + providersCopy := make(map[reflect.Type]interface{}, len(globalRegistry.providers)) + for k, v := range globalRegistry.providers { + providersCopy[k] = v + } + + return RegistrySnapshot{providers: providersCopy} +} + +// Restore replaces the current registry state with a saved snapshot. +// +// This function is intended for testing only. It restores the registry +// to the exact state it was in when Snapshot() was called. +// +// Example in tests: +// +// func TestSomething(t *testing.T) { +// snapshot := registry.Snapshot() +// defer registry.Restore(snapshot) // Restore original state +// +// registry.RegisterAs[MyProvider](mockImpl) +// // ... test code +// } +func Restore(snapshot RegistrySnapshot) { + globalRegistry.mu.Lock() + defer globalRegistry.mu.Unlock() + + // Create a copy to preserve snapshot immutability + providersCopy := make(map[reflect.Type]interface{}, len(snapshot.providers)) + for k, v := range snapshot.providers { + providersCopy[k] = v + } + globalRegistry.providers = providersCopy +} diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go new file mode 100644 index 000000000..7275e3024 --- /dev/null +++ b/pkg/registry/registry_test.go @@ -0,0 +1,452 @@ +/* +Copyright 2025 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package registry + +import ( + "strings" + "sync" + "testing" +) + +// Test interfaces +type SimpleService interface { + GetValue() string +} + +type ComplexService interface { + Process(input string) (string, error) + Validate(data interface{}) bool +} + +type AnotherService interface { + Execute() int +} + +// Test implementations +type simpleImpl struct { + value string +} + +func (s *simpleImpl) GetValue() string { + return s.value +} + +type complexImpl struct { + prefix string +} + +func (c *complexImpl) Process(input string) (string, error) { + return c.prefix + input, nil +} + +func (c *complexImpl) Validate(data interface{}) bool { + return data != nil +} + +type anotherImpl struct { + result int +} + +func (a *anotherImpl) Execute() int { + return a.result +} + +// TestRegisterAndGet_BasicInterface tests basic registration and retrieval +func TestRegisterAndGet_BasicInterface(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + impl := &simpleImpl{value: "test-value"} + RegisterAs[SimpleService](impl) + + retrieved, ok := Get[SimpleService]() + if !ok { + t.Fatal("Expected to find registered service, but got false") + } + + if retrieved.GetValue() != "test-value" { + t.Errorf("Expected value 'test-value', got '%s'", retrieved.GetValue()) + } +} + +// TestRegisterAndGet_Override tests that last write wins +func TestRegisterAndGet_Override(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + // Register first implementation + impl1 := &simpleImpl{value: "first"} + RegisterAs[SimpleService](impl1) + + // Register second implementation (override) + impl2 := &simpleImpl{value: "second"} + RegisterAs[SimpleService](impl2) + + // Should get the second implementation + retrieved, ok := Get[SimpleService]() + if !ok { + t.Fatal("Expected to find registered service, but got false") + } + + if retrieved.GetValue() != "second" { + t.Errorf("Expected value 'second' (last registered), got '%s'", retrieved.GetValue()) + } +} + +// TestGet_NotFound tests retrieval of unregistered service +func TestGet_NotFound(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + // Don't register anything, try to get + retrieved, ok := Get[AnotherService]() + if ok { + t.Fatal("Expected to not find service, but got true") + } + + // Verify zero value is returned + if retrieved != nil { + t.Errorf("Expected nil (zero value) for interface, got %v", retrieved) + } +} + +// TestRegisterAs_PanicsOnNonInterface tests that concrete types are rejected +func TestRegisterAs_PanicsOnNonInterface(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + defer func() { + r := recover() + if r == nil { + t.Fatal("Expected panic when registering concrete type, but didn't panic") + } + + panicMsg := r.(string) + if !strings.Contains(panicMsg, "must be an interface type") { + t.Errorf("Expected panic message about interface type, got: %s", panicMsg) + } + }() + + // Try to register a concrete struct type (should panic) + type ConcreteStruct struct { + Value string + } + concrete := ConcreteStruct{Value: "test"} + RegisterAs[ConcreteStruct](concrete) +} + +// TestRegisterAs_PanicsOnNil tests that nil implementations are rejected +func TestRegisterAs_PanicsOnNil(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + defer func() { + r := recover() + if r == nil { + t.Fatal("Expected panic when registering nil, but didn't panic") + } + + panicMsg := r.(string) + if !strings.Contains(panicMsg, "cannot register nil") { + t.Errorf("Expected panic message about nil, got: %s", panicMsg) + } + }() + + // Try to register nil (should panic) + var nilImpl SimpleService + RegisterAs[SimpleService](nilImpl) +} + +// TestConcurrentGet tests that concurrent Get operations are thread-safe +func TestConcurrentGet(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + // Register multiple services + RegisterAs[SimpleService](&simpleImpl{value: "concurrent-test"}) + RegisterAs[ComplexService](&complexImpl{prefix: "prefix-"}) + RegisterAs[AnotherService](&anotherImpl{result: 42}) + + const numGoroutines = 100 + const numIterations = 100 + + var wg sync.WaitGroup + errors := make(chan error, numGoroutines*3) + + // Launch goroutines that read concurrently + for i := 0; i < numGoroutines; i++ { + wg.Add(3) + + // Test SimpleService Get + go func() { + defer wg.Done() + for j := 0; j < numIterations; j++ { + svc, ok := Get[SimpleService]() + if !ok { + errors <- nil // Signal error + return + } + if svc.GetValue() != "concurrent-test" { + errors <- nil + return + } + } + }() + + // Test ComplexService Get + go func() { + defer wg.Done() + for j := 0; j < numIterations; j++ { + svc, ok := Get[ComplexService]() + if !ok { + errors <- nil + return + } + result, _ := svc.Process("test") + if result != "prefix-test" { + errors <- nil + return + } + } + }() + + // Test AnotherService Get + go func() { + defer wg.Done() + for j := 0; j < numIterations; j++ { + svc, ok := Get[AnotherService]() + if !ok { + errors <- nil + return + } + if svc.Execute() != 42 { + errors <- nil + return + } + } + }() + } + + wg.Wait() + close(errors) + + // Check for errors + errorCount := 0 + for range errors { + errorCount++ + } + if errorCount > 0 { + t.Errorf("Concurrent Get operations had %d errors", errorCount) + } +} + +// TestConcurrentRegisterAndGet tests concurrent RegisterAs and Get operations +func TestConcurrentRegisterAndGet(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + const numGoroutines = 50 + + var wg sync.WaitGroup + + // Goroutines that register and override + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + // Each goroutine registers its own value + impl := &anotherImpl{result: id} + RegisterAs[AnotherService](impl) + }(i) + } + + // Goroutines that read + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + // Just verify Get doesn't panic or deadlock + _, _ = Get[AnotherService]() + }() + } + + wg.Wait() + + // Verify we can still get a valid service after concurrent access + svc, ok := Get[AnotherService]() + if !ok { + t.Fatal("Expected to find service after concurrent operations") + } + + // Result should be one of the registered values (0 to numGoroutines-1) + result := svc.Execute() + if result < 0 || result >= numGoroutines { + t.Errorf("Expected result in range [0, %d), got %d", numGoroutines, result) + } +} + +// TestSnapshotAndRestore tests the snapshot/restore functionality +func TestSnapshotAndRestore(t *testing.T) { + // Save initial state to restore after test + initialState := Snapshot() + defer Restore(initialState) + + // Register initial services + RegisterAs[SimpleService](&simpleImpl{value: "original"}) + RegisterAs[ComplexService](&complexImpl{prefix: "original-"}) + + // Take a snapshot + snapshot := Snapshot() + + // Modify the registry + RegisterAs[SimpleService](&simpleImpl{value: "modified"}) + RegisterAs[AnotherService](&anotherImpl{result: 42}) + + // Verify modifications took effect + svc, ok := Get[SimpleService]() + if !ok || svc.GetValue() != "modified" { + t.Fatal("Expected modified value") + } + if _, ok := Get[AnotherService](); !ok { + t.Fatal("Expected AnotherService to be registered") + } + + // Restore the snapshot + Restore(snapshot) + + // Verify original state is restored + svc, ok = Get[SimpleService]() + if !ok || svc.GetValue() != "original" { + t.Errorf("Expected 'original' after restore, got '%s'", svc.GetValue()) + } + + complex, ok := Get[ComplexService]() + if !ok { + t.Fatal("ComplexService should be restored") + } + result, _ := complex.Process("test") + if result != "original-test" { + t.Errorf("Expected 'original-test', got '%s'", result) + } + + // Verify AnotherService is gone (wasn't in snapshot) + if _, ok := Get[AnotherService](); ok { + t.Error("AnotherService should not exist after restore") + } + + // Test cleanup happens via defer Restore(initialState) +} + +// Value receiver types for testing +type valueReceiverStruct struct { + data string +} + +type valueReceiverInterface interface { + GetData() string +} + +func (v valueReceiverStruct) GetData() string { + return v.data +} + +// TestRegisterAs_StructValueReceiver tests registration with struct value receivers +func TestRegisterAs_StructValueReceiver(t *testing.T) { + snapshot := Snapshot() + defer Restore(snapshot) + + // Register a struct value (not a pointer) + val := valueReceiverStruct{data: "test"} + RegisterAs[valueReceiverInterface](val) + + retrieved, ok := Get[valueReceiverInterface]() + if !ok { + t.Fatal("Expected to retrieve registered struct value") + } + if retrieved.GetData() != "test" { + t.Errorf("Expected 'test', got '%s'", retrieved.GetData()) + } +} + +// TestSnapshotImmutability verifies that snapshots remain immutable after Restore +func TestSnapshotImmutability(t *testing.T) { + // Save initial state to restore after test + initialState := Snapshot() + defer Restore(initialState) + + // Create a snapshot with a specific state + RegisterAs[SimpleService](&simpleImpl{value: "snapshot-value"}) + snapshot := Snapshot() + + // First restore + Restore(snapshot) + + // Modify the registry after restore + RegisterAs[SimpleService](&simpleImpl{value: "modified-value"}) + RegisterAs[AnotherService](&anotherImpl{result: 999}) + + // Second restore - should restore to original snapshot state + // This would fail if Restore() didn't copy the map + Restore(snapshot) + + // Verify the snapshot's state is preserved + svc, ok := Get[SimpleService]() + if !ok { + t.Fatal("SimpleService should exist after second restore") + } + if svc.GetValue() != "snapshot-value" { + t.Errorf("Expected 'snapshot-value' after second restore, got '%s'. Snapshot was corrupted!", svc.GetValue()) + } + + // Verify AnotherService is not present (wasn't in snapshot) + if _, ok := Get[AnotherService](); ok { + t.Error("AnotherService should not exist after restore - snapshot was corrupted!") + } +} + +// TestSnapshotPreservesBootstrapServices demonstrates the recommended pattern +func TestSnapshotPreservesBootstrapServices(t *testing.T) { + // Save initial state to restore after test + initialState := Snapshot() + defer Restore(initialState) + + // Simulate bootstrap registering a service + RegisterAs[SimpleService](&simpleImpl{value: "bootstrap-service"}) + + // Test code that uses Snapshot/Restore + snapshot := Snapshot() + defer Restore(snapshot) + + // Override with mock for testing + RegisterAs[SimpleService](&simpleImpl{value: "mock-service"}) + + // Test code uses mock + svc, ok := Get[SimpleService]() + if !ok || svc.GetValue() != "mock-service" { + t.Fatal("Expected mock service during test") + } + + // Manually restore to verify bootstrap service comes back + Restore(snapshot) + + svc, ok = Get[SimpleService]() + if !ok || svc.GetValue() != "bootstrap-service" { + t.Error("Bootstrap service should be restored after Restore()") + } + + // Test cleanup happens via outer defer Restore(initialState) +}