✨ Add DynamicRESTMapper #3316

gman0 · 2025-02-28T16:11:44Z

Summary

This PR adds DynamicRESTMapper. See the linked issue for details.

Related issue(s)

Release Notes

NONE

kcp-ci-bot · 2025-02-28T16:11:50Z

[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 sttts 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:

OWNERS

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 · 2025-02-28T16:11:50Z

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

sttts · 2025-03-01T18:47:42Z

pkg/dynamicrestmapper/dynamicrestmapper.go

+	pluralToSingular     map[schema.GroupVersionResource]schema.GroupVersionResource
+
+	// Protects all DynamicRESTMapper's mappings.
+	mapperLock sync.RWMutex


usual convention: put the lock just above (without empty lines) what it protects.

sttts · 2025-03-01T18:52:39Z

pkg/dynamicrestmapper/dynamicrestmapper.go

+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+type DynamicRESTMapper struct {


not sure where this is going. The data structures below are just the DefaultRESTMapper ones, right?

What we need is that for every logical cluster in the system, i.e. something like map[logicalcluster.Name]DefaultRESTMapper.

As discussed async, this needs to comply with the DefaultRestMapper interface but be logicalcluster compabile so every process in the shard could resolve particular cluster mappings. And as inspiration index https://github.com/kcp-dev/kcp/blob/main/pkg/index/index.go#L79 code could be referred.

Basically what sttts said :)

I was thinking something under lines:

m := mapper.For(logicalcluster) or mapper.For(logicalcluster). KindFor()

which is basically same. Not sure if we want "global restmapper per shard at this point".

Thanks for the examples! It would be great if we could use DefaultRESTMapper as-is, but if I understood correctly, we'll need to delete the mappings too (when a CRD is removed), and since DefaultRESTMapper doesn't implement deletion, we'd need to copy-paste all its code first :/

"global restmapper per shard at this point".

This is only for the local shard.

and since DefaultRESTMapper doesn't implement deletion, we'd need to copy-paste all its code first :/

The existing code is pretty complex. Wondering how we can soft-fork it. I could see that with one additional constructor and some wrapping with locking we could reuse all the existing code, while being able to change the involved maps.

Thinking out loud:

soft-fork = maybe copy over through go generate and add a removal method in another file.

sttts · 2025-03-01T18:55:17Z

Sorry for early review. This somehow hit my timeline 😀 I figure this is super early. Let me know when it's ready for a real review.

gman0 · 2025-03-06T17:34:54Z

Commits more or less clean, and mapping for APIBindings, CRDs and built-in types should work. We're still missing tests though so I'm keeping this in Draft still.

@sttts I've added the generator and a wrapper around the (generated) forked version of DefaultRESTMapper as you suggested.

mjudeikis · 2025-03-16T16:44:43Z

pkg/server/server.go

@@ -178,6 +180,8 @@ func NewServer(c CompletedConfig) (*Server, error) {
 		}
 	}

+	s.DynRESTMapper = dynamicrestmapper.NewDynamicRESTMapper(nil)


So this would be global mapper every controller can pull In? Can we add a comment here tell how this can be used?

Added a comment. Something like that?

mjudeikis · 2025-03-16T16:46:21Z

hack/gen-patch-defaultrestmapper.sh

@@ -0,0 +1,122 @@
+#!/usr/bin/env bash


Is this now part of the rebathe se procedure? Do you think we should add it to the docs?

The script is now run as a part of make codegen (5d2cf25) ; that way we don't need to rely on docs.

In general it really should be in the docs too, but the rebase docs are missing a lot of things. I can open a ticket and work on updating them, and mention the need to run the various generators we have. Would that be ok?

gman0 · 2025-03-26T23:54:49Z

I've added unit tests and I feel like the code is ready for a review now. PTAL, thank you!

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

We need to fork upstream's DefaultRESTMapper to be able to update and delete its contents dynamically. The script added in this commit does it automatically. On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

Ran: hack/gen-patch-defaultrestmapper.sh Generated from k8s.io/apimachinery v0.31.6. On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

gman0 · 2025-03-27T22:27:09Z

/retest all

kcp-ci-bot · 2025-03-27T22:27:14Z

