[mjudeikis]

Summary

What Type of PR Is This?

Related Issue(s)

Fixes #

Release Notes

NONE

[kcp-ci-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mjudeikis for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS
• config/crds/OWNERS
• pkg/admission/OWNERS
• staging/src/github.com/kcp-dev/sdk/apis/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mjudeikis]

/build-image

[mjudeikis]

Claimed custom subresources are authorized by the APIExport VW but never served through it

Tested this branch (ef58fdd09) end to end with two service providers, and the own-export path works well: a widgets/frobnicate entry with storage.virtual pointing at an endpoint slice is routed by the shard to the provider's endpoint, X-Remote-User/X-Remote-Group/X-Remote-Extra-* are stamped, cross-workspace calls are refused by the shard's own authz.

The cross-provider case has a gap, though. Setup:

• Provider A exports widgets.a.example.com and declares the custom subresource widgets/frobnicate (virtual storage → A's endpoint).
• Provider B exports things.b.example.com and claims a.example.com/widgets (get,list,watch,create,update) and a.example.com/widgets/frobnicate (create). A consumer workspace binds both and accepts B's claims.

Through B's APIExport virtual workspace (/services/apiexport/<cluster>/things.b.example.com/clusters/<consumer>/...):

• GET/POST .../apis/a.example.com/v1alpha1/widgets — works, as expected for a claimed resource.
• POST .../apis/a.example.com/v1alpha1/widgets/x/frobnicate — 404. The VW has no API for it.

Why: the claim is accepted by the authorizer but nothing builds an API behind it.

• pkg/virtual/apiexport/authorizer/binding.go:147 explicitly matches resource/sub claims ("subresources must be claimed explicitly"), so the authz half exists.
• pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_reconcile.go:93 skips every claim whose resource contains / ("the API comes from the parent resource claim"), and :342 skips the export's own <resource>/<sub> entries ("served by the virtual workspace its own storage names").
• pkg/virtual/apiexport/builder/forwarding.go:160 only ever builds subresource storages for status, scale and BuiltInSubresources.

So a claim on a custom subresource is a declaration with nothing behind it. The practical consequence is that the only way for B to call A's verb today is the shard path /clusters/<consumer>/apis/a.example.com/v1alpha1/widgets/x/frobnicate, which requires B to hold RBAC inside every consumer workspace — the thing permission claims exist to avoid. For us it means custom subresources replace our own REST grammar for user-facing calls, but provider-to-provider calls still need an out-of-band identity per consumer workspace.

Ask: for a claimed (and own) <resource>/<sub> entry, build a forwarding storage in the APIExport VW that sends create to the shard the way builtin subresources are forwarded at forwarding.go:160-190 (including the parent-GET gate that prevents reaching a subresource of an unclaimed parent). The shard's virtualresources handler (resolveVirtualStorage / requestsCustomSubresource, pkg/server/virtualresources/server.go:485,507) already resolves the entry and routes it to A's endpoint, so the missing piece looks confined to the VW's API set.

Related question (identity on that hop): when the request arrives at A's endpoint via B's VW, A sees X-Remote-User = B's impersonated identity (for a ServiceAccount, system:serviceaccount:<ns>:<name> — the same spelling as any consumer's SA of that name, disambiguated only by the authentication.kcp.io/cluster-name extra) plus the VW's warrant in X-Remote-Extra-*. If A then runs a SubjectAccessReview for that user in the consumer workspace, does the SAR honour warrant extras from the spec? If yes, forwarding the warrant to a third-party backend is a footgun (the backend can mint itself system:kcp:admin in a SAR); if no, A has no way to authorize the caller other than trusting that the shard already did. Either answer is fine, but it should be documented for endpoint authors.

Happy to turn the first part into a failing e2e in test/e2e/virtual/apiexport if useful.

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

An identity-agnostic claim on a resource served from a ClusterCachedResource is authorized and discoverable, but always lists empty

Follow-up from the same setup, tested against this branch's head (4688fe6e7). The claimed-resource path works for anything the producing export serves from storage.crd; it silently serves nothing when the producer serves that kind from a cached resource.

Setup:

