From f556f2f82f198bfd5c30704a000c183e8101c7c8 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Thu, 30 Oct 2014 23:03:52 -0700 Subject: [PATCH] Add some validation for externalized service ports. --- examples/examples_test.go | 3 +- pkg/api/errors/errors.go | 13 +++++ pkg/api/types.go | 7 +++ pkg/api/validation/validation.go | 20 +++++++- pkg/api/validation/validation_test.go | 72 +++++++++++++++++++++++++-- pkg/registry/service/rest.go | 4 +- 6 files changed, 110 insertions(+), 9 deletions(-) diff --git a/examples/examples_test.go b/examples/examples_test.go index 253643e537b..ff4552a1f4b 100644 --- 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 index 90425a546ad..760de6dd334 100644 --- 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 index 811c98eb271..a2b1a479dec 100644 --- 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 index 3b6742c7591..1e773496de1 100644 --- 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 index e67bf79455b..909ec2659cf 100644 --- 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 index 8bd1652caf6..09664541fdf 100644 --- 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) {