Merge pull request #4105 from thockin/kill_proxy_port

pkg/api/rest/types.go

@@ -98,8 +98,6 @@ func (svcStrategy) NamespaceScoped() bool {
 // ResetBeforeCreate clears fields that are not allowed to be set by end users on creation.
 func (svcStrategy) ResetBeforeCreate(obj runtime.Object) {
 	service := obj.(*api.Service)
-	// TODO: Get rid of ProxyPort.
-	service.Spec.ProxyPort = 0
 	service.Status = api.ServiceStatus{}
 }
 

pkg/api/types.go

@@ -672,10 +672,6 @@ type ServiceSpec struct {
 	// not be changed by updates.
 	PortalIP string `json:"portalIP,omitempty"`
 
-	// ProxyPort is assigned by the master.  If 0, the proxy will choose an ephemeral port.
-	// TODO: This is awkward - if we had a BoundService, it would be better factored.
-	ProxyPort int `json:"proxyPort,omitempty"`
-
 	// CreateExternalLoadBalancer indicates whether a load balancer should be created for this service.
 	CreateExternalLoadBalancer bool `json:"createExternalLoadBalancer,omitempty"`
 	// PublicIPs are used by external load balancers.

pkg/api/v1beta1/conversion.go

@@ -634,7 +634,6 @@ func init() {
 			out.PublicIPs = in.Spec.PublicIPs
 			out.ContainerPort = in.Spec.ContainerPort
 			out.PortalIP = in.Spec.PortalIP
-			out.ProxyPort = in.Spec.ProxyPort
 			if err := s.Convert(&in.Spec.SessionAffinity, &out.SessionAffinity, 0); err != nil {
 				return err
 			}
@@ -661,7 +660,6 @@ func init() {
 			out.Spec.PublicIPs = in.PublicIPs
 			out.Spec.ContainerPort = in.ContainerPort
 			out.Spec.PortalIP = in.PortalIP
-			out.Spec.ProxyPort = in.ProxyPort
 			if err := s.Convert(&in.SessionAffinity, &out.Spec.SessionAffinity, 0); err != nil {
 				return err
 			}

pkg/api/v1beta1/types.go

@@ -544,7 +544,7 @@ type Service struct {
 	// not be changed by updates.
 	PortalIP string `json:"portalIP,omitempty" description:"IP address of the service; usually assigned by the system; if specified, it will be allocated to the service if unused, and creation of the service will fail otherwise; cannot be updated"`
 
-	// ProxyPort is assigned by the master.  If specified by the user it will be ignored.
+	// DEPRECATED: has no implementation.
 	ProxyPort int `json:"proxyPort,omitempty" description:"if non-zero, a pre-allocated host port used for this service by the proxy on each node; assigned by the master and ignored on input"`
 
 	// Optional: Supports "ClientIP" and "None".  Used to maintain session affinity.

pkg/api/v1beta2/conversion.go

@@ -554,7 +554,6 @@ func init() {
 			out.PublicIPs = in.Spec.PublicIPs
 			out.ContainerPort = in.Spec.ContainerPort
 			out.PortalIP = in.Spec.PortalIP
-			out.ProxyPort = in.Spec.ProxyPort
 			if err := s.Convert(&in.Spec.SessionAffinity, &out.SessionAffinity, 0); err != nil {
 				return err
 			}
@@ -581,7 +580,6 @@ func init() {
 			out.Spec.PublicIPs = in.PublicIPs
 			out.Spec.ContainerPort = in.ContainerPort
 			out.Spec.PortalIP = in.PortalIP
-			out.Spec.ProxyPort = in.ProxyPort
 			if err := s.Convert(&in.SessionAffinity, &out.Spec.SessionAffinity, 0); err != nil {
 				return err
 			}

pkg/api/v1beta2/types.go

@@ -508,7 +508,7 @@ type Service struct {
 	// not be changed by updates.
 	PortalIP string `json:"portalIP,omitempty" description:"IP address of the service; usually assigned by the system; if specified, it will be allocated to the service if unused, and creation of the service will fail otherwise; cannot be updated"`
 
-	// ProxyPort is assigned by the master.  If specified by the user it will be ignored.
+	// DEPRECATED: has no implementation.
 	ProxyPort int `json:"proxyPort,omitempty" description:"if non-zero, a pre-allocated host port used for this service by the proxy on each node; assigned by the master and ignored on input"`
 
 	// Optional: Supports "ClientIP" and "None".  Used to maintain session affinity.

pkg/api/v1beta3/types.go

@@ -691,9 +691,6 @@ type ServiceSpec struct {
 	// not be changed by updates.
 	PortalIP string `json:"portalIP,omitempty"`
 
-	// ProxyPort is assigned by the master.  If 0, the proxy will choose an ephemeral port.
-	ProxyPort int `json:"proxyPort,omitempty"`
-
 	// CreateExternalLoadBalancer indicates whether a load balancer should be created for this service.
 	CreateExternalLoadBalancer bool `json:"createExternalLoadBalancer,omitempty"`
 	// PublicIPs are used by external load balancers.

pkg/proxy/proxier.go

@@ -481,8 +481,8 @@ func (proxier *Proxier) OnUpdate(services []api.Service) {
 				glog.Errorf("Failed to stop service %q: %v", service.Name, err)
 			}
 		}
-		glog.V(1).Infof("Adding new service %q at %s:%d/%s (local :%d)", service.Name, serviceIP, service.Spec.Port, service.Spec.Protocol, service.Spec.ProxyPort)
-		info, err := proxier.addServiceOnPort(service.Name, service.Spec.Protocol, service.Spec.ProxyPort, udpIdleTimeout)
+		glog.V(1).Infof("Adding new service %q at %s:%d/%s", service.Name, serviceIP, service.Spec.Port, service.Spec.Protocol)
+		info, err := proxier.addServiceOnPort(service.Name, service.Spec.Protocol, 0, udpIdleTimeout)
 		if err != nil {
 			glog.Errorf("Failed to start proxy for %q: %v", service.Name, err)
 			continue

pkg/proxy/proxier_test.go

@@ -23,7 +23,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"strconv"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -384,8 +383,12 @@ func TestTCPProxyUpdateDeleteUpdate(t *testing.T) {
 	}
 	waitForNumProxyLoops(t, p, 0)
 	p.OnUpdate([]api.Service{
-		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: svcInfo.proxyPort, Protocol: "TCP", ProxyPort: svcInfo.proxyPort}, Status: api.ServiceStatus{}},
+		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: svcInfo.proxyPort, Protocol: "TCP"}, Status: api.ServiceStatus{}},
 	})
+	svcInfo, exists := p.getServiceInfo("echo")
+	if !exists {
+		t.Fatalf("can't find serviceInfo")
+	}
 	testEchoTCP(t, "127.0.0.1", svcInfo.proxyPort)
 	waitForNumProxyLoops(t, p, 1)
 }
@@ -419,8 +422,12 @@ func TestUDPProxyUpdateDeleteUpdate(t *testing.T) {
 	}
 	waitForNumProxyLoops(t, p, 0)
 	p.OnUpdate([]api.Service{
-		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: svcInfo.proxyPort, Protocol: "UDP", ProxyPort: svcInfo.proxyPort}, Status: api.ServiceStatus{}},
+		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: svcInfo.proxyPort, Protocol: "UDP"}, Status: api.ServiceStatus{}},
 	})