@gman0: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/build-image
/test pull-kcp-build-image
/test pull-kcp-lint
/test pull-kcp-test-e2e
/test pull-kcp-test-e2e-multiple-runs
/test pull-kcp-test-e2e-sharded
/test pull-kcp-test-e2e-shared
/test pull-kcp-test-unit
/test pull-kcp-validate-prow-yaml
/test pull-kcp-verify
/test pull-kcp-verify-codegen

Use /test all to run the following jobs that were automatically triggered:

pull-kcp-build-image
pull-kcp-lint
pull-kcp-test-e2e
pull-kcp-test-e2e-multiple-runs
pull-kcp-test-e2e-sharded
pull-kcp-test-e2e-shared
pull-kcp-test-unit
pull-kcp-validate-prow-yaml
pull-kcp-verify
pull-kcp-verify-codegen

In response to this:

/retest all

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.

gman0 · 2025-03-27T22:27:36Z

/test all

gman0 · 2025-03-28T10:13:02Z

The CI is failing on following error:

    apibinding_test.go:411: === Verify that root:e2e-workspace-hlqv5:service-provider-2|today-cowboys export virtual workspace shows cowboys
    apibinding_test.go:423: Listing root:e2e-workspace-hlqv5:service-provider-2|today-cowboys cowboys via virtual workspace https://172.25.201.3:39095/services/apiexport/1xc6abs4fb4d7dfv/today-cowboys/clusters/1itk6s9i3nrdmiak
    apibinding_test.go:427: Error: cowboys.wildwest.dev is forbidden: User "kcp-admin" cannot list resource "cowboys" in API group "wildwest.dev" at the cluster scope: access denied
    apibinding_test.go:439: 
        	Error Trace:	/home/prow/go/src/github.com/kcp-dev/kcp/test/e2e/apibinding/apibinding_test.go:439
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestAPIBinding
        	Messages:   	cowboys not found exactly on one shard, but on 0/1
    fixture.go:140: Stopping kcp servers...
    fixture.go:145: Gathering metrics for kcp server shared
    fixture.go:294: cleanup: canceling context
    fixture.go:298: cleanup: waiting for shutdownComplete
    fixture.go:302: cleanup: received shutdownComplete

DONE 302 tests, 4 skipped, 1 failure in 1349.186s
make: *** [Makefile:292: test-e2e] Error 1

I ran the test locally without this PR (just the main branch). In a loop, after several attempts I received the same error:

$ while true ; go test -p 1 -v  ./test/e2e/apibinding/...  --kcp-kubeconfig=$(pwd)/.kcp/admin.kubeconfig --shard-kubeconfigs=root=$(pwd)/.kcp-0/admin.kubeconfig --shard-kubeconfigs=shard-1=$(pwd)/.kcp-1/admin.kubeconfig --run='^TestAPIBinding$' || break ; end

...

    apibinding_test.go:411: === Verify that root:e2e-workspace-7rpcx:service-provider-2|today-cowboys export virtual workspace shows cowboys
    apibinding_test.go:423: Listing root:e2e-workspace-7rpcx:service-provider-2|today-cowboys cowboys via virtual workspace https://192.168.32.7:7444/services/apiexport/1o06cd9vyon800tr/today-cowboys/clusters/1tgc94zwx6bymqns
    apibinding_test.go:427: Error: cowboys.wildwest.dev is forbidden: User "kcp-admin" cannot list resource "cowboys" in API group "wildwest.dev" at the cluster scope: access denied
    apibinding_test.go:423: Listing root:e2e-workspace-7rpcx:service-provider-2|today-cowboys cowboys via virtual workspace https://192.168.32.7:7445/services/apiexport/1o06cd9vyon800tr/today-cowboys/clusters/1tgc94zwx6bymqns
    apibinding_test.go:427: Error: cowboys.wildwest.dev is forbidden: User "kcp-admin" cannot list resource "cowboys" in API group "wildwest.dev" at the cluster scope: access denied
    apibinding_test.go:439:
        	Error Trace:	/tmp/ws/kcp/test/e2e/apibinding/apibinding_test.go:439
        	Error:      	Not equal:
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestAPIBinding
        	Messages:   	cowboys not found exactly on one shard, but on 0/2
