From 27c0858f43de043f066c4b78c08446a2f184a88b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 28 Oct 2017 19:34:58 +0200 Subject: [PATCH] Fix service update --host-rm not being granular enough Removing a host by `:` should only remove occurences of the host with a matching IP-address, instead of removing all entries for that host. In addition, combining `--host-rm` and `--host-add` for the same host should result in the new host being added. This patch fixes the way the diff is calculated to allow combining removing/adding, and to support entries having both a canonical, and aliases. Aliases cannot be added by the CLI, but are supported in the Service spec, thus should be taken into account: Entries can be removed by either a specific `:` mapping, or by `` alone: - If both IP-address and host-name is provided, only remove the hostname from entries that match the given IP-address. - If only a host-name is provided, remove the hostname from any entry it is part of (either as _canonical_ host-name, or as _alias_). - If, after removing the host-name from an entry, no host-names remain in the entry, the entry itself should be removed. For example, the list of host-entries before processing could look like this: hosts = &[]string{ "127.0.0.2 host3 host1 host2 host4", "127.0.0.1 host1 host4", "127.0.0.3 host1", "127.0.0.1 host1", } Removing `host1` removes every occurrence: hosts = &[]string{ "127.0.0.2 host3 host2 host4", "127.0.0.1 host4", } Whereas removing `host1:127.0.0.1` only remove the host if the IP-address matches: hosts = &[]string{ "127.0.0.2 host3 host1 host2 host4", "127.0.0.1 host4", "127.0.0.3 host1", } Before this patch: $ docker service create --name my-service --host foo:127.0.0.1 --host foo:127.0.0.2 --host foo:127.0.0.3 nginx:alpine $ docker service update --host-rm foo:127.0.0.1 --host-add foo:127.0.0.4 my-service $ docker service inspect --format '{{.Spec.TaskTemplate.ContainerSpec.Hosts}}' my-service [] After this patch is applied: $ docker service create --name my-service --host foo:127.0.0.1 --host foo:127.0.0.2 --host foo:127.0.0.3 nginx:alpine $ docker service update --host-rm foo:127.0.0.1 --host-add foo:127.0.0.5 my-service $ docker service inspect --format '{{.Spec.TaskTemplate.ContainerSpec.Hosts}}' my-service [127.0.0.2 foo 127.0.0.3 foo 127.0.0.4 foo] Signed-off-by: Sebastiaan van Stijn --- cli/command/service/update.go | 96 +++++++++++++++++++++++------- cli/command/service/update_test.go | 56 ++++++++++++++++- 2 files changed, 130 insertions(+), 22 deletions(-) diff --git a/cli/command/service/update.go b/cli/command/service/update.go index ca9752a656..c17a28ef8a 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -963,45 +963,99 @@ func updateReplicas(flags *pflag.FlagSet, serviceMode *swarm.ServiceMode) error return nil } +type hostMapping struct { + IPAddr string + Host string +} + // updateHosts performs a diff between existing host entries, entries to be // removed, and entries to be added. Host entries preserve the order in which they // were added, as the specification mentions that in case multiple entries for a // host exist, the first entry should be used (by default). +// +// Note that, even though unsupported by the the CLI, the service specs format +// allow entries with both a _canonical_ hostname, and one or more aliases +// in an entry (IP-address canonical_hostname [alias ...]) +// +// Entries can be removed by either a specific `:` mapping, +// or by `` alone: +// +// - If both IP-address and host-name is provided, the hostname is removed only +// from entries that match the given IP-address. +// - If only a host-name is provided, the hostname is removed from any entry it +// is part of (either as canonical host-name, or as alias). +// - If, after removing the host-name from an entry, no host-names remain in +// the entry, the entry itself is removed. +// +// For example, the list of host-entries before processing could look like this: +// +// hosts = &[]string{ +// "127.0.0.2 host3 host1 host2 host4", +// "127.0.0.1 host1 host4", +// "127.0.0.3 host1", +// "127.0.0.1 host1", +// } +// +// Removing `host1` removes every occurrence: +// +// hosts = &[]string{ +// "127.0.0.2 host3 host2 host4", +// "127.0.0.1 host4", +// } +// +// Removing `host1:127.0.0.1` on the other hand, only remove the host if the +// IP-address matches: +// +// hosts = &[]string{ +// "127.0.0.2 host3 host1 host2 host4", +// "127.0.0.1 host4", +// "127.0.0.3 host1", +// } func updateHosts(flags *pflag.FlagSet, hosts *[]string) error { - // Combine existing Hosts (in swarmkit format) with the host to add (convert to swarmkit format) - if flags.Changed(flagHostAdd) { - values := convertExtraHostsToSwarmHosts(flags.Lookup(flagHostAdd).Value.(*opts.ListOpts).GetAll()) - *hosts = append(*hosts, values...) - } - // Remove duplicate - *hosts = removeDuplicates(*hosts) - - keysToRemove := make(map[string]struct{}) + var toRemove []hostMapping if flags.Changed(flagHostRemove) { - var empty struct{} extraHostsToRemove := flags.Lookup(flagHostRemove).Value.(*opts.ListOpts).GetAll() for _, entry := range extraHostsToRemove { - key := strings.SplitN(entry, ":", 2)[0] - keysToRemove[key] = empty + v := strings.SplitN(entry, ":", 2) + if len(v) > 1 { + toRemove = append(toRemove, hostMapping{IPAddr: v[1], Host: v[0]}) + } else { + toRemove = append(toRemove, hostMapping{Host: v[0]}) + } } } - newHosts := []string{} + var newHosts []string for _, entry := range *hosts { - // Since this is in swarmkit format, we need to find the key, which is canonical_hostname of: + // Since this is in SwarmKit format, we need to find the key, which is canonical_hostname of: // IP_address canonical_hostname [aliases...] parts := strings.Fields(entry) - if len(parts) > 1 { - key := parts[1] - if _, exists := keysToRemove[key]; !exists { - newHosts = append(newHosts, entry) + if len(parts) == 0 { + continue + } + ip := parts[0] + hostNames := parts[1:] + for _, rm := range toRemove { + if rm.IPAddr != "" && rm.IPAddr != ip { + continue } - } else { - newHosts = append(newHosts, entry) + for i, h := range hostNames { + if h == rm.Host { + hostNames = append(hostNames[:i], hostNames[i+1:]...) + } + } + } + if len(hostNames) > 0 { + newHosts = append(newHosts, fmt.Sprintf("%s %s", ip, strings.Join(hostNames, " "))) } } - *hosts = newHosts + // Append new hosts (in SwarmKit format) + if flags.Changed(flagHostAdd) { + values := convertExtraHostsToSwarmHosts(flags.Lookup(flagHostAdd).Value.(*opts.ListOpts).GetAll()) + newHosts = append(newHosts, values...) + } + *hosts = removeDuplicates(newHosts) return nil } diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index d647093254..e98f279435 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -366,7 +366,7 @@ func TestUpdateHosts(t *testing.T) { assert.ErrorContains(t, flags.Set("host-add", "$example.com$"), `bad format for add-host: "$example.com$"`) hosts := []string{"1.2.3.4 example.com", "4.3.2.1 example.org", "2001:db8:abc8::1 example.net"} - expected := []string{"1.2.3.4 example.com", "4.3.2.1 example.org", "2001:db8:abc8::1 ipv6.net"} + expected := []string{"1.2.3.4 example.com", "4.3.2.1 example.org", "2.2.2.2 example.net", "2001:db8:abc8::1 ipv6.net"} err := updateHosts(flags, &hosts) assert.NilError(t, err) @@ -385,6 +385,60 @@ func TestUpdateHostsPreservesOrder(t *testing.T) { assert.Check(t, is.DeepEqual([]string{"127.0.0.2 foobar", "127.0.0.1 foobar", "127.0.0.3 foobar"}, hosts)) } +func TestUpdateHostsReplaceEntry(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("host-add", "foobar:127.0.0.4") + flags.Set("host-rm", "foobar:127.0.0.2") + + hosts := []string{"127.0.0.2 foobar", "127.0.0.1 foobar", "127.0.0.3 foobar"} + + err := updateHosts(flags, &hosts) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual([]string{"127.0.0.1 foobar", "127.0.0.3 foobar", "127.0.0.4 foobar"}, hosts)) +} + +func TestUpdateHostsRemoveHost(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("host-rm", "host1") + + hosts := []string{"127.0.0.2 host3 host1 host2 host4", "127.0.0.1 host1 host4", "127.0.0.3 host1"} + + err := updateHosts(flags, &hosts) + assert.NilError(t, err) + + // Removing host `host1` should remove the entry from each line it appears in. + // If there are no other hosts in the entry, the entry itself should be removed. + assert.Check(t, is.DeepEqual([]string{"127.0.0.2 host3 host2 host4", "127.0.0.1 host4"}, hosts)) +} + +func TestUpdateHostsRemoveHostIP(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("host-rm", "host1:127.0.0.1") + + hosts := []string{"127.0.0.2 host3 host1 host2 host4", "127.0.0.1 host1 host4", "127.0.0.3 host1", "127.0.0.1 host1"} + + err := updateHosts(flags, &hosts) + assert.NilError(t, err) + + // Removing host `host1` should remove the entry from each line it appears in, + // but only if the IP-address matches. If there are no other hosts in the entry, + // the entry itself should be removed. + assert.Check(t, is.DeepEqual([]string{"127.0.0.2 host3 host1 host2 host4", "127.0.0.1 host4", "127.0.0.3 host1"}, hosts)) +} + +func TestUpdateHostsRemoveAll(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("host-add", "host-three:127.0.0.4") + flags.Set("host-add", "host-one:127.0.0.5") + flags.Set("host-rm", "host-one") + + hosts := []string{"127.0.0.1 host-one", "127.0.0.2 host-two", "127.0.0.3 host-one"} + + err := updateHosts(flags, &hosts) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual([]string{"127.0.0.2 host-two", "127.0.0.4 host-three", "127.0.0.5 host-one"}, hosts)) +} + func TestUpdatePortsRmWithProtocol(t *testing.T) { flags := newUpdateCommand(nil).Flags() flags.Set("publish-add", "8081:81")