kubelet: grant permission for DeleteCollection

2e34e187c9 enabled kubelet to do List and Watch
requests with the caveat that kubelet should better use a field selector (which
it does). The same is now also needed for DeleteCollection because kubelet will
use that to clean up in one operation instead of using multiple.

plugin/pkg/admission/noderestriction/admission.go

@@ -641,8 +641,14 @@ func (p *Plugin) admitCSINode(nodeName string, a admission.Attributes) error {
 
 func (p *Plugin) admitResourceSlice(nodeName string, a admission.Attributes) error {
 	// The create request must come from a node with the same name as the NodeName field.
-	// Other requests gets checked by the node authorizer.
+	// Same when deleting an object.
-	if a.GetOperation() == admission.Create {
+	//
+	// Other requests get checked by the node authorizer. The checks here are necessary
+	// because the node authorizer does not know the object content for a create request
+	// and not each deleted object in a DeleteCollection. DeleteCollection checks each
+	// individual object.
+	switch a.GetOperation() {
+	case admission.Create:
 		slice, ok := a.GetObject().(*resource.ResourceSlice)
 		if !ok {
 			return admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetObject()))
@@ -651,6 +657,15 @@ func (p *Plugin) admitResourceSlice(nodeName string, a admission.Attributes) err
 		if slice.NodeName != nodeName {
 			return admission.NewForbidden(a, errors.New("can only create ResourceSlice with the same NodeName as the requesting node"))
 		}
+	case admission.Delete:
+		slice, ok := a.GetOldObject().(*resource.ResourceSlice)
+		if !ok {
+			return admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetOldObject()))
+		}
+
+		if slice.NodeName != nodeName {
+			return admission.NewForbidden(a, errors.New("can only delete ResourceSlice with the same NodeName as the requesting node"))
+		}
 	}
 
 	return nil

plugin/pkg/admission/noderestriction/admission_test.go

