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

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

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

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>

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

/retest

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

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!

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

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

New changes are detected. LGTM label has been removed.

ntnn added 5 commits August 26, 2026 23:08

Allow resource/subresourcce in validation

9a0035f

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

codegen

c4e51af

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

Add NamedCreaterFunc

012ebe1

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

Serve all subresources

0381bad

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

Handle subresources in claims

f4aebc2

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

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 added 6 commits August 27, 2026 08:02

Add e2e tests for ServiceAccount and TokenRequest

7106c64

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

Disallow setting selectors on subresource claims

5f3f522

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

codegen

07811b9

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

Set matchall for subresource claim in admission

39a8008

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

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

610db7e

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

4aba907

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

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

Retry creating APIBindings for sharded setups

4243015

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

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

ntnn added 2 commits August 28, 2026 18:51

Add test for scale (and status) subresource through VW

caa2147

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

Implement scale subresource in virtual workspaces

ddf4fc5

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

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

		// 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants