Unify validation logic for create and update paths

examples/examples_test.go

@@ -27,7 +27,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 	"github.com/golang/glog"
 )
@@ -49,7 +48,7 @@ func validateObject(obj runtime.Object) (errors []error) {
 			t.Namespace = api.NamespaceDefault
 		}
 		api.ValidNamespace(ctx, &t.ObjectMeta)
-		errors = validation.ValidateService(t, registrytest.NewServiceRegistry(), api.NewDefaultContext())
+		errors = validation.ValidateService(t)
 	case *api.ServiceList:
 		for i := range t.Items {
 			errors = append(errors, validateObject(&t.Items[i])...)

pkg/api/errors/validation.go

@@ -85,9 +85,10 @@ var _ error = &ValidationError{}
 
 func (v *ValidationError) Error() string {
 	var s string
-	if v.Type == ValidationErrorTypeRequired && isEmpty(v.BadValue) {
+	switch v.Type {
+	case ValidationErrorTypeRequired:
 		s = spew.Sprintf("%s: %s", v.Field, v.Type)
-	} else {
+	default:
 		s = spew.Sprintf("%s: %s '%+v'", v.Field, v.Type, v.BadValue)
 	}
 	if len(v.Detail) != 0 {
@@ -96,18 +97,8 @@ func (v *ValidationError) Error() string {
 	return s
 }
 
-func isEmpty(obj interface{}) bool {
-	if obj == nil {
-		return true
-	}
-	switch t := obj.(type) {
-	case string:
-		return len(t) == 0
-	}
-	return false
-}
-
 // NewFieldRequired returns a *ValidationError indicating "value required"
+// TODO: remove "value"
 func NewFieldRequired(field string, value interface{}) *ValidationError {
 	return &ValidationError{ValidationErrorTypeRequired, field, value, ""}
 }

pkg/api/errors/validation_test.go

@@ -71,7 +71,7 @@ func TestValidationErrorUsefulMessage(t *testing.T) {
 		Inner interface{}
 		KV    map[string]int
 	}
-	s = NewFieldRequired(
+	s = NewFieldInvalid(
 		"foo",
 		&complicated{
 			Baz:   1,
@@ -79,11 +79,12 @@ func TestValidationErrorUsefulMessage(t *testing.T) {
 			Inner: &complicated{Qux: "asdf"},
 			KV:    map[string]int{"Billy": 2},
 		},
+		"detail",
 	).Error()
 	t.Logf("message: %v", s)
 	for _, part := range []string{
-		"foo", ValidationErrorTypeRequired.String(),
-		"Baz", "Qux", "Inner", "KV",
+		"foo", ValidationErrorTypeInvalid.String(),
+		"Baz", "Qux", "Inner", "KV", "detail",
 		"1", "aoeu", "asdf", "Billy", "2",
 	} {
 		if !strings.Contains(s, part) {

pkg/api/validation/validation_test.go

@@ -1081,7 +1081,7 @@ func TestValidateService(t *testing.T) {
 	for _, tc := range testCases {
 		registry := registrytest.NewServiceRegistry()
 		registry.List = tc.existing
-		errs := ValidateService(&tc.svc, registry, api.NewDefaultContext())
+		errs := ValidateService(&tc.svc)
 		if len(errs) != tc.numErrs {
 			t.Errorf("Unexpected error list for case %q: %v", tc.name, utilerrors.NewAggregate(errs))
 		}
@@ -1094,7 +1094,7 @@ func TestValidateService(t *testing.T) {
 			Selector: map[string]string{"foo": "bar"},
 		},
 	}
-	errs := ValidateService(&svc, registrytest.NewServiceRegistry(), api.NewDefaultContext())
+	errs := ValidateService(&svc)
 	if len(errs) != 0 {
 		t.Errorf("Unexpected non-zero error list: %#v", errs)
 	}
@@ -1287,15 +1287,15 @@ func TestValidateReplicationController(t *testing.T) {
 		for i := range errs {
 			field := errs[i].(*errors.ValidationError).Field
 			if !strings.HasPrefix(field, "spec.template.") &&
-				field != "name" &&
-				field != "namespace" &&
+				field != "metadata.name" &&
+				field != "metadata.namespace" &&
 				field != "spec.selector" &&
 				field != "spec.template" &&
 				field != "GCEPersistentDisk.ReadOnly" &&
 				field != "spec.replicas" &&
 				field != "spec.template.labels" &&
-				field != "labels" &&
-				field != "annotations" {
+				field != "metadata.annotations" &&
+				field != "metadata.labels" {
 				t.Errorf("%s: missing prefix for: %v", k, errs[i])
 			}
 		}
@@ -1376,9 +1376,10 @@ func TestValidateMinion(t *testing.T) {
 		}
 		for i := range errs {
 			field := errs[i].(*errors.ValidationError).Field
-			if field != "name" &&
-				field != "labels" &&
-				field != "annotations" {
+			if field != "metadata.name" &&
+				field != "metadata.labels" &&
+				field != "metadata.annotations" &&
+				field != "metadata.namespace" {
 				t.Errorf("%s: missing prefix for: %v", k, errs[i])
 			}
 		}
@@ -1504,14 +1505,170 @@ func TestValidateMinionUpdate(t *testing.T) {
 			},
 		}, true},
 	}
-	for _, test := range tests {
+	for i, test := range tests {
 		errs := ValidateMinionUpdate(&test.oldMinion, &test.minion)
 		if test.valid && len(errs) > 0 {
-			t.Errorf("Unexpected error: %v", errs)
+			t.Errorf("%d: Unexpected error: %v", i, errs)
 			t.Logf("%#v vs %#v", test.oldMinion.ObjectMeta, test.minion.ObjectMeta)
 		}
 		if !test.valid && len(errs) == 0 {
-			t.Errorf("Unexpected non-error")
+			t.Errorf("%d: Unexpected non-error", i)
+		}
+	}
+}
+
+func TestValidateServiceUpdate(t *testing.T) {
+	tests := []struct {
+		oldService api.Service
+		service    api.Service
+		valid      bool
+	}{
+		{ // 0
+			api.Service{},
+			api.Service{},
+			true},
+		{ // 1
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo"}},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "bar"},
+			}, false},
+		{ // 2
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "bar"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 3
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 4
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 5
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:        "foo",
+					Annotations: map[string]string{"bar": "foo"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:        "foo",
+					Annotations: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 6
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"foo": "baz"},
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"foo": "baz"},
+				},
+			}, true},
+		{ // 7
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.1",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "fooobaz"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "new",
+				},
+			}, false},
+		{ // 8
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.1",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "fooobaz"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "",
+				},
+			}, false},
+		{ // 9
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "foo"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.1",
+				},
+			},
+			api.Service{
+				ObjectMeta: api.ObjectMeta{
+					Name:   "foo",
+					Labels: map[string]string{"bar": "fooobaz"},
+				},
+				Spec: api.ServiceSpec{
+					PortalIP: "127.0.0.2",
+				},
+			}, false},
+	}
+	for i, test := range tests {
+		errs := ValidateServiceUpdate(&test.oldService, &test.service)
+		if test.valid && len(errs) > 0 {
+			t.Errorf("%d: Unexpected error: %v", i, errs)
+			t.Logf("%#v vs %#v", test.oldService.ObjectMeta, test.service.ObjectMeta)
+		}
+		if !test.valid && len(errs) == 0 {
+			t.Errorf("%d: Unexpected non-error", i)
 		}
 	}
 }

