add common fields validation before updaing a resource; make the repair of…

cmd/kube-apiserver/app/server.go

@@ -37,6 +37,7 @@ import (
 	"k8s.io/kubernetes/pkg/api/latest"
 	"k8s.io/kubernetes/pkg/api/meta"
 	apiutil "k8s.io/kubernetes/pkg/api/util"
+	"k8s.io/kubernetes/pkg/api/validation"
 	"k8s.io/kubernetes/pkg/apiserver"
 	"k8s.io/kubernetes/pkg/capabilities"
 	client "k8s.io/kubernetes/pkg/client/unversioned"
@@ -245,6 +246,8 @@ func (s *APIServer) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&s.KubeletConfig.CAFile, "kubelet-certificate-authority", s.KubeletConfig.CAFile, "Path to a cert. file for the certificate authority.")
 	//See #14282 for details on how to test/try this option out.  TODO remove this comment once this option is tested in CI.
 	fs.IntVar(&s.KubernetesServiceNodePort, "kubernetes-service-node-port", 0, "If non-zero, the Kubernetes master service (which apiserver creates/maintains) will be of type NodePort, using this as the value of the port. If zero, the Kubernetes master service will be of type ClusterIP.")
+	// 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", true, "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: Longer term we should read this from some config store, rather than a flag.

hack/verify-flags/known-flags.txt

@@ -303,4 +303,5 @@ cpu-cfs-quota
 terminated-pod-gc-threshold
 reconcile-cidr
 register-schedulable
+repair-malformed-updates
 

pkg/api/rest/update.go

@@ -19,6 +19,7 @@ package rest
 import (
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/errors"
+	"k8s.io/kubernetes/pkg/api/validation"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/util/fielderrors"
 )
@@ -44,6 +45,22 @@ type RESTUpdateStrategy interface {
 	AllowUnconditionalUpdate() bool
 }
 
+// TODO: add other common fields that require global validation.
+func validateCommonFields(obj, old runtime.Object) fielderrors.ValidationErrorList {
+	allErrs := fielderrors.ValidationErrorList{}
+	objectMeta, err := api.ObjectMetaFor(obj)
+	if err != nil {
+		return append(allErrs, errors.NewInternalError(err))
+	}
+	oldObjectMeta, err := api.ObjectMetaFor(old)
+	if err != nil {
+		return append(allErrs, errors.NewInternalError(err))
+	}
+	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(objectMeta, oldObjectMeta)...)
+
+	return allErrs
+}
+
 // BeforeUpdate ensures that common operations for all resources are performed on update. It only returns
 // errors that can be converted to api.Status. It will invoke update validation with the provided existing
 // and updated objects.
@@ -59,8 +76,14 @@ func BeforeUpdate(strategy RESTUpdateStrategy, ctx api.Context, obj, old runtime
 	} else {
 		objectMeta.Namespace = api.NamespaceNone
 	}
+
 	strategy.PrepareForUpdate(obj, old)
-	if errs := strategy.ValidateUpdate(ctx, obj, old); len(errs) > 0 {
+
+	// Ensure some common fields, like UID, are validated for all resources.
+	errs := validateCommonFields(obj, old)
+
+	errs = append(errs, strategy.ValidateUpdate(ctx, obj, old)...)
+	if len(errs) > 0 {
 		return errors.NewInvalid(kind, objectMeta.Name, errs)
 	}
 	return nil

pkg/api/validation/validation.go

@@ -38,6 +38,10 @@ import (
 	"github.com/golang/glog"
 )
 
+// TODO: delete this global variable when we enable the validation of common
+// fields by default.
+var RepairMalformedUpdates bool = true
+
 const cIdentifierErrorMsg string = `must be a C identifier (matching regex ` + validation.CIdentifierFmt + `): e.g. "my_name" or "MyName"`
 const isNegativeErrorMsg string = `must be non-negative`
 
@@ -231,6 +235,7 @@ func ValidatePositiveQuantity(value resource.Quantity, fieldName string) errs.Va
 
 // ValidateObjectMeta validates an object's metadata on creation. It expects that name generation has already
 // been performed.
+// TODO: Remove calls to this method scattered in validations of specific resources, e.g., ValidatePodUpdate.
 func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn ValidateNameFunc) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 
