Skip to content

Implement subresources in VWs #4342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ntnn wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Implement subresources in VWs #4342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9a0035f
Allow resource/subresourcce in validation
ntnn Aug 26, 2026
c4e51af
codegen
ntnn Aug 26, 2026
012ebe1
Add NamedCreaterFunc
ntnn Aug 26, 2026
0381bad
Serve all subresources
ntnn Aug 26, 2026
f4aebc2
Handle subresources in claims
ntnn Aug 26, 2026
7106c64
Add e2e tests for ServiceAccount and TokenRequest
ntnn Aug 26, 2026
5f3f522
Disallow setting selectors on subresource claims
ntnn Aug 26, 2026
07811b9
codegen
ntnn Aug 26, 2026
39a8008
Set matchall for subresource claim in admission
ntnn Aug 26, 2026
610db7e
Add a maxmimum length for resource names to lessen estimated CEL cost
ntnn Aug 27, 2026
4aba907
codegen
ntnn Aug 27, 2026
4243015
Retry creating APIBindings for sharded setups
ntnn Aug 27, 2026
caa2147
Add test for scale (and status) subresource through VW
ntnn Aug 27, 2026
ddf4fc5
Implement scale subresource in virtual workspaces
ntnn Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions config/crds/apis.kcp.io_apibindings.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
resourceSelector:
description: resourceSelector is a list of claimed resource
Expand Down Expand Up @@ -198,9 +201,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
resourceSelector:
description: resourceSelector is a list of claimed resource
Expand Down Expand Up @@ -378,9 +384,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
resourceSelector:
description: resourceSelector is a list of claimed resource
Expand Down Expand Up @@ -556,9 +565,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
selector:
description: |-
Expand Down Expand Up @@ -636,6 +648,12 @@ spec:
- state
- verbs
type: object
x-kubernetes-validations:
- message: subresource claims must use selector.matchAll

@mjudeikis mjudeikis Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ntnn ntnn Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rule: '!self.resource.contains(''/'') || (has(self.selector.matchAll)
&& self.selector.matchAll)'
- message: defaultSelector is not allowed on subresource claims
rule: '!self.resource.contains(''/'') || !has(self.defaultSelector)'
type: array
x-kubernetes-list-map-keys:
- group
Expand Down Expand Up @@ -764,9 +782,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
selector:
description: |-
Expand Down Expand Up @@ -838,6 +859,12 @@ spec:
- selector
- verbs
type: object
x-kubernetes-validations:
- message: subresource claims must use selector.matchAll
rule: '!self.resource.contains(''/'') || (has(self.selector.matchAll)
&& self.selector.matchAll)'
- message: defaultSelector is not allowed on subresource claims
rule: '!self.resource.contains(''/'') || !has(self.defaultSelector)'
type: array
x-kubernetes-list-map-keys:
- group
Expand Down Expand Up @@ -1042,9 +1069,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
verbs:
description: |-
Expand All @@ -1060,6 +1090,9 @@ spec:
- resource
- verbs
type: object
x-kubernetes-validations:
- message: defaultSelector is not allowed on subresource claims
rule: '!self.resource.contains(''/'') || !has(self.defaultSelector)'
type: array
phase:
description: |-
Expand Down
13 changes: 11 additions & 2 deletions config/crds/apis.kcp.io_apiexports.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
resourceSelector:
description: resourceSelector is a list of claimed resource
Expand Down Expand Up @@ -472,9 +475,12 @@ spec:
resource:
description: |-
resource is the name of the resource.
A subresource may be claimed as "resource/subresource",
e.g. "serviceaccounts/token", in the style of RBAC rules.
Note: it is worth noting that you can not ask for permissions for resource provided by a CRD
not provided by an api export.
pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
maxLength: 127
pattern: ^[a-z][-a-z0-9]*[a-z0-9](/[a-z][-a-z0-9]*[a-z0-9])?$
type: string
verbs:
description: |-
Expand All @@ -490,6 +496,9 @@ spec:
- resource
- verbs
type: object
x-kubernetes-validations:
- message: defaultSelector is not allowed on subresource claims
rule: '!self.resource.contains(''/'') || !has(self.defaultSelector)'
type: array
x-kubernetes-list-map-keys:
- group
Expand Down
12 changes: 12 additions & 0 deletions pkg/admission/apibinding/apibinding_admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"errors"
"fmt"
"io"
"strings"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -177,6 +178,17 @@ func (o *apiBindingAdmission) Admit(ctx context.Context, a admission.Attributes,
ab.SetLabels(lbls)
}

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

