Preserve sort-order of extra hosts, and allow duplicate entries

Extra hosts (`extra_hosts` in compose-file, or `--hosts` in services) adds
custom host/ip mappings to the container's `/etc/hosts`.

The current implementation used a `map[string]string{}` as intermediate
storage, and sorted the results alphabetically when converting to a service-spec.

As a result, duplicate hosts were removed, and order of host/ip mappings was not
preserved (in case the compose-file used a list instead of a map).

According to the **host.conf(5)** man page (http://man7.org/linux/man-pages/man5/host.conf.5.html)

    multi  Valid values are on and off.  If set to on, the resolver
      library will return all valid addresses for a host that
      appears in the /etc/hosts file, instead of only the first.
      This is off by default, as it may cause a substantial
      performance loss at sites with large hosts files.

Multiple entries for a host are allowed, and even required for some situations,
for example, to add mappings for IPv4 and IPv6 addreses for a host, as illustrated
by the example hosts file in the **hosts(5)** man page (http://man7.org/linux/man-pages/man5/hosts.5.html):

    # The following lines are desirable for IPv4 capable hosts
    127.0.0.1       localhost

    # 127.0.1.1 is often used for the FQDN of the machine
    127.0.1.1       thishost.mydomain.org  thishost
    192.168.1.10    foo.mydomain.org       foo
    192.168.1.13    bar.mydomain.org       bar
    146.82.138.7    master.debian.org      master
    209.237.226.90  www.opensource.org

    # The following lines are desirable for IPv6 capable hosts
    ::1             localhost ip6-localhost ip6-loopback
    ff02::1         ip6-allnodes
    ff02::2         ip6-allrouters

This patch changes the intermediate storage format to use a `[]string`, and only
sorts entries if the input format in the compose file is a mapping. If the input
format is a list, the original sort-order is preserved.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cli/command/service/update.go

@@ -868,6 +868,10 @@ func updateReplicas(flags *pflag.FlagSet, serviceMode *swarm.ServiceMode) error
 	return nil
 }
 
+// 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).
 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) {
@@ -902,9 +906,6 @@ func updateHosts(flags *pflag.FlagSet, hosts *[]string) error {
 		}
 	}
 
-	// Sort so that result is predictable.
-	sort.Strings(newHosts)
-
 	*hosts = newHosts
 	return nil
 }

cli/command/service/update_test.go

