Skip to content

[17.06 backport] Fix changing host target port failed #2864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dperny merged 1 commit into from
May 30, 2019
Merged

[17.06 backport] Fix changing host target port failed #2864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions manager/allocator/networkallocator/portallocator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,18 @@ func (pa *portAllocator) isPortsAllocatedOnInit(s *api.Service, onInit bool) boo
}

portStates := allocatedPorts{}
hostTargetPorts := map[uint32]struct{}{}
for _, portState := range s.Endpoint.Ports {
if portState.PublishMode == api.PublishModeIngress {
switch portState.PublishMode {
case api.PublishModeIngress:
portStates.addState(portState)
case api.PublishModeHost:
// build a map of host mode ports we've seen. if in the spec we get
// a host port that's not in the service, then we need to do
// allocation. if we get the same target port but something else
// has changed, then HostPublishPortsNeedUpdate will cover that
// case. see docker/swarmkit#2376
hostTargetPorts[portState.TargetPort] = struct{}{}
}
}

Expand All @@ -344,18 +353,28 @@ func (pa *portAllocator) isPortsAllocatedOnInit(s *api.Service, onInit bool) boo
// 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 portConfig.PublishedPort == 0 && portStates.delState(portConfig) == nil {
return false
}
switch portConfig.PublishMode {
case api.PublishModeIngress:
if portConfig.PublishedPort == 0 && portStates.delState(portConfig) == nil {
return false
}

// If SwarmPort was not defined by user and the func
// is called during allocator initialization state then
// we are not allocated.
if portConfig.PublishedPort == 0 && onInit {
return false
// If SwarmPort was not defined by user and the func
// is called during allocator initialization state then
// we are not allocated.
if portConfig.PublishedPort == 0 && onInit {
return false
}
case api.PublishModeHost:
// check if the target port is already in the port config. if it
// isn't, then it's our problem.
if _, ok := hostTargetPorts[portConfig.TargetPort]; !ok {
return false
}
// NOTE(dperny) there could be a further case where we check if
// there are host ports in the config that aren't in the spec, but
// that's only possible if there's a mismatch in the number of
// ports, which is handled by a length check earlier in the code
}
}

Expand Down
59 changes: 55 additions & 4 deletions manager/allocator/networkallocator/portallocator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,13 +269,15 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
assert.NoError(t, err)

type Data struct {
name string
input *api.Service
expect bool
}

testCases := []Data{
{
// both Endpoint and Spec.Endpoint are nil
name: "NilEndpointAndSpec",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: nil,
Expand All @@ -286,6 +288,7 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
},
{
// non host mode does not impact
name: "NonHostModePort",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand All @@ -305,6 +308,7 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
},
{
// publish mode is different
name: "PublishModeDifferent",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -333,6 +337,7 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
expect: true,
},
{
name: "NothingChanged",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -365,6 +370,7 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
// published port not specified
// we are not in charge of allocating one, for us it
// is as allocated, we need to skip the allocation
name: "PublishPortNotSpecified",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -394,6 +400,7 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
{
// one published port not specified, the other specified
// we are still in charge of allocating one
name: "OnePublishPortSpecifiedButDone",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -436,6 +443,7 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
{
// one published port not specified, the other specified
// we are still in charge of allocating one and we did.
name: "OnePublishPortSpecifiedButDone",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -478,8 +486,10 @@ func TestHostPublishPortsNeedUpdate(t *testing.T) {
},
}
for _, singleTest := range testCases {
actual := pa.hostPublishPortsNeedUpdate(singleTest.input)
assert.Equal(t, singleTest.expect, actual)
t.Run(singleTest.name, func(t *testing.T) {
actual := pa.hostPublishPortsNeedUpdate(singleTest.input)
assert.Equal(t, singleTest.expect, actual)
})
}
}

Expand All @@ -488,13 +498,15 @@ func TestIsPortsAllocated(t *testing.T) {
assert.NoError(t, err)

type Data struct {
name string
input *api.Service
expect bool
}

testCases := []Data{
{
// both Endpoint and Spec.Endpoint are nil
name: "BothNil",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: nil,
Expand All @@ -505,6 +517,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint is non-nil and Spec.Endpoint is nil
name: "NilSpec",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand All @@ -524,6 +537,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint is nil and Spec.Endpoint is non-nil
name: "NilEndpoint",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: nil,
Expand All @@ -543,6 +557,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint and Spec.Endpoint have different length
name: "DifferentLengths",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -577,6 +592,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint and Spec.Endpoint have different TargetPort
name: "DifferentTargetPort",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -605,6 +621,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint and Spec.Endpoint have different PublishedPort
name: "DifferentPublishedPort",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -633,6 +650,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint and Spec.Endpoint are the same and PublishedPort is 0
name: "NotYetAssignedPublishedPort",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -661,6 +679,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint and Spec.Endpoint are the same and PublishedPort is non-0
name: "NonzeroPublishedPort",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -689,6 +708,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint and Spec.Endpoint are the same except PublishedPort, and PublishedPort in Endpoint is non-0
name: "AlreadyAssignedPublishedPort",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -717,6 +737,7 @@ func TestIsPortsAllocated(t *testing.T) {
},
{
// Endpoint and Spec.Endpoint are the same except the ports are in different order
name: "DifferentOrders",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -772,6 +793,7 @@ func TestIsPortsAllocated(t *testing.T) {
{
// Endpoint and Spec.Endpoint have multiple PublishedPort
// See docker/docker#29730
name: "MultiplePublishedPort",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Expand Down Expand Up @@ -826,11 +848,40 @@ func TestIsPortsAllocated(t *testing.T) {
},
expect: true,
},
{
// one published host port is removed and another is added
name: "DifferentTargetPortHostMode",
input: &api.Service{
Spec: api.ServiceSpec{
Endpoint: &api.EndpointSpec{
Ports: []*api.PortConfig{
{
Protocol: api.ProtocolTCP,
TargetPort: 99,
PublishMode: api.PublishModeHost,
},
},
},
},
Endpoint: &api.Endpoint{
Ports: []*api.PortConfig{
{
Protocol: api.ProtocolTCP,
TargetPort: 77,
PublishMode: api.PublishModeHost,
},
},
},
},
expect: false,
},
}

for _, singleTest := range testCases {
expect := pa.isPortsAllocated(singleTest.input)
assert.Equal(t, expect, singleTest.expect)
t.Run(singleTest.name, func(t *testing.T) {
expect := pa.isPortsAllocated(singleTest.input)
assert.Equal(t, expect, singleTest.expect)
})
}
}

Expand Down