--- FAIL: TestAPIBinding (28.93s)
FAIL
FAIL	github.com/kcp-dev/kcp/test/e2e/apibinding	29.035s
FAIL

Is this a test known to be flaky? In any case, it doesn't seem to be caused by this PR.

gman0 · 2025-03-28T10:13:31Z

/test pull-kcp-test-e2e-multiple-runs

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

kcp-ci-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 28, 2025

sttts reviewed Mar 1, 2025

View reviewed changes

gman0 force-pushed the dyn-restmapper branch from b3c4183 to 44af52a Compare March 4, 2025 15:24

gman0 force-pushed the dyn-restmapper branch from da8429d to 4b1bd1b Compare March 6, 2025 17:30

mjudeikis reviewed Mar 16, 2025

View reviewed changes

gman0 force-pushed the dyn-restmapper branch from 4b1bd1b to 7af94b0 Compare March 26, 2025 23:46

gman0 marked this pull request as ready for review March 26, 2025 23:53

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 Mar 26, 2025

gman0 force-pushed the dyn-restmapper branch from 7af94b0 to 247cc03 Compare March 27, 2025 12:19

gman0 changed the title ~~✨ Add DynamicRESTMapper~~ ✨ Add DynamicRESTMapper Mar 27, 2025

gman0 added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 27, 2025

gman0 force-pushed the dyn-restmapper branch 2 times, most recently from 4e79e56 to b1bd7ef Compare March 27, 2025 20:33

gman0 added 3 commits March 27, 2025 23:23

Added DynamicRESTMapper

06102ac

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

Wire DynamicRESTMapper into pkg/server

25d8d92

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

Added code generator for DefaultRESTMapper

4ca53be

We need to fork upstream's DefaultRESTMapper to be able to update and delete its contents dynamically. The script added in this commit does it automatically. On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

gman0 added 4 commits March 27, 2025 23:24

Generated pkg/dynamicrestmapper/defaultrestmapper_patch.go

b52e4f6

Ran: hack/gen-patch-defaultrestmapper.sh Generated from k8s.io/apimachinery v0.31.6. On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

Makefile: run gen-patch-defaultrestmapper.sh in codegen target

41ee727

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

Added a unit test for DynamicRESTMapper

6473438

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

Install goimports into hack/tools

b672c0d

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

gman0 force-pushed the dyn-restmapper branch from b1bd7ef to b672c0d Compare March 27, 2025 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add DynamicRESTMapper #3316

✨ Add DynamicRESTMapper #3316

gman0 commented Feb 28, 2025

kcp-ci-bot commented Feb 28, 2025

kcp-ci-bot commented Feb 28, 2025

sttts Mar 1, 2025

sttts Mar 1, 2025

mjudeikis Mar 2, 2025

mjudeikis Mar 2, 2025

gman0 Mar 3, 2025 •

edited

Loading

sttts Mar 4, 2025

sttts Mar 4, 2025

sttts commented Mar 1, 2025

gman0 commented Mar 6, 2025 •

edited

Loading

mjudeikis Mar 16, 2025

gman0 Mar 26, 2025

mjudeikis Mar 16, 2025

gman0 Mar 26, 2025 •

edited

Loading

gman0 commented Mar 26, 2025

gman0 commented Mar 27, 2025

kcp-ci-bot commented Mar 27, 2025

gman0 commented Mar 27, 2025

gman0 commented Mar 28, 2025

gman0 commented Mar 28, 2025

✨ Add DynamicRESTMapper #3316

Are you sure you want to change the base?

✨ Add DynamicRESTMapper #3316

Conversation

gman0 commented Feb 28, 2025

Summary

Related issue(s)

Release Notes

kcp-ci-bot commented Feb 28, 2025

kcp-ci-bot commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gman0 Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Mar 1, 2025

gman0 commented Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gman0 Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

gman0 commented Mar 26, 2025

gman0 commented Mar 27, 2025

kcp-ci-bot commented Mar 27, 2025

gman0 commented Mar 27, 2025

gman0 commented Mar 28, 2025

gman0 commented Mar 28, 2025

gman0 Mar 3, 2025 •

edited

Loading

gman0 commented Mar 6, 2025 •

edited

Loading

gman0 Mar 26, 2025 •

edited

Loading