From 0301e5a9f88eea45783acc228e4245b22a0b136e Mon Sep 17 00:00:00 2001 From: Rita Zhang Date: Mon, 30 Dec 2024 15:56:45 -0800 Subject: [PATCH] DRA: AdminAccess validate based on namespace label Signed-off-by: Rita Zhang --- pkg/apis/resource/types.go | 9 + .../resourceclaim/controller_test.go | 82 +++- pkg/controlplane/instance.go | 9 +- pkg/controlplane/instance_test.go | 2 +- .../resource/resourceclaim/storage/storage.go | 21 +- .../resourceclaim/storage/storage_test.go | 5 +- .../resource/resourceclaim/strategy.go | 66 ++- .../resource/resourceclaim/strategy_test.go | 444 ++++++++++++++++-- .../resourceclaimtemplate/storage/storage.go | 15 +- .../storage/storage_test.go | 5 +- .../resourceclaimtemplate/strategy.go | 36 +- .../resourceclaimtemplate/strategy_test.go | 189 +++++++- .../resource/rest/storage_resource.go | 21 +- pkg/registry/resource/utils.go | 99 ++++ .../src/k8s.io/api/resource/v1alpha3/types.go | 9 + .../src/k8s.io/api/resource/v1beta1/types.go | 9 + test/e2e/dra/dra.go | 40 +- test/integration/dra/dra_test.go | 44 +- 18 files changed, 927 insertions(+), 178 deletions(-) create mode 100644 pkg/registry/resource/utils.go diff --git a/pkg/apis/resource/types.go b/pkg/apis/resource/types.go index c2cb135a36d..49e9888af52 100644 --- a/pkg/apis/resource/types.go +++ b/pkg/apis/resource/types.go @@ -385,6 +385,15 @@ const ( DeviceConfigMaxSize = 32 ) +// DRAAdminNamespaceLabelKey is a label key used to grant administrative access +// to certain resource.k8s.io API types within a namespace. When this label is +// set on a namespace with the value "true" (case-sensitive), it allows the use +// of adminAccess: true in any namespaced resource.k8s.io API types. Currently, +// this permission applies to ResourceClaim and ResourceClaimTemplate objects. +const ( + DRAAdminNamespaceLabelKey = "resource.k8s.io/admin-access" +) + // DeviceRequest is a request for devices required for a claim. // This is typically a request for a single resource like a device, but can // also ask for several identical devices. diff --git a/pkg/controller/resourceclaim/controller_test.go b/pkg/controller/resourceclaim/controller_test.go index 8aa5d5316ea..88704b60fc0 100644 --- a/pkg/controller/resourceclaim/controller_test.go +++ b/pkg/controller/resourceclaim/controller_test.go @@ -40,6 +40,7 @@ import ( "k8s.io/kubernetes/pkg/controller" "k8s.io/kubernetes/pkg/controller/resourceclaim/metrics" "k8s.io/kubernetes/test/utils/ktesting" + "k8s.io/utils/ptr" ) var ( @@ -61,13 +62,15 @@ var ( testClaimReserved = reserveClaim(testClaimAllocated, testPodWithResource) testClaimReservedTwice = reserveClaim(testClaimReserved, otherTestPod) - generatedTestClaim = makeGeneratedClaim(podResourceClaimName, testPodName+"-"+podResourceClaimName+"-", testNamespace, className, 1, makeOwnerReference(testPodWithResource, true)) + generatedTestClaim = makeGeneratedClaim(podResourceClaimName, testPodName+"-"+podResourceClaimName+"-", testNamespace, className, 1, makeOwnerReference(testPodWithResource, true), nil) + generatedTestClaimWithAdmin = makeGeneratedClaim(podResourceClaimName, testPodName+"-"+podResourceClaimName+"-", testNamespace, className, 1, makeOwnerReference(testPodWithResource, true), ptr.To(true)) generatedTestClaimAllocated = allocateClaim(generatedTestClaim) generatedTestClaimReserved = reserveClaim(generatedTestClaimAllocated, testPodWithResource) - conflictingClaim = makeClaim(testPodName+"-"+podResourceClaimName, testNamespace, className, nil) - otherNamespaceClaim = makeClaim(testPodName+"-"+podResourceClaimName, otherNamespace, className, nil) - template = makeTemplate(templateName, testNamespace, className) + conflictingClaim = makeClaim(testPodName+"-"+podResourceClaimName, testNamespace, className, nil) + otherNamespaceClaim = makeClaim(testPodName+"-"+podResourceClaimName, otherNamespace, className, nil) + template = makeTemplate(templateName, testNamespace, className, nil) + templateWithAdminAccess = makeTemplate(templateName, testNamespace, className, ptr.To(true)) testPodWithNodeName = func() *v1.Pod { pod := testPodWithResource.DeepCopy() @@ -78,6 +81,7 @@ var ( }) return pod }() + adminAccessFeatureOffError = "admin access is requested, but the feature is disabled" ) func TestSyncHandler(t *testing.T) { @@ -93,7 +97,7 @@ func TestSyncHandler(t *testing.T) { templates []*resourceapi.ResourceClaimTemplate expectedClaims []resourceapi.ResourceClaim expectedStatuses map[string][]v1.PodResourceClaimStatus - expectedError bool + expectedError string expectedMetrics expectedMetrics }{ { @@ -109,6 +113,27 @@ func TestSyncHandler(t *testing.T) { }, expectedMetrics: expectedMetrics{1, 0}, }, + { + name: "create with admin and feature gate off", + pods: []*v1.Pod{testPodWithResource}, + templates: []*resourceapi.ResourceClaimTemplate{templateWithAdminAccess}, + key: podKey(testPodWithResource), + expectedError: adminAccessFeatureOffError, + }, + { + name: "create with admin and feature gate on", + pods: []*v1.Pod{testPodWithResource}, + templates: []*resourceapi.ResourceClaimTemplate{templateWithAdminAccess}, + key: podKey(testPodWithResource), + expectedClaims: []resourceapi.ResourceClaim{*generatedTestClaimWithAdmin}, + expectedStatuses: map[string][]v1.PodResourceClaimStatus{ + testPodWithResource.Name: { + {Name: testPodWithResource.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaimWithAdmin.Name}, + }, + }, + adminAccessEnabled: true, + expectedMetrics: expectedMetrics{1, 0}, + }, { name: "nop", pods: []*v1.Pod{func() *v1.Pod { @@ -153,7 +178,7 @@ func TestSyncHandler(t *testing.T) { pods: []*v1.Pod{testPodWithResource}, templates: nil, key: podKey(testPodWithResource), - expectedError: true, + expectedError: "resource claim template \"my-template\": resourceclaimtemplate.resource.k8s.io \"my-template\" not found", }, { name: "find-existing-claim-by-label", @@ -219,7 +244,7 @@ func TestSyncHandler(t *testing.T) { key: podKey(testPodWithResource), claims: []*resourceapi.ResourceClaim{conflictingClaim}, expectedClaims: []resourceapi.ResourceClaim{*conflictingClaim}, - expectedError: true, + expectedError: "resource claim template \"my-template\": resourceclaimtemplate.resource.k8s.io \"my-template\" not found", }, { name: "create-conflict", @@ -227,7 +252,7 @@ func TestSyncHandler(t *testing.T) { templates: []*resourceapi.ResourceClaimTemplate{template}, key: podKey(testPodWithResource), expectedMetrics: expectedMetrics{1, 1}, - expectedError: true, + expectedError: "create ResourceClaim : Operation cannot be fulfilled on resourceclaims.resource.k8s.io \"fake name\": fake conflict", }, { name: "stay-reserved-seen", @@ -424,11 +449,12 @@ func TestSyncHandler(t *testing.T) { } err = ec.syncHandler(tCtx, tc.key) - if err != nil && !tc.expectedError { - t.Fatalf("unexpected error while running handler: %v", err) + if err != nil { + assert.ErrorContains(t, err, tc.expectedError, "the error message should have contained the expected error message") + return } - if err == nil && tc.expectedError { - t.Fatalf("unexpected success") + if tc.expectedError != "" { + t.Fatalf("expected error, got none") } claims, err := fakeKubeClient.ResourceV1beta1().ResourceClaims("").List(tCtx, metav1.ListOptions{}) @@ -558,7 +584,7 @@ func makeClaim(name, namespace, classname string, owner *metav1.OwnerReference) return claim } -func makeGeneratedClaim(podClaimName, generateName, namespace, classname string, createCounter int, owner *metav1.OwnerReference) *resourceapi.ResourceClaim { +func makeGeneratedClaim(podClaimName, generateName, namespace, classname string, createCounter int, owner *metav1.OwnerReference, adminAccess *bool) *resourceapi.ResourceClaim { claim := &resourceapi.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s-%d", generateName, createCounter), @@ -570,6 +596,19 @@ func makeGeneratedClaim(podClaimName, generateName, namespace, classname string, if owner != nil { claim.OwnerReferences = []metav1.OwnerReference{*owner} } + if adminAccess != nil { + claim.Spec = resourceapi.ResourceClaimSpec{ + Devices: resourceapi.DeviceClaim{ + Requests: []resourceapi.DeviceRequest{ + { + Name: "req-0", + DeviceClassName: "class", + AdminAccess: adminAccess, + }, + }, + }, + } + } return claim } @@ -618,10 +657,25 @@ func makePod(name, namespace string, uid types.UID, podClaims ...v1.PodResourceC return pod } -func makeTemplate(name, namespace, classname string) *resourceapi.ResourceClaimTemplate { +func makeTemplate(name, namespace, classname string, adminAccess *bool) *resourceapi.ResourceClaimTemplate { template := &resourceapi.ResourceClaimTemplate{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, } + if adminAccess != nil { + template.Spec = resourceapi.ResourceClaimTemplateSpec{ + Spec: resourceapi.ResourceClaimSpec{ + Devices: resourceapi.DeviceClaim{ + Requests: []resourceapi.DeviceRequest{ + { + Name: "req-0", + DeviceClassName: "class", + AdminAccess: adminAccess, + }, + }, + }, + }, + } + } return template } diff --git a/pkg/controlplane/instance.go b/pkg/controlplane/instance.go index faf0b01219c..5b8e74965b3 100644 --- a/pkg/controlplane/instance.go +++ b/pkg/controlplane/instance.go @@ -63,7 +63,6 @@ import ( genericapiserver "k8s.io/apiserver/pkg/server" serverstorage "k8s.io/apiserver/pkg/server/storage" utilfeature "k8s.io/apiserver/pkg/util/feature" - clientdiscovery "k8s.io/client-go/discovery" "k8s.io/client-go/kubernetes" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" discoveryclient "k8s.io/client-go/kubernetes/typed/discovery/v1" @@ -328,7 +327,7 @@ func (c CompletedConfig) New(delegationTarget genericapiserver.DelegationTarget) return nil, err } - restStorageProviders, err := c.StorageProviders(client.Discovery()) + restStorageProviders, err := c.StorageProviders(client) if err != nil { return nil, err } @@ -379,7 +378,7 @@ func (c CompletedConfig) New(delegationTarget genericapiserver.DelegationTarget) } -func (c CompletedConfig) StorageProviders(discovery clientdiscovery.DiscoveryInterface) ([]controlplaneapiserver.RESTStorageProvider, error) { +func (c CompletedConfig) StorageProviders(client *kubernetes.Clientset) ([]controlplaneapiserver.RESTStorageProvider, error) { legacyRESTStorageProvider, err := corerest.New(corerest.Config{ GenericConfig: *c.ControlPlane.NewCoreGenericConfig(), Proxy: corerest.ProxyConfig{ @@ -425,9 +424,9 @@ func (c CompletedConfig) StorageProviders(discovery clientdiscovery.DiscoveryInt // keep apps after extensions so legacy clients resolve the extensions versions of shared resource names. // See https://github.com/kubernetes/kubernetes/issues/42392 appsrest.StorageProvider{}, - admissionregistrationrest.RESTStorageProvider{Authorizer: c.ControlPlane.Generic.Authorization.Authorizer, DiscoveryClient: discovery}, + admissionregistrationrest.RESTStorageProvider{Authorizer: c.ControlPlane.Generic.Authorization.Authorizer, DiscoveryClient: client.Discovery()}, eventsrest.RESTStorageProvider{TTL: c.ControlPlane.EventTTL}, - resourcerest.RESTStorageProvider{}, + resourcerest.RESTStorageProvider{NamespaceClient: client.CoreV1().Namespaces()}, }, nil } diff --git a/pkg/controlplane/instance_test.go b/pkg/controlplane/instance_test.go index 7c84959a1c8..e5c4da2868f 100644 --- a/pkg/controlplane/instance_test.go +++ b/pkg/controlplane/instance_test.go @@ -490,7 +490,7 @@ func TestGenericStorageProviders(t *testing.T) { if err != nil { t.Fatal(err) } - kube, err := completed.StorageProviders(client.Discovery()) + kube, err := completed.StorageProviders(client) if err != nil { t.Fatal(err) } diff --git a/pkg/registry/resource/resourceclaim/storage/storage.go b/pkg/registry/resource/resourceclaim/storage/storage.go index 5784c6e309c..d4186d5cbef 100644 --- a/pkg/registry/resource/resourceclaim/storage/storage.go +++ b/pkg/registry/resource/resourceclaim/storage/storage.go @@ -18,12 +18,14 @@ package storage import ( "context" + "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/printers" printersinternal "k8s.io/kubernetes/pkg/printers/internalversion" @@ -38,7 +40,11 @@ type REST struct { } // NewREST returns a RESTStorage object that will work against ResourceClaims. -func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) { +func NewREST(optsGetter generic.RESTOptionsGetter, nsClient v1.NamespaceInterface) (*REST, *StatusREST, error) { + if nsClient == nil { + return nil, nil, fmt.Errorf("namespace client is required") + } + strategy := resourceclaim.NewStrategy(nsClient) store := &genericregistry.Store{ NewFunc: func() runtime.Object { return &resource.ResourceClaim{} }, NewListFunc: func() runtime.Object { return &resource.ResourceClaimList{} }, @@ -46,11 +52,11 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) { DefaultQualifiedResource: resource.Resource("resourceclaims"), SingularQualifiedResource: resource.Resource("resourceclaim"), - CreateStrategy: resourceclaim.Strategy, - UpdateStrategy: resourceclaim.Strategy, - DeleteStrategy: resourceclaim.Strategy, + CreateStrategy: strategy, + UpdateStrategy: strategy, + DeleteStrategy: strategy, ReturnDeletedObject: true, - ResetFieldsStrategy: resourceclaim.Strategy, + ResetFieldsStrategy: strategy, TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)}, } @@ -60,8 +66,9 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) { } statusStore := *store - statusStore.UpdateStrategy = resourceclaim.StatusStrategy - statusStore.ResetFieldsStrategy = resourceclaim.StatusStrategy + statusStrategy := resourceclaim.NewStatusStrategy(strategy) + statusStore.UpdateStrategy = statusStrategy + statusStore.ResetFieldsStrategy = statusStrategy rest := &REST{store} diff --git a/pkg/registry/resource/resourceclaim/storage/storage_test.go b/pkg/registry/resource/resourceclaim/storage/storage_test.go index 031aa8c95e7..83fc7a8cdfe 100644 --- a/pkg/registry/resource/resourceclaim/storage/storage_test.go +++ b/pkg/registry/resource/resourceclaim/storage/storage_test.go @@ -30,6 +30,7 @@ import ( genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" "k8s.io/apiserver/pkg/registry/rest" etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing" + "k8s.io/client-go/kubernetes/fake" "k8s.io/kubernetes/pkg/apis/resource" _ "k8s.io/kubernetes/pkg/apis/resource/install" "k8s.io/kubernetes/pkg/registry/registrytest" @@ -43,7 +44,9 @@ func newStorage(t *testing.T) (*REST, *StatusREST, *etcd3testing.EtcdTestServer) DeleteCollectionWorkers: 1, ResourcePrefix: "resourceclaims", } - resourceClaimStorage, statusStorage, err := NewREST(restOptions) + fakeClient := fake.NewSimpleClientset() + mockNSClient := fakeClient.CoreV1().Namespaces() + resourceClaimStorage, statusStorage, err := NewREST(restOptions, mockNSClient) if err != nil { t.Fatalf("unexpected error from REST storage: %v", err) } diff --git a/pkg/registry/resource/resourceclaim/strategy.go b/pkg/registry/resource/resourceclaim/strategy.go index cb770b2a6e1..75aeb494f78 100644 --- a/pkg/registry/resource/resourceclaim/strategy.go +++ b/pkg/registry/resource/resourceclaim/strategy.go @@ -30,11 +30,13 @@ import ( "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/dynamic-resource-allocation/structured" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/apis/resource/validation" "k8s.io/kubernetes/pkg/features" + resourceutils "k8s.io/kubernetes/pkg/registry/resource" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) @@ -42,20 +44,26 @@ import ( type resourceclaimStrategy struct { runtime.ObjectTyper names.NameGenerator + nsClient v1.NamespaceInterface } -// Strategy is the default logic that applies when creating and updating -// ResourceClaim objects via the REST API. -var Strategy = resourceclaimStrategy{legacyscheme.Scheme, names.SimpleNameGenerator} +// NewStrategy is the default logic that applies when creating and updating ResourceClaim objects. +func NewStrategy(nsClient v1.NamespaceInterface) *resourceclaimStrategy { + return &resourceclaimStrategy{ + legacyscheme.Scheme, + names.SimpleNameGenerator, + nsClient, + } +} -func (resourceclaimStrategy) NamespaceScoped() bool { +func (*resourceclaimStrategy) NamespaceScoped() bool { return true } // GetResetFields returns the set of fields that get reset by the strategy and // should not be modified by the user. For a new ResourceClaim that is the // status. -func (resourceclaimStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { +func (*resourceclaimStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { fields := map[fieldpath.APIVersion]*fieldpath.Set{ "resource.k8s.io/v1alpha3": fieldpath.NewSet( fieldpath.MakePathOrDie("status"), @@ -68,7 +76,7 @@ func (resourceclaimStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpat return fields } -func (resourceclaimStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { +func (*resourceclaimStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { claim := obj.(*resource.ResourceClaim) // Status must not be set by user on create. claim.Status = resource.ResourceClaimStatus{} @@ -76,23 +84,25 @@ func (resourceclaimStrategy) PrepareForCreate(ctx context.Context, obj runtime.O dropDisabledFields(claim, nil) } -func (resourceclaimStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { +func (s *resourceclaimStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { claim := obj.(*resource.ResourceClaim) - return validation.ValidateResourceClaim(claim) + + allErrs := resourceutils.AuthorizedForAdmin(ctx, claim.Spec.Devices.Requests, claim.Namespace, s.nsClient) + return append(allErrs, validation.ValidateResourceClaim(claim)...) } -func (resourceclaimStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { +func (*resourceclaimStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { return nil } -func (resourceclaimStrategy) Canonicalize(obj runtime.Object) { +func (*resourceclaimStrategy) Canonicalize(obj runtime.Object) { } -func (resourceclaimStrategy) AllowCreateOnUpdate() bool { +func (*resourceclaimStrategy) AllowCreateOnUpdate() bool { return false } -func (resourceclaimStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { +func (*resourceclaimStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { newClaim := obj.(*resource.ResourceClaim) oldClaim := old.(*resource.ResourceClaim) newClaim.Status = oldClaim.Status @@ -100,30 +110,34 @@ func (resourceclaimStrategy) PrepareForUpdate(ctx context.Context, obj, old runt dropDisabledFields(newClaim, oldClaim) } -func (resourceclaimStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { +func (s *resourceclaimStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { newClaim := obj.(*resource.ResourceClaim) oldClaim := old.(*resource.ResourceClaim) + // AuthorizedForAdmin isn't needed here because the spec is immutable. errorList := validation.ValidateResourceClaim(newClaim) return append(errorList, validation.ValidateResourceClaimUpdate(newClaim, oldClaim)...) } -func (resourceclaimStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { +func (*resourceclaimStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { return nil } -func (resourceclaimStrategy) AllowUnconditionalUpdate() bool { +func (*resourceclaimStrategy) AllowUnconditionalUpdate() bool { return true } type resourceclaimStatusStrategy struct { - resourceclaimStrategy + *resourceclaimStrategy } -var StatusStrategy = resourceclaimStatusStrategy{Strategy} +// NewStatusStrategy creates a strategy for operating the status object. +func NewStatusStrategy(resourceclaimStrategy *resourceclaimStrategy) *resourceclaimStatusStrategy { + return &resourceclaimStatusStrategy{resourceclaimStrategy} +} // GetResetFields returns the set of fields that get reset by the strategy and // should not be modified by the user. For a status update that is the spec. -func (resourceclaimStatusStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { +func (*resourceclaimStatusStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { fields := map[fieldpath.APIVersion]*fieldpath.Set{ "resource.k8s.io/v1alpha3": fieldpath.NewSet( fieldpath.MakePathOrDie("spec"), @@ -136,7 +150,7 @@ func (resourceclaimStatusStrategy) GetResetFields() map[fieldpath.APIVersion]*fi return fields } -func (resourceclaimStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { +func (*resourceclaimStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { newClaim := obj.(*resource.ResourceClaim) oldClaim := old.(*resource.ResourceClaim) newClaim.Spec = oldClaim.Spec @@ -146,14 +160,22 @@ func (resourceclaimStatusStrategy) PrepareForUpdate(ctx context.Context, obj, ol dropDisabledFields(newClaim, oldClaim) } -func (resourceclaimStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { +func (r *resourceclaimStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { newClaim := obj.(*resource.ResourceClaim) oldClaim := old.(*resource.ResourceClaim) - return validation.ValidateResourceClaimStatusUpdate(newClaim, oldClaim) + var newAllocationResult, oldAllocationResult []resource.DeviceRequestAllocationResult + if newClaim.Status.Allocation != nil { + newAllocationResult = newClaim.Status.Allocation.Devices.Results + } + if oldClaim.Status.Allocation != nil { + oldAllocationResult = oldClaim.Status.Allocation.Devices.Results + } + allErrs := resourceutils.AuthorizedForAdminStatus(ctx, newAllocationResult, oldAllocationResult, newClaim.Namespace, r.nsClient) + return append(allErrs, validation.ValidateResourceClaimStatusUpdate(newClaim, oldClaim)...) } // WarningsOnUpdate returns warnings for the given update. -func (resourceclaimStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { +func (*resourceclaimStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { return nil } diff --git a/pkg/registry/resource/resourceclaim/strategy_test.go b/pkg/registry/resource/resourceclaim/strategy_test.go index fb855f0bf96..cefd0716190 100644 --- a/pkg/registry/resource/resourceclaim/strategy_test.go +++ b/pkg/registry/resource/resourceclaim/strategy_test.go @@ -21,9 +21,12 @@ import ( "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/kubernetes/fake" + testclient "k8s.io/client-go/testing" featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/features" @@ -33,7 +36,7 @@ import ( var obj = &resource.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim", - Namespace: "default", + Namespace: "kube-system", }, Spec: resource.ResourceClaimSpec{ Devices: resource.DeviceClaim{ @@ -51,7 +54,7 @@ var obj = &resource.ResourceClaim{ var objWithStatus = &resource.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim", - Namespace: "default", + Namespace: "kube-system", }, Spec: resource.ResourceClaimSpec{ Devices: resource.DeviceClaim{ @@ -81,6 +84,43 @@ var objWithStatus = &resource.ResourceClaim{ } var objWithAdminAccess = &resource.ResourceClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim", + Namespace: "kube-system", + }, + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + AdminAccess: ptr.To(true), + }, + }, + }, + }, +} + +var objInNonAdminNamespace = &resource.ResourceClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim", + Namespace: "default", + }, + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + }, + }, + }, + }, +} + +var objWithAdminAccessInNonAdminNamespace = &resource.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim", Namespace: "default", @@ -99,7 +139,38 @@ var objWithAdminAccess = &resource.ResourceClaim{ }, } -var objWithAdminAccessStatus = &resource.ResourceClaim{ +var objStatusInNonAdminNamespace = &resource.ResourceClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim", + Namespace: "default", + }, + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + }, + }, + }, + }, + Status: resource.ResourceClaimStatus{ + Allocation: &resource.AllocationResult{ + Devices: resource.DeviceAllocationResult{ + Results: []resource.DeviceRequestAllocationResult{ + { + Request: "req-0", + Driver: "dra.example.com", + Pool: "pool-0", + Device: "device-0", + }, + }, + }, + }, + }, +} +var objWithAdminAccessStatusInNonAdminNamespace = &resource.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim", Namespace: "default", @@ -111,7 +182,6 @@ var objWithAdminAccessStatus = &resource.ResourceClaim{ Name: "req-0", DeviceClassName: "class", AllocationMode: resource.DeviceAllocationModeAll, - AdminAccess: ptr.To(true), }, }, }, @@ -157,6 +227,58 @@ var objWithPrioritizedList = &resource.ResourceClaim{ }, } +var objWithAdminAccessStatus = &resource.ResourceClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim", + Namespace: "kube-system", + }, + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + AdminAccess: ptr.To(true), + }, + }, + }, + }, + Status: resource.ResourceClaimStatus{ + Allocation: &resource.AllocationResult{ + Devices: resource.DeviceAllocationResult{ + Results: []resource.DeviceRequestAllocationResult{ + { + Request: "req-0", + Driver: "dra.example.com", + Pool: "pool-0", + Device: "device-0", + AdminAccess: ptr.To(true), + }, + }, + }, + }, + }, +} + +var ns1 = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Labels: map[string]string{"key": "value"}, + }, +} +var ns2 = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kube-system", + Labels: map[string]string{resource.DRAAdminNamespaceLabelKey: "true"}, + }, +} + +var adminAccessError = "Forbidden: admin access to devices requires the `resource.k8s.io/admin-access: true` label" +var fieldImmutableError = "field is immutable" +var metadataError = "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters" +var deviceRequestError = "exactly one of `deviceClassName` or `firstAvailable` must be specified" + const ( testRequest = "test-request" testDriver = "test-driver" @@ -165,27 +287,35 @@ const ( ) func TestStrategy(t *testing.T) { - if !Strategy.NamespaceScoped() { + fakeClient := fake.NewSimpleClientset() + mockNSClient := fakeClient.CoreV1().Namespaces() + strategy := NewStrategy(mockNSClient) + if !strategy.NamespaceScoped() { t.Errorf("ResourceClaim must be namespace scoped") } - if Strategy.AllowCreateOnUpdate() { + if strategy.AllowCreateOnUpdate() { t.Errorf("ResourceClaim should not allow create on update") } } func TestStrategyCreate(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - testcases := map[string]struct { obj *resource.ResourceClaim adminAccess bool prioritizedList bool - expectValidationError bool + expectValidationError string expectObj *resource.ResourceClaim + verify func(*testing.T, []testclient.Action) }{ "simple": { obj: obj, expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "validation-error": { obj: func() *resource.ResourceClaim { @@ -193,69 +323,139 @@ func TestStrategyCreate(t *testing.T) { obj.Name = "%#@$%$" return obj }(), - expectValidationError: true, + expectValidationError: metadataError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-fields-admin-access": { obj: objWithAdminAccess, adminAccess: false, expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-admin-access": { obj: objWithAdminAccess, adminAccess: true, expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "kube-system" { + t.Errorf("expected to get the kube-system namespace but got '%s'", ns) + } + }, }, "drop-fields-prioritized-list": { obj: objWithPrioritizedList, prioritizedList: false, - expectValidationError: true, + expectValidationError: deviceRequestError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-prioritized-list": { obj: objWithPrioritizedList, prioritizedList: true, expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "admin-access-admin-namespace": { + obj: objWithAdminAccess, + adminAccess: true, + expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "kube-system" { + t.Errorf("expected to get the kube-system namespace but got '%s'", ns) + } + }, + }, + "admin-access-non-admin-namespace": { + obj: objWithAdminAccessInNonAdminNamespace, + adminAccess: true, + expectObj: objWithAdminAccessInNonAdminNamespace, + expectValidationError: adminAccessError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "default" { + t.Errorf("expected to get the default namespace but got '%s'", ns) + } + }, }, } for name, tc := range testcases { t.Run(name, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAAdminAccess, tc.adminAccess) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAPrioritizedList, tc.prioritizedList) + strategy := NewStrategy(mockNSClient) obj := tc.obj.DeepCopy() - Strategy.PrepareForCreate(ctx, obj) - if errs := Strategy.Validate(ctx, obj); len(errs) != 0 { - if !tc.expectValidationError { - t.Fatalf("unexpected validation errors: %q", errs) - } + strategy.PrepareForCreate(ctx, obj) + if errs := strategy.Validate(ctx, obj); len(errs) != 0 { + assert.ErrorContains(t, errs[0], tc.expectValidationError, "the error message should have contained the expected error message") return - } else if tc.expectValidationError { + } + if tc.expectValidationError != "" { t.Fatal("expected validation error(s), got none") } - if warnings := Strategy.WarningsOnCreate(ctx, obj); len(warnings) != 0 { + if warnings := strategy.WarningsOnCreate(ctx, obj); len(warnings) != 0 { t.Fatalf("unexpected warnings: %q", warnings) } - Strategy.Canonicalize(obj) + strategy.Canonicalize(obj) assert.Equal(t, tc.expectObj, obj) + tc.verify(t, fakeClient.Actions()) }) } } func TestStrategyUpdate(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - testcases := map[string]struct { oldObj *resource.ResourceClaim newObj *resource.ResourceClaim adminAccess bool + expectValidationError string prioritizedList bool - expectValidationError bool expectObj *resource.ResourceClaim + verify func(*testing.T, []testclient.Action) }{ "no-changes-okay": { oldObj: obj, newObj: obj, expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "name-change-not-allowed": { oldObj: obj, @@ -264,97 +464,167 @@ func TestStrategyUpdate(t *testing.T) { obj.Name += "-2" return obj }(), - expectValidationError: true, + expectValidationError: fieldImmutableError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-fields-admin-access": { oldObj: obj, newObj: objWithAdminAccess, adminAccess: false, expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-admin-access": { oldObj: obj, newObj: objWithAdminAccess, adminAccess: true, - expectValidationError: true, // Spec is immutable. + expectValidationError: fieldImmutableError, // Spec is immutable. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-existing-fields-admin-access": { oldObj: objWithAdminAccess, newObj: objWithAdminAccess, adminAccess: true, expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "admin-access-admin-namespace": { + oldObj: objWithAdminAccess, + newObj: objWithAdminAccess, + adminAccess: true, + expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "admin-access-non-admin-namespace": { + oldObj: objInNonAdminNamespace, + newObj: objWithAdminAccessInNonAdminNamespace, + adminAccess: true, + expectValidationError: fieldImmutableError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-fields-prioritized-list": { oldObj: obj, newObj: objWithPrioritizedList, prioritizedList: false, - expectValidationError: true, + expectValidationError: deviceRequestError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-prioritized-list": { oldObj: obj, newObj: objWithPrioritizedList, prioritizedList: true, - expectValidationError: true, // Spec is immutable. + expectValidationError: fieldImmutableError, // Spec is immutable. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-existing-fields-prioritized-list": { oldObj: objWithPrioritizedList, newObj: objWithPrioritizedList, prioritizedList: true, expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-existing-fields-prioritized-list-disabled-feature": { oldObj: objWithPrioritizedList, newObj: objWithPrioritizedList, prioritizedList: false, expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, } for name, tc := range testcases { t.Run(name, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAAdminAccess, tc.adminAccess) + strategy := NewStrategy(mockNSClient) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAPrioritizedList, tc.prioritizedList) oldObj := tc.oldObj.DeepCopy() newObj := tc.newObj.DeepCopy() newObj.ResourceVersion = "4" - Strategy.PrepareForUpdate(ctx, newObj, oldObj) - if errs := Strategy.ValidateUpdate(ctx, newObj, oldObj); len(errs) != 0 { - if !tc.expectValidationError { - t.Fatalf("unexpected validation errors: %q", errs) - } + strategy.PrepareForUpdate(ctx, newObj, oldObj) + if errs := strategy.ValidateUpdate(ctx, newObj, oldObj); len(errs) != 0 { + assert.ErrorContains(t, errs[0], tc.expectValidationError, "the error message should have contained the expected error message") return - } else if tc.expectValidationError { + } + if tc.expectValidationError != "" { t.Fatal("expected validation error(s), got none") } - if warnings := Strategy.WarningsOnUpdate(ctx, newObj, oldObj); len(warnings) != 0 { + if warnings := strategy.WarningsOnUpdate(ctx, newObj, oldObj); len(warnings) != 0 { t.Fatalf("unexpected warnings: %q", warnings) } - Strategy.Canonicalize(newObj) - + strategy.Canonicalize(newObj) expectObj := tc.expectObj.DeepCopy() expectObj.ResourceVersion = "4" assert.Equal(t, expectObj, newObj) + tc.verify(t, fakeClient.Actions()) }) } } func TestStatusStrategyUpdate(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - testcases := map[string]struct { oldObj *resource.ResourceClaim newObj *resource.ResourceClaim adminAccess bool deviceStatusFeatureGate bool - expectValidationError bool + expectValidationError string expectObj *resource.ResourceClaim + verify func(*testing.T, []testclient.Action) }{ "no-changes-okay": { oldObj: obj, newObj: obj, expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "name-change-not-allowed": { oldObj: obj, @@ -363,7 +633,12 @@ func TestStatusStrategyUpdate(t *testing.T) { obj.Name += "-2" return obj }(), - expectValidationError: true, + expectValidationError: fieldImmutableError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, // Cannot add finalizers, annotations and labels during status update. "drop-meta-changes": { @@ -376,12 +651,22 @@ func TestStatusStrategyUpdate(t *testing.T) { return obj }(), expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-fields-admin-access": { oldObj: obj, newObj: objWithAdminAccessStatus, adminAccess: false, expectObj: objWithStatus, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-admin-access": { oldObj: obj, @@ -393,12 +678,48 @@ func TestStatusStrategyUpdate(t *testing.T) { expectObj.Spec = obj.Spec return expectObj }(), + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "kube-system" { + t.Errorf("expected to get the kube-system namespace but got '%s'", ns) + } + }, + }, + "keep-fields-admin-access-NonAdminNamespace": { + oldObj: objStatusInNonAdminNamespace, + newObj: objWithAdminAccessStatusInNonAdminNamespace, + adminAccess: true, + expectValidationError: adminAccessError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "default" { + t.Errorf("expected to get the default namespace but got '%s'", ns) + } + }, }, "keep-fields-admin-access-because-of-spec": { oldObj: objWithAdminAccess, newObj: objWithAdminAccessStatus, adminAccess: false, expectObj: objWithAdminAccessStatus, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "kube-system" { + t.Errorf("expected to get the kube-system namespace but got '%s'", ns) + } + }, }, // Normally a claim without admin access in the spec shouldn't // have one in the status either, but it's not invalid and thus @@ -416,6 +737,11 @@ func TestStatusStrategyUpdate(t *testing.T) { oldObj.Spec.Devices.Requests[0].AdminAccess = ptr.To(false) return oldObj }(), + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-fields-devices-status": { oldObj: func() *resource.ResourceClaim { @@ -438,6 +764,11 @@ func TestStatusStrategyUpdate(t *testing.T) { addStatusAllocationDevicesResults(obj, testDriver, testPool, testDevice, testRequest) return obj }(), + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-devices-status-disable-feature-gate": { oldObj: func() *resource.ResourceClaim { @@ -462,6 +793,11 @@ func TestStatusStrategyUpdate(t *testing.T) { addStatusDevices(obj, testDriver, testPool, testDevice) return obj }(), + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-devices-status": { oldObj: func() *resource.ResourceClaim { @@ -485,6 +821,11 @@ func TestStatusStrategyUpdate(t *testing.T) { addStatusDevices(obj, testDriver, testPool, testDevice) return obj }(), + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-status-deallocated-device": { oldObj: func() *resource.ResourceClaim { @@ -506,6 +847,11 @@ func TestStatusStrategyUpdate(t *testing.T) { addSpecDevicesRequest(obj, testRequest) return obj }(), + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-status-deallocated-device-disable-feature-gate": { oldObj: func() *resource.ResourceClaim { @@ -527,35 +873,45 @@ func TestStatusStrategyUpdate(t *testing.T) { addSpecDevicesRequest(obj, testRequest) return obj }(), + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, } for name, tc := range testcases { t.Run(name, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() + strategy := NewStrategy(mockNSClient) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAAdminAccess, tc.adminAccess) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAResourceClaimDeviceStatus, tc.deviceStatusFeatureGate) + statusStrategy := NewStatusStrategy(strategy) oldObj := tc.oldObj.DeepCopy() newObj := tc.newObj.DeepCopy() newObj.ResourceVersion = "4" - StatusStrategy.PrepareForUpdate(ctx, newObj, oldObj) - if errs := StatusStrategy.ValidateUpdate(ctx, newObj, oldObj); len(errs) != 0 { - if !tc.expectValidationError { - t.Fatalf("unexpected validation errors: %q", errs) - } + statusStrategy.PrepareForUpdate(ctx, newObj, oldObj) + if errs := statusStrategy.ValidateUpdate(ctx, newObj, oldObj); len(errs) != 0 { + assert.ErrorContains(t, errs[0], tc.expectValidationError, "the error message should have contained the expected error message") return - } else if tc.expectValidationError { + } + if tc.expectValidationError != "" { t.Fatal("expected validation error(s), got none") } - if warnings := StatusStrategy.WarningsOnUpdate(ctx, newObj, oldObj); len(warnings) != 0 { + if warnings := statusStrategy.WarningsOnUpdate(ctx, newObj, oldObj); len(warnings) != 0 { t.Fatalf("unexpected warnings: %q", warnings) } - StatusStrategy.Canonicalize(newObj) + statusStrategy.Canonicalize(newObj) expectObj := tc.expectObj.DeepCopy() expectObj.ResourceVersion = "4" assert.Equal(t, expectObj, newObj) + tc.verify(t, fakeClient.Actions()) }) } } diff --git a/pkg/registry/resource/resourceclaimtemplate/storage/storage.go b/pkg/registry/resource/resourceclaimtemplate/storage/storage.go index f32851f9550..8dd1a5c5766 100644 --- a/pkg/registry/resource/resourceclaimtemplate/storage/storage.go +++ b/pkg/registry/resource/resourceclaimtemplate/storage/storage.go @@ -17,9 +17,12 @@ limitations under the License. package storage import ( + "fmt" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" + "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/printers" printersinternal "k8s.io/kubernetes/pkg/printers/internalversion" @@ -33,16 +36,20 @@ type REST struct { } // NewREST returns a RESTStorage object that will work against ResourceClass. -func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, error) { +func NewREST(optsGetter generic.RESTOptionsGetter, nsClient v1.NamespaceInterface) (*REST, error) { + if nsClient == nil { + return nil, fmt.Errorf("namespace client is required") + } + strategy := resourceclaimtemplate.NewStrategy(nsClient) store := &genericregistry.Store{ NewFunc: func() runtime.Object { return &resource.ResourceClaimTemplate{} }, NewListFunc: func() runtime.Object { return &resource.ResourceClaimTemplateList{} }, DefaultQualifiedResource: resource.Resource("resourceclaimtemplates"), SingularQualifiedResource: resource.Resource("resourceclaimtemplate"), - CreateStrategy: resourceclaimtemplate.Strategy, - UpdateStrategy: resourceclaimtemplate.Strategy, - DeleteStrategy: resourceclaimtemplate.Strategy, + CreateStrategy: strategy, + UpdateStrategy: strategy, + DeleteStrategy: strategy, ReturnDeletedObject: true, TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)}, diff --git a/pkg/registry/resource/resourceclaimtemplate/storage/storage_test.go b/pkg/registry/resource/resourceclaimtemplate/storage/storage_test.go index ab10d139bdc..ff572d3d6da 100644 --- a/pkg/registry/resource/resourceclaimtemplate/storage/storage_test.go +++ b/pkg/registry/resource/resourceclaimtemplate/storage/storage_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/apiserver/pkg/registry/generic" genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing" + "k8s.io/client-go/kubernetes/fake" "k8s.io/kubernetes/pkg/apis/resource" _ "k8s.io/kubernetes/pkg/apis/resource/install" "k8s.io/kubernetes/pkg/registry/registrytest" @@ -39,7 +40,9 @@ func newStorage(t *testing.T) (*REST, *etcd3testing.EtcdTestServer) { DeleteCollectionWorkers: 1, ResourcePrefix: "resourceclaimtemplates", } - resourceClaimTemplateStorage, err := NewREST(restOptions) + fakeClient := fake.NewSimpleClientset() + mockNSClient := fakeClient.CoreV1().Namespaces() + resourceClaimTemplateStorage, err := NewREST(restOptions, mockNSClient) if err != nil { t.Fatalf("unexpected error from REST storage: %v", err) } diff --git a/pkg/registry/resource/resourceclaimtemplate/strategy.go b/pkg/registry/resource/resourceclaimtemplate/strategy.go index 3fa78b2a7f0..547d3489c23 100644 --- a/pkg/registry/resource/resourceclaimtemplate/strategy.go +++ b/pkg/registry/resource/resourceclaimtemplate/strategy.go @@ -27,60 +27,72 @@ import ( "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/apis/resource/validation" "k8s.io/kubernetes/pkg/features" + resourceutils "k8s.io/kubernetes/pkg/registry/resource" ) // resourceClaimTemplateStrategy implements behavior for ResourceClaimTemplate objects type resourceClaimTemplateStrategy struct { runtime.ObjectTyper names.NameGenerator + nsClient v1.NamespaceInterface } -var Strategy = resourceClaimTemplateStrategy{legacyscheme.Scheme, names.SimpleNameGenerator} +// NewStrategy is the default logic that applies when creating and updating ResourceClaimTemplate objects. +func NewStrategy(nsClient v1.NamespaceInterface) *resourceClaimTemplateStrategy { + return &resourceClaimTemplateStrategy{ + legacyscheme.Scheme, + names.SimpleNameGenerator, + nsClient, + } +} -func (resourceClaimTemplateStrategy) NamespaceScoped() bool { +func (*resourceClaimTemplateStrategy) NamespaceScoped() bool { return true } -func (resourceClaimTemplateStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { +func (*resourceClaimTemplateStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { claimTemplate := obj.(*resource.ResourceClaimTemplate) dropDisabledFields(claimTemplate, nil) } -func (resourceClaimTemplateStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { +func (s *resourceClaimTemplateStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { resourceClaimTemplate := obj.(*resource.ResourceClaimTemplate) - return validation.ValidateResourceClaimTemplate(resourceClaimTemplate) + allErrs := resourceutils.AuthorizedForAdmin(ctx, resourceClaimTemplate.Spec.Spec.Devices.Requests, resourceClaimTemplate.Namespace, s.nsClient) + return append(allErrs, validation.ValidateResourceClaimTemplate(resourceClaimTemplate)...) } -func (resourceClaimTemplateStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { +func (*resourceClaimTemplateStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { return nil } -func (resourceClaimTemplateStrategy) Canonicalize(obj runtime.Object) { +func (*resourceClaimTemplateStrategy) Canonicalize(obj runtime.Object) { } -func (resourceClaimTemplateStrategy) AllowCreateOnUpdate() bool { +func (*resourceClaimTemplateStrategy) AllowCreateOnUpdate() bool { return false } -func (resourceClaimTemplateStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { +func (*resourceClaimTemplateStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { claimTemplate, oldClaimTemplate := obj.(*resource.ResourceClaimTemplate), old.(*resource.ResourceClaimTemplate) dropDisabledFields(claimTemplate, oldClaimTemplate) } -func (resourceClaimTemplateStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { +func (s *resourceClaimTemplateStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { + // AuthorizedForAdmin isn't needed here because the spec is immutable. errorList := validation.ValidateResourceClaimTemplate(obj.(*resource.ResourceClaimTemplate)) return append(errorList, validation.ValidateResourceClaimTemplateUpdate(obj.(*resource.ResourceClaimTemplate), old.(*resource.ResourceClaimTemplate))...) } -func (resourceClaimTemplateStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { +func (*resourceClaimTemplateStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { return nil } -func (resourceClaimTemplateStrategy) AllowUnconditionalUpdate() bool { +func (*resourceClaimTemplateStrategy) AllowUnconditionalUpdate() bool { return true } diff --git a/pkg/registry/resource/resourceclaimtemplate/strategy_test.go b/pkg/registry/resource/resourceclaimtemplate/strategy_test.go index eb547cfe469..81e260a144d 100644 --- a/pkg/registry/resource/resourceclaimtemplate/strategy_test.go +++ b/pkg/registry/resource/resourceclaimtemplate/strategy_test.go @@ -21,9 +21,12 @@ import ( "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/kubernetes/fake" + testclient "k8s.io/client-go/testing" featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/features" @@ -33,7 +36,7 @@ import ( var obj = &resource.ResourceClaimTemplate{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim-template", - Namespace: "default", + Namespace: "kube-system", }, Spec: resource.ResourceClaimTemplateSpec{ Spec: resource.ResourceClaimSpec{ @@ -51,6 +54,27 @@ var obj = &resource.ResourceClaimTemplate{ } var objWithAdminAccess = &resource.ResourceClaimTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim-template", + Namespace: "kube-system", + }, + Spec: resource.ResourceClaimTemplateSpec{ + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + AdminAccess: ptr.To(true), + }, + }, + }, + }, + }, +} + +var objWithAdminAccessInNonAdminNamespace = &resource.ResourceClaimTemplate{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim-template", Namespace: "default", @@ -97,11 +121,32 @@ var objWithPrioritizedList = &resource.ResourceClaimTemplate{ }, } +var ns1 = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Labels: map[string]string{"key": "value"}, + }, +} +var ns2 = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kube-system", + Labels: map[string]string{resource.DRAAdminNamespaceLabelKey: "true"}, + }, +} +var adminAccessError = "Forbidden: admin access to devices requires the `resource.k8s.io/admin-access: true` label on the containing namespace" +var fieldImmutableError = "field is immutable" +var metadataError = "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters" +var deviceRequestError = "exactly one of `deviceClassName` or `firstAvailable` must be specified" + func TestClaimTemplateStrategy(t *testing.T) { - if !Strategy.NamespaceScoped() { + fakeClient := fake.NewSimpleClientset() + mockNSClient := fakeClient.CoreV1().Namespaces() + strategy := NewStrategy(mockNSClient) + + if !strategy.NamespaceScoped() { t.Errorf("ResourceClaimTemplate must be namespace scoped") } - if Strategy.AllowCreateOnUpdate() { + if strategy.AllowCreateOnUpdate() { t.Errorf("ResourceClaimTemplate should not allow create on update") } } @@ -112,13 +157,19 @@ func TestClaimTemplateStrategyCreate(t *testing.T) { testcases := map[string]struct { obj *resource.ResourceClaimTemplate adminAccess bool + expectValidationError string prioritizedList bool - expectValidationError bool expectObj *resource.ResourceClaimTemplate + verify func(*testing.T, []testclient.Action) }{ "simple": { obj: obj, expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "validation-error": { obj: func() *resource.ResourceClaimTemplate { @@ -126,50 +177,114 @@ func TestClaimTemplateStrategyCreate(t *testing.T) { obj.Name = "%#@$%$" return obj }(), - expectValidationError: true, + expectValidationError: metadataError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "drop-fields-admin-access": { obj: objWithAdminAccess, adminAccess: false, expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-admin-access": { obj: objWithAdminAccess, adminAccess: true, expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "kube-system" { + t.Errorf("expected to get the kube-system namespace but got '%s'", ns) + } + }, }, "drop-fields-prioritized-list": { obj: objWithPrioritizedList, prioritizedList: false, - expectValidationError: true, + expectValidationError: deviceRequestError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, }, "keep-fields-prioritized-list": { obj: objWithPrioritizedList, prioritizedList: true, expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "admin-access-admin-namespace": { + obj: objWithAdminAccess, + adminAccess: true, + expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "kube-system" { + t.Errorf("expected to get the kube-system namespace but got '%s'", ns) + } + }, + }, + "admin-access-non-admin-namespace": { + obj: objWithAdminAccessInNonAdminNamespace, + adminAccess: true, + expectObj: objWithAdminAccessInNonAdminNamespace, + expectValidationError: adminAccessError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 1 { + t.Errorf("expected one action but got %d", len(as)) + return + } + ns := as[0].(testclient.GetAction).GetName() + if ns != "default" { + t.Errorf("expected to get the default namespace but got '%s'", ns) + } + }, }, } for name, tc := range testcases { t.Run(name, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAAdminAccess, tc.adminAccess) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAPrioritizedList, tc.prioritizedList) + strategy := NewStrategy(mockNSClient) obj := tc.obj.DeepCopy() - Strategy.PrepareForCreate(ctx, obj) - if errs := Strategy.Validate(ctx, obj); len(errs) != 0 { - if !tc.expectValidationError { - t.Fatalf("unexpected validation errors: %q", errs) - } + strategy.PrepareForCreate(ctx, obj) + if errs := strategy.Validate(ctx, obj); len(errs) != 0 { + assert.ErrorContains(t, errs[0], tc.expectValidationError, "the error message should have contained the expected error message") return - } else if tc.expectValidationError { + } + if tc.expectValidationError != "" { t.Fatal("expected validation error(s), got none") } - if warnings := Strategy.WarningsOnCreate(ctx, obj); len(warnings) != 0 { + if warnings := strategy.WarningsOnCreate(ctx, obj); len(warnings) != 0 { t.Fatalf("unexpected warnings: %q", warnings) } - Strategy.Canonicalize(obj) + strategy.Canonicalize(obj) assert.Equal(t, tc.expectObj, obj) + tc.verify(t, fakeClient.Actions()) }) } } @@ -177,28 +292,66 @@ func TestClaimTemplateStrategyCreate(t *testing.T) { func TestClaimTemplateStrategyUpdate(t *testing.T) { t.Run("no-changes-okay", func(t *testing.T) { ctx := genericapirequest.NewDefaultContext() + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() + + strategy := NewStrategy(mockNSClient) resourceClaimTemplate := obj.DeepCopy() newClaimTemplate := resourceClaimTemplate.DeepCopy() newClaimTemplate.ResourceVersion = "4" - Strategy.PrepareForUpdate(ctx, newClaimTemplate, resourceClaimTemplate) - errs := Strategy.ValidateUpdate(ctx, newClaimTemplate, resourceClaimTemplate) + strategy.PrepareForUpdate(ctx, newClaimTemplate, resourceClaimTemplate) + errs := strategy.ValidateUpdate(ctx, newClaimTemplate, resourceClaimTemplate) if len(errs) != 0 { t.Errorf("unexpected validation errors: %v", errs) } + if len(fakeClient.Actions()) != 0 { + t.Errorf("expected no action to be taken") + } }) t.Run("name-change-not-allowed", func(t *testing.T) { ctx := genericapirequest.NewDefaultContext() + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() + strategy := NewStrategy(mockNSClient) resourceClaimTemplate := obj.DeepCopy() newClaimTemplate := resourceClaimTemplate.DeepCopy() newClaimTemplate.Name = "valid-class-2" newClaimTemplate.ResourceVersion = "4" - Strategy.PrepareForUpdate(ctx, newClaimTemplate, resourceClaimTemplate) - errs := Strategy.ValidateUpdate(ctx, newClaimTemplate, resourceClaimTemplate) + strategy.PrepareForUpdate(ctx, newClaimTemplate, resourceClaimTemplate) + errs := strategy.ValidateUpdate(ctx, newClaimTemplate, resourceClaimTemplate) if len(errs) == 0 { t.Errorf("expected a validation error") } + if len(fakeClient.Actions()) != 0 { + t.Errorf("expected no action to be taken") + } + }) + + t.Run("adminaccess-update-not-allowed", func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAAdminAccess, true) + ctx := genericapirequest.NewDefaultContext() + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() + strategy := NewStrategy(mockNSClient) + resourceClaimTemplate := obj.DeepCopy() + newClaimTemplate := resourceClaimTemplate.DeepCopy() + newClaimTemplate.ResourceVersion = "4" + newClaimTemplate.Spec.Spec.Devices.Requests[0].AdminAccess = ptr.To(true) + + strategy.PrepareForUpdate(ctx, newClaimTemplate, resourceClaimTemplate) + errs := strategy.ValidateUpdate(ctx, newClaimTemplate, resourceClaimTemplate) + if len(errs) != 0 { + assert.ErrorContains(t, errs[0], fieldImmutableError, "the error message should have contained the expected error message") + return + } + if len(errs) == 0 { + t.Errorf("expected a validation error") + } + if len(fakeClient.Actions()) != 0 { + t.Errorf("expected no action to be taken") + } }) } diff --git a/pkg/registry/resource/rest/storage_resource.go b/pkg/registry/resource/rest/storage_resource.go index 18c18d95601..315fb626378 100644 --- a/pkg/registry/resource/rest/storage_resource.go +++ b/pkg/registry/resource/rest/storage_resource.go @@ -23,6 +23,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" serverstorage "k8s.io/apiserver/pkg/server/storage" + "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/apis/resource" deviceclassstore "k8s.io/kubernetes/pkg/registry/resource/deviceclass/storage" @@ -35,20 +36,22 @@ import ( // feature gate because it might be useful to provide access to these resources // while their feature is off to allow cleaning them up. -type RESTStorageProvider struct{} +type RESTStorageProvider struct { + NamespaceClient v1.NamespaceInterface +} func (p RESTStorageProvider) NewRESTStorage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter) (genericapiserver.APIGroupInfo, error) { apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(resource.GroupName, legacyscheme.Scheme, legacyscheme.ParameterCodec, legacyscheme.Codecs) // If you add a version here, be sure to add an entry in `k8s.io/kubernetes/cmd/kube-apiserver/app/aggregator.go with specific priorities. // TODO refactor the plumbing to provide the information in the APIGroupInfo - if storageMap, err := p.v1alpha3Storage(apiResourceConfigSource, restOptionsGetter); err != nil { + if storageMap, err := p.v1alpha3Storage(apiResourceConfigSource, restOptionsGetter, p.NamespaceClient); err != nil { return genericapiserver.APIGroupInfo{}, err } else if len(storageMap) > 0 { apiGroupInfo.VersionedResourcesStorageMap[resourcev1alpha3.SchemeGroupVersion.Version] = storageMap } - if storageMap, err := p.v1beta1Storage(apiResourceConfigSource, restOptionsGetter); err != nil { + if storageMap, err := p.v1beta1Storage(apiResourceConfigSource, restOptionsGetter, p.NamespaceClient); err != nil { return genericapiserver.APIGroupInfo{}, err } else if len(storageMap) > 0 { apiGroupInfo.VersionedResourcesStorageMap[resourcev1beta1.SchemeGroupVersion.Version] = storageMap @@ -57,7 +60,7 @@ func (p RESTStorageProvider) NewRESTStorage(apiResourceConfigSource serverstorag return apiGroupInfo, nil } -func (p RESTStorageProvider) v1alpha3Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter) (map[string]rest.Storage, error) { +func (p RESTStorageProvider) v1alpha3Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter, nsClient v1.NamespaceInterface) (map[string]rest.Storage, error) { storage := map[string]rest.Storage{} if resource := "deviceclasses"; apiResourceConfigSource.ResourceEnabled(resourcev1alpha3.SchemeGroupVersion.WithResource(resource)) { @@ -69,7 +72,7 @@ func (p RESTStorageProvider) v1alpha3Storage(apiResourceConfigSource serverstora } if resource := "resourceclaims"; apiResourceConfigSource.ResourceEnabled(resourcev1alpha3.SchemeGroupVersion.WithResource(resource)) { - resourceClaimStorage, resourceClaimStatusStorage, err := resourceclaimstore.NewREST(restOptionsGetter) + resourceClaimStorage, resourceClaimStatusStorage, err := resourceclaimstore.NewREST(restOptionsGetter, nsClient) if err != nil { return nil, err } @@ -78,7 +81,7 @@ func (p RESTStorageProvider) v1alpha3Storage(apiResourceConfigSource serverstora } if resource := "resourceclaimtemplates"; apiResourceConfigSource.ResourceEnabled(resourcev1alpha3.SchemeGroupVersion.WithResource(resource)) { - resourceClaimTemplateStorage, err := resourceclaimtemplatestore.NewREST(restOptionsGetter) + resourceClaimTemplateStorage, err := resourceclaimtemplatestore.NewREST(restOptionsGetter, nsClient) if err != nil { return nil, err } @@ -96,7 +99,7 @@ func (p RESTStorageProvider) v1alpha3Storage(apiResourceConfigSource serverstora return storage, nil } -func (p RESTStorageProvider) v1beta1Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter) (map[string]rest.Storage, error) { +func (p RESTStorageProvider) v1beta1Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter, nsClient v1.NamespaceInterface) (map[string]rest.Storage, error) { storage := map[string]rest.Storage{} if resource := "deviceclasses"; apiResourceConfigSource.ResourceEnabled(resourcev1beta1.SchemeGroupVersion.WithResource(resource)) { @@ -108,7 +111,7 @@ func (p RESTStorageProvider) v1beta1Storage(apiResourceConfigSource serverstorag } if resource := "resourceclaims"; apiResourceConfigSource.ResourceEnabled(resourcev1beta1.SchemeGroupVersion.WithResource(resource)) { - resourceClaimStorage, resourceClaimStatusStorage, err := resourceclaimstore.NewREST(restOptionsGetter) + resourceClaimStorage, resourceClaimStatusStorage, err := resourceclaimstore.NewREST(restOptionsGetter, nsClient) if err != nil { return nil, err } @@ -117,7 +120,7 @@ func (p RESTStorageProvider) v1beta1Storage(apiResourceConfigSource serverstorag } if resource := "resourceclaimtemplates"; apiResourceConfigSource.ResourceEnabled(resourcev1beta1.SchemeGroupVersion.WithResource(resource)) { - resourceClaimTemplateStorage, err := resourceclaimtemplatestore.NewREST(restOptionsGetter) + resourceClaimTemplateStorage, err := resourceclaimtemplatestore.NewREST(restOptionsGetter, nsClient) if err != nil { return nil, err } diff --git a/pkg/registry/resource/utils.go b/pkg/registry/resource/utils.go new file mode 100644 index 00000000000..688f45e3930 --- /dev/null +++ b/pkg/registry/resource/utils.go @@ -0,0 +1,99 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package resource + +import ( + "context" + "fmt" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + v1 "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/kubernetes/pkg/apis/resource" +) + +// AuthorizedForAdmin checks if the request is authorized to get admin access to devices +// based on namespace label +func AuthorizedForAdmin(ctx context.Context, deviceRequests []resource.DeviceRequest, namespaceName string, nsClient v1.NamespaceInterface) field.ErrorList { + var allErrs field.ErrorList + adminRequested := false + var adminAccessPath *field.Path + + // no need to check old request since spec is immutable + + for i := range deviceRequests { + value := deviceRequests[i].AdminAccess + if value != nil && *value { + adminRequested = true + adminAccessPath = field.NewPath("spec", "devices", "requests").Index(i).Child("adminAccess") + break + } + } + if !adminRequested { + // No need to validate unless admin access is requested + return allErrs + } + + // Retrieve the namespace object from the store + ns, err := nsClient.Get(ctx, namespaceName, metav1.GetOptions{}) + if err != nil { + return append(allErrs, field.InternalError(adminAccessPath, fmt.Errorf("could not retrieve namespace to verify admin access: %w", err))) + } + if ns.Labels[resource.DRAAdminNamespaceLabelKey] != "true" { + return append(allErrs, field.Forbidden(adminAccessPath, fmt.Sprintf("admin access to devices requires the `%s: true` label on the containing namespace", resource.DRAAdminNamespaceLabelKey))) + } + + return allErrs +} + +// AuthorizedForAdminStatus checks if the request status is authorized to get admin access to devices +// based on namespace label +func AuthorizedForAdminStatus(ctx context.Context, newAllocationResult, oldAllocationResult []resource.DeviceRequestAllocationResult, namespaceName string, nsClient v1.NamespaceInterface) field.ErrorList { + var allErrs field.ErrorList + var adminAccessPath *field.Path + + if wasGranted, _ := adminRequested(oldAllocationResult); wasGranted { + // No need to validate if old status has admin access granted, since status.Allocation is immutable + return allErrs + } + isRequested, adminAccessPath := adminRequested(newAllocationResult) + if !isRequested { + // No need to validate unless admin access is requested + return allErrs + } + + // Retrieve the namespace object from the store + ns, err := nsClient.Get(ctx, namespaceName, metav1.GetOptions{}) + if err != nil { + return append(allErrs, field.InternalError(adminAccessPath, fmt.Errorf("could not retrieve namespace to verify admin access: %w", err))) + } + if ns.Labels[resource.DRAAdminNamespaceLabelKey] != "true" { + return append(allErrs, field.Forbidden(adminAccessPath, fmt.Sprintf("admin access to devices requires the `%s: true` label on the containing namespace", resource.DRAAdminNamespaceLabelKey))) + } + + return allErrs +} + +func adminRequested(deviceRequestResults []resource.DeviceRequestAllocationResult) (bool, *field.Path) { + for i := range deviceRequestResults { + value := deviceRequestResults[i].AdminAccess + if value != nil && *value { + return true, field.NewPath("status", "allocation", "devices", "results").Index(i).Child("adminAccess") + } + } + return false, nil +} diff --git a/staging/src/k8s.io/api/resource/v1alpha3/types.go b/staging/src/k8s.io/api/resource/v1alpha3/types.go index b6c6c31840e..ccfe278c2ea 100644 --- a/staging/src/k8s.io/api/resource/v1alpha3/types.go +++ b/staging/src/k8s.io/api/resource/v1alpha3/types.go @@ -383,6 +383,15 @@ const ( DeviceConfigMaxSize = 32 ) +// DRAAdminNamespaceLabelKey is a label key used to grant administrative access +// to certain resource.k8s.io API types within a namespace. When this label is +// set on a namespace with the value "true" (case-sensitive), it allows the use +// of adminAccess: true in any namespaced resource.k8s.io API types. Currently, +// this permission applies to ResourceClaim and ResourceClaimTemplate objects. +const ( + DRAAdminNamespaceLabelKey = "resource.k8s.io/admin-access" +) + // DeviceRequest is a request for devices required for a claim. // This is typically a request for a single resource like a device, but can // also ask for several identical devices. diff --git a/staging/src/k8s.io/api/resource/v1beta1/types.go b/staging/src/k8s.io/api/resource/v1beta1/types.go index cc1f02161f5..24496e68319 100644 --- a/staging/src/k8s.io/api/resource/v1beta1/types.go +++ b/staging/src/k8s.io/api/resource/v1beta1/types.go @@ -391,6 +391,15 @@ const ( DeviceConfigMaxSize = 32 ) +// DRAAdminNamespaceLabelKey is a label key used to grant administrative access +// to certain resource.k8s.io API types within a namespace. When this label is +// set on a namespace with the value "true" (case-sensitive), it allows the use +// of adminAccess: true in any namespaced resource.k8s.io API types. Currently, +// this permission applies to ResourceClaim and ResourceClaimTemplate objects. +const ( + DRAAdminNamespaceLabelKey = "resource.k8s.io/admin-access" +) + // DeviceRequest is a request for devices required for a claim. // This is typically a request for a single resource like a device, but can // also ask for several identical devices. diff --git a/test/e2e/dra/dra.go b/test/e2e/dra/dra.go index b93c44a9b1e..cfa77355915 100644 --- a/test/e2e/dra/dra.go +++ b/test/e2e/dra/dra.go @@ -32,7 +32,6 @@ import ( "github.com/onsi/gomega/gstruct" "github.com/onsi/gomega/types" - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" resourceapi "k8s.io/api/resource/v1beta1" @@ -59,9 +58,6 @@ const ( podStartTimeout = 5 * time.Minute ) -//go:embed test-driver/deploy/example/admin-access-policy.yaml -var adminAccessPolicyYAML string - // networkResources can be passed to NewDriver directly. func networkResources() Resources { return Resources{} @@ -1379,30 +1375,13 @@ var _ = framework.SIGDescribe("node")("DRA", feature.DynamicResourceAllocation, driver := NewDriver(f, nodes, networkResources) b := newBuilder(f, driver) - f.It("support validating admission policy for admin access", feature.DRAAdminAccess, framework.WithFeatureGate(features.DRAAdminAccess), framework.WithFeatureGate(features.DynamicResourceAllocation), func(ctx context.Context) { - // Create VAP, after making it unique to the current test. - adminAccessPolicyYAML := strings.ReplaceAll(adminAccessPolicyYAML, "dra.example.com", b.f.UniqueName) - driver.createFromYAML(ctx, []byte(adminAccessPolicyYAML), "") - - // Wait for both VAPs to be processed. This ensures that there are no check errors in the status. - matchStatus := gomega.Equal(admissionregistrationv1.ValidatingAdmissionPolicyStatus{ObservedGeneration: 1, TypeChecking: &admissionregistrationv1.TypeChecking{}}) - gomega.Eventually(ctx, framework.ListObjects(b.f.ClientSet.AdmissionregistrationV1().ValidatingAdmissionPolicies().List, metav1.ListOptions{})).Should(gomega.HaveField("Items", gomega.ContainElements( - gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ - "ObjectMeta": gomega.HaveField("Name", "resourceclaim-policy."+b.f.UniqueName), - "Status": matchStatus, - }), - gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ - "ObjectMeta": gomega.HaveField("Name", "resourceclaimtemplate-policy."+b.f.UniqueName), - "Status": matchStatus, - }), - ))) - + f.It("validate ResourceClaimTemplate and ResourceClaim for admin access", feature.DRAAdminAccess, framework.WithFeatureGate(features.DRAAdminAccess), framework.WithFeatureGate(features.DynamicResourceAllocation), func(ctx context.Context) { // Attempt to create claim and claim template with admin access. Must fail eventually. claim := b.externalClaim() claim.Spec.Devices.Requests[0].AdminAccess = ptr.To(true) _, claimTemplate := b.podInline() claimTemplate.Spec.Spec.Devices.Requests[0].AdminAccess = ptr.To(true) - matchVAPError := gomega.MatchError(gomega.ContainSubstring("admin access to devices not enabled in namespace " + b.f.Namespace.Name)) + matchValidationError := gomega.MatchError(gomega.ContainSubstring("admin access to devices requires the `resource.k8s.io/admin-access: true` label on the containing namespace")) gomega.Eventually(ctx, func(ctx context.Context) error { // First delete, in case that it succeeded earlier. if err := b.f.ClientSet.ResourceV1beta1().ResourceClaims(b.f.Namespace.Name).Delete(ctx, claim.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { @@ -1410,7 +1389,7 @@ var _ = framework.SIGDescribe("node")("DRA", feature.DynamicResourceAllocation, } _, err := b.f.ClientSet.ResourceV1beta1().ResourceClaims(b.f.Namespace.Name).Create(ctx, claim, metav1.CreateOptions{}) return err - }).Should(matchVAPError) + }).Should(matchValidationError) gomega.Eventually(ctx, func(ctx context.Context) error { // First delete, in case that it succeeded earlier. @@ -1419,11 +1398,11 @@ var _ = framework.SIGDescribe("node")("DRA", feature.DynamicResourceAllocation, } _, err := b.f.ClientSet.ResourceV1beta1().ResourceClaimTemplates(b.f.Namespace.Name).Create(ctx, claimTemplate, metav1.CreateOptions{}) return err - }).Should(matchVAPError) + }).Should(matchValidationError) // After labeling the namespace, creation must (eventually...) succeed. _, err := b.f.ClientSet.CoreV1().Namespaces().Apply(ctx, - applyv1.Namespace(b.f.Namespace.Name).WithLabels(map[string]string{"admin-access." + b.f.UniqueName: "on"}), + applyv1.Namespace(b.f.Namespace.Name).WithLabels(map[string]string{"resource.k8s.io/admin-access": "true"}), metav1.ApplyOptions{FieldManager: b.f.UniqueName}) framework.ExpectNoError(err) gomega.Eventually(ctx, func(ctx context.Context) error { @@ -1499,6 +1478,12 @@ var _ = framework.SIGDescribe("node")("DRA", feature.DynamicResourceAllocation, }) f.It("DaemonSet with admin access", feature.DRAAdminAccess, framework.WithFeatureGate(features.DRAAdminAccess), framework.WithFeatureGate(features.DynamicResourceAllocation), func(ctx context.Context) { + // Ensure namespace has the dra admin label. + _, err := b.f.ClientSet.CoreV1().Namespaces().Apply(ctx, + applyv1.Namespace(b.f.Namespace.Name).WithLabels(map[string]string{"resource.k8s.io/admin-access": "true"}), + metav1.ApplyOptions{FieldManager: b.f.UniqueName}) + framework.ExpectNoError(err) + pod, template := b.podInline() template.Spec.Spec.Devices.Requests[0].AdminAccess = ptr.To(true) // Limit the daemon set to the one node where we have the driver. @@ -1507,7 +1492,8 @@ var _ = framework.SIGDescribe("node")("DRA", feature.DynamicResourceAllocation, pod.Spec.RestartPolicy = v1.RestartPolicyAlways daemonSet := &appsv1.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ - Name: "monitoring-ds", + Name: "monitoring-ds", + Namespace: b.f.Namespace.Name, }, Spec: appsv1.DaemonSetSpec{ Selector: &metav1.LabelSelector{ diff --git a/test/integration/dra/dra_test.go b/test/integration/dra/dra_test.go index eea35f5a620..8ef04908240 100644 --- a/test/integration/dra/dra_test.go +++ b/test/integration/dra/dra_test.go @@ -87,7 +87,7 @@ var ( // - Non-alpha-numeric characters replaced by hyphen. // - Truncated in the middle to make it short enough for GenerateName. // - Hyphen plus random suffix added by the apiserver. -func createTestNamespace(tCtx ktesting.TContext) string { +func createTestNamespace(tCtx ktesting.TContext, labels map[string]string) string { tCtx.Helper() name := regexp.MustCompile(`[^[:alnum:]_-]`).ReplaceAllString(tCtx.Name(), "-") name = strings.ToLower(name) @@ -95,6 +95,7 @@ func createTestNamespace(tCtx ktesting.TContext) string { name = name[:30] + "--" + name[len(name)-30:] } ns := &v1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: name + "-"}} + ns.Labels = labels ns, err := tCtx.Client().CoreV1().Namespaces().Create(tCtx, ns, metav1.CreateOptions{}) tCtx.ExpectNoError(err, "create test namespace") tCtx.CleanupCtx(func(tCtx ktesting.TContext) { @@ -211,7 +212,7 @@ func newDefaultSchedulerComponentConfig(tCtx ktesting.TContext) *config.KubeSche // whether that field is or isn't getting dropped. func testPod(tCtx ktesting.TContext, draEnabled bool) { tCtx.Parallel() - namespace := createTestNamespace(tCtx) + namespace := createTestNamespace(tCtx, nil) podWithClaimName := podWithClaimName.DeepCopy() podWithClaimName.Namespace = namespace pod, err := tCtx.Client().CoreV1().Pods(namespace).Create(tCtx, podWithClaimName, metav1.CreateOptions{}) @@ -235,7 +236,7 @@ func testAPIDisabled(tCtx ktesting.TContext) { // testConvert creates a claim using a one API version and reads it with another. func testConvert(tCtx ktesting.TContext) { tCtx.Parallel() - namespace := createTestNamespace(tCtx) + namespace := createTestNamespace(tCtx, nil) claim := claim.DeepCopy() claim.Namespace = namespace claim, err := tCtx.Client().ResourceV1beta1().ResourceClaims(namespace).Create(tCtx, claim, metav1.CreateOptions{}) @@ -248,17 +249,34 @@ func testConvert(tCtx ktesting.TContext) { // testAdminAccess creates a claim with AdminAccess and then checks // whether that field is or isn't getting dropped. +// when the AdminAccess feature is enabled, it also checks that the field +// is only allowed to be used in namespace with the Resource Admin Access label func testAdminAccess(tCtx ktesting.TContext, adminAccessEnabled bool) { - tCtx.Parallel() - namespace := createTestNamespace(tCtx) - claim := claim.DeepCopy() - claim.Namespace = namespace - claim.Spec.Devices.Requests[0].AdminAccess = ptr.To(true) - claim, err := tCtx.Client().ResourceV1beta1().ResourceClaims(namespace).Create(tCtx, claim, metav1.CreateOptions{}) - tCtx.ExpectNoError(err, "create claim") + namespace := createTestNamespace(tCtx, nil) + claim1 := claim.DeepCopy() + claim1.Namespace = namespace + claim1.Spec.Devices.Requests[0].AdminAccess = ptr.To(true) + // create claim with AdminAccess in non-admin namespace + _, err := tCtx.Client().ResourceV1beta1().ResourceClaims(namespace).Create(tCtx, claim1, metav1.CreateOptions{}) if adminAccessEnabled { - if !ptr.Deref(claim.Spec.Devices.Requests[0].AdminAccess, false) { - tCtx.Fatal("should store AdminAccess in ResourceClaim") + if err != nil { + // should result in validation error + assert.ErrorContains(tCtx, err, "admin access to devices requires the `resource.k8s.io/admin-access: true` label on the containing namespace", "the error message should have contained the expected error message") + return + } else { + tCtx.Fatal("expected validation error(s), got none") + } + + // create claim with AdminAccess in admin namespace + adminNS := createTestNamespace(tCtx, map[string]string{"resource.k8s.io/admin-access": "true"}) + claim2 := claim.DeepCopy() + claim2.Namespace = adminNS + claim2.Name = "claim2" + claim2.Spec.Devices.Requests[0].AdminAccess = ptr.To(true) + claim2, err := tCtx.Client().ResourceV1beta1().ResourceClaims(adminNS).Create(tCtx, claim2, metav1.CreateOptions{}) + tCtx.ExpectNoError(err, "create claim") + if !ptr.Deref(claim2.Spec.Devices.Requests[0].AdminAccess, true) { + tCtx.Fatalf("should store AdminAccess in ResourceClaim %v", claim2) } } else { if claim.Spec.Devices.Requests[0].AdminAccess != nil { @@ -271,7 +289,7 @@ func testPrioritizedList(tCtx ktesting.TContext, enabled bool) { tCtx.Parallel() _, err := tCtx.Client().ResourceV1beta1().DeviceClasses().Create(tCtx, class, metav1.CreateOptions{}) tCtx.ExpectNoError(err, "create class") - namespace := createTestNamespace(tCtx) + namespace := createTestNamespace(tCtx, nil) claim := claimPrioritizedList.DeepCopy() claim.Namespace = namespace claim, err = tCtx.Client().ResourceV1beta1().ResourceClaims(namespace).Create(tCtx, claim, metav1.CreateOptions{})