Implement subresources in VWs by ntnn · Pull Request #4342 · kcp-dev/kcp

ntnn · 2026-08-26T22:40:29Z

Summary

Implements subresources in VWs, specifically the TokenRequest.
Went with RBAC style in the claims for less API changes and because it feels closer. I tested a separate Subresoure as part of the claims but that didn't look quite right.

I've been hacking on this on and off for two weeks.
I think its secure now because to access a subresource a) the claim for the subresource needs to be present and b) the parent resource needs to be claimed as well, so the usual gates apply.
subresource claims cannot have their own labels to prevent mismatching labels between parent and subresource (I could see the value of being able to distinguish that a provider has access to resource set A but only to subset B for accessing the subresource - but that feels like the edge case of an edge case).

What Type of PR Is This?

/kind feature

Related Issue(s)

Fixes #

Release Notes

VWs can now handle subresources other than status of core APIs such as TokenRequest for ServiceAccount

kcp-ci-bot · 2026-08-26T22:40:37Z

[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 embik 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

ntnn · 2026-08-27T06:54:35Z

/retest

ntnn · 2026-08-27T09:03:56Z

/retest

One is #4343
The other === FAIL: test/e2e/apibinding TestAPIBindingPermissionClaimsAppliedAcrossShards (43.62s) looks like a flake

ntnn · 2026-08-27T09:06:17Z

/retest

infra failure

ntnn · 2026-08-27T10:58:36Z

/retest

 + set +o xtrace
Command terminated with 0
Compressing build artifacts...
tar: kcp/audit.log: file changed as we read it
tar: kcp: Cannot rmdir: Directory not empty
tar: Exiting with failure status due to previous errors

gman0 · 2026-08-27T11:49:10Z

Nice!

/lgtm

I'll leave approval for a bit if someone else wants to have a look.

kcp-ci-bot · 2026-08-27T11:49:17Z

LGTM label has been added.

Details

Git tree hash: 0bb5af6988e649277b698d6e26a194d0f83dfff4

xrstf · 2026-08-27T14:10:37Z

+	// subresource status is implicitly granted with its parent resource.
+	// others must be claimed explicitly


Was this always the case?

Yes, we just never checked for subresource permissions because only implemented status so far.
Given kube also doesn't give implicit access to the status subresource - but requiring this now would break existing APIExport/-Bindings.
I wanted to bring this up in todays community meeting to see how we feel about this.
On one hand I'd rather we are exact, on the other I'm not sure how we could make this "nice" for end users.

E.g. we could gate the implicit status on a feature gate, but that isn't really nice.
We could just keep status being implicit for now and require it explicitly in the next API version.
But I'm not really a fan of either.

Albeit not on paper, but the "spirit" of offering a service is for the consumer to give the spec, and provider to update the status - so I think it's assumed the provider needs access to that at all times, even when claiming. So if we're looking for an excuse to keep these perms implicit, I think this could be one :D but it does sound a bit weak.

I mean yeah but for resources exported via an APIExport we are already implicitly claiming the resource and the subresources^^
But I get what you mean. Plus that subresources are only really interesting for update/patch, so at that point the consumer is already accepting mutations from the provider on the claimed resource - and that could include subresources.

xrstf · 2026-08-27T14:11:13Z

@@ -119,6 +124,50 @@ func provideDelegatingRestStorage(ctx context.Context, dynamicClusterClientFunc

 		// TODO(sttts): add scale subresource


Is this TODO still relevant?

Yes. CRDs can support the scale subresource, so technically a CRD could be exported via an APIExport that supports the scale subresource.
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#scale-subresource

But why is it different than status? Isn't this PR working for all subresources?

Not all, but making it possible to handle subresources other than status with the changes to permission claims and handling it in authorization.

I can take a stab at implementing the scale subresource as well for completeness :D
I'm not sure if scale is actually different or if sttts just didn't bother with it at the time.

But I think there's more work down the line because I'm not sure how well this works for e.g. virtual resource. But I haven't used virtual resources at all so far so 🤷

Done :D Scale subresource works!

kcp-ci-bot · 2026-08-28T16:51:53Z

New changes are detected. LGTM label has been removed.

mjudeikis

Few other comments:

Two silent no-ops slip through admission:

an export claiming only serviceaccounts/token without claiming serviceaccounts → no API served, no error, no condition;
a CRD claim for a nonexistent subresource (cowboys/notreal + identityHash) → accepted, dead.

I have more things, but I think all and all we need better e2e:

Non existing resource
Only status claim
Inherited status permissions (status does not have claims)
....

mjudeikis · 2026-08-31T05:43:07Z

                  - verbs
                  type: object
+                  x-kubernetes-validations:
+                  - message: subresource claims must use selector.matchAll


Does this meant that if I claim named resource virtualmachine I will get all other virtualmachines subresources?

It just means that you can apply a selector on top of the selectors for the parent resource.

e.g. you have the virtualmachines resource with label my.exposed.resources=true and then you claim the subresource virtualmachines/ssh you cannot apply another label filter on top.
I had that implemented and I see some use for it (I mentioned that in the description) but I don't think that too many people will use it.
The story is basically "when you claim a subresource you must have access to its parent".
Hence the CEL validation that the selector must be matchall.

mjudeikis · 2026-08-31T06:25:50Z

 		ab.SetLabels(lbls)
 	}

+	// subresource claims have no selector, set a matchAll so users


This sets MatchAll = trueunconditionally, even when the user explicitly set matchLabels/matchExpressions on the subresource claim. ValidateAPIBindingPermissionClaims then rejects with "matchLabels cannot be used with matchAll" — for a matchAll the user never wrote, which will be very confusing to debug :)

