Merge pull request #2720 from ncdc/fix-apiexport-vw-queue-gap

🐛 Fix gap in APIExport virtual workspace queues

pkg/indexers/apiexport.go

@@ -17,11 +17,11 @@ limitations under the License.
 package indexers
 
 import (
-	"fmt"
-
 	kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
 	"github.com/kcp-dev/logicalcluster/v3"
 
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	apisv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1"
 )
 
@@ -30,25 +30,21 @@ const (
 	APIExportByIdentity = "APIExportByIdentity"
 	// APIExportBySecret is the indexer name for retrieving APIExports by secret.
 	APIExportBySecret = "APIExportSecret"
+	// APIExportByClaimedIdentities is the indexer name for retrieving APIExports that have a permission claim for a
+	// particular identity hash.
+	APIExportByClaimedIdentities = "APIExportByClaimedIdentities"
 )
 
 // IndexAPIExportByIdentity is an index function that indexes an APIExport by its identity hash.
 func IndexAPIExportByIdentity(obj interface{}) ([]string, error) {
-	apiExport, ok := obj.(*apisv1alpha1.APIExport)
-	if !ok {
-		return []string{}, fmt.Errorf("obj %T is not an APIExport", obj)
-	}
-
+	apiExport := obj.(*apisv1alpha1.APIExport)
 	return []string{apiExport.Status.IdentityHash}, nil
 }
 
 // IndexAPIExportBySecret is an index function that indexes an APIExport by its identity secret references. Index values
 // are of the form <cluster name>|<secret reference namespace>/<secret reference name> (cache keys).
 func IndexAPIExportBySecret(obj interface{}) ([]string, error) {
-	apiExport, ok := obj.(*apisv1alpha1.APIExport)
-	if !ok {
-		return []string{}, fmt.Errorf("obj %T is not an APIExport", obj)
-	}
+	apiExport := obj.(*apisv1alpha1.APIExport)
 
 	if apiExport.Spec.Identity == nil {
 		return []string{}, nil
@@ -65,3 +61,14 @@ func IndexAPIExportBySecret(obj interface{}) ([]string, error) {
 
 	return []string{kcpcache.ToClusterAwareKey(logicalcluster.From(apiExport).String(), ref.Namespace, ref.Name)}, nil
 }
+
+// IndexAPIExportByClaimedIdentities is an index function that indexes an APIExport by its permission claims' identity
+// hashes.
+func IndexAPIExportByClaimedIdentities(obj interface{}) ([]string, error) {
+	apiExport := obj.(*apisv1alpha1.APIExport)
+	claimedIdentities := sets.NewString()
+	for _, claim := range apiExport.Spec.PermissionClaims {
+		claimedIdentities.Insert(claim.IdentityHash)
+	}
+	return claimedIdentities.List(), nil
+}

pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_controller.go

@@ -84,7 +84,8 @@ func NewAPIReconciler(
 	indexers.AddIfNotPresentOrDie(
 		apiExportInformer.Informer().GetIndexer(),
 		cache.Indexers{
-			indexers.APIExportByIdentity: indexers.IndexAPIExportByIdentity,
+			indexers.APIExportByIdentity:          indexers.IndexAPIExportByIdentity,
+			indexers.APIExportByClaimedIdentities: indexers.IndexAPIExportByClaimedIdentities,
 		},
 	)
 
@@ -174,6 +175,25 @@ func (c *APIReconciler) enqueueAPIExport(apiExport *apisv1alpha1.APIExport, logg
 	}
 	logging.WithQueueKey(logger, key).V(2).Info("queueing APIExport")
 	c.queue.Add(key)
+
+	if apiExport.Status.IdentityHash != "" {
+		logger.V(4).Info("looking for APIExports to queue that have claims against this identity", "identity", apiExport.Status.IdentityHash)
+		others, err := indexers.ByIndex[*apisv1alpha1.APIExport](c.apiExportIndexer, indexers.APIExportByClaimedIdentities, apiExport.Status.IdentityHash)
+		if err != nil {
+			logger.Error(err, "error getting APIExports for claimed identity", "identity", apiExport.Status.IdentityHash)
+			return
+		}
+		logger.V(4).Info("got APIExports", "identity", apiExport.Status.IdentityHash, "count", len(others))
+		for _, other := range others {
+			key, err := kcpcache.MetaClusterNamespaceKeyFunc(other)
+			if err != nil {
+				logger.Error(err, "error getting key!")
+				continue
+			}
+			logging.WithQueueKey(logger, key).V(2).Info("queueing APIExport for claim")
+			c.queue.Add(key)
+		}
+	}
 }
 
 func (c *APIReconciler) startWorker(ctx context.Context) {

pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_reconcile.go

@@ -34,6 +34,7 @@ import (
 	apisv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1"
 	"github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1/permissionclaims"
 	"github.com/kcp-dev/kcp/pkg/indexers"
+	"github.com/kcp-dev/kcp/pkg/logging"
 	"github.com/kcp-dev/kcp/pkg/virtual/apiexport/schemas"
 	apiexportbuiltin "github.com/kcp-dev/kcp/pkg/virtual/apiexport/schemas/builtin"
 	"github.com/kcp-dev/kcp/pkg/virtual/framework/dynamic/apidefinition"
@@ -82,6 +83,9 @@ func (c *APIReconciler) reconcile(ctx context.Context, apiExport *apisv1alpha1.A
 	claims := map[schema.GroupResource]apisv1alpha1.PermissionClaim{}
 	claimsAPIBindings := false
 	for _, pc := range apiExport.Spec.PermissionClaims {
+		logger := logger.WithValues("claim", pc.String())
+		logger.V(4).Info("evaluating claim")
+
 		// APIExport resources have priority over claimed resources
 		gr := schema.GroupResource{Group: pc.Group, Resource: pc.Resource}
 		if _, found := apiResourceSchemas[gr]; found {
@@ -131,11 +135,16 @@ func (c *APIReconciler) reconcile(ctx context.Context, apiExport *apisv1alpha1.A
 			continue
 		}
 
+		logger = logger.WithValues("identity", pc.IdentityHash)
+
+		logger.V(4).Info("getting APIExports by identity")
 		exports, err := c.apiExportIndexer.ByIndex(indexers.APIExportByIdentity, pc.IdentityHash)
 		if err != nil {
 			return err
 		}
 
+		logger.V(4).Info("got APIExports", "count", len(exports))
+
 		// there might be multiple exports with the same identity hash all exporting the same GR.
 		// This is fine. Same identity means same owner. They have to ensure the schemas are compatible.
 		// The kcp server resource handlers will make sure the right structural schemas are applied. Here,
@@ -148,14 +157,22 @@ func (c *APIReconciler) reconcile(ctx context.Context, apiExport *apisv1alpha1.A
 
 		for _, obj := range exports {
 			export := obj.(*apisv1alpha1.APIExport)
+			logger := logger.WithValues(logging.FromPrefix("candidateAPIExport", export)...)
+			logger.V(4).Info("getting APIResourceSchemas for candidate APIExport")
 			candidates, err := c.getSchemasFromAPIExport(ctx, export)
 			if err != nil {
 				return err
 			}
+			logger.V(4).Info("got APIResourceSchemas for candidate APIExport", "count", len(candidates))
 			for _, apiResourceSchema := range candidates {
+				logger := logger.WithValues(logging.FromPrefix("candidateAPIResourceSchema", apiResourceSchema)...)
+				logger = logger.WithValues("candidateGroup", apiResourceSchema.Spec.Group, "candidateResource", apiResourceSchema.Spec.Names.Plural)
+				logger.V(4).Info("evaluating candidate APIResourceSchema")
 				if apiResourceSchema.Spec.Group != pc.Group || apiResourceSchema.Spec.Names.Plural != pc.Resource {
+					logger.V(4).Info("not a match")
 					continue
 				}
+				logger.V(4).Info("got a match!")
 				apiResourceSchemas[gr] = apiResourceSchema
 				identities[gr] = pc.IdentityHash
 				claims[gr] = pc

test/e2e/virtual/apiexport/authorizer_test.go

@@ -386,11 +386,13 @@ metadata:
 	require.NoError(t, err)
 
 	t.Logf("verify that service-provider-2-admin cannot list sherrifs resources via virtual apiexport apiserver because we have no local maximal permissions yet granted")
-	_, err = user2DynamicVWClient.Resource(schema.GroupVersionResource{Version: "v1", Resource: "sheriffs", Group: "wild.wild.west"}).List(ctx, metav1.ListOptions{})
-	require.ErrorContains(
-		t, err,
-		`sheriffs.wild.wild.west is forbidden: User "service-provider-2-admin" cannot list resource "sheriffs" in API group "wild.wild.west" at the cluster scope: access denied`,
-		"service-provider-2-admin must not be allowed to list sheriff resources")
+	framework.Eventually(t, func() (success bool, reason string) {
+		_, err = user2DynamicVWClient.Resource(schema.GroupVersionResource{Version: "v1", Resource: "sheriffs", Group: "wild.wild.west"}).List(ctx, metav1.ListOptions{})
+		if err == nil {
+			return false, "expected an error but got nil"
+		}
+		return strings.Contains(err.Error(), `sheriffs.wild.wild.west is forbidden: User "service-provider-2-admin" cannot list resource "sheriffs" in API group "wild.wild.west" at the cluster scope: access denied`), fmt.Sprintf("unexpected error: %v", err)
+	}, wait.ForeverTestTimeout, 100*time.Millisecond, "service-provider-2-admin must not be allowed to list sheriff resources")
 
 	_, err = user2DynamicVWClient.Resource(schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}).List(ctx, metav1.ListOptions{})
 	require.NoError(t, err, "service-provider-2-admin must be allowed to list native types")