diff --git a/pkg/indexers/apiexport.go b/pkg/indexers/apiexport.go index c3ac9021950..413f9e0ac3d 100644 --- a/pkg/indexers/apiexport.go +++ b/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 |/ (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 +} diff --git a/pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_controller.go b/pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_controller.go index 681110f0c35..36178acd4f0 100644 --- a/pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_controller.go +++ b/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) { diff --git a/pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_reconcile.go b/pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_reconcile.go index 8b2fd7d9d30..faba7446463 100644 --- a/pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_reconcile.go +++ b/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 diff --git a/test/e2e/virtual/apiexport/authorizer_test.go b/test/e2e/virtual/apiexport/authorizer_test.go index b6da5347bfa..c28c7580fdc 100644 --- a/test/e2e/virtual/apiexport/authorizer_test.go +++ b/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")