Merge pull request #30781 from AkihiroSuda/fix-stack-env

compose: fix environment interpolation from the client

cli/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"
 )
 
@@ -122,9 +123,26 @@ func getConfigDetails(opts deployOptions) (composetypes.ConfigDetails, error) {
 	}
 	// TODO: support multiple files
 	details.ConfigFiles = []composetypes.ConfigFile{*configFile}
+	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 result, errors.Errorf("unexpected environment %q", s)
+		}
+		kv := strings.SplitN(s, "=", 2)
+		result[kv[0]] = kv[1]
+	}
+	return result, nil
+}
+
 func getConfigFile(filename string) (*composetypes.ConfigFile, error) {
 	bytes, err := ioutil.ReadFile(filename)
 	if err != nil {

cli/compose/convert/service.go

@@ -394,11 +394,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

cli/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)

cli/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

cli/compose/loader/example2.env

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

cli/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

cli/compose/loader/loader.go

@@ -2,15 +2,16 @@ package loader
 
 import (
 	"fmt"
-	"os"
 	"path"
 	"reflect"
 	"regexp"
 	"sort"
 	"strings"
 
+	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/cli/compose/interpolation"
 	"github.com/docker/docker/cli/compose/schema"
+	"github.com/docker/docker/cli/compose/template"
 	"github.com/docker/docker/cli/compose/types"
 	"github.com/docker/docker/opts"
 	runconfigopts "github.com/docker/docker/runconfig/opts"
@@ -69,13 +70,17 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) {
 	}
 
 	cfg := types.Config{}
+	lookupEnv := func(k string) (string, bool) {
+		v, ok := configDetails.Environment[k]
+		return v, ok
+	}
 	if services, ok := configDict["services"]; ok {
-		servicesConfig, err := interpolation.Interpolate(services.(types.Dict), "service", os.LookupEnv)
+		servicesConfig, err := interpolation.Interpolate(services.(types.Dict), "service", lookupEnv)
 		if err != nil {
 			return nil, err
 		}
 
-		servicesList, err := LoadServices(servicesConfig, configDetails.WorkingDir)
+		servicesList, err := LoadServices(servicesConfig, configDetails.WorkingDir, lookupEnv)
 		if err != nil {
 			return nil, err
 		}
@@ -84,7 +89,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) {
 	}
 
 	if networks, ok := configDict["networks"]; ok {
-		networksConfig, err := interpolation.Interpolate(networks.(types.Dict), "network", os.LookupEnv)
+		networksConfig, err := interpolation.Interpolate(networks.(types.Dict), "network", lookupEnv)
 		if err != nil {
 			return nil, err
 		}
@@ -98,7 +103,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) {
 	}
 
 	if volumes, ok := configDict["volumes"]; ok {
-		volumesConfig, err := interpolation.Interpolate(volumes.(types.Dict), "volume", os.LookupEnv)
+		volumesConfig, err := interpolation.Interpolate(volumes.(types.Dict), "volume", lookupEnv)
 		if err != nil {
 			return nil, err
 		}
@@ -112,7 +117,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) {
 	}
 
 	if secrets, ok := configDict["secrets"]; ok {
-		secretsConfig, err := interpolation.Interpolate(secrets.(types.Dict), "secret", os.LookupEnv)
+		secretsConfig, err := interpolation.Interpolate(secrets.(types.Dict), "secret", lookupEnv)
 		if err != nil {
 			return nil, err
 		}
@@ -248,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)
 	}