@@ -273,23 +278,32 @@ func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn Val
 func ValidateObjectMetaUpdate(new, old *api.ObjectMeta) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 
-	// in the event it is left empty, set it, to allow clients more flexibility
-	if len(new.UID) == 0 {
-		new.UID = old.UID
-	}
-	// ignore changes to timestamp
-	if old.CreationTimestamp.IsZero() {
-		old.CreationTimestamp = new.CreationTimestamp
-	} else {
-		new.CreationTimestamp = old.CreationTimestamp
-	}
-	// an object can never remove a deletion timestamp or clear/change grace period seconds
-	if !old.DeletionTimestamp.IsZero() {
-		new.DeletionTimestamp = old.DeletionTimestamp
+	if !RepairMalformedUpdates && new.UID != old.UID {
+		allErrs = append(allErrs, errs.NewFieldInvalid("uid", new.UID, "field is immutable"))
 	}
-	if old.DeletionGracePeriodSeconds != nil && new.DeletionGracePeriodSeconds == nil {
-		new.DeletionGracePeriodSeconds = old.DeletionGracePeriodSeconds
+	// 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(new.UID) == 0 {
+			new.UID = old.UID
+		}
+		// ignore changes to timestamp
+		if old.CreationTimestamp.IsZero() {
+			old.CreationTimestamp = new.CreationTimestamp
+		} else {
+			new.CreationTimestamp = old.CreationTimestamp
+		}
+		// an object can never remove a deletion timestamp or clear/change grace period seconds
+		if !old.DeletionTimestamp.IsZero() {
+			new.DeletionTimestamp = old.DeletionTimestamp
+		}
+		if old.DeletionGracePeriodSeconds != nil && new.DeletionGracePeriodSeconds == nil {
+			new.DeletionGracePeriodSeconds = old.DeletionGracePeriodSeconds
+		}
 	}
+
+	// TODO: needs to check if new==nil && old !=nil after the repair logic is removed.
 	if new.DeletionGracePeriodSeconds != nil && old.DeletionGracePeriodSeconds != nil && *new.DeletionGracePeriodSeconds != *old.DeletionGracePeriodSeconds {
 		allErrs = append(allErrs, errs.NewFieldInvalid("deletionGracePeriodSeconds", new.DeletionGracePeriodSeconds, "field is immutable; may only be changed via deletion"))
 	}

pkg/registry/generic/etcd/etcd_test.go

@@ -332,6 +332,11 @@ func TestEtcdCreate(t *testing.T) {
 	}
 }
 
+func podCopy(in *api.Pod) *api.Pod {
+	out := *in
+	return &out
+}
+
 func TestEtcdUpdate(t *testing.T) {
 	podA := &api.Pod{
 		ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test"},
@@ -407,18 +412,18 @@ func TestEtcdUpdate(t *testing.T) {
 		"normal": {
 			existing: nodeWithPodA,
 			expect:   nodeWithPodB,
-			toUpdate: podB,
+			toUpdate: podCopy(podB),
 			errOK:    func(err error) bool { return err == nil },
 		},
 		"notExisting": {
 			existing: emptyNode,
 			expect:   emptyNode,
-			toUpdate: podA,
+			toUpdate: podCopy(podA),
 			errOK:    func(err error) bool { return errors.IsNotFound(err) },
 		},
 		"createIfNotFound": {
 			existing:    emptyNode,
-			toUpdate:    podA,
+			toUpdate:    podCopy(podA),
 			allowCreate: true,
 			objOK:       hasCreated(t, podA),
 			errOK:       func(err error) bool { return err == nil },
@@ -426,13 +431,13 @@ func TestEtcdUpdate(t *testing.T) {
 		"outOfDate": {
 			existing: newerNodeWithPodA,
 			expect:   newerNodeWithPodA,
-			toUpdate: podB,
+			toUpdate: podCopy(podB),
 			errOK:    func(err error) bool { return errors.IsConflict(err) },
 		},
 		"unconditionalUpdate": {
 			existing:                 nodeWithPodAWithResourceVersion,
 			allowUnconditionalUpdate: true,
-			toUpdate:                 podA,
+			toUpdate:                 podCopy(podA),
 			objOK:                    func(obj runtime.Object) bool { return true },
 			errOK:                    func(err error) bool { return err == nil },
 		},