Clean up home workspaces

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

config/root/bootstrap.go

@@ -25,7 +25,6 @@ import (
 	"k8s.io/client-go/dynamic"
 
 	confighelpers "github.com/kcp-dev/kcp/config/helpers"
-	kcpclient "github.com/kcp-dev/kcp/pkg/client/clientset/versioned"
 )
 
 //go:embed *.yaml
@@ -34,13 +33,19 @@ var fs embed.FS
 // Bootstrap creates resources in this package by continuously retrying the list.
 // This is blocking, i.e. it only returns (with error) when the context is closed or with nil when
 // the bootstrapping is successfully completed.
-func Bootstrap(ctx context.Context, kcpClient kcpclient.Interface, rootDiscoveryClient discovery.DiscoveryInterface, rootDynamicClient dynamic.Interface, homeWorkspaceCreatorGroups []string, batteriesIncluded sets.String) error {
+func Bootstrap(
+	ctx context.Context,
+	rootDiscoveryClient discovery.DiscoveryInterface,
+	rootDynamicClient dynamic.Interface,
+	homeWorkspaceCreatorGroups []string,
+	batteriesIncluded sets.String,
+) error {
 	homeWorkspaceCreatorGroupReplacement := ""
 	for _, group := range homeWorkspaceCreatorGroups {
 		homeWorkspaceCreatorGroupReplacement += `
-- apiGroup: rbac.authorization.k8s.io
-  kind: Group
-  name: ` + group
+ - apiGroup: rbac.authorization.k8s.io
+   kind: Group
+   name: ` + group
 	}
 	if homeWorkspaceCreatorGroupReplacement == "" {
 		homeWorkspaceCreatorGroupReplacement = "[]"

pkg/server/config.go

@@ -391,10 +391,6 @@ func NewConfig(opts *kcpserveroptions.CompletedOptions) (*Config, error) {
 				c.KubeSharedInformerFactory,
 				c.KcpSharedInformerFactory,
 				c.GenericConfig.ExternalAddress,
-				opts.HomeWorkspaces.CreationDelaySeconds,
-				logicalcluster.NewPath(opts.HomeWorkspaces.HomeRootPrefix),
-				opts.HomeWorkspaces.BucketLevels,
-				opts.HomeWorkspaces.BucketSize,
 			)
 			if err != nil {
 				panic(err) // shouldn't happen due to flag validation

pkg/server/home_workspaces.go

@@ -88,25 +88,14 @@ func WithHomeWorkspaces(
 	kubeSharedInformerFactory kcpkubernetesinformers.SharedInformerFactory,
 	kcpSharedInformerFactory kcpinformers.SharedInformerFactory,
 	externalHost string,
-	creationDelaySeconds int,
-	homePrefix logicalcluster.Path,
-	bucketLevels,
-	bucketSize int,
 ) (http.Handler, error) {
-	if bucketLevels > 5 || bucketSize > 4 {
-		return nil, fmt.Errorf("bucketLevels and bucketSize must be <= 5 and <= 4")
-	}
 	h := &homeWorkspaceHandler{
 		delegate: apiHandler,
 
 		authz: a,
 
-		homePrefix:           homePrefix,
-		bucketLevels:         bucketLevels,
-		bucketSize:           bucketSize,
-		creationDelaySeconds: creationDelaySeconds,
-		creationTimeout:      time.Minute,
-		externalHost:         externalHost,
+		creationTimeout: time.Minute,
+		externalHost:    externalHost,
 
 		kcpClusterClient:  kcpClusterClient,
 		kubeClusterClient: kubeClusterClient,
@@ -133,11 +122,8 @@ type homeWorkspaceHandler struct {
 
 	authz authorizer.Authorizer
 
-	homePrefix               logicalcluster.Path
-	bucketLevels, bucketSize int
-	creationDelaySeconds     int
-	creationTimeout          time.Duration
-	externalHost             string
+	creationTimeout time.Duration
+	externalHost    string
 
 	transitiveTypeResolver workspacetypeexists.TransitiveTypeResolver
 
@@ -258,6 +244,8 @@ func (h *homeWorkspaceHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque
 		}
 	}
 
+	const retrySeconds = "1"
+
 	// here we have a LogicalCluster. Create ClusterRoleBinding. Again: if this is pre-existing
 	// and it is not belonging to the current user, the user will get a 403 through normal authorization.
 
@@ -291,7 +279,7 @@ func (h *homeWorkspaceHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque
 		logicalCluster, err = h.kcpClusterClient.Cluster(homeClusterName.Path()).CoreV1alpha1().LogicalClusters().UpdateStatus(ctx, logicalCluster, metav1.UpdateOptions{})
 		if err != nil {
 			if kerrors.IsConflict(err) {
-				rw.Header().Set("Retry-After", fmt.Sprintf("%d", h.creationDelaySeconds))
+				rw.Header().Set("Retry-After", retrySeconds)
 				http.Error(rw, "Creating the home workspace", http.StatusTooManyRequests)
 				return
 			}
@@ -306,7 +294,7 @@ func (h *homeWorkspaceHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque
 			return
 		}
 
-		rw.Header().Set("Retry-After", fmt.Sprintf("%d", h.creationDelaySeconds))
+		rw.Header().Set("Retry-After", retrySeconds)
 		http.Error(rw, "Creating the home workspace", http.StatusTooManyRequests)
 		return
 	}

pkg/server/options/flags.go

@@ -168,12 +168,8 @@ var (
 		"embedded-etcd-force-new-cluster",   // Starts a new cluster from existing data restored from a different system
 
 		// Home workspaces flags
-		"enable-home-workspaces",                 // Enable the Home Workspaces feature (enabled by default). Home workspaces allow a personal home workspace to provisioned on first access per-user. A user is cluster-admin inside his personal Home workspace.
-		"home-workspaces-creation-delay-seconds", // Delay, in seconds, before accessing the Home is retried after its automatic creation. This value is used when sending 'retry-after' responses to the Kubernetes client.
-		"home-workspaces-bucket-levels",          // Number of levels of bucket workspaces when bucketing home workspaces
-		"home-workspaces-bucket-size",            // Number of characters of bucket workspace names used when bucketing home workspaces
-		"home-workspaces-home-creator-groups",    // Groups of users who can have their home workspace created automatically create when first accessing it.
-		"home-workspaces-root-prefix",            // Logical cluster name of the workspace that will contains home workspaces for all workspaces.
+		"enable-home-workspaces",              // Enable the Home Workspaces feature (enabled by default). Home workspaces allow a personal home workspace to provisioned on first access per-user. A user is cluster-admin inside his personal Home workspace.
+		"home-workspaces-home-creator-groups", // Groups of users who can have their home workspaces provisioned upon first access.
 
 		// KCP Controllers flags
 		"auto-publish-apis",                      // If true, the APIs imported from physical clusters will be published automatically as CRDs

pkg/server/options/homeworkspaces.go

@@ -17,74 +17,28 @@ limitations under the License.
 package options
 
 import (
-	"fmt"
-
-	"github.com/kcp-dev/logicalcluster/v3"
 	"github.com/spf13/pflag"
 
 	"k8s.io/apiserver/pkg/authentication/user"
-
-	"github.com/kcp-dev/kcp/pkg/apis/core"
 )
 
 type HomeWorkspaces struct {
-	Enabled bool
-
-	CreationDelaySeconds int
-	BucketLevels         int
-	BucketSize           int
-
+	Enabled           bool
 	HomeCreatorGroups []string
-	HomeRootPrefix    string
 }
 
 func NewHomeWorkspaces() *HomeWorkspaces {
 	return &HomeWorkspaces{
-		Enabled:              true,
-		CreationDelaySeconds: 2,
-		BucketLevels:         2,
-		BucketSize:           2,
-		HomeCreatorGroups:    []string{user.AllAuthenticated},
-		HomeRootPrefix:       "root:users",
+		Enabled:           true,
+		HomeCreatorGroups: []string{user.AllAuthenticated},
 	}
 }
 
 func (hw *HomeWorkspaces) AddFlags(fs *pflag.FlagSet) {
-	fs.BoolVar(&hw.Enabled, "enable-home-workspaces", hw.Enabled, "Enable the Home Workspaces feature. Home workspaces allow a personal home workspace to provisioned on first access per-user. A user is cluster-admin inside his personal Home workspace.")
-	fs.IntVar(&hw.CreationDelaySeconds, "home-workspaces-creation-delay-seconds", hw.CreationDelaySeconds, "Delay, in seconds, before retrying accessing the Home workspace after its automatic creation. This value is used when sending 'retry-after' responses to the Kubernetes client.")
-	fs.IntVar(&hw.BucketLevels, "home-workspaces-bucket-levels", hw.BucketLevels, "Number of levels of bucket workspaces when bucketing home workspaces")
-	fs.IntVar(&hw.BucketLevels, "home-workspaces-bucket-size", hw.BucketSize, "Number of characters of bucket workspace names used when bucketing home workspaces")
-	fs.StringSliceVar(&hw.HomeCreatorGroups, "home-workspaces-home-creator-groups", hw.HomeCreatorGroups, "Groups of users who can have their home workspace created automatically create when first accessing it.")
-	fs.StringVar(&hw.HomeRootPrefix, "home-workspaces-root-prefix", hw.HomeRootPrefix, "Logical cluster name of the workspace that will contains home workspaces for all workspaces.")
-
-	fs.MarkDeprecated("home-workspaces-home-creator-groups", "This flag is deprecated and will be removed in a future release.")    //nolint:errcheck
-	fs.MarkDeprecated("home-workspaces-root-prefix", "This flag is deprecated and will be removed in a future release.")            //nolint:errcheck
-	fs.MarkDeprecated("home-workspaces-creation-delay-seconds", "This flag is deprecated and will be removed in a future release.") //nolint:errcheck
-	fs.MarkDeprecated("home-workspaces-bucket-levels", "This flag is deprecated and will be removed in a future release.")          //nolint:errcheck
-	fs.MarkDeprecated("home-workspaces-bucket-size", "This flag is deprecated and will be removed in a future release.")            //nolint:errcheck
+	fs.BoolVar(&hw.Enabled, "enable-home-workspaces", hw.Enabled, "Enable home workspaces, where a private workspace is provisioned upon first access per user, and the user is cluster-admin.")
+	fs.StringSliceVar(&hw.HomeCreatorGroups, "home-workspaces-home-creator-groups", hw.HomeCreatorGroups, "Groups of users who can have their home workspaces provisioned upon first access.")
 }
 
 func (hw *HomeWorkspaces) Validate() []error {
-	var errs []error
-
-	if hw.Enabled {
-		if hw.BucketLevels < 1 || hw.BucketLevels > 5 {
-			errs = append(errs, fmt.Errorf("--home-workspaces-bucket-levels should be between 1 and 5"))
-		}
-		if hw.BucketSize < 1 || hw.BucketLevels > 4 {
-			errs = append(errs, fmt.Errorf("--home-workspaces-bucket-size should be between 1 and 4"))
-		}
-		if hw.CreationDelaySeconds < 1 {
-			errs = append(errs, fmt.Errorf("--home-workspaces-creation-delay-seconds should be between 1"))
-		}
-		if homePrefix := logicalcluster.NewPath(hw.HomeRootPrefix); !homePrefix.IsValid() ||
-			homePrefix == logicalcluster.Wildcard ||
-			!homePrefix.HasPrefix(core.RootCluster.Path()) {
-			errs = append(errs, fmt.Errorf("--home-workspaces-root-prefix should be a valid logical cluster name"))
-		} else if parent, ok := homePrefix.Parent(); !ok || parent != core.RootCluster.Path() {
-			errs = append(errs, fmt.Errorf("--home-workspaces-root-prefix should be a direct child of the root logical cluster"))
-		}
-	}
-
-	return errs
+	return nil
 }

pkg/server/server.go

@@ -305,8 +305,8 @@ func (s *Server) Run(ctx context.Context) error {
 		if s.Options.Extra.ShardName == corev1alpha1.RootShard {
 			// the root ws is only present on the root shard
 			logger.Info("starting bootstrapping root workspace phase 1")
-			if err := configroot.Bootstrap(goContext(hookContext),
-				s.KcpClusterClient.Cluster(core.RootCluster.Path()),
+			if err := configroot.Bootstrap(
+				goContext(hookContext),
 				s.BootstrapApiExtensionsClusterClient.Cluster(core.RootCluster.Path()).Discovery(),
 				s.BootstrapDynamicClusterClient.Cluster(core.RootCluster.Path()),
 				s.Options.HomeWorkspaces.HomeCreatorGroups,

test/e2e/homeworkspaces/home_workspaces_test.go

@@ -18,20 +18,16 @@ package homeworkspaces
 
 import (
 	"context"
-	"path"
 	"testing"
 
-	kcpkubernetesclientset "github.com/kcp-dev/client-go/kubernetes"
 	"github.com/kcp-dev/logicalcluster/v3"
 	"github.com/stretchr/testify/require"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/rest"
 
-	virtualoptions "github.com/kcp-dev/kcp/cmd/virtual-workspaces/options"
 	"github.com/kcp-dev/kcp/pkg/apis/core"
 	corev1alpha1 "github.com/kcp-dev/kcp/pkg/apis/core/v1alpha1"
-	kcpclientset "github.com/kcp-dev/kcp/pkg/client/clientset/versioned"
 	kcpclusterclientset "github.com/kcp-dev/kcp/pkg/client/clientset/versioned/cluster"
 	"github.com/kcp-dev/kcp/test/e2e/framework"
 )
@@ -40,101 +36,42 @@ func TestUserHomeWorkspaces(t *testing.T) {
 	t.Parallel()
 	framework.Suite(t, "control-plane")
 
-	type clientInfo struct {
-		Token string
-	}
+	tokenAuthFile := framework.WriteTokenAuthFile(t)
+	serverArgs := framework.TestServerArgsWithTokenAuthFile(tokenAuthFile)
+	serverArgs = append(serverArgs, "--home-workspaces-home-creator-groups=team-1")
+	server := framework.PrivateKcpServer(t, framework.WithCustomArguments(serverArgs...))
 
-	type runningServer struct {
-		framework.RunningServer
-		kubeClusterClient             kcpkubernetesclientset.ClusterInterface
-		rootShardKcpClusterClient     kcpclusterclientset.ClusterInterface
-		kcpUserClusterClients         []kcpclusterclientset.ClusterInterface
-		virtualPersonalClusterClients []VirtualClusterClient
-	}
+	ctx, cancelFunc := context.WithCancel(context.Background())
+	t.Cleanup(cancelFunc)
 
-	var testCases = []struct {
-		name    string
-		kcpArgs []string
-		work    func(ctx context.Context, t *testing.T, server runningServer)
-	}{
-		{
-			name: "Create a workspace in the non-existing home and have it created automatically through ~",
-			kcpArgs: []string{
-				"--home-workspaces-home-creator-groups",
-				"team-1",
-			},
-			work: func(ctx context.Context, t *testing.T, server runningServer) {
-				t.Helper()
-
-				kcpUser1Client := server.kcpUserClusterClients[0]
-				kcpUser2Client := server.kcpUserClusterClients[1]
-
-				t.Logf("Get ~ Home workspace URL for user-1")
-				createdHome, err := kcpUser1Client.Cluster(core.RootCluster.Path()).TenancyV1alpha1().Workspaces().Get(ctx, "~", metav1.GetOptions{})
-				require.NoError(t, err, "user-1 should be able to get ~ workspace")
-				require.NotEqual(t, metav1.Time{}, createdHome.CreationTimestamp, "should have a creation timestamp, i.e. is not virtual")
-				require.Equal(t, corev1alpha1.LogicalClusterPhaseReady, createdHome.Status.Phase, "created home workspace should be ready")
-
-				t.Logf("Get the logical cluster inside user:user-1 (alias of ~)")
-				_, err = kcpUser1Client.Cluster(logicalcluster.NewPath("user:user-1")).CoreV1alpha1().LogicalClusters().Get(ctx, "cluster", metav1.GetOptions{})
-				require.NoError(t, err, "user-1 should be able to get a logical cluster in home workspace")
-
-				t.Logf("Get ~ Home workspace URL for user-2")
-				_, err = kcpUser2Client.Cluster(core.RootCluster.Path()).TenancyV1alpha1().Workspaces().Get(ctx, "~", metav1.GetOptions{})
-				require.EqualError(t, err, `workspaces.tenancy.kcp.io "~" is forbidden: User "user-2" cannot create resource "workspaces" in API group "tenancy.kcp.io" at the cluster scope: workspace access not permitted`, "user-2 should not be allowed to get his home workspace even before it exists")
-			},
-		},
-	}
+	kcpConfig := server.BaseConfig(t)
 
-	for _, testCase := range testCases {
-		testCase := testCase
-		t.Run(testCase.name, func(t *testing.T) {
-			t.Parallel()
-			tokenAuthFile := framework.WriteTokenAuthFile(t)
-			server := framework.PrivateKcpServer(t, framework.WithCustomArguments(append(framework.TestServerArgsWithTokenAuthFile(tokenAuthFile), testCase.kcpArgs...)...))
-			ctx, cancelFunc := context.WithCancel(context.Background())
-			t.Cleanup(cancelFunc)
-
-			// create non-virtual clients
-			kcpConfig := server.BaseConfig(t)
-			rootShardCfg := server.RootShardSystemMasterBaseConfig(t)
-			kubeClusterClient, err := kcpkubernetesclientset.NewForConfig(kcpConfig)
-			require.NoError(t, err, "failed to construct client for server")
-			rootShardKcpClusterClient, err := kcpclusterclientset.NewForConfig(rootShardCfg)
-			require.NoError(t, err, "failed to construct client for server")
-
-			// create kcp client and virtual clients for all users requested
-			var kcpUserClusterClients []kcpclusterclientset.ClusterInterface
-			var virtualPersonalClusterClients []VirtualClusterClient
-			for _, ci := range []clientInfo{{Token: "user-1-token"}, {Token: "user-2-token"}} {
-				userConfig := framework.ConfigWithToken(ci.Token, rest.CopyConfig(kcpConfig))
-				virtualPersonalClusterClients = append(virtualPersonalClusterClients, &virtualClusterClient{config: userConfig})
-				kcpUserClusterClient, err := kcpclusterclientset.NewForConfig(userConfig)
-				require.NoError(t, err)
-				kcpUserClusterClients = append(kcpUserClusterClients, kcpUserClusterClient)
-			}
-
-			testCase.work(ctx, t, runningServer{
-				RunningServer:                 server,
-				kubeClusterClient:             kubeClusterClient,
-				rootShardKcpClusterClient:     rootShardKcpClusterClient,
-				kcpUserClusterClients:         kcpUserClusterClients,
-				virtualPersonalClusterClients: virtualPersonalClusterClients,
-			})
-		})
+	clientFor := func(token string) kcpclusterclientset.ClusterInterface {
+		userConfig := framework.ConfigWithToken(token, rest.CopyConfig(kcpConfig))
+		client, err := kcpclusterclientset.NewForConfig(userConfig)
+		require.NoError(t, err)
+		return client
 	}
-}
-
-type VirtualClusterClient interface {
-	Cluster(cluster logicalcluster.Path) kcpclientset.Interface
-}
-
-type virtualClusterClient struct {
-	config *rest.Config
-}
 
-func (c *virtualClusterClient) Cluster(cluster logicalcluster.Path) kcpclientset.Interface {
-	config := rest.CopyConfig(c.config)
-	config.Host += path.Join(virtualoptions.DefaultRootPathPrefix, "workspaces", cluster.String())
-	return kcpclientset.NewForConfigOrDie(config)
+	kcpUser1Client := clientFor("user-1-token")
+	kcpUser2Client := clientFor("user-2-token")
+
+	t.Logf("Get ~ Home workspace URL for user-1")
+	user1Home, err := kcpUser1Client.Cluster(core.RootCluster.Path()).TenancyV1alpha1().Workspaces().Get(ctx, "~", metav1.GetOptions{})
+	require.NoError(t, err, "user-1 should be able to get ~ workspace")
+	require.NotEqual(t, metav1.Time{}, user1Home.CreationTimestamp, "should have a creation timestamp, i.e. is not virtual")
+	require.Equal(t, corev1alpha1.LogicalClusterPhaseReady, user1Home.Status.Phase, "created home workspace should be ready")
+
+	t.Logf("Get the logical cluster inside user:user-1 (alias of ~)")
+	_, err = kcpUser1Client.Cluster(logicalcluster.NewPath("user:user-1")).CoreV1alpha1().LogicalClusters().Get(ctx, "cluster", metav1.GetOptions{})
+	require.NoError(t, err, "user-1 should be able to get a logical cluster in home workspace")
+
+	t.Logf("Get ~ Home workspace URL for user-2")
+	_, err = kcpUser2Client.Cluster(core.RootCluster.Path()).TenancyV1alpha1().Workspaces().Get(ctx, "~", metav1.GetOptions{})
+	require.EqualError(
+		t,
+		err,
+		`workspaces.tenancy.kcp.io "~" is forbidden: User "user-2" cannot create resource "workspaces" in API group "tenancy.kcp.io" at the cluster scope: workspace access not permitted`,
+		"user-2 should not forbidden from having a home workspace auto-created",
+	)
 }