+	svcInfo, exists := p.getServiceInfo("echo")
+	if !exists {
+		t.Fatalf("can't find serviceInfo")
+	}
 	testEchoUDP(t, "127.0.0.1", svcInfo.proxyPort)
 	waitForNumProxyLoops(t, p, 1)
 }
@@ -444,36 +451,21 @@ func TestTCPProxyUpdatePort(t *testing.T) {
 	testEchoTCP(t, "127.0.0.1", svcInfo.proxyPort)
 	waitForNumProxyLoops(t, p, 1)
 
-	// add a new dummy listener in order to get a port that is free
-	l, _ := net.Listen("tcp", ":0")
-	_, newPortStr, _ := net.SplitHostPort(l.Addr().String())
-	newPort, _ := strconv.Atoi(newPortStr)
-	l.Close()
-
-	// Wait for the socket to actually get free.
-	if err := waitForClosedPortTCP(p, newPort); err != nil {
-		t.Fatalf(err.Error())
-	}
-	if svcInfo.proxyPort == newPort {
-		t.Errorf("expected difference, got %d %d", newPort, svcInfo.proxyPort)
-	}
 	p.OnUpdate([]api.Service{
-		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: newPort, Protocol: "TCP", ProxyPort: newPort}, Status: api.ServiceStatus{}},
+		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: 99, Protocol: "TCP"}, Status: api.ServiceStatus{}},
 	})
+	// Wait for the socket to actually get free.
 	if err := waitForClosedPortTCP(p, svcInfo.proxyPort); err != nil {
 		t.Fatalf(err.Error())
 	}
-	testEchoTCP(t, "127.0.0.1", newPort)
+	svcInfo, exists := p.getServiceInfo("echo")
+	if !exists {
+		t.Fatalf("can't find serviceInfo")
+	}
+	testEchoTCP(t, "127.0.0.1", svcInfo.proxyPort)
 	// This is a bit async, but this should be sufficient.
 	time.Sleep(500 * time.Millisecond)
 	waitForNumProxyLoops(t, p, 1)
