From 146d3eb3049d16058d2ae52464ec944d5ebeb18b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 14 Mar 2017 12:39:26 -0400 Subject: [PATCH] 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 --- command/stack/deploy_composefile.go | 18 +++-- compose/convert/service.go | 9 ++- compose/convert/service_test.go | 10 ++- compose/loader/example1.env | 6 +- compose/loader/example2.env | 4 +- compose/loader/full-example.yml | 6 +- compose/loader/loader.go | 79 ++++++++++--------- compose/loader/loader_test.go | 118 +++++++++++++++------------- compose/types/types.go | 15 ++-- 9 files changed, 147 insertions(+), 118 deletions(-) diff --git a/command/stack/deploy_composefile.go b/command/stack/deploy_composefile.go index b176b47e0b..f415f42f8c 100644 --- a/command/stack/deploy_composefile.go +++ b/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) { diff --git a/compose/convert/service.go b/compose/convert/service.go index ab90d7319a..6b542f7701 100644 --- a/compose/convert/service.go +++ b/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 diff --git a/compose/convert/service_test.go b/compose/convert/service_test.go index 56f495df3f..352e9a61b5 100644 --- a/compose/convert/service_test.go +++ b/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) diff --git a/compose/loader/example1.env b/compose/loader/example1.env index 3e7a059613..f19ec0df4e 100644 --- a/compose/loader/example1.env +++ b/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 diff --git a/compose/loader/example2.env b/compose/loader/example2.env index 642334e9fd..f47d1e6145 100644 --- a/compose/loader/example2.env +++ b/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 diff --git a/compose/loader/full-example.yml b/compose/loader/full-example.yml index fb5686a380..e8f3716013 100644 --- a/compose/loader/full-example.yml +++ b/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 diff --git a/compose/loader/loader.go b/compose/loader/loader.go index 7c8bfa0a2a..3edcd81668 100644 --- a/compose/loader/loader.go +++ b/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) } diff --git a/compose/loader/loader_test.go b/compose/loader/loader_test.go index 4f424d6126..661e2c615c 100644 --- a/compose/loader/loader_test.go +++ b/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 diff --git a/compose/types/types.go b/compose/types/types.go index e91b5a7ac8..bb12f8497b 100644 --- a/compose/types/types.go +++ b/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 }