ignore selector changes for deployment, replicaset and daemonset prio…

…r update
kubernetes · Aug 30, 2017 · 9929e03 · 9929e03
1 parent 4457e43
commit 9929e03
Show file tree

Hide file tree

Showing 13 changed files with 526 additions and 8 deletions.
diff --git a/pkg/registry/extensions/daemonset/BUILD b/pkg/registry/extensions/daemonset/BUILD
@@ -16,8 +16,12 @@ go_library(
         "//pkg/api:go_default_library",
         "//pkg/apis/extensions:go_default_library",
         "//pkg/apis/extensions/validation:go_default_library",
+        "//vendor/k8s.io/api/apps/v1beta2:go_default_library",
+        "//vendor/k8s.io/api/extensions/v1beta1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/validation:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/registry/rest:go_default_library",
@@ -31,6 +35,10 @@ go_test(
     library = ":go_default_library",
     deps = [
         "//pkg/api:go_default_library",
+        "//pkg/apis/extensions:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/registry/rest:go_default_library",
     ],
 )

diff --git a/pkg/registry/extensions/daemonset/strategy.go b/pkg/registry/extensions/daemonset/strategy.go
@@ -17,8 +17,14 @@ limitations under the License.
 package daemonset
 
 import (
+	"fmt"
+
+	appsv1beta2 "k8s.io/api/apps/v1beta2"
+	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
+	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
@@ -109,9 +115,29 @@ func (daemonSetStrategy) AllowCreateOnUpdate() bool {
 
 // ValidateUpdate is the default update validation for an end user.
 func (daemonSetStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
-	validationErrorList := validation.ValidateDaemonSet(obj.(*extensions.DaemonSet))
-	updateErrorList := validation.ValidateDaemonSetUpdate(obj.(*extensions.DaemonSet), old.(*extensions.DaemonSet))
-	return append(validationErrorList, updateErrorList...)
+	newDaemonSet := obj.(*extensions.DaemonSet)
+	oldDaemonSet := old.(*extensions.DaemonSet)
+	allErrs := validation.ValidateDaemonSet(obj.(*extensions.DaemonSet))
+	allErrs = append(allErrs, validation.ValidateDaemonSetUpdate(newDaemonSet, oldDaemonSet)...)
+
+	// Update is not allowed to set Spec.Selector for all groups/versions except extensions/v1beta1.
+	// If RequestInfo is nil, it is better to revert to old behavior (i.e. allow update to set Spec.Selector)
+	// to prevent unintentionally breaking users who may rely on the old behavior.
+	// TODO(#50791): after extensions/v1beta1 is removed, move selector immutability check inside ValidateDaemonSetUpdate().
+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+		switch groupVersion {
+		case extensionsv1beta1.SchemeGroupVersion:
+			// no-op for compatibility
+		case appsv1beta2.SchemeGroupVersion:
+			// disallow mutation of selector
+			allErrs = append(allErrs, apivalidation.ValidateImmutableField(newDaemonSet.Spec.Selector, oldDaemonSet.Spec.Selector, field.NewPath("spec").Child("selector"))...)
+		default:
+			panic(fmt.Sprintf("unexpected group/version: %v", groupVersion))
+		}
+	}
+
+	return allErrs
 }
 
 // AllowUnconditionalUpdate is the default update policy for daemon set objects.

diff --git a/pkg/registry/extensions/daemonset/strategy_test.go b/pkg/registry/extensions/daemonset/strategy_test.go
@@ -17,10 +17,22 @@ limitations under the License.
 package daemonset
 
 import (
+	"reflect"
 	"testing"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
-	_ "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/apis/extensions"
+)
+
+const (
+	fakeImageName = "fake-name"
+	fakeImage     = "fakeimage"
+	daemonsetName = "test-daemonset"
+	namespace     = "test-namespace"
 )
 
 func TestDefaultGarbageCollectionPolicy(t *testing.T) {
@@ -31,3 +43,84 @@ func TestDefaultGarbageCollectionPolicy(t *testing.T) {
 		t.Errorf("DefaultGarbageCollectionPolicy() = %#v, want %#v", got, want)
 	}
 }
+
+func TestSelectorImmutability(t *testing.T) {
+	tests := []struct {
+		requestInfo       genericapirequest.RequestInfo
+		oldSelectorLabels map[string]string
+		newSelectorLabels map[string]string
+		expectedErrorList field.ErrorList
+	}{
+		{
+			genericapirequest.RequestInfo{
+				APIGroup:   "apps",
+				APIVersion: "v1beta2",
+				Resource:   "daemonsets",
+			},
+			map[string]string{"a": "b"},
+			map[string]string{"c": "d"},
+			field.ErrorList{
+				&field.Error{
+					Type:  field.ErrorTypeInvalid,
+					Field: field.NewPath("spec").Child("selector").String(),
+					BadValue: &metav1.LabelSelector{
+						MatchLabels:      map[string]string{"c": "d"},
+						MatchExpressions: []metav1.LabelSelectorRequirement{},
+					},
+					Detail: "field is immutable",
+				},
+			},
+		},
+		{
+			genericapirequest.RequestInfo{
+				APIGroup:   "extensions",
+				APIVersion: "v1beta1",
+				Resource:   "daemonsets",
+			},
+			map[string]string{"a": "b"},
+			map[string]string{"c": "d"},
+			field.ErrorList{},
+		},
+	}
+
+	for _, test := range tests {
+		oldDaemonSet := newDaemonSetWithSelectorLabels(test.oldSelectorLabels, 1)
+		newDaemonSet := newDaemonSetWithSelectorLabels(test.newSelectorLabels, 2)
+		context := genericapirequest.NewContext()
+		context = genericapirequest.WithRequestInfo(context, &test.requestInfo)
+		errorList := daemonSetStrategy{}.ValidateUpdate(context, newDaemonSet, oldDaemonSet)
+		if !reflect.DeepEqual(test.expectedErrorList, errorList) {
+			t.Errorf("Unexpected error list, expected: %v, actual: %v", test.expectedErrorList, errorList)
+		}
+	}
+}
+
+func newDaemonSetWithSelectorLabels(selectorLabels map[string]string, templateGeneration int64) *extensions.DaemonSet {
+	return &extensions.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            daemonsetName,
+			Namespace:       namespace,
+			ResourceVersion: "1",
+		},
+		Spec: extensions.DaemonSetSpec{
+			Selector: &metav1.LabelSelector{
+				MatchLabels:      selectorLabels,
+				MatchExpressions: []metav1.LabelSelectorRequirement{},
+			},
+			UpdateStrategy: extensions.DaemonSetUpdateStrategy{
+				Type: extensions.OnDeleteDaemonSetStrategyType,
+			},
+			TemplateGeneration: templateGeneration,
+			Template: api.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: selectorLabels,
+				},
+				Spec: api.PodSpec{
+					RestartPolicy: api.RestartPolicyAlways,
+					DNSPolicy:     api.DNSClusterFirst,
+					Containers:    []api.Container{{Name: fakeImageName, Image: fakeImage, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: api.TerminationMessageReadFile}},
+				},
+			},
+		},
+	}
+}
diff --git a/pkg/registry/extensions/deployment/BUILD b/pkg/registry/extensions/deployment/BUILD
@@ -17,10 +17,15 @@ go_library(
         "//pkg/api:go_default_library",
         "//pkg/apis/extensions:go_default_library",
         "//pkg/apis/extensions/validation:go_default_library",
+        "//vendor/k8s.io/api/apps/v1beta1:go_default_library",
+        "//vendor/k8s.io/api/apps/v1beta2:go_default_library",
+        "//vendor/k8s.io/api/extensions/v1beta1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/validation:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/internalversion:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/registry/rest:go_default_library",
@@ -37,6 +42,8 @@ go_test(
         "//pkg/apis/extensions:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
     ],
 )

