review comments

Author: mjudeikis

backend/provider/kcp/controllers/apibindingtemplate/controller.go

@@ -23,10 +23,8 @@ package apibindingtemplate
 import (
 	"context"
 	"fmt"
-	"net/url"
 	"strings"
 
-	apisv1alpha1 "github.com/kcp-dev/sdk/apis/apis/v1alpha1"
 	apisv1alpha2 "github.com/kcp-dev/sdk/apis/apis/v1alpha2"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -43,6 +41,7 @@ import (
 	mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager"
 	mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile"
 
+	"github.com/kube-bind/kube-bind/backend/provider/kcp/controllers/shared"
 	kubebindv1alpha2 "github.com/kube-bind/kube-bind/sdk/apis/kubebind/v1alpha2"
 )
 
@@ -54,11 +53,8 @@ type APIBindingTemplateReconciler struct {
 	manager        mcmanager.Manager
 	opts           controller.TypedOptions[mcreconcile.Request]
 	ignorePrefixes []string
-
-	// baseConfig is the kcp admin/root REST config used to construct
-	// VW clients for the apiresourceschema virtual workspace.
-	baseConfig *rest.Config
-	scheme     *runtime.Scheme
+	scheme         *runtime.Scheme
+	vwCache        *shared.VWClientCache
 }
 
 // New returns a new APIBindingTemplateReconciler.
@@ -74,8 +70,8 @@ func New(
 		manager:        mgr,
 		opts:           opts,
 		ignorePrefixes: ignorePrefixes,
-		baseConfig:     baseConfig,
 		scheme:         scheme,
+		vwCache:        shared.NewVWClientCache(baseConfig, scheme),
 	}
 
 	return r, nil
@@ -92,39 +88,6 @@ func (r *APIBindingTemplateReconciler) shouldIgnore(name string) bool {
 	return false
 }
 
-// extractClusterID extracts the cluster ID from an apiexport virtual workspace
-// URL. The URL format is:
-// https://host:port/services/apiexport/root:org:ws/<apiexport-name>/clusters/{cluster-id}
-func extractClusterID(clusterConfig *rest.Config) (string, error) {
-	u, err := url.Parse(clusterConfig.Host)
-	if err != nil {
-		return "", fmt.Errorf("failed to parse cluster host URL: %w", err)
-	}
-
-	pathParts := strings.Split(strings.Trim(u.Path, "/"), "/")
-	if len(pathParts) < 6 || pathParts[4] != "clusters" {
-		return "", fmt.Errorf("unexpected apiexport URL format: %s", u.Path)
-	}
-
-	return pathParts[5], nil
-}
-
-// newVWClient creates a client pointing at the apiresourceschema virtual workspace
-// for the given cluster ID:
-// https://host:port/services/apiresourceschema/{clusterID}/clusters/*/
-func (r *APIBindingTemplateReconciler) newVWClient(clusterID string) (client.Client, error) {
-	cfg := rest.CopyConfig(r.baseConfig)
-	u, err := url.Parse(cfg.Host)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse base config host: %w", err)
-	}
-
-	u.Path = fmt.Sprintf("/services/apiresourceschema/%s/clusters/*", clusterID)
-	cfg.Host = u.String()
-
-	return client.New(cfg, client.Options{Scheme: r.scheme})
-}
-
 // Reconcile implements reconcile.Reconciler for multicluster-runtime.
 func (r *APIBindingTemplateReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (ctrl.Result, error) {
 	logger := log.FromContext(ctx)
@@ -152,8 +115,8 @@ func (r *APIBindingTemplateReconciler) Reconcile(ctx context.Context, req mcreco
 		return ctrl.Result{}, fmt.Errorf("failed to get APIBinding %q: %w", req.Name, err)
 	}
 
-	// Build the schema getter with VW fallback.
-	getSchema := r.schemaGetterWithFallback(c, clusterConfig)
+	// Build the schema getter with VW fallback using the shared cache.
+	getSchema := shared.SchemaGetterWithFallback(c, clusterConfig, r.vwCache)
 
 	rec := reconciler{
 		client:               c,
@@ -169,57 +132,19 @@ func (r *APIBindingTemplateReconciler) Reconcile(ctx context.Context, req mcreco
 	return ctrl.Result{}, nil
 }
 
-// schemaGetterWithFallback returns a function that first tries to get the
-// APIResourceSchema from the current workspace, and if not found, falls back
-// to the apiresourceschema virtual workspace.
-func (r *APIBindingTemplateReconciler) schemaGetterWithFallback(
-	workspaceClient client.Client,
-	clusterConfig *rest.Config,
-) func(ctx context.Context, name string) (*apisv1alpha1.APIResourceSchema, error) {
-	return func(ctx context.Context, name string) (*apisv1alpha1.APIResourceSchema, error) {
-		logger := log.FromContext(ctx)
-
-		// 1. Try the current workspace first.
-		var schema apisv1alpha1.APIResourceSchema
-		err := workspaceClient.Get(ctx, client.ObjectKey{Name: name}, &schema)
-		if err == nil {
-			return &schema, nil
-		}
-		if !errors.IsNotFound(err) {
-			return nil, err
-		}
-
-		// 2. Fallback: try the apiresourceschema virtual workspace.
-		logger.V(2).Info("APIResourceSchema not found in workspace, trying VW fallback", "schema", name)
-
-		clusterID, err := extractClusterID(clusterConfig)
-		if err != nil {
-			return nil, fmt.Errorf("cannot build VW fallback client: %w", err)
-		}
-
-		vwClient, err := r.newVWClient(clusterID)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create VW client for cluster %q: %w", clusterID, err)
-		}
-
-		var vwSchema apisv1alpha1.APIResourceSchema
-		if err := vwClient.Get(ctx, client.ObjectKey{Name: name}, &vwSchema); err != nil {
-			return nil, fmt.Errorf("APIResourceSchema %q not found in workspace or VW: %w", name, err)
-		}
-
-		return &vwSchema, nil
-	}
-}
-
 // getTemplateMapper returns a mapper that enqueues the owning APIBinding when
 // an APIServiceExportTemplate changes.
+//
+// This function has the signature func(clusterName string, cl cluster.Cluster) handler.TypedEventHandler
+// because multicluster-runtime's mcbuilder.Watches accepts a "per-cluster event handler factory"
+// rather than a plain handler — it calls this factory for each cluster that is engaged.
 func getTemplateMapper(clusterName string, cl cluster.Cluster) handler.TypedEventHandler[client.Object, mcreconcile.Request] {
 	return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []mcreconcile.Request {
 		annotations := obj.GetAnnotations()
 		if annotations == nil {
 			return nil
 		}
-		ownerName, ok := annotations[annotationOwnerBinding]
+		ownerName, ok := annotations[shared.AnnotationOwnerBinding]
 		if !ok {
 			return nil
 		}

backend/provider/kcp/controllers/apibindingtemplate/reconcile.go

@@ -32,6 +32,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	"github.com/kube-bind/kube-bind/backend/provider/kcp/controllers/shared"
 	kubebindv1alpha2 "github.com/kube-bind/kube-bind/sdk/apis/kubebind/v1alpha2"
 )
 
@@ -46,10 +47,6 @@ func templateNameForBinding(bindingName string, scope kubebindv1alpha2.InformerS
 	}
 }
 
-// annotationOwnerBinding is the annotation key used to link a template back to
-// the APIBinding that owns it.
-const annotationOwnerBinding = "apibindingtemplate.kube-bind.io/owner-binding"
-
 type reconciler struct {
 	client               client.Client
 	scheme               *runtime.Scheme
@@ -73,7 +70,10 @@ func (r *reconciler) reconcile(ctx context.Context, binding *apisv1alpha2.APIBin
 		return err
 	}
 
+	desiredNames := make(map[string]bool, len(templates))
 	for _, desired := range templates {
+		desiredNames[desired.Name] = true
+
 		// Set owner reference: APIBinding owns the template.
 		if err := controllerutil.SetOwnerReference(binding, desired, r.scheme); err != nil {
 			return fmt.Errorf("failed to set owner reference on APIServiceExportTemplate %q: %w", desired.Name, err)
@@ -84,6 +84,40 @@ func (r *reconciler) reconcile(ctx context.Context, binding *apisv1alpha2.APIBin
 		}
 	}
 
+	// Prune orphaned templates: if a binding used to have resources of a
+	// given scope but no longer does, the old template must be deleted.
+	if err := r.pruneOrphanedTemplates(ctx, binding, desiredNames); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// pruneOrphanedTemplates lists templates owned by this binding and deletes any
+// that are no longer in the desired set.
+func (r *reconciler) pruneOrphanedTemplates(ctx context.Context, binding *apisv1alpha2.APIBinding, desiredNames map[string]bool) error {
+	logger := klog.FromContext(ctx)
+
+	var allTemplates kubebindv1alpha2.APIServiceExportTemplateList
+	if err := r.client.List(ctx, &allTemplates); err != nil {
+		return fmt.Errorf("failed to list APIServiceExportTemplates: %w", err)
+	}
+
+	for i := range allTemplates.Items {
+		tmpl := &allTemplates.Items[i]
+		ann := tmpl.Annotations
+		if ann == nil || ann[shared.AnnotationOwnerBinding] != binding.Name {
+			continue
+		}
+		if desiredNames[tmpl.Name] {
+			continue
+		}
+		logger.Info("Deleting orphaned APIServiceExportTemplate", "name", tmpl.Name, "binding", binding.Name)
+		if err := r.client.Delete(ctx, tmpl); err != nil && !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to delete orphaned APIServiceExportTemplate %q: %w", tmpl.Name, err)
+		}
+	}
+
 	return nil
 }
 
@@ -184,7 +218,7 @@ func (r *reconciler) buildTemplates(ctx context.Context, binding *apisv1alpha2.A
 			ObjectMeta: metav1.ObjectMeta{
 				Name: templateName,
 				Annotations: map[string]string{
-					annotationOwnerBinding: binding.Name,
+					shared.AnnotationOwnerBinding: binding.Name,
 				},
 			},
 			Spec: kubebindv1alpha2.APIServiceExportTemplateSpec{

backend/provider/kcp/controllers/apiresourceschema/controller.go

@@ -22,40 +22,33 @@ package apiresourceschema
 import (
 	"context"
 	"fmt"
-	"net/url"
-	"strings"
 
-	apisv1alpha1 "github.com/kcp-dev/sdk/apis/apis/v1alpha1"
 	apisv1alpha2 "github.com/kcp-dev/sdk/apis/apis/v1alpha2"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	mcbuilder "sigs.k8s.io/multicluster-runtime/pkg/builder"
 	mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager"
 	mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile"
 
+	"github.com/kube-bind/kube-bind/backend/provider/kcp/controllers/shared"
 	kubebindv1alpha2 "github.com/kube-bind/kube-bind/sdk/apis/kubebind/v1alpha2"
 )
 
 const controllerName = "kube-bind-kcp-apiresourceschema"
 
-// annotationOwnerBinding is the annotation key linking a template to its APIBinding.
-// Shared with the apibindingtemplate controller.
-const annotationOwnerBinding = "apibindingtemplate.kube-bind.io/owner-binding"
-
 // APIResourceSchemaReconciler watches APIServiceExportTemplates and ensures
 // that the APIResourceSchemas referenced by the owning APIBinding are copied
 // into the workspace with the kube-bind.io/exported=true label.
 type APIResourceSchemaReconciler struct {
-	manager    mcmanager.Manager
-	opts       controller.TypedOptions[mcreconcile.Request]
-	baseConfig *rest.Config
-	scheme     *runtime.Scheme
+	manager mcmanager.Manager
+	opts    controller.TypedOptions[mcreconcile.Request]
+	scheme  *runtime.Scheme
+	vwCache *shared.VWClientCache
 }
 
 // New returns a new APIResourceSchemaReconciler.
@@ -67,10 +60,10 @@ func New(
 	scheme *runtime.Scheme,
 ) (*APIResourceSchemaReconciler, error) {
 	return &APIResourceSchemaReconciler{
-		manager:    mgr,
-		opts:       opts,
-		baseConfig: baseConfig,
-		scheme:     scheme,
+		manager: mgr,
+		opts:    opts,
+		scheme:  scheme,
+		vwCache: shared.NewVWClientCache(baseConfig, scheme),
 	}, nil
 }
 
@@ -97,7 +90,7 @@ func (r *APIResourceSchemaReconciler) Reconcile(ctx context.Context, req mcrecon
 	}
 
 	// Find the owning APIBinding via annotation.
-	bindingName, ok := tmpl.Annotations[annotationOwnerBinding]
+	bindingName, ok := tmpl.Annotations[shared.AnnotationOwnerBinding]
 	if !ok {
 		logger.V(4).Info("Template has no owner-binding annotation, skipping", "name", tmpl.Name)
 		return ctrl.Result{}, nil
@@ -116,8 +109,8 @@ func (r *APIResourceSchemaReconciler) Reconcile(ctx context.Context, req mcrecon
 		return ctrl.Result{}, nil
 	}
 
-	// Build schema getter with VW fallback.
-	getSchema := r.schemaGetterWithFallback(c, clusterConfig)
+	// Build schema getter with VW fallback using the shared cache.
+	getSchema := shared.SchemaGetterWithFallback(c, clusterConfig, r.vwCache)
 
 	rec := reconciler{
 		client:               c,
@@ -133,74 +126,6 @@ func (r *APIResourceSchemaReconciler) Reconcile(ctx context.Context, req mcrecon
 	return ctrl.Result{}, nil
 }
 
-// extractClusterID extracts the cluster ID from an apiexport virtual workspace URL.
-func extractClusterID(clusterConfig *rest.Config) (string, error) {
-	u, err := url.Parse(clusterConfig.Host)
-	if err != nil {
-		return "", fmt.Errorf("failed to parse cluster host URL: %w", err)
-	}
-
-	pathParts := strings.Split(strings.Trim(u.Path, "/"), "/")
-	if len(pathParts) < 6 || pathParts[4] != "clusters" {
-		return "", fmt.Errorf("unexpected apiexport URL format: %s", u.Path)
-	}
-
-	return pathParts[5], nil
-}
-
-// newVWClient creates a client pointing at the apiresourceschema virtual workspace.
-func (r *APIResourceSchemaReconciler) newVWClient(clusterID string) (client.Client, error) {
-	cfg := rest.CopyConfig(r.baseConfig)
-	u, err := url.Parse(cfg.Host)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse base config host: %w", err)
-	}
-
-	u.Path = fmt.Sprintf("/services/apiresourceschema/%s/clusters/*", clusterID)
-	cfg.Host = u.String()
-
-	return client.New(cfg, client.Options{Scheme: r.scheme})
-}
-
-// schemaGetterWithFallback returns a function that first tries the workspace,
-// then falls back to the apiresourceschema virtual workspace.
-func (r *APIResourceSchemaReconciler) schemaGetterWithFallback(
-	workspaceClient client.Client,
-	clusterConfig *rest.Config,
-) func(ctx context.Context, name string) (*apisv1alpha1.APIResourceSchema, error) {
-	return func(ctx context.Context, name string) (*apisv1alpha1.APIResourceSchema, error) {
-		logger := log.FromContext(ctx)
-
-		var schema apisv1alpha1.APIResourceSchema
-		err := workspaceClient.Get(ctx, client.ObjectKey{Name: name}, &schema)
-		if err == nil {
-			return &schema, nil
-		}
-		if !errors.IsNotFound(err) {
-			return nil, err
-		}
-
-		logger.V(2).Info("APIResourceSchema not found in workspace, trying VW fallback", "schema", name)
-
-		clusterID, err := extractClusterID(clusterConfig)
-		if err != nil {
-			return nil, fmt.Errorf("cannot build VW fallback client: %w", err)
-		}
-
-		vwClient, err := r.newVWClient(clusterID)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create VW client for cluster %q: %w", clusterID, err)
-		}
-
-		var vwSchema apisv1alpha1.APIResourceSchema
-		if err := vwClient.Get(ctx, client.ObjectKey{Name: name}, &vwSchema); err != nil {
-			return nil, fmt.Errorf("APIResourceSchema %q not found in workspace or VW: %w", name, err)
-		}
-
-		return &vwSchema, nil
-	}
-}
-
 // SetupWithManager registers the controller with the multicluster-runtime Manager.
 func (r *APIResourceSchemaReconciler) SetupWithManager(mgr mcmanager.Manager) error {
 	return mcbuilder.ControllerManagedBy(mgr).