Skip to content

Commit

Permalink
WithClusterScope: should not assign a default cluster name
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
p0lyn0mial committed Sep 15, 2022
1 parent f197112 commit d1b7b8b
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 12 deletions.
6 changes: 1 addition & 5 deletions pkg/server/filters/filters.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
kaudit "k8s.io/apiserver/pkg/audit"
"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
"k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/kubernetes/pkg/genericcontrolplane"
)

type (
Expand Down Expand Up @@ -120,9 +119,7 @@ func WithClusterScope(apiHandler http.Handler) http.HandlerFunc {
cluster.Wildcard = true
// fallthrough
cluster.Name = logicalcluster.Wildcard
case clusterName.Empty():
cluster.Name = genericcontrolplane.LocalAdminCluster
default:
case !clusterName.Empty():
if !reClusterName.MatchString(clusterName.String()) {
responsewriters.ErrorNegotiated(
apierrors.NewBadRequest(fmt.Sprintf("invalid cluster: %q does not match the regex", clusterName)),
Expand All @@ -134,7 +131,6 @@ func WithClusterScope(apiHandler http.Handler) http.HandlerFunc {
}

ctx := request.WithCluster(req.Context(), cluster)

apiHandler.ServeHTTP(w, req.WithContext(ctx))
}
}
Expand Down
8 changes: 5 additions & 3 deletions pkg/server/home_workspaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,11 @@ func (h *homeWorkspaceHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque

ctx := req.Context()
logger := klog.FromContext(ctx)
lcluster, err := request.ValidClusterFrom(ctx)
if err != nil {
responsewriters.InternalError(rw, req, err)
lcluster := request.ClusterFrom(req.Context())
if lcluster == nil {
// this is not a home workspace request
// just pass it to the next handler
h.apiHandler.ServeHTTP(rw, req)
return
}
logger = logging.WithCluster(logger, lcluster)
Expand Down
7 changes: 3 additions & 4 deletions pkg/server/home_workspaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1258,14 +1258,13 @@ func TestServeHTTP(t *testing.T) {
expectedResponseHeaders map[string]string
}{
{
testName: "Error when no cluster in context",
testName: "delegate to the next handler when no cluster in context",
contextUser: &kuser.DefaultInfo{Name: "user-1"},
contextRequestInfo: &request.RequestInfo{},
synced: true,

expectedStatusCode: 500,
expectedToDelegate: false,
expectedResponseBody: `Internal Server Error: "/dummy-target": no cluster in the request context - RequestInfo: &request.RequestInfo{IsResourceRequest:false, Path:"", Verb:"", APIPrefix:"", APIGroup:"", APIVersion:"", Namespace:"", Resource:"", Subresource:"", Name:"", Parts:[]string(nil)}`,
expectedStatusCode: 200,
expectedToDelegate: true,
},
{
testName: "Error when no user in context",
Expand Down
1 change: 1 addition & 0 deletions test/e2e/virtual/workspaces/virtual_workspace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,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)
virtualWorkspaceKubeConfig := clientcmdapi.Config{
Clusters: map[string]*clientcmdapi.Cluster{
"shard": &baseCluster,
Expand Down

0 comments on commit d1b7b8b

Please sign in to comment.