diff --git a/pkg/api/types.go b/pkg/api/types.go index baf7eefab6d..a3970c76208 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -486,11 +486,13 @@ type Service struct { // Optional, if unspecified use the first port on the container. ContainerPort util.IntOrString `json:"containerPort,omitempty" yaml:"containerPort,omitempty"` - // PortalIP is assigned by the master. If specified by the user it will be ignored. - // TODO: This is awkward - if we had a BoundService, it would be better factored. + // 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 + // not be changed by updates. PortalIP string `json:"portalIP,omitempty" yaml:"portalIP,omitempty"` // ProxyPort is assigned by the master. If specified by the user it will be ignored. + // TODO: This is awkward - if we had a BoundService, it would be better factored. ProxyPort int `json:"proxyPort,omitempty" yaml:"proxyPort,omitempty"` } diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index b3420f21fcc..341b037a9ca 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -454,7 +454,9 @@ type Service struct { // Optional, if unspecified use the first port on the container. ContainerPort util.IntOrString `json:"containerPort,omitempty" yaml:"containerPort,omitempty"` - // PortalIP is assigned by the master. If specified by the user it will be ignored. + // 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 + // not be changed by updates. PortalIP string `json:"portalIP,omitempty" yaml:"portalIP,omitempty"` // ProxyPort is assigned by the master. If specified by the user it will be ignored. diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index 43ea1383230..e73a3db7bdf 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -419,7 +419,9 @@ type Service struct { // Optional, if unspecified use the first port on the container. ContainerPort util.IntOrString `json:"containerPort,omitempty" yaml:"containerPort,omitempty"` - // PortalIP is assigned by the master. If specified by the user it will be ignored. + // 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 + // not be changed by updates. PortalIP string `json:"portalIP,omitempty" yaml:"portalIP,omitempty"` // ProxyPort is assigned by the master. If specified by the user it will be ignored. diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 122810e1b18..a510291172e 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -572,9 +572,6 @@ type ReplicationControllerList struct { // ServiceStatus represents the current status of a service type ServiceStatus struct { - // PortalIP is assigned by the master. - PortalIP string `json:"portalIP,omitempty" yaml:"portalIP,omitempty"` - // ProxyPort is assigned by the master. If 0, the proxy will choose an ephemeral port. ProxyPort int `json:"proxyPort,omitempty" yaml:"proxyPort,omitempty"` } @@ -591,6 +588,11 @@ type ServiceSpec struct { // This service will route traffic to pods having labels matching this selector. Selector map[string]string `json:"selector,omitempty" yaml:"selector,omitempty"` + // 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 + // not be changed by updates. + PortalIP string `json:"portalIP,omitempty" yaml:"portalIP,omitempty"` + // CreateExternalLoadBalancer indicates whether a load balancer should be created for this service. CreateExternalLoadBalancer bool `json:"createExternalLoadBalancer,omitempty" yaml:"createExternalLoadBalancer,omitempty"` diff --git a/pkg/registry/registrytest/service.go b/pkg/registry/registrytest/service.go index d2a0c194f00..02f10c3aa2a 100644 --- a/pkg/registry/registrytest/service.go +++ b/pkg/registry/registrytest/service.go @@ -56,7 +56,8 @@ func (r *ServiceRegistry) CreateService(ctx api.Context, svc *api.Service) error r.mu.Lock() defer r.mu.Unlock() - r.Service = svc + r.Service = new(api.Service) + *r.Service = *svc r.List.Items = append(r.List.Items, *svc) return r.Err } @@ -83,7 +84,7 @@ func (r *ServiceRegistry) UpdateService(ctx api.Context, svc *api.Service) error defer r.mu.Unlock() r.UpdatedID = svc.Name - r.Service = svc + *r.Service = *svc return r.Err } diff --git a/pkg/registry/service/rest.go b/pkg/registry/service/rest.go index 8ed59b5c413..45223119c48 100644 --- a/pkg/registry/service/rest.go +++ b/pkg/registry/service/rest.go @@ -90,10 +90,20 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje service.CreationTimestamp = util.Now() - if ip, err := rs.portalMgr.AllocateNext(); err != nil { - return nil, err + if service.PortalIP == "" { + // Allocate next available. + if ip, err := rs.portalMgr.AllocateNext(); err != nil { + return nil, err + } else { + service.PortalIP = ip.String() + } } else { - service.PortalIP = ip.String() + // Try to respect the requested IP. + if err := rs.portalMgr.Allocate(net.ParseIP(service.PortalIP)); err != nil { + // TODO: Differentiate "IP already allocated" from real errors. + el := errors.ValidationErrorList{errors.NewFieldInvalid("portalIP", service.PortalIP)} + return nil, errors.NewInvalid("service", service.Name, el) + } } return apiserver.MakeAsync(func() (runtime.Object, error) { @@ -224,8 +234,12 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje if err != nil { return nil, err } + if service.PortalIP != cur.PortalIP { + // TODO: Would be nice to pass "field is immutable" to users. + el := errors.ValidationErrorList{errors.NewFieldInvalid("portalIP", service.PortalIP)} + return nil, errors.NewInvalid("service", service.Name, el) + } // Copy over non-user fields. - service.PortalIP = cur.PortalIP service.ProxyPort = cur.ProxyPort // TODO: check to see if external load balancer status changed err = rs.registry.UpdateService(ctx, service) diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index 5f0a3fae827..c8baf416c57 100644 --- a/pkg/registry/service/rest_test.go +++ b/pkg/registry/service/rest_test.go @@ -446,6 +446,20 @@ func TestServiceRegistryIPAllocation(t *testing.T) { if created_service_2.PortalIP != "1.2.3.2" { // new IP t.Errorf("Unexpected PortalIP: %s", created_service_2.PortalIP) } + + svc3 := &api.Service{ + Port: 6502, + ObjectMeta: api.ObjectMeta{Name: "quux"}, + Selector: map[string]string{"bar": "baz"}, + PortalIP: "1.2.3.93", + } + ctx = api.NewDefaultContext() + c3, _ := rest.Create(ctx, svc3) + created_svc3 := <-c3 + created_service_3 := created_svc3.(*api.Service) + if created_service_3.PortalIP != "1.2.3.93" { // specific IP + t.Errorf("Unexpected PortalIP: %s", created_service_3.PortalIP) + } } func TestServiceRegistryIPReallocation(t *testing.T) { @@ -518,8 +532,7 @@ func TestServiceRegistryIPUpdate(t *testing.T) { update := new(api.Service) *update = *created_service update.Port = 6503 - update.PortalIP = "8.6.7.5" - update.ProxyPort = 309 + update.ProxyPort = 309 // should be ignored c, _ = rest.Update(ctx, update) updated_svc := <-c @@ -527,12 +540,20 @@ func TestServiceRegistryIPUpdate(t *testing.T) { if updated_service.Port != 6503 { t.Errorf("Expected port 6503, but got %v", updated_service.Port) } - if updated_service.PortalIP != "1.2.3.1" { // unchanged, despite trying - t.Errorf("Unexpected PortalIP: %s", updated_service.PortalIP) - } if updated_service.ProxyPort != 0 { // unchanged, despite trying t.Errorf("Unexpected ProxyPort: %d", updated_service.ProxyPort) } + + *update = *created_service + update.Port = 6503 + update.PortalIP = "1.2.3.76" // error + + c, _ = rest.Update(ctx, update) + result := <-c + st := result.(*api.Status) + if st.Reason != api.StatusReasonInvalid { + t.Errorf("Expected to get an invalid error, got %v", st) + } } func TestServiceRegistryIPExternalLoadBalancer(t *testing.T) {