@@ -366,12 +366,23 @@ func TestUpdateHosts(t *testing.T) {
 	testutil.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"}
 
-	updateHosts(flags, &hosts)
-	require.Len(t, hosts, 3)
-	assert.Equal(t, "1.2.3.4 example.com", hosts[0])
-	assert.Equal(t, "2001:db8:abc8::1 ipv6.net", hosts[1])
-	assert.Equal(t, "4.3.2.1 example.org", hosts[2])
+	err := updateHosts(flags, &hosts)
+	assert.NoError(t, err)
+	assert.Equal(t, expected, hosts)
+}
+
+func TestUpdateHostsPreservesOrder(t *testing.T) {
+	flags := newUpdateCommand(nil).Flags()
+	flags.Set("host-add", "foobar:127.0.0.2")
+	flags.Set("host-add", "foobar:127.0.0.1")
+	flags.Set("host-add", "foobar:127.0.0.3")
+
+	hosts := []string{}
+	err := updateHosts(flags, &hosts)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"127.0.0.2 foobar", "127.0.0.1 foobar", "127.0.0.3 foobar"}, hosts)
 }
 
 func TestUpdatePortsRmWithProtocol(t *testing.T) {

cli/compose/convert/service.go

@@ -133,7 +133,7 @@ func Service(
 				Command:         service.Entrypoint,
 				Args:            service.Command,
 				Hostname:        service.Hostname,
-				Hosts:           sortStrings(convertExtraHosts(service.ExtraHosts)),
+				Hosts:           convertExtraHosts(service.ExtraHosts),
 				DNSConfig:       dnsConfig,
 				Healthcheck:     healthcheck,
 				Env:             sortStrings(convertEnvironment(service.Environment)),
@@ -365,10 +365,15 @@ func uint32Ptr(value uint32) *uint32 {
 	return &value
 }
 
-func convertExtraHosts(extraHosts map[string]string) []string {
+// convertExtraHosts converts <host>:<ip> mappings to SwarmKit notation:
+// "IP-address hostname(s)". The original order of mappings is preserved.
+func convertExtraHosts(extraHosts composetypes.HostsList) []string {
 	hosts := []string{}
-	for host, ip := range extraHosts {
-		hosts = append(hosts, fmt.Sprintf("%s %s", ip, host))
+	for _, hostIP := range extraHosts {
+		if v := strings.SplitN(hostIP, ":", 2); len(v) == 2 {
+			// Convert to SwarmKit notation: IP-address hostname(s)
+			hosts = append(hosts, fmt.Sprintf("%s %s", v[1], v[0]))
+		}
 	}
 	return hosts
 }

cli/compose/convert/service_test.go

@@ -57,6 +57,15 @@ func TestConvertEnvironment(t *testing.T) {
 	assert.Equal(t, []string{"foo=bar", "key=value"}, env)
 }
 
+func TestConvertExtraHosts(t *testing.T) {
+	source := composetypes.HostsList{
+		"zulu:127.0.0.2",
+		"alpha:127.0.0.1",
+		"zulu:ff02::1",
+	}
+	assert.Equal(t, []string{"127.0.0.2 zulu", "127.0.0.1 alpha", "ff02::1 zulu"}, convertExtraHosts(source))
+}
+
 func TestConvertResourcesFull(t *testing.T) {
 	source := composetypes.Resources{
 		Limits: &composetypes.Resource{

cli/compose/loader/loader.go

@@ -244,6 +244,7 @@ func createTransformHook() mapstructure.DecodeHookFuncType {
 		reflect.TypeOf(types.MappingWithEquals{}):                transformMappingOrListFunc("=", true),
 		reflect.TypeOf(types.Labels{}):                           transformMappingOrListFunc("=", false),
 		reflect.TypeOf(types.MappingWithColon{}):                 transformMappingOrListFunc(":", false),
+		reflect.TypeOf(types.HostsList{}):                        transformListOrMappingFunc(":", false),
 		reflect.TypeOf(types.ServiceVolumeConfig{}):              transformServiceVolumeConfig,
 		reflect.TypeOf(types.BuildConfig{}):                      transformBuildConfig,
 	}
@@ -647,6 +648,22 @@ func transformMappingOrListFunc(sep string, allowNil bool) func(interface{}) (in
 	}
 }
 
+func transformListOrMappingFunc(sep string, allowNil bool) func(interface{}) (interface{}, error) {
+	return func(data interface{}) (interface{}, error) {
+		return transformListOrMapping(data, sep, allowNil), nil
+	}
+}
+
+func transformListOrMapping(listOrMapping interface{}, sep string, allowNil bool) interface{} {
+	switch value := listOrMapping.(type) {
+	case map[string]interface{}:
+		return toStringList(value, sep, allowNil)
+	case []interface{}:
+		return listOrMapping
+	}
+	panic(errors.Errorf("expected a map or a list, got %T: %#v", listOrMapping, listOrMapping))
+}
+
 func transformMappingOrList(mappingOrList interface{}, sep string, allowNil bool) interface{} {
 	switch value := mappingOrList.(type) {
 	case map[string]interface{}:
@@ -749,3 +766,15 @@ func toString(value interface{}, allowNil bool) interface{} {
 		return ""
 	}
 }
+
+func toStringList(value map[string]interface{}, separator string, allowNil bool) []string {
+	output := []string{}
+	for key, value := range value {
+		if value == nil && !allowNil {
+			continue
+		}
+		output = append(output, fmt.Sprintf("%s%s%s", key, separator, value))
+	}
+	sort.Strings(output)
+	return output
+}

cli/compose/loader/loader_test.go

@@ -916,9 +916,9 @@ func TestFullExample(t *testing.T) {
 			"project_db_1:mysql",
 			"project_db_1:postgresql",
 		},
-		ExtraHosts: map[string]string{
-			"otherhost": "50.31.209.229",
-			"somehost":  "162.242.195.82",
+		ExtraHosts: []string{
+			"somehost:162.242.195.82",
+			"otherhost:50.31.209.229",
 		},
 		HealthCheck: &types.HealthCheckConfig{
 			Test:        types.HealthCheckTest([]string{"CMD-SHELL", "echo \"hello world\""}),
@@ -1362,3 +1362,47 @@ volumes:
 	assert.Len(t, config.Services[0].Volumes, 1)
 	assert.Equal(t, expected, config.Services[0].Volumes[0])
 }
+
+func TestLoadExtraHostsMap(t *testing.T) {
+	config, err := loadYAML(`
+version: "3.2"
+services:
+  web:
+    image: busybox
+    extra_hosts:
+      "zulu": "162.242.195.82"
+      "alpha": "50.31.209.229"
+`)
+	require.NoError(t, err)
+
+	expected := types.HostsList{
+		"alpha:50.31.209.229",
+		"zulu:162.242.195.82",
+	}
+
+	require.Len(t, config.Services, 1)
+	assert.Equal(t, expected, config.Services[0].ExtraHosts)
+}
+
+func TestLoadExtraHostsList(t *testing.T) {
+	config, err := loadYAML(`
+version: "3.2"
+services:
+  web:
+    image: busybox
+    extra_hosts:
+      - "zulu:162.242.195.82"
+      - "alpha:50.31.209.229"
+      - "zulu:ff02::1"
+`)
+	require.NoError(t, err)
+
+	expected := types.HostsList{
+		"zulu:162.242.195.82",
+		"alpha:50.31.209.229",
+		"zulu:ff02::1",
+	}
+
+	require.Len(t, config.Services, 1)
+	assert.Equal(t, expected, config.Services[0].ExtraHosts)
+}

cli/compose/types/types.go

@@ -98,7 +98,7 @@ type ServiceConfig struct {
 	EnvFile         StringList `mapstructure:"env_file"`
 	Expose          StringOrNumberList
 	ExternalLinks   []string  `mapstructure:"external_links"`
-	ExtraHosts      MappingWithColon `mapstructure:"extra_hosts"`
+	ExtraHosts      HostsList `mapstructure:"extra_hosts"`
 	Hostname        string
 	HealthCheck     *HealthCheckConfig
 	Image           string
@@ -162,6 +162,9 @@ type Labels map[string]string
 // 'key: value' strings
 type MappingWithColon map[string]string
 
+// HostsList is a list of colon-separated host-ip mappings
+type HostsList []string
+
 // LoggingConfig the logging configuration for a service
 type LoggingConfig struct {
 	Driver  string