From f32302e744e294cd3b61af4f7b5fe1529e42e330 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Tue, 21 Feb 2023 15:36:55 +0100
Subject: [PATCH] api: drop Resources.Claims from PVC and PVC template

PVC and containers share the same ResourceRequirements struct. The Claims field
in it only makes sense when used in containers. When used in a PVC, the field
should have been rejected by validation. This was overlooked when introducing
it, so now persisted objects might have it set and/or people may have started
to rely on it being accepted even when it has no effect.

Therefore we cannot reject it in validation anymore, but we can still strip
it out on create or update.
---
 pkg/api/persistentvolumeclaim/util.go      |  4 +++
 pkg/api/persistentvolumeclaim/util_test.go |  6 ++++
 pkg/api/pod/util.go                        | 10 ++++++
 pkg/api/pod/util_test.go                   | 38 ++++++++++++++++++++++
 pkg/apis/core/types.go                     |  2 +-
 staging/src/k8s.io/api/core/v1/types.go    |  2 +-
 6 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go
index 4334afcca59..1304c1fe048 100644
--- a/pkg/api/persistentvolumeclaim/util.go
+++ b/pkg/api/persistentvolumeclaim/util.go
@@ -49,6 +49,10 @@ func DropDisabledFields(pvcSpec, oldPVCSpec *core.PersistentVolumeClaimSpec) {
 			pvcSpec.DataSourceRef = nil
 		}
 	}
+
+	// Setting VolumeClaimTemplate.Resources.Claims should have been caught by validation when
+	// extending ResourceRequirements in 1.26. Now we can only accept it and drop the field.
+	pvcSpec.Resources.Claims = nil
 }
 
 // EnforceDataSourceBackwardsCompatibility drops the data source field under certain conditions
diff --git a/pkg/api/persistentvolumeclaim/util_test.go b/pkg/api/persistentvolumeclaim/util_test.go
index 945ff2f0446..ebbb0fecb5c 100644
--- a/pkg/api/persistentvolumeclaim/util_test.go
+++ b/pkg/api/persistentvolumeclaim/util_test.go
@@ -263,6 +263,9 @@ func TestDataSourceFilter(t *testing.T) {
 			anyEnabled: true,
 			wantRef:    xnsVolumeDataSourceRef, // existing field isn't dropped.
 		},
+		"clear Resources.Claims": {
+			spec: core.PersistentVolumeClaimSpec{Resources: core.ResourceRequirements{Claims: []core.ResourceClaim{{Name: "dra"}}}},
+		},
 	}
 
 	for testName, test := range tests {
@@ -278,6 +281,9 @@ func TestDataSourceFilter(t *testing.T) {
 				t.Errorf("expected condition was not met, test: %s, anyEnabled: %v, xnsEnabled: %v, spec: %+v, expected DataSourceRef: %+v",
 					testName, test.anyEnabled, test.xnsEnabled, test.spec, test.wantRef)
 			}
+			if test.spec.Resources.Claims != nil {
+				t.Errorf("expected Resources.Claims to be cleared")
+			}
 		})
 	}
 }
diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go
index 9cc7aeb1156..95d9f876b77 100644
--- a/pkg/api/pod/util.go
+++ b/pkg/api/pod/util.go
@@ -577,6 +577,16 @@ func dropDisabledDynamicResourceAllocationFields(podSpec, oldPodSpec *api.PodSpe
 		dropEphemeralResourceClaimRequests(podSpec.EphemeralContainers)
 		podSpec.ResourceClaims = nil
 	}
+
+	// Setting VolumeClaimTemplate.Resources.Claims should have been
+	// treated as an error by validation when extending
+	// ResourceRequirements in 1.26. Now we can only accept it and drop the
+	// field.
+	for i := range podSpec.Volumes {
+		if podSpec.Volumes[i].Ephemeral != nil && podSpec.Volumes[i].Ephemeral.VolumeClaimTemplate != nil {
+			podSpec.Volumes[i].Ephemeral.VolumeClaimTemplate.Spec.Resources.Claims = nil
+		}
+	}
 }
 
 func dynamicResourceAllocationInUse(podSpec *api.PodSpec) bool {
diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go
index 17ae3fe78ad..5def1ade4d8 100644
--- a/pkg/api/pod/util_test.go
+++ b/pkg/api/pod/util_test.go
@@ -2236,3 +2236,41 @@ func TestValidateTopologySpreadConstraintLabelSelectorOption(t *testing.T) {
 		})
 	}
 }
+
+func TestDropVolumesClaimField(t *testing.T) {
+	pod := &api.Pod{
+		Spec: api.PodSpec{
+			Volumes: []api.Volume{
+				{},
+				{
+					VolumeSource: api.VolumeSource{
+						Ephemeral: &api.EphemeralVolumeSource{},
+					},
+				},
+				{
+					VolumeSource: api.VolumeSource{
+						Ephemeral: &api.EphemeralVolumeSource{
+							VolumeClaimTemplate: &api.PersistentVolumeClaimTemplate{
+								Spec: api.PersistentVolumeClaimSpec{
+									Resources: api.ResourceRequirements{
+										Claims: []api.ResourceClaim{
+											{Name: "dra"},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	DropDisabledPodFields(pod, nil)
+
+	for i, volume := range pod.Spec.Volumes {
+		if volume.Ephemeral != nil && volume.Ephemeral.VolumeClaimTemplate != nil && volume.Ephemeral.VolumeClaimTemplate.Spec.Resources.Claims != nil {
+			t.Errorf("volume #%d: Resources.Claim should be nil", i)
+		}
+	}
+}
diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go
index 45514929eba..7b0ea2fb247 100644
--- a/pkg/apis/core/types.go
+++ b/pkg/apis/core/types.go
@@ -2190,7 +2190,7 @@ type ResourceRequirements struct {
 	// This is an alpha field and requires enabling the
 	// DynamicResourceAllocation feature gate.
 	//
-	// This field is immutable.
+	// This field is immutable. It can only be set for containers.
 	//
 	// +featureGate=DynamicResourceAllocation
 	// +optional
diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go
index b03840ce62e..63d92f2e5c1 100644
--- a/staging/src/k8s.io/api/core/v1/types.go
+++ b/staging/src/k8s.io/api/core/v1/types.go
@@ -2319,7 +2319,7 @@ type ResourceRequirements struct {
 	// This is an alpha field and requires enabling the
 	// DynamicResourceAllocation feature gate.
 	//
-	// This field is immutable.
+	// This field is immutable. It can only be set for containers.
 	//
 	// +listType=map
 	// +listMapKey=name