improve controllers

On-behalf-of: @SAP christoph.mewes@sap.com

Author: xrstf

internal/controller/apiexport/controller.go

@@ -26,6 +26,8 @@ import (
 
 	"github.com/kcp-dev/api-syncagent/internal/controllerutil"
 	predicateutil "github.com/kcp-dev/api-syncagent/internal/controllerutil/predicate"
+	"github.com/kcp-dev/api-syncagent/internal/discovery"
+	"github.com/kcp-dev/api-syncagent/internal/kcp"
 	"github.com/kcp-dev/api-syncagent/internal/projection"
 	"github.com/kcp-dev/api-syncagent/internal/resources/reconciling"
 	"github.com/kcp-dev/api-syncagent/internal/validation"
@@ -53,14 +55,15 @@ const (
 )
 
 type Reconciler struct {
-	localClient   ctrlruntimeclient.Client
-	kcpClient     ctrlruntimeclient.Client
-	log           *zap.SugaredLogger
-	recorder      record.EventRecorder
-	lcName        logicalcluster.Name
-	apiExportName string
-	agentName     string
-	prFilter      labels.Selector
+	localClient     ctrlruntimeclient.Client
+	kcpClient       ctrlruntimeclient.Client
+	discoveryClient *discovery.Client
+	log             *zap.SugaredLogger
+	recorder        record.EventRecorder
+	lcName          logicalcluster.Name
+	apiExportName   string
+	agentName       string
+	prFilter        labels.Selector
 }
 
 // Add creates a new controller and adds it to the given manager.
@@ -73,15 +76,21 @@ func Add(
 	agentName string,
 	prFilter labels.Selector,
 ) error {
+	discoveryClient, err := discovery.NewClient(mgr.GetConfig())
+	if err != nil {
+		return fmt.Errorf("failed to create discovery client: %w", err)
+	}
+
 	reconciler := &Reconciler{
-		localClient:   mgr.GetClient(),
-		kcpClient:     kcpCluster.GetClient(),
-		lcName:        lcName,
-		log:           log.Named(ControllerName),
-		recorder:      kcpCluster.GetEventRecorderFor(ControllerName),
-		apiExportName: apiExportName,
-		agentName:     agentName,
-		prFilter:      prFilter,
+		localClient:     mgr.GetClient(),
+		kcpClient:       kcpCluster.GetClient(),
+		discoveryClient: discoveryClient,
+		lcName:          lcName,
+		log:             log.Named(ControllerName),
+		recorder:        kcpCluster.GetEventRecorderFor(ControllerName),
+		apiExportName:   apiExportName,
+		agentName:       agentName,
+		prFilter:        prFilter,
 	}
 
 	hasARS := predicate.NewPredicateFuncs(func(object ctrlruntimeclient.Object) bool {
@@ -93,7 +102,7 @@ func Add(
 		return publishedResource.Status.ResourceSchemaName != ""
 	})
 
-	_, err := builder.ControllerManagedBy(mgr).
+	_, err = builder.ControllerManagedBy(mgr).
 		Named(ControllerName).
 		WithOptions(controller.Options{
 			// we reconcile a single object in kcp, no need for parallel workers
@@ -151,7 +160,12 @@ func (r *Reconciler) reconcile(ctx context.Context, apiExport *kcpdevv1alpha1.AP
 	// for each PR, we note down the created ARS and also the GVKs of related resources
 	newARSList := sets.New[string]()
 	for _, pubResource := range filteredPubResources {
-		newARSList.Insert(pubResource.Status.ResourceSchemaName)
+		schemaName, err := r.getSchemaName(ctx, &pubResource)
+		if err != nil {
+			return fmt.Errorf("failed to determine schema name for PublishedResource %s: %w", pubResource.Name, err)
+		}
+
+		newARSList.Insert(schemaName)
 	}
 
 	// To determine if the GVR of a related resource needs to be listed as a permission claim,
@@ -259,3 +273,22 @@ func (r *Reconciler) reconcile(ctx context.Context, apiExport *kcpdevv1alpha1.AP
 
 	return nil
 }
+
+func (r *Reconciler) getSchemaName(ctx context.Context, pubRes *syncagentv1alpha1.PublishedResource) (string, error) {
+	if pubRes.Status.ResourceSchemaName != "" {
+		return pubRes.Status.ResourceSchemaName, nil
+	}
+
+	// Technically we *could* wait and let the apiresourceschema controller do its
+	// job and provide the status field above. But this would mean we potentially
+	// temporarily misidentify related resources, adding unnecessary and invalid
+	// permission claims in the APIExport. To avoid these it's worth it to basically
+	// do the same projection logic as the other controller here, just to calculate
+	// what the name of the schema would/will be.
+	projectedCRD, err := projection.ProjectPublishedResource(ctx, r.discoveryClient, pubRes)
+	if err != nil {
+		return "", fmt.Errorf("failed to apply projection rules: %w", err)
+	}
+
+	return kcp.GetAPIResourceSchemaName(projectedCRD), nil
+}

internal/controller/apiexport/reconciler.go

@@ -191,7 +191,7 @@ func parseSchemaName(name string) (schema.GroupVersionResource, error) {
 	// <version>.<resource>.<group>
 	parts := strings.SplitN(name, ".", 3)
 	if len(parts) != 3 {
-		return schema.GroupVersionResource{}, fmt.Errorf("invalid schema name %q, must consist of version.resource.group.", name)
+		return schema.GroupVersionResource{}, fmt.Errorf("invalid schema name %q, must consist of version.resource.group", name)
 	}
 
 	return schema.GroupVersionResource{

internal/controller/apiresourceschema/controller.go

@@ -144,22 +144,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 }
 
 func (r *Reconciler) reconcile(ctx context.Context, log *zap.SugaredLogger, pubResource *syncagentv1alpha1.PublishedResource) (*reconcile.Result, error) {
-	// find the resource that the PublishedResource is referring to
-	localGK := projection.PublishedResourceSourceGK(pubResource)
-
+	// get the projected CRD (i.e. strip unwanted versions, rename values etc.)
 	client, err := discovery.NewClient(r.restConfig)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create discovery client: %w", err)
 	}
 
-	// fetch the original, full CRD from the cluster
-	crd, err := client.RetrieveCRD(ctx, localGK)
-	if err != nil {
-		return nil, fmt.Errorf("failed to discover resource defined in PublishedResource: %w", err)
-	}
-
-	// project the CRD (i.e. strip unwanted versions, rename values etc.)
-	projectedCRD, err := projection.ProjectCRD(crd, pubResource)
+	projectedCRD, err := projection.ProjectPublishedResource(ctx, client, pubResource)
 	if err != nil {
 		return nil, fmt.Errorf("failed to apply projection rules: %w", err)
 	}

internal/projection/projection.go

@@ -17,11 +17,13 @@ limitations under the License.
 package projection
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"slices"
 	"strings"
 
+	"github.com/kcp-dev/api-syncagent/internal/discovery"
 	syncagentv1alpha1 "github.com/kcp-dev/api-syncagent/sdk/apis/syncagent/v1alpha1"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -113,6 +115,25 @@ func ProjectCRD(crd *apiextensionsv1.CustomResourceDefinition, pubRes *syncagent
 	return result, nil
 }
 
+func ProjectPublishedResource(ctx context.Context, client *discovery.Client, pubRes *syncagentv1alpha1.PublishedResource) (*apiextensionsv1.CustomResourceDefinition, error) {
+	// find the resource that the PublishedResource is referring to
+	localGK := PublishedResourceSourceGK(pubRes)
+
+	// fetch the original, full CRD from the cluster
+	crd, err := client.RetrieveCRD(ctx, localGK)
+	if err != nil {
+		return nil, fmt.Errorf("failed to discover resource defined in PublishedResource: %w", err)
+	}
+
+	// project the CRD (i.e. strip unwanted versions, rename values etc.)
+	projectedCRD, err := ProjectCRD(crd, pubRes)
+	if err != nil {
+		return nil, fmt.Errorf("failed to apply projection rules: %w", err)
+	}
+
+	return projectedCRD, nil
+}
+
 func RelatedResourceGVR(rr *syncagentv1alpha1.RelatedResourceSpec) schema.GroupVersionResource {
 	resultGVR := schema.GroupVersionResource{
 		Group:    rr.Group,
@@ -124,8 +145,12 @@ func RelatedResourceGVR(rr *syncagentv1alpha1.RelatedResourceSpec) schema.GroupV
 	//nolint:staticcheck
 	switch rr.Kind {
 	case "ConfigMap":
+		resultGVR.Group = ""
+		resultGVR.Version = "v1"
 		resultGVR.Resource = "configmaps"
 	case "Secret":
+		resultGVR.Group = ""
+		resultGVR.Version = "v1"
 		resultGVR.Resource = "secrets"
 	}