handle errors when ingress ports are not available (#1136)

cmd/service-controller/controller.go

@@ -612,7 +612,10 @@ func (c *Controller) updateBridgeConfig(name string) error {
 		if errWithProfiles != nil {
 			return fmt.Errorf("error checking SSL profiles before adding the bindings: %s", errWithProfiles)
 		}
-		desiredBridges := service.RequiredBridges(c.bindings, c.origin)
+		desiredBridges, err := service.RequiredBridges(c.bindings, c.origin)
+		if err != nil {
+			return fmt.Errorf("Error creating bridges: %s", err)
+		}
 		update, err := desiredBridges.UpdateConfigMap(cm)
 		if err != nil {
 			return fmt.Errorf("Error updating %s: %s", cm.ObjectMeta.Name, err)
@@ -787,7 +790,11 @@ func (c *Controller) processNextEvent() bool {
 							event.Recordf(ServiceControllerError, "Could not parse service definition for %s: %s", k, err)
 							continue
 						}
-						c.updateServiceBindings(si, portAllocations)
+						err = c.updateServiceBindings(si, portAllocations)
+						if err != nil {
+							event.Recordf(ServiceControllerError, "Could not update service bindings correctly for %s: %s", k, err)
+						}
+
 						if bindings, ok := c.bindings[si.Address]; ok {
 							updated = append(updated, bindings)
 						}

pkg/service/bindings.go

@@ -421,13 +421,18 @@ func (sb *ServiceBindings) Stop() {
 	}
 }
 
-func (sb *ServiceBindings) updateBridgeConfiguration(siteId string, bridges *qdr.BridgeConfig) {
+func (sb *ServiceBindings) updateBridgeConfiguration(siteId string, bridges *qdr.BridgeConfig) error {
 	if sb.headless == nil && !sb.RequiresExternalBridge() {
-		addIngressBridge(sb, siteId, bridges)
+		_, err := addIngressBridge(sb, siteId, bridges)
+		if err != nil {
+			return err
+		}
 		for _, eb := range sb.targets {
 			eb.updateBridgeConfiguration(sb, siteId, bridges)
 		}
 	} // headless proxies are not specified through the main bridge configuration
+
+	return nil
 }
 
 func (eb *EgressBindings) stop() {
@@ -543,6 +548,10 @@ func addEgressBridge(protocol string, host string, port map[int]int, address str
 }
 
 func addIngressBridge(sb *ServiceBindings, siteId string, bridges *qdr.BridgeConfig) (bool, error) {
+	if len(sb.publicPorts) > len(sb.ingressPorts) {
+		return false, fmt.Errorf("there are not enough ingress ports available for service %s", sb.Address)
+	}
+
 	for i := 0; i < len(sb.publicPorts); i++ {
 		pPort := sb.publicPorts[i]
 		iPort := sb.ingressPorts[i]
@@ -601,16 +610,19 @@ func addIngressBridge(sb *ServiceBindings, siteId string, bridges *qdr.BridgeCon
 			bridges.AddTcpListener(tcpListener)
 
 		default:
-			return false, fmt.Errorf("Unrecognised protocol for service %s: %s", sb.Address, sb.protocol)
+			return false, nil
 		}
 	}
 	return true, nil
 }
 
-func RequiredBridges(services map[string]*ServiceBindings, siteId string) *qdr.BridgeConfig {
+func RequiredBridges(services map[string]*ServiceBindings, siteId string) (*qdr.BridgeConfig, error) {
 	bridges := newBridgeConfiguration()
 	for _, service := range services {
-		service.updateBridgeConfiguration(siteId, bridges)
+		err := service.updateBridgeConfiguration(siteId, bridges)
+		if err != nil {
+			return nil, err
+		}
 	}
-	return bridges
+	return bridges, nil
 }

pkg/service/bindings_test.go

@@ -1,6 +1,7 @@
 package service
 
 import (
+	"fmt"
 	"log"
 	"reflect"
 	"testing"
@@ -1403,7 +1404,7 @@ func TestRequiredBridges(t *testing.T) {
 			for _, svc := range s.services {
 				bindings[svc.Address] = NewServiceBindings(svc, svc.Ports, context)
 			}
-			actual := RequiredBridges(bindings, s.siteId)
+			actual, _ := RequiredBridges(bindings, s.siteId)
 			assert.Assert(t, reflect.DeepEqual(actual.TcpListeners, s.expected.TcpListeners), "Expected %v got %v", s.expected.TcpListeners, actual.TcpListeners)
 			assert.Assert(t, reflect.DeepEqual(actual.HttpListeners, s.expected.HttpListeners), "Expected %v got %v", s.expected.HttpListeners, actual.HttpListeners)
 			assert.Assert(t, reflect.DeepEqual(actual.TcpConnectors, s.expected.TcpConnectors), "Expected %v got %v", s.expected.TcpConnectors, actual.TcpConnectors)
@@ -1412,6 +1413,57 @@ func TestRequiredBridges(t *testing.T) {
 	}
 }
 
+func TestRequiredBridgesIngresNotAvailable(t *testing.T) {
+	type scenario struct {
+		name     string
+		services []types.ServiceInterface
+		siteId   string
+		expected error
+	}
+
+	scenarios := []scenario{
+		{
+			name: "problem-with-ingress",
+			services: []types.ServiceInterface{
+				{
+					Address:  "testing",
+					Protocol: "http2",
+					Ports:    []int{8080},
+					Targets: []types.ServiceInterfaceTarget{
+						{
+							Name:        "target1",
+							Selector:    "app=foo",
+							TargetPorts: map[int]int{8080: 8888},
+							Service:     "",
+						},
+					},
+				},
+			},
+			siteId:   "test-ingress-issues",
+			expected: fmt.Errorf("there are not enough ingress ports available for service testing"),
+		},
+	}
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			context := &DummyServiceBindingContext{
+				hosts: map[string][]string{
+					"app=foo":    {"foo-pod-1"},
+					"app=bar":    {"bar-pod-1", "bar-pod-2"},
+					"app=broken": {""},
+				},
+			}
+			bindings := map[string]*ServiceBindings{}
+			for _, svc := range s.services {
+				sb := NewServiceBindings(svc, svc.Ports, context)
+				sb.ingressPorts = nil
+				bindings[svc.Address] = sb
+			}
+			_, err := RequiredBridges(bindings, s.siteId)
+			assert.Equal(t, err.Error(), "there are not enough ingress ports available for service testing")
+		})
+	}
+}
+
 func TestFindLocalTarget(t *testing.T) {
 	type scenario struct {
 		name                string