diff --git a/manager/allocator/networkallocator/portallocator.go b/manager/allocator/networkallocator/portallocator.go index 73a51c5b80..a070c65c06 100644 --- a/manager/allocator/networkallocator/portallocator.go +++ b/manager/allocator/networkallocator/portallocator.go @@ -156,40 +156,53 @@ func reconcilePortConfigs(s *api.Service) []*api.PortConfig { return s.Spec.Endpoint.Ports } - allocatedPorts := make(map[api.PortConfig]*api.PortConfig) + portStates := allocatedPorts{} for _, portState := range s.Endpoint.Ports { - if portState.PublishMode != api.PublishModeIngress { - continue + if portState.PublishMode == api.PublishModeIngress { + portStates.addState(portState) } - - allocatedPorts[getPortConfigKey(portState)] = portState } var portConfigs []*api.PortConfig + + // Process the portConfig with portConfig.PublishMode != api.PublishModeIngress + // and PublishedPort != 0 (high priority) for _, portConfig := range s.Spec.Endpoint.Ports { - // If the PublishMode is not Ingress simply pick up - // the port config. if portConfig.PublishMode != api.PublishModeIngress { + // If the PublishMode is not Ingress simply pick up the port config. portConfigs = append(portConfigs, portConfig) - continue - } + } else if portConfig.PublishedPort != 0 { + // Otherwise we only process PublishedPort != 0 in this round - 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 + // Remove record from portState + portStates.delState(portConfig) + + // For PublishedPort != 0 prefer the portConfig + portConfigs = append(portConfigs, portConfig) } + } + + // Iterate portConfigs with PublishedPort == 0 (low priority) + for _, portConfig := range s.Spec.Endpoint.Ports { + // Ignore ports which are not PublishModeIngress (already processed) + // And we only process PublishedPort == 0 in this round + // So the following: + // `portConfig.PublishMode == api.PublishModeIngress && portConfig.PublishedPort == 0` + if portConfig.PublishMode == api.PublishModeIngress && portConfig.PublishedPort == 0 { + // 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. + + // Remove record from portState + if portState := portStates.delState(portConfig); portState != nil { + portConfigs = append(portConfigs, portState) + continue + } - // For all other cases prefer the portConfig - portConfigs = append(portConfigs, portConfig) + // For all other cases prefer the portConfig + portConfigs = append(portConfigs, portConfig) + } } return portConfigs @@ -306,40 +319,31 @@ func (pa *portAllocator) isPortsAllocated(s *api.Service) bool { return false } - allocatedPorts := make(map[api.PortConfig]*api.PortConfig) + portStates := allocatedPorts{} for _, portState := range s.Endpoint.Ports { - if portState.PublishMode != api.PublishModeIngress { - continue + if portState.PublishMode == api.PublishModeIngress { + portStates.addState(portState) } - - allocatedPorts[getPortConfigKey(portState)] = portState } + // Iterate portConfigs with PublishedPort != 0 (high priority) for _, portConfig := range s.Spec.Endpoint.Ports { // Ignore ports which are not PublishModeIngress if portConfig.PublishMode != api.PublishModeIngress { continue } - - portState, ok := allocatedPorts[getPortConfigKey(portConfig)] - - // If name, port, protocol values don't match then we - // are not allocated. - if !ok { + if portConfig.PublishedPort != 0 && portStates.delState(portConfig) == nil { 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 + // Iterate portConfigs with PublishedPort == 0 (low priority) + for _, portConfig := range s.Spec.Endpoint.Ports { + // Ignore ports which are not PublishModeIngress + if portConfig.PublishMode != api.PublishModeIngress { + continue } - - // 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 { + if portConfig.PublishedPort == 0 && portStates.delState(portConfig) == nil { return false } } diff --git a/manager/allocator/networkallocator/portallocator_test.go b/manager/allocator/networkallocator/portallocator_test.go index 69105f5be9..816d5c7164 100644 --- a/manager/allocator/networkallocator/portallocator_test.go +++ b/manager/allocator/networkallocator/portallocator_test.go @@ -769,6 +769,63 @@ 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, + }, + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 0, + }, + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 0, + }, + }, + }, + }, + Endpoint: &api.Endpoint{ + Ports: []*api.PortConfig{ + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 5000, + }, + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 5001, + }, + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 30000, + }, + { + Protocol: api.ProtocolTCP, + TargetPort: 80, + PublishedPort: 30001, + }, + }, + }, + }, + expect: true, + }, } for _, singleTest := range testCases {