@@ -1607,7 +1607,8 @@ func TestAdmitResourceSlice(t *testing.T) {
 	apiResource := resourceapi.SchemeGroupVersion.WithResource("resourceslices")
 	nodename := "mynode"
 	mynode := &user.DefaultInfo{Name: "system:node:" + nodename, Groups: []string{"system:nodes"}}
-	err := "can only create ResourceSlice with the same NodeName as the requesting node"
+	createErr := "can only create ResourceSlice with the same NodeName as the requesting node"
+	deleteErr := "can only delete ResourceSlice with the same NodeName as the requesting node"
 
 	sliceNode := &resourceapi.ResourceSlice{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1624,53 +1625,88 @@ func TestAdmitResourceSlice(t *testing.T) {
 
 	tests := map[string]struct {
 		operation      admission.Operation
-		obj            runtime.Object
+		options        runtime.Object
+		obj, oldObj    runtime.Object
 		featureEnabled bool
 		expectError    string
 	}{
 		"create allowed, enabled": {
 			operation:      admission.Create,
+			options:        &metav1.CreateOptions{},
 			obj:            sliceNode,
 			featureEnabled: true,
 			expectError:    "",
 		},
 		"create disallowed, enabled": {
 			operation:      admission.Create,
+			options:        &metav1.CreateOptions{},
 			obj:            sliceOtherNode,
 			featureEnabled: true,
-			expectError:    err,
+			expectError:    createErr,
 		},
 		"create allowed, disabled": {
 			operation:      admission.Create,
+			options:        &metav1.CreateOptions{},
 			obj:            sliceNode,
 			featureEnabled: false,
 			expectError:    "",
 		},
 		"create disallowed, disabled": {
 			operation:      admission.Create,
+			options:        &metav1.CreateOptions{},
 			obj:            sliceOtherNode,
 			featureEnabled: false,
-			expectError:    err,
+			expectError:    createErr,
 		},
 		"update allowed, same node": {
 			operation:      admission.Update,
+			options:        &metav1.UpdateOptions{},
 			obj:            sliceNode,
 			featureEnabled: true,
 			expectError:    "",
 		},
 		"update allowed, other node": {
 			operation:      admission.Update,
+			options:        &metav1.UpdateOptions{},
 			obj:            sliceOtherNode,
 			featureEnabled: true,
 			expectError:    "",
 		},
+		"delete allowed, enabled": {
+			operation:      admission.Delete,
+			options:        &metav1.DeleteOptions{},
+			oldObj:         sliceNode,
+			featureEnabled: true,
+			expectError:    "",
+		},
+		"delete disallowed, enabled": {
+			operation:      admission.Delete,
+			options:        &metav1.DeleteOptions{},
+			oldObj:         sliceOtherNode,
+			featureEnabled: true,
+			expectError:    deleteErr,
+		},
+		"delete allowed, disabled": {
+			operation:      admission.Delete,
+			options:        &metav1.DeleteOptions{},
+			oldObj:         sliceNode,
+			featureEnabled: false,
+			expectError:    "",
+		},
+		"delete disallowed, disabled": {
+			operation:      admission.Delete,
+			options:        &metav1.DeleteOptions{},
+			oldObj:         sliceOtherNode,
+			featureEnabled: false,
+			expectError:    deleteErr,
+		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
 			attributes := admission.NewAttributesRecord(
-				test.obj, nil, schema.GroupVersionKind{},
+				test.obj, test.oldObj, schema.GroupVersionKind{},
-				"", "foo", apiResource, "", test.operation, &metav1.CreateOptions{}, false, mynode)
+				"", "foo", apiResource, "", test.operation, test.options, false, mynode)
 			featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.DynamicResourceAllocation, test.featureEnabled)
 			a := &admitTestCase{
 				name:       name,

plugin/pkg/auth/authorizer/node/node_authorizer.go

@@ -309,30 +309,34 @@ func (r *NodeAuthorizer) authorizeResourceSlice(nodeName string, attrs authorize
 		return authorizer.DecisionNoOpinion, "cannot authorize ResourceSlice subresources", nil
 	}
 
-	// allowed verbs: get, create, update, patch, delete
+	// allowed verbs: get, create, update, patch, delete, watch, list, deletecollection
 	verb := attrs.GetVerb()
 	switch verb {
-	case "get", "create", "update", "patch", "delete":
+	case "create":
-		// Okay, but check individual object permission below.
-	case "watch", "list":
-		// Okay. The kubelet is trusted to use a filter for its own objects.
-		return authorizer.DecisionAllow, "", nil
-	default:
-		klog.V(2).Infof("NODE DENY: '%s' %#v", nodeName, attrs)
-		return authorizer.DecisionNoOpinion, "can only get, create, update, patch, or delete a ResourceSlice", nil
-	}
-
 		// The request must come from a node with the same name as the ResourceSlice.NodeName field.
 		//
 		// For create, the noderestriction admission plugin is performing this check.
 		// Here we don't have access to the content of the new object.
-	if verb == "create" {
 		return authorizer.DecisionAllow, "", nil
-	}
+	case "get", "update", "patch", "delete":
-
+		// Checking the existing object must have established that access
-	// For any other verb, checking the existing object must have established that access
 		// is allowed by recording a graph edge.
 		return r.authorize(nodeName, sliceVertexType, attrs)
+	case "watch", "list", "deletecollection":
+		// Okay. The kubelet is trusted to use a filter for its own objects in watch and list.
+		// The NodeRestriction admission plugin (plugin/pkg/admission/noderestriction)
+		// ensures that the node is not deleting some ResourceSlice belonging to
+		// some other node.
+		//
+		// TODO (https://github.com/kubernetes/kubernetes/issues/125355):
+		// Once https://github.com/kubernetes/enhancements/pull/4600 is implemented,
+		// this code needs to be extended to verify that the node filter is indeed set.
+		// Then the admission check can be removed.
+		return authorizer.DecisionAllow, "", nil
+	default:
+		klog.V(2).Infof("NODE DENY: '%s' %#v", nodeName, attrs)
+		return authorizer.DecisionNoOpinion, "only the following verbs are allowed for a ResourceSlice: get, watch, list, create, update, patch, delete, deletecollection", nil
+	}
 }
 
 // hasPathFrom returns true if there is a directed path from the specified type/namespace/name to the specified Node

test/integration/auth/node_test.go

@@ -41,6 +41,7 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/test/integration/framework"
 	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 )
 
 func TestNodeAuthorizer(t *testing.T) {
@@ -113,6 +114,13 @@ func TestNodeAuthorizer(t *testing.T) {
 	if _, err := superuserClient.ResourceV1alpha2().ResourceClaims("ns").Create(context.TODO(), &v1alpha2.ResourceClaim{ObjectMeta: metav1.ObjectMeta{Name: "mytemplatizedresourceclaim"}, Spec: v1alpha2.ResourceClaimSpec{ResourceClassName: "example.com"}}, metav1.CreateOptions{}); err != nil {
 		t.Fatal(err)
 	}
+	model := v1alpha2.ResourceModel{NamedResources: &v1alpha2.NamedResourcesResources{}}
+	if _, err := superuserClient.ResourceV1alpha2().ResourceSlices().Create(context.TODO(), &v1alpha2.ResourceSlice{ObjectMeta: metav1.ObjectMeta{Name: "myslice1"}, NodeName: "node1", DriverName: "dra.example.com", ResourceModel: model}, metav1.CreateOptions{}); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := superuserClient.ResourceV1alpha2().ResourceSlices().Create(context.TODO(), &v1alpha2.ResourceSlice{ObjectMeta: metav1.ObjectMeta{Name: "myslice2"}, NodeName: "node2", DriverName: "dra.example.com", ResourceModel: model}, metav1.CreateOptions{}); err != nil {
+		t.Fatal(err)
+	}
 
 	pvName := "mypv"
 	if _, err := superuserClientExternal.StorageV1().VolumeAttachments().Create(context.TODO(), &storagev1.VolumeAttachment{
@@ -195,6 +203,15 @@ func TestNodeAuthorizer(t *testing.T) {
 			return err
 		}
 	}
+	deleteResourceSliceCollection := func(client clientset.Interface, nodeName *string) func() error {
+		return func() error {
+			var listOptions metav1.ListOptions
+			if nodeName != nil {
+				listOptions.FieldSelector = "nodeName=" + *nodeName
+			}
+			return client.ResourceV1alpha2().ResourceSlices().DeleteCollection(context.TODO(), metav1.DeleteOptions{}, listOptions)
+		}
+	}
 	addResourceClaimTemplateReference := func(client clientset.Interface) func() error {
 		return func() error {
 			_, err := client.CoreV1().Pods("ns").Patch(context.TODO(), "node2normalpod", types.MergePatchType,
@@ -640,6 +657,32 @@ func TestNodeAuthorizer(t *testing.T) {
 	expectForbidden(t, updateNode1CSINode(csiNode2Client))
 	expectForbidden(t, patchNode1CSINode(csiNode2Client))
 	expectForbidden(t, deleteNode1CSINode(csiNode2Client))
+
+	// Always allowed. Permission to delete specific objects is checked per object.
+	// Beware, this is destructive!
+	expectAllowed(t, deleteResourceSliceCollection(csiNode1Client, ptr.To("node1")))
+
+	// One slice must have been deleted, the other not.
+	slices, err := superuserClient.ResourceV1alpha2().ResourceSlices().List(context.TODO(), metav1.ListOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(slices.Items) != 1 {
+		t.Fatalf("unexpected slices: %v", slices.Items)
+	}
+	if slices.Items[0].NodeName != "node2" {
+		t.Fatal("wrong slice deleted")
+	}
+
+	// Superuser can delete.
+	expectAllowed(t, deleteResourceSliceCollection(superuserClient, nil))
+	slices, err = superuserClient.ResourceV1alpha2().ResourceSlices().List(context.TODO(), metav1.ListOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(slices.Items) != 0 {
+		t.Fatalf("unexpected slices: %v", slices.Items)
+	}
 }
 
 // expect executes a function a set number of times until it either returns the