Add a minimal, interface based provider registry to break complex import cycle (#7021)
Some checks failed
Webhook Upgrade Validation / webhook-upgrade-check (push) Failing after 1m45s

Signed-off-by: Brian Kane <briankane1@gmail.com>
This commit is contained in:
Brian Kane
2026-01-19 11:22:28 +00:00
committed by GitHub
parent 568b1c578b
commit 2a75dbdc35
5 changed files with 799 additions and 0 deletions

51
cmd/core/app/bootstrap.go Normal file
View File

@@ -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")
}

View File

@@ -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,

115
pkg/registry/README.md Normal file
View File

@@ -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`.

177
pkg/registry/registry.go Normal file
View File

@@ -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 = &registry{
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
}

View File

@@ -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)
}