Skip to content

Rework foreign apibinding permissionclaims logic#4088

Merged
kcp-ci-bot merged 2 commits into
kcp-dev:mainfrom
mjudeikis:foreign.informer.apibinding
May 4, 2026
Merged

Rework foreign apibinding permissionclaims logic#4088
kcp-ci-bot merged 2 commits into
kcp-dev:mainfrom
mjudeikis:foreign.informer.apibinding

Conversation

@mjudeikis

@mjudeikis mjudeikis commented Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

In this PR, a few things happen:

  1. Changes how we purge bound-crds, move to index. So we can track it more wider
  2. Adds an additional reconciler to materialise CRDs when its claimed via consumer which is not yet materialized in the shard
  3. Add additional triggers when informer is updated

Added extensive code comments as this was a ⛏️

What Type of PR Is This?

/kind bug

Related Issue(s)

Fixes #4087

Release Notes

Fix foreign apiexports, claimed in apibindings in sharded environments.

@kcp-ci-bot kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. dco-signoff: yes Indicates the PR's author has signed the DCO. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 29, 2026
gman0
gman0 reviewed Apr 29, 2026
View reviewed changes
Comment thread pkg/reconciler/apis/crdcleanup/crdcleanup_controller.go Outdated
@gman0

gman0 commented Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

In general, is it really okay to claim resources that haven't been bound / don't exist in APIBinding's workspace?

An argument in favor could be that we don't care about the order in which APIBindings come (those who have claims against a resource VS the binding that binds that resource); depends 🤷 other than that,

/lgtm

It will be interesting to see how this affects perf with large number of bindings though.

@gman0

gman0 commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

^

/lgtm

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

kcp-ci-bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 39cef409f73c601a9873b7a7e5492463cde9b6bf

@mjudeikis

mjudeikis commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

In general, is it really okay to claim resources that haven't been bound / don't exist in APIBinding's workspace

This is an existing feature, so it just does not work in this edge case

@kcp-ci-bot kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2026
@kcp-ci-bot kcp-ci-bot requested a review from gman0 April 30, 2026 07:27
@gman0

gman0 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

/lgtm

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

kcp-ci-bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 78ccd199e32d3c9d526efa49c6a45b6a62efe59d

@gman0

gman0 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

https://docs.kcp.io/kcp/main/concepts/apis/exporting-apis/#permission-claims_1

Permission claims must be accepted by the user explicitly, before this access is granted. The resources can be builtin Kubernetes resources or resources bound with other APIBindings.

Docs might need a change too then.

ntnn
ntnn reviewed Apr 30, 2026
View reviewed changes
Comment thread pkg/reconciler/apis/permissionclaimlabel/permissionclaimlabel_controller.go Outdated
Comment thread pkg/reconciler/apis/apibinding/apibinding_reconcile.go
Comment thread pkg/indexers/apiexport.go
@ntnn ntnn moved this to Reviewing in tbd Apr 30, 2026
@ntnn ntnn added this to tbd Apr 30, 2026
@mjudeikis
Rework foreign apibinding permissionclaims logic
de105e8 
Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt>
On-behalf-of: SAP <mangirdas.judeikis@sap.com>
@mjudeikis mjudeikis force-pushed the foreign.informer.apibinding branch from cf2f4ec to 28e1707 Compare May 4, 2026 08:26
@kcp-ci-bot kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label May 4, 2026
@mjudeikis mjudeikis force-pushed the foreign.informer.apibinding branch from 28e1707 to 720783e Compare May 4, 2026 08:31
ntnn
ntnn approved these changes May 4, 2026
View reviewed changes

@ntnn ntnn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm
/approve

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

kcp-ci-bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 9eaec19adaf46aec89706ec0aee4bf49453a0d49

@kcp-ci-bot

kcp-ci-bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ntnn

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2026
@mjudeikis

mjudeikis commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@kcp-ci-bot kcp-ci-bot merged commit d1be278 into kcp-dev:main May 4, 2026
14 checks passed
@github-project-automation github-project-automation Bot moved this from Reviewing to Done in tbd May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntnn ntnn ntnn approved these changes

@gman0 gman0 Awaiting requested review from gman0

Assignees

@gman0 gman0

@ntnn ntnn

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: Failed to get informer in sharded setup

4 participants

@mjudeikis @gman0 @kcp-ci-bot @ntnn