diff --git a/pkg/apis/resource/v1/zz_generated.validations.go b/pkg/apis/resource/v1/zz_generated.validations.go index 1f778faea9a8..33eba06837f7 100644 --- a/pkg/apis/resource/v1/zz_generated.validations.go +++ b/pkg/apis/resource/v1/zz_generated.validations.go @@ -593,6 +593,10 @@ func Validate_ResourceClaimStatus(ctx context.Context, op operation.Operation, f if e := validate.OptionalSlice(ctx, op, fldPath, obj, oldObj); len(e) != 0 { return // do not proceed } + // lists with map semantics require unique keys + errs = append(errs, validate.Unique(ctx, op, fldPath, obj, oldObj, func(a resourcev1.ResourceClaimConsumerReference, b resourcev1.ResourceClaimConsumerReference) bool { + return a.UID == b.UID + })...) return }(fldPath.Child("reservedFor"), obj.ReservedFor, safe.Field(oldObj, func(oldObj *resourcev1.ResourceClaimStatus) []resourcev1.ResourceClaimConsumerReference { return oldObj.ReservedFor diff --git a/pkg/apis/resource/v1beta1/zz_generated.validations.go b/pkg/apis/resource/v1beta1/zz_generated.validations.go index d0b91250be32..8d47c5a0571e 100644 --- a/pkg/apis/resource/v1beta1/zz_generated.validations.go +++ b/pkg/apis/resource/v1beta1/zz_generated.validations.go @@ -606,6 +606,10 @@ func Validate_ResourceClaimStatus(ctx context.Context, op operation.Operation, f if e := validate.OptionalSlice(ctx, op, fldPath, obj, oldObj); len(e) != 0 { return // do not proceed } + // lists with map semantics require unique keys + errs = append(errs, validate.Unique(ctx, op, fldPath, obj, oldObj, func(a resourcev1beta1.ResourceClaimConsumerReference, b resourcev1beta1.ResourceClaimConsumerReference) bool { + return a.UID == b.UID + })...) return }(fldPath.Child("reservedFor"), obj.ReservedFor, safe.Field(oldObj, func(oldObj *resourcev1beta1.ResourceClaimStatus) []resourcev1beta1.ResourceClaimConsumerReference { return oldObj.ReservedFor diff --git a/pkg/apis/resource/v1beta2/zz_generated.validations.go b/pkg/apis/resource/v1beta2/zz_generated.validations.go index f98232a99631..517492677167 100644 --- a/pkg/apis/resource/v1beta2/zz_generated.validations.go +++ b/pkg/apis/resource/v1beta2/zz_generated.validations.go @@ -613,6 +613,10 @@ func Validate_ResourceClaimStatus(ctx context.Context, op operation.Operation, f if e := validate.OptionalSlice(ctx, op, fldPath, obj, oldObj); len(e) != 0 { return // do not proceed } + // lists with map semantics require unique keys + errs = append(errs, validate.Unique(ctx, op, fldPath, obj, oldObj, func(a resourcev1beta2.ResourceClaimConsumerReference, b resourcev1beta2.ResourceClaimConsumerReference) bool { + return a.UID == b.UID + })...) return }(fldPath.Child("reservedFor"), obj.ReservedFor, safe.Field(oldObj, func(oldObj *resourcev1beta2.ResourceClaimStatus) []resourcev1beta2.ResourceClaimConsumerReference { return oldObj.ReservedFor diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index adee91765beb..a5e385d6fa20 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -125,8 +125,8 @@ func validateDeviceClaim(deviceClaim *resource.DeviceClaim, fldPath *field.Path, func(request resource.DeviceRequest, fldPath *field.Path) field.ErrorList { return validateDeviceRequest(request, fldPath, stored) }, - func(request resource.DeviceRequest) (string, string) { - return request.Name, "name" + func(request resource.DeviceRequest) string { + return request.Name }, fldPath.Child("requests"), sizeCovered)...) allErrs = append(allErrs, validateSlice(deviceClaim.Constraints, resource.DeviceConstraintsMaxSize, @@ -200,8 +200,8 @@ func validateDeviceRequest(request resource.DeviceRequest, fldPath *field.Path, func(subRequest resource.DeviceSubRequest, fldPath *field.Path) field.ErrorList { return validateDeviceSubRequest(subRequest, fldPath, stored) }, - func(subRequest resource.DeviceSubRequest) (string, string) { - return subRequest.Name, "name" + func(subRequest resource.DeviceSubRequest) string { + return subRequest.Name }, fldPath.Child("firstAvailable"), sizeCovered)...) } @@ -393,8 +393,8 @@ func validateResourceClaimStatusUpdate(status, oldStatus *resource.ResourceClaim var allErrs field.ErrorList allErrs = append(allErrs, validateSet(status.ReservedFor, resource.ResourceClaimReservedForMaxSize, validateResourceClaimUserReference, - func(consumer resource.ResourceClaimConsumerReference) (types.UID, string) { return consumer.UID, "uid" }, - fldPath.Child("reservedFor"))...) + func(consumer resource.ResourceClaimConsumerReference) types.UID { return consumer.UID }, + fldPath.Child("reservedFor"), uniquenessCovered)...) var allocatedDevices sets.Set[structured.SharedDeviceID] if status.Allocation != nil { @@ -404,9 +404,9 @@ func validateResourceClaimStatusUpdate(status, oldStatus *resource.ResourceClaim func(device resource.AllocatedDeviceStatus, fldPath *field.Path) field.ErrorList { return validateDeviceStatus(device, fldPath, allocatedDevices) }, - func(device resource.AllocatedDeviceStatus) (structured.SharedDeviceID, string) { + func(device resource.AllocatedDeviceStatus) structured.SharedDeviceID { deviceID := structured.MakeDeviceID(device.Driver, device.Pool, device.Device) - return structured.MakeSharedDeviceID(deviceID, (*types.UID)(device.ShareID)), "deviceID" + return structured.MakeSharedDeviceID(deviceID, (*types.UID)(device.ShareID)) }, fldPath.Child("devices"))...) @@ -673,8 +673,8 @@ func validateResourceSliceSpec(spec, oldSpec *resource.ResourceSliceSpec, fldPat func(device resource.Device, fldPath *field.Path) field.ErrorList { return validateDevice(device, fldPath, sharedCounterToCounterNames, spec.PerDeviceNodeSelection) }, - func(device resource.Device) (string, string) { - return device.Name, "name" + func(device resource.Device) string { + return device.Name }, fldPath.Child("devices"))...) // Size limit for total number of counters in devices enforced here. @@ -698,8 +698,8 @@ func validateResourceSliceSpec(spec, oldSpec *resource.ResourceSliceSpec, fldPat } allErrs = append(allErrs, validateSet(spec.SharedCounters, -1, validateCounterSet, - func(counterSet resource.CounterSet) (string, string) { - return counterSet.Name, "name" + func(counterSet resource.CounterSet) string { + return counterSet.Name }, fldPath.Child("sharedCounters"))...) return allErrs @@ -768,8 +768,8 @@ func validateDevice(device resource.Device, fldPath *field.Path, sharedCounterTo allErrs = append(allErrs, validateSet(device.ConsumesCounters, -1, validateDeviceCounterConsumption, - func(deviceCapacityConsumption resource.DeviceCounterConsumption) (string, string) { - return deviceCapacityConsumption.CounterSet, "counterSet" + func(deviceCapacityConsumption resource.DeviceCounterConsumption) string { + return deviceCapacityConsumption.CounterSet }, fldPath.Child("consumesCounters"))...) var countersLength int @@ -1138,17 +1138,14 @@ func validateSlice[T any](slice []T, maxSize int, validateItem func(T, *field.Pa // validateSet ensures that a slice contains no duplicates, does not // exceed a certain maximum size and that all entries are valid. -func validateSet[T any, K comparable](slice []T, maxSize int, validateItem func(item T, fldPath *field.Path) field.ErrorList, itemKey func(T) (K, string), fldPath *field.Path, opts ...validationOption) field.ErrorList { +func validateSet[T any, K comparable](slice []T, maxSize int, validateItem func(item T, fldPath *field.Path) field.ErrorList, itemKey func(T) K, fldPath *field.Path, opts ...validationOption) field.ErrorList { allErrs := validateSlice(slice, maxSize, validateItem, fldPath, opts...) allItems := sets.New[K]() for i, item := range slice { idxPath := fldPath.Index(i) - key, fieldName := itemKey(item) + key := itemKey(item) childPath := idxPath - if fieldName != "" { - childPath = childPath.Child(fieldName) - } if allItems.Has(key) { err := field.Duplicate(childPath, key) if slices.Contains(opts, uniquenessCovered) { @@ -1163,13 +1160,13 @@ func validateSet[T any, K comparable](slice []T, maxSize int, validateItem func( } // stringKey uses the item itself as a key for validateSet. -func stringKey(item string) (string, string) { - return item, "" +func stringKey(item string) string { + return item } // quantityKey uses the item itself as a key for validateSet. -func quantityKey(item apiresource.Quantity) (string, string) { - return strconv.FormatInt(item.Value(), 10), "" +func quantityKey(item apiresource.Quantity) string { + return strconv.FormatInt(item.Value(), 10) } // validateMap validates keys, items and the maximum length of a map. diff --git a/pkg/apis/resource/validation/validation_resourceclaim_test.go b/pkg/apis/resource/validation/validation_resourceclaim_test.go index cddc1b285025..6732cbd98f78 100644 --- a/pkg/apis/resource/validation/validation_resourceclaim_test.go +++ b/pkg/apis/resource/validation/validation_resourceclaim_test.go @@ -414,7 +414,7 @@ func TestValidateClaim(t *testing.T) { field.Invalid(field.NewPath("spec", "devices", "requests").Index(2).Child("exactly", "count"), int64(-1), "must be greater than zero"), field.NotSupported(field.NewPath("spec", "devices", "requests").Index(3).Child("exactly", "allocationMode"), resource.DeviceAllocationMode("other"), []resource.DeviceAllocationMode{resource.DeviceAllocationModeAll, resource.DeviceAllocationModeExactCount}), field.Invalid(field.NewPath("spec", "devices", "requests").Index(4).Child("exactly", "count"), int64(2), "must not be specified when allocationMode is 'All'"), - field.Duplicate(field.NewPath("spec", "devices", "requests").Index(5).Child("name"), "foo"), + field.Duplicate(field.NewPath("spec", "devices", "requests").Index(5), "foo"), }, claim: func() *resource.ResourceClaim { claim := testClaim(goodName, goodNS, validClaimSpec) @@ -638,7 +638,7 @@ func TestValidateClaim(t *testing.T) { }, "prioritized-list-nested-requests-same-name": { wantFailures: field.ErrorList{ - field.Duplicate(field.NewPath("spec", "devices", "requests").Index(0).Child("firstAvailable").Index(1).Child("name"), "foo"), + field.Duplicate(field.NewPath("spec", "devices", "requests").Index(0).Child("firstAvailable").Index(1), "foo"), }, claim: func() *resource.ResourceClaim { claim := testClaim(goodName, goodNS, validClaimSpecWithFirstAvailable) @@ -1064,7 +1064,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) { }, }, "invalid-reserved-for-duplicate": { - wantFailures: field.ErrorList{field.Duplicate(field.NewPath("status", "reservedFor").Index(1).Child("uid"), types.UID("1"))}, + wantFailures: field.ErrorList{field.Duplicate(field.NewPath("status", "reservedFor").Index(1), types.UID("1")).MarkCoveredByDeclarative()}, oldClaim: validAllocatedClaim, update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { for i := 0; i < 2; i++ { @@ -1330,7 +1330,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) { "invalid-device-status-duplicate": { wantFailures: field.ErrorList{ field.Duplicate(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(1), "2001:db8::1/64"), - field.Duplicate(field.NewPath("status", "devices").Index(1).Child("deviceID"), structured.MakeSharedDeviceID(structured.MakeDeviceID(goodName, goodName, goodName), nil)), + field.Duplicate(field.NewPath("status", "devices").Index(1), structured.MakeSharedDeviceID(structured.MakeDeviceID(goodName, goodName, goodName), nil)), }, oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(), update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { @@ -1358,7 +1358,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) { }, "invalid-device-status-duplicate-with-share-id": { wantFailures: field.ErrorList{ - field.Duplicate(field.NewPath("status", "devices").Index(1).Child("deviceID"), + field.Duplicate(field.NewPath("status", "devices").Index(1), structured.MakeSharedDeviceID(structured.MakeDeviceID(goodName, goodName, goodName), ptr.To(goodShareID))), }, oldClaim: func() *resource.ResourceClaim { @@ -1493,7 +1493,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) { "invalid-device-status-duplicate-disabled-feature-gate": { wantFailures: field.ErrorList{ field.Duplicate(field.NewPath("status", "devices").Index(0).Child("networkData", "ips").Index(1), "2001:db8::1/64"), - field.Duplicate(field.NewPath("status", "devices").Index(1).Child("deviceID"), structured.MakeSharedDeviceID(structured.MakeDeviceID(goodName, goodName, goodName), nil)), + field.Duplicate(field.NewPath("status", "devices").Index(1), structured.MakeSharedDeviceID(structured.MakeDeviceID(goodName, goodName, goodName), nil)), }, oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(), update: func(claim *resource.ResourceClaim) *resource.ResourceClaim { @@ -1521,7 +1521,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) { }, "invalid-device-status-duplicate-with-share-id-disabled-feature-gate": { wantFailures: field.ErrorList{ - field.Duplicate(field.NewPath("status", "devices").Index(1).Child("deviceID"), + field.Duplicate(field.NewPath("status", "devices").Index(1), structured.MakeSharedDeviceID(structured.MakeDeviceID(goodName, goodName, goodName), ptr.To(goodShareID))), }, oldClaim: func() *resource.ResourceClaim { diff --git a/pkg/apis/resource/validation/validation_resourceslice_test.go b/pkg/apis/resource/validation/validation_resourceslice_test.go index 7669cf15fa4c..3552733b510f 100644 --- a/pkg/apis/resource/validation/validation_resourceslice_test.go +++ b/pkg/apis/resource/validation/validation_resourceslice_test.go @@ -694,7 +694,7 @@ func TestValidateResourceSlice(t *testing.T) { }, "duplicate-shared-counters": { wantFailures: field.ErrorList{ - field.Duplicate(field.NewPath("spec", "sharedCounters").Index(1).Child("name"), goodName), + field.Duplicate(field.NewPath("spec", "sharedCounters").Index(1), goodName), }, slice: func() *resourceapi.ResourceSlice { slice := testResourceSlice(goodName, goodName, driverName, 1) diff --git a/pkg/registry/resource/resourceclaim/declarative_validation_test.go b/pkg/registry/resource/resourceclaim/declarative_validation_test.go index da9bc6fbd3e5..76e51ca6bf96 100644 --- a/pkg/registry/resource/resourceclaim/declarative_validation_test.go +++ b/pkg/registry/resource/resourceclaim/declarative_validation_test.go @@ -21,6 +21,7 @@ import ( "strings" "testing" + "github.com/google/uuid" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -34,6 +35,11 @@ import ( var apiVersions = []string{"v1beta1", "v1beta2", "v1"} // "v1alpha3" is excluded because it doesn't have ResourceClaim +const ( + validUUID = "550e8400-e29b-41d4-a716-446655440000" + validUUID1 = "550e8400-e29b-41d4-a716-446655440001" +) + func TestDeclarativeValidate(t *testing.T) { for _, apiVersion := range apiVersions { t.Run(apiVersion, func(t *testing.T) { @@ -383,7 +389,7 @@ func TestValidateStatusUpdateForDeclarative(t *testing.T) { // .Status.Allocation.Devices.Results[%d].ShareID "valid status.Allocation.Devices.Results[].ShareID": { old: mkValidResourceClaim(), - update: mkResourceClaimWithStatus(tweakStatusDeviceRequestAllocationResultShareID("123e4567-e89b-12d3-a456-426614174000")), + update: mkResourceClaimWithStatus(tweakStatusDeviceRequestAllocationResultShareID(validUUID)), }, "invalid status.Allocation.Devices.Results[].ShareID": { old: mkValidResourceClaim(), @@ -404,8 +410,8 @@ func TestValidateStatusUpdateForDeclarative(t *testing.T) { old: mkValidResourceClaim(), update: mkResourceClaimWithStatus( tweakStatusDevices(standardAllocatedDeviceStatus()), - tweakStatusDeviceRequestAllocationResultShareID("123e4567-e89b-12d3-a456-426614174000"), - tweakStatusAllocatedDeviceStatusShareID("123e4567-e89b-12d3-a456-426614174000"), + tweakStatusDeviceRequestAllocationResultShareID(validUUID), + tweakStatusAllocatedDeviceStatusShareID(validUUID), ), }, "invalid status.Devices[].ShareID": { @@ -432,6 +438,34 @@ func TestValidateStatusUpdateForDeclarative(t *testing.T) { field.Invalid(field.NewPath("status", "devices").Index(0).Child("shareID"), "invalid-uid", "must be a lowercase UUID in 8-4-4-4-12 format").WithOrigin("format=k8s-uuid"), }, }, + // UID in status.ReservedFor + "duplicate uid in status.ReservedFor": { + old: mkValidResourceClaim(), + update: mkResourceClaimWithStatus( + tweakStatusReservedFor( + resourceClaimReference(validUUID), + resourceClaimReference(uuid.New().String()), + resourceClaimReference(validUUID), + )), + expectedErrs: field.ErrorList{ + field.Duplicate(field.NewPath("status", "reservedFor").Index(2), ""), + }, + }, + "multiple- duplicate uid in status.ReservedFor": { + old: mkValidResourceClaim(), + update: mkResourceClaimWithStatus(tweakStatusReservedFor( + resourceClaimReference(validUUID), + resourceClaimReference(uuid.New().String()), + resourceClaimReference(validUUID), + resourceClaimReference(validUUID1), + resourceClaimReference(validUUID1), + resourceClaimReference(uuid.New().String()), + )), + expectedErrs: field.ErrorList{ + field.Duplicate(field.NewPath("status", "reservedFor").Index(2), ""), + field.Duplicate(field.NewPath("status", "reservedFor").Index(4), ""), + }, + }, } for k, tc := range testCases { t.Run(k, func(t *testing.T) { @@ -546,3 +580,17 @@ func standardAllocatedDeviceStatus() resource.AllocatedDeviceStatus { Device: "device-0", } } + +func resourceClaimReference(uid string) resource.ResourceClaimConsumerReference { + return resource.ResourceClaimConsumerReference{ + UID: types.UID(uid), + Resource: "Pod", + Name: "pod-name", + } +} + +func tweakStatusReservedFor(refs ...resource.ResourceClaimConsumerReference) func(rc *resource.ResourceClaim) { + return func(rc *resource.ResourceClaim) { + rc.Status.ReservedFor = refs + } +} diff --git a/staging/src/k8s.io/api/resource/v1/generated.proto b/staging/src/k8s.io/api/resource/v1/generated.proto index 4d18995205ef..c41481091726 100644 --- a/staging/src/k8s.io/api/resource/v1/generated.proto +++ b/staging/src/k8s.io/api/resource/v1/generated.proto @@ -1379,11 +1379,13 @@ message ResourceClaimStatus { // the future, but not reduced. // // +optional - // +k8s:optional // +listType=map // +listMapKey=uid // +patchStrategy=merge // +patchMergeKey=uid + // +k8s:optional + // +k8s:listType=map + // +k8s:listMapKey=uid repeated ResourceClaimConsumerReference reservedFor = 2; // Devices contains the status of each device allocated for this diff --git a/staging/src/k8s.io/api/resource/v1/types.go b/staging/src/k8s.io/api/resource/v1/types.go index 053fa9431ab4..4a778e19f5ef 100644 --- a/staging/src/k8s.io/api/resource/v1/types.go +++ b/staging/src/k8s.io/api/resource/v1/types.go @@ -1382,11 +1382,13 @@ type ResourceClaimStatus struct { // the future, but not reduced. // // +optional - // +k8s:optional // +listType=map // +listMapKey=uid // +patchStrategy=merge // +patchMergeKey=uid + // +k8s:optional + // +k8s:listType=map + // +k8s:listMapKey=uid ReservedFor []ResourceClaimConsumerReference `json:"reservedFor,omitempty" protobuf:"bytes,2,opt,name=reservedFor" patchStrategy:"merge" patchMergeKey:"uid"` // DeallocationRequested is tombstoned since Kubernetes 1.32 where diff --git a/staging/src/k8s.io/api/resource/v1beta1/generated.proto b/staging/src/k8s.io/api/resource/v1beta1/generated.proto index bed605d7fa85..e7bcda2cd823 100644 --- a/staging/src/k8s.io/api/resource/v1beta1/generated.proto +++ b/staging/src/k8s.io/api/resource/v1beta1/generated.proto @@ -1393,11 +1393,13 @@ message ResourceClaimStatus { // the future, but not reduced. // // +optional - // +k8s:optional // +listType=map // +listMapKey=uid // +patchStrategy=merge // +patchMergeKey=uid + // +k8s:optional + // +k8s:listType=map + // +k8s:listMapKey=uid repeated ResourceClaimConsumerReference reservedFor = 2; // Devices contains the status of each device allocated for this diff --git a/staging/src/k8s.io/api/resource/v1beta1/types.go b/staging/src/k8s.io/api/resource/v1beta1/types.go index e1e7a5d9e51f..b4052ed1ba00 100644 --- a/staging/src/k8s.io/api/resource/v1beta1/types.go +++ b/staging/src/k8s.io/api/resource/v1beta1/types.go @@ -1389,11 +1389,13 @@ type ResourceClaimStatus struct { // the future, but not reduced. // // +optional - // +k8s:optional // +listType=map // +listMapKey=uid // +patchStrategy=merge // +patchMergeKey=uid + // +k8s:optional + // +k8s:listType=map + // +k8s:listMapKey=uid ReservedFor []ResourceClaimConsumerReference `json:"reservedFor,omitempty" protobuf:"bytes,2,opt,name=reservedFor" patchStrategy:"merge" patchMergeKey:"uid"` // DeallocationRequested is tombstoned since Kubernetes 1.32 where diff --git a/staging/src/k8s.io/api/resource/v1beta2/generated.proto b/staging/src/k8s.io/api/resource/v1beta2/generated.proto index 6245087880c3..e7959a20e2b8 100644 --- a/staging/src/k8s.io/api/resource/v1beta2/generated.proto +++ b/staging/src/k8s.io/api/resource/v1beta2/generated.proto @@ -1379,11 +1379,13 @@ message ResourceClaimStatus { // the future, but not reduced. // // +optional - // +k8s:optional // +listType=map // +listMapKey=uid // +patchStrategy=merge // +patchMergeKey=uid + // +k8s:optional + // +k8s:listType=map + // +k8s:listMapKey=uid repeated ResourceClaimConsumerReference reservedFor = 2; // Devices contains the status of each device allocated for this diff --git a/staging/src/k8s.io/api/resource/v1beta2/types.go b/staging/src/k8s.io/api/resource/v1beta2/types.go index 458fba5b8ca6..b66c519dcb8f 100644 --- a/staging/src/k8s.io/api/resource/v1beta2/types.go +++ b/staging/src/k8s.io/api/resource/v1beta2/types.go @@ -1382,11 +1382,13 @@ type ResourceClaimStatus struct { // the future, but not reduced. // // +optional - // +k8s:optional // +listType=map // +listMapKey=uid // +patchStrategy=merge // +patchMergeKey=uid + // +k8s:optional + // +k8s:listType=map + // +k8s:listMapKey=uid ReservedFor []ResourceClaimConsumerReference `json:"reservedFor,omitempty" protobuf:"bytes,2,opt,name=reservedFor" patchStrategy:"merge" patchMergeKey:"uid"` // DeallocationRequested is tombstoned since Kubernetes 1.32 where