Skip to content

✨ Promote mounts to spec #3282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
6 changes: 3 additions & 3 deletions cli/pkg/workspace/plugin/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,17 +127,17 @@ func (o *CreateWorkspaceOptions) Run(ctx context.Context) error {
return fmt.Errorf("--ignore-existing must not be used with non-absolute type path")
}

var structuredWorkspaceType tenancyv1alpha1.WorkspaceTypeReference
var structuredWorkspaceType *tenancyv1alpha1.WorkspaceTypeReference
if o.Type != "" {
separatorIndex := strings.LastIndex(o.Type, ":")
switch separatorIndex {
case -1:
structuredWorkspaceType = tenancyv1alpha1.WorkspaceTypeReference{
structuredWorkspaceType = &tenancyv1alpha1.WorkspaceTypeReference{
Name: tenancyv1alpha1.WorkspaceTypeName(strings.ToLower(o.Type)),
// path is defaulted through admission
}
default:
structuredWorkspaceType = tenancyv1alpha1.WorkspaceTypeReference{
structuredWorkspaceType = &tenancyv1alpha1.WorkspaceTypeReference{
Name: tenancyv1alpha1.WorkspaceTypeName(strings.ToLower(o.Type[separatorIndex+1:])),
Path: o.Type[:separatorIndex],
}
Expand Down
15 changes: 7 additions & 8 deletions cli/pkg/workspace/plugin/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestCreate(t *testing.T) {
markReady bool

newWorkspaceName string
newWorkspaceType tenancyv1alpha1.WorkspaceTypeReference
newWorkspaceType *tenancyv1alpha1.WorkspaceTypeReference
useAfterCreation, ignoreExisting bool

expected *clientcmdapi.Config
Expand Down Expand Up @@ -145,7 +145,7 @@ func TestCreate(t *testing.T) {
},
existingWorkspaces: []string{"bar"},
newWorkspaceName: "bar",
newWorkspaceType: tenancyv1alpha1.WorkspaceTypeReference{Path: "root", Name: "universal"},
newWorkspaceType: &tenancyv1alpha1.WorkspaceTypeReference{Path: "root", Name: "universal"},
useAfterCreation: true,
markReady: true,
ignoreExisting: true,
Expand All @@ -171,7 +171,7 @@ func TestCreate(t *testing.T) {
},
newWorkspaceName: "bar",
ignoreExisting: true,
newWorkspaceType: tenancyv1alpha1.WorkspaceTypeReference{Name: "universal"},
newWorkspaceType: &tenancyv1alpha1.WorkspaceTypeReference{Name: "universal"},
wantErr: true,
},
}
Expand All @@ -197,7 +197,7 @@ func TestCreate(t *testing.T) {
},
Spec: tenancyv1alpha1.WorkspaceSpec{
URL: fmt.Sprintf("https://test%s", currentClusterName.Join(name).RequestPath()),
Type: tenancyv1alpha1.WorkspaceTypeReference{
Type: &tenancyv1alpha1.WorkspaceTypeReference{
Name: "universal",
Path: "root",
},
Expand All @@ -209,10 +209,9 @@ func TestCreate(t *testing.T) {
}
client := kcpfakeclient.NewSimpleClientset(objects...)

workspaceType := tt.newWorkspaceType //nolint:govet // TODO(sttts): fixing this above breaks the test
empty := tenancyv1alpha1.WorkspaceTypeReference{}
if workspaceType == empty {
workspaceType = tenancyv1alpha1.WorkspaceTypeReference{
workspaceType := tt.newWorkspaceType
if tt.newWorkspaceType == nil {
workspaceType = &tenancyv1alpha1.WorkspaceTypeReference{
Name: "universal",
Path: "root",
}
Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/workspace/plugin/use.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func (o *UseWorkspaceOptions) Run(ctx context.Context) (err error) {
if ws, err := o.kcpClusterClient.Cluster(parentClusterName).TenancyV1alpha1().Workspaces().Get(ctx, workspaceName, metav1.GetOptions{}); apierrors.IsNotFound(err) {
notFound = true
} else if err == nil {
workspaceType = &ws.Spec.Type
workspaceType = ws.Spec.Type
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions cli/pkg/workspace/plugin/use_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,7 @@ func TestUse(t *testing.T) {
Annotations: map[string]string{logicalcluster.AnnotationKey: lcluster.String()},
},
Spec: tenancyv1alpha1.WorkspaceSpec{
Type: tenancyv1alpha1.WorkspaceTypeReference{
Type: &tenancyv1alpha1.WorkspaceTypeReference{
Name: "universal",
Path: "root",
},
Expand All @@ -780,7 +780,7 @@ func TestUse(t *testing.T) {
},
Spec: tenancyv1alpha1.WorkspaceSpec{
URL: fmt.Sprintf("https://test%s", homeWorkspace.RequestPath()),
Type: tenancyv1alpha1.WorkspaceTypeReference{
Type: &tenancyv1alpha1.WorkspaceTypeReference{
Name: "home",
Path: "root",
},
Expand Down
41 changes: 40 additions & 1 deletion config/crds/tenancy.kcp.io_workspaces.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ spec:
name: Region
type: string
- description: The current phase (e.g. Scheduling, Initializing, Ready, Deleting)
jsonPath: .metadata.labels['tenancy\.kcp\.io/phase']
jsonPath: .status.phase
name: Phase
type: string
- description: URL to access the workspace
Expand Down Expand Up @@ -147,6 +147,45 @@ spec:
type: object
x-kubernetes-map-type: atomic
type: object
mount:
description: |-
Mount is a reference to a 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.
properties:
ref:
description: Reference is an ObjectReference to the object that
is mounted.
properties:
apiVersion:
description: APIVersion is the API group and version of the
object.
type: string
kind:
description: Kind is the kind of the object.
type: string
name:
description: Name is the name of the object.
type: string
namespace:
description: Namespace is the namespace of the object.
type: string
required:
- apiVersion
- kind
- name
type: object
x-kubernetes-validations:
- message: apiVersion is immutable
rule: has(oldSelf.apiVersion) == has(self.apiVersion)
- message: kind is immutable
rule: has(oldSelf.kind) == has(self.kind)
- message: name is immutable
rule: has(oldSelf.name) == has(self.name)
required:
- ref
type: object
type:
description: |-
type defines properties of the workspace both on creation (e.g. initial
Expand Down
2 changes: 1 addition & 1 deletion config/root-phase0/apiexport-tenancy.kcp.io.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ spec:
resources:
- group: tenancy.kcp.io
name: workspaces
schema: v241020-fce06d31d.workspaces.tenancy.kcp.io
schema: v250315-28b43d5a9.workspaces.tenancy.kcp.io
storage:
crd: {}
- group: tenancy.kcp.io
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: apis.kcp.io/v1alpha1
kind: APIResourceSchema
metadata:
creationTimestamp: null
name: v241020-fce06d31d.workspaces.tenancy.kcp.io
name: v250315-28b43d5a9.workspaces.tenancy.kcp.io
spec:
group: tenancy.kcp.io
names:
Expand All @@ -26,7 +26,7 @@ spec:
name: Region
type: string
- description: The current phase (e.g. Scheduling, Initializing, Ready, Deleting)
jsonPath: .metadata.labels['tenancy\.kcp\.io/phase']
jsonPath: .status.phase
name: Phase
type: string
- description: URL to access the workspace
Expand Down Expand Up @@ -145,6 +145,45 @@ spec:
type: object
x-kubernetes-map-type: atomic
type: object
mount:
description: |-
Mount is a reference to a 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.
properties:
ref:
description: Reference is an ObjectReference to the object that
is mounted.
properties:
apiVersion:
description: APIVersion is the API group and version of the
object.
type: string
kind:
description: Kind is the kind of the object.
type: string
name:
description: Name is the name of the object.
type: string
namespace:
description: Namespace is the namespace of the object.
type: string
required:
- apiVersion
- kind
- name
type: object
x-kubernetes-validations:
- message: apiVersion is immutable
rule: has(oldSelf.apiVersion) == has(self.apiVersion)
- message: kind is immutable
rule: has(oldSelf.kind) == has(self.kind)
- message: name is immutable
rule: has(oldSelf.name) == has(self.name)
required:
- ref
type: object
type:
description: |-
type defines properties of the workspace both on creation (e.g. initial
Expand Down
93 changes: 71 additions & 22 deletions pkg/admission/workspace/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ func (o *workspace) Admit(ctx context.Context, a admission.Attributes, _ admissi
// - has a valid type and it is not mutated
// - the cluster is not removed
// - the user is recorded in annotations on create
// - the required groups match with the LogicalCluster.
// - the required groups match with the LogicalCluster
// - only system privileged users can set both spec.Type and spec.Mount.
func (o *workspace) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) (err error) {
clusterName, err := genericapirequest.ClusterNameFrom(ctx)
if err != nil {
Expand Down Expand Up @@ -164,30 +165,62 @@ func (o *workspace) Validate(ctx context.Context, a admission.Attributes, _ admi
return fmt.Errorf("failed to convert unstructured to Workspace: %w", err)
}

if old.Spec.Cluster != "" && ws.Spec.Cluster == "" {
return admission.NewForbidden(a, errors.New("spec.cluster cannot be unset"))
}
if old.Spec.Cluster != ws.Spec.Cluster && !isSystemPrivileged {
return admission.NewForbidden(a, errors.New("spec.cluster can only be changed by system privileged users"))
}
if old.Spec.URL != ws.Spec.URL && !isSystemPrivileged {
return admission.NewForbidden(a, errors.New("spec.URL can only be changed by system privileged users"))
}
// Not a mountpoint - validate the spec fields
if !old.Spec.IsMounted() {
if old.Spec.Cluster != "" && ws.Spec.Cluster == "" {
return admission.NewForbidden(a, errors.New("spec.cluster cannot be unset"))
}
if old.Spec.Cluster != ws.Spec.Cluster && !isSystemPrivileged {
return admission.NewForbidden(a, errors.New("spec.cluster can only be changed by system privileged users"))
}
if old.Spec.URL != ws.Spec.URL && !isSystemPrivileged {
return admission.NewForbidden(a, errors.New("spec.URL can only be changed by system privileged users"))
}

if errs := validation.ValidateImmutableField(ws.Spec.Type, old.Spec.Type, field.NewPath("spec", "type")); len(errs) > 0 {
return admission.NewForbidden(a, errs.ToAggregate())
}
if old.Spec.Type.Path != ws.Spec.Type.Path || old.Spec.Type.Name != ws.Spec.Type.Name {
return admission.NewForbidden(a, errors.New("spec.type is immutable"))
}
if errs := validation.ValidateImmutableField(ws.Spec.Type, old.Spec.Type, field.NewPath("spec", "type")); len(errs) > 0 {
return admission.NewForbidden(a, errs.ToAggregate())
}
if old.Spec.Type.Path != ws.Spec.Type.Path || old.Spec.Type.Name != ws.Spec.Type.Name {
return admission.NewForbidden(a, errors.New("spec.type is immutable"))
}
// If we're transitioning to "Ready", make sure that spec.cluster and spec.URL are set.
// This applies only for non-mounted workspaces.
if old.Status.Phase != corev1alpha1.LogicalClusterPhaseReady && ws.Status.Phase == corev1alpha1.LogicalClusterPhaseReady {
if ws.Spec.Cluster == "" {
return admission.NewForbidden(a, fmt.Errorf("spec.cluster must be set for phase %s", ws.Status.Phase))
}
if ws.Spec.URL == "" {
return admission.NewForbidden(a, fmt.Errorf("spec.URL must be set for phase %s", ws.Status.Phase))
}
}
} else {
// Mounted - validate the mount fields
if old.Spec.Mount.Reference.Kind != ws.Spec.Mount.Reference.Kind {
return admission.NewForbidden(a, errors.New("spec.mount.kind is immutable"))
}
if old.Spec.Mount.Reference.Name != ws.Spec.Mount.Reference.Name {
return admission.NewForbidden(a, errors.New("spec.mount.name is immutable"))
}
if old.Spec.Mount.Reference.Namespace != ws.Spec.Mount.Reference.Namespace {
return admission.NewForbidden(a, errors.New("spec.mount.namespace is immutable"))
}
if old.Spec.Mount.Reference.APIVersion != ws.Spec.Mount.Reference.APIVersion {
return admission.NewForbidden(a, errors.New("spec.mount.apiVersion is immutable"))
}

// If we're transitioning to "Ready", make sure that spec.cluster and spec.URL are set.
if old.Status.Phase != corev1alpha1.LogicalClusterPhaseReady && ws.Status.Phase == corev1alpha1.LogicalClusterPhaseReady {
if ws.Spec.Cluster == "" {
return admission.NewForbidden(a, fmt.Errorf("spec.cluster must be set for phase %s", ws.Status.Phase))
// if not system privileged, disallow setting spec.type
if !isSystemPrivileged && ws.Spec.Type != nil {
return admission.NewForbidden(a, errors.New("spec.type cannot be set for mounted workspaces"))
}
if ws.Spec.URL == "" {
return admission.NewForbidden(a, fmt.Errorf("spec.URL must be set for phase %s", ws.Status.Phase))
// Check for immutability of spec.type via pointers checks first.
if !isSystemPrivileged && ((old.Spec.Type == nil && ws.Spec.Type != nil) || (old.Spec.Type != nil && ws.Spec.Type == nil)) {
return admission.NewForbidden(a, errors.New("spec.type is immutable"))
}
// Check for immutability of spec.type via field checks.
if !isSystemPrivileged && old.Spec.Type != nil && ws.Spec.Type != nil {
if old.Spec.Type.Path != ws.Spec.Type.Path || old.Spec.Type.Name != ws.Spec.Type.Name {
return admission.NewForbidden(a, errors.New("spec.type is immutable"))
}
}
}
case admission.Create:
Expand Down Expand Up @@ -223,6 +256,22 @@ func (o *workspace) Validate(ctx context.Context, a admission.Attributes, _ admi
return admission.NewForbidden(a, fmt.Errorf("missing required groups annotation %s=%s", authorization.RequiredGroupsAnnotationKey, expected))
}
}

if ws.Spec.IsMounted() {
if ws.Spec.Mount.Reference.Kind == "" {
return admission.NewForbidden(a, errors.New("spec.mount.kind must be set"))
}
if ws.Spec.Mount.Reference.Name == "" {
return admission.NewForbidden(a, errors.New("spec.mount.name must be set"))
}
if ws.Spec.Mount.Reference.APIVersion == "" {
return admission.NewForbidden(a, errors.New("spec.mount.apiVersion must be set"))
}

if !isSystemPrivileged && ws.Spec.Type != nil {
return admission.NewForbidden(a, errors.New("spec.type cannot be set for mounted workspaces"))
}
}
}

return nil
Expand Down
Loading