diff --git a/pkg/api/BUILD b/pkg/api/BUILD index a67b5f09397..50a0ca048b8 100644 --- a/pkg/api/BUILD +++ b/pkg/api/BUILD @@ -42,7 +42,6 @@ go_library( "//vendor:k8s.io/apimachinery/pkg/selection", "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apimachinery/pkg/util/sets", - "//vendor:k8s.io/apimachinery/pkg/util/validation/field", ], ) diff --git a/pkg/api/conversion.go b/pkg/api/conversion.go index f21eed92bef..420813579e9 100644 --- a/pkg/api/conversion.go +++ b/pkg/api/conversion.go @@ -26,7 +26,6 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/pkg/api/resource" "k8s.io/kubernetes/pkg/util/intstr" utillabels "k8s.io/kubernetes/pkg/util/labels" @@ -247,9 +246,6 @@ func Convert_map_to_unversioned_LabelSelector(in *map[string]string, out *metav1 func Convert_unversioned_LabelSelector_to_map(in *metav1.LabelSelector, out *map[string]string, s conversion.Scope) error { var err error *out, err = metav1.LabelSelectorAsMap(in) - if err != nil { - err = field.Invalid(field.NewPath("labelSelector"), *in, fmt.Sprintf("cannot convert to old selector: %v", err)) - } return err } diff --git a/pkg/api/helpers.go b/pkg/api/helpers.go index e402134f373..2a39e3473d4 100644 --- a/pkg/api/helpers.go +++ b/pkg/api/helpers.go @@ -50,6 +50,23 @@ func (c *ConversionError) Error() string { ) } +const ( + // annotation key prefix used to identify non-convertible json paths. + NonConvertibleAnnotationPrefix = "non-convertible.kubernetes.io" +) + +// NonConvertibleFields iterates over the provided map and filters out all but +// any keys with the "non-convertible.kubernetes.io" prefix. +func NonConvertibleFields(annotations map[string]string) map[string]string { + nonConvertibleKeys := map[string]string{} + for key, value := range annotations { + if strings.HasPrefix(key, NonConvertibleAnnotationPrefix) { + nonConvertibleKeys[key] = value + } + } + return nonConvertibleKeys +} + // Semantic can do semantic deep equality checks for api objects. // Example: api.Semantic.DeepEqual(aPod, aPodWithNonNilButEmptyMaps) == true var Semantic = conversion.EqualitiesOrDie( diff --git a/pkg/api/v1/conversion.go b/pkg/api/v1/conversion.go index 8c3d550fa18..02a99e83d1f 100644 --- a/pkg/api/v1/conversion.go +++ b/pkg/api/v1/conversion.go @@ -35,9 +35,6 @@ const ( // Value used to identify mirror pods from pre-v1.1 kubelet. mirrorAnnotationValue_1_0 = "mirror" - - // annotation key prefix used to identify non-convertible json paths. - NonConvertibleAnnotationPrefix = "kubernetes.io/non-convertible" ) // This is a "fast-path" that avoids reflection for common types. It focuses on the objects that are @@ -311,7 +308,7 @@ func Convert_extensions_ReplicaSet_to_v1_ReplicationController(in *extensions.Re if out.Annotations == nil { out.Annotations = make(map[string]string) } - out.Annotations[NonConvertibleAnnotationPrefix+"/"+fieldErr.Field] = reflect.ValueOf(fieldErr.BadValue).String() + out.Annotations[api.NonConvertibleAnnotationPrefix+"/"+fieldErr.Field] = reflect.ValueOf(fieldErr.BadValue).String() } if err := Convert_extensions_ReplicaSetStatus_to_v1_ReplicationControllerStatus(&in.Status, &out.Status, s); err != nil { return err diff --git a/pkg/registry/core/controller/strategy.go b/pkg/registry/core/controller/strategy.go index 8be918d634c..2f7b77511a5 100644 --- a/pkg/registry/core/controller/strategy.go +++ b/pkg/registry/core/controller/strategy.go @@ -22,6 +22,7 @@ import ( "fmt" "reflect" "strconv" + "strings" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" @@ -102,9 +103,31 @@ func (rcStrategy) AllowCreateOnUpdate() bool { // ValidateUpdate is the default update validation for an end user. func (rcStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList { - validationErrorList := validation.ValidateReplicationController(obj.(*api.ReplicationController)) - updateErrorList := validation.ValidateReplicationControllerUpdate(obj.(*api.ReplicationController), old.(*api.ReplicationController)) - return append(validationErrorList, updateErrorList...) + oldRc := old.(*api.ReplicationController) + newRc := obj.(*api.ReplicationController) + + validationErrorList := validation.ValidateReplicationController(newRc) + updateErrorList := validation.ValidateReplicationControllerUpdate(newRc, oldRc) + errs := append(validationErrorList, updateErrorList...) + + for key, value := range api.NonConvertibleFields(oldRc.Annotations) { + parts := strings.Split(key, "/") + if len(parts) != 2 { + continue + } + brokenField := parts[1] + + switch { + case strings.Contains(brokenField, "selector"): + if !reflect.DeepEqual(oldRc.Spec.Selector, newRc.Spec.Selector) { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("selector"), newRc.Spec.Selector, "cannot update non-convertible selector")) + } + default: + errs = append(errs, &field.Error{Type: field.ErrorTypeNotFound, BadValue: value, Field: brokenField, Detail: "unknown non-convertible field"}) + } + } + + return errs } func (rcStrategy) AllowUnconditionalUpdate() bool { diff --git a/pkg/registry/core/controller/strategy_test.go b/pkg/registry/core/controller/strategy_test.go index d60dcee1dda..fc2c526ca16 100644 --- a/pkg/registry/core/controller/strategy_test.go +++ b/pkg/registry/core/controller/strategy_test.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -150,3 +151,67 @@ func TestSelectableFieldLabelConversions(t *testing.T) { nil, ) } + +func TestValidateUpdate(t *testing.T) { + ctx := genericapirequest.NewDefaultContext() + validSelector := map[string]string{"a": "b"} + validPodTemplate := api.PodTemplate{ + Template: api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: validSelector, + }, + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyAlways, + DNSPolicy: api.DNSClusterFirst, + Containers: []api.Container{{Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent"}}, + }, + }, + } + oldController := &api.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: api.NamespaceDefault, ResourceVersion: "10", Annotations: make(map[string]string)}, + Spec: api.ReplicationControllerSpec{ + Replicas: 3, + Selector: validSelector, + Template: &validPodTemplate.Template, + }, + Status: api.ReplicationControllerStatus{ + Replicas: 1, + ObservedGeneration: int64(10), + }, + } + // Conversion sets this annotation + oldController.Annotations[api.NonConvertibleAnnotationPrefix+"/"+"spec.selector"] = "no way" + + // Deep-copy so we won't mutate both selectors. + objCopy, err := api.Scheme.DeepCopy(oldController) + if err != nil { + t.Fatalf("unexpected deep-copy error: %v", err) + } + newController, ok := objCopy.(*api.ReplicationController) + if !ok { + t.Fatalf("unexpected object: %#v", objCopy) + } + // Irrelevant (to the selector) update for the replication controller. + newController.Spec.Replicas = 5 + + // If they didn't try to update the selector then we should not return any error. + errs := Strategy.ValidateUpdate(ctx, newController, oldController) + if len(errs) > 0 { + t.Fatalf("unexpected errors: %v", errs) + } + + // Update the selector - validation should return an error. + newController.Spec.Selector["shiny"] = "newlabel" + newController.Spec.Template.Labels["shiny"] = "newlabel" + + errs = Strategy.ValidateUpdate(ctx, newController, oldController) + for _, err := range errs { + t.Logf("%#v\n", err) + } + if len(errs) != 1 { + t.Fatalf("expected a validation error") + } + if !strings.Contains(errs[0].Error(), "selector") { + t.Fatalf("expected error related to the selector") + } +}