From 4bfabbaee47fefd3cb11f7788731c60611d8efdf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 8 Apr 2021 09:57:14 -0400 Subject: [PATCH 01/38] restore saved dependencies on delete error Is a resource delete action fails and the provider returned a new state, we need to ensure the stored dependencies are retained. --- terraform/context_apply_test.go | 11 ++++++++--- terraform/node_resource_abstract_instance.go | 7 +++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index d22ca20a5d..f8c4c23c61 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -12467,9 +12467,10 @@ func TestContext2Apply_errorRestoreStatus(t *testing.T) { state := states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent(addr, &states.ResourceInstanceObjectSrc{ - Status: states.ObjectTainted, - AttrsJSON: []byte(`{"test_string":"foo"}`), - Private: []byte("private"), + Status: states.ObjectTainted, + AttrsJSON: []byte(`{"test_string":"foo"}`), + Private: []byte("private"), + Dependencies: []addrs.ConfigResource{mustConfigResourceAddr("test_object.b")}, }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) }) @@ -12506,6 +12507,10 @@ func TestContext2Apply_errorRestoreStatus(t *testing.T) { t.Fatal("resource should still be tainted in the state") } + if len(res.Current.Dependencies) != 1 || !res.Current.Dependencies[0].Equal(mustConfigResourceAddr("test_object.b")) { + t.Fatalf("incorrect dependencies, got %q", res.Current.Dependencies) + } + if string(res.Current.Private) != "private" { t.Fatalf("incorrect private data, got %q", res.Current.Private) } diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index 7197311f54..2284d54020 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -2102,6 +2102,13 @@ func (n *NodeAbstractResourceInstance) apply( Private: resp.Private, CreateBeforeDestroy: createBeforeDestroy, } + + // if the resource was being deleted, the dependencies are not going to + // be recalculated and we need to restore those as well. + if change.Action == plans.Delete { + newState.Dependencies = state.Dependencies + } + return newState, diags case !newVal.IsNull(): From 1ad01debf3835e322ec9e07f11708416f7789be5 Mon Sep 17 00:00:00 2001 From: Sergey Elantsev Date: Fri, 9 Apr 2021 22:59:30 +0300 Subject: [PATCH 02/38] tiny optimisations of dag.Set 1. Use hint for map size in Set.Copy(). 2. Use Set.Copy() if Set.Difference() argument is empty. --- dag/set.go | 12 ++++++------ dag/set_test.go | 9 +++++++++ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/dag/set.go b/dag/set.go index 2e5d85b2d2..390415e316 100644 --- a/dag/set.go +++ b/dag/set.go @@ -56,13 +56,13 @@ func (s Set) Intersection(other Set) Set { // Difference returns a set with the elements that s has but // other doesn't. func (s Set) Difference(other Set) Set { + if other == nil || other.Len() == 0 { + return s.Copy() + } + result := make(Set) for k, v := range s { - var ok bool - if other != nil { - _, ok = other[k] - } - if !ok { + if _, ok := other[k]; !ok { result.Add(v) } } @@ -105,7 +105,7 @@ func (s Set) List() []interface{} { // Copy returns a shallow copy of the set. func (s Set) Copy() Set { - c := make(Set) + c := make(Set, len(s)) for k, v := range s { c[k] = v } diff --git a/dag/set_test.go b/dag/set_test.go index d59eacfbdc..721f2467dc 100644 --- a/dag/set_test.go +++ b/dag/set_test.go @@ -31,6 +31,12 @@ func TestSetDifference(t *testing.T) { []interface{}{3, 2, 1, 4}, []interface{}{}, }, + { + "B is nil", + []interface{}{1, 2, 3}, + nil, + []interface{}{1, 2, 3}, + }, } for i, tc := range cases { @@ -44,6 +50,9 @@ func TestSetDifference(t *testing.T) { for _, v := range tc.B { two.Add(v) } + if tc.B == nil { + two = nil + } for _, v := range tc.Expected { expected.Add(v) } From c1f719345417b8bccdfe001096ed630c7fbedaec Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 9 Apr 2021 14:33:49 -0400 Subject: [PATCH 03/38] lang/funcs: Make nonsensitive more permissive Calling the nonsensitive function with values which are not sensitive will result in an error. This restriction was added with the goal of preventing confusingly redundant use of this function. Unfortunately, this breaks when using nonsensitive to reveal the value of sensitive resource attributes. This is because the validate walk does not (and cannot) mark attributes as sensitive based on the schema, because the resource value itself is unknown. This commit therefore alters this restriction such that it permits nonsensitive unknown values, and adds a test case to cover this specific scenario. --- lang/funcs/sensitive.go | 2 +- lang/funcs/sensitive_test.go | 13 ++++--- terraform/context_plan2_test.go | 62 +++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/lang/funcs/sensitive.go b/lang/funcs/sensitive.go index b132dc12b3..2fc505e73f 100644 --- a/lang/funcs/sensitive.go +++ b/lang/funcs/sensitive.go @@ -48,7 +48,7 @@ var NonsensitiveFunc = function.New(&function.Spec{ return args[0].Type(), nil }, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { - if !args[0].HasMark("sensitive") { + if args[0].IsKnown() && !args[0].HasMark("sensitive") { return cty.DynamicVal, function.NewArgErrorf(0, "the given value is not sensitive, so this call is redundant") } v, marks := args[0].Unmark() diff --git a/lang/funcs/sensitive_test.go b/lang/funcs/sensitive_test.go index 522346b4cc..591f53cf61 100644 --- a/lang/funcs/sensitive_test.go +++ b/lang/funcs/sensitive_test.go @@ -133,17 +133,20 @@ func TestNonsensitive(t *testing.T) { cty.NumberIntVal(1), `the given value is not sensitive, so this call is redundant`, }, - { - cty.DynamicVal, - `the given value is not sensitive, so this call is redundant`, - }, { cty.NullVal(cty.String), `the given value is not sensitive, so this call is redundant`, }, + + // Unknown values may become sensitive once they are known, so we + // permit them to be marked nonsensitive. + { + cty.DynamicVal, + ``, + }, { cty.UnknownVal(cty.String), - `the given value is not sensitive, so this call is redundant`, + ``, }, } diff --git a/terraform/context_plan2_test.go b/terraform/context_plan2_test.go index b2d2384950..0444a775ca 100644 --- a/terraform/context_plan2_test.go +++ b/terraform/context_plan2_test.go @@ -377,3 +377,65 @@ resource "test_object" "a" { } } } + +func TestContext2Plan_unmarkingSensitiveAttributeForOutput(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_resource" "foo" { +} + +output "result" { + value = nonsensitive(test_resource.foo.sensitive_attr) +} +`, + }) + + p := new(MockProvider) + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "sensitive_attr": { + Type: cty.String, + Computed: true, + Sensitive: true, + }, + }, + }, + }, + }) + + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: cty.UnknownVal(cty.Object(map[string]cty.Type{ + "id": cty.String, + "sensitive_attr": cty.String, + })), + } + } + + state := states.NewState() + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + State: state, + }) + + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + for _, res := range plan.Changes.Resources { + if res.Action != plans.Create { + t.Fatalf("expected create, got: %q %s", res.Addr, res.Action) + } + } +} From 140c613ae8cae547b4b0c6d43a865fbbdae8d779 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 8 Jan 2021 16:02:56 -0800 Subject: [PATCH 04/38] lang/funcs: "one" function In the Terraform language we typically use lists of zero or one values in some sense interchangably with single values that might be null, because various Terraform language constructs are designed to work with collections rather than with nullable values. In Terraform v0.12 we made the splat operator [*] have a "special power" of concisely converting from a possibly-null single value into a zero-or-one list as a way to make that common operation more concise. In a sense this "one" function is the opposite operation to that special power: it goes from a zero-or-one collection (list, set, or tuple) to a possibly-null single value. This is a concise alternative to the following clunky conditional expression, with the additional benefit that the following expression is also not viable for set values, and it also properly handles the case where there's unexpectedly more than one value: length(var.foo) != 0 ? var.foo[0] : null Instead, we can write: one(var.foo) As with the splat operator, this is a tricky tradeoff because it could be argued that it's not something that'd be immediately intuitive to someone unfamiliar with Terraform. However, I think that's justified given how often zero-or-one collections arise in typical Terraform configurations. Unlike the splat operator, it should at least be easier to search for its name and find its documentation the first time you see it in a configuration. My expectation that this will become a common pattern is also my justification for giving it a short, concise name. Arguably it could be better named something like "oneornull", but that's a pretty clunky name and I'm not convinced it really adds any clarity for someone who isn't already familiar with it. --- lang/funcs/collection.go | 84 ++++++ lang/funcs/collection_test.go | 281 ++++++++++++++++++++ lang/functions.go | 1 + lang/functions_test.go | 11 + website/docs/language/functions/one.html.md | 121 +++++++++ website/layouts/language.erb | 4 + 6 files changed, 502 insertions(+) create mode 100644 website/docs/language/functions/one.html.md diff --git a/lang/funcs/collection.go b/lang/funcs/collection.go index 33073d7477..ce6c8dc8f0 100644 --- a/lang/funcs/collection.go +++ b/lang/funcs/collection.go @@ -10,6 +10,7 @@ import ( "github.com/zclconf/go-cty/cty/convert" "github.com/zclconf/go-cty/cty/function" "github.com/zclconf/go-cty/cty/function/stdlib" + "github.com/zclconf/go-cty/cty/gocty" ) var LengthFunc = function.New(&function.Spec{ @@ -381,6 +382,83 @@ var MatchkeysFunc = function.New(&function.Spec{ }, }) +// OneFunc returns either the first element of a one-element list, or null +// if given a zero-element list. +var OneFunc = function.New(&function.Spec{ + Params: []function.Parameter{ + { + Name: "list", + Type: cty.DynamicPseudoType, + }, + }, + Type: func(args []cty.Value) (cty.Type, error) { + ty := args[0].Type() + switch { + case ty.IsListType() || ty.IsSetType(): + return ty.ElementType(), nil + case ty.IsTupleType(): + etys := ty.TupleElementTypes() + switch len(etys) { + case 0: + // No specific type information, so we'll ultimately return + // a null value of unknown type. + return cty.DynamicPseudoType, nil + case 1: + return etys[0], nil + } + } + return cty.NilType, function.NewArgErrorf(0, "must be a list, set, or tuple value with either zero or one elements") + }, + Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { + val := args[0] + ty := val.Type() + + // Our parameter spec above doesn't set AllowUnknown or AllowNull, + // so we can assume our top-level collection is both known and non-null + // in here. + + switch { + case ty.IsListType() || ty.IsSetType(): + lenVal := val.Length() + if !lenVal.IsKnown() { + return cty.UnknownVal(retType), nil + } + var l int + err := gocty.FromCtyValue(lenVal, &l) + if err != nil { + // It would be very strange to get here, because that would + // suggest that the length is either not a number or isn't + // an integer, which would suggest a bug in cty. + return cty.NilVal, fmt.Errorf("invalid collection length: %s", err) + } + switch l { + case 0: + return cty.NullVal(retType), nil + case 1: + var ret cty.Value + // We'll use an iterator here because that works for both lists + // and sets, whereas indexing directly would only work for lists. + // Since we've just checked the length, we should only actually + // run this loop body once. + for it := val.ElementIterator(); it.Next(); { + _, ret = it.Element() + } + return ret, nil + } + case ty.IsTupleType(): + etys := ty.TupleElementTypes() + switch len(etys) { + case 0: + return cty.NullVal(retType), nil + case 1: + ret := val.Index(cty.NumberIntVal(0)) + return ret, nil + } + } + return cty.NilVal, function.NewArgErrorf(0, "must be a list, set, or tuple value with either zero or one elements") + }, +}) + // SumFunc constructs a function that returns the sum of all // numbers provided in a list var SumFunc = function.New(&function.Spec{ @@ -595,6 +673,12 @@ func Matchkeys(values, keys, searchset cty.Value) (cty.Value, error) { return MatchkeysFunc.Call([]cty.Value{values, keys, searchset}) } +// One returns either the first element of a one-element list, or null +// if given a zero-element list.. +func One(list cty.Value) (cty.Value, error) { + return OneFunc.Call([]cty.Value{list}) +} + // Sum adds numbers in a list, set, or tuple func Sum(list cty.Value) (cty.Value, error) { return SumFunc.Call([]cty.Value{list}) diff --git a/lang/funcs/collection_test.go b/lang/funcs/collection_test.go index 0b61738ace..ad53cbb6cd 100644 --- a/lang/funcs/collection_test.go +++ b/lang/funcs/collection_test.go @@ -993,6 +993,287 @@ func TestMatchkeys(t *testing.T) { } } +func TestOne(t *testing.T) { + tests := []struct { + List cty.Value + Want cty.Value + Err string + }{ + { + cty.ListVal([]cty.Value{ + cty.NumberIntVal(1), + }), + cty.NumberIntVal(1), + "", + }, + { + cty.ListValEmpty(cty.Number), + cty.NullVal(cty.Number), + "", + }, + { + cty.ListVal([]cty.Value{ + cty.NumberIntVal(1), + cty.NumberIntVal(2), + cty.NumberIntVal(3), + }), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.ListVal([]cty.Value{ + cty.UnknownVal(cty.Number), + }), + cty.UnknownVal(cty.Number), + "", + }, + { + cty.ListVal([]cty.Value{ + cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number), + }), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.UnknownVal(cty.List(cty.String)), + cty.UnknownVal(cty.String), + "", + }, + { + cty.NullVal(cty.List(cty.String)), + cty.NilVal, + "argument must not be null", + }, + { + cty.ListVal([]cty.Value{ + cty.NumberIntVal(1), + }).Mark("boop"), + cty.NumberIntVal(1).Mark("boop"), + "", + }, + { + cty.ListValEmpty(cty.Bool).Mark("boop"), + cty.NullVal(cty.Bool).Mark("boop"), + "", + }, + { + cty.ListVal([]cty.Value{ + cty.NumberIntVal(1).Mark("boop"), + }), + cty.NumberIntVal(1).Mark("boop"), + "", + }, + + { + cty.SetVal([]cty.Value{ + cty.NumberIntVal(1), + }), + cty.NumberIntVal(1), + "", + }, + { + cty.SetValEmpty(cty.Number), + cty.NullVal(cty.Number), + "", + }, + { + cty.SetVal([]cty.Value{ + cty.NumberIntVal(1), + cty.NumberIntVal(2), + cty.NumberIntVal(3), + }), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.SetVal([]cty.Value{ + cty.UnknownVal(cty.Number), + }), + cty.UnknownVal(cty.Number), + "", + }, + { + cty.SetVal([]cty.Value{ + cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number), + }), + // The above would be valid if those two unknown values were + // equal known values, so this returns unknown rather than failing. + cty.UnknownVal(cty.Number), + "", + }, + { + cty.UnknownVal(cty.Set(cty.String)), + cty.UnknownVal(cty.String), + "", + }, + { + cty.NullVal(cty.Set(cty.String)), + cty.NilVal, + "argument must not be null", + }, + { + cty.SetVal([]cty.Value{ + cty.NumberIntVal(1), + }).Mark("boop"), + cty.NumberIntVal(1).Mark("boop"), + "", + }, + { + cty.SetValEmpty(cty.Bool).Mark("boop"), + cty.NullVal(cty.Bool).Mark("boop"), + "", + }, + { + cty.SetVal([]cty.Value{ + cty.NumberIntVal(1).Mark("boop"), + }), + cty.NumberIntVal(1).Mark("boop"), + "", + }, + + { + cty.TupleVal([]cty.Value{ + cty.NumberIntVal(1), + }), + cty.NumberIntVal(1), + "", + }, + { + cty.EmptyTupleVal, + cty.NullVal(cty.DynamicPseudoType), + "", + }, + { + cty.TupleVal([]cty.Value{ + cty.NumberIntVal(1), + cty.NumberIntVal(2), + cty.NumberIntVal(3), + }), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.TupleVal([]cty.Value{ + cty.UnknownVal(cty.Number), + }), + cty.UnknownVal(cty.Number), + "", + }, + { + cty.TupleVal([]cty.Value{ + cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number), + }), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.UnknownVal(cty.EmptyTuple), + // Could actually return null here, but don't for consistency with unknown lists + cty.UnknownVal(cty.DynamicPseudoType), + "", + }, + { + cty.UnknownVal(cty.Tuple([]cty.Type{cty.Bool})), + cty.UnknownVal(cty.Bool), + "", + }, + { + cty.UnknownVal(cty.Tuple([]cty.Type{cty.Bool, cty.Number})), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.NullVal(cty.EmptyTuple), + cty.NilVal, + "argument must not be null", + }, + { + cty.NullVal(cty.Tuple([]cty.Type{cty.Bool})), + cty.NilVal, + "argument must not be null", + }, + { + cty.NullVal(cty.Tuple([]cty.Type{cty.Bool, cty.Number})), + cty.NilVal, + "argument must not be null", + }, + { + cty.TupleVal([]cty.Value{ + cty.NumberIntVal(1), + }).Mark("boop"), + cty.NumberIntVal(1).Mark("boop"), + "", + }, + { + cty.EmptyTupleVal.Mark("boop"), + cty.NullVal(cty.DynamicPseudoType).Mark("boop"), + "", + }, + { + cty.TupleVal([]cty.Value{ + cty.NumberIntVal(1).Mark("boop"), + }), + cty.NumberIntVal(1).Mark("boop"), + "", + }, + + { + cty.DynamicVal, + cty.DynamicVal, + "", + }, + { + cty.NullVal(cty.DynamicPseudoType), + cty.NilVal, + "argument must not be null", + }, + { + cty.MapValEmpty(cty.String), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.EmptyObjectVal, + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.True, + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + { + cty.UnknownVal(cty.Bool), + cty.NilVal, + "must be a list, set, or tuple value with either zero or one elements", + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("one(%#v)", test.List), func(t *testing.T) { + got, err := One(test.List) + + if test.Err != "" { + if err == nil { + t.Fatal("succeeded; want error") + } else if got, want := err.Error(), test.Err; got != want { + t.Fatalf("wrong error\n got: %s\nwant: %s", got, want) + } + return + } else if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + if !test.Want.RawEquals(got) { + t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, test.Want) + } + }) + } +} + func TestSum(t *testing.T) { tests := []struct { List cty.Value diff --git a/lang/functions.go b/lang/functions.go index 3365d42b4f..aa182d17ed 100644 --- a/lang/functions.go +++ b/lang/functions.go @@ -93,6 +93,7 @@ func (s *Scope) Functions() map[string]function.Function { "md5": funcs.Md5Func, "merge": stdlib.MergeFunc, "min": stdlib.MinFunc, + "one": funcs.OneFunc, "parseint": stdlib.ParseIntFunc, "pathexpand": funcs.PathExpandFunc, "pow": stdlib.PowFunc, diff --git a/lang/functions_test.go b/lang/functions_test.go index 7c9b069fb9..c33bc3672a 100644 --- a/lang/functions_test.go +++ b/lang/functions_test.go @@ -614,6 +614,17 @@ func TestFunctions(t *testing.T) { }, }, + "one": { + { + `one([])`, + cty.NullVal(cty.DynamicPseudoType), + }, + { + `one([true])`, + cty.True, + }, + }, + "parseint": { { `parseint("100", 10)`, diff --git a/website/docs/language/functions/one.html.md b/website/docs/language/functions/one.html.md new file mode 100644 index 0000000000..0fc5918ffc --- /dev/null +++ b/website/docs/language/functions/one.html.md @@ -0,0 +1,121 @@ +--- +layout: "language" +page_title: "one - Functions - Configuration Language" +sidebar_current: "docs-funcs-collection-one" +description: |- + The 'one' function transforms a list with either zero or one elements into + either a null value or the value of the first element. +--- + +# `one` Function + +-> **Note:** This function is available only in Terraform v0.15 and later. + +`one` takes a list, set, or tuple value with either zero or one elements. +If the collection is empty, `one` returns `null`. Otherwise, `one` returns +the first element. If there are two or more elements then `one` will return +an error. + +This is a specialized function intended for the common situation where a +conditional item is represented as either a zero- or one-element list, where +a module author wishes to return a single value that might be null instead. + +For example: + +```hcl +variable "include_ec2_instance" { + type = bool + default = true +} + +resource "aws_instance" "example" { + count = var.include_ec2_instance ? 1 : 0 + + # (other resource arguments...) +} + +output "instance_ip_address" { + value = one(aws_instance.example[*].private_ip) +} +``` + +Because the `aws_instance` resource above has the `count` argument set to a +conditional that returns either zero or one, the value of +`aws_instance.example` is a list of either zero or one elements. The +`instance_ip_address` output value uses the `one` function as a concise way +to return either the private IP address of a single instance, or `null` if +no instances were created. + +## Relationship to the "Splat" Operator + +The Terraform language has a built-in operator `[*]`, known as +[the _splat_ operator](../expressions/splat.html), and one if its functions +is to translate a primitive value that might be null into a list of either +zero or one elements: + +```hcl +variable "ec2_instance_type" { + description = "The type of instance to create. If set to null, no instance will be created." + + type = string + default = null +} + +resource "aws_instance" "example" { + count = length(var.ec2_instance_type[*]) + + instance_type = var.ec2_instance_type + # (other resource arguments...) +} + +output "instance_ip_address" { + value = one(aws_instance.example[*].private_ip) +} +``` + +In this case we can see that the `one` function is, in a sense, the opposite +of applying `[*]` to a primitive-typed value. Splat can convert a possibly-null +value into a zero-or-one list, and `one` can reverse that to return to a +primitive value that might be null. + +## Examples + +``` +> one([]) +null +> one(["hello"]) +"hello" +> one(["hello", "goodbye"]) + +Error: Invalid function argument + +Invalid value for "list" parameter: must be a list, set, or tuple value with +either zero or one elements. +``` + +### Using `one` with sets + +The `one` function can be particularly helpful in situations where you have a +set that you know has only zero or one elements. Set values don't support +indexing, so it's not valid to write `var.set[0]` to extract the "first" +element of a set, but if you know that there's only one item then `one` can +isolate and return that single item: + +``` +> one(toset([])) +null +> one(toset(["hello"])) +"hello" +``` + +Don't use `one` with sets that might have more than one element. This function +will fail in that case: + +``` +> one(toset(["hello","goodbye"])) + +Error: Invalid function argument + +Invalid value for "list" parameter: must be a list, set, or tuple value with +either zero or one elements. +``` diff --git a/website/layouts/language.erb b/website/layouts/language.erb index 2e0e12b5fd..f7ac17820d 100644 --- a/website/layouts/language.erb +++ b/website/layouts/language.erb @@ -523,6 +523,10 @@ merge +
  • + one +
  • +
  • range
  • From b515ab583a50b88c08210e0177fed44f9eade089 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Apr 2021 18:42:15 -0400 Subject: [PATCH 05/38] make blocktoattr an hcldec.UnknownBody This will allow any dynamic blocks that are fixed up as a blocktoattr still decode into an unknown value. --- lang/blocktoattr/fixup.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lang/blocktoattr/fixup.go b/lang/blocktoattr/fixup.go index 0af708ec40..056946ab99 100644 --- a/lang/blocktoattr/fixup.go +++ b/lang/blocktoattr/fixup.go @@ -41,6 +41,17 @@ type fixupBody struct { names map[string]struct{} } +type unknownBlock interface { + Unknown() bool +} + +func (b *fixupBody) Unknown() bool { + if u, ok := b.original.(unknownBlock); ok { + return u.Unknown() + } + return false +} + // Content decodes content from the body. The given schema must be the lower-level // representation of the same schema that was previously passed to FixUpBlockAttrs, // or else the result is undefined. From 5f5432e8ea443daa1a77eb9abd65445d3583f6c8 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 14 Apr 2021 06:04:40 -0700 Subject: [PATCH 06/38] website: v0.15 upgrade guide for sensitive resource attributes (#28355) * website: v0.15 upgrade guide for sensitive resource attributes Our earlier draft of this guide didn't include a section about the stabilization of the "provider_sensitive_attrs" language experiment. This new section aims to address the situation where a module might previously have been returning a sensitive value without having marked it as such, and thus that module will begin returning an error after upgrading to Terraform v0.15. As part of that, I also reviewed the existing documentation about these features and made some edits aiming to make these four different sections work well together if users refer to them all at once, as they are likely to do if they follow the new links from the upgrade guide. I aimed to retain all of the content we had before, but some of it is now in a new location. In particular, I moved the discussion about the v0.14 language experiment into the upgrade guide, because it seems like a topic only really relevant to those upgrading from an earlier version and not something folks need to know about if they are using Terraform for the first time in v0.15 or later. * minor fixups Co-authored-by: Kristin Laemmert --- .../language/expressions/references.html.md | 71 +++++----- website/docs/language/values/outputs.html.md | 23 ++-- .../docs/language/values/variables.html.md | 47 ++++--- website/upgrade-guides/0-15.html.markdown | 122 ++++++++++++++++++ 4 files changed, 191 insertions(+), 72 deletions(-) diff --git a/website/docs/language/expressions/references.html.md b/website/docs/language/expressions/references.html.md index 2b1bb7366e..f69420f928 100644 --- a/website/docs/language/expressions/references.html.md +++ b/website/docs/language/expressions/references.html.md @@ -277,6 +277,36 @@ Note that unlike `count`, splat expressions are _not_ directly applicable to res * `values(aws_instance.example)[*].id` +### Sensitive Resource Attributes + +When defining the schema for a resource type, a provider developer can mark +certain attributes as _sensitive_, in which case Terraform will show a +placeholder marker `(sensitive)` instead of the actual value when rendering +a plan involving that attribute. + +A provider attribute marked as sensitive behaves similarly to an +[an input variable declared as sensitive](/docs/language/values/variables.html#suppressing-values-in-cli-output), +where Terraform will hide the value in the plan and apply messages and will +also hide any other values you derive from it as sensitive. +However, there are some limitations to that behavior as described in +[Cases where Terraform may disclose a sensitive variable](/docs/language/values/variables.html#cases-where-terraform-may-disclose-a-sensitive-variable). + +If you use a sensitive value from a resource attribute as part of an +[output value](/docs/language/values/outputs.html) then Terraform will require +you to also mark the output value itself as sensitive, to confirm that you +intended to export it. + +Terraform will still record sensitive values in the [state](/docs/language/state/index.html), +and so anyone who can access the state data will have access to the sensitive +values in cleartext. For more information, see +[_Sensitive Data in State_](/docs/language/state/sensitive-data.html). + +-> **Note:** Treating values derived from a sensitive resource attribute as +sensitive themselves was introduced in Terraform v0.15. Earlier versions of +Terraform will obscure the direct value of a sensitive resource attribute, +but will _not_ automatically obscure other values derived from sensitive +resource attributes. + ### Values Not Yet Known When Terraform is planning a set of changes that will apply your configuration, @@ -317,44 +347,3 @@ effect: until the apply phase, causing the apply to fail. Unknown values appear in the `terraform plan` output as `(not yet known)`. - -### Sensitive Resource Attributes - -When defining the schema for a resource type, a provider developer can mark -certain attributes as _sensitive_, in which case Terraform will show a -placeholder marker `(sensitive)` instead of the actual value when rendering -a plan involving that attribute. - -The treatment of these particular sensitive values is currently different than -for values in -[input variables](/docs/language/values/variables.html) -and -[output values](/docs/language/values/outputs.html) -that have `sensitive = true` set. Sensitive resource attributes will be -obscured in the plan when they appear directly, but other values that you -_derive_ from a sensitive resource attribute will not themselves be considered -sensitive, and so Terraform will include those derived values in its output -without redacting them. - -Terraform v0.15.0 and later treats resource attributes that are marked as -sensitive (by the provider) in the same way as sensitive input variables and -output values, so that Terraform will consider any derived values as sensitive too. - -If you are using Terraform v0.14.x, this feature is considered experimental. -You can activate that experiment for your module using the -`provider_sensitive_attrs` experiment keyword: - -```hcl -terraform { - experiments = [provider_sensitive_attrs] -} -``` - -The behavior of this experiment might change even in future patch releases of -Terraform, so we don't recommend using this experiment in modules you use -to describe production infrastructure. - -If you enable this experiment and you have exported any sensitive resource -attributes via your module's output values then you will see an error unless -you also mark the output value as `sensitive = true`, confirming your intent -to export it. diff --git a/website/docs/language/values/outputs.html.md b/website/docs/language/values/outputs.html.md index 60450f83d0..35b8aa4413 100644 --- a/website/docs/language/values/outputs.html.md +++ b/website/docs/language/values/outputs.html.md @@ -102,9 +102,10 @@ output "db_password" { } ``` -Setting an output value as sensitive prevents Terraform from showing its value -in `plan` and `apply`. In the following scenario, our root module has an output declared as sensitive -and a module call with a sensitive output, which we then use in a resource attribute. +Terraform will hide values marked as sensitive in the messages from +`terraform plan` and `terraform apply`. In the following scenario, our root +module has an output declared as sensitive and a module call with a +sensitive output, which we then use in a resource attribute. ```hcl # main.tf @@ -130,11 +131,9 @@ output "a" { } ``` -When we run a `plan` or `apply`, the sensitive value is redacted from output: +When we run a plan or apply, the sensitive value is redacted from output: ``` -# CLI output - Terraform will perform the following actions: # test_instance.x will be created @@ -148,11 +147,15 @@ Changes to Outputs: + out = (sensitive value) ``` --> **Note:** In Terraform versions prior to Terraform 0.14, setting an output value in the root module as sensitive would prevent Terraform from showing its value in the list of outputs at the end of `terraform apply`. However, the value could still display in the CLI output for other reasons, like if the value is referenced in an expression for a resource argument. +-> **Note:** In Terraform versions prior to Terraform 0.14, setting an output +value in the root module as sensitive would prevent Terraform from showing its +value in the list of outputs at the end of `terraform apply`. However, the +value could still display in the CLI output for other reasons, like if the +value is referenced in an expression for a resource argument. -Sensitive output values are still recorded in the -[state](/docs/language/state/index.html), and so will be visible to anyone who is able -to access the state data. For more information, see +Terraform will still record sensitive values in the [state](/docs/language/state/index.html), +and so anyone who can access the state data will have access to the sensitive +values in cleartext. For more information, see [_Sensitive Data in State_](/docs/language/state/sensitive-data.html). diff --git a/website/docs/language/values/variables.html.md b/website/docs/language/values/variables.html.md index f911b0406c..3c167eb1df 100644 --- a/website/docs/language/values/variables.html.md +++ b/website/docs/language/values/variables.html.md @@ -211,13 +211,16 @@ using a sentence structure similar to the above examples. > **Hands-on:** Try the [Protect Sensitive Input Variables](https://learn.hashicorp.com/tutorials/terraform/sensitive-variables?in=terraform/configuration-language&utm_source=WEBSITE&utm_medium=WEB_IO&utm_offer=ARTICLE_PAGE&utm_content=DOCS) tutorial on HashiCorp Learn. -Setting a variable as `sensitive` prevents Terraform from showing its value in the `plan` or `apply` output, when that variable is used within a configuration. +Setting a variable as `sensitive` prevents Terraform from showing its value in +the `plan` or `apply` output, when you use that variable elsewhere in your +configuration. -Sensitive values are still recorded in the [state](/docs/language/state/index.html), and so will be visible to anyone who is able to access the state data. For more information, see [_Sensitive Data in State_](/docs/language/state/sensitive-data.html). +Terraform will still record sensitive values in the [state](/docs/language/state/index.html), +and so anyone who can access the state data will have access to the sensitive +values in cleartext. For more information, see +[_Sensitive Data in State_](/docs/language/state/sensitive-data.html). -A provider can define [an attribute as sensitive](/docs/extend/best-practices/sensitive-state.html#using-the-sensitive-flag), which prevents the value of that attribute from being displayed in logs or regular output. The `sensitive` argument on variables allows users to replicate this behavior for values in their configuration, by defining a variable as `sensitive`. - -Define a variable as sensitive by setting the `sensitive` argument to `true`: +Declare a variable as sensitive by setting the `sensitive` argument to `true`: ``` variable "user_information" { @@ -234,7 +237,9 @@ resource "some_resource" "a" { } ``` -Using this variable throughout your configuration will obfuscate the value from display in `plan` or `apply` output: +Any expressions whose result depends on the sensitive variable will be treated +as sensitive themselves, and so in the above example the two arguments of +`resource "some_resource" "a"` will also be hidden in the plan output: ``` Terraform will perform the following actions: @@ -248,22 +253,12 @@ Terraform will perform the following actions: Plan: 1 to add, 0 to change, 0 to destroy. ``` -In some cases where a sensitive variable is used in a nested block, the whole block can be redacted. This happens with resources which can have multiple blocks of the same type, where the values must be unique. This looks like: +In some cases where you use a sensitive variable inside a nested block, Terraform +may treat the entire block as redacted. This happens for resource types where +all of the blocks of a particular type are required to be unique, and so +disclosing the content of one block might imply the content of a sibling block. ``` -# main.tf - -resource "some_resource" "a" { - nested_block { - user_information = var.user_information # a sensitive variable - other_information = "not sensitive data" - } -} - -# CLI output - -Terraform will perform the following actions: - # some_resource.a will be updated in-place ~ resource "some_resource" "a" { ~ nested_block { @@ -271,9 +266,19 @@ Terraform will perform the following actions: # so its contents will not be displayed. } } - ``` +A provider can also +[declare an attribute as sensitive](/docs/extend/best-practices/sensitive-state.html#using-the-sensitive-flag), +which will cause Terraform to hide it from regular output regardless of how +you assign it a value. For more information, see +[Sensitive Resource Attributes](/docs/language/expressions/references.html#sensitive-resource-attributes). + +If you use a sensitive value from as part of an +[output value](/docs/language/values/outputs.html) then Terraform will require +you to also mark the output value itself as sensitive, to confirm that you +intended to export it. + #### Cases where Terraform may disclose a sensitive variable A `sensitive` variable is a configuration-centered concept, and values are sent to providers without any obfuscation. A provider error could disclose a value if that value is included in the error message. For example, a provider might return the following error even if "foo" is a sensitive value: `"Invalid value 'foo' for field"` diff --git a/website/upgrade-guides/0-15.html.markdown b/website/upgrade-guides/0-15.html.markdown index aa79773a04..d8de73a3be 100644 --- a/website/upgrade-guides/0-15.html.markdown +++ b/website/upgrade-guides/0-15.html.markdown @@ -46,12 +46,134 @@ may be able to reproduce it and offer advice. Upgrade guide sections: +* [Sensitive Output Values](#sensitive-output-values) * [Legacy Configuration Language Features](#legacy-configuration-language-features) * [Alternative (Aliased) Provider Configurations Within Modules](#alternative-provider-configurations-within-modules) * [Commands Accepting a Configuration Directory Argument](#commands-accepting-a-configuration-directory-argument) * [Microsoft Windows Terminal Support](#microsoft-windows-terminal-support) * [Other Minor Command Line Behavior Changes](#other-minor-command-line-behavior-changes) +## Sensitive Output Values + +Terraform v0.14 previously introduced the ability for Terraform to track and +propagate the "sensitivity" of values through expressions that include +references to sensitive input variables and output values. For example: + +```hcl +variable "example" { + type = string + sensitive = true +} + +resource "example" "example" { + # The following value is also treated as sensitive, because it's derived + # from var.example. + name = "foo-${var.example}" +} +``` + +As part of that feature, Terraform also began requiring you to mark an output +value as sensitive if its definition includes any sensitive values itself: + +```hcl +output "example" { + value = "foo-${var.example}" + + # Must mark this output value as sensitive, because it's derived from + # var.example that is declared as sensitive. + sensitive = true +} +``` + +Terraform v0.15 extends this mechanism to also work for values derived from +resource attributes that the provider has declared as being sensitive. +Provider developers will typically mark an attribute as sensitive if the +remote system has documented the corresponding field as being sensitive, such +as if the attribute always contains a password or a private key. + +As a result of that, after upgrading to Terraform v0.15 you may find that +Terraform now reports some of your output values as invalid, if they were +derived from sensitive attributes without also being marked as sensitive: + +``` +╷ +│ Error: Output refers to sensitive values +│ +│ on sensitive-resource-attr.tf line 5: +│ 5: output "private_key" { +│ +│ Expressions used in outputs can only refer to sensitive values if the +│ sensitive attribute is true. +╵ +``` + +If you were intentionally exporting a sensitive value, you can address the +error by adding an explicit declaration `sensitive = true` to the output +value declaration: + +```hcl +output "private_key" { + value = tls_private_key.example.private_key_pem + sensitive = true +} +``` + +With that addition, if this output value was a root module output value then +Terraform will hide its value in the `terraform plan` and `terraform apply` +output: + +``` +Changes to Outputs: + + private_key = (sensitive value) +``` + +-> **Note:** The declaration of an output value as sensitive must be made +within the module that declares the output, so if you depend on a third-party +module that has a sensitive output value that is lacking this declaration then +you will need to wait for a new version of that module before you can upgrade +to Terraform v0.15. + +The value is only hidden in the main UI, and so the sensitive value +will still be recorded in the state. If you declared this output value in order +to use it as part of integration with other software, you can still retrieve +the cleartext value using commands intended for machine rather than human +consumption, such as `terraform output -json` or `terraform output -raw`: + +```shellsession +$ terraform output -raw private_key +-----BEGIN RSA PRIVATE KEY----- +MIIEowIBAAKCAQEAoahsvJ1rIxTIOOmJZ7yErs5eOq/Kv9+5l3h0LbxW78K8//Kb +OMU3v8F3h8jp+AB/1zGr5UBYfnYp5ncJm/OTCXLFAHxGibEbRnf1m2A3o0hEaWsw +# (etc...) +``` + +If you consider Terraform's treatment of a sensitive value to be too +conservative and you'd like to force Terraform to treat a sensitive value as +non-sensitive, you can use +[the `nonsensitive` function](/docs/language/functions/nonsensitive.html) to +override Terraform's automatic detection: + +```hcl +output "private_key" { + # WARNING: Terraform will display this result as cleartext + value = nonsensitive(tls_private_key.example.private_key_pem) +} +``` + +For more information on the various situations where sensitive values can +originate in Terraform, refer to the following sections: + +* [Sensitive Input Variables](/docs/language/values/variables.html#suppressing-values-in-cli-output) +* [Sensitive Resource Attributes](/docs/language/expressions/references.html#sensitive-resource-attributes) +* [Sensitive Output Values](/docs/language/values/outputs.html#sensitive) + +-> **Note:** The new behavior described in this section was previously +available in Terraform v0.14 as the +[language experiment](/docs/language/settings/#experimental-language-features) +`provider_sensitive_attrs`. That experiment has now concluded, so if you were +participating in that experiment then you'll now need to remove the experiment +opt-in from your module as part of upgrading to Terraform v0.15. + ## Legacy Configuration Language Features Terraform v0.12 introduced new syntax for a variety of existing Terraform From e4031eaccf1555f853bbeb5aafdf50fc5902f6a6 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Wed, 14 Apr 2021 09:30:49 -0400 Subject: [PATCH 07/38] command: Add a newline before confirming apply This blank line delineating the plan and the query was accidentally dropped as part of the views migration. --- backend/local/backend_apply.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 1817dcb40f..4571041071 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -107,7 +107,7 @@ func (b *Local) opApply( v, err := op.UIIn.Input(stopCtx, &terraform.InputOpts{ Id: "approve", - Query: query, + Query: "\n" + query, Description: desc, }) if err != nil { From 59c828137891cfe39c81a55493f28ac2400a2b92 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Apr 2021 18:43:32 -0400 Subject: [PATCH 08/38] use new dynamic decoding behavior Dynamic blocks with unknown for_each expressions are now decoded into an unknown value rather than using a sentinel object with unknown and null attributes. This will allow providers to precisely plan the block values, rather than trying to heuristically paper over the incorrect plans when dynamic is in use. --- plans/objchange/compatible.go | 114 +--------------------- plans/objchange/compatible_test.go | 147 +++-------------------------- plans/objchange/objchange.go | 8 +- 3 files changed, 25 insertions(+), 244 deletions(-) diff --git a/plans/objchange/compatible.go b/plans/objchange/compatible.go index 0ef4b01e76..e4ece3f976 100644 --- a/plans/objchange/compatible.go +++ b/plans/objchange/compatible.go @@ -75,20 +75,12 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu plannedV, _ := planned.GetAttr(name).Unmark() actualV, _ := actual.GetAttr(name).Unmark() - // As a special case, if there were any blocks whose leaf attributes - // are all unknown then we assume (possibly incorrectly) that the - // HCL dynamic block extension is in use with an unknown for_each - // argument, and so we will do looser validation here that allows - // for those blocks to have expanded into a different number of blocks - // if the for_each value is now known. - maybeUnknownBlocks := couldHaveUnknownBlockPlaceholder(plannedV, blockS, false) - path := append(path, cty.GetAttrStep{Name: name}) switch blockS.Nesting { case configschema.NestingSingle, configschema.NestingGroup: // If an unknown block placeholder was present then the placeholder // may have expanded out into zero blocks, which is okay. - if maybeUnknownBlocks && actualV.IsNull() { + if !plannedV.IsKnown() && actualV.IsNull() { continue } moreErrs := assertObjectCompatible(&blockS.Block, plannedV, actualV, path) @@ -102,14 +94,6 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu continue } - if maybeUnknownBlocks { - // When unknown blocks are present the final blocks may be - // at different indices than the planned blocks, so unfortunately - // we can't do our usual checks in this case without generating - // false negatives. - continue - } - plannedL := plannedV.LengthInt() actualL := actualV.LengthInt() if plannedL != actualL { @@ -144,7 +128,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu moreErrs := assertObjectCompatible(&blockS.Block, plannedEV, actualEV, append(path, cty.GetAttrStep{Name: k})) errs = append(errs, moreErrs...) } - if !maybeUnknownBlocks { // new blocks may appear if unknown blocks were present in the plan + if plannedV.IsKnown() { // new blocks may appear if unknown blocks were present in the plan for k := range actualAtys { if _, ok := plannedAtys[k]; !ok { errs = append(errs, path.NewErrorf("new block key %q has appeared", k)) @@ -158,7 +142,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu } plannedL := plannedV.LengthInt() actualL := actualV.LengthInt() - if plannedL != actualL && !maybeUnknownBlocks { // new blocks may appear if unknown blocks were persent in the plan + if plannedL != actualL && plannedV.IsKnown() { // new blocks may appear if unknown blocks were persent in the plan errs = append(errs, path.NewErrorf("block count changed from %d to %d", plannedL, actualL)) continue } @@ -177,7 +161,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu continue } - if maybeUnknownBlocks { + if !plannedV.IsKnown() { // When unknown blocks are present the final number of blocks // may be different, either because the unknown set values // become equal and are collapsed, or the count is unknown due @@ -328,96 +312,6 @@ func indexStrForErrors(v cty.Value) string { } } -// couldHaveUnknownBlockPlaceholder is a heuristic that recognizes how the -// HCL dynamic block extension behaves when it's asked to expand a block whose -// for_each argument is unknown. In such cases, it generates a single placeholder -// block with all leaf attribute values unknown, and once the for_each -// expression becomes known the placeholder may be replaced with any number -// of blocks, so object compatibility checks would need to be more liberal. -// -// Set "nested" if testing a block that is nested inside a candidate block -// placeholder; this changes the interpretation of there being no blocks of -// a type to allow for there being zero nested blocks. -func couldHaveUnknownBlockPlaceholder(v cty.Value, blockS *configschema.NestedBlock, nested bool) bool { - switch blockS.Nesting { - case configschema.NestingSingle, configschema.NestingGroup: - if nested && v.IsNull() { - return true // for nested blocks, a single block being unset doesn't disqualify from being an unknown block placeholder - } - return couldBeUnknownBlockPlaceholderElement(v, blockS) - default: - // These situations should be impossible for correct providers, but - // we permit the legacy SDK to produce some incorrect outcomes - // for compatibility with its existing logic, and so we must be - // tolerant here. - if !v.IsKnown() { - return true - } - if v.IsNull() { - return false // treated as if the list were empty, so we would see zero iterations below - } - - // Unmark before we call ElementIterator in case this iterable is marked sensitive. - // This can arise in the case where a member of a Set is sensitive, and thus the - // whole Set is marked sensitive - v, _ := v.Unmark() - // For all other nesting modes, our value should be something iterable. - for it := v.ElementIterator(); it.Next(); { - _, ev := it.Element() - if couldBeUnknownBlockPlaceholderElement(ev, blockS) { - return true - } - } - - // Our default changes depending on whether we're testing the candidate - // block itself or something nested inside of it: zero blocks of a type - // can never contain a dynamic block placeholder, but a dynamic block - // placeholder might contain zero blocks of one of its own nested block - // types, if none were set in the config at all. - return nested - } -} - -func couldBeUnknownBlockPlaceholderElement(v cty.Value, schema *configschema.NestedBlock) bool { - if v.IsNull() { - return false // null value can never be a placeholder element - } - if !v.IsKnown() { - return true // this should never happen for well-behaved providers, but can happen with the legacy SDK opt-outs - } - for name := range schema.Attributes { - av := v.GetAttr(name) - - // Unknown block placeholders contain only unknown or null attribute - // values, depending on whether or not a particular attribute was set - // explicitly inside the content block. Note that this is imprecise: - // non-placeholders can also match this, so this function can generate - // false positives. - if av.IsKnown() && !av.IsNull() { - - // FIXME: only required for the legacy SDK, but we don't have a - // separate codepath to switch the comparisons, and we still want - // the rest of the checks from AssertObjectCompatible to apply. - // - // The legacy SDK cannot handle missing strings from set elements, - // and will insert an empty string into the planned value. - // Skipping these treats them as null values in this case, - // preventing false alerts from AssertObjectCompatible. - if schema.Nesting == configschema.NestingSet && av.Type() == cty.String && av.AsString() == "" { - continue - } - - return false - } - } - for name, blockS := range schema.BlockTypes { - if !couldHaveUnknownBlockPlaceholder(v.GetAttr(name), blockS, true) { - return false - } - } - return true -} - // assertSetValuesCompatible checks that each of the elements in a can // be correlated with at least one equivalent element in b and vice-versa, // using the given correlation function. diff --git a/plans/objchange/compatible_test.go b/plans/objchange/compatible_test.go index 678ce8ce34..942a477eaa 100644 --- a/plans/objchange/compatible_test.go +++ b/plans/objchange/compatible_test.go @@ -30,10 +30,6 @@ func TestAssertObjectCompatible(t *testing.T) { "foo": cty.StringVal("bar"), "bar": cty.NullVal(cty.String), // simulating the situation where bar isn't set in the config at all }) - fooBarBlockDynamicPlaceholder := cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - "bar": cty.NullVal(cty.String), // simulating the situation where bar isn't set in the config at all - }) tests := []struct { Schema *configschema.Block @@ -919,13 +915,11 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ObjectVal(map[string]cty.Value{ - // One wholly unknown block is what "dynamic" blocks - // generate when the for_each expression is unknown. - "foo": cty.UnknownVal(cty.String), + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "key": cty.Object(map[string]cty.Type{ + "foo": cty.String, }), - }), + })), cty.ObjectVal(map[string]cty.Value{ "key": cty.NullVal(cty.Object(map[string]cty.Type{ "foo": cty.String, @@ -1011,11 +1005,9 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBarBlockDynamicPlaceholder, // the presence of this disables some of our checks - }), - }), + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "key": cty.List(fooBarBlockValue.Type()), + })), cty.ObjectVal(map[string]cty.Value{ "key": cty.ListVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ @@ -1026,35 +1018,7 @@ func TestAssertObjectCompatible(t *testing.T) { }), }), }), - nil, // a single block whose attrs are all unknown is allowed to expand into multiple, because that's how dynamic blocks behave when for_each is unknown - }, - { - &configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "key": { - Nesting: configschema.NestingList, - Block: schemaWithFooBar, - }, - }, - }, - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBarBlockValue, // the presence of one static block does not negate that the following element looks like a dynamic placeholder - fooBarBlockDynamicPlaceholder, // the presence of this disables some of our checks - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "key": cty.ListVal([]cty.Value{ - fooBlockValue, - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("hello"), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("world"), - }), - }), - }), - nil, // as above, the presence of a block whose attrs are all unknown indicates dynamic block expansion, so our usual count checks don't apply + nil, // an unknown block is allowed to expand into multiple, because that's how dynamic blocks behave when for_each is unknown }, { &configschema.Block{ @@ -1195,14 +1159,11 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), + "block": cty.UnknownVal(cty.Set( + cty.Object(map[string]cty.Type{ + "foo": cty.String, }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + )), }), cty.ObjectVal(map[string]cty.Value{ "block": cty.SetVal([]cty.Value{ @@ -1221,47 +1182,6 @@ func TestAssertObjectCompatible(t *testing.T) { // indicates this may be a dynamic block, and the length is unknown nil, }, - { - &configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "block": { - Nesting: configschema.NestingSet, - Block: schemaWithFooBar, - }, - }, - }, - // The legacy SDK cannot handle missing strings in sets, and will - // insert empty strings to the planned value. Empty strings should - // be handled as nulls, and this object should represent a possible - // dynamic block. - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - "bar": cty.StringVal(""), - }), - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("hello"), - "bar": cty.StringVal(""), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("world"), - "bar": cty.StringVal(""), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("nope"), - "bar": cty.StringVal(""), - }), - }), - }), - // there is no error here, because the presence of unknowns - // indicates this may be a dynamic block, and the length is unknown - nil, - }, { &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{ @@ -1335,11 +1255,7 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + "block": cty.UnknownVal(cty.Set(fooBlockValue.Type())), }), cty.ObjectVal(map[string]cty.Value{ "block": cty.SetVal([]cty.Value{ @@ -1364,11 +1280,7 @@ func TestAssertObjectCompatible(t *testing.T) { }, }, cty.ObjectVal(map[string]cty.Value{ - "block2": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - }), - }), + "block2": cty.UnknownVal(cty.Set(fooBlockValue.Type())), }), cty.ObjectVal(map[string]cty.Value{ "block2": cty.SetValEmpty(cty.Object(map[string]cty.Type{ @@ -1406,37 +1318,6 @@ func TestAssertObjectCompatible(t *testing.T) { }), nil, }, - { - &configschema.Block{ - BlockTypes: map[string]*configschema.NestedBlock{ - "block": { - Nesting: configschema.NestingSet, - Block: schemaWithFooBar, - }, - }, - }, - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.UnknownVal(cty.String), - "bar": cty.NullVal(cty.String), - }), - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "block": cty.SetVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("a"), - "bar": cty.StringVal(""), - }), - cty.ObjectVal(map[string]cty.Value{ - "foo": cty.StringVal("b"), - "bar": cty.StringVal(""), - }), - }), - }), - nil, - }, { &configschema.Block{ BlockTypes: map[string]*configschema.NestedBlock{ diff --git a/plans/objchange/objchange.go b/plans/objchange/objchange.go index fade668c28..6bfc06e22c 100644 --- a/plans/objchange/objchange.go +++ b/plans/objchange/objchange.go @@ -93,6 +93,12 @@ func proposedNew(schema *configschema.Block, prior, config cty.Value) cty.Value } func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty.Value) cty.Value { + // The only time we should encounter an entirely unknown block is from the + // use of dynamic with an unknown for_each expression. + if !config.IsKnown() { + return config + } + var newV cty.Value switch schema.Nesting { @@ -103,7 +109,7 @@ func proposedNewNestedBlock(schema *configschema.NestedBlock, prior, config cty. case configschema.NestingList: // Nested blocks are correlated by index. configVLen := 0 - if config.IsKnown() && !config.IsNull() { + if !config.IsNull() { configVLen = config.LengthInt() } if configVLen > 0 { From 2390a11d603b703c02ce51b717ab19bebb188d6d Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Thu, 15 Apr 2021 07:30:50 -0400 Subject: [PATCH 09/38] core: Fix double-marked sensitive attributes We need to unmark the decoded state and merge the marks with those from the resource schema. Co-authored-by: James Bardin --- terraform/context_apply2_test.go | 78 ++++++++++++++++++++++++++++++++ terraform/evaluate.go | 16 +++---- terraform/evaluate_test.go | 4 +- 3 files changed, 88 insertions(+), 10 deletions(-) diff --git a/terraform/context_apply2_test.go b/terraform/context_apply2_test.go index 973a103966..dd028726cc 100644 --- a/terraform/context_apply2_test.go +++ b/terraform/context_apply2_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/states" "github.com/zclconf/go-cty/cty" @@ -367,3 +368,80 @@ resource "aws_instance" "bin" { } } + +func TestContext2Apply_additionalSensitiveFromState(t *testing.T) { + // Ensure we're not trying to double-mark values decoded from state + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "secret" { + sensitive = true + default = ["secret"] +} + +resource "test_resource" "a" { + sensitive_attr = var.secret +} + +resource "test_resource" "b" { + value = test_resource.a.id +} +`, + }) + + p := new(MockProvider) + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "value": { + Type: cty.String, + Optional: true, + }, + "sensitive_attr": { + Type: cty.List(cty.String), + Optional: true, + Sensitive: true, + }, + }, + }, + }, + }) + + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustResourceInstanceAddr(`test_resource.a`), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"id":"a","sensitive_attr":["secret"]}`), + AttrSensitivePaths: []cty.PathValueMarks{ + { + Path: cty.GetAttrPath("sensitive_attr"), + Marks: cty.NewValueMarks("sensitive"), + }, + }, + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + }) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + State: state, + }) + + _, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +} diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 0e5da5e005..92e50614d7 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -782,9 +782,15 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc val := ios.Value - // If our schema contains sensitive values, mark those as sensitive + // If our schema contains sensitive values, mark those as sensitive. + // Since decoding the instance object can also apply sensitivity marks, + // we must remove and combine those before remarking to avoid a double- + // mark error. if schema.ContainsSensitive() { - val = markProviderSensitiveAttributes(schema, val) + var marks []cty.PathValueMarks + val, marks = val.UnmarkDeepWithPaths() + marks = append(marks, getValMarks(schema, val, nil)...) + val = val.MarkWithPaths(marks) } instances[key] = val } @@ -954,12 +960,6 @@ func moduleDisplayAddr(addr addrs.ModuleInstance) string { } } -// markProviderSensitiveAttributes returns an updated value -// where attributes that are Sensitive are marked -func markProviderSensitiveAttributes(schema *configschema.Block, val cty.Value) cty.Value { - return val.MarkWithPaths(getValMarks(schema, val, nil)) -} - func getValMarks(schema *configschema.Block, val cty.Value, path cty.Path) []cty.PathValueMarks { var pvm []cty.PathValueMarks for name, attrS := range schema.Attributes { diff --git a/terraform/evaluate_test.go b/terraform/evaluate_test.go index 3061df5f7e..c25429e930 100644 --- a/terraform/evaluate_test.go +++ b/terraform/evaluate_test.go @@ -563,7 +563,7 @@ func evaluatorForModule(stateSync *states.SyncState, changesSync *plans.ChangesS } } -func TestMarkProviderSensitive(t *testing.T) { +func TestGetValMarks(t *testing.T) { schema := &configschema.Block{ Attributes: map[string]*configschema.Attribute{ "unsensitive": { @@ -647,7 +647,7 @@ func TestMarkProviderSensitive(t *testing.T) { }, } { t.Run(fmt.Sprintf("%#v", tc.given), func(t *testing.T) { - got := markProviderSensitiveAttributes(schema, tc.given) + got := tc.given.MarkWithPaths(getValMarks(schema, tc.given, nil)) if !got.RawEquals(tc.expect) { t.Fatalf("\nexpected: %#v\ngot: %#v\n", tc.expect, got) } From 035d1648e4bdd466dd4013de785afccd9506e224 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 15 Apr 2021 09:58:49 -0700 Subject: [PATCH 10/38] website: Link to the v0.15 upgrade guide Unfortunately it seems that this link got lost in a merge conflict when we did the big nav refactor earlier in the v0.15 cycle, so here we'll retoractively add it to the new location for upgrade guide nav, in the language layout rather than the downloads layout. --- website/layouts/language.erb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/website/layouts/language.erb b/website/layouts/language.erb index f7ac17820d..b6e0033e28 100644 --- a/website/layouts/language.erb +++ b/website/layouts/language.erb @@ -971,6 +971,10 @@ Overview +
  • + Upgrading to v0.15 +
  • +
  • Upgrading to v0.14
  • From dedac2cdd682d471dc87e31c4c0b1dcc0e308c46 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 15 Apr 2021 10:12:56 -0700 Subject: [PATCH 11/38] website: v0.15 Upgrade Guide entry for Azure Backend arguments Terraform v0.15 includes the conclusion of the deprecation cycle for some renamed arguments in the "azure" backend. We missed this on the first draft of the upgrade guide because this change arrived along with various other more innocuous updates and so we didn't spot it during our change review. --- website/upgrade-guides/0-15.html.markdown | 42 +++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/website/upgrade-guides/0-15.html.markdown b/website/upgrade-guides/0-15.html.markdown index d8de73a3be..33044891da 100644 --- a/website/upgrade-guides/0-15.html.markdown +++ b/website/upgrade-guides/0-15.html.markdown @@ -52,6 +52,7 @@ Upgrade guide sections: * [Commands Accepting a Configuration Directory Argument](#commands-accepting-a-configuration-directory-argument) * [Microsoft Windows Terminal Support](#microsoft-windows-terminal-support) * [Other Minor Command Line Behavior Changes](#other-minor-command-line-behavior-changes) +* [Azure Backend `arm_`-prefixed Arguments](#azure-backend-removed-arguments) ## Sensitive Output Values @@ -493,3 +494,44 @@ cleanup of obsolete features and improved consistency: If you are using `-force` in an automated call to `terraform destroy`, change to using `-auto-approve` instead. + +## Azure Backend Removed Arguments + +In an earlier release the `azure` backend changed to remove the `arm_` prefix +from a number of the configuration arguments: + +| Old Name | New Name | +|-----------------------|-------------------| +| `arm_client_id` | `client_id` | +| `arm_client_secret` | `client_secret` | +| `arm_subscription_id` | `subscription_id` | +| `arm_tenant_id` | `tenant_id` | + +The old names were previously deprecated, but we've removed them altogether +in Terraform v0.15 in order to conclude that deprecation cycle. + +If you have a backend configuration using the old names then you may see +errors like the following when upgrading to Terraform v0.15: + +``` +╷ +│ Error: Invalid backend configuration argument +│ +│ The backend configuration argument "arm_client_id" given on +│ the command line is not expected for the selected backend type. +╵ +``` + +If you see errors like this, rename the arguments in your backend configuration +as shown in the table above and then run the following to re-initialize your +backend configuration: + +``` +terraform init -reconfigure +``` + +The `-reconfigure` argument instructs Terraform to just replace the old +configuration with the new configuration directly, rather than offering to +migrate the latest state snapshots from the old to the new configuration. +Migration would not be appropriate in this case because the old and new +configurations are equivalent and refer to the same remote objects. From 3e49c4b388ad6ffdcdcde7203b75a6a88a8707c6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 15 Apr 2021 17:17:36 -0400 Subject: [PATCH 12/38] MinItems and MaxItems can be validated once again The new hcldec dynamic block behavior no longer tried to validate MinItems and MaxItems when the number of values is unknown. --- configs/configschema/decoder_spec.go | 18 ++++++----------- configs/configschema/decoder_spec_test.go | 24 ++++++++++++++--------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/configs/configschema/decoder_spec.go b/configs/configschema/decoder_spec.go index 41a3fc4971..333274503c 100644 --- a/configs/configschema/decoder_spec.go +++ b/configs/configschema/decoder_spec.go @@ -101,15 +101,6 @@ func (b *Block) DecoderSpec() hcldec.Spec { childSpec := blockS.Block.DecoderSpec() - // We can only validate 0 or 1 for MinItems, because a dynamic block - // may satisfy any number of min items while only having a single - // block in the config. We cannot validate MaxItems because a - // configuration may have any number of dynamic blocks. - minItems := 0 - if blockS.MinItems > 1 { - minItems = 1 - } - switch blockS.Nesting { case NestingSingle, NestingGroup: ret[name] = &hcldec.BlockSpec{ @@ -134,13 +125,15 @@ func (b *Block) DecoderSpec() hcldec.Spec { ret[name] = &hcldec.BlockTupleSpec{ TypeName: name, Nested: childSpec, - MinItems: minItems, + MinItems: blockS.MinItems, + MaxItems: blockS.MaxItems, } } else { ret[name] = &hcldec.BlockListSpec{ TypeName: name, Nested: childSpec, - MinItems: minItems, + MinItems: blockS.MinItems, + MaxItems: blockS.MaxItems, } } case NestingSet: @@ -154,7 +147,8 @@ func (b *Block) DecoderSpec() hcldec.Spec { ret[name] = &hcldec.BlockSetSpec{ TypeName: name, Nested: childSpec, - MinItems: minItems, + MinItems: blockS.MinItems, + MaxItems: blockS.MaxItems, } case NestingMap: // We prefer to use a list where possible, since it makes our diff --git a/configs/configschema/decoder_spec_test.go b/configs/configschema/decoder_spec_test.go index 1eefc4d8b6..a6571eaa64 100644 --- a/configs/configschema/decoder_spec_test.go +++ b/configs/configschema/decoder_spec_test.go @@ -344,15 +344,12 @@ func TestBlockDecoderSpec(t *testing.T) { }, &hcl.Block{ Type: "foo", - Body: hcl.EmptyBody(), + Body: unknownBody{hcl.EmptyBody()}, }, }, }), cty.ObjectVal(map[string]cty.Value{ - "foo": cty.ListVal([]cty.Value{ - cty.EmptyObjectVal, - cty.EmptyObjectVal, - }), + "foo": cty.UnknownVal(cty.List(cty.EmptyObject)), }), 0, // max items cannot be validated during decode }, @@ -372,14 +369,12 @@ func TestBlockDecoderSpec(t *testing.T) { Blocks: hcl.Blocks{ &hcl.Block{ Type: "foo", - Body: hcl.EmptyBody(), + Body: unknownBody{hcl.EmptyBody()}, }, }, }), cty.ObjectVal(map[string]cty.Value{ - "foo": cty.ListVal([]cty.Value{ - cty.EmptyObjectVal, - }), + "foo": cty.UnknownVal(cty.List(cty.EmptyObject)), }), 0, }, @@ -401,6 +396,7 @@ func TestBlockDecoderSpec(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { spec := test.Schema.DecoderSpec() + got, diags := hcldec.Decode(test.TestBody, spec, nil) if len(diags) != test.DiagCount { t.Errorf("wrong number of diagnostics %d; want %d", len(diags), test.DiagCount) @@ -427,6 +423,16 @@ func TestBlockDecoderSpec(t *testing.T) { } } +// this satisfies hcldec.UnknownBody to simulate a dynamic block with an +// unknown number of values. +type unknownBody struct { + hcl.Body +} + +func (b unknownBody) Unknown() bool { + return true +} + func TestAttributeDecoderSpec(t *testing.T) { tests := map[string]struct { Schema *Attribute From d351d712c44e15ac9698027dde0ca8c776697166 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 15 Apr 2021 17:34:33 -0400 Subject: [PATCH 13/38] dynamic block MinItems MaxItems validation test --- terraform/context_validate_test.go | 92 ++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 7e2b0a25ec..688fc2d456 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -2008,3 +2008,95 @@ func TestContext2Validate_sensitiveProvisionerConfig(t *testing.T) { t.Fatal("ValidateProvisionerConfig not called") } } + +func TestContext2Plan_validateMinMaxDynamicBlock(t *testing.T) { + p := new(MockProvider) + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "things": { + Type: cty.List(cty.String), + Computed: true, + }, + }, + BlockTypes: map[string]*configschema.NestedBlock{ + "foo": { + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "bar": {Type: cty.String, Optional: true}, + }, + }, + Nesting: configschema.NestingList, + MinItems: 2, + MaxItems: 3, + }, + }, + }, + }, + }) + + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_instance" "a" { + // MinItems 2 + foo { + bar = "a" + } + foo { + bar = "b" + } +} + +resource "test_instance" "b" { + // one dymamic block can satisfy MinItems of 2 + dynamic "foo" { + for_each = test_instance.a.things + content { + bar = foo.value + } + } +} + +resource "test_instance" "c" { + // we may have more than MaxItems dynamic blocks when they are unknown + foo { + bar = "b" + } + dynamic "foo" { + for_each = test_instance.a.things + content { + bar = foo.value + } + } + dynamic "foo" { + for_each = test_instance.a.things + content { + bar = "${foo.value}-2" + } + } + dynamic "foo" { + for_each = test_instance.b.things + content { + bar = foo.value + } + } +} +`}) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +} From fad305f884c439969292f003e41bfe8074c207bf Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 16 Apr 2021 08:28:33 -0400 Subject: [PATCH 14/38] cli: Fix remote backend UI issues Fix two bugs which surface when using the remote backend: - When migrating to views, we removed the call to `(*Meta).process` which initialized the color boolean. This resulted in the legacy UI calls in the remote backend stripping color codes. To fix this, we populate this boolean from the common arguments. - Remote apply will output the resource summary and output changes, and these are rendered via the remote backend streaming. We need to special case this in the apply command and prevent displaying a zero-change summary line. Neither of these are coverable by automated tests, as we don't have any command-package level testing for the remote backend. Manually verified. --- command/apply.go | 17 +++++++++++++---- command/plan.go | 5 +++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/command/apply.go b/command/apply.go index 506ffce60c..6365917d42 100644 --- a/command/apply.go +++ b/command/apply.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/hashicorp/terraform/backend" + remoteBackend "github.com/hashicorp/terraform/backend/remote" "github.com/hashicorp/terraform/command/arguments" "github.com/hashicorp/terraform/command/views" "github.com/hashicorp/terraform/plans/planfile" @@ -26,6 +27,11 @@ func (c *ApplyCommand) Run(rawArgs []string) int { common, rawArgs := arguments.ParseView(rawArgs) c.View.Configure(common) + // Propagate -no-color for the remote backend's legacy use of Ui. This + // should be removed when the remote backend is migrated to views. + c.Meta.color = !common.NoColor + c.Meta.Color = c.Meta.color + // Parse and validate flags args, diags := arguments.ParseApply(rawArgs) @@ -116,10 +122,13 @@ func (c *ApplyCommand) Run(rawArgs []string) int { return op.Result.ExitStatus() } - // // Render the resource count and outputs - view.ResourceCount(args.State.StateOutPath) - if !c.Destroy && op.State != nil { - view.Outputs(op.State.RootModule().OutputValues) + // Render the resource count and outputs, unless we're using the remote + // backend, in which case these are rendered remotely + if _, isRemoteBackend := be.(*remoteBackend.Remote); !isRemoteBackend { + view.ResourceCount(args.State.StateOutPath) + if !c.Destroy && op.State != nil { + view.Outputs(op.State.RootModule().OutputValues) + } } view.Diagnostics(diags) diff --git a/command/plan.go b/command/plan.go index bf43048163..4d408fcbba 100644 --- a/command/plan.go +++ b/command/plan.go @@ -21,6 +21,11 @@ func (c *PlanCommand) Run(rawArgs []string) int { common, rawArgs := arguments.ParseView(rawArgs) c.View.Configure(common) + // Propagate -no-color for the remote backend's legacy use of Ui. This + // should be removed when the remote backend is migrated to views. + c.Meta.color = !common.NoColor + c.Meta.Color = c.Meta.color + // Parse and validate flags args, diags := arguments.ParsePlan(rawArgs) From 69e7922a33022c3cba0a2fc0232d1d116a3fc484 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 16 Apr 2021 09:57:04 -0400 Subject: [PATCH 15/38] cli: Fix missing apply summary for remote runs Disabling the resource count and outputs rendering when the remote backend is in use causes them to be omitted from Terraform Cloud runs. This commit changes the condition to render these values if either the remote backend is not in use, or the command is running in automation via the TF_IN_AUTOMATION flag. As this is intended to be set by Terraform Cloud and other remote backend implementations, this addresses the problem. --- command/apply.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/apply.go b/command/apply.go index 6365917d42..c5f7180dd8 100644 --- a/command/apply.go +++ b/command/apply.go @@ -123,8 +123,8 @@ func (c *ApplyCommand) Run(rawArgs []string) int { } // Render the resource count and outputs, unless we're using the remote - // backend, in which case these are rendered remotely - if _, isRemoteBackend := be.(*remoteBackend.Remote); !isRemoteBackend { + // backend locally, in which case these are rendered remotely + if _, isRemoteBackend := be.(*remoteBackend.Remote); !isRemoteBackend || c.RunningInAutomation { view.ResourceCount(args.State.StateOutPath) if !c.Destroy && op.State != nil { view.Outputs(op.State.RootModule().OutputValues) From 8dcf768f4ec182e6c5366572ce447fc5a2ea45cf Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 16 Apr 2021 11:43:57 -0400 Subject: [PATCH 16/38] backend/remote: Add IsLocalOperations To ensure that the apply command can determine whether an operation is executed locally or remotely, we add an IsLocalOperations method on the remote backend. This returns the internal forceLocal boolean. We also update this flag after checking if the corresponding remote workspace is in local operations mode or not. This ensures that we know if an operation is running locally (entirely on the practitioner's machine), pseudo-locally (on a Terraform Cloud worker), or remotely (executing on a worker, rendering locally). --- backend/remote/backend.go | 7 +++++++ command/apply.go | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/backend/remote/backend.go b/backend/remote/backend.go index 28f9d95c45..43eb77fbc9 100644 --- a/backend/remote/backend.go +++ b/backend/remote/backend.go @@ -704,6 +704,9 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend // Check if we need to use the local backend to run the operation. if b.forceLocal || !w.Operations { + // Record that we're forced to run operations locally to allow the + // command package UI to operate correctly + b.forceLocal = true return b.local.Operation(ctx, op) } @@ -949,6 +952,10 @@ func (b *Remote) VerifyWorkspaceTerraformVersion(workspaceName string) tfdiags.D return diags } +func (b *Remote) IsLocalOperations() bool { + return b.forceLocal +} + // Colorize returns the Colorize structure that can be used for colorizing // output. This is guaranteed to always return a non-nil value and so useful // as a helper to wrap any potentially colored strings. diff --git a/command/apply.go b/command/apply.go index c5f7180dd8..178cfbebea 100644 --- a/command/apply.go +++ b/command/apply.go @@ -124,7 +124,7 @@ func (c *ApplyCommand) Run(rawArgs []string) int { // Render the resource count and outputs, unless we're using the remote // backend locally, in which case these are rendered remotely - if _, isRemoteBackend := be.(*remoteBackend.Remote); !isRemoteBackend || c.RunningInAutomation { + if rb, isRemoteBackend := be.(*remoteBackend.Remote); !isRemoteBackend || rb.IsLocalOperations() { view.ResourceCount(args.State.StateOutPath) if !c.Destroy && op.State != nil { view.Outputs(op.State.RootModule().OutputValues) From d0cc7f1d5ee1b2b1925ea1d1851b1748b96ba6c2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Apr 2021 12:37:50 -0400 Subject: [PATCH 17/38] resolve provider types when building the config All the information is available to resolve provider types when building the configuration, but some provider references still had no FQN. This caused validation to assume a default type, and incorrectly reject valid module calls with non-default namespaced providers. Resolve as much provider type information as possible when loading the config. Only use this internally for now, but this should be useful outside of the package to avoid re-resolving the providers later on. We can come back and find where this might be useful elsewhere, but for now keep the change as small as possible to avoid any changes in behavior. --- configs/config.go | 47 +++++++++++++++++++ configs/config_build.go | 4 ++ configs/provider.go | 7 +++ configs/provider_validation.go | 20 ++++++-- configs/resource.go | 7 +++ .../nested-providers-fqns/child/main.tf | 10 +++- .../nested-providers-fqns/main.tf | 3 ++ 7 files changed, 92 insertions(+), 6 deletions(-) diff --git a/configs/config.go b/configs/config.go index cc88981781..6f1603bfa7 100644 --- a/configs/config.go +++ b/configs/config.go @@ -329,6 +329,53 @@ func (c *Config) addProviderRequirements(reqs getproviders.Requirements, recurse return diags } +// resolveProviderTypes walks through the providers in the module and ensures +// the true types are assigned based on the provider requirements for the +// module. +func (c *Config) resolveProviderTypes() { + for _, child := range c.Children { + child.resolveProviderTypes() + } + + // collect the required_providers, and then add any missing default providers + providers := map[string]addrs.Provider{} + for name, p := range c.Module.ProviderRequirements.RequiredProviders { + providers[name] = p.Type + } + + // ensure all provider configs know their correct type + for _, p := range c.Module.ProviderConfigs { + addr, required := providers[p.Name] + if required { + p.providerType = addr + } else { + addr := addrs.NewDefaultProvider(p.Name) + p.providerType = addr + providers[p.Name] = addr + } + } + + // connect module call providers to the correct type + for _, mod := range c.Module.ModuleCalls { + for _, p := range mod.Providers { + if addr, known := providers[p.InParent.Name]; known { + p.InParent.providerType = addr + } + } + } + + // fill in parent module calls too + if c.Parent != nil { + for _, mod := range c.Parent.Module.ModuleCalls { + for _, p := range mod.Providers { + if addr, known := providers[p.InChild.Name]; known { + p.InChild.providerType = addr + } + } + } + } +} + // ProviderTypes returns the FQNs of each distinct provider type referenced // in the receiving configuration. // diff --git a/configs/config_build.go b/configs/config_build.go index 345c678144..1083a1a2d3 100644 --- a/configs/config_build.go +++ b/configs/config_build.go @@ -23,6 +23,10 @@ func BuildConfig(root *Module, walker ModuleWalker) (*Config, hcl.Diagnostics) { cfg.Root = cfg // Root module is self-referential. cfg.Children, diags = buildChildModules(cfg, walker) + // Now that the config is built, we can connect the provider names to all + // the known types for validation. + cfg.resolveProviderTypes() + diags = append(diags, validateProviderConfigs(nil, cfg, false)...) return cfg, diags diff --git a/configs/provider.go b/configs/provider.go index 7fd39b174f..3bbba30521 100644 --- a/configs/provider.go +++ b/configs/provider.go @@ -25,6 +25,13 @@ type Provider struct { Config hcl.Body DeclRange hcl.Range + + // TODO: this may not be set in some cases, so it is not yet suitable for + // use outside of this package. We currently only use it for internal + // validation, but once we verify that this can be set in all cases, we can + // export this so providers don't need to be re-resolved. + // This same field is also added to the ProviderConfigRef struct. + providerType addrs.Provider } func decodeProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) { diff --git a/configs/provider_validation.go b/configs/provider_validation.go index 57ad0fcdf8..ec8bda2f30 100644 --- a/configs/provider_validation.go +++ b/configs/provider_validation.go @@ -136,9 +136,18 @@ func validateProviderConfigs(call *ModuleCall, cfg *Config, noProviderConfig boo // You cannot pass in a provider that cannot be used for name, passed := range passedIn { + childTy := passed.InChild.providerType + // get a default type if there was none set + if childTy.IsZero() { + // This means the child module is only using an inferred + // provider type. We allow this but will generate a warning to + // declare provider_requirements below. + childTy = addrs.NewDefaultProvider(passed.InChild.Name) + } + providerAddr := addrs.AbsProviderConfig{ Module: cfg.Path, - Provider: addrs.NewDefaultProvider(passed.InChild.Name), + Provider: childTy, Alias: passed.InChild.Alias, } @@ -172,9 +181,12 @@ func validateProviderConfigs(call *ModuleCall, cfg *Config, noProviderConfig boo } // The provider being passed in must also be of the correct type. - // While we would like to ensure required_providers exists here, - // implied default configuration is still allowed. - pTy := addrs.NewDefaultProvider(passed.InParent.Name) + pTy := passed.InParent.providerType + if pTy.IsZero() { + // While we would like to ensure required_providers exists here, + // implied default configuration is still allowed. + pTy = addrs.NewDefaultProvider(passed.InParent.Name) + } // use the full address for a nice diagnostic output parentAddr := addrs.AbsProviderConfig{ diff --git a/configs/resource.go b/configs/resource.go index 05bb60c3af..73c0d7c897 100644 --- a/configs/resource.go +++ b/configs/resource.go @@ -374,6 +374,13 @@ type ProviderConfigRef struct { NameRange hcl.Range Alias string AliasRange *hcl.Range // nil if alias not set + + // TODO: this may not be set in some cases, so it is not yet suitable for + // use outside of this package. We currently only use it for internal + // validation, but once we verify that this can be set in all cases, we can + // export this so providers don't need to be re-resolved. + // This same field is also added to the Provider struct. + providerType addrs.Provider } func decodeProviderConfigRef(expr hcl.Expression, argName string) (*ProviderConfigRef, hcl.Diagnostics) { diff --git a/configs/testdata/valid-modules/nested-providers-fqns/child/main.tf b/configs/testdata/valid-modules/nested-providers-fqns/child/main.tf index 1973912d5c..79e449bf46 100644 --- a/configs/testdata/valid-modules/nested-providers-fqns/child/main.tf +++ b/configs/testdata/valid-modules/nested-providers-fqns/child/main.tf @@ -3,11 +3,13 @@ terraform { bar-test = { source = "bar/test" } + foo-test = { + source = "foo/test" + configuration_aliases = [foo-test.other] + } } } -provider "bar-test" {} - resource "test_instance" "explicit" { // explicitly setting provider bar-test provider = bar-test @@ -17,3 +19,7 @@ resource "test_instance" "implicit" { // since the provider type name "test" does not match an entry in // required_providers, the default provider "test" should be used } + +resource "test_instance" "other" { + provider = foo-test.other +} diff --git a/configs/testdata/valid-modules/nested-providers-fqns/main.tf b/configs/testdata/valid-modules/nested-providers-fqns/main.tf index 3a9dc4a13e..27988f42b2 100644 --- a/configs/testdata/valid-modules/nested-providers-fqns/main.tf +++ b/configs/testdata/valid-modules/nested-providers-fqns/main.tf @@ -10,6 +10,9 @@ provider "foo-test" {} module "child" { source = "./child" + providers = { + foo-test.other = foo-test + } } resource "test_instance" "explicit" { From 85adad0ec7715b5723516e6a0e4a745cc8c2cbe8 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Mon, 19 Apr 2021 09:04:46 -0400 Subject: [PATCH 18/38] docs: small update for provider binary locations (#28413) * docs: add note that provider binaries need to be placed in appropriate subdirectories under the default locations --- website/docs/cli/config/config-file.html.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/website/docs/cli/config/config-file.html.md b/website/docs/cli/config/config-file.html.md index 97ba3a7d6d..8dce016620 100644 --- a/website/docs/cli/config/config-file.html.md +++ b/website/docs/cli/config/config-file.html.md @@ -282,10 +282,14 @@ the operating system where you are running Terraform: `~/.local/share/terraform/plugins`, `/usr/local/share/terraform/plugins`, and `/usr/share/terraform/plugins`. -Terraform will create an implied `filesystem_mirror` method block for each of -the directories indicated above that exists when Terraform starts up. -In addition, if a `terraform.d/plugins` directory exists in the current working -directory, it will be added as a filesystem mirror. +If a `terraform.d/plugins` directory exists in the current working directory +then Terraform will also include that directory, regardless of your operating +system. + +Terraform will check each of the paths above to see if it exists, and if so +treat it as a filesystem mirror. The directory structure inside each one must +therefore match one of the two structures described for `filesystem_mirror` +blocks in [Explicit Installation Method Configuration](#explicit-installation-method-configuration). In addition to the zero or more implied `filesystem_mirror` blocks, Terraform also creates an implied `direct` block. Terraform will scan all of the From 56483d10d29fa391c31cc81ccb38daef8d73a78c Mon Sep 17 00:00:00 2001 From: Rachel Sharp Date: Mon, 19 Apr 2021 12:18:02 -0400 Subject: [PATCH 19/38] Update debugging.html.md --- website/docs/internals/debugging.html.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/website/docs/internals/debugging.html.md b/website/docs/internals/debugging.html.md index b822f29d36..5262c62b1a 100644 --- a/website/docs/internals/debugging.html.md +++ b/website/docs/internals/debugging.html.md @@ -8,6 +8,9 @@ description: |- # Debugging Terraform +> **Hands-on:** Try the [Create Dynamic Expressions](https://learn.hashicorp.com/tutorials/terraform/troubleshooting-workflow#bug-reporting-best-practices?utm_source=WEBSITE&utm_medium=WEB_IO&utm_offer=ARTICLE_PAGE&utm_content=DOCS) tutorial on HashiCorp Learn. + + Terraform has detailed logs which can be enabled by setting the `TF_LOG` environment variable to any value. This will cause detailed logs to appear on stderr. You can set `TF_LOG` to one of the log levels `TRACE`, `DEBUG`, `INFO`, `WARN` or `ERROR` to change the verbosity of the logs. From 877348c031100d682f5f18f3a49cffac4bc4a1f3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 19 Apr 2021 12:35:10 -0400 Subject: [PATCH 20/38] wrong operation during destroy plan walk The destroy plan walk was identifying itself as a normal plan, and causing providers to be configured when they were not needed. Since the provider configuration may not be complete during the minimal destroy plan walk, validation or configuration may fail. --- terraform/context.go | 2 +- terraform/context_plan2_test.go | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/terraform/context.go b/terraform/context.go index ca15073bad..3809061ab5 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -633,7 +633,7 @@ func (c *Context) destroyPlan() (*plans.Plan, tfdiags.Diagnostics) { } // Do the walk - walker, walkDiags := c.walk(graph, walkPlan) + walker, walkDiags := c.walk(graph, walkPlanDestroy) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { diff --git a/terraform/context_plan2_test.go b/terraform/context_plan2_test.go index 0444a775ca..f95f7f6e35 100644 --- a/terraform/context_plan2_test.go +++ b/terraform/context_plan2_test.go @@ -439,3 +439,49 @@ output "result" { } } } + +func TestContext2Plan_destroyNoProviderConfig(t *testing.T) { + // providers do not need to be configured during a destroy plan + p := simpleMockProvider() + p.ValidateProviderConfigFn = func(req providers.ValidateProviderConfigRequest) (resp providers.ValidateProviderConfigResponse) { + v := req.Config.GetAttr("test_string") + if v.IsNull() || !v.IsKnown() || v.AsString() != "ok" { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("invalid provider configuration")) + } + return resp + } + + m := testModuleInline(t, map[string]string{ + "main.tf": ` +locals { + value = "ok" +} + +provider "test" { + test_string = local.value +} +`, + }) + + addr := mustResourceInstanceAddr("test_object.a") + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent(addr, &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"test_string":"foo"}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + State: state, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + Destroy: true, + }) + + _, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } +} From 751fba49a7c89c5130eecb3697571fdd3c731a62 Mon Sep 17 00:00:00 2001 From: sanflores Date: Mon, 19 Apr 2021 14:20:03 -0300 Subject: [PATCH 21/38] website: v0.15 upgrade guide had invalid example for tomap(...) tomap expects an object value with braces, not a tuple value with brackets. --- website/upgrade-guides/0-15.html.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/upgrade-guides/0-15.html.markdown b/website/upgrade-guides/0-15.html.markdown index 33044891da..6c43678964 100644 --- a/website/upgrade-guides/0-15.html.markdown +++ b/website/upgrade-guides/0-15.html.markdown @@ -203,7 +203,7 @@ upgrading to Terraform v0.15: ``` If you need to update a module which was using the `map` function, you - can get the same result by replacing `map(...)` with `tomap([...])`. + can get the same result by replacing `map(...)` with `tomap({...})`. For example: ```diff From 8f233cde4ce64a2b1217b3b0863966140c70c8cd Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 19 Apr 2021 09:04:51 -0700 Subject: [PATCH 22/38] cli: Diagnostics can include collections with sensitive elements We previously had a shallow IsMarked call in compactValueStr's caller but then a more-conservative deep ContainsMarked call inside compactValueStr with a different resulting message. As well as causing an inconsistency in messages, this was also a bit confusing because it made it seem like a non-sensitive collection containing a sensitive element value was wholly sensitive, making the debug information in the diagnostic messages not trustworthy for debugging certain varieties of problem. I originally considered just removing the redundant check in compactValueStr here, but ultimately I decided to keep it as a sort of defense in depth in case a future refactoring disconnects these two checks. This should also serve as a prompt to someone making later changes to compactValueStr to think about the implications of sensitive values in there, which otherwise wouldn't be mentioned at all. Disclosing information about a collection containing sensitive values is safe here because compactValueStr only discloses information about the value's type and element keys, and neither of those can be sensitive in isolation. (Constructing a map with sensitive keys reduces to a sensitive overall map.) --- command/views/json/diagnostic.go | 12 +++- command/views/json/diagnostic_test.go | 56 +++++++++++++++++++ ...llection-containing-a-sensitive-value.json | 31 ++++++++++ 3 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-a-collection-containing-a-sensitive-value.json diff --git a/command/views/json/diagnostic.go b/command/views/json/diagnostic.go index 8ff595594a..12a8d27308 100644 --- a/command/views/json/diagnostic.go +++ b/command/views/json/diagnostic.go @@ -317,10 +317,20 @@ func compactValueStr(val cty.Value) string { // helpful but concise messages in diagnostics. It is not comprehensive // nor intended to be used for other purposes. - if val.ContainsMarked() { + if val.IsMarked() { + // We check this in here just to make sure, but note that the caller + // of compactValueStr ought to have already checked this and skipped + // calling into compactValueStr anyway, so this shouldn't actually + // be reachable. return "(sensitive value)" } + // WARNING: We've only checked that the value isn't sensitive _shallowly_ + // here, and so we must never show any element values from complex types + // in here. However, it's fine to show map keys and attribute names because + // those are never sensitive in isolation: the entire value would be + // sensitive in that case. + ty := val.Type() switch { case val.IsNull(): diff --git a/command/views/json/diagnostic_test.go b/command/views/json/diagnostic_test.go index 182c8ff5fe..8308b18a60 100644 --- a/command/views/json/diagnostic_test.go +++ b/command/views/json/diagnostic_test.go @@ -360,6 +360,62 @@ func TestNewDiagnostic(t *testing.T) { }, }, }, + "error with source code subject and expression referring to a collection containing a sensitive value": { + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Wrong noises", + Detail: "Biological sounds are not allowed", + Subject: &hcl.Range{ + Filename: "test.tf", + Start: hcl.Pos{Line: 2, Column: 9, Byte: 42}, + End: hcl.Pos{Line: 2, Column: 26, Byte: 59}, + }, + Expression: hcltest.MockExprTraversal(hcl.Traversal{ + hcl.TraverseRoot{Name: "var"}, + hcl.TraverseAttr{Name: "boop"}, + }), + EvalContext: &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "var": cty.ObjectVal(map[string]cty.Value{ + "boop": cty.MapVal(map[string]cty.Value{ + "hello!": cty.StringVal("bleurgh").Mark("sensitive"), + }), + }), + }, + }, + }, + &Diagnostic{ + Severity: "error", + Summary: "Wrong noises", + Detail: "Biological sounds are not allowed", + Range: &DiagnosticRange{ + Filename: "test.tf", + Start: Pos{ + Line: 2, + Column: 9, + Byte: 42, + }, + End: Pos{ + Line: 2, + Column: 26, + Byte: 59, + }, + }, + Snippet: &DiagnosticSnippet{ + Context: strPtr(`resource "test_resource" "test"`), + Code: (` foo = var.boop["hello!"]`), + StartLine: (2), + HighlightStartOffset: (8), + HighlightEndOffset: (25), + Values: []DiagnosticExpressionValue{ + { + Traversal: `var.boop`, + Statement: `is map of string with 1 element`, + }, + }, + }, + }, + }, "error with source code subject and unknown string expression": { &hcl.Diagnostic{ Severity: hcl.DiagError, diff --git a/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-a-collection-containing-a-sensitive-value.json b/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-a-collection-containing-a-sensitive-value.json new file mode 100644 index 0000000000..e6599faf5a --- /dev/null +++ b/command/views/json/testdata/diagnostic/error-with-source-code-subject-and-expression-referring-to-a-collection-containing-a-sensitive-value.json @@ -0,0 +1,31 @@ +{ + "severity": "error", + "summary": "Wrong noises", + "detail": "Biological sounds are not allowed", + "range": { + "filename": "test.tf", + "start": { + "line": 2, + "column": 9, + "byte": 42 + }, + "end": { + "line": 2, + "column": 26, + "byte": 59 + } + }, + "snippet": { + "context": "resource \"test_resource\" \"test\"", + "code": " foo = var.boop[\"hello!\"]", + "start_line": 2, + "highlight_start_offset": 8, + "highlight_end_offset": 25, + "values": [ + { + "traversal": "var.boop", + "statement": "is map of string with 1 element" + } + ] + } +} From 14336ae6f8c661c16b9ba1f091048a095b5fa38b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 19 Apr 2021 10:13:08 -0700 Subject: [PATCH 23/38] lang/funcs: Conversion functions can handle sensitive values In order to avoid updating every one of our existing functions with explicit support for sensitive values, there's a default rule in the functions system which makes the result of a function sensitive if any of its arguments contain sensitive values. We were applying that default to the various type conversion functions, like tomap and tolist, which meant that converting a complex-typed value with a sensitive value anywhere inside it would result in a wholly-sensitive result. That's unnecessarily conservative because the cty conversion layer (which these functions are wrapping) already knows how to handle sensitivity in a more precise way. Therefore we can opt in to handling marked values (which Terraform uses for sensitivity) here and the only special thing we need to do is handle errors related to sensitive values differently, so we won't print their values out literally in case of an error (and so that the attempt to print them out literally won't panic trying to extract the marked values). --- lang/funcs/conversion.go | 10 ++++++-- lang/funcs/conversion_test.go | 48 +++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/lang/funcs/conversion.go b/lang/funcs/conversion.go index 83f8597972..4991f60a85 100644 --- a/lang/funcs/conversion.go +++ b/lang/funcs/conversion.go @@ -28,8 +28,9 @@ func MakeToFunc(wantTy cty.Type) function.Function { // messages to be more appropriate for an explicit type // conversion, whereas the cty function system produces // messages aimed at _implicit_ type conversions. - Type: cty.DynamicPseudoType, - AllowNull: true, + Type: cty.DynamicPseudoType, + AllowNull: true, + AllowMarked: true, }, }, Type: func(args []cty.Value) (cty.Type, error) { @@ -65,6 +66,11 @@ func MakeToFunc(wantTy cty.Type) function.Function { // once we note that the value isn't either "true" or "false". gotTy := args[0].Type() switch { + case args[0].ContainsMarked(): + // Generic message so we won't inadvertently disclose + // information about sensitive values. + return cty.NilVal, function.NewArgErrorf(0, "cannot convert this sensitive %s to %s", gotTy.FriendlyName(), wantTy.FriendlyNameForConstraint()) + case gotTy == cty.String && wantTy == cty.Bool: what := "string" if !args[0].IsNull() { diff --git a/lang/funcs/conversion_test.go b/lang/funcs/conversion_test.go index ca0ac46518..819a0e4d00 100644 --- a/lang/funcs/conversion_test.go +++ b/lang/funcs/conversion_test.go @@ -32,6 +32,18 @@ func TestTo(t *testing.T) { cty.NullVal(cty.String), ``, }, + { + cty.StringVal("a").Mark("boop"), + cty.String, + cty.StringVal("a").Mark("boop"), + ``, + }, + { + cty.NullVal(cty.String).Mark("boop"), + cty.String, + cty.NullVal(cty.String).Mark("boop"), + ``, + }, { cty.True, cty.String, @@ -44,12 +56,24 @@ func TestTo(t *testing.T) { cty.DynamicVal, `cannot convert "a" to bool; only the strings "true" or "false" are allowed`, }, + { + cty.StringVal("a").Mark("boop"), + cty.Bool, + cty.DynamicVal, + `cannot convert this sensitive string to bool`, + }, { cty.StringVal("a"), cty.Number, cty.DynamicVal, `cannot convert "a" to number; given string must be a decimal representation of a number`, }, + { + cty.StringVal("a").Mark("boop"), + cty.Number, + cty.DynamicVal, + `cannot convert this sensitive string to number`, + }, { cty.NullVal(cty.String), cty.Number, @@ -86,6 +110,30 @@ func TestTo(t *testing.T) { cty.MapVal(map[string]cty.Value{"foo": cty.StringVal("hello"), "bar": cty.StringVal("true")}), ``, }, + { + cty.ObjectVal(map[string]cty.Value{"foo": cty.StringVal("hello"), "bar": cty.StringVal("world").Mark("boop")}), + cty.Map(cty.String), + cty.MapVal(map[string]cty.Value{"foo": cty.StringVal("hello"), "bar": cty.StringVal("world").Mark("boop")}), + ``, + }, + { + cty.ObjectVal(map[string]cty.Value{"foo": cty.StringVal("hello"), "bar": cty.StringVal("world")}).Mark("boop"), + cty.Map(cty.String), + cty.MapVal(map[string]cty.Value{"foo": cty.StringVal("hello"), "bar": cty.StringVal("world")}).Mark("boop"), + ``, + }, + { + cty.TupleVal([]cty.Value{cty.StringVal("hello"), cty.StringVal("world").Mark("boop")}), + cty.List(cty.String), + cty.ListVal([]cty.Value{cty.StringVal("hello"), cty.StringVal("world").Mark("boop")}), + ``, + }, + { + cty.TupleVal([]cty.Value{cty.StringVal("hello"), cty.StringVal("world")}).Mark("boop"), + cty.List(cty.String), + cty.ListVal([]cty.Value{cty.StringVal("hello"), cty.StringVal("world")}).Mark("boop"), + ``, + }, { cty.EmptyTupleVal, cty.String, From fabdf0bea1fa2bf6a9d56cc3ea0f28242bf5e812 Mon Sep 17 00:00:00 2001 From: John Houston Date: Tue, 20 Apr 2021 10:05:45 -0400 Subject: [PATCH 24/38] Add config_paths and drop KUBECONFIG env variable in kubernetes backend (#26997) --- backend/remote-state/kubernetes/backend.go | 66 +++++++++++-------- backend/remote-state/kubernetes/client.go | 1 + .../settings/backends/kubernetes.html.md | 10 +-- 3 files changed, 46 insertions(+), 31 deletions(-) diff --git a/backend/remote-state/kubernetes/backend.go b/backend/remote-state/kubernetes/backend.go index 12530b0bfa..a3398a464c 100644 --- a/backend/remote-state/kubernetes/backend.go +++ b/backend/remote-state/kubernetes/backend.go @@ -6,11 +6,11 @@ import ( "fmt" "log" "os" + "path/filepath" "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/internal/legacy/helper/schema" "github.com/hashicorp/terraform/version" - "github.com/mitchellh/cli" "github.com/mitchellh/go-homedir" k8sSchema "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ -114,16 +114,17 @@ func New() backend.Backend { DefaultFunc: schema.EnvDefaultFunc("KUBE_CLUSTER_CA_CERT_DATA", ""), Description: "PEM-encoded root certificates bundle for TLS authentication.", }, + "config_paths": { + Type: schema.TypeList, + Elem: &schema.Schema{Type: schema.TypeString}, + Optional: true, + Description: "A list of paths to kube config files. Can be set with KUBE_CONFIG_PATHS environment variable.", + }, "config_path": { - Type: schema.TypeString, - Optional: true, - DefaultFunc: schema.MultiEnvDefaultFunc( - []string{ - "KUBE_CONFIG", - "KUBECONFIG", - }, - "~/.kube/config"), - Description: "Path to the kube config file, defaults to ~/.kube/config", + Type: schema.TypeString, + Optional: true, + DefaultFunc: schema.EnvDefaultFunc("KUBE_CONFIG_PATH", ""), + Description: "Path to the kube config file. Can be set with KUBE_CONFIG_PATH environment variable.", }, "config_context": { Type: schema.TypeString, @@ -285,15 +286,7 @@ func getInitialConfig(data *schema.ResourceData) (*restclient.Config, error) { var cfg *restclient.Config var err error - c := &cli.BasicUi{Writer: os.Stdout} - inCluster := data.Get("in_cluster_config").(bool) - cf := data.Get("load_config_file").(bool) - - if !inCluster && !cf { - c.Output(noConfigError) - } - if inCluster { cfg, err = restclient.InClusterConfig() if err != nil { @@ -313,13 +306,34 @@ func getInitialConfig(data *schema.ResourceData) (*restclient.Config, error) { } func tryLoadingConfigFile(d *schema.ResourceData) (*restclient.Config, error) { - path, err := homedir.Expand(d.Get("config_path").(string)) - if err != nil { - return nil, err + loader := &clientcmd.ClientConfigLoadingRules{} + + configPaths := []string{} + if v, ok := d.Get("config_path").(string); ok && v != "" { + configPaths = []string{v} + } else if v, ok := d.Get("config_paths").([]interface{}); ok && len(v) > 0 { + for _, p := range v { + configPaths = append(configPaths, p.(string)) + } + } else if v := os.Getenv("KUBE_CONFIG_PATHS"); v != "" { + configPaths = filepath.SplitList(v) } - loader := &clientcmd.ClientConfigLoadingRules{ - ExplicitPath: path, + expandedPaths := []string{} + for _, p := range configPaths { + path, err := homedir.Expand(p) + if err != nil { + log.Printf("[DEBUG] Could not expand path: %s", err) + return nil, err + } + log.Printf("[DEBUG] Using kubeconfig: %s", path) + expandedPaths = append(expandedPaths, path) + } + + if len(expandedPaths) == 1 { + loader.ExplicitPath = expandedPaths[0] + } else { + loader.Precedence = expandedPaths } overrides := &clientcmd.ConfigOverrides{} @@ -367,13 +381,13 @@ func tryLoadingConfigFile(d *schema.ResourceData) (*restclient.Config, error) { cfg, err := cc.ClientConfig() if err != nil { if pathErr, ok := err.(*os.PathError); ok && os.IsNotExist(pathErr.Err) { - log.Printf("[INFO] Unable to load config file as it doesn't exist at %q", path) + log.Printf("[INFO] Unable to load config file as it doesn't exist at %q", pathErr.Path) return nil, nil } - return nil, fmt.Errorf("Failed to load config (%s%s): %s", path, ctxSuffix, err) + return nil, fmt.Errorf("Failed to initialize kubernetes configuration: %s", err) } - log.Printf("[INFO] Successfully loaded config file (%s%s)", path, ctxSuffix) + log.Printf("[INFO] Successfully initialized config") return cfg, nil } diff --git a/backend/remote-state/kubernetes/client.go b/backend/remote-state/kubernetes/client.go index 5bc57f6bb8..3490f91b50 100644 --- a/backend/remote-state/kubernetes/client.go +++ b/backend/remote-state/kubernetes/client.go @@ -17,6 +17,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/dynamic" + _ "k8s.io/client-go/plugin/pkg/client/auth" // Import to initialize client auth plugins. "k8s.io/utils/pointer" coordinationv1 "k8s.io/api/coordination/v1" diff --git a/website/docs/language/settings/backends/kubernetes.html.md b/website/docs/language/settings/backends/kubernetes.html.md index 715db82c57..71d80e24fe 100644 --- a/website/docs/language/settings/backends/kubernetes.html.md +++ b/website/docs/language/settings/backends/kubernetes.html.md @@ -20,18 +20,18 @@ Stores the state in a [Kubernetes secret](https://kubernetes.io/docs/concepts/co terraform { backend "kubernetes" { secret_suffix = "state" - load_config_file = true + config_path = "~/.kube/config" } } ``` This assumes the user/service account running terraform has [permissions](https://kubernetes.io/docs/reference/access-authn-authz/authorization/) to read/write secrets in the [namespace](https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/) used to store the secret. -If the `load_config_file` flag is set the backend will attempt to use a [kubeconfig file](https://kubernetes.io/docs/concepts/configuration/organize-cluster-access-kubeconfig/) to gain access to the cluster. +If the `config_path` or `config_paths` attribute is set the backend will attempt to use a [kubeconfig file](https://kubernetes.io/docs/concepts/configuration/organize-cluster-access-kubeconfig/) to gain access to the cluster. If the `in_cluster_config` flag is set the backend will attempt to use a [service account](https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/) to access the cluster. This can be used if Terraform is being run from within a pod running in the Kubernetes cluster. -For most use cases either `in_cluster_config` or `load_config_file` will need to be set to `true`. If both flags are set the configuration from `load_config_file` will be used. +For most use cases either `in_cluster_config`, `config_path`, or `config_paths` will need to be set. If all flags are set the configuration at `config_path` will be used. Note that for the access credentials we recommend using a [partial configuration](/docs/language/settings/backends/configuration.html#partial-configuration). @@ -56,7 +56,6 @@ The following configuration options are supported: * `labels` - (Optional) Map of additional labels to be applied to the secret and lease. * `namespace` - (Optional) Namespace to store the secret and lease in. Can be sourced from `KUBE_NAMESPACE`. * `in_cluster_config` - (Optional) Used to authenticate to the cluster from inside a pod. Can be sourced from `KUBE_IN_CLUSTER_CONFIG`. -* `load_config_file` - (Optional) Use a kubeconfig file to access the cluster. Can be sourced from `KUBE_LOAD_CONFIG_FILE`. * `host` - (Optional) The hostname (in form of URI) of Kubernetes master. Can be sourced from `KUBE_HOST`. Defaults to `https://localhost`. * `username` - (Optional) The username to use for HTTP basic authentication when accessing the Kubernetes master endpoint. Can be sourced from `KUBE_USER`. * `password` - (Optional) The password to use for HTTP basic authentication when accessing the Kubernetes master endpoint. Can be sourced from `KUBE_PASSWORD`. @@ -64,7 +63,8 @@ The following configuration options are supported: * `client_certificate` - (Optional) PEM-encoded client certificate for TLS authentication. Can be sourced from `KUBE_CLIENT_CERT_DATA`. * `client_key` - (Optional) PEM-encoded client certificate key for TLS authentication. Can be sourced from `KUBE_CLIENT_KEY_DATA`. * `cluster_ca_certificate` - (Optional) PEM-encoded root certificates bundle for TLS authentication. Can be sourced from `KUBE_CLUSTER_CA_CERT_DATA`. -* `config_path` - (Optional) Path to the kube config file. Can be sourced from `KUBE_CONFIG` or `KUBECONFIG`. Defaults to `~/.kube/config`. +* `config_path` - (Optional) Path to the kube config file. Can be sourced from `KUBE_CONFIG_PATH`. +* `config_paths` - (Optional) List of paths to kube config files. Can be sourced from `KUBE_CONFIG_PATHS`. * `config_context` - (Optional) Context to choose from the config file. Can be sourced from `KUBE_CTX`. * `config_context_auth_info` - (Optional) Authentication info context of the kube config (name of the kubeconfig user, `--user` flag in `kubectl`). Can be sourced from `KUBE_CTX_AUTH_INFO`. * `config_context_cluster` - (Optional) Cluster context of the kube config (name of the kubeconfig cluster, `--cluster` flag in `kubectl`). Can be sourced from `KUBE_CTX_CLUSTER`. From 7f571b5ebb76111a2c671ac0c2212379cc676bcd Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 20 Apr 2021 12:31:32 -0400 Subject: [PATCH 25/38] additional null checks in provisioners Now that provisioners for directly with the plugin API and cty data types, we need to add a few null checks to catch invalid input that would have otherwise been masked by the legacy SDK. --- .../local-exec/resource_provisioner.go | 10 ++-- .../local-exec/resource_provisioner_test.go | 46 +++++++++++++++++++ .../remote-exec/resource_provisioner.go | 9 +++- .../remote-exec/resource_provisioner_test.go | 44 ++++++++++++++++++ 4 files changed, 105 insertions(+), 4 deletions(-) diff --git a/builtin/provisioners/local-exec/resource_provisioner.go b/builtin/provisioners/local-exec/resource_provisioner.go index 8bf7d4c49c..618df7e62c 100644 --- a/builtin/provisioners/local-exec/resource_provisioner.go +++ b/builtin/provisioners/local-exec/resource_provisioner.go @@ -82,8 +82,10 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques if !envVal.IsNull() { for k, v := range envVal.AsValueMap() { - entry := fmt.Sprintf("%s=%s", k, v.AsString()) - env = append(env, entry) + if !v.IsNull() { + entry := fmt.Sprintf("%s=%s", k, v.AsString()) + env = append(env, entry) + } } } @@ -93,7 +95,9 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques var cmdargs []string if !intrVal.IsNull() && intrVal.LengthInt() > 0 { for _, v := range intrVal.AsValueSlice() { - cmdargs = append(cmdargs, v.AsString()) + if !v.IsNull() { + cmdargs = append(cmdargs, v.AsString()) + } } } else { if runtime.GOOS == "windows" { diff --git a/builtin/provisioners/local-exec/resource_provisioner_test.go b/builtin/provisioners/local-exec/resource_provisioner_test.go index 235e696463..f10a3ac1e7 100644 --- a/builtin/provisioners/local-exec/resource_provisioner_test.go +++ b/builtin/provisioners/local-exec/resource_provisioner_test.go @@ -1,6 +1,7 @@ package localexec import ( + "fmt" "io/ioutil" "os" "strings" @@ -204,3 +205,48 @@ func TestResourceProvisioner_StopClose(t *testing.T) { p.Stop() p.Close() } + +func TestResourceProvisioner_nullsInOptionals(t *testing.T) { + output := cli.NewMockUi() + p := New() + schema := p.GetSchema().Provisioner + + for i, cfg := range []cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "command": cty.StringVal("echo OK"), + "environment": cty.MapVal(map[string]cty.Value{ + "FOO": cty.NullVal(cty.String), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "command": cty.StringVal("echo OK"), + "environment": cty.NullVal(cty.Map(cty.String)), + }), + cty.ObjectVal(map[string]cty.Value{ + "command": cty.StringVal("echo OK"), + "interpreter": cty.ListVal([]cty.Value{cty.NullVal(cty.String)}), + }), + cty.ObjectVal(map[string]cty.Value{ + "command": cty.StringVal("echo OK"), + "interpreter": cty.NullVal(cty.List(cty.String)), + }), + cty.ObjectVal(map[string]cty.Value{ + "command": cty.StringVal("echo OK"), + "working_dir": cty.NullVal(cty.String), + }), + } { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + cfg, err := schema.CoerceValue(cfg) + if err != nil { + t.Fatal(err) + } + + // verifying there are no panics + p.ProvisionResource(provisioners.ProvisionResourceRequest{ + Config: cfg, + UIOutput: output, + }) + }) + } +} diff --git a/builtin/provisioners/remote-exec/resource_provisioner.go b/builtin/provisioners/remote-exec/resource_provisioner.go index b3d42ba469..1abc562e6d 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner.go +++ b/builtin/provisioners/remote-exec/resource_provisioner.go @@ -128,6 +128,10 @@ func (p *provisioner) Close() error { func generateScripts(inline cty.Value) ([]string, error) { var lines []string for _, l := range inline.AsValueSlice() { + if l.IsNull() { + return nil, errors.New("invalid null string in 'scripts'") + } + s := l.AsString() if s == "" { return nil, errors.New("invalid empty string in 'scripts'") @@ -169,11 +173,14 @@ func collectScripts(v cty.Value) ([]io.ReadCloser, error) { if scriptList := v.GetAttr("scripts"); !scriptList.IsNull() { for _, script := range scriptList.AsValueSlice() { + if script.IsNull() { + return nil, errors.New("invalid null string in 'script'") + } s := script.AsString() if s == "" { return nil, errors.New("invalid empty string in 'script'") } - scripts = append(scripts, script.AsString()) + scripts = append(scripts, s) } } diff --git a/builtin/provisioners/remote-exec/resource_provisioner_test.go b/builtin/provisioners/remote-exec/resource_provisioner_test.go index 56bf23c812..eb9da611d5 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner_test.go +++ b/builtin/provisioners/remote-exec/resource_provisioner_test.go @@ -3,6 +3,7 @@ package remoteexec import ( "bytes" "context" + "fmt" "io" "log" "testing" @@ -274,3 +275,46 @@ func TestResourceProvisioner_connectionRequired(t *testing.T) { t.Fatalf("expected 'missing connection' error: got %q", got) } } + +func TestResourceProvisioner_nullsInOptionals(t *testing.T) { + output := cli.NewMockUi() + p := New() + schema := p.GetSchema().Provisioner + + for i, cfg := range []cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "script": cty.StringVal("echo"), + "inline": cty.NullVal(cty.List(cty.String)), + }), + cty.ObjectVal(map[string]cty.Value{ + "inline": cty.ListVal([]cty.Value{ + cty.NullVal(cty.String), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "script": cty.NullVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "scripts": cty.NullVal(cty.List(cty.String)), + }), + cty.ObjectVal(map[string]cty.Value{ + "scripts": cty.ListVal([]cty.Value{ + cty.NullVal(cty.String), + }), + }), + } { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + cfg, err := schema.CoerceValue(cfg) + if err != nil { + t.Fatal(err) + } + + // verifying there are no panics + p.ProvisionResource(provisioners.ProvisionResourceRequest{ + Config: cfg, + UIOutput: output, + }) + }) + } +} From f8493bf5cd78bc2a723f5ddc6f6bceb0e08813ea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Apr 2021 17:11:27 -0400 Subject: [PATCH 26/38] update hcl update to v2.10.0 --- go.mod | 2 +- go.sum | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 752fa0f6f0..7d278f4aec 100644 --- a/go.mod +++ b/go.mod @@ -65,7 +65,7 @@ require ( github.com/hashicorp/go-uuid v1.0.1 github.com/hashicorp/go-version v1.2.1 github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f - github.com/hashicorp/hcl/v2 v2.9.1 + github.com/hashicorp/hcl/v2 v2.10.0 github.com/hashicorp/memberlist v0.1.0 // indirect github.com/hashicorp/serf v0.0.0-20160124182025-e4ec8cc423bb // indirect github.com/hashicorp/terraform-config-inspect v0.0.0-20210209133302-4fd17a0faac2 diff --git a/go.sum b/go.sum index f54a793347..6e92bf4cc2 100644 --- a/go.sum +++ b/go.sum @@ -318,8 +318,6 @@ github.com/hashicorp/go-checkpoint v0.5.0/go.mod h1:7nfLNL10NsxqO4iWuW6tWW0HjZuD github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-cleanhttp v0.5.1 h1:dH3aiDG9Jvb5r5+bYHsikaOUIpcM0xvgMXVoDkXMzJM= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= -github.com/hashicorp/go-getter v1.5.1 h1:lM9sM02nvEApQGFgkXxWbhfqtyN+AyhQmi+MaMdBDOI= -github.com/hashicorp/go-getter v1.5.1/go.mod h1:a7z7NPPfNQpJWcn4rSWFtdrSldqLdLPEF3d8nFMsSLM= github.com/hashicorp/go-getter v1.5.2 h1:XDo8LiAcDisiqZdv0TKgz+HtX3WN7zA2JD1R1tjsabE= github.com/hashicorp/go-getter v1.5.2/go.mod h1:orNH3BTYLu/fIxGIdLjLoAJHWMDQ/UKQr5O4m3iBuoo= github.com/hashicorp/go-hclog v0.14.1/go.mod h1:whpDNt7SSdeAju8AWKIWsul05p54N/39EeqMAyrmvFQ= @@ -360,8 +358,8 @@ github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f h1:UdxlrJz4JOnY8W+DbLISwf2B8WXEolNRA8BGCwI9jws= github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f/go.mod h1:oZtUIOe8dh44I2q6ScRibXws4Ajl+d+nod3AaR9vL5w= github.com/hashicorp/hcl/v2 v2.0.0/go.mod h1:oVVDG71tEinNGYCxinCYadcmKU9bglqW9pV3txagJ90= -github.com/hashicorp/hcl/v2 v2.9.1 h1:eOy4gREY0/ZQHNItlfuEZqtcQbXIxzojlP301hDpnac= -github.com/hashicorp/hcl/v2 v2.9.1/go.mod h1:FwWsfWEjyV/CMj8s/gqAuiviY72rJ1/oayI9WftqcKg= +github.com/hashicorp/hcl/v2 v2.10.0 h1:1S1UnuhDGlv3gRFV4+0EdwB+znNP5HmcGbIqwnSCByg= +github.com/hashicorp/hcl/v2 v2.10.0/go.mod h1:FwWsfWEjyV/CMj8s/gqAuiviY72rJ1/oayI9WftqcKg= github.com/hashicorp/memberlist v0.1.0 h1:qSsCiC0WYD39lbSitKNt40e30uorm2Ss/d4JGU1hzH8= github.com/hashicorp/memberlist v0.1.0/go.mod h1:ncdBp14cuox2iFOq3kDiquKU6fqsTBc3W6JvZwjxxsE= github.com/hashicorp/serf v0.0.0-20160124182025-e4ec8cc423bb h1:ZbgmOQt8DOg796figP87/EFCVx2v2h9yRvwHF/zceX4= From b649a8ddd118dbd1275faca0cf8a979324a94d6d Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Wed, 21 Apr 2021 11:41:22 -0400 Subject: [PATCH 27/38] deps: update go-plugin to v1.4.1 (#28465) + go mod tidy --- go.mod | 2 +- go.sum | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 752fa0f6f0..17dc513d41 100644 --- a/go.mod +++ b/go.mod @@ -57,7 +57,7 @@ require ( github.com/hashicorp/go-immutable-radix v0.0.0-20180129170900-7f3cd4390caa // indirect github.com/hashicorp/go-msgpack v0.5.4 // indirect github.com/hashicorp/go-multierror v1.1.1 - github.com/hashicorp/go-plugin v1.4.0 + github.com/hashicorp/go-plugin v1.4.1 github.com/hashicorp/go-retryablehttp v0.5.2 github.com/hashicorp/go-rootcerts v1.0.0 // indirect github.com/hashicorp/go-sockaddr v0.0.0-20180320115054-6d291a969b86 // indirect diff --git a/go.sum b/go.sum index f54a793347..f4d0444a4a 100644 --- a/go.sum +++ b/go.sum @@ -318,8 +318,6 @@ github.com/hashicorp/go-checkpoint v0.5.0/go.mod h1:7nfLNL10NsxqO4iWuW6tWW0HjZuD github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-cleanhttp v0.5.1 h1:dH3aiDG9Jvb5r5+bYHsikaOUIpcM0xvgMXVoDkXMzJM= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= -github.com/hashicorp/go-getter v1.5.1 h1:lM9sM02nvEApQGFgkXxWbhfqtyN+AyhQmi+MaMdBDOI= -github.com/hashicorp/go-getter v1.5.1/go.mod h1:a7z7NPPfNQpJWcn4rSWFtdrSldqLdLPEF3d8nFMsSLM= github.com/hashicorp/go-getter v1.5.2 h1:XDo8LiAcDisiqZdv0TKgz+HtX3WN7zA2JD1R1tjsabE= github.com/hashicorp/go-getter v1.5.2/go.mod h1:orNH3BTYLu/fIxGIdLjLoAJHWMDQ/UKQr5O4m3iBuoo= github.com/hashicorp/go-hclog v0.14.1/go.mod h1:whpDNt7SSdeAju8AWKIWsul05p54N/39EeqMAyrmvFQ= @@ -332,8 +330,8 @@ github.com/hashicorp/go-msgpack v0.5.4/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iP github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo= github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM= -github.com/hashicorp/go-plugin v1.4.0 h1:b0O7rs5uiJ99Iu9HugEzsM67afboErkHUWddUSpUO3A= -github.com/hashicorp/go-plugin v1.4.0/go.mod h1:5fGEH17QVwTTcR0zV7yhDPLLmFX9YSZ38b18Udy6vYQ= +github.com/hashicorp/go-plugin v1.4.1 h1:6UltRQlLN9iZO513VveELp5xyaFxVD2+1OVylE+2E+w= +github.com/hashicorp/go-plugin v1.4.1/go.mod h1:5fGEH17QVwTTcR0zV7yhDPLLmFX9YSZ38b18Udy6vYQ= github.com/hashicorp/go-retryablehttp v0.5.2 h1:AoISa4P4IsW0/m4T6St8Yw38gTl5GtBAgfkhYh1xAz4= github.com/hashicorp/go-retryablehttp v0.5.2/go.mod h1:9B5zBasrRhHXnJnui7y6sL7es7NDiJgTc6Er0maI1Xs= github.com/hashicorp/go-rootcerts v1.0.0 h1:Rqb66Oo1X/eSV1x66xbDccZjhJigjg0+e82kpwzSwCI= From bfd4c964eef9a86f80981049d89b31b2b5754c02 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 20 Apr 2021 15:09:36 -0700 Subject: [PATCH 28/38] dependencies: update cty from v1.8.1 to v1.8.2 This includes the improvements to various collection-related functions to make them handle marks more precisely. For Terraform in particular that translates into handling sensitivity more precisely, so that non-sensitive collections that happen to contain sensitive elements won't get simplified into wholly-sensitive collections when using these functions. --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 17dc513d41..2673b62b38 100644 --- a/go.mod +++ b/go.mod @@ -114,7 +114,7 @@ require ( github.com/xanzy/ssh-agent v0.2.1 github.com/xiang90/probing v0.0.0-20160813154853-07dd2e8dfe18 // indirect github.com/xlab/treeprint v0.0.0-20161029104018-1d6e34225557 - github.com/zclconf/go-cty v1.8.1 + github.com/zclconf/go-cty v1.8.2 github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b github.com/zclconf/go-cty-yaml v1.0.2 go.uber.org/atomic v1.3.2 // indirect diff --git a/go.sum b/go.sum index f4d0444a4a..eaeb380e2c 100644 --- a/go.sum +++ b/go.sum @@ -597,6 +597,8 @@ github.com/zclconf/go-cty v1.2.0/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q github.com/zclconf/go-cty v1.8.0/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk= github.com/zclconf/go-cty v1.8.1 h1:SI0LqNeNxAgv2WWqWJMlG2/Ad/6aYJ7IVYYMigmfkuI= github.com/zclconf/go-cty v1.8.1/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk= +github.com/zclconf/go-cty v1.8.2 h1:u+xZfBKgpycDnTNjPhGiTEYZS5qS/Sb5MqSfm7vzcjg= +github.com/zclconf/go-cty v1.8.2/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b h1:FosyBZYxY34Wul7O/MSKey3txpPYyCqVO5ZyceuQJEI= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b/go.mod h1:ZRKQfBXbGkpdV6QMzT3rU1kSTAnfu1dO8dPKjYprgj8= github.com/zclconf/go-cty-yaml v1.0.2 h1:dNyg4QLTrv2IfJpm7Wtxi55ed5gLGOlPrZ6kMd51hY0= From 43bf3832d5c8e02ff1fcd00411d2bf2165bc90ba Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 20 Apr 2021 15:49:54 -0700 Subject: [PATCH 29/38] core: Loosen output value sensitivity requirement Non-root module outputs no longer strip sensitivity marks from their values, allowing dynamically sensitive values to propagate through the configuration. We also remove the requirement for non-root module outputs to be defined as sensitive if the value is marked as sensitive. This avoids a static/dynamic clash when using shared modules that might unknowingly receive sensitive values via input variables. Co-authored-by: Martin Atkins --- terraform/context_plan2_test.go | 30 ++++++++++++++++++++++++++++++ terraform/context_validate_test.go | 20 ++++++-------------- terraform/evaluate.go | 2 +- terraform/node_output.go | 28 ++++++++++++++++++---------- 4 files changed, 55 insertions(+), 25 deletions(-) diff --git a/terraform/context_plan2_test.go b/terraform/context_plan2_test.go index f95f7f6e35..73bdb283b3 100644 --- a/terraform/context_plan2_test.go +++ b/terraform/context_plan2_test.go @@ -2,6 +2,7 @@ package terraform import ( "errors" + "strings" "testing" "github.com/hashicorp/terraform/addrs" @@ -485,3 +486,32 @@ provider "test" { t.Fatal(diags.Err()) } } + +func TestContext2Plan_invalidSensitiveModuleOutput(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "child/main.tf": ` +output "out" { + value = sensitive("xyz") +}`, + "main.tf": ` +module "child" { + source = "./child" +} + +output "root" { + value = module.child.out +}`, + }) + + ctx := testContext2(t, &ContextOpts{ + Config: m, + }) + + _, diags := ctx.Plan() + if !diags.HasErrors() { + t.Fatal("succeeded; want errors") + } + if got, want := diags.Err().Error(), "Output refers to sensitive values"; !strings.Contains(got, want) { + t.Fatalf("wrong error:\ngot: %s\nwant: message containing %q", got, want) + } +} diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 7e2b0a25ec..0361e30e44 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -1379,7 +1379,7 @@ resource "aws_instance" "foo" { } } -func TestContext2Validate_invalidSensitiveModuleOutput(t *testing.T) { +func TestContext2Validate_sensitiveRootModuleOutput(t *testing.T) { m := testModuleInline(t, map[string]string{ "child/main.tf": ` variable "foo" { @@ -1395,27 +1395,19 @@ module "child" { source = "./child" } -resource "aws_instance" "foo" { - foo = module.child.out +output "root" { + value = module.child.out + sensitive = true }`, }) - p := testProvider("aws") ctx := testContext2(t, &ContextOpts{ Config: m, - Providers: map[addrs.Provider]providers.Factory{ - addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), - }, }) diags := ctx.Validate() - if !diags.HasErrors() { - t.Fatal("succeeded; want errors") - } - // Should get this error: - // Output refers to sensitive values: Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true. - if got, want := diags.Err().Error(), "Output refers to sensitive values"; !strings.Contains(got, want) { - t.Fatalf("wrong error:\ngot: %s\nwant: message containing %q", got, want) + if diags.HasErrors() { + t.Fatal(diags.Err()) } } diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 92e50614d7..2417d2f64e 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -459,7 +459,7 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc continue } - instance[cfg.Name] = change.After + instance[cfg.Name] = change.After.MarkWithPaths(changeSrc.AfterValMarks) if change.Sensitive && !change.After.HasMark("sensitive") { instance[cfg.Name] = change.After.Mark("sensitive") diff --git a/terraform/node_output.go b/terraform/node_output.go index c70c76e9c7..4db2055c8f 100644 --- a/terraform/node_output.go +++ b/terraform/node_output.go @@ -275,16 +275,24 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) (diags // depends_on expressions here too diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn)) - // Ensure that non-sensitive outputs don't include sensitive values + // For root module outputs in particular, an output value must be + // statically declared as sensitive in order to dynamically return + // a sensitive result, to help avoid accidental exposure in the state + // of a sensitive value that the user doesn't want to include there. _, marks := val.UnmarkDeep() _, hasSensitive := marks["sensitive"] - if !n.Config.Sensitive && hasSensitive { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Output refers to sensitive values", - Detail: "Expressions used in outputs can only refer to sensitive values if the sensitive attribute is true.", - Subject: n.Config.DeclRange.Ptr(), - }) + if n.Addr.Module.IsRoot() { + if !n.Config.Sensitive && hasSensitive { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Output refers to sensitive values", + Detail: `To reduce the risk of accidentally exporting sensitive data that was intended to be only internal, Terraform requires that any root module output containing sensitive data be explicitly marked as sensitive, to confirm your intent. + +If you do intend to export this data, annotate the output value as sensitive by adding the following argument: + sensitive = true`, + Subject: n.Config.DeclRange.Ptr(), + }) + } } } @@ -454,7 +462,7 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C sensitiveChange := sensitiveBefore || n.Config.Sensitive // strip any marks here just to be sure we don't panic on the True comparison - val, _ = val.UnmarkDeep() + unmarkedVal, _ := val.UnmarkDeep() action := plans.Update switch { @@ -468,7 +476,7 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C action = plans.Create case val.IsWhollyKnown() && - val.Equals(before).True() && + unmarkedVal.Equals(before).True() && n.Config.Sensitive == sensitiveBefore: // Sensitivity must also match to be a NoOp. // Theoretically marks may not match here, but sensitivity is the From b34588ffca065354a358229463e36c8eabfa03e1 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Tue, 20 Apr 2021 12:29:23 -0400 Subject: [PATCH 30/38] lang/funcs: TransposeFunc Tests This commit adds test cases to TestTranspose to document how this function handles marks. The short version is that any marks anywhere will be applied to the return value, be they marks on the input map or marks on elements (either the entire list of strings, or individual elemnets of those lists). --- lang/funcs/collection_test.go | 93 +++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/lang/funcs/collection_test.go b/lang/funcs/collection_test.go index ad53cbb6cd..9cc428a919 100644 --- a/lang/funcs/collection_test.go +++ b/lang/funcs/collection_test.go @@ -1541,6 +1541,99 @@ func TestTranspose(t *testing.T) { cty.NilVal, true, }, + { // marks (deep or shallow) on any elements will propegate to the entire return value + cty.MapVal(map[string]cty.Value{ + "key1": cty.ListVal([]cty.Value{ + cty.StringVal("a").Mark("beep"), // mark on the inner list element + cty.StringVal("b"), + }), + "key2": cty.ListVal([]cty.Value{ + cty.StringVal("a"), + cty.StringVal("b"), + cty.StringVal("c"), + }).Mark("boop"), // mark on the map element + "key3": cty.ListVal([]cty.Value{ + cty.StringVal("c"), + }), + "key4": cty.ListValEmpty(cty.String), + }), + cty.MapVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("key1"), + cty.StringVal("key2"), + }), + "b": cty.ListVal([]cty.Value{ + cty.StringVal("key1"), + cty.StringVal("key2"), + }), + "c": cty.ListVal([]cty.Value{ + cty.StringVal("key2"), + cty.StringVal("key3")}), + }).WithMarks(cty.NewValueMarks("beep", "boop")), + false, + }, + { // Marks on the input value will be applied to the return value + cty.MapVal(map[string]cty.Value{ + "key1": cty.ListVal([]cty.Value{ + cty.StringVal("a"), + cty.StringVal("b"), + }), + "key2": cty.ListVal([]cty.Value{ + cty.StringVal("a"), + cty.StringVal("b"), + cty.StringVal("c"), + }), + "key3": cty.ListVal([]cty.Value{ + cty.StringVal("c"), + }), + }).Mark("beep"), // mark on the entire input value + cty.MapVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("key1"), + cty.StringVal("key2"), + }), + "b": cty.ListVal([]cty.Value{ + cty.StringVal("key1"), + cty.StringVal("key2"), + }), + "c": cty.ListVal([]cty.Value{ + cty.StringVal("key2"), + cty.StringVal("key3"), + }), + }).Mark("beep"), + false, + }, + { // Marks on the entire input value AND inner elements (deep or shallow) ALL apply to the return + cty.MapVal(map[string]cty.Value{ + "key1": cty.ListVal([]cty.Value{ + cty.StringVal("a"), + cty.StringVal("b"), + }).Mark("beep"), // mark on the map element + "key2": cty.ListVal([]cty.Value{ + cty.StringVal("a"), + cty.StringVal("b"), + cty.StringVal("c"), + }), + "key3": cty.ListVal([]cty.Value{ + cty.StringVal("c").Mark("boop"), // mark on the inner list element + }), + }).Mark("bloop"), // mark on the entire input value + cty.MapVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("key1"), + cty.StringVal("key2"), + }), + "b": cty.ListVal([]cty.Value{ + cty.StringVal("key1"), + cty.StringVal("key2"), + }), + "c": cty.ListVal([]cty.Value{ + cty.StringVal("key2"), + cty.StringVal("key3"), + }), + }).WithMarks(cty.NewValueMarks("beep", "boop", "bloop")), + false, + }, } for _, test := range tests { From 5ae4c2f92b3367d47d49ef3bcd39a52790dee401 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Wed, 21 Apr 2021 11:09:31 -0400 Subject: [PATCH 31/38] lang/funcs: Defaults handling of marked arguments Defaults will now preserve marks from non-null inputs and apply marks from any default values used. I've added tests for various structural types with marks, as well as some basic unknown cases. --- lang/funcs/defaults.go | 58 +++++++++------- lang/funcs/defaults_test.go | 128 ++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 25 deletions(-) diff --git a/lang/funcs/defaults.go b/lang/funcs/defaults.go index 0e4fd41ffa..c1b854be3b 100644 --- a/lang/funcs/defaults.go +++ b/lang/funcs/defaults.go @@ -16,13 +16,15 @@ import ( var DefaultsFunc = function.New(&function.Spec{ Params: []function.Parameter{ { - Name: "input", - Type: cty.DynamicPseudoType, - AllowNull: true, + Name: "input", + Type: cty.DynamicPseudoType, + AllowNull: true, + AllowMarked: true, }, { - Name: "defaults", - Type: cty.DynamicPseudoType, + Name: "defaults", + Type: cty.DynamicPseudoType, + AllowMarked: true, }, }, Type: func(args []cty.Value) (cty.Type, error) { @@ -69,8 +71,14 @@ var DefaultsFunc = function.New(&function.Spec{ func defaultsApply(input, fallback cty.Value) cty.Value { wantTy := input.Type() - if !(input.IsKnown() && fallback.IsKnown()) { - return cty.UnknownVal(wantTy) + + umInput, inputMarks := input.Unmark() + umFb, fallbackMarks := fallback.Unmark() + + // If neither are known, we very conservatively return an unknown value + // with the union of marks on both input and default. + if !(umInput.IsKnown() && umFb.IsKnown()) { + return cty.UnknownVal(wantTy).WithMarks(inputMarks).WithMarks(fallbackMarks) } // For the rest of this function we're assuming that the given defaults @@ -83,15 +91,15 @@ func defaultsApply(input, fallback cty.Value) cty.Value { case wantTy.IsPrimitiveType(): // For leaf primitive values the rule is relatively simple: use the // input if it's non-null, or fallback if input is null. - if !input.IsNull() { + if !umInput.IsNull() { return input } - v, err := convert.Convert(fallback, wantTy) + v, err := convert.Convert(umFb, wantTy) if err != nil { // Should not happen because we checked in defaultsAssertSuitableFallback panic(err.Error()) } - return v + return v.WithMarks(fallbackMarks) case wantTy.IsObjectType(): // For structural types, a null input value must be passed through. We @@ -101,18 +109,18 @@ func defaultsApply(input, fallback cty.Value) cty.Value { // We also pass through the input if the fallback value is null. This // can happen if the given defaults do not include a value for this // attribute. - if input.IsNull() || fallback.IsNull() { + if umInput.IsNull() || umFb.IsNull() { return input } atys := wantTy.AttributeTypes() ret := map[string]cty.Value{} for attr, aty := range atys { - inputSub := input.GetAttr(attr) + inputSub := umInput.GetAttr(attr) fallbackSub := cty.NullVal(aty) - if fallback.Type().HasAttribute(attr) { - fallbackSub = fallback.GetAttr(attr) + if umFb.Type().HasAttribute(attr) { + fallbackSub = umFb.GetAttr(attr) } - ret[attr] = defaultsApply(inputSub, fallbackSub) + ret[attr] = defaultsApply(inputSub.WithMarks(inputMarks), fallbackSub.WithMarks(fallbackMarks)) } return cty.ObjectVal(ret) @@ -124,16 +132,16 @@ func defaultsApply(input, fallback cty.Value) cty.Value { // We also pass through the input if the fallback value is null. This // can happen if the given defaults do not include a value for this // attribute. - if input.IsNull() || fallback.IsNull() { + if umInput.IsNull() || umFb.IsNull() { return input } l := wantTy.Length() ret := make([]cty.Value, l) for i := 0; i < l; i++ { - inputSub := input.Index(cty.NumberIntVal(int64(i))) - fallbackSub := fallback.Index(cty.NumberIntVal(int64(i))) - ret[i] = defaultsApply(inputSub, fallbackSub) + inputSub := umInput.Index(cty.NumberIntVal(int64(i))) + fallbackSub := umFb.Index(cty.NumberIntVal(int64(i))) + ret[i] = defaultsApply(inputSub.WithMarks(inputMarks), fallbackSub.WithMarks(fallbackMarks)) } return cty.TupleVal(ret) @@ -148,10 +156,10 @@ func defaultsApply(input, fallback cty.Value) cty.Value { case wantTy.IsMapType(): newVals := map[string]cty.Value{} - if !input.IsNull() { - for it := input.ElementIterator(); it.Next(); { + if !umInput.IsNull() { + for it := umInput.ElementIterator(); it.Next(); { k, v := it.Element() - newVals[k.AsString()] = defaultsApply(v, fallback) + newVals[k.AsString()] = defaultsApply(v.WithMarks(inputMarks), fallback.WithMarks(fallbackMarks)) } } @@ -162,10 +170,10 @@ func defaultsApply(input, fallback cty.Value) cty.Value { case wantTy.IsListType(), wantTy.IsSetType(): var newVals []cty.Value - if !input.IsNull() { - for it := input.ElementIterator(); it.Next(); { + if !umInput.IsNull() { + for it := umInput.ElementIterator(); it.Next(); { _, v := it.Element() - newV := defaultsApply(v, fallback) + newV := defaultsApply(v.WithMarks(inputMarks), fallback.WithMarks(fallbackMarks)) newVals = append(newVals, newV) } } diff --git a/lang/funcs/defaults_test.go b/lang/funcs/defaults_test.go index 3d36d75cc8..e40163265a 100644 --- a/lang/funcs/defaults_test.go +++ b/lang/funcs/defaults_test.go @@ -13,6 +13,30 @@ func TestDefaults(t *testing.T) { Want cty.Value WantErr string }{ + { // When *either* input or default are unknown, an unknown is returned. + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.UnknownVal(cty.String), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello"), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.UnknownVal(cty.String), + }), + }, + { + // When *either* input or default are unknown, an unknown is + // returned with marks from both input and defaults. + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.UnknownVal(cty.String), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello").Mark("marked"), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.UnknownVal(cty.String).Mark("marked"), + }), + }, { Input: cty.ObjectVal(map[string]cty.Value{ "a": cty.NullVal(cty.String), @@ -494,6 +518,110 @@ func TestDefaults(t *testing.T) { }), WantErr: ".a: invalid default value for bool: bool required", }, + // marks: we should preserve marks from both input value and defaults as leafily as possible + { + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.NullVal(cty.String), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello").Mark("world"), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello").Mark("world"), + }), + }, + { // "unused" marks don't carry over + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.NullVal(cty.String).Mark("a"), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello"), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello"), + }), + }, + { // Marks on tuples remain attached to individual elements + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.TupleVal([]cty.Value{ + cty.NullVal(cty.String), + cty.StringVal("hey").Mark("input"), + cty.NullVal(cty.String), + }), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.TupleVal([]cty.Value{ + cty.StringVal("hello 0").Mark("fallback"), + cty.StringVal("hello 1"), + cty.StringVal("hello 2"), + }), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.TupleVal([]cty.Value{ + cty.StringVal("hello 0").Mark("fallback"), + cty.StringVal("hey").Mark("input"), + cty.StringVal("hello 2"), + }), + }), + }, + { // Marks from list elements + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.NullVal(cty.String), + cty.StringVal("hey").Mark("input"), + cty.NullVal(cty.String), + }), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello 0").Mark("fallback"), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("hello 0").Mark("fallback"), + cty.StringVal("hey").Mark("input"), + cty.StringVal("hello 0").Mark("fallback"), + }), + }), + }, + { + // Sets don't allow individually-marked elements, so the marks + // end up aggregating on the set itself anyway in this case. + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.SetVal([]cty.Value{ + cty.NullVal(cty.String), + cty.NullVal(cty.String), + cty.StringVal("hey").Mark("input"), + }), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello 0").Mark("fallback"), + }), + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.SetVal([]cty.Value{ + cty.StringVal("hello 0"), + cty.StringVal("hey"), + cty.StringVal("hello 0"), + }).WithMarks(cty.NewValueMarks("fallback", "input")), + }), + }, + { + Input: cty.ObjectVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.NullVal(cty.String), + }), + }), + Defaults: cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("hello").Mark("beep"), + }).Mark("boop"), + // This is the least-intuitive case. The mark "boop" is attached to + // the default object, not it's elements, but both marks end up + // aggregated on the list element. + Want: cty.ObjectVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("hello").WithMarks(cty.NewValueMarks("beep", "boop")), + }), + }), + }, } for _, test := range tests { From 58cba1dea382ff3c33f0f00e5fa2a06ca8398444 Mon Sep 17 00:00:00 2001 From: Chris Arcand Date: Wed, 21 Apr 2021 21:23:42 -0500 Subject: [PATCH 32/38] Add customized login success output for TFC/E When logging in to Terraform Cloud or Terraform Enterprise, change the success output to be a bit more customized for the platform. For Terraform Cloud, fetch a dynamic welcome banner that intentionally fails open and defaults to a hardcoded message if its not available for any reason. --- command/login.go | 97 +++++++++++++++++-- command/login_test.go | 46 ++++++--- .../testdata/login-tfe-server/tfeserver.go | 8 ++ 3 files changed, 127 insertions(+), 24 deletions(-) diff --git a/command/login.go b/command/login.go index 828eee74a0..b77dc342df 100644 --- a/command/login.go +++ b/command/login.go @@ -4,8 +4,10 @@ import ( "context" "crypto/sha256" "encoding/base64" + "encoding/json" "errors" "fmt" + "io/ioutil" "log" "math/rand" "net" @@ -131,9 +133,9 @@ func (c *LoginCommand) Run(args []string) int { } // If login service is unavailable, check for a TFE v2 API as fallback - var service *url.URL + var tfeservice *url.URL if clientConfig == nil { - service, err = host.ServiceURL("tfe.v2") + tfeservice, err = host.ServiceURL("tfe.v2") switch err.(type) { case nil: // Success! @@ -184,6 +186,8 @@ func (c *LoginCommand) Run(args []string) int { oauthToken, tokenDiags = c.interactiveGetTokenByCode(hostname, credsCtx, clientConfig) case clientConfig.SupportedGrantTypes.Has(disco.OAuthOwnerPasswordGrant) && hostname == svchost.Hostname("app.terraform.io"): // The password grant type is allowed only for Terraform Cloud SaaS. + // Note this case is purely theoretical at this point, as TFC currently uses + // its own bespoke login protocol (tfe) oauthToken, tokenDiags = c.interactiveGetTokenByPassword(hostname, credsCtx, clientConfig) default: tokenDiags = tokenDiags.Append(tfdiags.Sourceless( @@ -195,8 +199,8 @@ func (c *LoginCommand) Run(args []string) int { if oauthToken != nil { token = svcauth.HostCredentialsToken(oauthToken.AccessToken) } - } else if service != nil { - token, tokenDiags = c.interactiveGetTokenByUI(hostname, credsCtx, service) + } else if tfeservice != nil { + token, tokenDiags = c.interactiveGetTokenByUI(hostname, credsCtx, tfeservice) } diags = diags.Append(tokenDiags) @@ -220,19 +224,96 @@ func (c *LoginCommand) Run(args []string) int { } c.Ui.Output("\n---------------------------------------------------------------------------------\n") - c.Ui.Output( - fmt.Sprintf( - c.Colorize().Color(strings.TrimSpace(` + if hostname == "app.terraform.io" { // Terraform Cloud + var motd struct { + Message string `json:"msg"` + Errors []interface{} `json:"errors"` + } + + // Throughout the entire process of fetching a MOTD from TFC, use a default + // message if the platform-provided message is unavailable for any reason - + // be it the service isn't provided, the request failed, or any sort of + // platform error returned. + + motdServiceURL, err := host.ServiceURL("motd.v1") + if err != nil { + c.outputDefaultTFCLoginSuccess(err) + return 0 + } + + req, err := http.NewRequest("GET", motdServiceURL.String(), nil) + if err != nil { + c.outputDefaultTFCLoginSuccess(err) + return 0 + } + + req.Header.Set("Authorization", "Bearer "+token.Token()) + + resp, err := httpclient.New().Do(req) + if err != nil { + c.outputDefaultTFCLoginSuccess(err) + return 0 + } + + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + c.outputDefaultTFCLoginSuccess(err) + return 0 + } + + defer resp.Body.Close() + json.Unmarshal(body, &motd) + + if motd.Errors == nil && motd.Message != "" { + c.Ui.Output( + c.Colorize().Color(motd.Message), + ) + return 0 + } else { + c.outputDefaultTFCLoginSuccess(fmt.Errorf("platform responded with errors or an empty message")) + return 0 + } + } + + if tfeservice != nil { // Terraform Enterprise + c.outputDefaultTFELoginSuccess(dispHostname) + } else { + c.Ui.Output( + fmt.Sprintf( + c.Colorize().Color(strings.TrimSpace(` [green][bold]Success![reset] [bold]Terraform has obtained and saved an API token.[reset] The new API token will be used for any future Terraform command that must make authenticated requests to %s. +`)), + dispHostname, + ) + "\n", + ) + } + + return 0 +} + +func (c *LoginCommand) outputDefaultTFELoginSuccess(dispHostname string) { + c.Ui.Output( + fmt.Sprintf( + c.Colorize().Color(strings.TrimSpace(` +[green][bold]Success![reset] [bold]Logged in to Terraform Enterprise (%s)[reset] `)), dispHostname, ) + "\n", ) +} - return 0 +func (c *LoginCommand) outputDefaultTFCLoginSuccess(err error) { + log.Printf("[TRACE] login: An error occurred attempting to fetch a message of the day for Terraform Cloud: %s", err) + c.Ui.Output( + fmt.Sprintf( + c.Colorize().Color(strings.TrimSpace(` +[green][bold]Success![reset] [bold]Logged in to Terraform Cloud[reset] +`)), + ) + "\n", + ) } // Help implements cli.Command. diff --git a/command/login_test.go b/command/login_test.go index d0450099aa..9b0e8de960 100644 --- a/command/login_test.go +++ b/command/login_test.go @@ -56,16 +56,6 @@ func TestLogin(t *testing.T) { svcs := disco.NewWithCredentialsSource(creds) svcs.SetUserAgent(httpclient.TerraformUserAgent(version.String())) - svcs.ForceHostServices(svchost.Hostname("app.terraform.io"), map[string]interface{}{ - "login.v1": map[string]interface{}{ - // On app.terraform.io we use password-based authorization. - // That's the only hostname that it's permitted for, so we can't - // use a fake hostname here. - "client": "terraformcli", - "token": s.URL + "/token", - "grant_types": []interface{}{"password"}, - }, - }) svcs.ForceHostServices(svchost.Hostname("example.com"), map[string]interface{}{ "login.v1": map[string]interface{}{ // For this fake hostname we'll use a conventional OAuth flow, @@ -86,9 +76,17 @@ func TestLogin(t *testing.T) { "scopes": []interface{}{"app1.full_access", "app2.read_only"}, }, }) + svcs.ForceHostServices(svchost.Hostname("app.terraform.io"), map[string]interface{}{ + // This represents Terraform Cloud, which does not yet support the + // login API, but does support its own bespoke tokens API. + "tfe.v2": ts.URL + "/api/v2", + "tfe.v2.1": ts.URL + "/api/v2", + "tfe.v2.2": ts.URL + "/api/v2", + "motd.v1": ts.URL + "/api/terraform/motd", + }) svcs.ForceHostServices(svchost.Hostname("tfe.acme.com"), map[string]interface{}{ // This represents a Terraform Enterprise instance which does not - // yet support the login API, but does support the TFE tokens API. + // yet support the login API, but does support its own bespoke tokens API. "tfe.v2": ts.URL + "/api/v2", "tfe.v2.1": ts.URL + "/api/v2", "tfe.v2.2": ts.URL + "/api/v2", @@ -109,13 +107,14 @@ func TestLogin(t *testing.T) { } } - t.Run("defaulting to app.terraform.io with password flow", loginTestCase(func(t *testing.T, c *LoginCommand, ui *cli.MockUi) { + t.Run("app.terraform.io (no login support)", loginTestCase(func(t *testing.T, c *LoginCommand, ui *cli.MockUi) { + // Enter "yes" at the consent prompt, then paste a token with some + // accidental whitespace. defer testInputMap(t, map[string]string{ - "approve": "yes", - "username": "foo", - "password": "bar", + "approve": "yes", + "token": " good-token ", })() - status := c.Run(nil) + status := c.Run([]string{"app.terraform.io"}) if status != 0 { t.Fatalf("unexpected error code %d\nstderr:\n%s", status, ui.ErrorWriter.String()) } @@ -128,6 +127,9 @@ func TestLogin(t *testing.T) { if got, want := creds.Token(), "good-token"; got != want { t.Errorf("wrong token %q; want %q", got, want) } + if got, want := ui.OutputWriter.String(), "Welcome to Terraform Cloud!"; !strings.Contains(got, want) { + t.Errorf("expected output to contain %q, but was:\n%s", want, got) + } })) t.Run("example.com with authorization code flow", loginTestCase(func(t *testing.T, c *LoginCommand, ui *cli.MockUi) { @@ -148,6 +150,10 @@ func TestLogin(t *testing.T) { if got, want := creds.Token(), "good-token"; got != want { t.Errorf("wrong token %q; want %q", got, want) } + + if got, want := ui.OutputWriter.String(), "Terraform has obtained and saved an API token."; !strings.Contains(got, want) { + t.Errorf("expected output to contain %q, but was:\n%s", want, got) + } })) t.Run("example.com results in no scopes", loginTestCase(func(t *testing.T, c *LoginCommand, ui *cli.MockUi) { @@ -179,6 +185,10 @@ func TestLogin(t *testing.T) { if got, want := creds.Token(), "good-token"; got != want { t.Errorf("wrong token %q; want %q", got, want) } + + if got, want := ui.OutputWriter.String(), "Terraform has obtained and saved an API token."; !strings.Contains(got, want) { + t.Errorf("expected output to contain %q, but was:\n%s", want, got) + } })) t.Run("with-scopes.example.com results in expected scopes", loginTestCase(func(t *testing.T, c *LoginCommand, ui *cli.MockUi) { @@ -216,6 +226,10 @@ func TestLogin(t *testing.T) { if got, want := creds.Token(), "good-token"; got != want { t.Errorf("wrong token %q; want %q", got, want) } + + if got, want := ui.OutputWriter.String(), "Logged in to Terraform Enterprise"; !strings.Contains(got, want) { + t.Errorf("expected output to contain %q, but was:\n%s", want, got) + } })) t.Run("TFE host without login support, incorrectly pasted token", loginTestCase(func(t *testing.T, c *LoginCommand, ui *cli.MockUi) { diff --git a/command/testdata/login-tfe-server/tfeserver.go b/command/testdata/login-tfe-server/tfeserver.go index 11d164abc9..111c0712cc 100644 --- a/command/testdata/login-tfe-server/tfeserver.go +++ b/command/testdata/login-tfe-server/tfeserver.go @@ -11,6 +11,7 @@ import ( const ( goodToken = "good-token" accountDetails = `{"data":{"id":"user-abc123","type":"users","attributes":{"username":"testuser","email":"testuser@example.com"}}}` + MOTD = `{"msg":"Welcome to Terraform Cloud!"}` ) // Handler is an implementation of net/http.Handler that provides a stub @@ -29,6 +30,8 @@ func (h handler) ServeHTTP(resp http.ResponseWriter, req *http.Request) { h.servePing(resp, req) case "/api/v2/account/details": h.serveAccountDetails(resp, req) + case "/api/terraform/motd": + h.serveMOTD(resp, req) default: fmt.Printf("404 when fetching %s\n", req.URL.String()) http.Error(resp, `{"errors":[{"status":"404","title":"not found"}]}`, http.StatusNotFound) @@ -49,6 +52,11 @@ func (h handler) serveAccountDetails(resp http.ResponseWriter, req *http.Request resp.Write([]byte(accountDetails)) } +func (h handler) serveMOTD(resp http.ResponseWriter, req *http.Request) { + resp.WriteHeader(http.StatusOK) + resp.Write([]byte(MOTD)) +} + func init() { Handler = handler{} } From 4b97411890909e9b3ad9f72ac2c4ddd13ee1fcd5 Mon Sep 17 00:00:00 2001 From: Chris Arcand Date: Wed, 21 Apr 2021 21:27:19 -0500 Subject: [PATCH 33/38] Add purple to the list of colorstring codes This is the first color code introduced for 256 color terminals, and should be a safe addition when considering the oldest supported platform. --- command/meta.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/command/meta.go b/command/meta.go index 69f01ebd83..1cad1bffe4 100644 --- a/command/meta.go +++ b/command/meta.go @@ -250,8 +250,14 @@ func (m *Meta) StateOutPath() string { // Colorize returns the colorization structure for a command. func (m *Meta) Colorize() *colorstring.Colorize { + colors := make(map[string]string) + for k, v := range colorstring.DefaultColors { + colors[k] = v + } + colors["purple"] = "38;5;57" + return &colorstring.Colorize{ - Colors: colorstring.DefaultColors, + Colors: colors, Disable: !m.color, Reset: true, } From 58ec68063b3bd0db6d2db9d61c5cb749880b75ec Mon Sep 17 00:00:00 2001 From: Chris Arcand Date: Thu, 22 Apr 2021 08:55:59 -0500 Subject: [PATCH 34/38] Split off error MOTD error logging The error logging in outputDefaultTFCLoginSuccess() is an unrelated side effect. --- command/login.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/command/login.go b/command/login.go index b77dc342df..3a673b8eef 100644 --- a/command/login.go +++ b/command/login.go @@ -237,13 +237,15 @@ func (c *LoginCommand) Run(args []string) int { motdServiceURL, err := host.ServiceURL("motd.v1") if err != nil { - c.outputDefaultTFCLoginSuccess(err) + c.logMOTDError(err) + c.outputDefaultTFCLoginSuccess() return 0 } req, err := http.NewRequest("GET", motdServiceURL.String(), nil) if err != nil { - c.outputDefaultTFCLoginSuccess(err) + c.logMOTDError(err) + c.outputDefaultTFCLoginSuccess() return 0 } @@ -251,13 +253,15 @@ func (c *LoginCommand) Run(args []string) int { resp, err := httpclient.New().Do(req) if err != nil { - c.outputDefaultTFCLoginSuccess(err) + c.logMOTDError(err) + c.outputDefaultTFCLoginSuccess() return 0 } body, err := ioutil.ReadAll(resp.Body) if err != nil { - c.outputDefaultTFCLoginSuccess(err) + c.logMOTDError(err) + c.outputDefaultTFCLoginSuccess() return 0 } @@ -270,7 +274,8 @@ func (c *LoginCommand) Run(args []string) int { ) return 0 } else { - c.outputDefaultTFCLoginSuccess(fmt.Errorf("platform responded with errors or an empty message")) + c.logMOTDError(fmt.Errorf("platform responded with errors or an empty message")) + c.outputDefaultTFCLoginSuccess() return 0 } } @@ -305,8 +310,7 @@ func (c *LoginCommand) outputDefaultTFELoginSuccess(dispHostname string) { ) } -func (c *LoginCommand) outputDefaultTFCLoginSuccess(err error) { - log.Printf("[TRACE] login: An error occurred attempting to fetch a message of the day for Terraform Cloud: %s", err) +func (c *LoginCommand) outputDefaultTFCLoginSuccess() { c.Ui.Output( fmt.Sprintf( c.Colorize().Color(strings.TrimSpace(` @@ -316,6 +320,10 @@ func (c *LoginCommand) outputDefaultTFCLoginSuccess(err error) { ) } +func (c *LoginCommand) logMOTDError(err error) { + log.Printf("[TRACE] login: An error occurred attempting to fetch a message of the day for Terraform Cloud: %s", err) +} + // Help implements cli.Command. func (c *LoginCommand) Help() string { defaultFile := c.defaultOutputFile() From f6af7b4f7a29874775995d2b7bbf546fc6854513 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Fri, 23 Apr 2021 10:29:50 -0400 Subject: [PATCH 35/38] lang/funcs: add (console-only) TypeFunction (#28501) * lang/funcs: add (console-only) TypeFunction The type() function, which is only available for terraform console, prints out the type of a given value. This is mainly intended for debugging - it's nice to be able to print out terraform's understanding of a complex variable. This introduces a new field for Scope: ConsoleMode. When ConsoleMode is true, any additional functions intended for use in the console (only) may be added. --- command/console.go | 4 + go.sum | 2 - lang/funcs/conversion.go | 128 ++++++++++++++++++ lang/funcs/conversion_test.go | 90 ++++++++++++ lang/functions.go | 5 + lang/scope.go | 4 + repl/format.go | 6 +- .../docs/configuration/functions/type.html.md | 82 +++++++++++ 8 files changed, 318 insertions(+), 3 deletions(-) create mode 100644 website/docs/configuration/functions/type.html.md diff --git a/command/console.go b/command/console.go index fab428aa6a..ebecd9edf0 100644 --- a/command/console.go +++ b/command/console.go @@ -127,6 +127,10 @@ func (c *ConsoleCommand) Run(args []string) int { c.showDiagnostics(diags) return 1 } + + // set the ConsoleMode to true so any available console-only functions included. + scope.ConsoleMode = true + if diags.HasErrors() { diags = diags.Append(tfdiags.SimpleWarning("Due to the problems above, some expressions may produce unexpected results.")) } diff --git a/go.sum b/go.sum index eaeb380e2c..84b6ca85f4 100644 --- a/go.sum +++ b/go.sum @@ -595,8 +595,6 @@ github.com/zclconf/go-cty v1.0.0/go.mod h1:xnAOWiHeOqg2nWS62VtQ7pbOu17FtxJNW8RLE github.com/zclconf/go-cty v1.1.0/go.mod h1:xnAOWiHeOqg2nWS62VtQ7pbOu17FtxJNW8RLEih+O3s= github.com/zclconf/go-cty v1.2.0/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q/N4PQ8= github.com/zclconf/go-cty v1.8.0/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk= -github.com/zclconf/go-cty v1.8.1 h1:SI0LqNeNxAgv2WWqWJMlG2/Ad/6aYJ7IVYYMigmfkuI= -github.com/zclconf/go-cty v1.8.1/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk= github.com/zclconf/go-cty v1.8.2 h1:u+xZfBKgpycDnTNjPhGiTEYZS5qS/Sb5MqSfm7vzcjg= github.com/zclconf/go-cty v1.8.2/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b h1:FosyBZYxY34Wul7O/MSKey3txpPYyCqVO5ZyceuQJEI= diff --git a/lang/funcs/conversion.go b/lang/funcs/conversion.go index 4991f60a85..b557bb5a9e 100644 --- a/lang/funcs/conversion.go +++ b/lang/funcs/conversion.go @@ -1,7 +1,10 @@ package funcs import ( + "fmt" + "sort" "strconv" + "strings" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" @@ -91,3 +94,128 @@ func MakeToFunc(wantTy cty.Type) function.Function { }, }) } + +var TypeFunc = function.New(&function.Spec{ + Params: []function.Parameter{ + { + Name: "value", + Type: cty.DynamicPseudoType, + AllowDynamicType: true, + AllowUnknown: true, + AllowNull: true, + }, + }, + Type: function.StaticReturnType(cty.String), + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { + return cty.StringVal(TypeString(args[0].Type())).Mark("raw"), nil + }, +}) + +// Modified copy of TypeString from go-cty: +// https://github.com/zclconf/go-cty-debug/blob/master/ctydebug/type_string.go +// +// TypeString returns a string representation of a given type that is +// reminiscent of Go syntax calling into the cty package but is mainly +// intended for easy human inspection of values in tests, debug output, etc. +// +// The resulting string will include newlines and indentation in order to +// increase the readability of complex structures. It always ends with a +// newline, so you can print this result directly to your output. +func TypeString(ty cty.Type) string { + var b strings.Builder + writeType(ty, &b, 0) + return b.String() +} + +func writeType(ty cty.Type, b *strings.Builder, indent int) { + switch { + case ty == cty.NilType: + b.WriteString("nil") + return + case ty.IsObjectType(): + atys := ty.AttributeTypes() + if len(atys) == 0 { + b.WriteString("object({})") + return + } + attrNames := make([]string, 0, len(atys)) + for name := range atys { + attrNames = append(attrNames, name) + } + sort.Strings(attrNames) + b.WriteString("object({\n") + indent++ + for _, name := range attrNames { + aty := atys[name] + b.WriteString(indentSpaces(indent)) + fmt.Fprintf(b, "%s: ", name) + writeType(aty, b, indent) + b.WriteString(",\n") + } + indent-- + b.WriteString(indentSpaces(indent)) + b.WriteString("})") + case ty.IsTupleType(): + etys := ty.TupleElementTypes() + if len(etys) == 0 { + b.WriteString("tuple([])") + return + } + b.WriteString("tuple([\n") + indent++ + for _, ety := range etys { + b.WriteString(indentSpaces(indent)) + writeType(ety, b, indent) + b.WriteString(",\n") + } + indent-- + b.WriteString(indentSpaces(indent)) + b.WriteString("])") + case ty.IsCollectionType(): + ety := ty.ElementType() + switch { + case ty.IsListType(): + b.WriteString("list(") + case ty.IsMapType(): + b.WriteString("map(") + case ty.IsSetType(): + b.WriteString("set(") + default: + // At the time of writing there are no other collection types, + // but we'll be robust here and just pass through the GoString + // of anything we don't recognize. + b.WriteString(ty.FriendlyName()) + return + } + // Because object and tuple types render split over multiple + // lines, a collection type container around them can end up + // being hard to see when scanning, so we'll generate some extra + // indentation to make a collection of structural type more visually + // distinct from the structural type alone. + complexElem := ety.IsObjectType() || ety.IsTupleType() + if complexElem { + indent++ + b.WriteString("\n") + b.WriteString(indentSpaces(indent)) + } + writeType(ty.ElementType(), b, indent) + if complexElem { + indent-- + b.WriteString(",\n") + b.WriteString(indentSpaces(indent)) + } + b.WriteString(")") + default: + // For any other type we'll just use its GoString and assume it'll + // follow the usual GoString conventions. + b.WriteString(ty.FriendlyName()) + } +} + +func indentSpaces(level int) string { + return strings.Repeat(" ", level) +} + +func Type(input []cty.Value) (cty.Value, error) { + return TypeFunc.Call(input) +} diff --git a/lang/funcs/conversion_test.go b/lang/funcs/conversion_test.go index 819a0e4d00..97460f5b5b 100644 --- a/lang/funcs/conversion_test.go +++ b/lang/funcs/conversion_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/google/go-cmp/cmp" "github.com/zclconf/go-cty/cty" ) @@ -177,3 +178,92 @@ func TestTo(t *testing.T) { }) } } + +func TestType(t *testing.T) { + tests := []struct { + Input cty.Value + Want string + }{ + // Primititves + { + cty.StringVal("a"), + "string", + }, + { + cty.NumberIntVal(42), + "number", + }, + { + cty.BoolVal(true), + "bool", + }, + // Collections + { + cty.EmptyObjectVal, + `object({})`, + }, + { + cty.EmptyTupleVal, + `tuple([])`, + }, + { + cty.ListValEmpty(cty.String), + `list(string)`, + }, + { + cty.MapValEmpty(cty.String), + `map(string)`, + }, + { + cty.SetValEmpty(cty.String), + `set(string)`, + }, + { + cty.ListVal([]cty.Value{cty.StringVal("a")}), + `list(string)`, + }, + { + cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.NumberIntVal(42)})}), + `list(list(number))`, + }, + { + cty.ListVal([]cty.Value{cty.MapValEmpty(cty.String)}), + `list(map(string))`, + }, + { + cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("bar"), + })}), + "list(\n object({\n foo: string,\n }),\n)", + }, + // Unknowns and Nulls + { + cty.UnknownVal(cty.String), + "string", + }, + { + cty.NullVal(cty.Object(map[string]cty.Type{ + "foo": cty.String, + })), + "object({\n foo: string,\n})", + }, + { // irrelevant marks do nothing + cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("bar").Mark("ignore me"), + })}), + "list(\n object({\n foo: string,\n }),\n)", + }, + } + for _, test := range tests { + got, err := Type([]cty.Value{test.Input}) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + // The value is marked to help with formatting + got, _ = got.Unmark() + + if got.AsString() != test.Want { + t.Errorf("wrong result:\n%s", cmp.Diff(got.AsString(), test.Want)) + } + } +} diff --git a/lang/functions.go b/lang/functions.go index aa182d17ed..21be9a7f10 100644 --- a/lang/functions.go +++ b/lang/functions.go @@ -152,6 +152,11 @@ func (s *Scope) Functions() map[string]function.Function { return s.funcs }) + if s.ConsoleMode { + // The type function is only available in terraform console. + s.funcs["type"] = funcs.TypeFunc + } + if s.PureOnly { // Force our few impure functions to return unknown so that we // can defer evaluating them until a later pass. diff --git a/lang/scope.go b/lang/scope.go index 103d2529c6..3a34e9ca2a 100644 --- a/lang/scope.go +++ b/lang/scope.go @@ -37,6 +37,10 @@ type Scope struct { // considered as active in the module that this scope will be used for. // Callers can populate it by calling the SetActiveExperiments method. activeExperiments experiments.Set + + // ConsoleMode can be set to true to request any console-only functions are + // included in this scope. + ConsoleMode bool } // SetActiveExperiments allows a caller to declare that a set of experiments diff --git a/repl/format.go b/repl/format.go index 14e09d7082..c83e9ef2eb 100644 --- a/repl/format.go +++ b/repl/format.go @@ -16,7 +16,11 @@ func FormatValue(v cty.Value, indent int) string { if !v.IsKnown() { return "(known after apply)" } - if v.IsMarked() { + if v.Type().Equals(cty.String) && v.HasMark("raw") { + raw, _ := v.Unmark() + return raw.AsString() + } + if v.HasMark("sensitive") { return "(sensitive)" } if v.IsNull() { diff --git a/website/docs/configuration/functions/type.html.md b/website/docs/configuration/functions/type.html.md new file mode 100644 index 0000000000..ed4b0c3569 --- /dev/null +++ b/website/docs/configuration/functions/type.html.md @@ -0,0 +1,82 @@ +--- +layout: "language" +page_title: "type - Functions - Configuration Language" +sidebar_current: "docs-funcs-conversion-type" +description: |- + The type function returns the type of a given value. +--- + +# `type` Function + +-> **Note:** This function is available only in Terraform 1.0 and later. + +`type` retuns the type of a given value. + +Sometimes a Terraform configuration can result in confusing errors regarding +inconsistent types. This function displays terraform's evaluation of a given +value's type, which is useful in understanding this error message. + +This is a special function which is only available in the `terraform console` command. + +## Examples + +Here we have a conditional `output` which prints either the value of `var.list` or a local named `default_list`: + +```hcl +variable "list" { + default = [] +} + +locals { + default_list = [ + { + foo = "bar" + map = { bleep = "bloop" } + }, + { + beep = "boop" + }, + ] +} + +output "list" { + value = var.list != [] ? var.list : local.default_list +} +``` + +Applying this configuration results in the following error: + +``` +Error: Inconsistent conditional result types + + on main.tf line 18, in output "list": + 18: value = var.list != [] ? var.list : local.default_list + |---------------- + | local.default_list is tuple with 2 elements + | var.list is empty tuple + +The true and false result expressions must have consistent types. The given +expressions are tuple and tuple, respectively. +``` + +While this error message does include some type information, it can be helpful +to inspect the exact type that Terraform has determined for each given input. +Examining both `var.list` and `local.default_list` using the `type` function +provides more context for the error message: + +``` +> type(var.list) +tuple +> type(local.default_list) +tuple([ + object({ + foo: string, + map: object({ + bleep: string, + }), + }), + object({ + beep: string, + }), +]) +``` From 80f01e2cb73749509a688b12b0f1baeb28390995 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Fri, 23 Apr 2021 10:39:24 -0400 Subject: [PATCH 36/38] Update CHANGELOG.md --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65cc102d47..f21d50a90f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## Next Major Release -(nothing yet) +NEW FEATURES: + +* lang/funcs: add a new `type()` function, only available in `terraform console` [GH-28501] ## Previous Releases From e1d790be61166e7939e8c9748ccfbc0a86e16cd6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 22 Apr 2021 09:45:29 -0400 Subject: [PATCH 37/38] update HC PublicKey --- internal/getproviders/public_keys.go | 146 ++++++++++++++++++++++----- 1 file changed, 119 insertions(+), 27 deletions(-) diff --git a/internal/getproviders/public_keys.go b/internal/getproviders/public_keys.go index bbbcdc804f..7426564545 100644 --- a/internal/getproviders/public_keys.go +++ b/internal/getproviders/public_keys.go @@ -3,34 +3,126 @@ package getproviders // HashicorpPublicKey is the HashiCorp public key, also available at // https://www.hashicorp.com/security const HashicorpPublicKey = `-----BEGIN PGP PUBLIC KEY BLOCK----- -Version: GnuPG v1 -mQENBFMORM0BCADBRyKO1MhCirazOSVwcfTr1xUxjPvfxD3hjUwHtjsOy/bT6p9f -W2mRPfwnq2JB5As+paL3UGDsSRDnK9KAxQb0NNF4+eVhr/EJ18s3wwXXDMjpIifq -fIm2WyH3G+aRLTLPIpscUNKDyxFOUbsmgXAmJ46Re1fn8uKxKRHbfa39aeuEYWFA -3drdL1WoUngvED7f+RnKBK2G6ZEpO+LDovQk19xGjiMTtPJrjMjZJ3QXqPvx5wca -KSZLr4lMTuoTI/ZXyZy5bD4tShiZz6KcyX27cD70q2iRcEZ0poLKHyEIDAi3TM5k -SwbbWBFd5RNPOR0qzrb/0p9ksKK48IIfH2FvABEBAAG0K0hhc2hpQ29ycCBTZWN1 -cml0eSA8c2VjdXJpdHlAaGFzaGljb3JwLmNvbT6JATgEEwECACIFAlMORM0CGwMG -CwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJEFGFLYc0j/xMyWIIAIPhcVqiQ59n -Jc07gjUX0SWBJAxEG1lKxfzS4Xp+57h2xxTpdotGQ1fZwsihaIqow337YHQI3q0i -SqV534Ms+j/tU7X8sq11xFJIeEVG8PASRCwmryUwghFKPlHETQ8jJ+Y8+1asRydi -psP3B/5Mjhqv/uOK+Vy3zAyIpyDOMtIpOVfjSpCplVRdtSTFWBu9Em7j5I2HMn1w -sJZnJgXKpybpibGiiTtmnFLOwibmprSu04rsnP4ncdC2XRD4wIjoyA+4PKgX3sCO -klEzKryWYBmLkJOMDdo52LttP3279s7XrkLEE7ia0fXa2c12EQ0f0DQ1tGUvyVEW -WmJVccm5bq25AQ0EUw5EzQEIANaPUY04/g7AmYkOMjaCZ6iTp9hB5Rsj/4ee/ln9 -wArzRO9+3eejLWh53FoN1rO+su7tiXJA5YAzVy6tuolrqjM8DBztPxdLBbEi4V+j -2tK0dATdBQBHEh3OJApO2UBtcjaZBT31zrG9K55D+CrcgIVEHAKY8Cb4kLBkb5wM -skn+DrASKU0BNIV1qRsxfiUdQHZfSqtp004nrql1lbFMLFEuiY8FZrkkQ9qduixo -mTT6f34/oiY+Jam3zCK7RDN/OjuWheIPGj/Qbx9JuNiwgX6yRj7OE1tjUx6d8g9y -0H1fmLJbb3WZZbuuGFnK6qrE3bGeY8+AWaJAZ37wpWh1p0cAEQEAAYkBHwQYAQIA -CQUCUw5EzQIbDAAKCRBRhS2HNI/8TJntCAClU7TOO/X053eKF1jqNW4A1qpxctVc -z8eTcY8Om5O4f6a/rfxfNFKn9Qyja/OG1xWNobETy7MiMXYjaa8uUx5iFy6kMVaP -0BXJ59NLZjMARGw6lVTYDTIvzqqqwLxgliSDfSnqUhubGwvykANPO+93BBx89MRG -unNoYGXtPlhNFrAsB1VR8+EyKLv2HQtGCPSFBhrjuzH3gxGibNDDdFQLxxuJWepJ -EK1UbTS4ms0NgZ2Uknqn1WRU1Ki7rE4sTy68iZtWpKQXZEJa0IGnuI2sSINGcXCJ -oEIgXTMyCILo34Fa/C6VCm2WBgz9zZO8/rHIiQm1J5zqz0DrDwKBUM9C -=LYpS +mQINBGB9+xkBEACabYZOWKmgZsHTdRDiyPJxhbuUiKX65GUWkyRMJKi/1dviVxOX +PG6hBPtF48IFnVgxKpIb7G6NjBousAV+CuLlv5yqFKpOZEGC6sBV+Gx8Vu1CICpl +Zm+HpQPcIzwBpN+Ar4l/exCG/f/MZq/oxGgH+TyRF3XcYDjG8dbJCpHO5nQ5Cy9h +QIp3/Bh09kET6lk+4QlofNgHKVT2epV8iK1cXlbQe2tZtfCUtxk+pxvU0UHXp+AB +0xc3/gIhjZp/dePmCOyQyGPJbp5bpO4UeAJ6frqhexmNlaw9Z897ltZmRLGq1p4a +RnWL8FPkBz9SCSKXS8uNyV5oMNVn4G1obCkc106iWuKBTibffYQzq5TG8FYVJKrh +RwWB6piacEB8hl20IIWSxIM3J9tT7CPSnk5RYYCTRHgA5OOrqZhC7JefudrP8n+M +pxkDgNORDu7GCfAuisrf7dXYjLsxG4tu22DBJJC0c/IpRpXDnOuJN1Q5e/3VUKKW +mypNumuQpP5lc1ZFG64TRzb1HR6oIdHfbrVQfdiQXpvdcFx+Fl57WuUraXRV6qfb +4ZmKHX1JEwM/7tu21QE4F1dz0jroLSricZxfaCTHHWNfvGJoZ30/MZUrpSC0IfB3 +iQutxbZrwIlTBt+fGLtm3vDtwMFNWM+Rb1lrOxEQd2eijdxhvBOHtlIcswARAQAB +tERIYXNoaUNvcnAgU2VjdXJpdHkgKGhhc2hpY29ycC5jb20vc2VjdXJpdHkpIDxz +ZWN1cml0eUBoYXNoaWNvcnAuY29tPokCVAQTAQoAPhYhBMh0AR8KtAURDQIQVTQ2 +XZRy10aPBQJgffsZAhsDBQkJZgGABQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJ +EDQ2XZRy10aPtpcP/0PhJKiHtC1zREpRTrjGizoyk4Sl2SXpBZYhkdrG++abo6zs +buaAG7kgWWChVXBo5E20L7dbstFK7OjVs7vAg/OLgO9dPD8n2M19rpqSbbvKYWvp +0NSgvFTT7lbyDhtPj0/bzpkZEhmvQaDWGBsbDdb2dBHGitCXhGMpdP0BuuPWEix+ +QnUMaPwU51q9GM2guL45Tgks9EKNnpDR6ZdCeWcqo1IDmklloidxT8aKL21UOb8t +cD+Bg8iPaAr73bW7Jh8TdcV6s6DBFub+xPJEB/0bVPmq3ZHs5B4NItroZ3r+h3ke +VDoSOSIZLl6JtVooOJ2la9ZuMqxchO3mrXLlXxVCo6cGcSuOmOdQSz4OhQE5zBxx +LuzA5ASIjASSeNZaRnffLIHmht17BPslgNPtm6ufyOk02P5XXwa69UCjA3RYrA2P +QNNC+OWZ8qQLnzGldqE4MnRNAxRxV6cFNzv14ooKf7+k686LdZrP/3fQu2p3k5rY +0xQUXKh1uwMUMtGR867ZBYaxYvwqDrg9XB7xi3N6aNyNQ+r7zI2lt65lzwG1v9hg +FG2AHrDlBkQi/t3wiTS3JOo/GCT8BjN0nJh0lGaRFtQv2cXOQGVRW8+V/9IpqEJ1 +qQreftdBFWxvH7VJq2mSOXUJyRsoUrjkUuIivaA9Ocdipk2CkP8bpuGz7ZF4uQIN +BGB9+xkBEACoklYsfvWRCjOwS8TOKBTfl8myuP9V9uBNbyHufzNETbhYeT33Cj0M +GCNd9GdoaknzBQLbQVSQogA+spqVvQPz1MND18GIdtmr0BXENiZE7SRvu76jNqLp +KxYALoK2Pc3yK0JGD30HcIIgx+lOofrVPA2dfVPTj1wXvm0rbSGA4Wd4Ng3d2AoR +G/wZDAQ7sdZi1A9hhfugTFZwfqR3XAYCk+PUeoFrkJ0O7wngaon+6x2GJVedVPOs +2x/XOR4l9ytFP3o+5ILhVnsK+ESVD9AQz2fhDEU6RhvzaqtHe+sQccR3oVLoGcat +ma5rbfzH0Fhj0JtkbP7WreQf9udYgXxVJKXLQFQgel34egEGG+NlbGSPG+qHOZtY +4uWdlDSvmo+1P95P4VG/EBteqyBbDDGDGiMs6lAMg2cULrwOsbxWjsWka8y2IN3z +1stlIJFvW2kggU+bKnQ+sNQnclq3wzCJjeDBfucR3a5WRojDtGoJP6Fc3luUtS7V +5TAdOx4dhaMFU9+01OoH8ZdTRiHZ1K7RFeAIslSyd4iA/xkhOhHq89F4ECQf3Bt4 +ZhGsXDTaA/VgHmf3AULbrC94O7HNqOvTWzwGiWHLfcxXQsr+ijIEQvh6rHKmJK8R +9NMHqc3L18eMO6bqrzEHW0Xoiu9W8Yj+WuB3IKdhclT3w0pO4Pj8gQARAQABiQI8 +BBgBCgAmFiEEyHQBHwq0BRENAhBVNDZdlHLXRo8FAmB9+xkCGwwFCQlmAYAACgkQ +NDZdlHLXRo9ZnA/7BmdpQLeTjEiXEJyW46efxlV1f6THn9U50GWcE9tebxCXgmQf +u+Uju4hreltx6GDi/zbVVV3HCa0yaJ4JVvA4LBULJVe3ym6tXXSYaOfMdkiK6P1v +JgfpBQ/b/mWB0yuWTUtWx18BQQwlNEQWcGe8n1lBbYsH9g7QkacRNb8tKUrUbWlQ +QsU8wuFgly22m+Va1nO2N5C/eE/ZEHyN15jEQ+QwgQgPrK2wThcOMyNMQX/VNEr1 +Y3bI2wHfZFjotmek3d7ZfP2VjyDudnmCPQ5xjezWpKbN1kvjO3as2yhcVKfnvQI5 +P5Frj19NgMIGAp7X6pF5Csr4FX/Vw316+AFJd9Ibhfud79HAylvFydpcYbvZpScl +7zgtgaXMCVtthe3GsG4gO7IdxxEBZ/Fm4NLnmbzCIWOsPMx/FxH06a539xFq/1E2 +1nYFjiKg8a5JFmYU/4mV9MQs4bP/3ip9byi10V+fEIfp5cEEmfNeVeW5E7J8PqG9 +t4rLJ8FR4yJgQUa2gs2SNYsjWQuwS/MJvAv4fDKlkQjQmYRAOp1SszAnyaplvri4 +ncmfDsf0r65/sd6S40g5lHH8LIbGxcOIN6kwthSTPWX89r42CbY8GzjTkaeejNKx +v1aCrO58wAtursO1DiXCvBY7+NdafMRnoHwBk50iPqrVkNA8fv+auRyB2/G5Ag0E +YH3+JQEQALivllTjMolxUW2OxrXb+a2Pt6vjCBsiJzrUj0Pa63U+lT9jldbCCfgP +wDpcDuO1O05Q8k1MoYZ6HddjWnqKG7S3eqkV5c3ct3amAXp513QDKZUfIDylOmhU +qvxjEgvGjdRjz6kECFGYr6Vnj/p6AwWv4/FBRFlrq7cnQgPynbIH4hrWvewp3Tqw +GVgqm5RRofuAugi8iZQVlAiQZJo88yaztAQ/7VsXBiHTn61ugQ8bKdAsr8w/ZZU5 +HScHLqRolcYg0cKN91c0EbJq9k1LUC//CakPB9mhi5+aUVUGusIM8ECShUEgSTCi +KQiJUPZ2CFbbPE9L5o9xoPCxjXoX+r7L/WyoCPTeoS3YRUMEnWKvc42Yxz3meRb+ +BmaqgbheNmzOah5nMwPupJYmHrjWPkX7oyyHxLSFw4dtoP2j6Z7GdRXKa2dUYdk2 +x3JYKocrDoPHh3Q0TAZujtpdjFi1BS8pbxYFb3hHmGSdvz7T7KcqP7ChC7k2RAKO +GiG7QQe4NX3sSMgweYpl4OwvQOn73t5CVWYp/gIBNZGsU3Pto8g27vHeWyH9mKr4 +cSepDhw+/X8FGRNdxNfpLKm7Vc0Sm9Sof8TRFrBTqX+vIQupYHRi5QQCuYaV6OVr +ITeegNK3So4m39d6ajCR9QxRbmjnx9UcnSYYDmIB6fpBuwT0ogNtABEBAAGJBHIE +GAEKACYCGwIWIQTIdAEfCrQFEQ0CEFU0Nl2UctdGjwUCYH4bgAUJAeFQ2wJAwXQg +BBkBCgAdFiEEs2y6kaLAcwxDX8KAsLRBCXaFtnYFAmB9/iUACgkQsLRBCXaFtnYX +BhAAlxejyFXoQwyGo9U+2g9N6LUb/tNtH29RHYxy4A3/ZUY7d/FMkArmh4+dfjf0 +p9MJz98Zkps20kaYP+2YzYmaizO6OA6RIddcEXQDRCPHmLts3097mJ/skx9qLAf6 +rh9J7jWeSqWO6VW6Mlx8j9m7sm3Ae1OsjOx/m7lGZOhY4UYfY627+Jf7WQ5103Qs +lgQ09es/vhTCx0g34SYEmMW15Tc3eCjQ21b1MeJD/V26npeakV8iCZ1kHZHawPq/ +aCCuYEcCeQOOteTWvl7HXaHMhHIx7jjOd8XX9V+UxsGz2WCIxX/j7EEEc7CAxwAN +nWp9jXeLfxYfjrUB7XQZsGCd4EHHzUyCf7iRJL7OJ3tz5Z+rOlNjSgci+ycHEccL +YeFAEV+Fz+sj7q4cFAferkr7imY1XEI0Ji5P8p/uRYw/n8uUf7LrLw5TzHmZsTSC +UaiL4llRzkDC6cVhYfqQWUXDd/r385OkE4oalNNE+n+txNRx92rpvXWZ5qFYfv7E +95fltvpXc0iOugPMzyof3lwo3Xi4WZKc1CC/jEviKTQhfn3WZukuF5lbz3V1PQfI +xFsYe9WYQmp25XGgezjXzp89C/OIcYsVB1KJAKihgbYdHyUN4fRCmOszmOUwEAKR +3k5j4X8V5bk08sA69NVXPn2ofxyk3YYOMYWW8ouObnXoS8QJEDQ2XZRy10aPMpsQ +AIbwX21erVqUDMPn1uONP6o4NBEq4MwG7d+fT85rc1U0RfeKBwjucAE/iStZDQoM +ZKWvGhFR+uoyg1LrXNKuSPB82unh2bpvj4zEnJsJadiwtShTKDsikhrfFEK3aCK8 +Zuhpiu3jxMFDhpFzlxsSwaCcGJqcdwGhWUx0ZAVD2X71UCFoOXPjF9fNnpy80YNp +flPjj2RnOZbJyBIM0sWIVMd8F44qkTASf8K5Qb47WFN5tSpePq7OCm7s8u+lYZGK +wR18K7VliundR+5a8XAOyUXOL5UsDaQCK4Lj4lRaeFXunXl3DJ4E+7BKzZhReJL6 +EugV5eaGonA52TWtFdB8p+79wPUeI3KcdPmQ9Ll5Zi/jBemY4bzasmgKzNeMtwWP +fk6WgrvBwptqohw71HDymGxFUnUP7XYYjic2sVKhv9AevMGycVgwWBiWroDCQ9Ja +btKfxHhI2p+g+rcywmBobWJbZsujTNjhtme+kNn1mhJsD3bKPjKQfAxaTskBLb0V +wgV21891TS1Dq9kdPLwoS4XNpYg2LLB4p9hmeG3fu9+OmqwY5oKXsHiWc43dei9Y +yxZ1AAUOIaIdPkq+YG/PhlGE4YcQZ4RPpltAr0HfGgZhmXWigbGS+66pUj+Ojysc +j0K5tCVxVu0fhhFpOlHv0LWaxCbnkgkQH9jfMEJkAWMOuQINBGCAXCYBEADW6RNr +ZVGNXvHVBqSiOWaxl1XOiEoiHPt50Aijt25yXbG+0kHIFSoR+1g6Lh20JTCChgfQ +kGGjzQvEuG1HTw07YhsvLc0pkjNMfu6gJqFox/ogc53mz69OxXauzUQ/TZ27GDVp +UBu+EhDKt1s3OtA6Bjz/csop/Um7gT0+ivHyvJ/jGdnPEZv8tNuSE/Uo+hn/Q9hg +8SbveZzo3C+U4KcabCESEFl8Gq6aRi9vAfa65oxD5jKaIz7cy+pwb0lizqlW7H9t +Qlr3dBfdIcdzgR55hTFC5/XrcwJ6/nHVH/xGskEasnfCQX8RYKMuy0UADJy72TkZ +bYaCx+XXIcVB8GTOmJVoAhrTSSVLAZspfCnjwnSxisDn3ZzsYrq3cV6sU8b+QlIX +7VAjurE+5cZiVlaxgCjyhKqlGgmonnReWOBacCgL/UvuwMmMp5TTLmiLXLT7uxeG +ojEyoCk4sMrqrU1jevHyGlDJH9Taux15GILDwnYFfAvPF9WCid4UZ4Ouwjcaxfys +3LxNiZIlUsXNKwS3mhiMRL4TRsbs4k4QE+LIMOsauIvcvm8/frydvQ/kUwIhVTH8 +0XGOH909bYtJvY3fudK7ShIwm7ZFTduBJUG473E/Fn3VkhTmBX6+PjOC50HR/Hyb +waRCzfDruMe3TAcE/tSP5CUOb9C7+P+hPzQcDwARAQABiQRyBBgBCgAmFiEEyHQB +Hwq0BRENAhBVNDZdlHLXRo8FAmCAXCYCGwIFCQlmAYACQAkQNDZdlHLXRo/BdCAE +GQEKAB0WIQQ3TsdbSFkTYEqDHMfIIMbVzSerhwUCYIBcJgAKCRDIIMbVzSerh0Xw +D/9ghnUsoNCu1OulcoJdHboMazJvDt/znttdQSnULBVElgM5zk0Uyv87zFBzuCyQ +JWL3bWesQ2uFx5fRWEPDEfWVdDrjpQGb1OCCQyz1QlNPV/1M1/xhKGS9EeXrL8Dw +F6KTGkRwn1yXiP4BGgfeFIQHmJcKXEZ9HkrpNb8mcexkROv4aIPAwn+IaE+NHVtt +IBnufMXLyfpkWJQtJa9elh9PMLlHHnuvnYLvuAoOkhuvs7fXDMpfFZ01C+QSv1dz +Hm52GSStERQzZ51w4c0rYDneYDniC/sQT1x3dP5Xf6wzO+EhRMabkvoTbMqPsTEP +xyWr2pNtTBYp7pfQjsHxhJpQF0xjGN9C39z7f3gJG8IJhnPeulUqEZjhRFyVZQ6/ +siUeq7vu4+dM/JQL+i7KKe7Lp9UMrG6NLMH+ltaoD3+lVm8fdTUxS5MNPoA/I8cK +1OWTJHkrp7V/XaY7mUtvQn5V1yET5b4bogz4nME6WLiFMd+7x73gB+YJ6MGYNuO8 +e/NFK67MfHbk1/AiPTAJ6s5uHRQIkZcBPG7y5PpfcHpIlwPYCDGYlTajZXblyKrw +BttVnYKvKsnlysv11glSg0DphGxQJbXzWpvBNyhMNH5dffcfvd3eXJAxnD81GD2z +ZAriMJ4Av2TfeqQ2nxd2ddn0jX4WVHtAvLXfCgLM2Gveho4jD/9sZ6PZz/rEeTvt +h88t50qPcBa4bb25X0B5FO3TeK2LL3VKLuEp5lgdcHVonrcdqZFobN1CgGJua8TW +SprIkh+8ATZ/FXQTi01NzLhHXT1IQzSpFaZw0gb2f5ruXwvTPpfXzQrs2omY+7s7 +fkCwGPesvpSXPKn9v8uhUwD7NGW/Dm+jUM+QtC/FqzX7+/Q+OuEPjClUh1cqopCZ +EvAI3HjnavGrYuU6DgQdjyGT/UDbuwbCXqHxHojVVkISGzCTGpmBcQYQqhcFRedJ +yJlu6PSXlA7+8Ajh52oiMJ3ez4xSssFgUQAyOB16432tm4erpGmCyakkoRmMUn3p +wx+QIppxRlsHznhcCQKR3tcblUqH3vq5i4/ZAihusMCa0YrShtxfdSb13oKX+pFr +aZXvxyZlCa5qoQQBV1sowmPL1N2j3dR9TVpdTyCFQSv4KeiExmowtLIjeCppRBEK +eeYHJnlfkyKXPhxTVVO6H+dU4nVu0ASQZ07KiQjbI+zTpPKFLPp3/0sPRJM57r1+ +aTS71iR7nZNZ1f8LZV2OvGE6fJVtgJ1J4Nu02K54uuIhU3tg1+7Xt+IqwRc9rbVr +pHH/hFCYBPW2D2dxB+k2pQlg5NI+TpsXj5Zun8kRw5RtVb+dLuiH/xmxArIee8Jq +ZF5q4h4I33PSGDdSvGXn9UMY5Isjpg== +=7pIB -----END PGP PUBLIC KEY BLOCK-----` // HashicorpPartnersKey is a key created by HashiCorp, used to generate and From a4a6b40beedc0c175e559f8da9b11ccd45f8c6ca Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Apr 2021 11:03:20 -0400 Subject: [PATCH 38/38] update getproviders tests for new key --- .../package_authentication_test.go | 92 ++++++++++++++----- 1 file changed, 70 insertions(+), 22 deletions(-) diff --git a/internal/getproviders/package_authentication_test.go b/internal/getproviders/package_authentication_test.go index 62d7bbddb3..06b7621e5b 100644 --- a/internal/getproviders/package_authentication_test.go +++ b/internal/getproviders/package_authentication_test.go @@ -325,18 +325,6 @@ func TestSignatureAuthentication_success(t *testing.T) { keys []SigningKey result PackageAuthenticationResult }{ - "official provider": { - testHashicorpSignatureGoodBase64, - []SigningKey{ - { - ASCIIArmor: HashicorpPublicKey, - }, - }, - PackageAuthenticationResult{ - result: officialProvider, - KeyID: testHashiCorpPublicKeyID, - }, - }, "partner provider": { testAuthorSignatureGoodBase64, []SigningKey{ @@ -402,6 +390,49 @@ func TestSignatureAuthentication_success(t *testing.T) { } } +func TestNewSignatureAuthentication_success(t *testing.T) { + tests := map[string]struct { + signature string + keys []SigningKey + result PackageAuthenticationResult + }{ + "official provider": { + testHashicorpSignatureGoodBase64, + []SigningKey{ + { + ASCIIArmor: HashicorpPublicKey, + }, + }, + PackageAuthenticationResult{ + result: officialProvider, + KeyID: testHashiCorpPublicKeyID, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + // Location is unused + location := PackageLocalArchive("testdata/my-package.zip") + + signature, err := base64.StdEncoding.DecodeString(test.signature) + if err != nil { + t.Fatal(err) + } + + auth := NewSignatureAuthentication([]byte(testProviderShaSums), signature, test.keys) + result, err := auth.AuthenticatePackage(location) + + if result == nil || *result != test.result { + t.Errorf("wrong result: got %#v, want %#v", result, test.result) + } + if err != nil { + t.Errorf("wrong err: got %s, want nil", err) + } + }) + } +} + // Signature authentication can fail for many reasons, most of which are due // to OpenPGP failures from malformed keys or signatures. func TestSignatureAuthentication_failure(t *testing.T) { @@ -621,18 +652,35 @@ const testSignatureBadBase64 = `iQEzBAABCAAdFiEEW/7sQxfnRgCGIZcGN6arO88s` + `n1ayZdaCIw/r4w==` // testHashiCorpPublicKeyID is the Key ID of the HashiCorpPublicKey. -const testHashiCorpPublicKeyID = `51852D87348FFC4C` +const testHashiCorpPublicKeyID = `34365D9472D7468F` -// testHashicorpSignatureGoodBase64 is a signature of testShaSums signed with +const testProviderShaSums = `fea4227271ebf7d9e2b61b89ce2328c7262acd9fd190e1fd6d15a591abfa848e terraform-provider-null_3.1.0_darwin_amd64.zip +9ebf4d9704faba06b3ec7242c773c0fbfe12d62db7d00356d4f55385fc69bfb2 terraform-provider-null_3.1.0_darwin_arm64.zip +a6576c81adc70326e4e1c999c04ad9ca37113a6e925aefab4765e5a5198efa7e terraform-provider-null_3.1.0_freebsd_386.zip +5f9200bf708913621d0f6514179d89700e9aa3097c77dac730e8ba6e5901d521 terraform-provider-null_3.1.0_freebsd_amd64.zip +fc39cc1fe71234a0b0369d5c5c7f876c71b956d23d7d6f518289737a001ba69b terraform-provider-null_3.1.0_freebsd_arm.zip +c797744d08a5307d50210e0454f91ca4d1c7621c68740441cf4579390452321d terraform-provider-null_3.1.0_linux_386.zip +53e30545ff8926a8e30ad30648991ca8b93b6fa496272cd23b26763c8ee84515 terraform-provider-null_3.1.0_linux_amd64.zip +cecb6a304046df34c11229f20a80b24b1603960b794d68361a67c5efe58e62b8 terraform-provider-null_3.1.0_linux_arm64.zip +e1371aa1e502000d9974cfaff5be4cfa02f47b17400005a16f14d2ef30dc2a70 terraform-provider-null_3.1.0_linux_arm.zip +a8a42d13346347aff6c63a37cda9b2c6aa5cc384a55b2fe6d6adfa390e609c53 terraform-provider-null_3.1.0_windows_386.zip +02a1675fd8de126a00460942aaae242e65ca3380b5bb192e8773ef3da9073fd2 terraform-provider-null_3.1.0_windows_amd64.zip +` + +// testHashicorpSignatureGoodBase64 is a signature of testProviderShaSums signed with // HashicorpPublicKey, which represents the SHA256SUMS.sig file downloaded for // an official release. -const testHashicorpSignatureGoodBase64 = `iQFLBAABCAA1FiEEkabn+F0FxlYwvvGJUYUth` + - `zSP/EwFAl5w784XHHNlY3VyaXR5QGhhc2hpY29ycC5jb20ACgkQUYUthzSP/EyB8QgAv9ijp` + - `kTcoFwDAs+1iEUrcW18h/2cU+bvFtdqNDiffzk7+YJ9ioxeWisPta/Z6hEyhdss2+5L1MNbo` + - `oUBLABI+Aebfxa/uYFT2kX6r/eySmlY9kqNVpjXdemOQutS4NNZxdJL7CEbh2qIKCVuyo0ul` + - `YrTdDH35vwVyLXImWiZLnrXcT/fXLpQGx/N8PDy6WmCeju5Y5RD7TuntB71eCaCZi7wFe1tR` + - `qSoe9tD9A7ONB0rGuCY7BxqUj0S81hhz960YbNR9Q81WoNvF7b5SmcLJ1qJx1yvBLyqya6Su` + - `DKjU/YYCh7bwHIYzpk1/nK/7SaTHpisekqojVsfDth4TA+jGA==` +const testHashicorpSignatureGoodBase64 = `wsFcBAABCAAQBQJgga+GCRCwtEEJdoW2dgAA` + + `o0YQAAW911BGDr2WHLo5NwcZenwHyxL5DX9g+4BknKbc/WxRC1hD8Afi3eygZk1yR6eT4Gp2H` + + `yNOwCjGL1PTONBumMfj9udIeuX8onrJMMvjFHh+bORGxBi4FKr4V3b2ZV1IYOjWMEyyTGRDvw` + + `SCdxBkp3apH3s2xZLmRoAj84JZ4KaxGF7hlT0j4IkNyQKd2T5cCByN9DV80+x+HtzaOieFwJL` + + `97iyGj6aznXfKfslK6S4oIrVTwyLTrQbxSxA0LsdUjRPHnJamL3sFOG77qUEUoXG3r61yi5vW` + + `V4P5gCH/+C+VkfGHqaB1s0jHYLxoTEXtwthe66MydDBPe2Hd0J12u9ppOIeK3leeb4uiixWIi` + + `rNdpWyjr/LU1KKWPxsDqMGYJ9TexyWkXjEpYmIEiY1Rxar8jrLh+FqVAhxRJajjgSRu5pZj50` + + `CNeKmmbyolLhPCmICjYYU/xKPGXSyDFqonVVyMWCSpO+8F38OmwDQHIk5AWyc8hPOAZ+g5N95` + + `cfUAzEqlvmNvVHQIU40Y6/Ip2HZzzFCLKQkMP1aDakYHq5w4ZO/ucjhKuoh1HDQMuMnZSu4eo` + + `2nMTBzYZnUxwtROrJZF1t103avbmP2QE/GaPvLIQn7o5WMV3ZcPCJ+szzzby7H2e33WIynrY/` + + `95ensBxh7mGFbcQ1C59b5o7viwIaaY2` // entityString function is used for logging the signing key. func TestEntityString(t *testing.T) { @@ -654,7 +702,7 @@ func TestEntityString(t *testing.T) { { "HashicorpPublicKey", testReadArmoredEntity(t, HashicorpPublicKey), - "51852D87348FFC4C HashiCorp Security ", + "34365D9472D7468F HashiCorp Security (hashicorp.com/security) ", }, { "HashicorpPartnersKey",