From 2c27f7d3320b34e8cb34d3e6327548cba4bac3e9 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 18 Nov 2014 12:49:00 -0500 Subject: [PATCH] Allow an empty service --- pkg/api/types.go | 6 ++- pkg/api/v1beta1/conversion_test.go | 32 ++++++++++++ pkg/api/v1beta1/types.go | 8 +-- pkg/api/v1beta2/conversion_test.go | 39 +++++++++++++- pkg/api/v1beta2/types.go | 8 +-- pkg/api/v1beta3/types.go | 4 +- pkg/api/validation/validation.go | 9 ++-- pkg/api/validation/validation_test.go | 19 +++++-- pkg/master/publish.go | 21 ++++---- pkg/registry/service/rest_test.go | 4 +- pkg/service/endpoints_controller.go | 6 +-- pkg/service/endpoints_controller_test.go | 65 ++++++++++++++++++++++++ pkg/util/fake_handler.go | 9 ++++ 13 files changed, 198 insertions(+), 32 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index 8b87f8b7da0..c1efe41dc64 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -593,8 +593,10 @@ type ServiceSpec struct { // Optional: Supports "TCP" and "UDP". Defaults to "TCP". Protocol Protocol `json:"protocol,omitempty"` - // This service will route traffic to pods having labels matching this selector. - Selector map[string]string `json:"selector,omitempty"` + // This service will route traffic to pods having labels matching this selector. If empty or not present, + // the service is assumed to have endpoints set by an external process and Kubernetes will not modify + // those endpoints. + Selector map[string]string `json:"selector"` // PortalIP is usually assigned by the master. If specified by the user // we will try to respect it or else fail the request. This field can diff --git a/pkg/api/v1beta1/conversion_test.go b/pkg/api/v1beta1/conversion_test.go index 5c9348207a5..302d7aed190 100644 --- a/pkg/api/v1beta1/conversion_test.go +++ b/pkg/api/v1beta1/conversion_test.go @@ -192,3 +192,35 @@ func TestMinionListConversionToOld(t *testing.T) { t.Errorf("Expected: %#v, got %#v", e, a) } } + +func TestServiceEmptySelector(t *testing.T) { + // Nil map should be preserved + svc := &v1beta1.Service{Selector: nil} + data, err := newer.Scheme.EncodeToVersion(svc, "v1beta1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + obj, err := newer.Scheme.Decode(data) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + selector := obj.(*newer.Service).Spec.Selector + if selector != nil { + t.Errorf("unexpected selector: %#v", obj) + } + + // Empty map should be preserved + svc2 := &v1beta1.Service{Selector: map[string]string{}} + data, err = newer.Scheme.EncodeToVersion(svc2, "v1beta1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + obj, err = newer.Scheme.Decode(data) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + selector = obj.(*newer.Service).Spec.Selector + if selector == nil || len(selector) != 0 { + t.Errorf("unexpected selector: %#v", obj) + } +} diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index fad4907f123..108ec89fce9 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -465,9 +465,11 @@ type Service struct { // This service's labels. Labels map[string]string `json:"labels,omitempty" description:"map of string keys and values that can be used to organize and categorize services"` - // This service will route traffic to pods having labels matching this selector. - Selector map[string]string `json:"selector,omitempty" description:"label keys and values that must match in order to receive traffic for this service"` - CreateExternalLoadBalancer bool `json:"createExternalLoadBalancer,omitempty" description:"set up a cloud-provider-specific load balancer on an external IP"` + // This service will route traffic to pods having labels matching this selector. If null, no endpoints will be automatically created. If empty, all pods will be selected. + Selector map[string]string `json:"selector" description:"label keys and values that must match in order to receive traffic for this service; if empty, all pods are selected, if not specified, endpoints must be manually specified"` + // An external load balancer should be set up via the cloud-provider + CreateExternalLoadBalancer bool `json:"createExternalLoadBalancer,omitempty" description:"set up a cloud-provider-specific load balancer on an external IP"` + // PublicIPs are used by external load balancers. PublicIPs []string `json:"publicIPs,omitempty" description:"externally visible IPs from which to select the address for the external load balancer"` diff --git a/pkg/api/v1beta2/conversion_test.go b/pkg/api/v1beta2/conversion_test.go index 1ddb3d9caf9..357435745d3 100644 --- a/pkg/api/v1beta2/conversion_test.go +++ b/pkg/api/v1beta2/conversion_test.go @@ -16,4 +16,41 @@ limitations under the License. package v1beta2_test -import () +import ( + "testing" + + newer "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta2" +) + +func TestServiceEmptySelector(t *testing.T) { + // Nil map should be preserved + svc := &v1beta2.Service{Selector: nil} + data, err := newer.Scheme.EncodeToVersion(svc, "v1beta2") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + obj, err := newer.Scheme.Decode(data) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + selector := obj.(*newer.Service).Spec.Selector + if selector != nil { + t.Errorf("unexpected selector: %#v", obj) + } + + // Empty map should be preserved + svc2 := &v1beta2.Service{Selector: map[string]string{}} + data, err = newer.Scheme.EncodeToVersion(svc2, "v1beta2") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + obj, err = newer.Scheme.Decode(data) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + selector = obj.(*newer.Service).Spec.Selector + if selector == nil || len(selector) != 0 { + t.Errorf("unexpected selector: %#v", obj) + } +} diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index b5b1f1adca4..dd2268c094b 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -430,9 +430,11 @@ type Service struct { // This service's labels. Labels map[string]string `json:"labels,omitempty" description:"map of string keys and values that can be used to organize and categorize services"` - // This service will route traffic to pods having labels matching this selector. - Selector map[string]string `json:"selector,omitempty" description:"label keys and values that must match in order to receive traffic for this service"` - CreateExternalLoadBalancer bool `json:"createExternalLoadBalancer,omitempty" description:"set up a cloud-provider-specific load balancer on an external IP"` + // This service will route traffic to pods having labels matching this selector. If null, no endpoints will be automatically created. If empty, all pods will be selected. + Selector map[string]string `json:"selector" description:"label keys and values that must match in order to receive traffic for this service; if empty, all pods are selected, if not specified, endpoints must be manually specified"` + // An external load balancer should be set up via the cloud-provider + CreateExternalLoadBalancer bool `json:"createExternalLoadBalancer,omitempty" description:"set up a cloud-provider-specific load balancer on an external IP"` + // PublicIPs are used by external load balancers. PublicIPs []string `json:"publicIPs,omitempty" description:"externally visible IPs from which to select the address for the external load balancer"` diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 65160c5dc6c..69652e716ab 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -601,8 +601,8 @@ type ServiceSpec struct { // Optional: Supports "TCP" and "UDP". Defaults to "TCP". Protocol Protocol `json:"protocol,omitempty"` - // This service will route traffic to pods having labels matching this selector. - Selector map[string]string `json:"selector,omitempty"` + // This service will route traffic to pods having labels matching this selector. If null, no endpoints will be automatically created. If empty, all pods will be selected. + Selector map[string]string `json:"selector"` // PortalIP is usually assigned by the master. If specified by the user // we will try to respect it or else fail the request. This field can diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 9eeccc7edef..8fff5166cf4 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -438,9 +438,12 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context } else if !supportedPortProtocols.Has(strings.ToUpper(string(service.Spec.Protocol))) { allErrs = append(allErrs, errs.NewFieldNotSupported("spec.protocol", service.Spec.Protocol)) } - if labels.Set(service.Spec.Selector).AsSelector().Empty() { - allErrs = append(allErrs, errs.NewFieldRequired("spec.selector", service.Spec.Selector)) + + if service.Spec.Selector != nil { + allErrs = append(allErrs, validateLabels(service.Spec.Selector, "spec.selector")...) } + allErrs = append(allErrs, validateLabels(service.Labels, "labels")...) + if service.Spec.CreateExternalLoadBalancer { services, err := lister.ListServices(ctx) if err != nil { @@ -456,8 +459,6 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context } } } - allErrs = append(allErrs, validateLabels(service.Labels, "labels")...) - allErrs = append(allErrs, validateLabels(service.Spec.Selector, "selector")...) return allErrs } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 7c23dbe6370..40ea9cf12e9 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -710,8 +710,8 @@ func TestValidateService(t *testing.T) { Port: 8675, }, }, - // Should fail because the selector is missing. - numErrs: 1, + // Should be ok because the selector is missing. + numErrs: 0, }, { name: "valid 1", @@ -824,12 +824,25 @@ func TestValidateService(t *testing.T) { "NoUppercaseOrSpecialCharsLike=Equals": "bar", }, }, + Spec: api.ServiceSpec{ + Port: 8675, + }, + }, + numErrs: 1, + }, + { + name: "invalid selector", + svc: api.Service{ + ObjectMeta: api.ObjectMeta{ + Name: "abc123", + Namespace: api.NamespaceDefault, + }, Spec: api.ServiceSpec{ Port: 8675, Selector: map[string]string{"foo": "bar", "NoUppercaseOrSpecialCharsLike=Equals": "bar"}, }, }, - numErrs: 2, + numErrs: 1, }, } diff --git a/pkg/master/publish.go b/pkg/master/publish.go index 912656422da..5e4041b3c7b 100644 --- a/pkg/master/publish.go +++ b/pkg/master/publish.go @@ -32,6 +32,7 @@ func (m *Master) serviceWriterLoop(stop chan struct{}) { // stop polling and start watching. // TODO: add endpoints of all replicas, not just the elected master. if m.readWriteServer != "" { + // TODO: the public port should be part of the argument here, port will not always be 443 if err := m.createMasterServiceIfNeeded("kubernetes", 443); err != nil { glog.Errorf("Can't create rw service: %v", err) } @@ -54,6 +55,7 @@ func (m *Master) roServiceWriterLoop(stop chan struct{}) { // TODO: when it becomes possible to change this stuff, // stop polling and start watching. if m.readOnlyServer != "" { + // TODO: the public port should be part of the argument here, port will not always be 80 if err := m.createMasterServiceIfNeeded("kubernetes-ro", 80); err != nil { glog.Errorf("Can't create ro service: %v", err) } @@ -81,14 +83,13 @@ func (m *Master) createMasterServiceIfNeeded(serviceName string, port int) error svc := &api.Service{ ObjectMeta: api.ObjectMeta{ Name: serviceName, - Namespace: "default", + Namespace: api.NamespaceDefault, + Labels: map[string]string{"provider": "kubernetes", "component": "apiserver"}, }, Spec: api.ServiceSpec{ Port: port, - // We're going to add the endpoints by hand, so this selector is mainly to - // prevent identification of other pods. This selector will be useful when - // we start hosting apiserver in a pod. - Selector: map[string]string{"provider": "kubernetes", "component": "apiserver"}, + // maintained by this code, not by the pod selector + Selector: nil, }, } // Kids, don't do this at home: this is a hack. There's no good way to call the business @@ -113,10 +114,12 @@ func (m *Master) ensureEndpointsContain(serviceName string, endpoint string) err ctx := api.NewDefaultContext() e, err := m.endpointRegistry.GetEndpoints(ctx, serviceName) if err != nil { - e = &api.Endpoints{} - // Fill in ID if it didn't exist already - e.ObjectMeta.Name = serviceName - e.ObjectMeta.Namespace = "default" + e = &api.Endpoints{ + ObjectMeta: api.ObjectMeta{ + Name: serviceName, + Namespace: api.NamespaceDefault, + }, + } } found := false for i := range e.Endpoints { diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index 299cb599abd..36f16eeb983 100644 --- a/pkg/registry/service/rest_test.go +++ b/pkg/registry/service/rest_test.go @@ -167,11 +167,11 @@ func TestServiceStorageValidatesUpdate(t *testing.T) { Selector: map[string]string{"bar": "baz"}, }, }, - "empty selector": { + "invalid selector": { ObjectMeta: api.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ Port: 6502, - Selector: map[string]string{}, + Selector: map[string]string{"ThisSelectorFailsValidation": "ok"}, }, }, } diff --git a/pkg/service/endpoints_controller.go b/pkg/service/endpoints_controller.go index 0ff355bff98..49aab63f412 100644 --- a/pkg/service/endpoints_controller.go +++ b/pkg/service/endpoints_controller.go @@ -51,11 +51,11 @@ func (e *EndpointController) SyncServiceEndpoints() error { } var resultErr error for _, service := range services.Items { - if service.Name == "kubernetes" || service.Name == "kubernetes-ro" { - // This is a temporary hack for supporting the master services - // until we actually start running apiserver in a pod. + if service.Spec.Selector == nil { + // services without a selector receive no endpoints. The last endpoint will be used. continue } + glog.Infof("About to update endpoints for service %v", service.Name) pods, err := e.client.Pods(service.Namespace).List(labels.Set(service.Spec.Selector).AsSelector()) if err != nil { diff --git a/pkg/service/endpoints_controller_test.go b/pkg/service/endpoints_controller_test.go index 00e570bfc6c..91ec1af64a1 100644 --- a/pkg/service/endpoints_controller_test.go +++ b/pkg/service/endpoints_controller_test.go @@ -222,6 +222,71 @@ func TestSyncEndpointsError(t *testing.T) { } } +func TestSyncEndpointsItemsPreserveNoSelector(t *testing.T) { + serviceList := api.ServiceList{ + Items: []api.Service{ + { + ObjectMeta: api.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{}, + }, + }, + } + testServer, endpointsHandler := makeTestServer(t, + serverResponse{http.StatusOK, newPodList(0)}, + serverResponse{http.StatusOK, &serviceList}, + serverResponse{http.StatusOK, &api.Endpoints{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + ResourceVersion: "1", + }, + Endpoints: []string{"6.7.8.9:1000"}, + }}) + defer testServer.Close() + client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()}) + endpoints := NewEndpointController(client) + if err := endpoints.SyncServiceEndpoints(); err != nil { + t.Errorf("unexpected error: %v", err) + } + endpointsHandler.ValidateRequestCount(t, 0) +} + +func TestSyncEndpointsItemsEmptySelectorSelectsAll(t *testing.T) { + serviceList := api.ServiceList{ + Items: []api.Service{ + { + ObjectMeta: api.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{}, + }, + }, + }, + } + testServer, endpointsHandler := makeTestServer(t, + serverResponse{http.StatusOK, newPodList(1)}, + serverResponse{http.StatusOK, &serviceList}, + serverResponse{http.StatusOK, &api.Endpoints{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + ResourceVersion: "1", + }, + Endpoints: []string{}, + }}) + defer testServer.Close() + client := client.NewOrDie(&client.Config{Host: testServer.URL, Version: testapi.Version()}) + endpoints := NewEndpointController(client) + if err := endpoints.SyncServiceEndpoints(); err != nil { + t.Errorf("unexpected error: %v", err) + } + data := runtime.EncodeOrDie(testapi.Codec(), &api.Endpoints{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + ResourceVersion: "1", + }, + Endpoints: []string{"1.2.3.4:8080"}, + }) + endpointsHandler.ValidateRequest(t, "/api/"+testapi.Version()+"/endpoints/foo", "PUT", &data) +} + func TestSyncEndpointsItemsPreexisting(t *testing.T) { serviceList := api.ServiceList{ Items: []api.Service{ diff --git a/pkg/util/fake_handler.go b/pkg/util/fake_handler.go index 4ec43b3bfb4..6cd17fab2f1 100644 --- a/pkg/util/fake_handler.go +++ b/pkg/util/fake_handler.go @@ -73,6 +73,15 @@ func (f *FakeHandler) ServeHTTP(response http.ResponseWriter, request *http.Requ f.RequestBody = string(bodyReceived) } +func (f *FakeHandler) ValidateRequestCount(t TestInterface, count int) { + f.lock.Lock() + defer f.lock.Unlock() + if f.requestCount != count { + t.Logf("Expected %d call, but got %d. Only the last call is recorded and checked.", count, f.requestCount) + } + f.hasBeenChecked = true +} + // ValidateRequest verifies that FakeHandler received a request with expected path, method, and body. func (f *FakeHandler) ValidateRequest(t TestInterface, expectedPath, expectedMethod string, body *string) { f.lock.Lock()