diff --git a/plugin/pkg/admission/noderestriction/admission.go b/plugin/pkg/admission/noderestriction/admission.go index 9388ac52252..7f869d18bda 100644 --- a/plugin/pkg/admission/noderestriction/admission.go +++ b/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. - if a.GetOperation() == admission.Create { + // Same when deleting an object. + // + // 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 diff --git a/plugin/pkg/admission/noderestriction/admission_test.go b/plugin/pkg/admission/noderestriction/admission_test.go index d4eb6d8fef4..6044c33ae4a 100644 --- a/plugin/pkg/admission/noderestriction/admission_test.go +++ b/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{}, - "", "foo", apiResource, "", test.operation, &metav1.CreateOptions{}, false, mynode) + test.obj, test.oldObj, schema.GroupVersionKind{}, + "", "foo", apiResource, "", test.operation, test.options, false, mynode) featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.DynamicResourceAllocation, test.featureEnabled) a := &admitTestCase{ name: name, diff --git a/plugin/pkg/auth/authorizer/node/node_authorizer.go b/plugin/pkg/auth/authorizer/node/node_authorizer.go index d6b5c8d3037..28d76af2dca 100644 --- a/plugin/pkg/auth/authorizer/node/node_authorizer.go +++ b/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": - // Okay, but check individual object permission below. - case "watch", "list": - // Okay. The kubelet is trusted to use a filter for its own objects. + case "create": + // 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. + return authorizer.DecisionAllow, "", nil + case "get", "update", "patch", "delete": + // 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, "can only get, create, update, patch, or delete a ResourceSlice", nil + return authorizer.DecisionNoOpinion, "only the following verbs are allowed for a ResourceSlice: get, watch, list, create, update, patch, delete, deletecollection", 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 - } - - // 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) } // hasPathFrom returns true if there is a directed path from the specified type/namespace/name to the specified Node diff --git a/test/integration/auth/node_test.go b/test/integration/auth/node_test.go index 774ebd7e3ff..8c16b02813e 100644 --- a/test/integration/auth/node_test.go +++ b/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