🌱 WithClusterScope: should not assign a default cluster name #1871

p0lyn0mial · 2022-08-31T15:25:20Z

Summary

clients should specify a cluster name before storing something in the db as that leads to more readable code.
the server should not assign a default cluster name.

at the moment requests without a cluster name are rejected by the storage layer.

Things to consider:
reject a request without a name in the filter
will this break some external clients?

Related issue(s)

Fixes #

p0lyn0mial · 2022-08-31T15:25:38Z

/assign @stevekuznetsov @sttts

stevekuznetsov

This is one of those super ancient decisions that I don't actually have any memory for why we did it. Hard to think through the implications.

@davidfestal thoughts?

ncdc · 2022-08-31T21:31:24Z

IIRC it was to get "unaware" clients (e.g. kube internals) to succeed in their work (e.g. to create default RBAC resources) in some logical cluster.

I'm in favor of proceeding to remove the defaulting and trying to fix any remaining clients that were relying on this default.

p0lyn0mial · 2022-09-01T06:01:46Z

/retest

p0lyn0mial · 2022-09-01T10:21:23Z

IIRC it was to get "unaware" clients (e.g. kube internals) to succeed in their work (e.g. to create default RBAC resources) in some logical cluster.

I think it was achieved by providing multiClusterClientConfigRoundTripper.

ncdc · 2022-09-01T13:06:51Z

E0901 06:16:38.200882   38061 errors.go:77] cluster name is empty in the request context - RequestInfo: &request.RequestInfo{IsResourceRequest:false, Path:"/livez", Verb:"get", APIPrefix:"", APIGroup:"", APIVersion:"", Namespace:"", Resource:"", Subresource:"", Name:"", Parts:[]string(nil)}

stevekuznetsov · 2022-09-01T15:55:51Z

