From 681d94ae207b0ff780642d68e98a518eb27eefce Mon Sep 17 00:00:00 2001 From: James Nugent Date: Wed, 20 Jul 2016 20:38:26 -0500 Subject: [PATCH] core: Allow lists and maps as variable overrides Terraform 0.7 introduces lists and maps as first-class values for variables, in addition to string values which were previously available. However, there was previously no way to override the default value of a list or map, and the functionality for overriding specific map keys was broken. Using the environment variable method for setting variable values, there was previously no way to give a variable a value of a list or map. These now support HCL for individual values - specifying: TF_VAR_test='["Hello", "World"]' will set the variable `test` to a two-element list containing "Hello" and "World". Specifying TF_VAR_test_map='{"Hello = "World", "Foo" = "bar"}' will set the variable `test_map` to a two-element map with keys "Hello" and "Foo", and values "World" and "bar" respectively. The same logic is applied to `-var` flags, and the file parsed by `-var-files` ("autoVariables"). Note that care must be taken to not run into shell expansion for `-var-` flags and environment variables. We also merge map keys where appropriate. The override syntax has changed (to be noted in CHANGELOG as a breaking change), so several tests needed their syntax updating from the old `amis.us-east-1 = "newValue"` style to `amis = "{ "us-east-1" = "newValue"}"` style as defined in TF-002. In order to continue supporting the `-var "foo=bar"` type of variable flag (which is not valid HCL), a special case error is checked after HCL parsing fails, and the old code path runs instead. --- command/flag_kv.go | 89 +++++++++- command/flag_kv_test.go | 101 ++++++++++-- command/init.go | 2 +- command/meta.go | 6 +- command/remote_config.go | 2 +- terraform/context.go | 156 ++++++++++++++++-- terraform/context_apply_test.go | 29 +++- terraform/context_input_test.go | 8 +- terraform/semantics.go | 38 ++++- terraform/terraform_test.go | 4 + .../test-fixtures/apply-vars-env/main.tf | 17 +- terraform/test-fixtures/apply-vars/main.tf | 10 ++ 12 files changed, 409 insertions(+), 53 deletions(-) diff --git a/command/flag_kv.go b/command/flag_kv.go index 492cc66be..b39aa1cd8 100644 --- a/command/flag_kv.go +++ b/command/flag_kv.go @@ -3,21 +3,46 @@ package command import ( "fmt" "io/ioutil" + "regexp" "strings" "github.com/hashicorp/hcl" "github.com/mitchellh/go-homedir" ) -// FlagKV is a flag.Value implementation for parsing user variables -// from the command-line in the format of '-var key=value'. -type FlagKV map[string]string +// FlagTypedKVis a flag.Value implementation for parsing user variables +// from the command-line in the format of '-var key=value', where value is +// a type intended for use as a Terraform variable +type FlagTypedKV map[string]interface{} -func (v *FlagKV) String() string { +func (v *FlagTypedKV) String() string { return "" } -func (v *FlagKV) Set(raw string) error { +func (v *FlagTypedKV) Set(raw string) error { + key, value, err := parseVarFlagAsHCL(raw) + if err != nil { + return err + } + + if *v == nil { + *v = make(map[string]interface{}) + } + + (*v)[key] = value + return nil +} + +// FlagStringKV is a flag.Value implementation for parsing user variables +// from the command-line in the format of '-var key=value', where value is +// only ever a primitive. +type FlagStringKV map[string]string + +func (v *FlagStringKV) String() string { + return "" +} + +func (v *FlagStringKV) Set(raw string) error { idx := strings.Index(raw, "=") if idx == -1 { return fmt.Errorf("No '=' value in arg: %s", raw) @@ -34,7 +59,7 @@ func (v *FlagKV) Set(raw string) error { // FlagKVFile is a flag.Value implementation for parsing user variables // from the command line in the form of files. i.e. '-var-file=foo' -type FlagKVFile map[string]string +type FlagKVFile map[string]interface{} func (v *FlagKVFile) String() string { return "" @@ -47,7 +72,7 @@ func (v *FlagKVFile) Set(raw string) error { } if *v == nil { - *v = make(map[string]string) + *v = make(map[string]interface{}) } for key, value := range vs { @@ -57,7 +82,7 @@ func (v *FlagKVFile) Set(raw string) error { return nil } -func loadKVFile(rawPath string) (map[string]string, error) { +func loadKVFile(rawPath string) (map[string]interface{}, error) { path, err := homedir.Expand(rawPath) if err != nil { return nil, fmt.Errorf( @@ -78,7 +103,7 @@ func loadKVFile(rawPath string) (map[string]string, error) { "Error parsing %s: %s", path, err) } - var result map[string]string + var result map[string]interface{} if err := hcl.DecodeObject(&result, obj); err != nil { return nil, fmt.Errorf( "Error decoding Terraform vars file: %s\n\n"+ @@ -103,3 +128,49 @@ func (v *FlagStringSlice) Set(raw string) error { return nil } + +// parseVarFlagAsHCL parses the value of a single variable as would have been specified +// on the command line via -var or in an environment variable named TF_VAR_x, where x is +// the name of the variable. In order to get around the restriction of HCL requiring a +// top level object, we prepend a sentinel key, decode the user-specified value as its +// value and pull the value back out of the resulting map. +func parseVarFlagAsHCL(input string) (string, interface{}, error) { + idx := strings.Index(input, "=") + if idx == -1 { + return "", nil, fmt.Errorf("No '=' value in variable: %s", input) + } + probablyName := input[0:idx] + + parsed, err := hcl.Parse(input) + if err != nil { + // This covers flags of the form `foo=bar` which is not valid HCL + // At this point, probablyName is actually the name, and the remainder + // of the expression after the equals sign is the value. + if regexp.MustCompile(`Unknown token: \d+:\d+ IDENT`).Match([]byte(err.Error())) { + value := input[idx+1:] + return probablyName, value, nil + } + return "", nil, fmt.Errorf("Cannot parse value for variable %s (%q) as valid HCL: %s", probablyName, input, err) + } + + var decoded map[string]interface{} + if hcl.DecodeObject(&decoded, parsed); err != nil { + return "", nil, fmt.Errorf("Cannot parse value for variable %s (%q) as valid HCL: %s", probablyName, input, err) + } + + // Cover cases such as key= + if len(decoded) == 0 { + return probablyName, "", nil + } + + if len(decoded) > 1 { + return "", nil, fmt.Errorf("Cannot parse value for variable %s (%q) as valid HCL. Only one value may be specified.", probablyName, input) + } + + for k, v := range decoded { + return k, v, nil + } + + // Should be unreachable + return "", nil, fmt.Errorf("No value for variable: %s", input) +} diff --git a/command/flag_kv_test.go b/command/flag_kv_test.go index 07b70d022..000915e61 100644 --- a/command/flag_kv_test.go +++ b/command/flag_kv_test.go @@ -2,16 +2,17 @@ package command import ( "flag" + "github.com/davecgh/go-spew/spew" "io/ioutil" "reflect" "testing" ) -func TestFlagKV_impl(t *testing.T) { - var _ flag.Value = new(FlagKV) +func TestFlagStringKV_impl(t *testing.T) { + var _ flag.Value = new(FlagStringKV) } -func TestFlagKV(t *testing.T) { +func TestFlagStringKV(t *testing.T) { cases := []struct { Input string Output map[string]string @@ -49,10 +50,10 @@ func TestFlagKV(t *testing.T) { } for _, tc := range cases { - f := new(FlagKV) + f := new(FlagStringKV) err := f.Set(tc.Input) if err != nil != tc.Error { - t.Fatalf("bad error. Input: %#v", tc.Input) + t.Fatalf("bad error. Input: %#v\n\nError: %s", tc.Input, err) } actual := map[string]string(*f) @@ -62,6 +63,86 @@ func TestFlagKV(t *testing.T) { } } +func TestFlagTypedKV_impl(t *testing.T) { + var _ flag.Value = new(FlagTypedKV) +} + +func TestFlagTypedKV(t *testing.T) { + cases := []struct { + Input string + Output map[string]interface{} + Error bool + }{ + { + "key=value", + map[string]interface{}{"key": "value"}, + false, + }, + + { + "key=", + map[string]interface{}{"key": ""}, + false, + }, + + { + "key=foo=bar", + map[string]interface{}{"key": "foo=bar"}, + false, + }, + + { + "map.key=foo", + map[string]interface{}{"map.key": "foo"}, + false, + }, + + { + "key", + nil, + true, + }, + + { + `key=["hello", "world"]`, + map[string]interface{}{"key": []interface{}{"hello", "world"}}, + false, + }, + + { + `key={"hello" = "world", "foo" = "bar"}`, + map[string]interface{}{ + "key": []map[string]interface{}{ + map[string]interface{}{ + "hello": "world", + "foo": "bar", + }, + }, + }, + false, + }, + + { + `key={"hello" = "world", "foo" = "bar"}\nkey2="invalid"`, + nil, + true, + }, + } + + for _, tc := range cases { + f := new(FlagTypedKV) + err := f.Set(tc.Input) + if err != nil != tc.Error { + t.Fatalf("bad error. Input: %#v\n\nError: %s", tc.Input, err) + } + + actual := map[string]interface{}(*f) + if !reflect.DeepEqual(actual, tc.Output) { + t.Fatalf("bad:\nexpected: %s\n\n got: %s\n", spew.Sdump(tc.Output), spew.Sdump(actual)) + } + } +} + func TestFlagKVFile_impl(t *testing.T) { var _ flag.Value = new(FlagKVFile) } @@ -76,24 +157,24 @@ foo = "bar" cases := []struct { Input string - Output map[string]string + Output map[string]interface{} Error bool }{ { inputLibucl, - map[string]string{"foo": "bar"}, + map[string]interface{}{"foo": "bar"}, false, }, { inputJson, - map[string]string{"foo": "bar"}, + map[string]interface{}{"foo": "bar"}, false, }, { `map.key = "foo"`, - map[string]string{"map.key": "foo"}, + map[string]interface{}{"map.key": "foo"}, false, }, } @@ -111,7 +192,7 @@ foo = "bar" t.Fatalf("bad error. Input: %#v, err: %s", tc.Input, err) } - actual := map[string]string(*f) + actual := map[string]interface{}(*f) if !reflect.DeepEqual(actual, tc.Output) { t.Fatalf("bad: %#v", actual) } diff --git a/command/init.go b/command/init.go index bcc339bee..c4026d48d 100644 --- a/command/init.go +++ b/command/init.go @@ -25,7 +25,7 @@ func (c *InitCommand) Run(args []string) int { remoteConfig := make(map[string]string) cmdFlags := flag.NewFlagSet("init", flag.ContinueOnError) cmdFlags.StringVar(&remoteBackend, "backend", "", "") - cmdFlags.Var((*FlagKV)(&remoteConfig), "backend-config", "config") + cmdFlags.Var((*FlagStringKV)(&remoteConfig), "backend-config", "config") cmdFlags.Usage = func() { c.Ui.Error(c.Help()) } if err := cmdFlags.Parse(args); err != nil { return 1 diff --git a/command/meta.go b/command/meta.go index 69f3e3e5c..5adf1b667 100644 --- a/command/meta.go +++ b/command/meta.go @@ -36,9 +36,9 @@ type Meta struct { // Variables for the context (private) autoKey string - autoVariables map[string]string + autoVariables map[string]interface{} input bool - variables map[string]string + variables map[string]interface{} // Targets for this context (private) targets []string @@ -315,7 +315,7 @@ func (m *Meta) contextOpts() *terraform.ContextOpts { func (m *Meta) flagSet(n string) *flag.FlagSet { f := flag.NewFlagSet(n, flag.ContinueOnError) f.BoolVar(&m.input, "input", true, "input") - f.Var((*FlagKV)(&m.variables), "var", "variables") + f.Var((*FlagTypedKV)(&m.variables), "var", "variables") f.Var((*FlagKVFile)(&m.variables), "var-file", "variable file") f.Var((*FlagStringSlice)(&m.targets), "target", "resource to target") diff --git a/command/remote_config.go b/command/remote_config.go index 6f53d0dbb..7f4a00b81 100644 --- a/command/remote_config.go +++ b/command/remote_config.go @@ -38,7 +38,7 @@ func (c *RemoteConfigCommand) Run(args []string) int { cmdFlags.StringVar(&c.conf.statePath, "state", DefaultStateFilename, "path") cmdFlags.StringVar(&c.conf.backupPath, "backup", "", "path") cmdFlags.StringVar(&c.remoteConf.Type, "backend", "atlas", "") - cmdFlags.Var((*FlagKV)(&config), "backend-config", "config") + cmdFlags.Var((*FlagStringKV)(&config), "backend-config", "config") cmdFlags.Usage = func() { c.Ui.Error(c.Help()) } if err := cmdFlags.Parse(args); err != nil { c.Ui.Error(fmt.Sprintf("\nError parsing CLI flags: %s", err)) diff --git a/terraform/context.go b/terraform/context.go index f653aba5e..86d7e58ce 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -9,6 +9,7 @@ import ( "sync" "github.com/hashicorp/go-multierror" + "github.com/hashicorp/hcl" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/module" ) @@ -119,24 +120,109 @@ func NewContext(opts *ContextOpts) (*Context, error) { par = 10 } - // Setup the variables. We first take the variables given to us. - // We then merge in the variables set in the environment. + // Set up the variables in the following sequence: + // 0 - Take default values from the configuration + // 1 - Take values from TF_VAR_x environment variables + // 2 - Take values specified in -var flags, overriding values + // set by environment variables if necessary. This includes + // values taken from -var-file in addition. variables := make(map[string]interface{}) - for _, v := range os.Environ() { - if !strings.HasPrefix(v, VarEnvPrefix) { - continue + + if opts.Module != nil { + for _, v := range opts.Module.Config().Variables { + if v.Default != nil { + if v.Type() == config.VariableTypeString { + // v.Default has already been parsed as HCL so there may be + // some stray ints in there + switch typedDefault := v.Default.(type) { + case string: + if typedDefault == "" { + continue + } + variables[v.Name] = typedDefault + case int, int64: + variables[v.Name] = fmt.Sprintf("%d", typedDefault) + case float32, float64: + variables[v.Name] = fmt.Sprintf("%f", typedDefault) + case bool: + variables[v.Name] = fmt.Sprintf("%t", typedDefault) + } + } else { + variables[v.Name] = v.Default + } + } } - // Strip off the prefix and get the value after the first "=" - idx := strings.Index(v, "=") - k := v[len(VarEnvPrefix):idx] - v = v[idx+1:] + for _, v := range os.Environ() { + if !strings.HasPrefix(v, VarEnvPrefix) { + continue + } - // Override the command-line set variable - variables[k] = v - } - for k, v := range opts.Variables { - variables[k] = v + // Strip off the prefix and get the value after the first "=" + idx := strings.Index(v, "=") + k := v[len(VarEnvPrefix):idx] + v = v[idx+1:] + + // Override the configuration-default values. Note that *not* finding the variable + // in configuration is OK, as we don't want to preclude people from having multiple + // sets of TF_VAR_whatever in their environment even if it is a little weird. + for _, schema := range opts.Module.Config().Variables { + if schema.Name == k { + varType := schema.Type() + varVal, err := parseVariableAsHCL(k, v, varType) + if err != nil { + return nil, err + } + switch varType { + case config.VariableTypeMap: + if existing, hasMap := variables[k]; !hasMap { + variables[k] = varVal + } else { + if existingMap, ok := existing.(map[string]interface{}); !ok { + panic(fmt.Sprintf("%s is not a map, this is a bug in Terraform.", k)) + } else { + if newMap, ok := varVal.(map[string]interface{}); ok { + for newKey, newVal := range newMap { + existingMap[newKey] = newVal + } + } else { + panic(fmt.Sprintf("%s is not a map, this is a bug in Terraform.", k)) + } + } + } + default: + variables[k] = varVal + } + } + } + } + + for k, v := range opts.Variables { + for _, schema := range opts.Module.Config().Variables { + if schema.Name == k { + switch schema.Type() { + case config.VariableTypeMap: + if existing, hasMap := variables[k]; !hasMap { + variables[k] = v + } else { + if existingMap, ok := existing.(map[string]interface{}); !ok { + panic(fmt.Sprintf("%s is not a map, this is a bug in Terraform.", k)) + } else { + if newMap, ok := v.([]map[string]interface{}); ok { + for newKey, newVal := range newMap[0] { + existingMap[newKey] = newVal + } + } else { + panic(fmt.Sprintf("%s is not a map, this is a bug in Terraform.", k)) + } + } + } + default: + variables[k] = v + } + } + } + } } return &Context{ @@ -548,3 +634,45 @@ func (c *Context) walk( walker := &ContextGraphWalker{Context: c, Operation: operation} return walker, graph.Walk(walker) } + +// parseVariableAsHCL parses the value of a single variable as would have been specified +// on the command line via -var or in an environment variable named TF_VAR_x, where x is +// the name of the variable. In order to get around the restriction of HCL requiring a +// top level object, we prepend a sentinel key, decode the user-specified value as its +// value and pull the value back out of the resulting map. +func parseVariableAsHCL(name string, input interface{}, targetType config.VariableType) (interface{}, error) { + if targetType == config.VariableTypeString { + return input, nil + } + + const sentinelValue = "SENTINEL_TERRAFORM_VAR_OVERRIDE_KEY" + inputWithSentinal := fmt.Sprintf("%s = %s", sentinelValue, input) + + var decoded map[string]interface{} + err := hcl.Decode(&decoded, inputWithSentinal) + if err != nil { + return nil, fmt.Errorf("Cannot parse value for variable %s (%q) as valid HCL: %s", name, input, err) + } + + if len(decoded) != 1 { + return nil, fmt.Errorf("Cannot parse value for variable %s (%q) as valid HCL. Only one value may be specified.", name, input) + } + + parsedValue, ok := decoded[sentinelValue] + if !ok { + return nil, fmt.Errorf("Cannot parse value for variable %s (%q) as valid HCL. One value must be specified.", name, input) + } + + switch targetType { + case config.VariableTypeList: + return parsedValue, nil + case config.VariableTypeMap: + if list, ok := parsedValue.([]map[string]interface{}); ok { + return list[0], nil + } + + return nil, fmt.Errorf("Cannot parse value for variable %s (%q) as valid HCL. One value must be specified.", name, input) + default: + panic(fmt.Errorf("unknown type %s", targetType)) + } +} diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index cc499a482..ce64ddfca 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1135,7 +1135,11 @@ func TestContext2Apply_mapVariableOverride(t *testing.T) { "aws": testProviderFuncFixed(p), }, Variables: map[string]interface{}{ - "images.us-west-2": "overridden", + "images": []map[string]interface{}{ + map[string]interface{}{ + "us-west-2": "overridden", + }, + }, }, }) @@ -4269,8 +4273,18 @@ func TestContext2Apply_vars(t *testing.T) { "aws": testProviderFuncFixed(p), }, Variables: map[string]interface{}{ - "foo": "us-west-2", - "amis.us-east-1": "override", + "foo": "us-west-2", + "test_list": []interface{}{"Hello", "World"}, + "test_map": map[string]interface{}{ + "Hello": "World", + "Foo": "Bar", + "Baz": "Foo", + }, + "amis": []map[string]interface{}{ + map[string]interface{}{ + "us-east-1": "override", + }, + }, }, }) @@ -4300,8 +4314,13 @@ func TestContext2Apply_vars(t *testing.T) { func TestContext2Apply_varsEnv(t *testing.T) { // Set the env var - old := tempEnv(t, "TF_VAR_ami", "baz") - defer os.Setenv("TF_VAR_ami", old) + old_ami := tempEnv(t, "TF_VAR_ami", "baz") + old_list := tempEnv(t, "TF_VAR_list", `["Hello", "World"]`) + old_map := tempEnv(t, "TF_VAR_map", `{"Hello" = "World", "Foo" = "Bar", "Baz" = "Foo"}`) + + defer os.Setenv("TF_VAR_ami", old_ami) + defer os.Setenv("TF_VAR_list", old_list) + defer os.Setenv("TF_VAR_list", old_map) m := testModule(t, "apply-vars-env") p := testProvider("aws") diff --git a/terraform/context_input_test.go b/terraform/context_input_test.go index 9791b06fb..13e372469 100644 --- a/terraform/context_input_test.go +++ b/terraform/context_input_test.go @@ -19,8 +19,12 @@ func TestContext2Input(t *testing.T) { "aws": testProviderFuncFixed(p), }, Variables: map[string]interface{}{ - "foo": "us-west-2", - "amis.us-east-1": "override", + "foo": "us-west-2", + "amis": []map[string]interface{}{ + map[string]interface{}{ + "us-east-1": "override", + }, + }, }, UIInput: input, }) diff --git a/terraform/semantics.go b/terraform/semantics.go index 6d001226a..e8e52b7aa 100644 --- a/terraform/semantics.go +++ b/terraform/semantics.go @@ -95,16 +95,42 @@ func smcUserVariables(c *config.Config, vs map[string]interface{}) []error { } // Check that types match up - for k, _ := range vs { - v, ok := cvs[k] + for name, proposedValue := range vs { + schema, ok := cvs[name] if !ok { continue } - if v.Type() != config.VariableTypeString { - errs = append(errs, fmt.Errorf( - "%s: cannot assign string value to map type", - k)) + declaredType := schema.Type() + + switch declaredType { + case config.VariableTypeString: + switch proposedValue.(type) { + case string: + continue + default: + errs = append(errs, fmt.Errorf("variable %s should be type %s, got %s", + name, declaredType.Printable(), hclTypeName(proposedValue))) + } + case config.VariableTypeMap: + switch proposedValue.(type) { + case map[string]interface{}: + continue + default: + errs = append(errs, fmt.Errorf("variable %s should be type %s, got %s", + name, declaredType.Printable(), hclTypeName(proposedValue))) + } + case config.VariableTypeList: + switch proposedValue.(type) { + case []interface{}: + continue + default: + errs = append(errs, fmt.Errorf("variable %s should be type %s, got %s", + name, declaredType.Printable(), hclTypeName(proposedValue))) + } + default: + errs = append(errs, fmt.Errorf("variable %s should be type %s, got %s", + name, declaredType.Printable(), hclTypeName(proposedValue))) } } diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 6be32ea03..fbcf6c61e 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -705,6 +705,8 @@ aws_instance.bar: aws_instance.foo: ID = foo bar = baz + list = Hello,World + map = Baz,Foo,Hello num = 2 type = aws_instance ` @@ -712,6 +714,8 @@ aws_instance.foo: const testTerraformApplyVarsEnvStr = ` aws_instance.bar: ID = foo + bar = Hello,World + baz = Baz,Foo,Hello foo = baz type = aws_instance ` diff --git a/terraform/test-fixtures/apply-vars-env/main.tf b/terraform/test-fixtures/apply-vars-env/main.tf index b686da065..245564323 100644 --- a/terraform/test-fixtures/apply-vars-env/main.tf +++ b/terraform/test-fixtures/apply-vars-env/main.tf @@ -1,7 +1,20 @@ variable "ami" { - default = "foo" + default = "foo" + type = "string" +} + +variable "list" { + default = [] + type = "list" +} + +variable "map" { + default = {} + type = "map" } resource "aws_instance" "bar" { - foo = "${var.ami}" + foo = "${var.ami}" + bar = "${join(",", var.list)}" + baz = "${join(",", keys(var.map))}" } diff --git a/terraform/test-fixtures/apply-vars/main.tf b/terraform/test-fixtures/apply-vars/main.tf index 7cd4b5316..7c426b227 100644 --- a/terraform/test-fixtures/apply-vars/main.tf +++ b/terraform/test-fixtures/apply-vars/main.tf @@ -5,6 +5,14 @@ variable "amis" { } } +variable "test_list" { + type = "list" +} + +variable "test_map" { + type = "map" +} + variable "bar" { default = "baz" } @@ -14,6 +22,8 @@ variable "foo" {} resource "aws_instance" "foo" { num = "2" bar = "${var.bar}" + list = "${join(",", var.test_list)}" + map = "${join(",", keys(var.test_map))}" } resource "aws_instance" "bar" {