Add finalizing virtualworkspace #3615

SimonTheLeg · 2025-10-01T15:29:42Z

This PR adds the finalizing virtual workspace and surrounding logic. Similiar to initializers, Workspacetypes can specify having a finalizer or inherit finalizers via extends.With. These finalizers get added to the Logicalclusters and need to be removed before a logicalcluster can be fully deleted. A virtual workspace is being provided which gives access to all Logicalclusters which have the corresponding finalizer. This allows end-users to write operators with custom cleanup logic.
More details can be obtained from the docs update in this PR.

Open discussions / TODOs

When we initially designed the feature, we decided that for end-user writing a finalizer operator, they should directly remove their finalizer from metadata.finalizers, as this has the most k8s native feel. This is also how it is implemented at the moment. This is slightly different to how initializers work, since for initializers you would remove it from status.initializers. Back then we decided that for finalizers we prefer the metadata.finalizers approach due to ease of use. However while working on this some new facts have come to light, which make me wonder if we want to stick with this approach: The main factor is that we cannot directly use regular finalizers (e.g. "root:myworkspacetype"), as the colon : character is not allowed in the metadata.finalizers field. We therefore always have to convert these. Currently we simply replace the colon character with a dot (https://github.com/SimonTheLeg/kcp/blob/d46891c8d2111ce3d1e94891ab4bfcc9137d4d46/sdk/apis/tenancy/finalization/utils.go#L71-L73). However for end-users I think this is rather confusing, because they would also need to do this conversion. As a result I am wondering if it would not be better if we keep this implementation detail hidden; Essentially build it exactly like we do it for initializers and have people patch the status there and then our controller would update the metadata.finalizers field. What do you prefer?
Currently the fianlizer virtualworkspace only gives you access to the LogicalCluster object. This is different from how initializers work, where the workspace gives access to every object in the LogicalCluster. For initializers this is done via proxying the requests directly to kcp (see https://github.com/SimonTheLeg/kcp/blob/d46891c8d2111ce3d1e94891ab4bfcc9137d4d46/pkg/virtual/initializingworkspaces/builder/build.go#L200-L291). In the past I remember we had two different "camps" whether we want the same feature as well. @embik do we still have reservations about this, or should I just use the same proxying as in initializers and extend the E2E tests?
TODO @SimonTheLeg → finish writing documentation once 1 + 2 are decided

Summary

What Type of PR Is This?

/kind feature

Release Notes

Allow for custom cleanup logic of LogicalClusters through the finalizing virtualworkspace

kcp-ci-bot · 2025-10-01T15:29:49Z

[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 mjudeikis for approval. For more information see the Kubernetes Code Review Process.

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

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

…rName This is required so we can use the LogicalClusterFinalizer as a custom string type like we do it with LogicalClusterInitializer On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

…urls" This reverts commit 623580d.

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

embik · 2025-10-06T08:34:00Z

Getting the ball rolling on the discussion items:

I started out writing a comment in favour of the the dot conversion, but I'm realizing this might have (security) implications since objects and workspaces can have dots in their name too. Can you please explore if it is possible to confuse dot-converted finalizers with a workspace name imitating it (e.g. you have a WorkspaceType in root:a:b, and someone creates root:a.b and tries to mess with your finalizers)?
I suspect that finalizers without access to the workspace are mostly useless (they only make sense for external system integration), so I would be in favour of such a proxy here as well. I'm interested in more opinions on this.

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

SimonTheLeg · 2025-10-06T09:54:44Z

/test pull-kcp-test-e2e-shared

mjudeikis · 2025-10-07T06:34:07Z

Getting the ball rolling on the discussion items:

I started out writing a comment in favour of the the dot conversion, but I'm realizing this might have (security) implications since objects and workspaces can have dots in their name too. Can you please explore if it is possible to confuse dot-converted finalizers with a workspace name imitating it (e.g. you have a WorkspaceType in root:a:b, and someone creates root:a.b and tries to mess with your finalizers)?

Im somehow tempted to say - let's use the same behaviour - patch status. Feels like the finalisers field should be protected from user modification here. And . does not make it easier to argue, too. Otherwise, we have 2 different behaviours for initialising and finalising, and I don't like this either. It might have been a mistake doing it this way, but we have it now and I think we're better off sticking with it.

If we agree otherwise, I would make the same change to both virtual workspaces to have consistent behaviour.

I suspect that finalizers without access to the workspace are mostly useless (they only make sense for external system integration), so I would be in favour of such a proxy here as well. I'm interested in more opinions on this.

+1 on having proxy. In my head consistent sysmte beahaviour is above trying to be smart :)

mjudeikis · 2025-10-07T06:37:48Z

pkg/reconciler/core/logicalcluster/logicalcluster_reconcile_metadata.go

 	}

+	// add finalizers from the status as hashed labels
+	finalizerKeys := sets.New[string]()


is. the hashed thing here due to . in the name?

Main motivation was to keep it consistent with how initializers do it (see code block directly above this one).
I think the idea behind this was:

kcp/sdk/apis/tenancy/initialization/utils.go

Lines 87 to 89 in 459fa3b

// InitializerToLabel transforms an initializer into a key-value pair to add to a label set. We use a hash

// to create a unique identifier from this information, prefixing the hash in order to create a value which

// is unlikely to collide, and adding the full hash as a value in order to make it difficult to forge the pair.

mjudeikis · 2025-10-07T06:39:32Z

sdk/apis/core/v1alpha1/logicalcluster_types.go

+	// LogicalClusterFinalizerName attached to the owner of the LogicalCluster resource (usually a Workspace) so that we can control
 	// deletion of LogicalCluster resources.
-	LogicalClusterFinalizer = "core.kcp.io/logicalcluster"
+	LogicalClusterFinalizerName = "core.kcp.io/logicalcluster"


This could be separate PR and diff for this would be way way smaller.

for next time. lets leave it for now.

mjudeikis

looks close :)

