Add some validation for externalized service ports.

satnam6502 · Nov 3, 2014 · f556f2f · f556f2f
1 parent a4abafe
commit f556f2f
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 9 deletions.
diff --git a/examples/examples_test.go b/examples/examples_test.go
@@ -27,6 +27,7 @@ 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"
 )
@@ -42,7 +43,7 @@ func validateObject(obj runtime.Object) (errors []error) {
 		}
 	case *api.Service:
 		api.ValidNamespace(ctx, &t.ObjectMeta)
-		errors = validation.ValidateService(t)
+		errors = validation.ValidateService(t, registrytest.NewServiceRegistry(), api.NewDefaultContext())
 	case *api.ServiceList:
 		for i := range t.Items {
 			errors = append(errors, validateObject(&t.Items[i])...)

diff --git a/pkg/api/errors/errors.go b/pkg/api/errors/errors.go
@@ -132,6 +132,19 @@ func NewBadRequest(reason string) error {
 	}
 }
 
+// NewInternalError returns an error indicating the item is invalid and cannot be processed.
+func NewInternalError(err error) error {
+	return &statusError{api.Status{
+		Status: api.StatusFailure,
+		Code:   500,
+		Reason: api.StatusReasonInternalError,
+		Details: &api.StatusDetails{
+			Causes: []api.StatusCause{{Message: err.Error()}},
+		},
+		Message: fmt.Sprintf("Internal error occurred: %v", err),
+	}}
+}
+
 // IsNotFound returns true if the specified error was created by NewNotFoundErr.
 func IsNotFound(err error) bool {
 	return reasonForError(err) == api.StatusReasonNotFound

diff --git a/pkg/api/types.go b/pkg/api/types.go
@@ -686,6 +686,13 @@ const (
 	// StatusReasonInvalid above which indicates that the API call could possibly succeed, but the
 	// data was invalid.  API calls that return BadRequest can never succeed.
 	StatusReasonBadRequest StatusReason = "BadRequest"
+
+	// StatusReasonInternalError indicates that an internal error occurred, it is unexpected
+	// and the outcome of the call is unknown.
+	// Details (optional):
+	//   "causes" - The original error
+	// Status code 500
+	StatusReasonInternalError = "InternalError"
 )
 
 // StatusCause provides more information about an api.Status failure, including

diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go
@@ -17,6 +17,7 @@ limitations under the License.
 package validation
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 
@@ -27,6 +28,10 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
+type ServiceLister interface {
+	ListServices(api.Context) (*api.ServiceList, error)
+}
+
 func validateVolumes(volumes []api.Volume) (util.StringSet, errs.ValidationErrorList) {
 	allErrs := errs.ValidationErrorList{}
 
@@ -382,7 +387,7 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
 }
 
 // ValidateService tests if required fields in the service are set.
-func ValidateService(service *api.Service) errs.ValidationErrorList {
+func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 	if len(service.Name) == 0 {
 		allErrs = append(allErrs, errs.NewFieldRequired("name", service.Name))
@@ -403,6 +408,19 @@ func ValidateService(service *api.Service) errs.ValidationErrorList {
 	if labels.Set(service.Selector).AsSelector().Empty() {
 		allErrs = append(allErrs, errs.NewFieldRequired("selector", service.Selector))
 	}
+	if service.CreateExternalLoadBalancer {
+		services, err := lister.ListServices(ctx)
+		if err != nil {
+			allErrs = append(allErrs, errs.NewInternalError(err))
+		} else {
+			for i := range services.Items {
+				if services.Items[i].CreateExternalLoadBalancer && services.Items[i].Port == service.Port {
+					allErrs = append(allErrs, errs.NewConflict("service", service.Namespace, fmt.Errorf("Port: %d is already in use", service.Port)))
+					break
+				}
+			}
+		}
+	}
 	allErrs = append(allErrs, validateLabels(service.Labels)...)
 	allErrs = append(allErrs, validateLabels(service.Selector)...)
 	return allErrs

diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/capabilities"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
@@ -625,9 +626,10 @@ func TestValidatePodUpdate(t *testing.T) {
 
 func TestValidateService(t *testing.T) {
 	testCases := []struct {
-		name    string
-		svc     api.Service
-		numErrs int
+		name     string
+		svc      api.Service
+		existing api.ServiceList
+		numErrs  int
 	}{
 		{
 			name: "missing id",
@@ -727,6 +729,64 @@ func TestValidateService(t *testing.T) {
 			},
 			numErrs: 0,
 		},
+		{
+			name: "invalid port in use",
+			svc: api.Service{
+				ObjectMeta: api.ObjectMeta{Name: "abc123", Namespace: api.NamespaceDefault},
+				Port:       80,
+				CreateExternalLoadBalancer: true,
+				Selector:                   map[string]string{"foo": "bar"},
+			},
+			existing: api.ServiceList{
+				Items: []api.Service{
+					{Port: 80, CreateExternalLoadBalancer: true},
+				},
+			},
+			numErrs: 1,
+		},
+		{
+			name: "same port in use, but not external",
+			svc: api.Service{
+				ObjectMeta: api.ObjectMeta{Name: "abc123", Namespace: api.NamespaceDefault},
+				Port:       80,
+				CreateExternalLoadBalancer: true,
+				Selector:                   map[string]string{"foo": "bar"},
+			},
+			existing: api.ServiceList{
+				Items: []api.Service{
+					{Port: 80},
+				},
+			},
+			numErrs: 0,
+		},
+		{
+			name: "same port in use, but not external on input",
+			svc: api.Service{
+				ObjectMeta: api.ObjectMeta{Name: "abc123", Namespace: api.NamespaceDefault},
+				Port:       80,
+				Selector:   map[string]string{"foo": "bar"},
+			},
+			existing: api.ServiceList{
+				Items: []api.Service{
+					{Port: 80, CreateExternalLoadBalancer: true},
+				},
+			},
+			numErrs: 0,
+		},
+		{
+			name: "same port in use, but neither external",
+			svc: api.Service{
+				ObjectMeta: api.ObjectMeta{Name: "abc123", Namespace: api.NamespaceDefault},
+				Port:       80,
+				Selector:   map[string]string{"foo": "bar"},
+			},
+			existing: api.ServiceList{
+				Items: []api.Service{
+					{Port: 80},
+				},
+			},
+			numErrs: 0,
+		},
 		{
 			name: "invalid label",
 			svc: api.Service{
@@ -745,7 +805,9 @@ func TestValidateService(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		errs := ValidateService(&tc.svc)
+		registry := registrytest.NewServiceRegistry()
+		registry.List = tc.existing
+		errs := ValidateService(&tc.svc, registry, api.NewDefaultContext())
 		if len(errs) != tc.numErrs {
 			t.Errorf("Unexpected error list for case %q: %+v", tc.name, errs)
 		}
@@ -756,7 +818,7 @@ func TestValidateService(t *testing.T) {
 		ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault},
 		Selector:   map[string]string{"foo": "bar"},
 	}
-	errs := ValidateService(&svc)
+	errs := ValidateService(&svc, registrytest.NewServiceRegistry(), api.NewDefaultContext())
 	if len(errs) != 0 {
 		t.Errorf("Unexpected non-zero error list: %#v", errs)
 	}

diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go
@@ -87,7 +87,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); len(errs) > 0 {
+	if errs := validation.ValidateService(service, rs.registry, ctx); len(errs) > 0 {
 		return nil, errors.NewInvalid("service", service.Name, errs)
 	}
 
@@ -229,7 +229,7 @@ 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); len(errs) > 0 {
+	if errs := validation.ValidateService(service, rs.registry, ctx); len(errs) > 0 {
 		return nil, errors.NewInvalid("service", service.Name, errs)
 	}
 	return apiserver.MakeAsync(func() (runtime.Object, error) {