Merge pull request kubernetes#23586 from mikedanese/automated-cherry-…

…pick-of-#23530-kubernetes#23467-upstream-release-1.2 Automated cherry pick of kubernetes#23530 kubernetes#23467
shyamjvs · Mar 30, 2016 · a6cf1e8 · a6cf1e8
2 parents fe35c8c + fc4b884
commit a6cf1e8
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 1 deletion.
diff --git a/pkg/apis/extensions/validation/validation.go b/pkg/apis/extensions/validation/validation.go
@@ -201,6 +201,9 @@ func ValidateDaemonSetSpec(spec *extensions.DaemonSetSpec, fldPath *field.Path)
 	if err == nil && !selector.Matches(labels.Set(spec.Template.Labels)) {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("template", "metadata", "labels"), spec.Template.Labels, "`selector` does not match template `labels`"))
 	}
+	if spec.Selector != nil && len(spec.Selector.MatchLabels)+len(spec.Selector.MatchExpressions) == 0 {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), spec.Selector, "empty selector is not valid for daemonset."))
+	}
 
 	allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(&spec.Template, fldPath.Child("template"))...)
 	// Daemons typically run on more than one node, so mark Read-Write persistent disks as invalid.

diff --git a/pkg/apis/extensions/validation/validation_test.go b/pkg/apis/extensions/validation/validation_test.go
@@ -693,9 +693,16 @@ func TestValidateDaemonSet(t *testing.T) {
 				Template: validPodTemplate.Template,
 			},
 		},
+		"nil selector": {
+			ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: api.NamespaceDefault},
+			Spec: extensions.DaemonSetSpec{
+				Template: validPodTemplate.Template,
+			},
+		},
 		"empty selector": {
 			ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: api.NamespaceDefault},
 			Spec: extensions.DaemonSetSpec{
+				Selector: &unversioned.LabelSelector{},
 				Template: validPodTemplate.Template,
 			},
 		},

diff --git a/pkg/controller/daemon/controller.go b/pkg/controller/daemon/controller.go
@@ -627,7 +627,7 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error {
 
 	everything := unversioned.LabelSelector{}
 	if reflect.DeepEqual(ds.Spec.Selector, &everything) {
-		dsc.eventRecorder.Eventf(ds, api.EventTypeWarning, "SelectingAll", "This controller is selecting all pods. Skipping sync.")
+		dsc.eventRecorder.Eventf(ds, api.EventTypeWarning, "SelectingAll", "This daemon set is selecting all pods. A non-empty selector is required.")
 		return nil
 	}
 

diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go
@@ -426,6 +426,11 @@ func (dc *DeploymentController) syncDeployment(key string) error {
 	}
 
 	d := obj.(*extensions.Deployment)
+	everything := unversioned.LabelSelector{}
+	if reflect.DeepEqual(d.Spec.Selector, &everything) {
+		dc.eventRecorder.Eventf(d, api.EventTypeWarning, "SelectingAll", "This deployment is selecting all pods. A non-empty selector is required.")
+		return nil
+	}
 
 	if d.Spec.Paused {
 		// TODO: Implement scaling for paused deployments.

diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go
@@ -782,3 +782,28 @@ func TestSyncDeploymentCreatesReplicaSet(t *testing.T) {
 
 	f.run(getKey(d, t))
 }
+
+// issue: https://github.com/kubernetes/kubernetes/issues/23218
+func TestDeploymentController_dontSyncDeploymentsWithEmptyPodSelector(t *testing.T) {
+	fake := &fake.Clientset{}
+	controller := NewDeploymentController(fake, controller.NoResyncPeriodFunc)
+
+	controller.eventRecorder = &record.FakeRecorder{}
+	controller.rsStoreSynced = alwaysReady
+	controller.podStoreSynced = alwaysReady
+
+	d := newDeployment(1, nil)
+	empty := unversioned.LabelSelector{}
+	d.Spec.Selector = &empty
+	controller.dStore.Store.Add(d)
+	// We expect the deployment controller to not take action here since it's configuration
+	// is invalid, even though no replicasets exist that match it's selector.
+	controller.syncDeployment(fmt.Sprintf("%s/%s", d.ObjectMeta.Namespace, d.ObjectMeta.Name))
+	if len(fake.Actions()) == 0 {
+		return
+	}
+	for _, action := range fake.Actions() {
+		t.Logf("unexpected action: %#v", action)
+	}
+	t.Errorf("expected deployment controller to not take action")
+}