Repository: kubernetes/kubernetes (language: go)

Issue:
Move selector immutability check to validation after v1beta1 retires
The check for selector immutability is located at `PrepareForUpdate` functions of `deploymentStrategy`, `rsStrategy` and `daemonSetStrategy`. We are not able to have the check at validation before _v1beta1_ API version retires due to breaking change to some tests (discussed in this closed [PR](https://github.com/kubernetes/kubernetes/pull/50348)). Once _v1beta1_ retires, we should move the check to validation.




Discussion / hints:
Issues go stale after 90d of inactivity.
Mark the issue as fresh with `/remove-lifecycle stale`.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an `/lifecycle frozen` comment.

If this issue is safe to close now please do so with `/close`.

Send feedback to sig-testing, kubernetes/test-infra and/or `@fejta`.
/lifecycle stale
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with `/remove-lifecycle rotten`.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with `/close`.

Send feedback to sig-testing, kubernetes/test-infra and/or [fejta](https://github.com/fejta).
/lifecycle rotten
/remove-lifecycle stale
the beta types have not been served since 1.16, so this can be done now.

relevant spots to update are in https://grep.app/search?q=TODO%28%2350791%29&filter[repo][0]=kubernetes/kubernetes
/assign
/unassign
@crimsonfaith91 is this issue still up for grabs? If so then as a new contributor I would like to take this up.
I had a PR to resolve it but it never got reviewed. If you do grab it:
1. #107858 is a good starting point
2. Expect for it to go unreviewed for a long time
> I had a PR to resolve it but it never got reviewed. If you do grab it:
> 
> 1. [Move selector immutability checks into validation #107858](https://github.com/kubernetes/kubernetes/pull/107858) is a good starting point
> 2. Expect for it to go unreviewed for a long time

Thanks a lot @yardenshoham! I did go through your PR and understand the basics of what the issue entails. Also I still can't believe that your PR went unreviewed for an entire year!
/assign
/assign

is this issue still open ?
/assign

@k8s-ci-robot is this issue still open? I'd love to contribute if it's available !!
/assign

Is it still open?

Fix patch:
diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go
index cff536d9ecf0a..c924b7a6e5dc8 100644
--- a/pkg/apis/apps/validation/validation.go
+++ b/pkg/apis/apps/validation/validation.go
@@ -341,6 +341,9 @@ func ValidateDaemonSetSpecUpdate(newSpec, oldSpec *apps.DaemonSetSpec, fldPath *
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("templateGeneration"), newSpec.TemplateGeneration, "must not be incremented without template update"))
 	}
 
+	// Spec.Selector is immutable
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(newSpec.Selector, oldSpec.Selector, field.NewPath("spec").Child("selector"))...)
+
 	return allErrs
 }
 
@@ -648,6 +651,7 @@ func ValidateDeploymentStatus(status *apps.DeploymentStatus, fldPath *field.Path
 func ValidateDeploymentUpdate(update, old *apps.Deployment, opts apivalidation.PodValidationOptions) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))
 	allErrs = append(allErrs, ValidateDeploymentSpec(&update.Spec, &old.Spec, field.NewPath("spec"), opts)...)
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(update.Spec.Selector, old.Spec.Selector, field.NewPath("spec").Child("selector"))...)
 	return allErrs
 }
 
@@ -702,6 +706,7 @@ func ValidateReplicaSetUpdate(rs, oldRs *apps.ReplicaSet, opts apivalidation.Pod
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&rs.ObjectMeta, &oldRs.ObjectMeta, field.NewPath("metadata"))...)
 	allErrs = append(allErrs, ValidateReplicaSetSpec(&rs.Spec, &oldRs.Spec, field.NewPath("spec"), opts)...)
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(rs.Spec.Selector, oldRs.Spec.Selector, field.NewPath("spec").Child("selector"))...)
 	return allErrs
 }
 
