From 7f840f4441957a6024f28262fafcde4696dac6c3 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 21 Mar 2018 01:20:34 -0400 Subject: [PATCH] Deprecate repair-malformed-updates flag, move object meta mutation into BeforeCreate --- cmd/kube-apiserver/app/options/BUILD | 1 - cmd/kube-apiserver/app/options/options.go | 10 ++--- pkg/apis/core/validation/validation.go | 4 -- pkg/apis/core/validation/validation_test.go | 2 +- pkg/registry/core/service/storage/rest.go | 5 +-- .../pkg/api/validation/objectmeta.go | 39 +------------------ .../pkg/api/validation/objectmeta_test.go | 24 ++++++------ .../apiserver/pkg/registry/rest/update.go | 16 ++++++++ 8 files changed, 37 insertions(+), 64 deletions(-) diff --git a/cmd/kube-apiserver/app/options/BUILD b/cmd/kube-apiserver/app/options/BUILD index 5459e1ce7d..265f078412 100644 --- a/cmd/kube-apiserver/app/options/BUILD +++ b/cmd/kube-apiserver/app/options/BUILD @@ -16,7 +16,6 @@ go_library( deps = [ "//pkg/api/legacyscheme:go_default_library", "//pkg/apis/core:go_default_library", - "//pkg/apis/core/validation:go_default_library", "//pkg/features:go_default_library", "//pkg/kubeapiserver/options:go_default_library", "//pkg/kubelet/client:go_default_library", diff --git a/cmd/kube-apiserver/app/options/options.go b/cmd/kube-apiserver/app/options/options.go index 4a68bb78eb..45008378cf 100644 --- a/cmd/kube-apiserver/app/options/options.go +++ b/cmd/kube-apiserver/app/options/options.go @@ -26,7 +26,6 @@ import ( genericoptions "k8s.io/apiserver/pkg/server/options" "k8s.io/apiserver/pkg/storage/storagebackend" api "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/apis/core/validation" kubeoptions "k8s.io/kubernetes/pkg/kubeapiserver/options" kubeletclient "k8s.io/kubernetes/pkg/kubelet/client" "k8s.io/kubernetes/pkg/master/ports" @@ -213,11 +212,10 @@ func (s *ServerRunOptions) AddFlags(fs *pflag.FlagSet) { fs.StringVar(&s.KubeletConfig.CAFile, "kubelet-certificate-authority", s.KubeletConfig.CAFile, "Path to a cert file for the certificate authority.") - // TODO: delete this flag as soon as we identify and fix all clients that send malformed updates, like #14126. - fs.BoolVar(&validation.RepairMalformedUpdates, "repair-malformed-updates", validation.RepairMalformedUpdates, ""+ - "If true, server will do its best to fix the update request to pass the validation, "+ - "e.g., setting empty UID in update request to its existing value. This flag can be turned off "+ - "after we fix all the clients that send malformed updates.") + // TODO: delete this flag in 1.13 + repair := false + fs.BoolVar(&repair, "repair-malformed-updates", false, "deprecated") + fs.MarkDeprecated("repair-malformed-updates", "This flag will be removed in a future version") fs.StringVar(&s.ProxyClientCertFile, "proxy-client-cert-file", s.ProxyClientCertFile, ""+ "Client certificate used to prove the identity of the aggregator or kube-apiserver "+ diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 9238b9cf0b..4c9b343778 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -55,10 +55,6 @@ import ( "k8s.io/kubernetes/pkg/security/apparmor" ) -// TODO: delete this global variable when we enable the validation of common -// fields by default. -var RepairMalformedUpdates bool = apimachineryvalidation.RepairMalformedUpdates - const isNegativeErrorMsg string = apimachineryvalidation.IsNegativeErrorMsg const isInvalidQuotaResource string = `must be a standard resource for quota` const fieldImmutableErrorMsg string = apimachineryvalidation.FieldImmutableErrorMsg diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 5a12c19480..1e955e5bfd 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -7513,7 +7513,7 @@ func TestValidatePodUpdate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "foo", DeletionTimestamp: &now}, Spec: core.PodSpec{Containers: []core.Container{{Image: "foo:V1"}}}, }, - "", + "metadata.deletionTimestamp", "deletion timestamp removed", }, { diff --git a/pkg/registry/core/service/storage/rest.go b/pkg/registry/core/service/storage/rest.go index 0190a8b84d..e818b577bd 100644 --- a/pkg/registry/core/service/storage/rest.go +++ b/pkg/registry/core/service/storage/rest.go @@ -339,9 +339,8 @@ func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObj } // Copy over non-user fields - // TODO: make this a merge function - if errs := validation.ValidateServiceUpdate(service, oldService); len(errs) > 0 { - return nil, false, errors.NewInvalid(api.Kind("Service"), service.Name, errs) + if err := rest.BeforeUpdate(registry.Strategy, ctx, service, oldService); err != nil { + return nil, false, err } // TODO: this should probably move to strategy.PrepareForCreate() diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go b/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go index 3c32a937ac..44b9b16006 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go @@ -30,10 +30,6 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// TODO: delete this global variable when we enable the validation of common -// fields by default. -var RepairMalformedUpdates bool = true - const FieldImmutableErrorMsg string = `field is immutable` const totalAnnotationSizeLimitB int = 256 * (1 << 10) // 256 kB @@ -254,39 +250,6 @@ func ValidateObjectMetaUpdate(newMeta, oldMeta *metav1.ObjectMeta, fldPath *fiel func ValidateObjectMetaAccessorUpdate(newMeta, oldMeta metav1.Object, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList - if !RepairMalformedUpdates && newMeta.GetUID() != oldMeta.GetUID() { - allErrs = append(allErrs, field.Invalid(fldPath.Child("uid"), newMeta.GetUID(), "field is immutable")) - } - // in the event it is left empty, set it, to allow clients more flexibility - // TODO: remove the following code that repairs the update request when we retire the clients that modify the immutable fields. - // Please do not copy this pattern elsewhere; validation functions should not be modifying the objects they are passed! - if RepairMalformedUpdates { - if len(newMeta.GetUID()) == 0 { - newMeta.SetUID(oldMeta.GetUID()) - } - // ignore changes to timestamp - if oldCreationTime := oldMeta.GetCreationTimestamp(); oldCreationTime.IsZero() { - oldMeta.SetCreationTimestamp(newMeta.GetCreationTimestamp()) - } else { - newMeta.SetCreationTimestamp(oldMeta.GetCreationTimestamp()) - } - // an object can never remove a deletion timestamp or clear/change grace period seconds - if !oldMeta.GetDeletionTimestamp().IsZero() { - newMeta.SetDeletionTimestamp(oldMeta.GetDeletionTimestamp()) - } - if oldMeta.GetDeletionGracePeriodSeconds() != nil && newMeta.GetDeletionGracePeriodSeconds() == nil { - newMeta.SetDeletionGracePeriodSeconds(oldMeta.GetDeletionGracePeriodSeconds()) - } - } - - // TODO: needs to check if newMeta==nil && oldMeta !=nil after the repair logic is removed. - if newMeta.GetDeletionGracePeriodSeconds() != nil && (oldMeta.GetDeletionGracePeriodSeconds() == nil || *newMeta.GetDeletionGracePeriodSeconds() != *oldMeta.GetDeletionGracePeriodSeconds()) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("deletionGracePeriodSeconds"), newMeta.GetDeletionGracePeriodSeconds(), "field is immutable; may only be changed via deletion")) - } - if newMeta.GetDeletionTimestamp() != nil && (oldMeta.GetDeletionTimestamp() == nil || !newMeta.GetDeletionTimestamp().Equal(oldMeta.GetDeletionTimestamp())) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("deletionTimestamp"), newMeta.GetDeletionTimestamp(), "field is immutable; may only be changed via deletion")) - } - // Finalizers cannot be added if the object is already being deleted. if oldMeta.GetDeletionTimestamp() != nil { allErrs = append(allErrs, ValidateNoNewFinalizers(newMeta.GetFinalizers(), oldMeta.GetFinalizers(), fldPath.Child("finalizers"))...) @@ -308,6 +271,8 @@ func ValidateObjectMetaAccessorUpdate(newMeta, oldMeta metav1.Object, fldPath *f allErrs = append(allErrs, ValidateImmutableField(newMeta.GetNamespace(), oldMeta.GetNamespace(), fldPath.Child("namespace"))...) allErrs = append(allErrs, ValidateImmutableField(newMeta.GetUID(), oldMeta.GetUID(), fldPath.Child("uid"))...) allErrs = append(allErrs, ValidateImmutableField(newMeta.GetCreationTimestamp(), oldMeta.GetCreationTimestamp(), fldPath.Child("creationTimestamp"))...) + allErrs = append(allErrs, ValidateImmutableField(newMeta.GetDeletionTimestamp(), oldMeta.GetDeletionTimestamp(), fldPath.Child("deletionTimestamp"))...) + allErrs = append(allErrs, ValidateImmutableField(newMeta.GetDeletionGracePeriodSeconds(), oldMeta.GetDeletionGracePeriodSeconds(), fldPath.Child("deletionGracePeriodSeconds"))...) allErrs = append(allErrs, ValidateImmutableField(newMeta.GetClusterName(), oldMeta.GetClusterName(), fldPath.Child("clusterName"))...) allErrs = append(allErrs, v1validation.ValidateLabels(newMeta.GetLabels(), fldPath.Child("labels"))...) diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go b/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go index 9ec73b040c..ebd6c7e7c7 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go @@ -219,21 +219,21 @@ func TestValidateObjectMetaUpdateIgnoresCreationTimestamp(t *testing.T) { &metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, &metav1.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: metav1.NewTime(time.Unix(10, 0))}, field.NewPath("field"), - ); len(errs) != 0 { + ); len(errs) != 1 { t.Fatalf("unexpected errors: %v", errs) } if errs := ValidateObjectMetaUpdate( &metav1.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: metav1.NewTime(time.Unix(10, 0))}, &metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, field.NewPath("field"), - ); len(errs) != 0 { + ); len(errs) != 1 { t.Fatalf("unexpected errors: %v", errs) } if errs := ValidateObjectMetaUpdate( &metav1.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: metav1.NewTime(time.Unix(10, 0))}, &metav1.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: metav1.NewTime(time.Unix(11, 0))}, field.NewPath("field"), - ); len(errs) != 0 { + ); len(errs) != 1 { t.Fatalf("unexpected errors: %v", errs) } } @@ -328,38 +328,38 @@ func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, - ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: 1970-01-01 00:16:40 +0000 UTC: field is immutable; may only be changed via deletion"}, + ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: 1970-01-01 00:16:40 +0000 UTC: field is immutable"}, }, "invalid clear deletionTimestamp": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, - ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, - ExpectedErrs: []string{}, // no errors, validation copies the old value + ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, + ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"null\": field is immutable"}, }, "invalid change deletionTimestamp": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &later}, - ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, - ExpectedErrs: []string{}, // no errors, validation copies the old value + ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &later}, + ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: 1970-01-01 00:33:20 +0000 UTC: field is immutable"}, }, "invalid set deletionGracePeriodSeconds": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort}, ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort}, - ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: 30: field is immutable; may only be changed via deletion"}, + ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: 30: field is immutable"}, }, "invalid clear deletionGracePeriodSeconds": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, - ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort}, - ExpectedErrs: []string{}, // no errors, validation copies the old value + ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, + ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: \"null\": field is immutable"}, }, "invalid change deletionGracePeriodSeconds": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodLong}, ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodLong}, - ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: 40: field is immutable; may only be changed via deletion"}, + ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: 40: field is immutable"}, }, } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go index aa3e452c4c..b08dd14791 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go @@ -112,6 +112,22 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx context.Context, obj, old run // ClusterName is ignored and should not be saved objectMeta.SetClusterName("") + // Use the existing UID if none is provided + if len(objectMeta.GetUID()) == 0 { + objectMeta.SetUID(oldMeta.GetUID()) + } + // ignore changes to timestamp + if oldCreationTime := oldMeta.GetCreationTimestamp(); !oldCreationTime.IsZero() { + objectMeta.SetCreationTimestamp(oldMeta.GetCreationTimestamp()) + } + // an update can never remove/change a deletion timestamp + if !oldMeta.GetDeletionTimestamp().IsZero() { + objectMeta.SetDeletionTimestamp(oldMeta.GetDeletionTimestamp()) + } + // an update can never remove/change grace period seconds + if oldMeta.GetDeletionGracePeriodSeconds() != nil && objectMeta.GetDeletionGracePeriodSeconds() == nil { + objectMeta.SetDeletionGracePeriodSeconds(oldMeta.GetDeletionGracePeriodSeconds()) + } // Ensure some common fields, like UID, are validated for all resources. errs, err := validateCommonFields(obj, old, strategy)