pkg/registry/minion/rest_test.go

@@ -42,7 +42,7 @@ func TestMinionRegistryREST(t *testing.T) {
 
 	c, err := ms.Create(ctx, &api.Node{ObjectMeta: api.ObjectMeta{Name: "baz"}})
 	if err != nil {
-		t.Errorf("insert failed")
+		t.Fatalf("insert failed: %v", err)
 	}
 	obj := <-c
 	if !api.HasObjectMetaSystemFieldValues(&obj.Object.(*api.Node).ObjectMeta) {
@@ -57,7 +57,7 @@ func TestMinionRegistryREST(t *testing.T) {
 
 	c, err = ms.Delete(ctx, "bar")
 	if err != nil {
-		t.Errorf("delete failed")
+		t.Fatalf("delete failed")
 	}
 	obj = <-c
 	if s, ok := obj.Object.(*api.Status); !ok || s.Status != api.StatusSuccess {
@@ -69,7 +69,7 @@ func TestMinionRegistryREST(t *testing.T) {
 
 	_, err = ms.Delete(ctx, "bar")
 	if err != ErrDoesNotExist {
-		t.Errorf("delete returned wrong error")
+		t.Fatalf("delete returned wrong error")
 	}
 
 	list, err := ms.List(ctx, labels.Everything(), labels.Everything())
@@ -103,7 +103,7 @@ func TestMinionRegistryHealthCheck(t *testing.T) {
 
 	c, err := ms.Create(ctx, &api.Node{ObjectMeta: api.ObjectMeta{Name: "m1"}})
 	if err != nil {
-		t.Errorf("insert failed")
+		t.Fatalf("insert failed: %v", err)
 	}
 	result := <-c
 	if m, ok := result.Object.(*api.Node); !ok || m.Name != "m1" {

pkg/registry/service/rest.go

@@ -84,7 +84,7 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	if !api.ValidNamespace(ctx, &service.ObjectMeta) {
 		return nil, errors.NewConflict("service", service.Namespace, fmt.Errorf("Service.Namespace does not match the provided context"))
 	}
-	if errs := validation.ValidateService(service, rs.registry, ctx); len(errs) > 0 {
+	if errs := validation.ValidateService(service); len(errs) > 0 {
 		return nil, errors.NewInvalid("service", service.Name, errs)
 	}
 
@@ -226,20 +226,21 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	if !api.ValidNamespace(ctx, &service.ObjectMeta) {
 		return nil, errors.NewConflict("service", service.Namespace, fmt.Errorf("Service.Namespace does not match the provided context"))
 	}
-	if errs := validation.ValidateService(service, rs.registry, ctx); len(errs) > 0 {
+
+	oldService, err := rs.registry.GetService(ctx, service.Name)
+	if err != nil {
+		return nil, err
+	}
+
+	// Copy over non-user fields
+	// TODO: this should be a Status field, since the end user does not set it.
+	// TODO: make this a merge function
+	service.Spec.ProxyPort = oldService.Spec.ProxyPort
+
+	if errs := validation.ValidateServiceUpdate(oldService, service); len(errs) > 0 {
 		return nil, errors.NewInvalid("service", service.Name, errs)
 	}
 	return apiserver.MakeAsync(func() (runtime.Object, error) {
-		cur, err := rs.registry.GetService(ctx, service.Name)
-		if err != nil {
-			return nil, err
-		}
-		if service.Spec.PortalIP != "" && service.Spec.PortalIP != cur.Spec.PortalIP {
-			el := errors.ValidationErrorList{errors.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, "field is immutable")}
-			return nil, errors.NewInvalid("service", service.Name, el)
-		}
-		// Copy over non-user fields.
-		service.Spec.ProxyPort = cur.Spec.ProxyPort
 		// TODO: check to see if external load balancer status changed
 		err = rs.registry.UpdateService(ctx, service)
 		if err != nil {

pkg/registry/service/rest_test.go

@@ -118,7 +118,7 @@ func TestServiceRegistryUpdate(t *testing.T) {
 	ctx := api.NewDefaultContext()
 	registry := registrytest.NewServiceRegistry()
 	registry.CreateService(ctx, &api.Service{
-		ObjectMeta: api.ObjectMeta{Name: "foo"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault},
 		Spec: api.ServiceSpec{
 			Port:     6502,
 			Selector: map[string]string{"bar": "baz1"},
@@ -132,12 +132,12 @@ func TestServiceRegistryUpdate(t *testing.T) {
 			Selector: map[string]string{"bar": "baz2"},
 		},
 	})
+	if err != nil {
+		t.Fatalf("Expected no error: %v", err)
+	}
 	if c == nil {
 		t.Errorf("Expected non-nil channel")
 	}
-	if err != nil {
-		t.Errorf("Expected no error")
-	}
 	updated_svc := <-c
 	updated_service := updated_svc.Object.(*api.Service)
 	if updated_service.Name != "foo" {
@@ -531,11 +531,9 @@ func TestServiceRegistryIPUpdate(t *testing.T) {
 	update.Spec.Port = 6503
 	update.Spec.PortalIP = "1.2.3.76" // error
 
-	c, _ = rest.Update(ctx, update)
-	result := <-c
-	st := result.Object.(*api.Status)
-	if st.Reason != api.StatusReasonInvalid {
-		t.Errorf("Expected to get an invalid error, got %v", st)
+	_, err := rest.Update(ctx, update)
+	if err == nil || !errors.IsInvalid(err) {
+		t.Error("Unexpected error type: %v", err)
 	}
 }
 

test/integration/auth_test.go

@@ -106,6 +106,7 @@ var aService string = `
   "apiVersion": "v1beta1",
   "id": "a",
   "port": 8000,
+  "portalIP": "10.0.0.1",
   "labels": { "name": "a" },
   "selector": { "name": "a" }
 }