diff --git a/pkg/registry/apps/daemonset/strategy.go b/pkg/registry/apps/daemonset/strategy.go
index 32665566fa0fb..87cbaacfc4305 100644
--- a/pkg/registry/apps/daemonset/strategy.go
+++ b/pkg/registry/apps/daemonset/strategy.go
@@ -19,14 +19,10 @@ package daemonset
 import (
 	"context"
 
-	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
-	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/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"
 	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
@@ -151,21 +147,6 @@ func (daemonSetStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Ob
 	allErrs := validation.ValidateDaemonSet(obj.(*apps.DaemonSet), opts)
 	allErrs = append(allErrs, validation.ValidateDaemonSetUpdate(newDaemonSet, oldDaemonSet, opts)...)
 
-	// Update is not allowed to set Spec.Selector for apps/v1 and apps/v1beta2 (allowed for 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
-		default:
-			// disallow mutation of selector
-			allErrs = append(allErrs, apivalidation.ValidateImmutableField(newDaemonSet.Spec.Selector, oldDaemonSet.Spec.Selector, field.NewPath("spec").Child("selector"))...)
-		}
-	}
-
 	return allErrs
 }
 
diff --git a/pkg/registry/apps/deployment/strategy.go b/pkg/registry/apps/deployment/strategy.go
index d8ca5ef2c3040..ad896c341c9be 100644
--- a/pkg/registry/apps/deployment/strategy.go
+++ b/pkg/registry/apps/deployment/strategy.go
@@ -20,15 +20,10 @@ import (
 	"context"
 	"fmt"
 
-	appsv1beta1 "k8s.io/api/apps/v1beta1"
-	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"
 	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
-	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
 	"k8s.io/apiserver/pkg/storage/names"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
@@ -136,22 +131,6 @@ func (deploymentStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.O
 	opts := pod.GetValidationOptionsFromPodTemplate(&newDeployment.Spec.Template, &oldDeployment.Spec.Template)
 	allErrs := appsvalidation.ValidateDeploymentUpdate(newDeployment, oldDeployment, opts)
 
-	// 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, extensionsv1beta1.SchemeGroupVersion:
-			// no-op for compatibility
-		default:
-			// disallow mutation of selector
-			allErrs = append(allErrs, apivalidation.ValidateImmutableField(newDeployment.Spec.Selector, oldDeployment.Spec.Selector, field.NewPath("spec").Child("selector"))...)
-		}
-	}
-
 	return allErrs
 }
 
diff --git a/pkg/registry/apps/replicaset/strategy.go b/pkg/registry/apps/replicaset/strategy.go
index e795cc7358799..96788e4ec84bc 100644
--- a/pkg/registry/apps/replicaset/strategy.go
+++ b/pkg/registry/apps/replicaset/strategy.go
@@ -23,16 +23,12 @@ import (
 	"fmt"
 	"strconv"
 
-	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
-	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
-	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/generic"
 	"k8s.io/apiserver/pkg/registry/rest"
 	apistorage "k8s.io/apiserver/pkg/storage"
@@ -149,21 +145,6 @@ func (rsStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) f
 	allErrs := appsvalidation.ValidateReplicaSet(obj.(*apps.ReplicaSet), opts)
 	allErrs = append(allErrs, appsvalidation.ValidateReplicaSetUpdate(newReplicaSet, oldReplicaSet, opts)...)
 
-	// 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 ValidateReplicaSetUpdate().
-	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
-		default:
-			// disallow mutation of selector
-			allErrs = append(allErrs, apivalidation.ValidateImmutableField(newReplicaSet.Spec.Selector, oldReplicaSet.Spec.Selector, field.NewPath("spec").Child("selector"))...)
-		}
-	}
-
 	return allErrs
 }
 


Test patch:
diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go
index 42d4035f01ff9..1c49b05394f7f 100644
--- a/pkg/apis/apps/validation/validation_test.go
+++ b/pkg/apis/apps/validation/validation_test.go
@@ -1548,14 +1548,6 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
 			Spec: validPodSpecNodeSelector,
 		},
 	}
-	validPodTemplateAbc2 := api.PodTemplate{
-		Template: api.PodTemplateSpec{
-			ObjectMeta: metav1.ObjectMeta{
-				Labels: validSelector2,
-			},
-			Spec: validPodSpecAbc,
-		},
-	}
 	validPodTemplateDef := api.PodTemplate{
 		Template: api.PodTemplateSpec{
 			ObjectMeta: metav1.ObjectMeta{
@@ -1611,30 +1603,6 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
 				},
 			},
 		},
