Review comments

mjudeikis · mjudeikis · commit 0542a4231bf4 · 2025-04-21T14:16:20.000+03:00
diff --git a/config/crds/tenancy.kcp.io_workspaces.yaml b/config/crds/tenancy.kcp.io_workspaces.yaml
@@ -48,6 +48,10 @@ spec:
           depending on access pattern. All workspace implementations share the characteristic
           that the URL that serves a given workspace can be used with standard Kubernetes
           API machinery and client libraries and command line tools.
+
+          Workspaces supports mounting, by specifying an Mount object in the spec.
+          If a Mount is specified, the workspace will be mounted to the specified mount object and
+          LogicalCluster will not be created.
         properties:
           apiVersion:
             description: |-
@@ -149,7 +153,7 @@ spec:
                 type: object
               mount:
                 description: |-
-                  Mount is a reference to a an object implementing a mounting feature. It is used to orchestrate
+                  Mount is a reference to an object implementing a mounting feature. It is used to orchestrate
                   where the traffic, intended for the workspace, is sent.
                   If specified, logicalcluster will not be created and the workspace will be mounted
                   using reference mount object.
@@ -186,6 +190,9 @@ spec:
                 required:
                 - ref
                 type: object
+                x-kubernetes-validations:
+                - message: mount is immutable
+                  rule: self == oldSelf
               type:
                 description: |-
                   type defines properties of the workspace both on creation (e.g. initial
diff --git a/config/root-phase0/apiexport-tenancy.kcp.io.yaml b/config/root-phase0/apiexport-tenancy.kcp.io.yaml
@@ -9,7 +9,7 @@ spec:
   resources:
   - group: tenancy.kcp.io
     name: workspaces
-    schema: v250315-28b43d5a9.workspaces.tenancy.kcp.io
+    schema: v250421-25d98218b.workspaces.tenancy.kcp.io
     storage:
       crd: {}
   - group: tenancy.kcp.io
diff --git a/config/root-phase0/apiresourceschema-workspaces.tenancy.kcp.io.yaml b/config/root-phase0/apiresourceschema-workspaces.tenancy.kcp.io.yaml
@@ -2,7 +2,7 @@ apiVersion: apis.kcp.io/v1alpha1
 kind: APIResourceSchema
 metadata:
   creationTimestamp: null
-  name: v250315-28b43d5a9.workspaces.tenancy.kcp.io
+  name: v250421-25d98218b.workspaces.tenancy.kcp.io
 spec:
   group: tenancy.kcp.io
   names:
@@ -46,6 +46,10 @@ spec:
         depending on access pattern. All workspace implementations share the characteristic
         that the URL that serves a given workspace can be used with standard Kubernetes
         API machinery and client libraries and command line tools.
+
+        Workspaces supports mounting, by specifying an Mount object in the spec.
+        If a Mount is specified, the workspace will be mounted to the specified mount object and
+        LogicalCluster will not be created.
       properties:
         apiVersion:
           description: |-
@@ -147,7 +151,7 @@ spec:
               type: object
             mount:
               description: |-
-                Mount is a reference to a an object implementing a mounting feature. It is used to orchestrate
+                Mount is a reference to an object implementing a mounting feature. It is used to orchestrate
                 where the traffic, intended for the workspace, is sent.
                 If specified, logicalcluster will not be created and the workspace will be mounted
                 using reference mount object.
@@ -184,6 +188,9 @@ spec:
               required:
               - ref
               type: object
+              x-kubernetes-validations:
+              - message: mount is immutable
+                rule: self == oldSelf
             type:
               description: |-
                 type defines properties of the workspace both on creation (e.g. initial
diff --git a/pkg/admission/workspace/admission.go b/pkg/admission/workspace/admission.go
@@ -166,7 +166,7 @@ func (o *workspace) Validate(ctx context.Context, a admission.Attributes, _ admi
 		}
 
 		// Not a mountpoint - validate the spec fields
-		if !old.Spec.IsMounted() {
+		if old.Spec.Mount == nil {
 			if old.Spec.Cluster != "" && ws.Spec.Cluster == "" {
 				return admission.NewForbidden(a, errors.New("spec.cluster cannot be unset"))
 			}
@@ -257,7 +257,7 @@ func (o *workspace) Validate(ctx context.Context, a admission.Attributes, _ admi
 			}
 		}
 
-		if ws.Spec.IsMounted() {
+		if ws.Spec.Mount != nil {
 			if ws.Spec.Mount.Reference.Kind == "" {
 				return admission.NewForbidden(a, errors.New("spec.mount.kind must be set"))
 			}
diff --git a/pkg/index/index.go b/pkg/index/index.go
@@ -183,11 +183,17 @@ func (c *State) DeleteWorkspace(shard string, ws *tenancyv1alpha1.Workspace) {
 		delete(c.shardClusterWorkspaceMount[shard][clusterName], ws.Name)
 		if len(c.shardClusterWorkspaceMount[shard][clusterName]) == 0 {
 			delete(c.shardClusterWorkspaceMount[shard], clusterName)
+			if len(c.shardClusterWorkspaceMount[shard]) == 0 {
+				delete(c.shardClusterWorkspaceName, shard)
+			}
 		}
 	}
 	delete(c.shardClusterWorkspaceNameErrorCode[shard][clusterName], ws.Name)
 	if len(c.shardClusterWorkspaceNameErrorCode[shard][clusterName]) == 0 {
 		delete(c.shardClusterWorkspaceNameErrorCode[shard], clusterName)
+		if len(c.shardClusterWorkspaceNameErrorCode[shard]) == 0 {
+			delete(c.shardClusterWorkspaceNameErrorCode, shard)
+		}
 	}
 }
 
diff --git a/pkg/openapi/zz_generated.openapi.go b/pkg/openapi/zz_generated.openapi.go
diff --git a/pkg/reconciler/tenancy/workspace/workspace_reconcile_metadata.go b/pkg/reconciler/tenancy/workspace/workspace_reconcile_metadata.go
@@ -32,7 +32,7 @@ type metaDataReconciler struct {
 
 func (r *metaDataReconciler) reconcile(ctx context.Context, workspace *tenancyv1alpha1.Workspace) (reconcileStatus, error) {
 	logger := klog.FromContext(ctx).WithValues("reconciler", "metadata")
-	if workspace.Spec.IsMounted() {
+	if workspace.Spec.Mount != nil {
 		return reconcileStatusContinue, nil
 	}
 
diff --git a/pkg/reconciler/tenancy/workspace/workspace_reconcile_phase.go b/pkg/reconciler/tenancy/workspace/workspace_reconcile_phase.go
@@ -42,7 +42,7 @@ type phaseReconciler struct {
 func (r *phaseReconciler) reconcile(ctx context.Context, workspace *tenancyv1alpha1.Workspace) (reconcileStatus, error) {
 	logger := klog.FromContext(ctx).WithValues("reconciler", "phase")
 
-	if !workspace.Spec.IsMounted() {
+	if workspace.Spec.Mount == nil {
 		switch workspace.Status.Phase {
 		case corev1alpha1.LogicalClusterPhaseScheduling:
 			if workspace.Spec.URL != "" && workspace.Spec.Cluster != "" {
diff --git a/pkg/reconciler/tenancy/workspace/workspace_reconcile_scheduling.go b/pkg/reconciler/tenancy/workspace/workspace_reconcile_scheduling.go
@@ -83,7 +83,7 @@ type schedulingReconciler struct {
 
 func (r *schedulingReconciler) reconcile(ctx context.Context, workspace *tenancyv1alpha1.Workspace) (reconcileStatus, error) {
 	logger := klog.FromContext(ctx).WithValues("reconciler", "scheduling")
-	if workspace.Spec.IsMounted() {
+	if workspace.Spec.Mount != nil {
 		return reconcileStatusContinue, nil
 	}
 
diff --git a/pkg/reconciler/tenancy/workspacemounts/workspacemounts_controller.go b/pkg/reconciler/tenancy/workspacemounts/workspacemounts_controller.go
@@ -207,6 +207,9 @@ func (c *Controller) process(ctx context.Context, key string) (bool, error) {
 		} else {
 			gvr.Version = ref.APIVersion
 		}
+		if ref.Namespace != "" {
+			return c.dynamicClusterClient.Cluster(cluster).Resource(gvr).Namespace(ref.Namespace).Get(ctx, ref.Name, metav1.GetOptions{})
+		}
 		return c.dynamicClusterClient.Cluster(cluster).Resource(gvr).Get(ctx, ref.Name, metav1.GetOptions{})
 	}
 
diff --git a/pkg/virtual/framework/internalapis/fixtures/workspaces.yaml b/pkg/virtual/framework/internalapis/fixtures/workspaces.yaml
@@ -15,6 +15,8 @@ spec:
         Workspace defines a generic Kubernetes-cluster-like endpoint, with standard Kubernetes discovery APIs, OpenAPI and resource API endpoints.
 
         A workspace can be backed by different concrete types of workspace implementation, depending on access pattern. All workspace implementations share the characteristic that the URL that serves a given workspace can be used with standard Kubernetes API machinery and client libraries and command line tools.
+
+        Workspaces supports mounting, by specifying an Mount object in the spec. If a Mount is specified, the workspace will be mounted to the specified mount object and LogicalCluster will not be created.
       properties:
         apiVersion:
           description: 'APIVersion defines the versioned schema of this representation
@@ -98,7 +100,7 @@ spec:
                   type: object
               type: object
             mount:
-              description: Mount is a reference to a an object implementing a mounting
+              description: Mount is a reference to an object implementing a mounting
                 feature. It is used to orchestrate where the traffic, intended for
                 the workspace, is sent. If specified, logicalcluster will not be created
                 and the workspace will be mounted using reference mount object.
diff --git a/sdk/apis/tenancy/v1alpha1/types_workspace.go b/sdk/apis/tenancy/v1alpha1/types_workspace.go
@@ -108,6 +108,10 @@ const LogicalClusterTypeAnnotationKey = "internal.tenancy.kcp.io/type"
 // that the URL that serves a given workspace can be used with standard Kubernetes
 // API machinery and client libraries and command line tools.
 //
+// Workspaces supports mounting, by specifying an Mount object in the spec.
+// If a Mount is specified, the workspace will be mounted to the specified mount object and
+// LogicalCluster will not be created.
+//
 // +crd
 // +genclient
 // +genclient:nonNamespaced
@@ -175,20 +179,17 @@ type WorkspaceSpec struct {
 	// +kubebuilder:format:uri
 	URL string `json:"URL,omitempty"`
 
-	// Mount is a reference to a an object implementing a mounting feature. It is used to orchestrate
+	// Mount is a reference to an object implementing a mounting feature. It is used to orchestrate
 	// where the traffic, intended for the workspace, is sent.
 	// If specified, logicalcluster will not be created and the workspace will be mounted
 	// using reference mount object.
 	//
 	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="mount is immutable"
 	Mount *Mount `json:"mount,omitempty"`
 }
 
-func (in *WorkspaceSpec) IsMounted() bool {
-	return in.Mount != nil
-}
-
-// Mount is a reference to a an object implementing a mounting feature. It is used to orchestrate
+// Mount is a reference to an object implementing a mounting feature. It is used to orchestrate
 // where the traffic, intended for the workspace, is sent.
 type Mount struct {
 	// Reference is an ObjectReference to the object that is mounted.

Original file line number	Diff line number	Diff line change
`@@ -166,7 +166,7 @@ func (o *workspace) Validate(ctx context.Context, a admission.Attributes, _ admi`
`166`	`166`	`}`
`167`	`167`
`168`	`168`	`// Not a mountpoint - validate the spec fields`
`169`		`- if !old.Spec.IsMounted() {`
	`169`	`+ if old.Spec.Mount == nil {`
`170`	`170`	`if old.Spec.Cluster != "" && ws.Spec.Cluster == "" {`
`171`	`171`	`return admission.NewForbidden(a, errors.New("spec.cluster cannot be unset"))`
`172`	`172`	`}`
`@@ -257,7 +257,7 @@ func (o *workspace) Validate(ctx context.Context, a admission.Attributes, _ admi`
`257`	`257`	`}`
`258`	`258`	`}`
`259`	`259`
`260`		`- if ws.Spec.IsMounted() {`
	`260`	`+ if ws.Spec.Mount != nil {`
`261`	`261`	`if ws.Spec.Mount.Reference.Kind == "" {`
`262`	`262`	`return admission.NewForbidden(a, errors.New("spec.mount.kind must be set"))`
`263`	`263`	`}`
Original file line number	Diff line number	Diff line change
`@@ -183,11 +183,17 @@ func (c State) DeleteWorkspace(shard string, ws tenancyv1alpha1.Workspace) {`
`183`	`183`	`delete(c.shardClusterWorkspaceMount[shard][clusterName], ws.Name)`
`184`	`184`	`if len(c.shardClusterWorkspaceMount[shard][clusterName]) == 0 {`
`185`	`185`	`delete(c.shardClusterWorkspaceMount[shard], clusterName)`
	`186`	`+ if len(c.shardClusterWorkspaceMount[shard]) == 0 {`
	`187`	`+ delete(c.shardClusterWorkspaceName, shard)`
	`188`	`+ }`
`186`	`189`	`}`
`187`	`190`	`}`
`188`	`191`	`delete(c.shardClusterWorkspaceNameErrorCode[shard][clusterName], ws.Name)`
`189`	`192`	`if len(c.shardClusterWorkspaceNameErrorCode[shard][clusterName]) == 0 {`
`190`	`193`	`delete(c.shardClusterWorkspaceNameErrorCode[shard], clusterName)`
	`194`	`+ if len(c.shardClusterWorkspaceNameErrorCode[shard]) == 0 {`
	`195`	`+ delete(c.shardClusterWorkspaceNameErrorCode, shard)`
	`196`	`+ }`
`191`	`197`	`}`
`192`	`198`	`}`
`193`	`199`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ type metaDataReconciler struct {`
`32`	`32`
`33`	`33`	`func (r metaDataReconciler) reconcile(ctx context.Context, workspace tenancyv1alpha1.Workspace) (reconcileStatus, error) {`
`34`	`34`	`logger := klog.FromContext(ctx).WithValues("reconciler", "metadata")`
`35`		`- if workspace.Spec.IsMounted() {`
	`35`	`+ if workspace.Spec.Mount != nil {`
`36`	`36`	`return reconcileStatusContinue, nil`
`37`	`37`	`}`
`38`	`38`
Original file line number	Diff line number	Diff line change
`@@ -83,7 +83,7 @@ type schedulingReconciler struct {`
`83`	`83`
`84`	`84`	`func (r schedulingReconciler) reconcile(ctx context.Context, workspace tenancyv1alpha1.Workspace) (reconcileStatus, error) {`
`85`	`85`	`logger := klog.FromContext(ctx).WithValues("reconciler", "scheduling")`
`86`		`- if workspace.Spec.IsMounted() {`
	`86`	`+ if workspace.Spec.Mount != nil {`
`87`	`87`	`return reconcileStatusContinue, nil`
`88`	`88`	`}`
`89`	`89`
Original file line number	Diff line number	Diff line change
`@@ -207,6 +207,9 @@ func (c *Controller) process(ctx context.Context, key string) (bool, error) {`
`207`	`207`	`} else {`
`208`	`208`	`gvr.Version = ref.APIVersion`
`209`	`209`	`}`
	`210`	`+ if ref.Namespace != "" {`
	`211`	`+ return c.dynamicClusterClient.Cluster(cluster).Resource(gvr).Namespace(ref.Namespace).Get(ctx, ref.Name, metav1.GetOptions{})`
	`212`	`+ }`
`210`	`213`	`return c.dynamicClusterClient.Cluster(cluster).Resource(gvr).Get(ctx, ref.Name, metav1.GetOptions{})`
`211`	`214`	`}`
`212`	`215`