@mjudeikis mjudeikis Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@ntnn ntnn Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// don't have to repeat it for every subresource
if v2, ok := ab.(*apiBindingV1alpha2); ok {
for i := range v2.binding.Spec.PermissionClaims {
pc := &v2.binding.Spec.PermissionClaims[i]
if strings.Contains(pc.Resource, "/") {
pc.Selector.MatchAll = true
}
}
}

// write back
raw, err := ab.ToUnstructured()
if err != nil {
Expand Down
8 changes: 6 additions & 2 deletions pkg/reconciler/apis/permissionclaimlabel/permissionclaimlabel_reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ func (c *controller) reconcile(ctx context.Context, apiBinding *apisv1alpha2.API

for _, s := range sets.List[string](allChanges) {
claim := claimFromSetKey(s)
// the claim references a subresource in rbac-style, the parent label applies
if strings.Contains(claim.Resource, "/") {
continue
}
if _, nonPersisted := permissionclaim.NonPersistedResourcesClaimable[schema.GroupResource{Group: claim.Group, Resource: claim.Resource}]; nonPersisted {
continue
}
Expand Down Expand Up @@ -297,11 +301,11 @@ func (c *controller) reconcile(ctx context.Context, apiBinding *apisv1alpha2.API
}

func setKeyForClaim(claim apisv1alpha2.PermissionClaim) string {
return fmt.Sprintf("%s/%s/%s", claim.Resource, claim.Group, claim.IdentityHash)
return fmt.Sprintf("%s|%s|%s", claim.Resource, claim.Group, claim.IdentityHash)
}

func claimFromSetKey(key string) apisv1alpha2.PermissionClaim {
parts := strings.SplitN(key, "/", 3)
parts := strings.SplitN(key, "|", 3)
return apisv1alpha2.PermissionClaim{
GroupResource: apisv1alpha2.GroupResource{
Group: parts[1],
Expand Down
6 changes: 3 additions & 3 deletions pkg/reconciler/apis/permissionclaimlabel/permissionclaimlabel_reconcile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestClaimSetKeys(t *testing.T) {
},
IdentityHash: "",
},
key: "configmaps//",
key: "configmaps||",
},
"non-core built-in gr": {
claim: apisv1alpha2.PermissionClaim{
Expand All @@ -53,7 +53,7 @@ func TestClaimSetKeys(t *testing.T) {
},
IdentityHash: "",
},
key: "roles/rbac.authorization.k8s.io/",
key: "roles|rbac.authorization.k8s.io|",
},
"3rd party gr + hash": {
claim: apisv1alpha2.PermissionClaim{
Expand All @@ -63,7 +63,7 @@ func TestClaimSetKeys(t *testing.T) {
},
IdentityHash: "hash",
},
key: "apibindings/apis.kcp.io/hash",
key: "apibindings|apis.kcp.io|hash",
},
}

Expand Down
11 changes: 8 additions & 3 deletions pkg/virtual/apiexport/authorizer/binding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,20 +131,25 @@ func (a *boundAPIAuthorizer) Authorize(ctx context.Context, attr authorizer.Attr
// check if a resource claim for this resource has been accepted and has correct verbs.
// normalize the requested group/resource to handle the events.k8s.io ↔ core/v1 equivalence.
normalizedGR := permissionclaim.NormalizeEventGroupResource(schema.GroupResource{Group: attr.GetAPIGroup(), Resource: attr.GetResource()})
// subresource status is implicitly granted with its parent resource.
// others must be claimed explicitly
Comment on lines +134 to +135

@xrstf xrstf Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this always the case?

@ntnn ntnn Aug 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gman0 gman0 Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ntnn ntnn Aug 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

claimedResource := normalizedGR.Resource
if sub := attr.GetSubresource(); sub != "" && sub != "status" {
claimedResource = normalizedGR.Resource + "/" + sub
}
for _, permissionClaim := range apiBinding.Spec.PermissionClaims {
if permissionClaim.State != apisv1alpha2.ClaimAccepted {
// if the claim is not accepted it cannot be used.
continue
}

if permissionClaim.Group == normalizedGR.Group && permissionClaim.Resource == normalizedGR.Resource {
if permissionClaim.Group == normalizedGR.Group && permissionClaim.Resource == claimedResource {
apiBindingVerbs := sets.New(permissionClaim.Verbs...)
apiExportVerbs := sets.New[string]()

apiExportVerbs := sets.New[string]()
for _, exportPermpermissionClaim := range apiExport.Spec.PermissionClaims {
if exportPermpermissionClaim.EqualGRI(permissionClaim.PermissionClaim) {
apiExportVerbs.Insert(exportPermpermissionClaim.Verbs...)

break
}
}
Expand Down
Loading