Fix environment resolving.

Load from env should only happen if the value is unset.
Extract a buildEnvironment function and revert some changes to tests.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

command/stack/deploy_composefile.go

@@ -15,6 +15,7 @@ import (
 	composetypes "github.com/docker/docker/cli/compose/types"
 	apiclient "github.com/docker/docker/client"
 	dockerclient "github.com/docker/docker/client"
+	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 )
 
@@ -115,17 +116,24 @@ func getConfigDetails(opts deployOptions) (composetypes.ConfigDetails, error) {
 	}
 	// TODO: support multiple files
 	details.ConfigFiles = []composetypes.ConfigFile{*configFile}
-	env := os.Environ()
-	details.Environment = make(map[string]string, len(env))
+	details.Environment, err = buildEnvironment(os.Environ())
+	if err != nil {
+		return details, err
+	}
+	return details, nil
+}
+
+func buildEnvironment(env []string) (map[string]string, error) {
+	result := make(map[string]string, len(env))
 	for _, s := range env {
 		// if value is empty, s is like "K=", not "K".
 		if !strings.Contains(s, "=") {
-			return details, fmt.Errorf("unexpected environment %q", s)
+			return result, errors.Errorf("unexpected environment %q", s)
 		}
 		kv := strings.SplitN(s, "=", 2)
-		details.Environment[kv[0]] = kv[1]
+		result[kv[0]] = kv[1]
 	}
-	return details, nil
+	return result, nil
 }
 
 func getConfigFile(filename string) (*composetypes.ConfigFile, error) {

compose/convert/service.go

@@ -393,11 +393,16 @@ func convertEndpointSpec(endpointMode string, source []composetypes.ServicePortC
 	}, nil
 }
 