-		"change template and selector": {
-			old: apps.DaemonSet{
-				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
-				Spec: apps.DaemonSetSpec{
-					Selector:           &metav1.LabelSelector{MatchLabels: validSelector},
-					TemplateGeneration: 2,
-					Template:           validPodTemplateAbc.Template,
-					UpdateStrategy: apps.DaemonSetUpdateStrategy{
-						Type: apps.OnDeleteDaemonSetStrategyType,
-					},
-				},
-			},
-			update: apps.DaemonSet{
-				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
-				Spec: apps.DaemonSetSpec{
-					Selector:           &metav1.LabelSelector{MatchLabels: validSelector2},
-					TemplateGeneration: 3,
-					Template:           validPodTemplateAbc2.Template,
-					UpdateStrategy: apps.DaemonSetUpdateStrategy{
-						Type: apps.OnDeleteDaemonSetStrategyType,
-					},
-				},
-			},
-		},
 		"change template": {
 			old: apps.DaemonSet{
 				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
@@ -1674,9 +1642,9 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
 			update: apps.DaemonSet{
 				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
 				Spec: apps.DaemonSetSpec{
-					Selector:           &metav1.LabelSelector{MatchLabels: validSelector2},
-					TemplateGeneration: 2,
-					Template:           validPodTemplateDef.Template,
+					Selector:           &metav1.LabelSelector{MatchLabels: validSelector},
+					TemplateGeneration: 1,
+					Template:           validPodTemplateAbc.Template,
 					UpdateStrategy: apps.DaemonSetUpdateStrategy{
 						Type: apps.OnDeleteDaemonSetStrategyType,
 					},
@@ -1805,6 +1773,31 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
 			},
 			expectedErrNum: 1,
 		},
+		"change selector": {
+			old: apps.DaemonSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
+				Spec: apps.DaemonSetSpec{
+					Selector:           &metav1.LabelSelector{MatchLabels: validSelector},
+					TemplateGeneration: 1,
+					Template:           validPodTemplateAbc.Template,
+					UpdateStrategy: apps.DaemonSetUpdateStrategy{
+						Type: apps.OnDeleteDaemonSetStrategyType,
+					},
+				},
+			},
+			update: apps.DaemonSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
+				Spec: apps.DaemonSetSpec{
+					Selector:           &metav1.LabelSelector{MatchLabels: validSelector2},
+					TemplateGeneration: 2,
+					Template:           validPodTemplateDef.Template,
+					UpdateStrategy: apps.DaemonSetUpdateStrategy{
+						Type: apps.OnDeleteDaemonSetStrategyType,
+					},
+				},
+			},
+			expectedErrNum: 1,
+		},
 		"invalid selector": {
 			old: apps.DaemonSet{
 				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
@@ -1828,7 +1821,7 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
 					},
 				},
 			},
-			expectedErrNum: 1,
+			expectedErrNum: 2,
 		},
 		"invalid pod": {
 			old: apps.DaemonSet{
@@ -1946,8 +1939,8 @@ func TestValidateDaemonSetUpdate(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault},
 				Spec: apps.DaemonSetSpec{
 					TemplateGeneration: 2,
-					Selector:           &metav1.LabelSelector{MatchLabels: validSelector2},
-					Template:           validPodTemplateAbc2.Template,
+					Selector:           &metav1.LabelSelector{MatchLabels: validSelector},
+					Template:           validPodTemplateNodeSelector.Template,
 					UpdateStrategy: apps.DaemonSetUpdateStrategy{
 						Type: apps.OnDeleteDaemonSetStrategyType,
 					},
@@ -2677,7 +2670,7 @@ func TestValidateDeploymentUpdate(t *testing.T) {
 						Strategy: apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType},
 					},
 				},
-				expectedErrNum: 3,
+				expectedErrNum: 4,
 			},
 			"invalid pod": {
 				old: apps.Deployment{
@@ -3093,7 +3086,7 @@ func TestValidateReplicaSetUpdate(t *testing.T) {
 					Template: validPodTemplate.Template,
 				},
 			},
-			expectedErrNum: 3,
+			expectedErrNum: 4,
 		},
 		"invalid pod": {
 			old: apps.ReplicaSet{
diff --git a/pkg/registry/apps/daemonset/strategy_test.go b/pkg/registry/apps/daemonset/strategy_test.go
index f1db558bf413d..3657760601826 100644
--- a/pkg/registry/apps/daemonset/strategy_test.go
+++ b/pkg/registry/apps/daemonset/strategy_test.go
@@ -82,16 +82,6 @@ func TestSelectorImmutability(t *testing.T) {
 				},
 			},
 		},
