🐛 Fix double identities for wildcard requests from APIExport virtual workspace #2306

ncdc · 2022-11-04T18:05:07Z

Summary

Fix a bug where wildcard requests coming from the APIExport virtual workspace for "built-in" kcp types (e.g. ClusterWorkspaceTypes) had the identity set by the virtual workspace, then set a second time by decorateWildcardPathsWithResourceIdentities.

Related issue(s)

Fixes #2184
Fixes #2308
Incorporates & supersedes #2245

ncdc · 2022-11-04T18:13:53Z

/hold
need to fix something

ncdc · 2022-11-04T18:21:21Z

/unhold

sttts · 2022-11-04T18:34:00Z

Wait.

sttts · 2022-11-04T18:34:25Z

No decorateWildcardPathsWithResourceIdentities client should be used to forward requests from a VW.

/hold

ncdc · 2022-11-07T20:26:18Z

Updated to change the VW server to use a "clean" client that doesn't inject identities. Had to reimplement similar logic in the initializing workspaces vw. PTAL. Happy to adjust as needed.

sttts · 2022-11-08T13:02:16Z

pkg/server/bootstrap/identity.go

+	// It's possible the incoming request already has an identity specified. Make sure we exclude that when
+	// determining the resource in question.
+	parts := strings.SplitN(comps[5], ":", 2)
+	if len(parts) == 0 {


can this ever happen?

Probably not, but can't hurt to have the code here?

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

1. Look up the root:tenancy.kcp.dev APIExport identity and use that when forwarding requests for clusterworkspaces from the initializingworkspaces service to kcp. 2. Fix security issue that exposed all workspaces regardless of phase and initializer for GET/UPDATE for a single clusterworkspace Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

ncdc · 2022-11-30T19:28:41Z

/unhold

stevekuznetsov · 2022-12-01T13:45:05Z

pkg/virtual/framework/forwardingregistry/rest.go

 		factory, listFactory, destroyer,
 		strategy, tableConvertor,
 		resource, apiExportIdentityHash, categories,
 		dynamicClusterClient, []string{}, *patchConflictRetryBackoff, ctx.Done(),
 	)
-	store := wrapper(resource.GroupResource(), delegate)
+	if wrapper != nil {


You have an inherent nil check in the implementation of Decorate() - do we need it here?

stevekuznetsov · 2022-12-01T13:45:39Z

pkg/virtual/framework/forwardingregistry/rest_test.go

-		func(_ schema.GroupResource, store *forwardingregistry.StoreFuncs) *forwardingregistry.StoreFuncs {
-			return store
-		})
+		forwardingregistry.StorageWrapperFunc(func(_ schema.GroupResource, store *forwardingregistry.StoreFuncs) {


You have an inherent nil check - can we just omit this?

If we keep the nil check in NewStorage, yes

stevekuznetsov

Some small comments, otherwise LGTM

stevekuznetsov · 2022-12-01T13:45:49Z

pkg/virtual/framework/forwardingregistry/rest.go

 		// We are explicitly setting forceAllowCreate to false in the call to the underlying storage because
 		// subresources should never allow create on update.
 		return delegateUpdate(ctx, name, objInfo, createValidation, updateValidation, false, options)
 	}
-	statusStore := wrapper(resource.GroupResource(), statusDelegate)
+	if wrapper != nil {


You have an inherent nil check in the implementation of Decorate() - do we need it here?

I would keep it - you never know if some other implementation omits the nil check.

stevekuznetsov · 2022-12-01T13:47:39Z

pkg/virtual/initializingworkspaces/builder/forwarding.go

 			ctx,
-			resource,
-			"", // ClusterWorkspaces have no identity


stevekuznetsov · 2022-12-01T14:00:09Z

/lgtm
/approve

openshift-ci · 2022-12-01T14:00:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov

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~~ [stevekuznetsov]

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

ncdc mentioned this pull request Nov 4, 2022

🐛 Fix url double decoration #2245

Closed

openshift-ci bot requested review from csams and davidfestal November 4, 2022 18:05

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2022

ncdc force-pushed the fix-double-identities branch from 61d279e to 9205f31 Compare November 4, 2022 18:21

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2022

ncdc force-pushed the fix-double-identities branch from 9205f31 to bb3c15a Compare November 7, 2022 20:24

sttts reviewed Nov 8, 2022

View reviewed changes

ncdc force-pushed the fix-double-identities branch from bb3c15a to 7916b6d Compare November 8, 2022 13:17

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2022

mjudeikis and others added 3 commits November 29, 2022 16:07

fix url decoration

b3449ef

add test for double decoration

1476697

Ensure users can't override identity for built-in kcp types

2ff4ea5

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

ncdc force-pushed the fix-double-identities branch from 7916b6d to 635489a Compare November 29, 2022 21:31

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2022

ncdc force-pushed the fix-double-identities branch from 635489a to 1f9ce1b Compare November 29, 2022 21:41

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2022

stevekuznetsov reviewed Dec 1, 2022

View reviewed changes

stevekuznetsov suggested changes Dec 1, 2022

View reviewed changes

openshift-ci bot assigned stevekuznetsov Dec 1, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2022

openshift-merge-robot merged commit f5865fc into kcp-dev:main Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix double identities for wildcard requests from APIExport virtual workspace #2306

🐛 Fix double identities for wildcard requests from APIExport virtual workspace #2306

ncdc commented Nov 4, 2022 •

edited

ncdc commented Nov 4, 2022

ncdc commented Nov 4, 2022

sttts commented Nov 4, 2022

sttts commented Nov 4, 2022

ncdc commented Nov 7, 2022

sttts Nov 8, 2022

ncdc Nov 8, 2022

ncdc commented Nov 30, 2022

stevekuznetsov Dec 1, 2022

stevekuznetsov Dec 1, 2022

ncdc Dec 1, 2022

stevekuznetsov left a comment

stevekuznetsov Dec 1, 2022

ncdc Dec 1, 2022

stevekuznetsov Dec 1, 2022

stevekuznetsov Dec 1, 2022

stevekuznetsov commented Dec 1, 2022

openshift-ci bot commented Dec 1, 2022

🐛 Fix double identities for wildcard requests from APIExport virtual workspace #2306

🐛 Fix double identities for wildcard requests from APIExport virtual workspace #2306

Conversation

ncdc commented Nov 4, 2022 • edited

Summary

Related issue(s)

ncdc commented Nov 4, 2022

ncdc commented Nov 4, 2022

sttts commented Nov 4, 2022

sttts commented Nov 4, 2022

ncdc commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov left a comment

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

Choose a reason for hiding this comment

stevekuznetsov commented Dec 1, 2022

openshift-ci bot commented Dec 1, 2022

ncdc commented Nov 4, 2022 •

edited