-func convertEnvironment(source map[string]string) []string {
+func convertEnvironment(source map[string]*string) []string {
 	var output []string
 
 	for name, value := range source {
-		output = append(output, fmt.Sprintf("%s=%s", name, value))
+		switch value {
+		case nil:
+			output = append(output, name)
+		default:
+			output = append(output, fmt.Sprintf("%s=%s", name, *value))
+		}
 	}
 
 	return output

compose/convert/service_test.go

@@ -43,10 +43,14 @@ func TestConvertRestartPolicyFromFailure(t *testing.T) {
 	assert.DeepEqual(t, policy, expected)
 }
 
+func strPtr(val string) *string {
+	return &val
+}
+
 func TestConvertEnvironment(t *testing.T) {
-	source := map[string]string{
-		"foo": "bar",
-		"key": "value",
+	source := map[string]*string{
+		"foo": strPtr("bar"),
+		"key": strPtr("value"),
 	}
 	env := convertEnvironment(source)
 	sort.Strings(env)

compose/loader/example1.env

@@ -1,8 +1,8 @@
 # passed through
-FOO=1
+FOO=foo_from_env_file
 
 # overridden in example2.env
-BAR=1
+BAR=bar_from_env_file
 
 # overridden in full-example.yml
-BAZ=1
+BAZ=baz_from_env_file

compose/loader/example2.env

@@ -1,4 +1,4 @@
-BAR=2
+BAR=bar_from_env_file_2
 
 # overridden in configDetails.Environment
-QUX=1
+QUX=quz_from_env_file_2

compose/loader/full-example.yml

@@ -77,10 +77,8 @@ services:
     # Mapping values can be strings, numbers or null
     # Booleans are not allowed - must be quoted
     environment:
-      RACK_ENV: development
-      SHOW: 'true'
-      SESSION_SECRET:
-      BAZ: 3
+      BAZ: baz_from_service_def
+      QUX:
     # environment:
     #   - RACK_ENV=development
     #   - SHOW=true

compose/loader/loader.go

@@ -253,9 +253,11 @@ func transformHook(
 	case reflect.TypeOf(map[string]*types.ServiceNetworkConfig{}):
 		return transformServiceNetworkMap(data)
 	case reflect.TypeOf(types.MappingWithEquals{}):
-		return transformMappingOrList(data, "="), nil
+		return transformMappingOrList(data, "=", true), nil
+	case reflect.TypeOf(types.Labels{}):
+		return transformMappingOrList(data, "=", false), nil
 	case reflect.TypeOf(types.MappingWithColon{}):
-		return transformMappingOrList(data, ":"), nil
+		return transformMappingOrList(data, ":", false), nil
 	case reflect.TypeOf(types.ServiceVolumeConfig{}):
 		return transformServiceVolumeConfig(data)
 	}
@@ -317,7 +319,7 @@ func LoadServices(servicesDict types.Dict, workingDir string, lookupEnv template
 	var services []types.ServiceConfig
 
 	for name, serviceDef := range servicesDict {
-		serviceConfig, err := loadService(name, serviceDef.(types.Dict), workingDir, lookupEnv)
+		serviceConfig, err := LoadService(name, serviceDef.(types.Dict), workingDir, lookupEnv)
 		if err != nil {
 			return nil, err
 		}
@@ -344,25 +346,20 @@ func LoadService(name string, serviceDict types.Dict, workingDir string, lookupE
 	return serviceConfig, nil
 }
 
-func updateEnvironment(environment map[string]string, vars map[string]string, lookupEnv template.Mapping) map[string]string {
-	result := make(map[string]string, len(environment))
-	for k, v := range environment {
-		result[k]=v
-	}
+func updateEnvironment(environment map[string]*string, vars map[string]*string, lookupEnv template.Mapping) {
 	for k, v := range vars {
 		interpolatedV, ok := lookupEnv(k)
-		if ok {
+		if (v == nil || *v == "") && ok {
 			// lookupEnv is prioritized over vars
-			result[k] = interpolatedV
+			environment[k] = &interpolatedV
 		} else {
-			result[k] = v
+			environment[k] = v
 		}
 	}
-	return result
 }
 
 func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, lookupEnv template.Mapping) error {
-	environment := make(map[string]string)
+	environment := make(map[string]*string)
 
 	if len(serviceConfig.EnvFile) > 0 {
 		var envVars []string
@@ -375,12 +372,12 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, l
 			}
 			envVars = append(envVars, fileVars...)
 		}
-		environment = updateEnvironment(environment,
-			runconfigopts.ConvertKVStringsToMap(envVars), lookupEnv)
+		updateEnvironment(environment,
+			runconfigopts.ConvertKVStringsToMapWithNil(envVars), lookupEnv)
 	}
 
-	serviceConfig.Environment = updateEnvironment(environment,
-		serviceConfig.Environment, lookupEnv)
+	updateEnvironment(environment, serviceConfig.Environment, lookupEnv)
+	serviceConfig.Environment = environment
 	return nil
 }
 
@@ -497,9 +494,9 @@ func absPath(workingDir string, filepath string) string {
 func transformMapStringString(data interface{}) (interface{}, error) {
 	switch value := data.(type) {
 	case map[string]interface{}:
-		return toMapStringString(value), nil
+		return toMapStringString(value, false), nil
 	case types.Dict:
-		return toMapStringString(value), nil
+		return toMapStringString(value, false), nil
 	case map[string]string:
 		return value, nil
 	default:
@@ -613,23 +610,27 @@ func transformStringList(data interface{}) (interface{}, error) {
 	}
 }
 
-func transformMappingOrList(mappingOrList interface{}, sep string) map[string]string {
-	if mapping, ok := mappingOrList.(types.Dict); ok {
-		return toMapStringString(mapping)
-	}
-	if list, ok := mappingOrList.([]interface{}); ok {
-		result := make(map[string]string)
-		for _, value := range list {
+func transformMappingOrList(mappingOrList interface{}, sep string, allowNil bool) interface{} {
+	switch value := mappingOrList.(type) {
+	case types.Dict:
+		return toMapStringString(value, allowNil)
+	case ([]interface{}):
+		result := make(map[string]interface{})
+		for _, value := range value {
 			parts := strings.SplitN(value.(string), sep, 2)
-			if len(parts) == 1 {
-				result[parts[0]] = ""
-			} else {
-				result[parts[0]] = parts[1]
+			key := parts[0]
+			switch {
+			case len(parts) == 1 && allowNil:
+				result[key] = nil
+			case len(parts) == 1 && !allowNil:
+				result[key] = ""
+			default:
+				result[key] = parts[1]
 			}
 		}
 		return result
 	}
-	panic(fmt.Errorf("expected a map or a slice, got: %#v", mappingOrList))
+	panic(fmt.Errorf("expected a map or a list, got %T: %#v", mappingOrList, mappingOrList))
 }
 
 func transformShellCommand(value interface{}) (interface{}, error) {
@@ -693,17 +694,21 @@ func toServicePortConfigs(value string) ([]interface{}, error) {
 	return portConfigs, nil
 }
 
-func toMapStringString(value map[string]interface{}) map[string]string {
-	output := make(map[string]string)
+func toMapStringString(value map[string]interface{}, allowNil bool) map[string]interface{} {
+	output := make(map[string]interface{})
 	for key, value := range value {
-		output[key] = toString(value)
+		output[key] = toString(value, allowNil)
 	}
 	return output
 }
 
-func toString(value interface{}) string {
-	if value == nil {
+func toString(value interface{}, allowNil bool) interface{} {
+	switch {
+	case value != nil:
+		return fmt.Sprint(value)
+	case allowNil:
+		return nil
+	default:
 		return ""
 	}
-	return fmt.Sprint(value)
 }

compose/loader/loader_test.go

@@ -27,6 +27,19 @@ func buildConfigDetails(source types.Dict, env map[string]string) types.ConfigDe
 	}
 }
 
+func loadYAML(yaml string) (*types.Config, error) {
+	return loadYAMLWithEnv(yaml, nil)
+}
+
+func loadYAMLWithEnv(yaml string, env map[string]string) (*types.Config, error) {
+	dict, err := ParseYAML([]byte(yaml))
+	if err != nil {
+		return nil, err
+	}
+
+	return Load(buildConfigDetails(dict, env))
+}
+
 var sampleYAML = `
 version: "3"
 services:
@@ -98,12 +111,16 @@ var sampleDict = types.Dict{
 	},
 }
 
+func strPtr(val string) *string {
+	return &val
+}
+
 var sampleConfig = types.Config{
 	Services: []types.ServiceConfig{
 		{
 			Name:        "foo",
 			Image:       "busybox",
-			Environment: map[string]string{},
+			Environment: map[string]*string{},
 			Networks: map[string]*types.ServiceNetworkConfig{
 				"with_me": nil,
 			},
@@ -111,7 +128,7 @@ var sampleConfig = types.Config{
 		{
 			Name:        "bar",
 			Image:       "busybox",
-			Environment: map[string]string{"FOO": "1"},
+			Environment: map[string]*string{"FOO": strPtr("1")},
 			Networks: map[string]*types.ServiceNetworkConfig{
 				"with_ipam": nil,
 			},
@@ -173,7 +190,7 @@ services:
 secrets:
   super:
     external: true
-`, nil)
+`)
 	if !assert.NoError(t, err) {
 		return
 	}
@@ -182,7 +199,7 @@ secrets:
 }
 
 func TestParseAndLoad(t *testing.T) {
-	actual, err := loadYAML(sampleYAML, nil)
+	actual, err := loadYAML(sampleYAML)
 	if !assert.NoError(t, err) {
 		return
 	}
@@ -192,15 +209,15 @@ func TestParseAndLoad(t *testing.T) {
 }
 
 func TestInvalidTopLevelObjectType(t *testing.T) {
-	_, err := loadYAML("1", nil)
+	_, err := loadYAML("1")
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Top-level object must be a mapping")
 
-	_, err = loadYAML("\"hello\"", nil)
+	_, err = loadYAML("\"hello\"")
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Top-level object must be a mapping")
 
-	_, err = loadYAML("[\"hello\"]", nil)
+	_, err = loadYAML("[\"hello\"]")
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Top-level object must be a mapping")
 }
@@ -211,7 +228,7 @@ version: "3"
 123:
   foo:
     image: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Non-string key at top level: 123")
 
@@ -222,7 +239,7 @@ services:
     image: busybox
   123:
     image: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Non-string key in services: 123")
 
@@ -236,7 +253,7 @@ networks:
     ipam:
       config:
         - 123: oh dear
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Non-string key in networks.default.ipam.config[0]: 123")
 
@@ -247,7 +264,7 @@ services:
     image: busybox
     environment:
       1: FOO
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "Non-string key in services.dict-env.environment: 1")
 }
@@ -258,7 +275,7 @@ version: "3"
 services:
   foo:
     image: busybox
-`, nil)
+`)
 	assert.NoError(t, err)
 
 	_, err = loadYAML(`
@@ -266,7 +283,7 @@ version: "3.0"
 services:
   foo:
     image: busybox
-`, nil)
+`)
 	assert.NoError(t, err)
 }
 
@@ -276,7 +293,7 @@ version: "2"
 services:
   foo:
     image: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "version")
 
@@ -285,7 +302,7 @@ version: "2.0"
 services:
   foo:
     image: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "version")
 }
@@ -296,7 +313,7 @@ version: 3
 services:
   foo:
     image: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "version must be a string")
 }
@@ -305,7 +322,7 @@ func TestV1Unsupported(t *testing.T) {
 	_, err := loadYAML(`
 foo:
   image: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 }
 
@@ -315,7 +332,7 @@ version: "3"
 services:
   - foo:
       image: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "services must be a mapping")
 
@@ -323,7 +340,7 @@ services:
 version: "3"
 services:
   foo: busybox
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "services.foo must be a mapping")
 
@@ -332,7 +349,7 @@ version: "3"
 networks:
   - default:
       driver: bridge
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "networks must be a mapping")
 
@@ -340,7 +357,7 @@ networks:
 version: "3"
 networks:
   default: bridge
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "networks.default must be a mapping")
 
@@ -349,7 +366,7 @@ version: "3"
 volumes:
   - data:
       driver: local
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "volumes must be a mapping")
 
@@ -357,7 +374,7 @@ volumes:
 version: "3"
 volumes:
   data: local
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "volumes.data must be a mapping")
 }
@@ -368,13 +385,13 @@ version: "3"
 services:
   foo:
     image: ["busybox", "latest"]
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "services.foo.image must be a string")
 }
 
-func TestValidEnvironment(t *testing.T) {
-	config, err := loadYAML(`
+func TestLoadWithEnvironment(t *testing.T) {
+	config, err := loadYAMLWithEnv(`
 version: "3"
 services:
   dict-env:
@@ -391,17 +408,17 @@ services:
       - FOO=1
       - BAR=2
       - BAZ=2.5
-      - QUX
-      - QUUX=
+      - QUX=
+      - QUUX
 `, map[string]string{"QUX": "qux"})
 	assert.NoError(t, err)
 
 	expected := types.MappingWithEquals{
-		"FOO":  "1",
-		"BAR":  "2",
-		"BAZ":  "2.5",
-		"QUX":  "qux",
-		"QUUX": "",
+		"FOO":  strPtr("1"),
+		"BAR":  strPtr("2"),
+		"BAZ":  strPtr("2.5"),
+		"QUX":  strPtr("qux"),
+		"QUUX": nil,
 	}
 
 	assert.Equal(t, 2, len(config.Services))
@@ -419,7 +436,7 @@ services:
     image: busybox
     environment:
       FOO: ["1"]
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "services.dict-env.environment.FOO must be a string, number or null")
 }
@@ -431,14 +448,14 @@ services:
   dict-env:
     image: busybox
     environment: "FOO=1"
-`, nil)
+`)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "services.dict-env.environment must be a mapping")
 }
 
 func TestEnvironmentInterpolation(t *testing.T) {
 	home := "/home/foo"
-	config, err := loadYAML(`
+	config, err := loadYAMLWithEnv(`
 version: "3"
 services:
   test:
@@ -461,7 +478,7 @@ volumes:
 
 	assert.NoError(t, err)
 
-	expectedLabels := types.MappingWithEquals{
+	expectedLabels := types.Labels{
 		"home1":       home,
 		"home2":       home,
 		"nonexistent": "",
@@ -534,7 +551,7 @@ services:
   bar:
     extends:
       service: foo
-`, nil)
+`)
 
 	assert.Error(t, err)
 	assert.IsType(t, &ForbiddenPropertiesError{}, err)
@@ -607,7 +624,8 @@ func TestFullExample(t *testing.T) {
 	assert.NoError(t, err)
 
 	homeDir := "/home/foo"
-	config, err := loadYAML(string(bytes), map[string]string{"HOME": homeDir, "QUX": "2"})
+	env := map[string]string{"HOME": homeDir, "QUX": "qux_from_environment"}
+	config, err := loadYAMLWithEnv(string(bytes), env)
 	if !assert.NoError(t, err) {
 		return
 	}
@@ -662,14 +680,11 @@ func TestFullExample(t *testing.T) {
 		DNSSearch:  []string{"dc1.example.com", "dc2.example.com"},
 		DomainName: "foo.com",
 		Entrypoint: []string{"/code/entrypoint.sh", "-p", "3000"},
-		Environment: map[string]string{
-			"RACK_ENV":       "development",
-			"SHOW":           "true",
-			"SESSION_SECRET": "",
-			"FOO":            "1",
-			"BAR":            "2",
-			"BAZ":            "3",
-			"QUX":            "2",
+		Environment: map[string]*string{
+			"FOO": strPtr("foo_from_env_file"),
+			"BAR": strPtr("bar_from_env_file_2"),
+			"BAZ": strPtr("baz_from_service_def"),
+			"QUX": strPtr("qux_from_environment"),
 		},
 		EnvFile: []string{
 			"./example1.env",
@@ -961,15 +976,6 @@ func TestFullExample(t *testing.T) {
 	assert.Equal(t, expectedVolumeConfig, config.Volumes)
 }
 
-func loadYAML(yaml string, env map[string]string) (*types.Config, error) {
-	dict, err := ParseYAML([]byte(yaml))
-	if err != nil {
-		return nil, err
-	}
-
-	return Load(buildConfigDetails(dict, env))
-}
-
 func serviceSort(services []types.ServiceConfig) []types.ServiceConfig {
 	sort.Sort(servicesByName(services))
 	return services

compose/types/types.go

@@ -99,7 +99,7 @@ type ServiceConfig struct {
 	HealthCheck     *HealthCheckConfig
 	Image           string
 	Ipc             string
-	Labels          MappingWithEquals
+	Labels          Labels
 	Links           []string
 	Logging         *LoggingConfig
 	MacAddress      string `mapstructure:"mac_address"`
@@ -135,7 +135,10 @@ type StringOrNumberList []string
 
 // MappingWithEquals is a mapping type that can be converted from a list of
 // key=value strings
-type MappingWithEquals map[string]string
+type MappingWithEquals map[string]*string
+
+// Labels is a mapping type for labels
+type Labels map[string]string
 
 // MappingWithColon is a mapping type that can be converted from a list of
 // 'key: value' strings
@@ -151,7 +154,7 @@ type LoggingConfig struct {
 type DeployConfig struct {
 	Mode          string
 	Replicas      *uint64
-	Labels        MappingWithEquals
+	Labels        Labels
 	UpdateConfig  *UpdateConfig `mapstructure:"update_config"`
 	Resources     Resources
 	RestartPolicy *RestartPolicy `mapstructure:"restart_policy"`
@@ -268,7 +271,7 @@ type NetworkConfig struct {
 	External   External
 	Internal   bool
 	Attachable bool
-	Labels     MappingWithEquals
+	Labels     Labels
 }
 
 // IPAMConfig for a network
@@ -287,7 +290,7 @@ type VolumeConfig struct {
 	Driver     string
 	DriverOpts map[string]string `mapstructure:"driver_opts"`
 	External   External
-	Labels     MappingWithEquals
+	Labels     Labels
 }
 
 // External identifies a Volume or Network as a reference to a resource that is
@@ -301,5 +304,5 @@ type External struct {
 type SecretConfig struct {
 	File     string
 	External External
-	Labels   MappingWithEquals
+	Labels   Labels
 }