Merge pull request #2588 from vincepri/disallow-parent-fix-test

🐛 WorkspaceType admission should respect LimitAllow{Parents, Children}.None
kcp-dev · Jan 10, 2023 · 95c4b1a · 95c4b1a
2 parents 8d3a51e + fd9f1b2
commit 95c4b1a
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 5 deletions.
diff --git a/config/crds/tenancy.kcp.io_workspacetypes.yaml b/config/crds/tenancy.kcp.io_workspacetypes.yaml
@@ -179,6 +179,11 @@ spec:
                 description: limitAllowedParents specifies constraints for the parent
                   workspace that workspaces of this type are created in. These are
                   in addition to parent constraints of types this one extends.
+                oneOf:
+                - required:
+                  - none
+                - required:
+                  - types
                 properties:
                   none:
                     description: none means that no type matches.

diff --git a/config/crds/tenancy.kcp.io_workspacetypes.yaml-patch b/config/crds/tenancy.kcp.io_workspacetypes.yaml-patch
@@ -11,6 +11,12 @@
         - system
         - any
 
+- op: add
+  path: /spec/versions/name=v1alpha1/schema/openAPIV3Schema/properties/spec/properties/limitAllowedParents/oneOf
+  value:
+  - required: ["none"]
+  - required: ["types"]
+
 - op: add
   path: /spec/versions/name=v1alpha1/schema/openAPIV3Schema/properties/spec/properties/limitAllowedChildren/oneOf
   value:

diff --git a/config/root-phase0/apiexport-tenancy.kcp.io.yaml b/config/root-phase0/apiexport-tenancy.kcp.io.yaml
@@ -7,7 +7,7 @@ spec:
   latestResourceSchemas:
   - v221219-c92ed8152.clusterworkspaces.tenancy.kcp.io
   - v230109-7504f774.workspaces.tenancy.kcp.io
-  - v230109-7504f774.workspacetypes.tenancy.kcp.io
+  - v230110-89146c99.workspacetypes.tenancy.kcp.io
   maximalPermissionPolicy:
     local: {}
 status: {}
diff --git a/config/root-phase0/apiresourceschema-workspacetypes.tenancy.kcp.io.yaml b/config/root-phase0/apiresourceschema-workspacetypes.tenancy.kcp.io.yaml
@@ -2,7 +2,7 @@ apiVersion: apis.kcp.io/v1alpha1
 kind: APIResourceSchema
 metadata:
   creationTimestamp: null
-  name: v230109-7504f774.workspacetypes.tenancy.kcp.io
+  name: v230110-89146c99.workspacetypes.tenancy.kcp.io
 spec:
   group: tenancy.kcp.io
   names:
@@ -176,6 +176,11 @@ spec:
               description: limitAllowedParents specifies constraints for the parent
                 workspace that workspaces of this type are created in. These are in
                 addition to parent constraints of types this one extends.
+              oneOf:
+              - required:
+                - none
+              - required:
+                - types
               properties:
                 none:
                   description: none means that no type matches.

diff --git a/pkg/admission/workspacetypeexists/admission.go b/pkg/admission/workspacetypeexists/admission.go
@@ -453,7 +453,14 @@ func (r *transitiveTypeResolver) resolve(wt *tenancyv1alpha1.WorkspaceType, seen
 func validateAllowedParents(parentAliases, childAliases []*tenancyv1alpha1.WorkspaceType, parentType, childType logicalcluster.Path) error {
 	var errs []error
 	for _, childAlias := range childAliases {
-		if childAlias.Spec.LimitAllowedParents == nil || len(childAlias.Spec.LimitAllowedParents.Types) == 0 {
+		if childAlias.Spec.LimitAllowedParents == nil {
+			continue
+		}
+		if childAlias.Spec.LimitAllowedParents.None {
+			errs = append(errs, fmt.Errorf("workspace type %s cannot have any parent", childType))
+			continue
+		}
+		if len(childAlias.Spec.LimitAllowedParents.Types) == 0 {
 			continue
 		}
 
@@ -487,11 +494,15 @@ func validateAllowedParents(parentAliases, childAliases []*tenancyv1alpha1.Works
 func validateAllowedChildren(parentAliases, childAliases []*tenancyv1alpha1.WorkspaceType, parentType, childType logicalcluster.Path) error {
 	var errs []error
 	for _, parentAlias := range parentAliases {
-		if parentAlias.Spec.LimitAllowedChildren == nil || len(parentAlias.Spec.LimitAllowedChildren.Types) == 0 {
+		if parentAlias.Spec.LimitAllowedChildren == nil {
 			continue
 		}
 		if parentAlias.Spec.LimitAllowedChildren.None {
-			return fmt.Errorf("workspace type %s cannot have any children", parentType)
+			errs = append(errs, fmt.Errorf("workspace type %s cannot have any child", parentType))
+			continue
+		}
+		if len(parentAlias.Spec.LimitAllowedChildren.Types) == 0 {
+			continue
 		}
 
 		qualifiedParent := canonicalPathFrom(parentAlias).Join(string(tenancyv1alpha1.TypeName(parentAlias.Name)))

diff --git a/pkg/admission/workspacetypeexists/admission_test.go b/pkg/admission/workspacetypeexists/admission_test.go
@@ -676,6 +676,15 @@ func TestValidateAllowedParents(t *testing.T) {
 			},
 			wantErr: "workspace type root:a only allows [root:b] parent workspaces, but parent type root:c only implements []",
 		},
+		{
+			name:       "no parents allowed",
+			childType:  logicalcluster.NewPath("root:a"),
+			parentType: logicalcluster.NewPath("root:c"),
+			childAliases: []*tenancyv1alpha1.WorkspaceType{
+				newType("root:a").disallowingParent().WorkspaceType,
+			},
+			wantErr: "workspace type root:a cannot have any parent",
+		},
 		{
 			name:       "no parents, any allowed parent",
 			childType:  logicalcluster.NewPath("root:a"),