From 3790ee2fe835fbd7dc32052416af5a86a1757c3d Mon Sep 17 00:00:00 2001 From: Sergey Kanzhelev Date: Mon, 22 Jul 2024 05:21:23 +0000 Subject: [PATCH] reset fields when the feature gate was not set --- pkg/api/pod/util.go | 27 +++ pkg/apis/core/validation/validation.go | 79 ++++++++ pkg/apis/core/validation/validation_test.go | 206 ++++++++++++++++++++ 3 files changed, 312 insertions(+) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 6b8b1e9580d..4c79e6381f3 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -813,6 +813,17 @@ func dropDisabledPodStatusFields(podStatus, oldPodStatus *api.PodStatus, podSpec } } + if !utilfeature.DefaultFeatureGate.Enabled(features.ResourceHealthStatus) { + setAllocatedResourcesStatusToNil := func(csl []api.ContainerStatus) { + for i := range csl { + csl[i].AllocatedResourcesStatus = nil + } + } + setAllocatedResourcesStatusToNil(podStatus.ContainerStatuses) + setAllocatedResourcesStatusToNil(podStatus.InitContainerStatuses) + setAllocatedResourcesStatusToNil(podStatus.EphemeralContainerStatuses) + } + // drop ContainerStatus.User field to empty (disable SupplementalGroupsPolicy) if !utilfeature.DefaultFeatureGate.Enabled(features.SupplementalGroupsPolicy) && !supplementalGroupsPolicyInUse(oldPodSpec) { dropUserField := func(csl []api.ContainerStatus) { @@ -1161,6 +1172,22 @@ func rroInUse(podSpec *api.PodSpec) bool { return inUse } +func allocatedResourcesStatusInUse(podSpec *api.PodStatus) bool { + if podSpec == nil { + return false + } + inUse := func(csl []api.ContainerStatus) bool { + for _, cs := range csl { + if len(cs.AllocatedResourcesStatus) > 0 { + return true + } + } + return false + } + + return inUse(podSpec.ContainerStatuses) || inUse(podSpec.InitContainerStatuses) || inUse(podSpec.EphemeralContainerStatuses) +} + func dropDisabledClusterTrustBundleProjection(podSpec, oldPodSpec *api.PodSpec) { if utilfeature.DefaultFeatureGate.Enabled(features.ClusterTrustBundleProjection) { return diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 6be9b84a17e..abbcf0916d7 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5378,6 +5378,11 @@ func ValidatePodStatusUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions allErrs = append(allErrs, validateContainerStatusUsers(newPod.Status.InitContainerStatuses, fldPath.Child("initContainerStatuses"), newPod.Spec.OS)...) allErrs = append(allErrs, validateContainerStatusUsers(newPod.Status.EphemeralContainerStatuses, fldPath.Child("ephemeralContainerStatuses"), newPod.Spec.OS)...) + allErrs = append(allErrs, validateContainerStatusAllocatedResourcesStatus(newPod.Status.ContainerStatuses, fldPath.Child("containerStatuses"), newPod.Spec.Containers)...) + allErrs = append(allErrs, validateContainerStatusAllocatedResourcesStatus(newPod.Status.InitContainerStatuses, fldPath.Child("initContainerStatuses"), newPod.Spec.InitContainers)...) + // ephemeral containers are not allowed to have resources allocated + allErrs = append(allErrs, validateContainerStatusNoAllocatedResourcesStatus(newPod.Status.EphemeralContainerStatuses, fldPath.Child("ephemeralContainerStatuses"))...) + return allErrs } @@ -8200,6 +8205,80 @@ func validateContainerStatusUsers(containerStatuses []core.ContainerStatus, fldP return allErrors } +func validateContainerStatusNoAllocatedResourcesStatus(containerStatuses []core.ContainerStatus, fldPath *field.Path) field.ErrorList { + allErrors := field.ErrorList{} + + for i, containerStatus := range containerStatuses { + if containerStatus.AllocatedResourcesStatus == nil { + continue + } else { + allErrors = append(allErrors, field.Forbidden(fldPath.Index(i).Child("allocatedResourcesStatus"), "cannot be set for a container status")) + } + } + + return allErrors +} + +// validateContainerStatusAllocatedResourcesStatus iterate the allocated resources health and validate: +// - resourceName matches one of resources in container's resource requirements +// - resourceID is not empty and unique +func validateContainerStatusAllocatedResourcesStatus(containerStatuses []core.ContainerStatus, fldPath *field.Path, containers []core.Container) field.ErrorList { + allErrors := field.ErrorList{} + + for i, containerStatus := range containerStatuses { + if containerStatus.AllocatedResourcesStatus == nil { + continue + } + + allocatedResources := containerStatus.AllocatedResourcesStatus + for j, allocatedResource := range allocatedResources { + var container core.Container + containerFound := false + // get container by name + for _, c := range containers { + if c.Name == containerStatus.Name { + containerFound = true + container = c + break + } + } + + // ignore missing container, see https://github.com/kubernetes/kubernetes/issues/124915 + if containerFound { + found := false + + // get container resources from the spec + containerResources := container.Resources + for resourceName := range containerResources.Requests { + if resourceName == allocatedResource.Name { + found = true + break + } + } + if !found { + allErrors = append(allErrors, field.Invalid(fldPath.Index(i).Child("allocatedResourcesStatus").Index(j).Child("name"), allocatedResource.Name, "must match one of the container's resource requirements")) + } + } + + uniqueResources := sets.New[core.ResourceID]() + // check resource IDs are unique + for k, r := range allocatedResource.Resources { + if r.Health != core.ResourceHealthStatusHealthy && r.Health != core.ResourceHealthStatusUnhealthy && r.Health != core.ResourceHealthStatusUnknown { + allErrors = append(allErrors, field.Invalid(fldPath.Index(i).Child("allocatedResourcesStatus").Index(j).Child("resources").Index(k).Child("health"), r.Health, "must be one of Healthy, Unhealthy, Unknown")) + } + + if uniqueResources.Has(r.ResourceID) { + allErrors = append(allErrors, field.Invalid(fldPath.Index(i).Child("allocatedResourcesStatus").Index(j).Child("resources").Index(k).Child("resourceID"), r.ResourceID, "must be unique")) + } else { + uniqueResources.Insert(r.ResourceID) + } + } + } + } + + return allErrors +} + func validateLinuxContainerUser(linuxContainerUser *core.LinuxContainerUser, fldPath *field.Path) field.ErrorList { allErrors := field.ErrorList{} if linuxContainerUser == nil { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index f7a724a1683..0573f0addc5 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -24423,3 +24423,209 @@ func TestValidatePodStatusUpdateWithSupplementalGroupsPolicy(t *testing.T) { } } } +func TestValidateContainerStatusNoAllocatedResourcesStatus(t *testing.T) { + containerStatuses := []core.ContainerStatus{ + { + Name: "container-1", + }, + { + Name: "container-2", + AllocatedResourcesStatus: []core.ResourceStatus{ + { + Name: "test.device/test", + Resources: nil, + }, + }, + }, + { + Name: "container-3", + AllocatedResourcesStatus: []core.ResourceStatus{ + { + Name: "test.device/test", + Resources: []core.ResourceHealth{ + { + ResourceID: "resource-1", + Health: core.ResourceHealthStatusHealthy, + }, + }, + }, + }, + }, + } + + fldPath := field.NewPath("spec", "containers") + + errs := validateContainerStatusNoAllocatedResourcesStatus(containerStatuses, fldPath) + + assert.Equal(t, 2, len(errs)) + assert.Equal(t, "spec.containers[1].allocatedResourcesStatus", errs[0].Field) + assert.Equal(t, "cannot be set for a container status", errs[0].Detail) + assert.Equal(t, "spec.containers[2].allocatedResourcesStatus", errs[1].Field) + assert.Equal(t, "cannot be set for a container status", errs[1].Detail) +} + +func TestValidateContainerStatusAllocatedResourcesStatus(t *testing.T) { + fldPath := field.NewPath("spec", "containers") + + testCases := map[string]struct { + containers []core.Container + containerStatuses []core.ContainerStatus + wantFieldErrors field.ErrorList + }{ + "basic correct status": { + containers: []core.Container{ + { + Name: "container-1", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + "test.device/test": resource.MustParse("1"), + }, + }, + }, + }, + containerStatuses: []core.ContainerStatus{ + { + Name: "container-1", + AllocatedResourcesStatus: []core.ResourceStatus{ + { + Name: "test.device/test", + Resources: []core.ResourceHealth{ + { + ResourceID: "resource-1", + Health: core.ResourceHealthStatusHealthy, + }, + }, + }, + }, + }, + }, + wantFieldErrors: field.ErrorList{}, + }, + "ignoring the missing container (see https://github.com/kubernetes/kubernetes/issues/124915)": { + containers: []core.Container{ + { + Name: "container-2", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + "test.device/test": resource.MustParse("1"), + }, + }, + }, + }, + containerStatuses: []core.ContainerStatus{ + { + Name: "container-1", + AllocatedResourcesStatus: []core.ResourceStatus{ + { + Name: "test.device/test", + Resources: []core.ResourceHealth{ + { + ResourceID: "resource-1", + Health: core.ResourceHealthStatusHealthy, + }, + }, + }, + }, + }, + }, + wantFieldErrors: field.ErrorList{}, + }, + "allow nil": { + containers: []core.Container{ + { + Name: "container-2", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + "test.device/test": resource.MustParse("1"), + }, + }, + }, + }, + containerStatuses: []core.ContainerStatus{ + { + Name: "container-1", + }, + }, + wantFieldErrors: field.ErrorList{}, + }, + "don't allow non-unique IDs": { + containers: []core.Container{ + { + Name: "container-2", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + "test.device/test": resource.MustParse("1"), + }, + }, + }, + }, + containerStatuses: []core.ContainerStatus{ + { + Name: "container-1", + AllocatedResourcesStatus: []core.ResourceStatus{ + { + Name: "test.device/test", + Resources: []core.ResourceHealth{ + { + ResourceID: "resource-1", + Health: core.ResourceHealthStatusHealthy, + }, + { + ResourceID: "resource-1", + Health: core.ResourceHealthStatusUnhealthy, + }, + }, + }, + }, + }, + }, + wantFieldErrors: field.ErrorList{ + field.Invalid(fldPath.Index(0).Child("allocatedResourcesStatus").Index(0).Child("resources").Index(1).Child("resourceID"), core.ResourceID("resource-1"), "must be unique"), + }, + }, + + "don't allow resources that are not in spec": { + containers: []core.Container{ + { + Name: "container-1", + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + "test.device/test": resource.MustParse("1"), + }, + }, + }, + }, + containerStatuses: []core.ContainerStatus{ + { + Name: "container-1", + AllocatedResourcesStatus: []core.ResourceStatus{ + { + Name: "test.device/test", + Resources: []core.ResourceHealth{ + { + ResourceID: "resource-1", + Health: core.ResourceHealthStatusHealthy, + }, + }, + }, + { + Name: "test.device/test2", + Resources: []core.ResourceHealth{}, + }, + }, + }, + }, + wantFieldErrors: field.ErrorList{ + field.Invalid(fldPath.Index(0).Child("allocatedResourcesStatus").Index(1).Child("name"), core.ResourceName("test.device/test2"), "must match one of the container's resource requirements"), + }, + }, + } + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { + errs := validateContainerStatusAllocatedResourcesStatus(tt.containerStatuses, fldPath, tt.containers) + if diff := cmp.Diff(tt.wantFieldErrors, errs); diff != "" { + t.Errorf("unexpected field errors (-want, +got):\n%s", diff) + } + }) + } +}