diff --git a/examples/examples_test.go b/examples/examples_test.go index f659517007f..ccbab837e08 100644 --- a/examples/examples_test.go +++ b/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])...) diff --git a/pkg/api/errors/validation.go b/pkg/api/errors/validation.go index e025777a0b5..a3b4eddf8d9 100644 --- a/pkg/api/errors/validation.go +++ b/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, ""} } diff --git a/pkg/api/errors/validation_test.go b/pkg/api/errors/validation_test.go index cc643d0236a..23fa2c5021d 100644 --- a/pkg/api/errors/validation_test.go +++ b/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) { diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 585fbf8481d..d0429c8bd93 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -29,9 +29,90 @@ import ( "github.com/golang/glog" ) -// ServiceLister is an abstract interface for testing. -type ServiceLister interface { - ListServices(api.Context) (*api.ServiceList, error) +// ValidateNameFunc validates that the provided name is valid for a given resource type. +// Not all resources have the same validation rules for names. +type ValidateNameFunc func(name string) (bool, string) + +// nameIsDNSSubdomain is a ValidateNameFunc for names that must be a DNS subdomain. +func nameIsDNSSubdomain(name string) (bool, string) { + if util.IsDNSSubdomain(name) { + return true, "" + } + return false, "name must be lowercase letters and numbers, with inline dashes or periods" +} + +// nameIsDNS952Label is a ValidateNameFunc for names that must be a DNS 952 label. +func nameIsDNS952Label(name string) (bool, string) { + if util.IsDNS952Label(name) { + return true, "" + } + return false, "name must be lowercase letters, numbers, and dashes" +} + +// ValidateObjectMeta validates an object's metadata. +func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn ValidateNameFunc) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + if len(meta.Name) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("name", meta.Name)) + } else { + if ok, qualifier := nameFn(meta.Name); !ok { + allErrs = append(allErrs, errs.NewFieldInvalid("name", meta.Name, qualifier)) + } + } + if requiresNamespace { + if len(meta.Namespace) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("namespace", meta.Namespace)) + } else if !util.IsDNSSubdomain(meta.Namespace) { + allErrs = append(allErrs, errs.NewFieldInvalid("namespace", meta.Namespace, "")) + } + } else { + if len(meta.Namespace) != 0 { + allErrs = append(allErrs, errs.NewFieldInvalid("namespace", meta.Namespace, "namespace is not allowed on this type")) + } + } + allErrs = append(allErrs, ValidateLabels(meta.Labels, "labels")...) + allErrs = append(allErrs, ValidateLabels(meta.Annotations, "annotations")...) + + // Clear self link internally + // TODO: move to its own area + meta.SelfLink = "" + + return allErrs +} + +// ValidateObjectMetaUpdate validates an object's metadata when updated +func ValidateObjectMetaUpdate(old, meta *api.ObjectMeta) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + + // in the event it is left empty, set it, to allow clients more flexibility + if len(meta.UID) == 0 { + meta.UID = old.UID + } + if meta.CreationTimestamp.IsZero() { + meta.CreationTimestamp = old.CreationTimestamp + } + + if old.Name != meta.Name { + allErrs = append(allErrs, errs.NewFieldInvalid("name", meta.Name, "field is immutable")) + } + if old.Namespace != meta.Namespace { + allErrs = append(allErrs, errs.NewFieldInvalid("namespace", meta.Namespace, "field is immutable")) + } + if old.UID != meta.UID { + allErrs = append(allErrs, errs.NewFieldInvalid("uid", meta.UID, "field is immutable")) + } + if old.CreationTimestamp != meta.CreationTimestamp { + allErrs = append(allErrs, errs.NewFieldInvalid("creationTimestamp", meta.CreationTimestamp, "field is immutable")) + } + + allErrs = append(allErrs, ValidateLabels(meta.Labels, "labels")...) + allErrs = append(allErrs, ValidateLabels(meta.Annotations, "annotations")...) + + // Clear self link internally + // TODO: move to its own area + meta.SelfLink = "" + + return allErrs } func validateVolumes(volumes []api.Volume) (util.StringSet, errs.ValidationErrorList) { @@ -387,19 +468,9 @@ func validateDNSPolicy(dnsPolicy *api.DNSPolicy) errs.ValidationErrorList { // ValidatePod tests if required fields in the pod are set. func ValidatePod(pod *api.Pod) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - if len(pod.Name) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("name", pod.Name)) - } else if !util.IsDNSSubdomain(pod.Name) { - allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, "")) - } - if len(pod.Namespace) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("namespace", pod.Namespace)) - } else if !util.IsDNSSubdomain(pod.Namespace) { - allErrs = append(allErrs, errs.NewFieldInvalid("namespace", pod.Namespace, "")) - } + allErrs = append(allErrs, ValidateObjectMeta(&pod.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...) allErrs = append(allErrs, ValidatePodSpec(&pod.Spec).Prefix("spec")...) - allErrs = append(allErrs, ValidateLabels(pod.Labels, "labels")...) - allErrs = append(allErrs, ValidateLabels(pod.Annotations, "annotations")...) + return allErrs } @@ -434,9 +505,7 @@ func ValidateLabels(labels map[string]string, field string) errs.ValidationError func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - if newPod.Name != oldPod.Name { - allErrs = append(allErrs, errs.NewFieldInvalid("name", newPod.Name, "field is immutable")) - } + allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...) if len(newPod.Spec.Containers) != len(oldPod.Spec.Containers) { allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "may not add or remove containers")) @@ -454,24 +523,17 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList { // TODO: a better error would include all immutable fields explicitly. allErrs = append(allErrs, errs.NewFieldInvalid("spec.containers", newPod.Spec.Containers, "some fields are immutable")) } + return allErrs } var supportedSessionAffinityType = util.NewStringSet(string(api.AffinityTypeClientIP), string(api.AffinityTypeNone)) // ValidateService tests if required fields in the service are set. -func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context) errs.ValidationErrorList { +func ValidateService(service *api.Service) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - if len(service.Name) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("name", service.Name)) - } else if !util.IsDNS952Label(service.Name) { - allErrs = append(allErrs, errs.NewFieldInvalid("name", service.Name, "")) - } - if len(service.Namespace) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("namespace", service.Namespace)) - } else if !util.IsDNSSubdomain(service.Namespace) { - allErrs = append(allErrs, errs.NewFieldInvalid("namespace", service.Namespace, "")) - } + allErrs = append(allErrs, ValidateObjectMeta(&service.ObjectMeta, true, nameIsDNS952Label).Prefix("metadata")...) + if !util.IsValidPortNum(service.Spec.Port) { allErrs = append(allErrs, errs.NewFieldInvalid("spec.port", service.Spec.Port, "")) } @@ -484,8 +546,6 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context if service.Spec.Selector != nil { allErrs = append(allErrs, ValidateLabels(service.Spec.Selector, "spec.selector")...) } - allErrs = append(allErrs, ValidateLabels(service.Labels, "labels")...) - allErrs = append(allErrs, ValidateLabels(service.Annotations, "annotations")...) if service.Spec.SessionAffinity == "" { service.Spec.SessionAffinity = api.AffinityTypeNone @@ -496,22 +556,35 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context return allErrs } +// ValidateServiceUpdate tests if required fields in the service are set during an update +func ValidateServiceUpdate(oldService, service *api.Service) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldService.ObjectMeta, &service.ObjectMeta).Prefix("metadata")...) + + // TODO: PortalIP should be a Status field, since the system can set a value != to the user's value + // PortalIP can only be set, not unset. + if oldService.Spec.PortalIP != "" && service.Spec.PortalIP != oldService.Spec.PortalIP { + allErrs = append(allErrs, errs.NewFieldInvalid("spec.portalIP", service.Spec.PortalIP, "field is immutable")) + } + + return allErrs +} + // ValidateReplicationController tests if required fields in the replication controller are set. func ValidateReplicationController(controller *api.ReplicationController) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - if len(controller.Name) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("name", controller.Name)) - } else if !util.IsDNSSubdomain(controller.Name) { - allErrs = append(allErrs, errs.NewFieldInvalid("name", controller.Name, "")) - } - if len(controller.Namespace) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("namespace", controller.Namespace)) - } else if !util.IsDNSSubdomain(controller.Namespace) { - allErrs = append(allErrs, errs.NewFieldInvalid("namespace", controller.Namespace, "")) - } + allErrs = append(allErrs, ValidateObjectMeta(&controller.ObjectMeta, true, nameIsDNSSubdomain).Prefix("metadata")...) allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...) - allErrs = append(allErrs, ValidateLabels(controller.Labels, "labels")...) - allErrs = append(allErrs, ValidateLabels(controller.Annotations, "annotations")...) + + return allErrs +} + +// ValidateReplicationControllerUpdate tests if required fields in the replication controller are set. +func ValidateReplicationControllerUpdate(oldController, controller *api.ReplicationController) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldController.ObjectMeta, &controller.ObjectMeta).Prefix("metadata")...) + allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...) + return allErrs } @@ -569,12 +642,15 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ValidationErrorL } // ValidateBoundPod tests if required fields on a bound pod are set. +// TODO: to be removed. func ValidateBoundPod(pod *api.BoundPod) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} if len(pod.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", pod.Name)) - } else if !util.IsDNSSubdomain(pod.Name) { - allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, "")) + } else { + if ok, qualifier := nameIsDNSSubdomain(pod.Name); !ok { + allErrs = append(allErrs, errs.NewFieldInvalid("name", pod.Name, qualifier)) + } } if len(pod.Namespace) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("namespace", pod.Namespace)) @@ -585,32 +661,26 @@ func ValidateBoundPod(pod *api.BoundPod) errs.ValidationErrorList { return allErrs } -// ValidateMinion tests if required fields in the minion are set. -func ValidateMinion(minion *api.Node) errs.ValidationErrorList { +// ValidateMinion tests if required fields in the node are set. +func ValidateMinion(node *api.Node) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - if len(minion.Name) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("name", minion.Name)) - } else if !util.IsDNSSubdomain(minion.Name) { - allErrs = append(allErrs, errs.NewFieldInvalid("name", minion.Name, "")) - } - if len(minion.Namespace) != 0 { - allErrs = append(allErrs, errs.NewFieldInvalid("namespace", minion.Namespace, "")) - } - allErrs = append(allErrs, ValidateLabels(minion.Labels, "labels")...) - allErrs = append(allErrs, ValidateLabels(minion.Annotations, "annotations")...) + allErrs = append(allErrs, ValidateObjectMeta(&node.ObjectMeta, false, nameIsDNSSubdomain).Prefix("metadata")...) return allErrs } // ValidateMinionUpdate tests to make sure a minion update can be applied. Modifies oldMinion. func ValidateMinionUpdate(oldMinion *api.Node, minion *api.Node) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} + allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldMinion.ObjectMeta, &minion.ObjectMeta).Prefix("metadata")...) if !api.Semantic.DeepEqual(minion.Status, api.NodeStatus{}) { allErrs = append(allErrs, errs.NewFieldInvalid("status", minion.Status, "status must be empty")) } - // Allow users to update labels and capacity - oldMinion.Labels = minion.Labels + // TODO: move reset function to its own location + // Ignore metadata changes now that they have been tested + oldMinion.ObjectMeta = minion.ObjectMeta + // Allow users to update capacity oldMinion.Spec.Capacity = minion.Spec.Capacity // Clear status oldMinion.Status = minion.Status @@ -619,6 +689,8 @@ func ValidateMinionUpdate(oldMinion *api.Node, minion *api.Node) errs.Validation glog.V(4).Infof("Update failed validation %#v vs %#v", oldMinion, minion) allErrs = append(allErrs, fmt.Errorf("update contains more than labels or capacity changes")) } + + // TODO: validate Spec.Capacity return allErrs } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 52fa51997e1..449ae5f234c 100644 --- a/pkg/api/validation/validation_test.go +++ b/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) } } } diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go index 8793ff595bd..195fa632598 100644 --- a/pkg/registry/minion/rest_test.go +++ b/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" { diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index b62e8c03edc..718b34b798e 100644 --- a/pkg/registry/service/rest.go +++ b/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 { diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index ab288b0ae91..eec35f117be 100644 --- a/pkg/registry/service/rest_test.go +++ b/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) } } diff --git a/test/integration/auth_test.go b/test/integration/auth_test.go index 550e20d0ef9..c635bebb7f2 100644 --- a/test/integration/auth_test.go +++ b/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" } }