Skip to content

Commit

Permalink
Idempotent service update --publish-add
Browse files Browse the repository at this point in the history
This fix tries to address the issue raised in 25375 where
`service update --publish-add` returns an error if the exact
same value is repeated (idempotent).

This fix use a map to filter out repeated port configs so
that `--publish-add` does not error out.

An integration test has been added.

This fix fixes 25375.

Signed-off-by: Yong Tang <[email protected]>
(cherry picked from commit b487497)
Signed-off-by: Tibor Vass <[email protected]>
  • Loading branch information
yongtang authored and Tibor Vass committed Aug 12, 2016
1 parent bdf7a55 commit f99fd5a
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 9 deletions.
49 changes: 43 additions & 6 deletions api/client/service/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package service

import (
"fmt"
"sort"
"strings"
"time"

Expand Down Expand Up @@ -216,7 +217,9 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
if spec.EndpointSpec == nil {
spec.EndpointSpec = &swarm.EndpointSpec{}
}
updatePorts(flags, &spec.EndpointSpec.Ports)
if err := updatePorts(flags, &spec.EndpointSpec.Ports); err != nil {
return err
}
}

if err := updateLogDriver(flags, &spec.TaskTemplate); err != nil {
Expand Down Expand Up @@ -369,23 +372,54 @@ func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) {
*mounts = newMounts
}

func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) {
type byPortConfig []swarm.PortConfig

func (r byPortConfig) Len() int { return len(r) }
func (r byPortConfig) Swap(i, j int) { r[i], r[j] = r[j], r[i] }
func (r byPortConfig) Less(i, j int) bool {
// We convert PortConfig into `port/protocol`, e.g., `80/tcp`
// In updatePorts we already filter out with map so there is duplicate entries
return portConfigToString(&r[i]) < portConfigToString(&r[j])
}

func portConfigToString(portConfig *swarm.PortConfig) string {
protocol := portConfig.Protocol
if protocol == "" {
protocol = "tcp"
}
return fmt.Sprintf("%v/%s", portConfig.PublishedPort, protocol)
}

func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) error {
// The key of the map is `port/protocol`, e.g., `80/tcp`
portSet := map[string]swarm.PortConfig{}
// Check to see if there are any conflict in flags.
if flags.Changed(flagPublishAdd) {
values := flags.Lookup(flagPublishAdd).Value.(*opts.ListOpts).GetAll()
ports, portBindings, _ := nat.ParsePortSpecs(values)

for port := range ports {
*portConfig = append(*portConfig, convertPortToPortConfig(port, portBindings)...)
newConfigs := convertPortToPortConfig(port, portBindings)
for _, entry := range newConfigs {
if v, ok := portSet[portConfigToString(&entry)]; ok && v != entry {
return fmt.Errorf("conflicting port mapping between %v:%v/%s and %v:%v/%s", entry.PublishedPort, entry.TargetPort, entry.Protocol, v.PublishedPort, v.TargetPort, v.Protocol)
}
portSet[portConfigToString(&entry)] = entry
}
}
}

if !flags.Changed(flagPublishRemove) {
return
// Override previous PortConfig in service if there is any duplicate
for _, entry := range *portConfig {
if _, ok := portSet[portConfigToString(&entry)]; !ok {
portSet[portConfigToString(&entry)] = entry
}
}

toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.ListOpts).GetAll()
newPorts := []swarm.PortConfig{}
portLoop:
for _, port := range *portConfig {
for _, port := range portSet {
for _, rawTargetPort := range toRemove {
targetPort := nat.Port(rawTargetPort)
if equalPort(targetPort, port) {
Expand All @@ -394,7 +428,10 @@ portLoop:
}
newPorts = append(newPorts, port)
}
// Sort the PortConfig to avoid unnecessary updates
sort.Sort(byPortConfig(newPorts))
*portConfig = newPorts
return nil
}

func equalPort(targetPort nat.Port, port swarm.PortConfig) bool {
Expand Down
54 changes: 51 additions & 3 deletions api/client/service/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,56 @@ func TestUpdatePorts(t *testing.T) {
{TargetPort: 555},
}

updatePorts(flags, &portConfigs)
err := updatePorts(flags, &portConfigs)
assert.Equal(t, err, nil)
assert.Equal(t, len(portConfigs), 2)
assert.Equal(t, portConfigs[0].TargetPort, uint32(555))
assert.Equal(t, portConfigs[1].TargetPort, uint32(1000))
// Do a sort to have the order (might have changed by map)
targetPorts := []int{int(portConfigs[0].TargetPort), int(portConfigs[1].TargetPort)}
sort.Ints(targetPorts)
assert.Equal(t, targetPorts[0], 555)
assert.Equal(t, targetPorts[1], 1000)
}

func TestUpdatePortsDuplicateEntries(t *testing.T) {
// Test case for #25375
flags := newUpdateCommand(nil).Flags()
flags.Set("publish-add", "80:80")

portConfigs := []swarm.PortConfig{
{TargetPort: 80, PublishedPort: 80},
}

err := updatePorts(flags, &portConfigs)
assert.Equal(t, err, nil)
assert.Equal(t, len(portConfigs), 1)
assert.Equal(t, portConfigs[0].TargetPort, uint32(80))
}

func TestUpdatePortsDuplicateKeys(t *testing.T) {
// Test case for #25375
flags := newUpdateCommand(nil).Flags()
flags.Set("publish-add", "80:20")

portConfigs := []swarm.PortConfig{
{TargetPort: 80, PublishedPort: 80},
}

err := updatePorts(flags, &portConfigs)
assert.Equal(t, err, nil)
assert.Equal(t, len(portConfigs), 1)
assert.Equal(t, portConfigs[0].TargetPort, uint32(20))
}

func TestUpdatePortsConflictingFlags(t *testing.T) {
// Test case for #25375
flags := newUpdateCommand(nil).Flags()
flags.Set("publish-add", "80:80")
flags.Set("publish-add", "80:20")

portConfigs := []swarm.PortConfig{
{TargetPort: 80, PublishedPort: 80},
}

err := updatePorts(flags, &portConfigs)
assert.Error(t, err, "conflicting port mapping")
}
26 changes: 26 additions & 0 deletions integration-cli/docker_cli_swarm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,29 @@ func (s *DockerSwarmSuite) TestSwarmNodeTaskListFilter(c *check.C) {
c.Assert(out, checker.Not(checker.Contains), name+".2")
c.Assert(out, checker.Not(checker.Contains), name+".3")
}

// Test case for #25375
func (s *DockerSwarmSuite) TestSwarmPublishAdd(c *check.C) {
d := s.AddDaemon(c, true, true)

name := "top"
out, err := d.Cmd("service", "create", "--name", name, "--label", "x=y", "busybox", "top")
c.Assert(err, checker.IsNil)
c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")

out, err = d.Cmd("service", "update", "--publish-add", "80:80", name)
c.Assert(err, checker.IsNil)

out, err = d.Cmd("service", "update", "--publish-add", "80:80", name)
c.Assert(err, checker.IsNil)

out, err = d.Cmd("service", "update", "--publish-add", "80:80", "--publish-add", "80:20", name)
c.Assert(err, checker.NotNil)

out, err = d.Cmd("service", "update", "--publish-add", "80:20", name)
c.Assert(err, checker.IsNil)

out, err = d.Cmd("service", "inspect", "--format", "{{ .Spec.EndpointSpec.Ports }}", name)
c.Assert(err, checker.IsNil)
c.Assert(strings.TrimSpace(out), checker.Equals, "[{ tcp 20 80}]")
}

0 comments on commit f99fd5a

Please sign in to comment.