-		{
-			genericapirequest.RequestInfo{
-				APIGroup:   "extensions",
-				APIVersion: "v1beta1",
-				Resource:   "daemonsets",
-			},
-			map[string]string{"a": "b"},
-			map[string]string{"c": "d"},
-			nil,
-		},
 	}
 
 	for _, test := range tests {
diff --git a/pkg/registry/apps/deployment/strategy_test.go b/pkg/registry/apps/deployment/strategy_test.go
index d514783765a9a..98996e7726908 100644
--- a/pkg/registry/apps/deployment/strategy_test.go
+++ b/pkg/registry/apps/deployment/strategy_test.go
@@ -17,10 +17,11 @@ limitations under the License.
 package deployment
 
 import (
-	"k8s.io/utils/ptr"
 	"reflect"
 	"testing"
 
+	"k8s.io/utils/ptr"
+
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -166,17 +167,17 @@ func TestSelectorImmutability(t *testing.T) {
 		{
 			genericapirequest.RequestInfo{
 				APIGroup:   "apps",
-				APIVersion: "v1beta2",
+				APIVersion: "v1",
 				Resource:   "deployments",
 			},
 			map[string]string{"a": "b"},
-			map[string]string{"c": "d"},
+			map[string]string{"c": "v1"},
 			field.ErrorList{
 				&field.Error{
 					Type:  field.ErrorTypeInvalid,
 					Field: field.NewPath("spec").Child("selector").String(),
 					BadValue: &metav1.LabelSelector{
-						MatchLabels:      map[string]string{"c": "d"},
+						MatchLabels:      map[string]string{"c": "v1"},
 						MatchExpressions: []metav1.LabelSelectorRequirement{},
 					},
 					Detail: "field is immutable",
@@ -186,21 +187,22 @@ func TestSelectorImmutability(t *testing.T) {
 		{
 			genericapirequest.RequestInfo{
 				APIGroup:   "apps",
-				APIVersion: "v1beta1",
+				APIVersion: "v1beta2",
 				Resource:   "deployments",
 			},
 			map[string]string{"a": "b"},
 			map[string]string{"c": "d"},
-			field.ErrorList{},
-		},
-		{
-			genericapirequest.RequestInfo{
-				APIGroup:   "extensions",
-				APIVersion: "v1beta1",
+			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",
+				},
 			},
-			map[string]string{"a": "b"},
-			map[string]string{"c": "d"},
-			field.ErrorList{},
 		},
 	}
 
diff --git a/pkg/registry/apps/replicaset/strategy_test.go b/pkg/registry/apps/replicaset/strategy_test.go
index 5bf8d46b326fe..f4cfcf492f0a3 100644
--- a/pkg/registry/apps/replicaset/strategy_test.go
+++ b/pkg/registry/apps/replicaset/strategy_test.go
@@ -17,10 +17,11 @@ limitations under the License.
 package replicaset
 
 import (
-	"k8s.io/utils/ptr"
 	"reflect"
 	"testing"
 
+	"k8s.io/utils/ptr"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
@@ -245,13 +246,23 @@ func TestSelectorImmutability(t *testing.T) {
 		},
 		{
 			genericapirequest.RequestInfo{
-				APIGroup:   "extensions",
-				APIVersion: "v1beta1",
+				APIGroup:   "apps",
+				APIVersion: "v1",
 				Resource:   "replicasets",
 			},
 			map[string]string{"a": "b"},
-			map[string]string{"c": "d"},
-			nil,
+			map[string]string{"c": "v1"},
+			field.ErrorList{
+				&field.Error{
+					Type:  field.ErrorTypeInvalid,
+					Field: field.NewPath("spec").Child("selector").String(),
+					BadValue: &metav1.LabelSelector{
+						MatchLabels:      map[string]string{"c": "v1"},
+						MatchExpressions: []metav1.LabelSelectorRequirement{},
+					},
+					Detail: "field is immutable",
+				},
+			},
 		},
 	}