Move permission claim object selectors from APIExport to APIBinding #3486

embik · 2025-07-14T14:07:08Z

Summary

This PR implements the API changes we wanted to get into v1alpha2 before we cut the release solidifying the API version. It's been mainly discussed on Slack: https://kubernetes.slack.com/archives/C021U8WSAFK/p1749552577773639. I don't think a strong consensus has been reached but the general direction is what this PR proposes.

It removes the all and resourceSelector fields from the PermissionClaim type (i.e. remove the object selector configuration from the APIExport on the service provider side). It instead adds a selector field in AcceptablePermissionClaim (the wrapper type used for permission claims in APIBindings) which allows the API consumer to scope access of an API provider to a specific resource.

Conversions have been added so that if a permission claim in an APIBinding had all: true, it would automatically translate into selector.matchAll: true (and back). To allow API roundtripping without loss of information, permission claims are now stored in an annotation when converting from v1alpha1 to v1alpha2, so that resourceSelector can be restored upon converting back to v1alpha1.

A few more things this PR does:

Update the usage of APIExport and APIBindings across the code base.
Split conversion tests in v1alpha2 package into two files.
Rename PermissionClaim.Equal to PermissionClaim.EqualGRI. Equal is used by cmp to compare objects, so (acceptable) permission claims were only compared partially, which shadowed some bugs in my code early on.
Switch compared objects in conversion tests so that the diff output was more readable.

What Type of PR Is This?

/kind cleanup
/kind api-change

Related Issue(s)

Fixes #3456

Release Notes

NONE

kcp-ci-bot · 2025-07-14T14:07:13Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

embik · 2025-07-15T15:00:06Z

/hold

Since I want to discuss this PR with @xmudrii.

embik · 2025-07-15T15:34:22Z

/retest

…ding On-behalf-of: SAP <marvin.beckers@sap.com> Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

On-behalf-of: SAP <marvin.beckers@sap.com> Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

sdk/apis/roundtrip_test.go

sdk/apis/apis/v1alpha2/types_apibinding_conversion.go

sdk/apis/apis/v1alpha2/types_apibinding.go

xmudrii · 2025-07-21T16:11:50Z

sdk/apis/apis/v1alpha2/types_apiexport_conversion_test.go

+			v2obj := v2.(*APIExport)
+			delete(v2obj.Annotations, PermissionClaimsV1Alpha1Annotation)
+			if len(v2obj.Annotations) == 0 {
+				v2obj.Annotations = nil
+			}


This is fine, but we should have a quick test to make sure the behavior around this annotation is correct.

sdk/apis/apis/v1alpha2/types_apibinding_conversion.go

kcp-ci-bot · 2025-07-23T08:43:25Z

LGTM label has been added.

Git tree hash: 083c7f231252d207ecee3f1cdf903dfcec7ebed2

mjudeikis · 2025-07-23T09:13:12Z

/hold

I think this is wrong. This moves permission claim responsibility to APIBinding from APIExport. This means that now you can have a need to check " accepted claims and state of them" on each binding. This will put big operational strain on the provider. Meaning on each reconcile loop provider needs to check what is the state of APIBinding and adjust reconciler for this. If user says 'selector=label=foo' provider will need to inject this.

mjudeikis · 2025-07-23T13:38:31Z

/hold cancel
Plan is to mutate on provider side so should be ok there,

sdk/apis/apis/fuzzer/fuzzer.go

cli/pkg/bind/plugin/bind.go

sdk/apis/roundtrip_test.go

sdk/apis/apis/v1alpha2/types_apibinding.go

sdk/apis/apis/v1alpha2/types_apibinding_conversion.go

sdk/apis/apis/v1alpha2/types_apibinding_test.go

sdk/apis/apis/v1alpha2/types_apiexport_conversion.go

On-behalf-of: SAP <marvin.beckers@sap.com> Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

embik · 2025-07-24T11:46:19Z

/retest

embik · 2025-07-24T12:02:40Z

/retest

Known flake: #3444

xrstf · 2025-07-24T12:53:48Z

/approve

kcp-ci-bot · 2025-07-24T12:53:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xmudrii, xrstf

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xrstf]
~~config/crds/OWNERS~~ [xrstf]
~~pkg/admission/OWNERS~~ [xrstf]
~~sdk/apis/OWNERS~~ [xrstf]

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

mjudeikis · 2025-07-24T15:13:34Z

/lgtm

kcp-ci-bot · 2025-07-24T15:13:39Z

LGTM label has been added.

Git tree hash: 3b2c2de7032b7d7600be6f7f2c70694e46d26128

kcp-ci-bot added area/cli Issues or PRs related to CLI changes size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 14, 2025

embik force-pushed the permission-claims-v1alpha2 branch from c63a31f to 17cf504 Compare July 15, 2025 11:41

embik marked this pull request as ready for review July 15, 2025 11:56

kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2025

embik force-pushed the permission-claims-v1alpha2 branch from 17cf504 to 1f528c8 Compare July 15, 2025 12:07

kcp-ci-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2025

embik force-pushed the permission-claims-v1alpha2 branch from 08aff20 to 6e05872 Compare July 15, 2025 15:11

Update v1alpha2 APIs to move object matching from APIExport to APIBin…

e338fac

…ding On-behalf-of: SAP <marvin.beckers@sap.com> Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

embik force-pushed the permission-claims-v1alpha2 branch from 6e05872 to f4fc17b Compare July 18, 2025 15:48

embik added 4 commits July 21, 2025 11:43

Update conversions for v1alpha2 API changes and clean existing ones up

faa668c

On-behalf-of: SAP <marvin.beckers@sap.com> Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

Replace usage of old v1alpha2 API fields with new ones

ae9b2e8

On-behalf-of: SAP <marvin.beckers@sap.com> Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

DeepCopy object metadata to allow setting metadata on out objects

00da074

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

Ignore known annotations on converted objects

e35699e

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

embik force-pushed the permission-claims-v1alpha2 branch from f4fc17b to e35699e Compare July 21, 2025 09:45

ntnn reviewed Jul 21, 2025

View reviewed changes

sdk/apis/roundtrip_test.go Show resolved Hide resolved

ntnn reviewed Jul 21, 2025

View reviewed changes

sdk/apis/apis/v1alpha2/types_apibinding_conversion.go Outdated Show resolved Hide resolved

ntnn reviewed Jul 21, 2025

View reviewed changes

sdk/apis/apis/v1alpha2/types_apibinding_conversion.go Outdated Show resolved Hide resolved

xmudrii suggested changes Jul 21, 2025

View reviewed changes

sdk/apis/apis/v1alpha2/types_apibinding.go Outdated Show resolved Hide resolved

kcp-ci-bot assigned xmudrii Jul 21, 2025

xmudrii reviewed Jul 21, 2025

View reviewed changes

sdk/apis/apis/v1alpha2/types_apibinding_conversion.go Show resolved Hide resolved

kcp-ci-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2025

mjudeikis reviewed Jul 23, 2025

View reviewed changes

sdk/apis/apis/fuzzer/fuzzer.go Show resolved Hide resolved