(and we've since learned that of course unaware clients doing "something" without being multi-cluster-aware just do nothing important whatsoever in the root logical cluster ... and no importance in supporting that)

p0lyn0mial · 2022-09-02T13:39:58Z

pkg/server/home_workspaces.go

@@ -272,7 +272,7 @@ func (h *homeWorkspaceHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque
 	logger := klog.FromContext(ctx)
 	lcluster, err := request.ValidClusterFrom(ctx)
 	if err != nil {
-		responsewriters.InternalError(rw, req, err)
+		h.apiHandler.ServeHTTP(rw, req)


it looks like the home-ws is first in the chain, capturing healthz/readyz as well.

p0lyn0mial · 2022-09-05T10:28:39Z

I almost have it, a few unit tests require fixing.

p0lyn0mial · 2022-09-05T10:29:53Z

test/e2e/virtual/workspaces/virtual_workspace_test.go

@@ -584,6 +584,7 @@ func testWorkspacesVirtualWorkspaces(t *testing.T, standalone bool) {
 		// write kubeconfig to disk, next to kcp kubeconfig
 		kcpAdminConfig, _ := server.RawConfig()
 		var baseCluster = *kcpAdminConfig.Clusters["base"] // shallow copy
+		baseCluster.Server = fmt.Sprintf("%s/clusters/system:admin", baseCluster.Server)


alternatively, we could add something to a handler but having a cluster name here is much cleaner.

sttts · 2022-09-09T12:29:18Z

How is this going? Any blocker?

p0lyn0mial · 2022-09-12T07:14:51Z

How is this going? Any blocker?

I need to fix a few failing unit tests and this is ready to go.

p0lyn0mial · 2022-09-14T15:10:25Z

pkg/server/home_workspaces.go

-	lcluster, err := request.ValidClusterFrom(ctx)
-	if err != nil {
-		responsewriters.InternalError(rw, req, err)
+	lcluster := request.ClusterFrom(req.Context())


I'll follow up with a PR that removes the usage of ValidClusterFrom from the authorizers.

I think that we should also refactor our fork and use only ValidClusterFrom on the storage layer.

p0lyn0mial · 2022-09-14T15:42:26Z

okay ci/prow/test is green, PTAL.

ncdc · 2022-09-14T15:59:31Z

pkg/server/filters/filters.go

@@ -121,7 +120,7 @@ func WithClusterScope(apiHandler http.Handler) http.HandlerFunc {
 			// fallthrough
 			cluster.Name = logicalcluster.Wildcard
 		case clusterName.Empty():
-			cluster.Name = genericcontrolplane.LocalAdminCluster


if we get rid of cluster.Wildcard, i don't even think we need the logic in here. instead:

if clusterName != logicalcluster.Wildcard { // do regex check } cluster.Name = clusterName

can do in a follow-up if you want

Didn't know we were planning to get rid of cluster.Wildcard. I can do that in a follow-up.
Meanwhile, I removed the case when the clusterName was empty. We will make use of the default value.

It has no value any more, since you can compare against logicalcluster.Wildcard 😄

Yeah I think the same arguments used to not have a .Wildcard for shard name apply here

clients should specify a cluster name before storing something in the db as that leads to more readable code. the server should not assing a default cluster name. at the moment requests without a cluster name are rejected by the storage layer.

ncdc · 2022-09-15T13:33:31Z

pkg/server/filters/filters.go

@@ -121,7 +120,7 @@ func WithClusterScope(apiHandler http.Handler) http.HandlerFunc {
 			// fallthrough
 			cluster.Name = logicalcluster.Wildcard
 		case clusterName.Empty():
-			cluster.Name = genericcontrolplane.LocalAdminCluster


It has no value any more, since you can compare against logicalcluster.Wildcard 😄

openshift-ci · 2022-09-15T16:51:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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

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

openshift-ci bot requested review from davidfestal and sttts August 31, 2022 15:25

openshift-ci bot assigned stevekuznetsov and sttts Aug 31, 2022

stevekuznetsov reviewed Aug 31, 2022

View reviewed changes

p0lyn0mial force-pushed the with-cluster-scope-no-default-value branch from f9f3c01 to 2f9b592 Compare September 2, 2022 13:37

p0lyn0mial commented Sep 2, 2022

View reviewed changes

p0lyn0mial force-pushed the with-cluster-scope-no-default-value branch from 2f9b592 to 402f45f Compare September 5, 2022 09:04

p0lyn0mial commented Sep 5, 2022

View reviewed changes

ncdc changed the title ~~WithClusterScope: should not assign a default cluster name~~ 🌱 WithClusterScope: should not assign a default cluster name Sep 9, 2022

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

p0lyn0mial force-pushed the with-cluster-scope-no-default-value branch from 402f45f to 637e428 Compare September 14, 2022 15:09

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

p0lyn0mial commented Sep 14, 2022

View reviewed changes

ncdc reviewed Sep 14, 2022

View reviewed changes

p0lyn0mial force-pushed the with-cluster-scope-no-default-value branch from 637e428 to 5f9bfa5 Compare September 15, 2022 08:23

p0lyn0mial force-pushed the with-cluster-scope-no-default-value branch from 5f9bfa5 to d1b7b8b Compare September 15, 2022 09:05

p0lyn0mial mentioned this pull request Sep 15, 2022

🌱 authorisation: stop using the ValidClusterFrom method #1996

Merged

ncdc approved these changes Sep 15, 2022

View reviewed changes

openshift-ci bot assigned ncdc Sep 15, 2022

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

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

openshift-merge-robot merged commit 8ccf2c4 into kcp-dev:main Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 WithClusterScope: should not assign a default cluster name #1871

🌱 WithClusterScope: should not assign a default cluster name #1871

p0lyn0mial commented Aug 31, 2022

p0lyn0mial commented Aug 31, 2022

stevekuznetsov left a comment

ncdc commented Aug 31, 2022

p0lyn0mial commented Sep 1, 2022

p0lyn0mial commented Sep 1, 2022

ncdc commented Sep 1, 2022

stevekuznetsov commented Sep 1, 2022

p0lyn0mial Sep 2, 2022

p0lyn0mial commented Sep 5, 2022

p0lyn0mial Sep 5, 2022

sttts commented Sep 9, 2022

p0lyn0mial commented Sep 12, 2022

p0lyn0mial Sep 14, 2022 •

edited

Loading

p0lyn0mial commented Sep 14, 2022

ncdc Sep 14, 2022

ncdc Sep 14, 2022

p0lyn0mial Sep 15, 2022

ncdc Sep 15, 2022

stevekuznetsov Sep 15, 2022

ncdc Sep 15, 2022

openshift-ci bot commented Sep 15, 2022

🌱 WithClusterScope: should not assign a default cluster name #1871

🌱 WithClusterScope: should not assign a default cluster name #1871

Conversation

p0lyn0mial commented Aug 31, 2022

Summary

Related issue(s)

p0lyn0mial commented Aug 31, 2022

stevekuznetsov left a comment

Choose a reason for hiding this comment

ncdc commented Aug 31, 2022

p0lyn0mial commented Sep 1, 2022

p0lyn0mial commented Sep 1, 2022

ncdc commented Sep 1, 2022

stevekuznetsov commented Sep 1, 2022

Choose a reason for hiding this comment

p0lyn0mial commented Sep 5, 2022

Choose a reason for hiding this comment

sttts commented Sep 9, 2022

p0lyn0mial commented Sep 12, 2022

p0lyn0mial Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

p0lyn0mial commented Sep 14, 2022

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

Choose a reason for hiding this comment

openshift-ci bot commented Sep 15, 2022

p0lyn0mial Sep 14, 2022 •

edited

Loading