From 7fcaf80f671f70c48c5d5b30b3ff4c8832359775 Mon Sep 17 00:00:00 2001 From: Satnam Singh Date: Fri, 22 Aug 2014 14:44:21 -0700 Subject: [PATCH 1/2] Make validation check for legal serive port numbers. --- pkg/api/validation.go | 4 ++++ pkg/api/validation_test.go | 17 ++++++++++++++++- pkg/registry/service/storage_test.go | 11 ++++++++++- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/pkg/api/validation.go b/pkg/api/validation.go index b1847c99b78..c8ec703714c 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -260,6 +260,10 @@ func ValidateService(service *Service) errs.ErrorList { } else if !util.IsDNS952Label(service.ID) { allErrs = append(allErrs, errs.NewInvalid("Service.ID", service.ID)) } + // Make sure the service port number is valid. + if !util.IsValidPortNum(service.Port) { + allErrs = append(allErrs, errs.NewInvalid("Service.Port", service.Port)) + } if labels.Set(service.Selector).AsSelector().Empty() { allErrs = append(allErrs, errs.NewInvalid("Service.Selector", service.Selector)) } diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index 2d13d246862..1e52c2869ca 100644 --- a/pkg/api/validation_test.go +++ b/pkg/api/validation_test.go @@ -333,17 +333,31 @@ func TestValidatePod(t *testing.T) { } func TestValidateService(t *testing.T) { + // This test should fail because the port number is invalid i.e. + // the Port field has a default value of 0. errs := ValidateService(&Service{ JSONBase: JSONBase{ID: "foo"}, Selector: map[string]string{ "foo": "bar", }, }) + if len(errs) != 1 { + t.Errorf("Unexpected error list: %#v", errs) + } + + errs = ValidateService(&Service{ + Port: 6502, + 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{ + Port: 6502, Selector: map[string]string{ "foo": "bar", }, @@ -353,6 +367,7 @@ func TestValidateService(t *testing.T) { } errs = ValidateService(&Service{ + Port: 6502, JSONBase: JSONBase{ID: "foo"}, }) if len(errs) != 1 { @@ -360,7 +375,7 @@ func TestValidateService(t *testing.T) { } errs = ValidateService(&Service{}) - if len(errs) != 2 { + if len(errs) != 3 { t.Errorf("Unexpected error list: %#v", errs) } } diff --git a/pkg/registry/service/storage_test.go b/pkg/registry/service/storage_test.go index e86f499efac..5685acb57c2 100644 --- a/pkg/registry/service/storage_test.go +++ b/pkg/registry/service/storage_test.go @@ -21,7 +21,7 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake" + fake_cloud "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/minion" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" @@ -34,6 +34,7 @@ func TestServiceRegistryCreate(t *testing.T) { machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) svc := &api.Service{ + Port: 6502, JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{"bar": "baz"}, } @@ -60,6 +61,7 @@ func TestServiceStorageValidatesCreate(t *testing.T) { storage := NewRegistryStorage(registry, nil, nil) failureCases := map[string]api.Service{ "empty ID": { + Port: 6502, JSONBase: api.JSONBase{ID: ""}, Selector: map[string]string{"bar": "baz"}, }, @@ -82,11 +84,13 @@ func TestServiceStorageValidatesCreate(t *testing.T) { func TestServiceRegistryUpdate(t *testing.T) { registry := registrytest.NewServiceRegistry() registry.CreateService(api.Service{ + Port: 6502, JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{"bar": "baz1"}, }) storage := NewRegistryStorage(registry, nil, nil) c, err := storage.Update(&api.Service{ + Port: 6502, JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{"bar": "baz2"}, }) @@ -109,16 +113,19 @@ func TestServiceRegistryUpdate(t *testing.T) { func TestServiceStorageValidatesUpdate(t *testing.T) { registry := registrytest.NewServiceRegistry() registry.CreateService(api.Service{ + Port: 6502, JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{"bar": "baz"}, }) storage := NewRegistryStorage(registry, nil, nil) failureCases := map[string]api.Service{ "empty ID": { + Port: 6502, JSONBase: api.JSONBase{ID: ""}, Selector: map[string]string{"bar": "baz"}, }, "empty selector": { + Port: 6502, JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{}, }, @@ -140,6 +147,7 @@ func TestServiceRegistryExternalService(t *testing.T) { machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) svc := &api.Service{ + Port: 6502, JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{"bar": "baz"}, CreateExternalLoadBalancer: true, @@ -166,6 +174,7 @@ func TestServiceRegistryExternalServiceError(t *testing.T) { machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) svc := &api.Service{ + Port: 6502, JSONBase: api.JSONBase{ID: "foo"}, Selector: map[string]string{"bar": "baz"}, CreateExternalLoadBalancer: true, From 4105f7ef617fdd785478c80e27823d59517d308d Mon Sep 17 00:00:00 2001 From: Satnam Singh Date: Mon, 25 Aug 2014 10:38:11 -0700 Subject: [PATCH 2/2] Code review changes to adjust pacakge name for storage_test.go --- pkg/api/validation.go | 1 - pkg/registry/service/storage_test.go | 17 +++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/api/validation.go b/pkg/api/validation.go index c8ec703714c..70b098ea8cc 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -260,7 +260,6 @@ func ValidateService(service *Service) errs.ErrorList { } else if !util.IsDNS952Label(service.ID) { allErrs = append(allErrs, errs.NewInvalid("Service.ID", service.ID)) } - // Make sure the service port number is valid. if !util.IsValidPortNum(service.Port) { allErrs = append(allErrs, errs.NewInvalid("Service.Port", service.Port)) } diff --git a/pkg/registry/service/storage_test.go b/pkg/registry/service/storage_test.go index 5685acb57c2..601efd64800 100644 --- a/pkg/registry/service/storage_test.go +++ b/pkg/registry/service/storage_test.go @@ -21,7 +21,7 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - fake_cloud "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake" + cloud "github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/minion" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" @@ -30,7 +30,7 @@ import ( func TestServiceRegistryCreate(t *testing.T) { registry := registrytest.NewServiceRegistry() - fakeCloud := &fake_cloud.FakeCloud{} + fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) svc := &api.Service{ @@ -78,6 +78,7 @@ func TestServiceStorageValidatesCreate(t *testing.T) { if err == nil { t.Errorf("Expected to get an error") } + } } @@ -143,7 +144,7 @@ func TestServiceStorageValidatesUpdate(t *testing.T) { func TestServiceRegistryExternalService(t *testing.T) { registry := registrytest.NewServiceRegistry() - fakeCloud := &fake_cloud.FakeCloud{} + fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) svc := &api.Service{ @@ -168,7 +169,7 @@ func TestServiceRegistryExternalService(t *testing.T) { func TestServiceRegistryExternalServiceError(t *testing.T) { registry := registrytest.NewServiceRegistry() - fakeCloud := &fake_cloud.FakeCloud{ + fakeCloud := &cloud.FakeCloud{ Err: fmt.Errorf("test error"), } machines := []string{"foo", "bar", "baz"} @@ -191,7 +192,7 @@ func TestServiceRegistryExternalServiceError(t *testing.T) { func TestServiceRegistryDelete(t *testing.T) { registry := registrytest.NewServiceRegistry() - fakeCloud := &fake_cloud.FakeCloud{} + fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) svc := api.Service{ @@ -211,7 +212,7 @@ func TestServiceRegistryDelete(t *testing.T) { func TestServiceRegistryDeleteExternal(t *testing.T) { registry := registrytest.NewServiceRegistry() - fakeCloud := &fake_cloud.FakeCloud{} + fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) svc := api.Service{ @@ -254,7 +255,7 @@ func TestServiceRegistryMakeLinkVariables(t *testing.T) { func TestServiceRegistryGet(t *testing.T) { registry := registrytest.NewServiceRegistry() - fakeCloud := &fake_cloud.FakeCloud{} + fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) registry.CreateService(api.Service{ @@ -272,7 +273,7 @@ func TestServiceRegistryGet(t *testing.T) { func TestServiceRegistryList(t *testing.T) { registry := registrytest.NewServiceRegistry() - fakeCloud := &fake_cloud.FakeCloud{} + fakeCloud := &cloud.FakeCloud{} machines := []string{"foo", "bar", "baz"} storage := NewRegistryStorage(registry, fakeCloud, minion.NewRegistry(machines)) registry.CreateService(api.Service{