Add more validation for updating node.

pkg/api/validation/validation.go

@@ -535,3 +535,15 @@ func ValidateMinion(minion *api.Minion) errs.ValidationErrorList {
 	allErrs = append(allErrs, validateLabels(minion.Labels)...)
 	return allErrs
 }
+
+// ValidateMinionUpdate tests to make sure a minion update can be applied.  Modifies oldMinion.
+func ValidateMinionUpdate(oldMinion *api.Minion, minion *api.Minion) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+	// TODO: why we need two labels for minion.
+	oldMinion.Labels = minion.Labels
+	oldMinion.ObjectMeta.Labels = minion.ObjectMeta.Labels
+	if !reflect.DeepEqual(oldMinion, minion) {
+		allErrs = append(allErrs, fmt.Errorf("Update contains more than labels changes"))
+	}
+	return allErrs
+}

pkg/api/validation/validation_test.go

@@ -1065,3 +1065,62 @@ func TestValidateMinion(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateMinionUpdate(t *testing.T) {
+	tests := []struct {
+		oldMinion api.Minion
+		minion    api.Minion
+		valid     bool
+	}{
+		{api.Minion{}, api.Minion{}, true},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo"}},
+			api.Minion{
+				ObjectMeta: api.ObjectMeta{
+					Name: "bar"},
+			}, false},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "bar"},
+			},
+		}, api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "baz"},
+			},
+		}, true},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo",
+			},
+		}, api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "baz"},
+			},
+		}, true},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"bar": "foo"},
+			},
+		}, api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "baz"},
+			},
+		}, true},
+	}
+	for _, test := range tests {
+		errs := ValidateMinionUpdate(&test.oldMinion, &test.minion)
+		if test.valid && len(errs) > 0 {
+			t.Errorf("Unexpected error: %v", errs)
+			t.Logf("%#v vs %#v", test.oldMinion.ObjectMeta, test.minion.ObjectMeta)
+		}
+		if !test.valid && len(errs) == 0 {
+			t.Errorf("Unexpected non-error")
+		}
+	}
+}

pkg/registry/minion/rest.go

@@ -105,16 +105,22 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	if !ok {
 		return nil, fmt.Errorf("not a minion: %#v", obj)
 	}
-	if errs := validation.ValidateMinion(minion); len(errs) > 0 {
+
+	// TODO: GetMinion will health check the minion, but we shouldn't require the minion to be
+	// running for updating labels.
+	oldMinion, err := rs.registry.GetMinion(ctx, minion.Name)
+	if err != nil {
+		return nil, err
+	}
+	if errs := validation.ValidateMinionUpdate(oldMinion, minion); len(errs) > 0 {
 		return nil, kerrors.NewInvalid("minion", minion.Name, errs)
 	}
+
 	return apiserver.MakeAsync(func() (runtime.Object, error) {
 		err := rs.registry.UpdateMinion(ctx, minion)
 		if err != nil {
 			return nil, err
 		}
-		// TODO: GetMinion will health check the minion, but we shouldn't require the minion to be
-		// running for updating labels.
 		return rs.registry.GetMinion(ctx, minion.Name)
 	}), nil
 }

pkg/registry/minion/rest_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package minion
 
 import (
-	"reflect"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -88,7 +87,7 @@ func TestMinionREST(t *testing.T) {
 	}
 }
 
-func TestMinionRESTWithHealthCheck(t *testing.T) {
+func TestMinionStorageWithHealthCheck(t *testing.T) {
 	minionRegistry := registrytest.NewMinionRegistry([]string{}, api.NodeResources{})
 	minionHealthRegistry := HealthyRegistry{
 		delegate: minionRegistry,
@@ -120,46 +119,40 @@ func contains(nodes *api.MinionList, nodeID string) bool {
 	return false
 }
 
-func TestMinionRegistryUpdate(t *testing.T) {
+func TestMinionStorageInvalidUpdate(t *testing.T) {
-	minionRegistry := registrytest.NewMinionRegistry([]string{}, api.NodeResources{})
+	storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))
-	ms := NewREST(minionRegistry)
-
 	ctx := api.NewContext()
-
+	obj, err := storage.Get(ctx, "foo")
-	expected_labels := map[string]string{"foo": "bar"}
-	c, err := ms.Create(ctx, &api.Minion{
-		ObjectMeta: api.ObjectMeta{Name: "m1"},
-		Labels:     expected_labels,
-	})
 	if err != nil {
-		t.Errorf("insert failed")
+		t.Errorf("Unexpected error: %v", err)
 	}
-	result := <-c
+	minion, ok := obj.(*api.Minion)
-
+	if !ok {
-	created, ok := result.Object.(*api.Minion)
+		t.Fatalf("Object is not a minion: %#v", obj)
-	if !ok || created.Name != "m1" {
-		t.Errorf("insert return value was weird: %#v", result)
 	}
-	if !reflect.DeepEqual(expected_labels, created.Labels) {
+	minion.HostIP = "1.2.3.4"
-		t.Errorf("unexpected labels: %#v", created.Labels)
+	if _, err = storage.Update(ctx, minion); err == nil {
+		t.Error("Unexpected non-error.")
 	}
+}
 
-	update := new(api.Minion)
+func TestMinionStorageValidUpdate(t *testing.T) {
-	*update = *created
+	storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))
-	update_labels := map[string]string{"bar": "foo"}
+	ctx := api.NewContext()
-	update.Labels = update_labels
+	obj, err := storage.Get(ctx, "foo")
-
-	c, err = ms.Update(ctx, update)
 	if err != nil {
-		t.Errorf("update failed")
+		t.Errorf("Unexpected error: %v", err)
 	}
-	result = <-c
+	minion, ok := obj.(*api.Minion)
-	updated, ok := result.Object.(*api.Minion)
+	if !ok {
-	if !ok || updated.Name != "m1" {
+		t.Fatalf("Object is not a minion: %#v", obj)
-		t.Errorf("update return value was weird: %#v", result)
 	}
-	if !reflect.DeepEqual(update_labels, updated.Labels) {
+	minion.Labels = map[string]string{
-		t.Errorf("unexpected labels: %#v", updated.Labels)
+		"foo": "bar",
+		"baz": "home",
+	}
+	if _, err = storage.Update(ctx, minion); err != nil {
+		t.Error("Unexpected error: %v", err)
 	}
 }
 

test/integration/auth_test.go

@@ -306,7 +306,7 @@ func getTestRequests() []struct {
 		// Normal methods on minions
 		{"GET", "/api/v1beta1/minions", "", code200},
 		{"POST", "/api/v1beta1/minions" + syncFlags, aMinion, code200},
-		{"PUT", "/api/v1beta1/minions/a" + syncFlags, aMinion, code409}, // See #2115 about why 409
+		{"PUT", "/api/v1beta1/minions/a" + syncFlags, aMinion, code422}, // TODO: GET and put back server-provided fields to avoid a 422
 		{"GET", "/api/v1beta1/minions", "", code200},
 		{"GET", "/api/v1beta1/minions/a", "", code200},
 		{"DELETE", "/api/v1beta1/minions/a" + syncFlags, "", code200},