diff --git a/pkg/registry/extensions/deployment/strategy.go b/pkg/registry/extensions/deployment/strategy.go
@@ -17,8 +17,15 @@ limitations under the License.
 package deployment
 
 import (
+	"fmt"
+
+	appsv1beta1 "k8s.io/api/apps/v1beta1"
+	appsv1beta2 "k8s.io/api/apps/v1beta2"
+	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
+	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
@@ -88,7 +95,31 @@ func (deploymentStrategy) PrepareForUpdate(ctx genericapirequest.Context, obj, o
 
 // ValidateUpdate is the default update validation for an end user.
 func (deploymentStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
-	return validation.ValidateDeploymentUpdate(obj.(*extensions.Deployment), old.(*extensions.Deployment))
+	newDeployment := obj.(*extensions.Deployment)
+	oldDeployment := old.(*extensions.Deployment)
+	allErrs := validation.ValidateDeploymentUpdate(newDeployment, oldDeployment)
+
+	// Update is not allowed to set Spec.Selector for all groups/versions except extensions/v1beta1.
+	// If RequestInfo is nil, it is better to revert to old behavior (i.e. allow update to set Spec.Selector)
+	// to prevent unintentionally breaking users who may rely on the old behavior.
+	// TODO(#50791): after apps/v1beta1 and extensions/v1beta1 are removed,
+	// move selector immutability check inside ValidateDeploymentUpdate().
+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+		switch groupVersion {
+		case appsv1beta1.SchemeGroupVersion:
+			// no-op for compatibility
+		case extensionsv1beta1.SchemeGroupVersion:
+			// no-op for compatibility
+		case appsv1beta2.SchemeGroupVersion:
+			// disallow mutation of selector
+			allErrs = append(allErrs, apivalidation.ValidateImmutableField(newDeployment.Spec.Selector, oldDeployment.Spec.Selector, field.NewPath("spec").Child("selector"))...)
+		default:
+			panic(fmt.Sprintf("unexpected group/version: %v", groupVersion))
+		}
+	}
+
+	return allErrs
 }
 
 func (deploymentStrategy) AllowUnconditionalUpdate() bool {

diff --git a/pkg/registry/extensions/deployment/strategy_test.go b/pkg/registry/extensions/deployment/strategy_test.go
@@ -22,11 +22,20 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/apis/extensions"
 )
 
+const (
+	fakeImageName  = "fake-name"
+	fakeImage      = "fakeimage"
+	deploymentName = "test-deployment"
+	namespace      = "test-namespace"
+)
+
 func TestStatusUpdates(t *testing.T) {
 	tests := []struct {
 		old      runtime.Object
@@ -78,3 +87,99 @@ func newDeployment(labels, annotations map[string]string) *extensions.Deployment
 		},
 	}
 }
