Merge pull request #29183 from kargakis/fix-conversion-annotation

Automatic merge from submit-queue (batch tested with PRs 39199, 37273, 29183, 39638, 40199)

Invalidate updates to non-convertible selectors

Follow-up to https://github.com/kubernetes/kubernetes/pull/24733

@deads2k @lavalamp @smarterclayton @bgrant0607 @liggitt @mfojtik 

First commit contains the necessary validation for replication controllers with non-convertible selectors.
Second commit updates the name for the annotation added during conversion since it is invalid currently:

```
+++ [0719 11:19:54] Running tests without code coverage
--- FAIL: TestValidateUpdate (0.00s)
    strategy_test.go:191: unexpected error: [metadata.annotations: Invalid value: "kubernetes.io/non-convertible/spec.selector": must match the regex ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9] (e.g. 'MyName' or 'my.name' or '123-abc') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName', metadata.annotations: Invalid value: "kubernetes.io/non-convertible/spec.selector": must match the regex ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9] (e.g. 'MyName' or 'my.name' or '123-abc') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName']
FAIL
FAIL    k8s.io/kubernetes/pkg/registry/controller   0.015s
```

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",
     ],
 )
 

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
 }
 

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(

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

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))
+	oldRc := old.(*api.ReplicationController)
-	updateErrorList := validation.ValidateReplicationControllerUpdate(obj.(*api.ReplicationController), old.(*api.ReplicationController))
+	newRc := obj.(*api.ReplicationController)
-	return append(validationErrorList, updateErrorList...)
+
+	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 {

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")
+	}
+}