The user cannot set matchlabels/-expression; I added a CEL rule for that.
Maybe the rule doesn't hold in which case I have to fix that :D

mjudeikis · 2026-09-08T06:04:10Z

Lets discuss this resource/status and resource relationship and permissioning in the cc.

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

127 because a resource can have up to 63 bytes per segment, so 63 for the resource, 63 for the subresource and 1 for the `/`. Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com> Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com> Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

ntnn · 2026-09-10T15:50:54Z

/retest

kcp-ci-bot · 2026-09-10T16:25:50Z

@ntnn: The following test 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-multiple-runs	`8f53854`	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.

mjudeikis · 2026-09-11T06:08:37Z

/retest

kcp-ci-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 26, 2026

ntnn force-pushed the vw-subresources-pr branch from 60af7fe to 3fc8c0e Compare August 26, 2026 22:42

platform-mesh-publisher Bot added this to Platform Mesh & Kube Projects - Backlog Aug 26, 2026

ntnn force-pushed the vw-subresources-pr branch from 3fc8c0e to 4aba907 Compare August 27, 2026 06:02

kcp-ci-bot assigned gman0 Aug 27, 2026

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2026

xrstf reviewed Aug 27, 2026

View reviewed changes

kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2026

kcp-ci-bot requested a review from gman0 August 28, 2026 16:51

kcp-ci-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 28, 2026

mjudeikis reviewed Aug 31, 2026

View reviewed changes

ntnn force-pushed the vw-subresources-pr branch 2 times, most recently from 91d92d3 to db1f579 Compare September 4, 2026 14:17

ntnn added 2 commits September 10, 2026 16:29

Allow resource/subresourcce in validation

301703c

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

codegen

ef3e56e

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

ntnn and others added 15 commits September 10, 2026 16:29

Add NamedCreaterFunc

3f9dfc1

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Serve all subresources

dd143b0

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Handle subresources in claims

5df9c4f

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Add e2e tests for ServiceAccount and TokenRequest

e49fa8c

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Disallow setting selectors on subresource claims

7192c13

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

codegen

8298545

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Set matchall for subresource claim in admission

e20da84

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Add a maxmimum length for resource names to lessen estimated CEL cost

af0fb3e

127 because a resource can have up to 63 bytes per segment, so 63 for the resource, 63 for the subresource and 1 for the `/`. Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

codegen

8c680b1

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Retry creating APIBindings for sharded setups

667e180

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com> Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Add test for scale (and status) subresource through VW

2509147

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Implement scale subresource in virtual workspaces

44c1d09

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Clearer validation message on permission claims for subresources

e382775

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com> Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

codegen

1c12683

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

Add test for implicit/explicit status subresource permission claim

8f53854

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com>

ntnn force-pushed the vw-subresources-pr branch from db1f579 to 8f53854 Compare September 10, 2026 15:00

		// subresource status is implicitly granted with its parent resource.
		// others must be claimed explicitly

		@@ -119,6 +124,50 @@ func provideDelegatingRestStorage(ctx context.Context, dynamicClusterClientFunc

		// TODO(sttts): add scale subresource

Uh oh!

Conversation

ntnn commented Aug 26, 2026

Summary

What Type of PR Is This?

Related Issue(s)

Release Notes

Uh oh!

kcp-ci-bot commented Aug 26, 2026

Uh oh!

ntnn commented Aug 27, 2026

Uh oh!

ntnn commented Aug 27, 2026

Uh oh!

ntnn commented Aug 27, 2026

Uh oh!

ntnn commented Aug 27, 2026

Uh oh!

gman0 commented Aug 27, 2026

Uh oh!

kcp-ci-bot commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcp-ci-bot commented Aug 28, 2026

Uh oh!

mjudeikis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mjudeikis commented Sep 8, 2026

Uh oh!

ntnn commented Sep 10, 2026

Uh oh!

kcp-ci-bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjudeikis commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kcp-ci-bot commented Sep 10, 2026 •

edited

Loading