mjudeikis · 2025-10-07T06:44:01Z

pkg/virtual/finalizingworkspaces/builder/build.go

+	withoutRootPathPrefix := strings.TrimPrefix(urlPath, rootPathPrefix)
+
+	// Incoming requests to this virtual workspace will look like:
+	//  /services/finalizingworkspace/<finalizer>/clusters/<something>/apis/workload.kcp.io/v1alpha1/synctargets


can we remove synctarget from example? :D

Ah good old copy pasting stuff. will change it :D

mjudeikis · 2025-10-07T06:45:41Z

pkg/virtual/finalizingworkspaces/doc.go

+//   - request finalization by a specific controller
+//
+// That is, a request for
+// GET /services/finalizingworkspaces/<finalizer>/clusters/*/apis/core.kcp.io/v1alpha1/logicalclusters


This would change if we go proxy way? I mean we would get access to the logical cluster via similar url?

Not directly. Initializers use 3 distinct virtual workspaces under the hood to handle this:

handles all the LIST requests (aka get clusters/*)

handles GET/PATCH requests to a single Logicalcluster (aka update clusters/asdfasdfa). I think this mainly exists so we can restrict which fields of a LogicalCluster a user can edit, because with 3., I think you can just do about anything you want with the objects

handles all other requests and proxies them straight through to kcp

But I agree that maybe this doc.go is written a bit misleading. I just copy pasted it from initializers and never looked back 😅. I'll update it to be a bit more precise once I have built virtualworkspace 3.

Let's leave this comment open for now.

mjudeikis · 2025-10-07T06:46:22Z

pkg/virtual/framework/forwardingregistry/wrappers.go

+
+// HasDeletionTimestamp returns whether a runtime.object has a deletion timestamp
+// wrapping the meta.Objects deletiontimestamp functionality.
+var HasDeletionTimestamp ObjectFilterFunc = func(ctx context.Context, obj runtime.Object) bool {


need to be public?

Oh yeah that is a good point, there is no big value in exposing this and it is only a helper func for WithDeletionTimestamp

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

- pkg/virtual/finalizingworkspaces - sdk/apis/tenancyfinalization - test/e2e/virtual/finalizingworkspaces On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

kcp-ci-bot · 2025-10-15T16:09:39Z

@SimonTheLeg: The following tests 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-unit	`6e24e0d`	link	true	`/test pull-kcp-test-unit`
pull-kcp-test-e2e-shared	`6e24e0d`	link	true	`/test pull-kcp-test-e2e-shared`
pull-kcp-test-e2e-sharded	`6e24e0d`	link	true	`/test pull-kcp-test-e2e-sharded`

Full PR test history

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/test-infra repository. I understand the commands that are listed here.

kcp-ci-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the DCO. labels Oct 1, 2025

SimonTheLeg added 4 commits October 1, 2025 17:32

refactor: allow for generation of multiple virtual workspace urls

83f6c38

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

Revert "refactor: allow for generation of multiple virtual workspace …

48bd1cb

…urls" This reverts commit 623580d.

post finalizingworkspace urls in workspacetype status

78c5dd9

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

SimonTheLeg force-pushed the finalizing-workspace branch from 564fb45 to 0eb0bc1 Compare October 1, 2025 16:03

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2025

SimonTheLeg force-pushed the finalizing-workspace branch 4 times, most recently from 0400091 to d46891c Compare October 4, 2025 14:57

kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 4, 2025

SimonTheLeg changed the title ~~WIP: add finalizer virtualworkspace~~ Add finalizing virtualworkspace Oct 4, 2025

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 Oct 4, 2025

embik self-requested a review October 6, 2025 08:28

SimonTheLeg force-pushed the finalizing-workspace branch from 0dc7d81 to c2ecefe Compare October 6, 2025 08:45

SimonTheLeg requested a review from mjudeikis October 6, 2025 09:00

correctly fetch objects in workspace deletion E2E test

6d71f01

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

SimonTheLeg force-pushed the finalizing-workspace branch from c2ecefe to 73b07b0 Compare October 6, 2025 09:08

mjudeikis reviewed Oct 7, 2025

View reviewed changes

add finalizer virtualworkspace

593e6e6

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

SimonTheLeg force-pushed the finalizing-workspace branch from 73b07b0 to 593e6e6 Compare October 14, 2025 10:20

SimonTheLeg mentioned this pull request Oct 15, 2025

feature: Allow accessing workspace content through terminating virtualworkspace #3647

Open

SimonTheLeg added 2 commits October 15, 2025 15:00

rename packages to terminator naming

1d163d0

- pkg/virtual/finalizingworkspaces - sdk/apis/tenancyfinalization - test/e2e/virtual/finalizingworkspaces On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

Move internal types from finalizers to terminators

6e24e0d

On-behalf-of: SAP <[email protected]> Signed-off-by: Simon Bein <[email protected]>

	// InitializerToLabel transforms an initializer into a key-value pair to add to a label set. We use a hash
	// to create a unique identifier from this information, prefixing the hash in order to create a value which
	// is unlikely to collide, and adding the full hash as a value in order to make it difficult to forge the pair.

Add finalizing virtualworkspace #3615

Are you sure you want to change the base?

Add finalizing virtualworkspace #3615

Conversation

SimonTheLeg commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Type of PR Is This?

Release Notes

Uh oh!

kcp-ci-bot commented Oct 1, 2025

Uh oh!

embik commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonTheLeg commented Oct 6, 2025

Uh oh!

mjudeikis commented Oct 7, 2025

Uh oh!

mjudeikis Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

SimonTheLeg Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SimonTheLeg commented Oct 1, 2025 •

edited

Loading

embik commented Oct 6, 2025 •

edited

Loading

mjudeikis Oct 7, 2025 •

edited

Loading

SimonTheLeg Oct 14, 2025 •

edited

Loading

kcp-ci-bot commented Oct 15, 2025 •

edited

Loading