From e97531b34940bab2a3965def074013985aeeaf38 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 14 Mar 2023 11:58:41 +0100 Subject: [PATCH] api: extend validation of dynamic resource allocation fields in PodSpec The generated ResourceClaim name and the names of the ResourceClaimTemplate and ResourceClaim referenced by a pod must be valid according to the resource API, otherwise the pod cannot start. Checking this was removed from the original implementation out of concerns about validating fields in core against limitations imposed by a separate, alpha API. But as this was pointed out again in https://github.com/kubernetes/kubernetes/pull/116254#discussion_r1134010324 it gets added back. The same strings that worked before still work now. In particular, the constraints for a spec.resourceClaim.name are still the same (DNS label). --- pkg/apis/core/validation/validation.go | 36 ++++++-- pkg/apis/core/validation/validation_test.go | 95 +++++++++++++++------ pkg/apis/resource/validation/validation.go | 12 +-- 3 files changed, 101 insertions(+), 42 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 61d0fb3af42..c506cda1985 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -299,6 +299,14 @@ var ValidateClassName = apimachineryvalidation.NameIsDNSSubdomain // class name is valid. var ValidatePriorityClassName = apimachineryvalidation.NameIsDNSSubdomain +// ValidateResourceClaimName can be used to check whether the given +// name for a ResourceClaim is valid. +var ValidateResourceClaimName = apimachineryvalidation.NameIsDNSSubdomain + +// ValidateResourceClaimTemplateName can be used to check whether the given +// name for a ResourceClaimTemplate is valid. +var ValidateResourceClaimTemplateName = apimachineryvalidation.NameIsDNSSubdomain + // ValidateRuntimeClassName can be used to check whether the given RuntimeClass name is valid. // Prefix indicates this name will be used as part of generation, in which case // trailing dashes are allowed. @@ -2747,11 +2755,11 @@ func ValidateVolumeDevices(devices []core.VolumeDevice, volmounts map[string]str return allErrs } -func validatePodResourceClaims(claims []core.PodResourceClaim, fldPath *field.Path) field.ErrorList { +func validatePodResourceClaims(podMeta *metav1.ObjectMeta, claims []core.PodResourceClaim, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList podClaimNames := sets.NewString() for i, claim := range claims { - allErrs = append(allErrs, validatePodResourceClaim(claim, &podClaimNames, fldPath.Index(i))...) + allErrs = append(allErrs, validatePodResourceClaim(podMeta, claim, &podClaimNames, fldPath.Index(i))...) } return allErrs } @@ -2769,14 +2777,22 @@ func gatherPodResourceClaimNames(claims []core.PodResourceClaim) sets.String { return podClaimNames } -func validatePodResourceClaim(claim core.PodResourceClaim, podClaimNames *sets.String, fldPath *field.Path) field.ErrorList { +func validatePodResourceClaim(podMeta *metav1.ObjectMeta, claim core.PodResourceClaim, podClaimNames *sets.String, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList if claim.Name == "" { allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) } else if podClaimNames.Has(claim.Name) { allErrs = append(allErrs, field.Duplicate(fldPath.Child("name"), claim.Name)) } else { - allErrs = append(allErrs, ValidateDNS1123Label(claim.Name, fldPath.Child("name"))...) + nameErrs := ValidateDNS1123Label(claim.Name, fldPath.Child("name")) + if len(nameErrs) > 0 { + allErrs = append(allErrs, nameErrs...) + } else if podMeta != nil && claim.Source.ResourceClaimTemplateName != nil { + claimName := podMeta.Name + "-" + claim.Name + for _, detail := range ValidateResourceClaimName(claimName, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), claimName, "final ResourceClaim name: "+detail)) + } + } podClaimNames.Insert(claim.Name) } allErrs = append(allErrs, validatePodResourceClaimSource(claim.Source, fldPath.Child("source"))...) @@ -2792,6 +2808,16 @@ func validatePodResourceClaimSource(claimSource core.ClaimSource, fldPath *field if claimSource.ResourceClaimName == nil && claimSource.ResourceClaimTemplateName == nil { allErrs = append(allErrs, field.Invalid(fldPath, claimSource, "must specify one of: `resourceClaimName`, `resourceClaimTemplateName`")) } + if claimSource.ResourceClaimName != nil { + for _, detail := range ValidateResourceClaimName(*claimSource.ResourceClaimName, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceClaimName"), *claimSource.ResourceClaimName, detail)) + } + } + if claimSource.ResourceClaimTemplateName != nil { + for _, detail := range ValidateResourceClaimTemplateName(*claimSource.ResourceClaimTemplateName, false) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceClaimTemplateName"), *claimSource.ResourceClaimTemplateName, detail)) + } + } return allErrs } @@ -3759,7 +3785,7 @@ func ValidatePodSpec(spec *core.PodSpec, podMeta *metav1.ObjectMeta, fldPath *fi vols, vErrs := ValidateVolumes(spec.Volumes, podMeta, fldPath.Child("volumes"), opts) allErrs = append(allErrs, vErrs...) podClaimNames := gatherPodResourceClaimNames(spec.ResourceClaims) - allErrs = append(allErrs, validatePodResourceClaims(spec.ResourceClaims, fldPath.Child("resourceClaims"))...) + allErrs = append(allErrs, validatePodResourceClaims(podMeta, spec.ResourceClaims, fldPath.Child("resourceClaims"))...) allErrs = append(allErrs, validateContainers(spec.Containers, vols, podClaimNames, fldPath.Child("containers"), opts)...) allErrs = append(allErrs, validateInitContainers(spec.InitContainers, spec.Containers, vols, podClaimNames, fldPath.Child("initContainers"), opts)...) allErrs = append(allErrs, validateEphemeralContainers(spec.EphemeralContainers, spec.Containers, spec.InitContainers, vols, podClaimNames, fldPath.Child("ephemeralContainers"), opts)...) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index d20f0ffa532..abab4f2d1cd 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -23680,34 +23680,42 @@ func TestValidateDynamicResourceAllocation(t *testing.T) { goodClaimSource := core.ClaimSource{ ResourceClaimName: &externalClaimName, } + shortPodName := &metav1.ObjectMeta{ + Name: "some-pod", + } + brokenPodName := &metav1.ObjectMeta{ + Name: ".dot.com", + } + goodClaimTemplate := core.PodSpec{ + Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", Resources: core.ResourceRequirements{Claims: []core.ResourceClaim{{Name: "my-claim-template"}}}}}, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + ResourceClaims: []core.PodResourceClaim{ + { + Name: "my-claim-template", + Source: core.ClaimSource{ + ResourceClaimTemplateName: &externalClaimTemplateName, + }, + }, + }, + } + goodClaimReference := core.PodSpec{ + Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", Resources: core.ResourceRequirements{Claims: []core.ResourceClaim{{Name: "my-claim-reference"}}}}}, + RestartPolicy: core.RestartPolicyAlways, + DNSPolicy: core.DNSClusterFirst, + ResourceClaims: []core.PodResourceClaim{ + { + Name: "my-claim-reference", + Source: core.ClaimSource{ + ResourceClaimName: &externalClaimName, + }, + }, + }, + } successCases := map[string]core.PodSpec{ - "resource claim reference": { - Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", Resources: core.ResourceRequirements{Claims: []core.ResourceClaim{{Name: "my-claim"}}}}}, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, - ResourceClaims: []core.PodResourceClaim{ - { - Name: "my-claim", - Source: core.ClaimSource{ - ResourceClaimName: &externalClaimName, - }, - }, - }, - }, - "resource claim template": { - Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", Resources: core.ResourceRequirements{Claims: []core.ResourceClaim{{Name: "my-claim"}}}}}, - RestartPolicy: core.RestartPolicyAlways, - DNSPolicy: core.DNSClusterFirst, - ResourceClaims: []core.PodResourceClaim{ - { - Name: "my-claim", - Source: core.ClaimSource{ - ResourceClaimTemplateName: &externalClaimTemplateName, - }, - }, - }, - }, + "resource claim reference": goodClaimTemplate, + "resource claim template": goodClaimTemplate, "multiple claims": { Containers: []core.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File", Resources: core.ResourceRequirements{Claims: []core.ResourceClaim{{Name: "my-claim"}, {Name: "another-claim"}}}}}, RestartPolicy: core.RestartPolicyAlways, @@ -23738,7 +23746,7 @@ func TestValidateDynamicResourceAllocation(t *testing.T) { } for k, v := range successCases { t.Run(k, func(t *testing.T) { - if errs := ValidatePodSpec(&v, nil, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { + if errs := ValidatePodSpec(&v, shortPodName, field.NewPath("field"), PodValidationOptions{}); len(errs) != 0 { t.Errorf("expected success: %v", errs) } }) @@ -23883,10 +23891,43 @@ func TestValidateDynamicResourceAllocation(t *testing.T) { }, }, }, + "invalid claim template name": func() core.PodSpec { + spec := goodClaimTemplate.DeepCopy() + notLabel := ".foo_bar" + spec.ResourceClaims[0].Source.ResourceClaimTemplateName = ¬Label + return *spec + }(), + "invalid claim reference name": func() core.PodSpec { + spec := goodClaimReference.DeepCopy() + notLabel := ".foo_bar" + spec.ResourceClaims[0].Source.ResourceClaimName = ¬Label + return *spec + }(), } for k, v := range failureCases { if errs := ValidatePodSpec(&v, nil, field.NewPath("field"), PodValidationOptions{}); len(errs) == 0 { t.Errorf("expected failure for %q", k) } } + + t.Run("generated-claim-name", func(t *testing.T) { + for _, spec := range []*core.PodSpec{&goodClaimTemplate, &goodClaimReference} { + claimName := spec.ResourceClaims[0].Name + t.Run(claimName, func(t *testing.T) { + for _, podMeta := range []*metav1.ObjectMeta{shortPodName, brokenPodName} { + t.Run(podMeta.Name, func(t *testing.T) { + errs := ValidatePodSpec(spec, podMeta, field.NewPath("field"), PodValidationOptions{}) + // Only one out of the four combinations fails. + expectError := spec == &goodClaimTemplate && podMeta == brokenPodName + if expectError && len(errs) == 0 { + t.Error("did not get the expected failure") + } + if !expectError && len(errs) > 0 { + t.Errorf("unexpected failures: %+v", errs) + } + }) + } + }) + } + }) } diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index c82093d76bc..283779dc9b7 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -25,21 +25,13 @@ import ( "k8s.io/kubernetes/pkg/apis/resource" ) -// validateResourceClaimName can be used to check whether the given -// name for a ResourceClaim is valid. -var validateResourceClaimName = apimachineryvalidation.NameIsDNSSubdomain - -// validateResourceClaimTemplateName can be used to check whether the given -// name for a ResourceClaimTemplate is valid. -var validateResourceClaimTemplateName = apimachineryvalidation.NameIsDNSSubdomain - // validateResourceDriverName reuses the validation of a CSI driver because // the allowed values are exactly the same. var validateResourceDriverName = corevalidation.ValidateCSIDriverName // ValidateClaim validates a ResourceClaim. func ValidateClaim(resourceClaim *resource.ResourceClaim) field.ErrorList { - allErrs := corevalidation.ValidateObjectMeta(&resourceClaim.ObjectMeta, true, validateResourceClaimName, field.NewPath("metadata")) + allErrs := corevalidation.ValidateObjectMeta(&resourceClaim.ObjectMeta, true, corevalidation.ValidateResourceClaimName, field.NewPath("metadata")) allErrs = append(allErrs, validateResourceClaimSpec(&resourceClaim.Spec, field.NewPath("spec"))...) return allErrs } @@ -304,7 +296,7 @@ func validatePodSchedulingClaim(status resource.ResourceClaimSchedulingStatus, f // ValidateClaimTemplace validates a ResourceClaimTemplate. func ValidateClaimTemplate(template *resource.ResourceClaimTemplate) field.ErrorList { - allErrs := corevalidation.ValidateObjectMeta(&template.ObjectMeta, true, validateResourceClaimTemplateName, field.NewPath("metadata")) + allErrs := corevalidation.ValidateObjectMeta(&template.ObjectMeta, true, corevalidation.ValidateResourceClaimTemplateName, field.NewPath("metadata")) allErrs = append(allErrs, validateResourceClaimTemplateSpec(&template.Spec, field.NewPath("spec"))...) return allErrs }