Fix issues of multiple published ports mapping to the same target port

This fix tries to address the issue raised in moby/moby#29370
where a service with multiple published ports mapping to the same target
port (e.g., `--publish 5000:80 --publish 5001:80`) can't be allocated.

The reason for the issue is that, `getPortConfigKey` is used for both
allocated ports and configured (may or may not be allocated) ports.
However, `getPortConfigKey` will not take into consideration the
`PublishedPort` field, which actually could be different for different
allocated ports.

This fix saves a map of `portKey:portNum:portState`,  instead of currently
used `portKey:portState` so that multiple published ports could be processed.

A test case has been added in the unit test. The newly added test case
will only work with this PR.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

manager/allocator/networkallocator/portallocator.go

@@ -91,13 +91,17 @@ func reconcilePortConfigs(s *api.Service) []*api.PortConfig {
 		return s.Spec.Endpoint.Ports
 	}
 
-	allocatedPorts := make(map[api.PortConfig]*api.PortConfig)
+	allocatedPorts := make(map[api.PortConfig]map[uint32]*api.PortConfig)
 	for _, portState := range s.Endpoint.Ports {
 		if portState.PublishMode != api.PublishModeIngress {
 			continue
 		}
 
-		allocatedPorts[getPortConfigKey(portState)] = portState
+		portKey := getPortConfigKey(portState)
+		if _, ok := allocatedPorts[portKey]; !ok {
+			allocatedPorts[portKey] = make(map[uint32]*api.PortConfig)
+		}
+		allocatedPorts[portKey][portState.PublishedPort] = portState
 	}
 
 	var portConfigs []*api.PortConfig
@@ -109,18 +113,27 @@ func reconcilePortConfigs(s *api.Service) []*api.PortConfig {
 			continue
 		}
 
-		portState, ok := allocatedPorts[getPortConfigKey(portConfig)]
-
 		// If the portConfig is exactly the same as portState
 		// except if SwarmPort is not user-define then prefer
 		// portState to ensure sticky allocation of the same
 		// port that was allocated before.
-		if ok && portConfig.Name == portState.Name &&
-			portConfig.TargetPort == portState.TargetPort &&
-			portConfig.Protocol == portState.Protocol &&
-			portConfig.PublishedPort == 0 {
-			portConfigs = append(portConfigs, portState)
-			continue
+		if portConfig.PublishedPort == 0 {
+			portKey := getPortConfigKey(portConfig)
+
+			if portStateMap, ok := allocatedPorts[portKey]; ok {
+				// Find the non-zero port for portState
+				var portState *api.PortConfig
+				for publishedPort, v := range portStateMap {
+					if publishedPort != 0 {
+						portState = v
+						break
+					}
+				}
+				if portState != nil {
+					portConfigs = append(portConfigs, portState)
+					continue
+				}
+			}
 		}
 
 		// For all other cases prefer the portConfig
@@ -213,13 +226,17 @@ func (pa *portAllocator) isPortsAllocated(s *api.Service) bool {
 		return false
 	}
 
-	allocatedPorts := make(map[api.PortConfig]*api.PortConfig)
+	allocatedPorts := make(map[api.PortConfig]map[uint32]*api.PortConfig)
 	for _, portState := range s.Endpoint.Ports {
 		if portState.PublishMode != api.PublishModeIngress {
 			continue
 		}
 
-		allocatedPorts[getPortConfigKey(portState)] = portState
+		portKey := getPortConfigKey(portState)
+		if _, ok := allocatedPorts[portKey]; !ok {
+			allocatedPorts[portKey] = make(map[uint32]*api.PortConfig)
+		}
+		allocatedPorts[portKey][portState.PublishedPort] = portState
 	}
 
 	for _, portConfig := range s.Spec.Endpoint.Ports {
@@ -228,26 +245,37 @@ func (pa *portAllocator) isPortsAllocated(s *api.Service) bool {
 			continue
 		}
 
-		portState, ok := allocatedPorts[getPortConfigKey(portConfig)]
+		portKey := getPortConfigKey(portConfig)
+
+		portStateMap, ok := allocatedPorts[portKey]
 
 		// If name, port, protocol values don't match then we
 		// are not allocated.
 		if !ok {
 			return false
 		}
 
-		// If SwarmPort was user defined but the port state
-		// SwarmPort doesn't match we are not allocated.
-		if portConfig.PublishedPort != portState.PublishedPort &&
-			portConfig.PublishedPort != 0 {
-			return false
-		}
+		if portConfig.PublishedPort != 0 {
+			// If SwarmPort was user defined but the port state
+			// SwarmPort doesn't match we are not allocated.
+			_, ok := portStateMap[portConfig.PublishedPort]
 
-		// If SwarmPort was not defined by user and port state
-		// is not initialized with a valid SwarmPort value then
-		// we are not allocated.
-		if portConfig.PublishedPort == 0 && portState.PublishedPort == 0 {
-			return false
+			if !ok {
+				return false
+			}
+		} else {
+			// If PublishedPort == 0 and we don't have non-zero port
+			// then we are not allocated
+			ok := false
+			for publishedPort := range portStateMap {
+				if publishedPort != 0 {
+					ok = true
+					break
+				}
+			}
+			if !ok {
+				return false
+			}
 		}
 	}
 

manager/allocator/networkallocator/portallocator_test.go

@@ -550,6 +550,43 @@ func TestIsPortsAllocated(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			// Endpoint and Spec.Endpoint have multiple PublishedPort
+			// See docker/docker#29730
+			input: &api.Service{
+				Spec: api.ServiceSpec{
+					Endpoint: &api.EndpointSpec{
+						Ports: []*api.PortConfig{
+							{
+								Protocol:      api.ProtocolTCP,
+								TargetPort:    80,
+								PublishedPort: 5000,
+							},
+							{
+								Protocol:      api.ProtocolTCP,
+								TargetPort:    80,
+								PublishedPort: 5001,
+							},
+						},
+					},
+				},
+				Endpoint: &api.Endpoint{
+					Ports: []*api.PortConfig{
+						{
+							Protocol:      api.ProtocolTCP,
+							TargetPort:    80,
+							PublishedPort: 5000,
+						},
+						{
+							Protocol:      api.ProtocolTCP,
+							TargetPort:    80,
+							PublishedPort: 5001,
+						},
+					},
+				},
+			},
+			expect: true,
+		},
 	}
 
 	for _, singleTest := range testCases {