+
+func TestSelectorImmutability(t *testing.T) {
+	tests := []struct {
+		requestInfo       genericapirequest.RequestInfo
+		oldSelectorLabels map[string]string
+		newSelectorLabels map[string]string
+		expectedErrorList field.ErrorList
+	}{
+		{
+			genericapirequest.RequestInfo{
+				APIGroup:   "apps",
+				APIVersion: "v1beta2",
+				Resource:   "deployments",
+			},
+			map[string]string{"a": "b"},
+			map[string]string{"c": "d"},
+			field.ErrorList{
+				&field.Error{
+					Type:  field.ErrorTypeInvalid,
+					Field: field.NewPath("spec").Child("selector").String(),
+					BadValue: &metav1.LabelSelector{
+						MatchLabels:      map[string]string{"c": "d"},
+						MatchExpressions: []metav1.LabelSelectorRequirement{},
+					},
+					Detail: "field is immutable",
+				},
+			},
+		},
+		{
+			genericapirequest.RequestInfo{
+				APIGroup:   "apps",
+				APIVersion: "v1beta1",
+				Resource:   "deployments",
+			},
+			map[string]string{"a": "b"},
+			map[string]string{"c": "d"},
+			field.ErrorList{},
+		},
+		{
+			genericapirequest.RequestInfo{
+				APIGroup:   "extensions",
+				APIVersion: "v1beta1",
+			},
+			map[string]string{"a": "b"},
+			map[string]string{"c": "d"},
+			field.ErrorList{},
+		},
+	}
+
+	for _, test := range tests {
+		oldDeployment := newDeploymentWithSelectorLabels(test.oldSelectorLabels)
+		newDeployment := newDeploymentWithSelectorLabels(test.newSelectorLabels)
+		context := genericapirequest.NewContext()
+		context = genericapirequest.WithRequestInfo(context, &test.requestInfo)
+		errorList := deploymentStrategy{}.ValidateUpdate(context, newDeployment, oldDeployment)
+		if len(test.expectedErrorList) == 0 && len(errorList) == 0 {
+			continue
+		}
+		if !reflect.DeepEqual(test.expectedErrorList, errorList) {
+			t.Errorf("Unexpected error list, expected: %v, actual: %v", test.expectedErrorList, errorList)
+		}
+	}
+}
+
+func newDeploymentWithSelectorLabels(selectorLabels map[string]string) *extensions.Deployment {
+	return &extensions.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            deploymentName,
+			Namespace:       namespace,
+			ResourceVersion: "1",
+		},
+		Spec: extensions.DeploymentSpec{
+			Selector: &metav1.LabelSelector{
+				MatchLabels:      selectorLabels,
+				MatchExpressions: []metav1.LabelSelectorRequirement{},
+			},
+			Strategy: extensions.DeploymentStrategy{
+				Type: extensions.RollingUpdateDeploymentStrategyType,
+				RollingUpdate: &extensions.RollingUpdateDeployment{
+					MaxSurge:       intstr.FromInt(1),
+					MaxUnavailable: intstr.FromInt(1),
+				},
+			},
+			Template: api.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: selectorLabels,
+				},
+				Spec: api.PodSpec{
+					RestartPolicy: api.RestartPolicyAlways,
+					DNSPolicy:     api.DNSDefault,
+					Containers:    []api.Container{{Name: fakeImageName, Image: fakeImage, ImagePullPolicy: api.PullNever, TerminationMessagePolicy: api.TerminationMessageReadFile}},
+				},
+			},
+		},
+	}
+}