-
-	// Ensure the old port is released and re-usable.
-	l, err = net.Listen("tcp", joinHostPort("", svcInfo.proxyPort))
-	if err != nil {
-		t.Fatalf("can't claim released port: %s", err)
-	}
-	l.Close()
 }
 
 func TestUDPProxyUpdatePort(t *testing.T) {
@@ -494,34 +486,19 @@ func TestUDPProxyUpdatePort(t *testing.T) {
 	}
 	waitForNumProxyLoops(t, p, 1)
 
-	// add a new dummy listener in order to get a port that is free
-	pc, _ := net.ListenPacket("udp", ":0")
-	_, newPortStr, _ := net.SplitHostPort(pc.LocalAddr().String())
-	newPort, _ := strconv.Atoi(newPortStr)
-	pc.Close()
-
-	// Wait for the socket to actually get free.
-	if err := waitForClosedPortUDP(p, newPort); err != nil {
-		t.Fatalf(err.Error())
-	}
-	if svcInfo.proxyPort == newPort {
-		t.Errorf("expected difference, got %d %d", newPort, svcInfo.proxyPort)
-	}
 	p.OnUpdate([]api.Service{
-		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: newPort, Protocol: "UDP", ProxyPort: newPort}, Status: api.ServiceStatus{}},
+		{ObjectMeta: api.ObjectMeta{Name: "echo"}, Spec: api.ServiceSpec{Port: 99, Protocol: "UDP"}, Status: api.ServiceStatus{}},
 	})
+	// Wait for the socket to actually get free.
 	if err := waitForClosedPortUDP(p, svcInfo.proxyPort); err != nil {
 		t.Fatalf(err.Error())
 	}
-	testEchoUDP(t, "127.0.0.1", newPort)
-	waitForNumProxyLoops(t, p, 1)
-
-	// Ensure the old port is released and re-usable.
-	pc, err = net.ListenPacket("udp", joinHostPort("", svcInfo.proxyPort))
-	if err != nil {
-		t.Fatalf("can't claim released port: %s", err)
+	svcInfo, exists := p.getServiceInfo("echo")
+	if !exists {
+		t.Fatalf("can't find serviceInfo")
 	}
-	pc.Close()
+	testEchoUDP(t, "127.0.0.1", svcInfo.proxyPort)
+	waitForNumProxyLoops(t, p, 1)
 }
 
 // TODO: Test UDP timeouts.

pkg/registry/service/rest.go

@@ -229,10 +229,7 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE
 	}
 
 	// Copy over non-user fields
-	// TODO: this should be a Status field, since the end user does not set it.
 	// TODO: make this a merge function
-	service.Spec.ProxyPort = oldService.Spec.ProxyPort
-
 	if errs := validation.ValidateServiceUpdate(oldService, service); len(errs) > 0 {
 		return nil, errors.NewInvalid("service", service.Name, errs)
 	}

pkg/registry/service/rest_test.go

@@ -69,9 +69,6 @@ func TestServiceRegistryCreate(t *testing.T) {
 	if created_service.Spec.PortalIP != "1.2.3.1" {
 		t.Errorf("Unexpected PortalIP: %s", created_service.Spec.PortalIP)
 	}
-	if created_service.Spec.ProxyPort != 0 {
-		t.Errorf("Unexpected ProxyPort: %d", created_service.Spec.ProxyPort)
-	}
 	if len(fakeCloud.Calls) != 0 {
 		t.Errorf("Unexpected call(s): %#v", fakeCloud.Calls)
 	}
@@ -509,14 +506,10 @@ func TestServiceRegistryIPUpdate(t *testing.T) {
 	if created_service.Spec.PortalIP != "1.2.3.1" {
 		t.Errorf("Unexpected PortalIP: %s", created_service.Spec.PortalIP)
 	}
-	if created_service.Spec.ProxyPort != 0 {
-		t.Errorf("Unexpected ProxyPort: %d", created_service.Spec.ProxyPort)
-	}
 
 	update := new(api.Service)
 	*update = *created_service
 	update.Spec.Port = 6503
-	update.Spec.ProxyPort = 309 // should be ignored
 
 	c, _ = rest.Update(ctx, update)
 	updated_svc := <-c
@@ -524,9 +517,6 @@ func TestServiceRegistryIPUpdate(t *testing.T) {
 	if updated_service.Spec.Port != 6503 {
 		t.Errorf("Expected port 6503, but got %v", updated_service.Spec.Port)
 	}
-	if updated_service.Spec.ProxyPort != 0 { // unchanged, despite trying
-		t.Errorf("Unexpected ProxyPort: %d", updated_service.Spec.ProxyPort)
-	}
 
 	*update = *created_service
 	update.Spec.Port = 6503
@@ -563,9 +553,6 @@ func TestServiceRegistryIPExternalLoadBalancer(t *testing.T) {
 	if created_service.Spec.PortalIP != "1.2.3.1" {
 		t.Errorf("Unexpected PortalIP: %s", created_service.Spec.PortalIP)
 	}
-	if created_service.Spec.ProxyPort != 0 {
-		t.Errorf("Unexpected ProxyPort: %d", created_service.Spec.ProxyPort)
-	}
 
 	update := new(api.Service)
 	*update = *created_service