From e9edfc754e78fda231f13039737bf4d327697c98 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Thu, 10 Jul 2014 12:45:01 -0700 Subject: [PATCH 1/4] Add initial validation of Service objects --- pkg/api/validation.go | 11 +++++++++++ pkg/api/validation_test.go | 33 ++++++++++++++++++++++++++++++++ pkg/registry/service_registry.go | 5 +++-- 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/pkg/api/validation.go b/pkg/api/validation.go index d58cbdf0521..e3572d4a3c4 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -246,3 +246,14 @@ func ValidateManifest(manifest *ContainerManifest) []error { allErrs.Append(validateContainers(manifest.Containers, allVolumes)...) return []error(allErrs) } + +func ValidateService(service *Service) []error { + allErrs := errorList{} + if service.ID == "" { + allErrs.Append(fmt.Errorf("ID should not be empty: %#v", *service)) + } + if len(service.Selector) == 0 { + allErrs.Append(fmt.Errorf("Service %#v missing a selector", *service)) + } + return []error(allErrs) +} diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index 34bf26bc1ee..84ae5ff20c5 100644 --- a/pkg/api/validation_test.go +++ b/pkg/api/validation_test.go @@ -262,3 +262,36 @@ func TestValidateManifest(t *testing.T) { } } } + +func TestValidateService(t *testing.T) { + errs := ValidateService(&Service{ + JSONBase: JSONBase{ID: "foo"}, + Selector: map[string]string{ + "foo": "bar", + }, + }) + if len(errs) != 0 { + t.Errorf("Unexpected non-zero error list: %#v", errs) + } + + errs = ValidateService(&Service{ + Selector: map[string]string{ + "foo": "bar", + }, + }) + if len(errs) != 1 { + t.Errorf("Unexpected error list: %#v", errs) + } + + errs = ValidateService(&Service{ + JSONBase: JSONBase{ID: "foo"}, + }) + if len(errs) != 1 { + t.Errorf("Unexpected error list: %#v", errs) + } + + errs = ValidateService(&Service{}) + if len(errs) != 2 { + t.Errorf("Unexpected error list: %#v", errs) + } +} diff --git a/pkg/registry/service_registry.go b/pkg/registry/service_registry.go index 89455a85c50..6d4e0d8fd4d 100644 --- a/pkg/registry/service_registry.go +++ b/pkg/registry/service_registry.go @@ -112,8 +112,9 @@ func (sr *ServiceRegistryStorage) Extract(body []byte) (interface{}, error) { func (sr *ServiceRegistryStorage) Create(obj interface{}) (<-chan interface{}, error) { srv := obj.(api.Service) - if srv.ID == "" { - return nil, fmt.Errorf("ID should not be empty: %#v", srv) + errs := api.ValidateService(&srv) + if len(errs) > 0 { + return nil, fmt.Errorf("Validation errors: %v", errs) } return apiserver.MakeAsync(func() (interface{}, error) { // TODO: Consider moving this to a rectification loop, so that we make/remove external load balancers From b59dcbbb807d1a243f90e4bcab26ffe8f29e2ed0 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Thu, 10 Jul 2014 15:05:45 -0700 Subject: [PATCH 2/4] Fix tests. --- pkg/registry/service_registry_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/registry/service_registry_test.go b/pkg/registry/service_registry_test.go index 023101aae2f..d16f4e8ea23 100644 --- a/pkg/registry/service_registry_test.go +++ b/pkg/registry/service_registry_test.go @@ -33,6 +33,7 @@ func TestServiceRegistry(t *testing.T) { svc := api.Service{ JSONBase: api.JSONBase{ID: "foo"}, + Selector: map[string]string{"bar": "baz"}, } c, _ := storage.Create(svc) <-c @@ -56,6 +57,7 @@ func TestServiceRegistryExternalService(t *testing.T) { svc := api.Service{ JSONBase: api.JSONBase{ID: "foo"}, + Selector: map[string]string{"bar": "baz"}, CreateExternalLoadBalancer: true, } c, _ := storage.Create(svc) @@ -82,6 +84,7 @@ func TestServiceRegistryExternalServiceError(t *testing.T) { svc := api.Service{ JSONBase: api.JSONBase{ID: "foo"}, + Selector: map[string]string{"bar": "baz"}, CreateExternalLoadBalancer: true, } c, _ := storage.Create(svc) @@ -106,6 +109,7 @@ func TestServiceRegistryDelete(t *testing.T) { svc := api.Service{ JSONBase: api.JSONBase{ID: "foo"}, + Selector: map[string]string{"bar": "baz"}, } memory.CreateService(svc) @@ -131,6 +135,7 @@ func TestServiceRegistryDeleteExternal(t *testing.T) { svc := api.Service{ JSONBase: api.JSONBase{ID: "foo"}, + Selector: map[string]string{"bar": "baz"}, CreateExternalLoadBalancer: true, } memory.CreateService(svc) From 5bf08205803b93b8d45b50638f3d2911eb9e05c0 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Thu, 10 Jul 2014 21:14:46 -0700 Subject: [PATCH 3/4] Fix integration tests. --- cmd/integration/integration.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go index 50f55df4513..0295b829c65 100644 --- a/cmd/integration/integration.go +++ b/cmd/integration/integration.go @@ -145,13 +145,19 @@ func runAtomicPutTest(c *client.Client) { Labels: map[string]string{ "name": "atomicService", }, + // This is here because validation requires it. + Selector: map[string]string{ + "foo": "bar", + }, }, ).Do().Into(&svc) if err != nil { glog.Fatalf("Failed creating atomicService: %v", err) } glog.Info("Created atomicService") - testLabels := labels.Set{} + testLabels := labels.Set{ + "foo": "bar", + } for i := 0; i < 5; i++ { // a: z, b: y, etc... testLabels[string([]byte{byte('a' + i)})] = string([]byte{byte('z' - i)}) From c9babe619e46b76a08d9d98fe0cdb84268043d6c Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Fri, 11 Jul 2014 14:24:31 -0700 Subject: [PATCH 4/4] Updated to use the makeInvalidError helper function. --- pkg/api/validation.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/api/validation.go b/pkg/api/validation.go index e3572d4a3c4..a73373efa97 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -250,10 +250,10 @@ func ValidateManifest(manifest *ContainerManifest) []error { func ValidateService(service *Service) []error { allErrs := errorList{} if service.ID == "" { - allErrs.Append(fmt.Errorf("ID should not be empty: %#v", *service)) + allErrs.Append(makeInvalidError("Service.ID", service.ID)) } if len(service.Selector) == 0 { - allErrs.Append(fmt.Errorf("Service %#v missing a selector", *service)) + allErrs.Append(makeInvalidError("Service.Selector", service.Selector)) } return []error(allErrs) }