Pass ports as []*api.ServicePort into LoadBalancer provider

pkg/cloudprovider/cloud.go

@@ -81,7 +81,7 @@ type TCPLoadBalancer interface {
 	// if so, what its status is.
 	GetTCPLoadBalancer(name, region string) (status *api.LoadBalancerStatus, exists bool, err error)
 	// CreateTCPLoadBalancer creates a new tcp load balancer. Returns the status of the balancer
-	CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []int, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error)
+	CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error)
 	// UpdateTCPLoadBalancer updates hosts under the specified load balancer.
 	UpdateTCPLoadBalancer(name, region string, hosts []string) error
 	// EnsureTCPLoadBalancerDeleted deletes the specified load balancer if it

pkg/cloudprovider/fake/fake.go

@@ -33,7 +33,7 @@ type FakeBalancer struct {
 	Name       string
 	Region     string
 	ExternalIP net.IP
-	Ports      []int
+	Ports      []*api.ServicePort
 	Hosts      []string
 }
 
@@ -119,7 +119,7 @@ func (f *FakeCloud) GetTCPLoadBalancer(name, region string) (*api.LoadBalancerSt
 
 // CreateTCPLoadBalancer is a test-spy implementation of TCPLoadBalancer.CreateTCPLoadBalancer.
 // It adds an entry "create" into the internal method call record.
-func (f *FakeCloud) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []int, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) {
+func (f *FakeCloud) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) {
 	f.addCall("create")
 	f.Balancers = append(f.Balancers, FakeBalancer{name, region, externalIP, ports, hosts})
 

pkg/cloudprovider/gce/gce.go

@@ -326,7 +326,7 @@ func translateAffinityType(affinityType api.ServiceAffinity) GCEAffinityType {
 // CreateTCPLoadBalancer is an implementation of TCPLoadBalancer.CreateTCPLoadBalancer.
 // TODO(a-robinson): Don't just ignore specified IP addresses. Check if they're
 // owned by the project and available to be used, and use them if they are.
-func (gce *GCECloud) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []int, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) {
+func (gce *GCECloud) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinityType api.ServiceAffinity) (*api.LoadBalancerStatus, error) {
 	err := gce.makeTargetPool(name, region, hosts, translateAffinityType(affinityType))
 	if err != nil {
 		if !isHTTPErrorCode(err, http.StatusConflict) {
@@ -341,11 +341,11 @@ func (gce *GCECloud) CreateTCPLoadBalancer(name, region string, externalIP net.I
 	minPort := 65536
 	maxPort := 0
 	for i := range ports {
-		if ports[i] < minPort {
-			minPort = ports[i]
+		if ports[i].Port < minPort {
+			minPort = ports[i].Port
 		}
-		if ports[i] > maxPort {
-			maxPort = ports[i]
+		if ports[i].Port > maxPort {
+			maxPort = ports[i].Port
 		}
 	}
 	req := &compute.ForwardingRule{

pkg/cloudprovider/openstack/openstack.go

@@ -495,7 +495,7 @@ func (lb *LoadBalancer) GetTCPLoadBalancer(name, region string) (*api.LoadBalanc
 // a list of regions (from config) and query/create loadbalancers in
 // each region.
 
-func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []int, hosts []string, affinity api.ServiceAffinity) (*api.LoadBalancerStatus, error) {
+func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP net.IP, ports []*api.ServicePort, hosts []string, affinity api.ServiceAffinity) (*api.LoadBalancerStatus, error) {
 	glog.V(4).Infof("CreateTCPLoadBalancer(%v, %v, %v, %v, %v, %v)", name, region, externalIP, ports, hosts, affinity)
 
 	if len(ports) > 1 {
@@ -534,7 +534,7 @@ func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP ne
 
 		_, err = members.Create(lb.network, members.CreateOpts{
 			PoolID:       pool.ID,
-			ProtocolPort: ports[0], //TODO: need to handle multi-port
+			ProtocolPort: ports[0].Port, //TODO: need to handle multi-port
 			Address:      addr,
 		}).Extract()
 		if err != nil {
@@ -569,7 +569,7 @@ func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP ne
 		Description:  fmt.Sprintf("Kubernetes external service %s", name),
 		Address:      externalIP.String(),
 		Protocol:     "TCP",
-		ProtocolPort: ports[0], //TODO: need to handle multi-port
+		ProtocolPort: ports[0].Port, //TODO: need to handle multi-port
 		PoolID:       pool.ID,
 		Persistence:  persistence,
 	}).Extract()

pkg/cloudprovider/servicecontroller/servicecontroller.go

@@ -331,7 +331,7 @@ func (s *ServiceController) persistUpdate(service *api.Service) error {
 }
 
 func (s *ServiceController) createExternalLoadBalancer(service *api.Service) error {
-	ports, err := getTCPPorts(service)
+	ports, err := getPortsForLB(service)
 	if err != nil {
 		return err
 	}
@@ -436,7 +436,7 @@ func needsUpdate(oldService *api.Service, newService *api.Service) bool {
 	if wantsExternalLoadBalancer(oldService) != wantsExternalLoadBalancer(newService) {
 		return true
 	}
-	if !portsEqual(oldService, newService) || oldService.Spec.SessionAffinity != newService.Spec.SessionAffinity {
+	if !portsEqualForLB(oldService, newService) || oldService.Spec.SessionAffinity != newService.Spec.SessionAffinity {
 		return true
 	}
 	if len(oldService.Spec.DeprecatedPublicIPs) != len(newService.Spec.DeprecatedPublicIPs) {
@@ -454,8 +454,8 @@ func (s *ServiceController) loadBalancerName(service *api.Service) string {
 	return cloudprovider.GetLoadBalancerName(service)
 }
 
-func getTCPPorts(service *api.Service) ([]int, error) {
-	ports := []int{}
+func getPortsForLB(service *api.Service) ([]*api.ServicePort, error) {
+	ports := []*api.ServicePort{}
 	for i := range service.Spec.Ports {
 		// TODO: Support UDP. Remove the check from the API validation package once
 		// it's supported.
@@ -463,21 +463,58 @@ func getTCPPorts(service *api.Service) ([]int, error) {
 		if sp.Protocol != api.ProtocolTCP {
 			return nil, fmt.Errorf("external load balancers for non TCP services are not currently supported.")
 		}
-		ports = append(ports, sp.Port)
+		ports = append(ports, sp)
 	}
 	return ports, nil
 }
 
-func portsEqual(x, y *api.Service) bool {
-	xPorts, err := getTCPPorts(x)
+func portsEqualForLB(x, y *api.Service) bool {
+	xPorts, err := getPortsForLB(x)
 	if err != nil {
 		return false
 	}
-	yPorts, err := getTCPPorts(y)
+	yPorts, err := getPortsForLB(y)
 	if err != nil {
 		return false
 	}
-	return intSlicesEqual(xPorts, yPorts)
+	return portSlicesEqualForLB(xPorts, yPorts)
+}
+
+func portSlicesEqualForLB(x, y []*api.ServicePort) bool {
+	if len(x) != len(y) {
+		return false
+	}
+
+	for i := range x {
+		if !portEqualForLB(x[i], y[i]) {
+			return false
+		}
+	}
+	return true
+}
+
+func portEqualForLB(x, y *api.ServicePort) bool {
+	// TODO: Should we check name?  (In theory, an LB could expose it)
+	if x.Name != y.Name {
+		return false
+	}
+
+	if x.Protocol != y.Protocol {
+		return false
+	}
+
+	if x.Port != y.Port {
+		return false
+	}
+
+	if x.NodePort != y.NodePort {
+		return false
+	}
+
+	// We don't check TargetPort; that is not relevant for load balancing
+	// TODO: Should we blank it out?  Or just check it anyway?
+
+	return true
 }
 
 func intSlicesEqual(x, y []int) bool {

pkg/cloudprovider/servicecontroller/servicecontroller_test.go

@@ -113,7 +113,7 @@ func TestCreateExternalLoadBalancer(t *testing.T) {
 				t.Errorf("expected one load balancer to be created, got %v", cloud.Balancers)
 			} else if cloud.Balancers[0].Name != controller.loadBalancerName(item.service) ||
 				cloud.Balancers[0].Region != region ||
-				cloud.Balancers[0].Ports[0] != item.service.Spec.Ports[0].Port {
+				cloud.Balancers[0].Ports[0].Port != item.service.Spec.Ports[0].Port {
 				t.Errorf("created load balancer has incorrect parameters: %v", cloud.Balancers[0])
 			}
 			actionFound := false