Merge pull request #1006 from satnam6502/master

Make validation check for legal service port numbers.

pkg/api/validation.go

@@ -260,6 +260,9 @@ func ValidateService(service *Service) errs.ErrorList {
 	} else if !util.IsDNS952Label(service.ID) {
 		allErrs = append(allErrs, errs.NewInvalid("Service.ID", service.ID))
 	}
+	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))
 	}

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)
 	}
 }

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"
+	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,10 +30,11 @@ 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{
+		Port:     6502,
 		JSONBase: api.JSONBase{ID: "foo"},
 		Selector: map[string]string{"bar": "baz"},
 	}
@@ -63,6 +64,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"},
 		},
@@ -79,17 +81,20 @@ func TestServiceStorageValidatesCreate(t *testing.T) {
 		if err == nil {
 			t.Errorf("Expected to get an error")
 		}
+
 	}
 }
 
 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"},
 	})
@@ -112,16 +117,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{},
 		},
@@ -139,10 +147,11 @@ 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{
+		Port:                       6502,
 		JSONBase:                   api.JSONBase{ID: "foo"},
 		Selector:                   map[string]string{"bar": "baz"},
 		CreateExternalLoadBalancer: true,
@@ -163,12 +172,13 @@ 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"}
 	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,
@@ -185,7 +195,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{
@@ -205,7 +215,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{
@@ -248,7 +258,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{
@@ -266,7 +276,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{