• Provider A exports infrastructure.example.ai and serves two kinds: instances with storage.crd, and templates with storage.virtual pointing at a ClusterCachedResourceEndpointSlice (a ClusterCachedResource replicating A's own Templates, so every consumer sees the same catalog without holding copies).
• Provider B claims infrastructure.example.ai/templates with an empty identityHash — the identity-agnostic form admitted through a PermissionClaimPolicy.
• A consumer workspace binds A and B and accepts B's claims. status.appliedPermissionClaims on the binding carries templates.

Observed through B's APIExport virtual workspace, /services/apiexport/<cluster>/<B's export>/clusters/<consumer>/apis/infrastructure.example.ai/v1alpha1/...:

Read	Result
discovery of infrastructure.example.ai/v1alpha1	lists templates with get,list,watch,create,update,patch,delete,deletecollection
GET .../templates	200, items: []
GET .../connections (a third provider's kind, storage.crd, claimed the same way)	200, the consumer's one real object

The objects are definitely there and definitely reachable by other routes: the consumer sees 8 Templates through its own binding to A, and A's own APIExport VW returns the same 8 for that consumer workspace. Reading the shard path directly as B's ServiceAccount is a clean 403, so this is not RBAC leaking through as an empty result.

Why, as far as the code goes. For pc.IdentityHash == "", apiexport_apireconciler_reconcile.go:160 resolves the identity per consumer and registers the schema, then sets identities[gr] = c.dynamicIdentities(apiExport, gr) at :193. That resolves through IdentityResolver.identityInCluster, which takes the hash straight off the consumer binding's bound resource:

// pkg/permissionclaim/identity.go:182
if bound.Group != gr.Group || bound.Resource != gr.Resource || bound.Schema.IdentityHash == "" {
    continue
}
return ClaimedIdentity{IdentityHash: bound.Schema.IdentityHash, ...}

For a kind backed by a cached resource those are two different identities:

Identity	Value (truncated)	What it addresses
A's APIExport	947904b2d432…	what bound.Schema.IdentityHash reports, for both instances and templates
ClusterCachedResource	6414a16a0b24…	status.identityHash, where the replicated Templates actually live

Exactly one hash comes back, so per DefaultDynamicDelegatedStoreFuncsWithIdentities the forwarded per-cluster request carries the :identity suffix rather than letting the shard resolve it from the binding. The claimed read therefore goes out as templates:947904b2…, which nothing answers, and a list against a store with no matching objects is an empty list rather than an error. Nothing under pkg/virtual/apiexport/ looks at storage.virtual or at cached resources at all, so there is no path that would notice.

That last link is read from the code rather than observed: the identity-suffixed form is refused for an ordinary admin token, so I could not probe templates:<hash> from outside to confirm it directly.

Ask. Two parts, and the second matters more than the first:

1. Resolve a bound resource whose producing export entry uses storage.virtual to the identity of the cached resource behind that endpoint slice, so a claim on such a kind is served the replicated objects. (Or route it the way the shard already routes the consumer's own read of that bound resource, which works today.)
2. Failing that, refuse to serve the claim, so it fails loudly. A silent empty list is the worst available outcome here: a reconciler reading a claimed catalog through its own VW cannot tell "this workspace has no Templates" from "this kind cannot be claimed", and will happily converge on the empty answer. An explicit refusal at admission, or a 501/404 on the VW, would have turned two days of bisecting into one error message.

Happy to turn this into a failing e2e in test/e2e/virtual/apiexport alongside the custom-subresource one, if that is useful.

[mjudeikis]

Correcting myself on the comment above: the identity story is wrong, and the defect is broader than I described. Two checks settled it.

1. The objects carry no claim label. Same consumer workspace, both kinds claimed and accepted by the same export:

# Template (producer serves it from a ClusterCachedResource)
metadata.labels: <none>

# Connection (producer serves it from storage.crd, claimed the same way)
metadata.labels:
  claimed.internal.apis.kcp.io/bTqLrHG7Cw8ij3SSx68nMsJVU89bko5TnVo8Hg: 5KGjMDkQGw5akbFVfDcw00dGOycbd9iX1jecir

2. Pinning the claim to an explicit identityHash changes nothing. I set identityHash to the producing APIExport's identity on both the claiming APIExport's spec.permissionClaims entry and the consumer APIBinding's, confirmed the binding went Bound with the pinned claim in status.appliedPermissionClaims, and listed again through the claiming export's VW: still 200 with items: [].

So nothing in dynamicIdentities or IdentityResolver.identityInCluster is implicated, and this is not specific to identity-agnostic claims. Please disregard that half of the previous comment.

The actual mechanism is the claim label selector. For a claimed resource the VW builds a label requirement and filters every read through it:

• apiexport_apireconciler_reconcile.go:292-307 builds key IN (claimLabels) from permissionclaims.ToLabelKeyAndValue(clusterName, apiExport.Name, claim).
• builder/build.go:169 wraps the delegating storage in forwardingregistry.WithLabelSelector with exactly those requirements.
• The labels themselves are put on objects by pkg/reconciler/apis/permissionclaimlabel/permissionclaimlabel_resource_controller.go, which drives off the discovering dynamic informer factory and patches objects in the consumer workspace.

Replicated objects never get labelled, so the selector matches nothing and the read is correct-but-empty. That is consistent with everything observed, including why a storage.crd claim on a neighbouring kind works and why the consumer's own read through its binding to the producer returns all 8 objects.

A better title for this would be: a permission claim on a resource its producing APIExport serves from a ClusterCachedResource is authorized and discoverable, but always reads empty, because the claim labeler never labels replicated objects.

The ask changes shape accordingly. Either the labeler has to cover replicated resources, or, if labelling a cached object is not meaningful (it is a copy, and the label is keyed by the consumer cluster and claiming export), the claimed read for such a kind needs to skip the label selector and be scoped some other way. What should not survive either way is the silent empty list: admission knows at claim time that the producing entry uses storage.virtual, so refusing the claim there would turn this into one error message instead of a bisect. Happy to write the failing e2e for it.

[mjudeikis]

so if we move to matchAll, it should work?

No, and that is worth stating in the PR, because it is already the case. The consumer binding's claim on the cached kind is selector: {matchAll: true} today, and that is the read that returns items: []. The claim on the neighbouring storage.crd kind, which works, is also matchAll. So matchAll is not a configuration anyone can adopt to get out of this.

The reason is that the selector never reaches the decision. apiexport_apireconciler_reconcile.go:292 gates the label requirement only on the resource being claimed at all:

var labelReqs labels.Requirements
if c, ok := claims[gvr.GroupResource()]; ok {
    key, label, err := permissionclaims.ToLabelKeyAndValue(clusterName, apiExport.Name, c)
    ...
    req, err := labels.NewRequirement(key, selection.In, claimLabels)
    ...
    labelReqs = labels.Requirements{*req}
}

c is in hand but c.Selector is not consulted. Every claimed resource gets the label filter, matchAll included. So "skip the selector for a matchAll claim on a cached kind" is a new branch here rather than a behavioural tweak, which I think makes the two-case rule easier to land, not harder:

• matchAll on a cached kind: skip the label selector. Agreed it is not the boundary. The VW authorizer already requires an accepted claim with matching verbs, and the cached-resource scoping confines the read to the producer's source cluster. The requirement as built can only ever exclude everything.
• matchLabels/matchExpressions on a cached kind: refuse it. Agreed, and refusing beats silently widening to matchAll.

Also confirming the API point, since it is load-bearing for stating the rule: the xor is already enforced, not just conventional. staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/validation.go:75-81 rejects matchLabels or matchExpressions together with matchAll, and requires exactly one of the three to be set. So the two cases are distinguishable from the claim alone, with no new fields.

And agreed that labelling replicas is the wrong direction, for the reason you give: the label key is derived from the consumer cluster and the claiming export, while the cache holds one copy shared by every consumer, so there is no single correct value to write even if the replication storage were writable.

I will put up the failing e2e in test/e2e/virtual/apiexport. The assertion is the same under either fix: a matchAll claim on a kind the producing export serves from a ClusterCachedResource, read through the claiming export's virtual workspace in a consumer workspace that accepted the claim, returns the producer's objects rather than an empty list. I will add the matchLabels case as a companion asserting a refusal, so whichever way the fix goes one of the two has to be updated deliberately.

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

/build-image

[mjudeikis]

/test all

[kcp-ci-bot]

@mjudeikis: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-test-e2e-shared	9790f69	link	true	/test pull-kcp-test-e2e-shared
pull-kcp-lint	9790f69	link	true	/test pull-kcp-lint
pull-kcp-test-e2e	9790f69	link	true	/test pull-kcp-test-e2e
pull-kcp-test-e2e-sharded	9790f69	link	true	/test pull-kcp-test-e2e-sharded
pull-kcp-test-e2e-sharded-embedded-vw	9790f69	link	false	/test pull-kcp-test-e2e-sharded-embedded-vw
pull-kcp-test-e2e-multiple-runs	9790f69	link	true	/test pull-kcp-test-e2e-multiple-runs

Full PR test history

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[kcp-ci-bot]

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.