@@ -308,11 +315,11 @@ func formatInvalidKeyError(keyPrefix string, key interface{}) error {
 
 // LoadServices produces a ServiceConfig map from a compose file Dict
 // the servicesDict is not validated if directly used. Use Load() to enable validation
-func LoadServices(servicesDict types.Dict, workingDir string) ([]types.ServiceConfig, error) {
+func LoadServices(servicesDict types.Dict, workingDir string, lookupEnv template.Mapping) ([]types.ServiceConfig, error) {
 	var services []types.ServiceConfig
 
 	for name, serviceDef := range servicesDict {
-		serviceConfig, err := LoadService(name, serviceDef.(types.Dict), workingDir)
+		serviceConfig, err := LoadService(name, serviceDef.(types.Dict), workingDir, lookupEnv)
 		if err != nil {
 			return nil, err
 		}
@@ -324,23 +331,35 @@ func LoadServices(servicesDict types.Dict, workingDir string) ([]types.ServiceCo
 
 // LoadService produces a single ServiceConfig from a compose file Dict
 // the serviceDict is not validated if directly used. Use Load() to enable validation
-func LoadService(name string, serviceDict types.Dict, workingDir string) (*types.ServiceConfig, error) {
+func LoadService(name string, serviceDict types.Dict, workingDir string, lookupEnv template.Mapping) (*types.ServiceConfig, error) {
 	serviceConfig := &types.ServiceConfig{}
 	if err := transform(serviceDict, serviceConfig); err != nil {
 		return nil, err
 	}
 	serviceConfig.Name = name
 
-	if err := resolveEnvironment(serviceConfig, workingDir); err != nil {
+	if err := resolveEnvironment(serviceConfig, workingDir, lookupEnv); err != nil {
 		return nil, err
 	}
 
-	resolveVolumePaths(serviceConfig.Volumes, workingDir)
+	resolveVolumePaths(serviceConfig.Volumes, workingDir, lookupEnv)
 	return serviceConfig, nil
 }
 
-func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string) error {
-	environment := make(map[string]string)
+func updateEnvironment(environment map[string]*string, vars map[string]*string, lookupEnv template.Mapping) {
+	for k, v := range vars {
+		interpolatedV, ok := lookupEnv(k)
+		if (v == nil || *v == "") && ok {
+			// lookupEnv is prioritized over vars
+			environment[k] = &interpolatedV
+		} else {
+			environment[k] = v
+		}
+	}
+}
+
+func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, lookupEnv template.Mapping) error {
+	environment := make(map[string]*string)
 
 	if len(serviceConfig.EnvFile) > 0 {
 		var envVars []string
@@ -353,36 +372,35 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string) e
 			}
 			envVars = append(envVars, fileVars...)
 		}
-
-		for k, v := range runconfigopts.ConvertKVStringsToMap(envVars) {
-			environment[k] = v
-		}
-	}
-
-	for k, v := range serviceConfig.Environment {
-		environment[k] = v
+		updateEnvironment(environment,
+			runconfigopts.ConvertKVStringsToMapWithNil(envVars), lookupEnv)
 	}
 
+	updateEnvironment(environment, serviceConfig.Environment, lookupEnv)
 	serviceConfig.Environment = environment
-
 	return nil
 }
 
-func resolveVolumePaths(volumes []types.ServiceVolumeConfig, workingDir string) {
+func resolveVolumePaths(volumes []types.ServiceVolumeConfig, workingDir string, lookupEnv template.Mapping) {
 	for i, volume := range volumes {
 		if volume.Type != "bind" {
 			continue
 		}
 
-		volume.Source = absPath(workingDir, expandUser(volume.Source))
+		volume.Source = absPath(workingDir, expandUser(volume.Source, lookupEnv))
 		volumes[i] = volume
 	}
 }
 
 // TODO: make this more robust
-func expandUser(path string) string {
+func expandUser(path string, lookupEnv template.Mapping) string {
 	if strings.HasPrefix(path, "~") {
-		return strings.Replace(path, "~", os.Getenv("HOME"), 1)
+		home, ok := lookupEnv("HOME")
+		if !ok {
+			logrus.Warn("cannot expand '~', because the environment lacks HOME")
+			return path
+		}
+		return strings.Replace(path, "~", home, 1)
 	}
 	return path
 }
@@ -476,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:
@@ -592,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) {
@@ -672,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)
 }

cli/compose/loader/loader_test.go

@@ -12,7 +12,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func buildConfigDetails(source types.Dict) types.ConfigDetails {
+func buildConfigDetails(source types.Dict, env map[string]string) types.ConfigDetails {
 	workingDir, err := os.Getwd()
 	if err != nil {
 		panic(err)
@@ -23,10 +23,23 @@ func buildConfigDetails(source types.Dict) types.ConfigDetails {
 		ConfigFiles: []types.ConfigFile{
 			{Filename: "filename.yml", Config: source},
 		},
-		Environment: nil,
+		Environment: env,
 	}
 }
 
+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,
 			},
@@ -154,7 +171,7 @@ func TestParseYAML(t *testing.T) {
 }
 
 func TestLoad(t *testing.T) {
-	actual, err := Load(buildConfigDetails(sampleDict))
+	actual, err := Load(buildConfigDetails(sampleDict, nil))
 	if !assert.NoError(t, err) {
 		return
 	}
@@ -373,8 +390,8 @@ services:
 	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:
@@ -383,6 +400,7 @@ services:
       FOO: "1"
       BAR: 2
       BAZ: 2.5
+      QUX:
       QUUX:
   list-env:
     image: busybox
@@ -390,15 +408,17 @@ services:
       - FOO=1
       - BAR=2
       - BAZ=2.5
-      - QUUX=
-`)
+      - QUX=
+      - QUUX
+`, map[string]string{"QUX": "qux"})
 	assert.NoError(t, err)
 
 	expected := types.MappingWithEquals{
-		"FOO":  "1",
-		"BAR":  "2",
-		"BAZ":  "2.5",
-		"QUUX": "",
+		"FOO":  strPtr("1"),
+		"BAR":  strPtr("2"),
+		"BAZ":  strPtr("2.5"),
+		"QUX":  strPtr("qux"),
+		"QUUX": nil,
 	}
 
 	assert.Equal(t, 2, len(config.Services))
@@ -434,7 +454,8 @@ services:
 }
 
 func TestEnvironmentInterpolation(t *testing.T) {
-	config, err := loadYAML(`
+	home := "/home/foo"
+	config, err := loadYAMLWithEnv(`
 version: "3"
 services:
   test:
@@ -450,13 +471,14 @@ networks:
 volumes:
   test:
     driver: $HOME
-`)
+`, map[string]string{
+		"HOME": home,
+		"FOO":  "foo",
+	})
 
 	assert.NoError(t, err)
 
-	home := os.Getenv("HOME")
-
-	expectedLabels := types.MappingWithEquals{
+	expectedLabels := types.Labels{
 		"home1":       home,
 		"home2":       home,
 		"nonexistent": "",
@@ -483,7 +505,7 @@ services:
 `))
 	assert.NoError(t, err)
 
-	configDetails := buildConfigDetails(dict)
+	configDetails := buildConfigDetails(dict, nil)
 
 	_, err = Load(configDetails)
 	assert.NoError(t, err)
@@ -506,7 +528,7 @@ services:
 `))
 	assert.NoError(t, err)
 
-	configDetails := buildConfigDetails(dict)
+	configDetails := buildConfigDetails(dict, nil)
 
 	_, err = Load(configDetails)
 	assert.NoError(t, err)
@@ -601,7 +623,9 @@ func TestFullExample(t *testing.T) {
 	bytes, err := ioutil.ReadFile("full-example.yml")
 	assert.NoError(t, err)
 
-	config, err := loadYAML(string(bytes))
+	homeDir := "/home/foo"
+	env := map[string]string{"HOME": homeDir, "QUX": "qux_from_environment"}
+	config, err := loadYAMLWithEnv(string(bytes), env)
 	if !assert.NoError(t, err) {
 		return
 	}
@@ -609,7 +633,6 @@ func TestFullExample(t *testing.T) {
 	workingDir, err := os.Getwd()
 	assert.NoError(t, err)
 
-	homeDir := os.Getenv("HOME")
 	stopGracePeriod := time.Duration(20 * time.Second)
 
 	expectedServiceConfig := types.ServiceConfig{
@@ -657,13 +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",
+		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",
@@ -955,15 +976,6 @@ func TestFullExample(t *testing.T) {
 	assert.Equal(t, expectedVolumeConfig, config.Volumes)
 }
 
-func loadYAML(yaml string) (*types.Config, error) {
-	dict, err := ParseYAML([]byte(yaml))
-	if err != nil {
-		return nil, err
-	}
-
-	return Load(buildConfigDetails(dict))
-}
-
 func serviceSort(services []types.ServiceConfig) []types.ServiceConfig {
 	sort.Sort(servicesByName(services))
 	return services

cli/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"`
@@ -134,8 +134,13 @@ type StringList []string
 type StringOrNumberList []string
 
 // MappingWithEquals is a mapping type that can be converted from a list of
-// key=value strings
-type MappingWithEquals map[string]string
+// key[=value] strings.
+// For the key with an empty value (`key=`), the mapped value is set to a pointer to `""`.
+// For the key without value (`key`), the mapped value is set to nil.
+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 +156,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 +273,7 @@ type NetworkConfig struct {
 	External   External
 	Internal   bool
 	Attachable bool
-	Labels     MappingWithEquals
+	Labels     Labels
 }
 
 // IPAMConfig for a network
@@ -287,7 +292,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 +306,5 @@ type External struct {
 type SecretConfig struct {
 	File     string
 	External External
-	Labels   MappingWithEquals
+	Labels   Labels
 }