From fa0d6484dfc811ab4a7edd6fe2353161d7d76fc4 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 20 Feb 2019 16:28:15 -0800 Subject: [PATCH] configs/configupgrade: Detect and fix element(...) usage with sets Although sets do not have indexed elements, in Terraform 0.11 and earlier element(...) would work with sets because we'd automatically convert them to lists on entry to HIL -- with an arbitrary-but-consistent ordering -- and this return an arbitrary-but-consistent element from the list. The element(...) function in Terraform 0.12 does not allow this because it is not safe in general, but there was an existing pattern relying on this in Terraform 0.11 configs which this upgrade rule is intended to preserve: resource "example" "example" { count = "${length(any_set_attribute)}" foo = "${element(any_set_attribute, count.index}" } The above works because the exact indices assigned in the conversion are irrelevant: we're just asking Terraform to create one resource for each distinct element in the set. This upgrade rule therefore inserts an explicit conversion to list if it is able to successfully provide that the given expression will return a set type: foo = "${element(tolist(any_set_attribute), count.index}" This makes the conversion explicit, allowing users to decide if it is safe and rework the configuration if not. Since our static type analysis functionality focuses mainly on resource type attributes, in practice this rule will only apply when the given expression is a statically-checkable resource reference. Since sets are an SDK-only concept in Terraform 0.11 and earlier anyway, in practice that works out just right: it's not possible for sets to appear anywhere else in older versions anyway. --- .../element-of-set/input/element-of-set.tf | 7 +++++++ .../valid/element-of-set/want/element-of-set.tf | 7 +++++++ .../valid/element-of-set/want/versions.tf | 3 +++ configs/configupgrade/upgrade_expr.go | 17 +++++++++++++++++ configs/configupgrade/upgrade_test.go | 1 + 5 files changed, 35 insertions(+) create mode 100644 configs/configupgrade/test-fixtures/valid/element-of-set/input/element-of-set.tf create mode 100644 configs/configupgrade/test-fixtures/valid/element-of-set/want/element-of-set.tf create mode 100644 configs/configupgrade/test-fixtures/valid/element-of-set/want/versions.tf diff --git a/configs/configupgrade/test-fixtures/valid/element-of-set/input/element-of-set.tf b/configs/configupgrade/test-fixtures/valid/element-of-set/input/element-of-set.tf new file mode 100644 index 000000000..7d64a6956 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/element-of-set/input/element-of-set.tf @@ -0,0 +1,7 @@ +resource "test_instance" "a" { + subnet_ids = ["boop"] # this attribute takes a set of strings +} + +output "b" { + value = "${element(test_instance.a.subnet_ids, 0)}" +} diff --git a/configs/configupgrade/test-fixtures/valid/element-of-set/want/element-of-set.tf b/configs/configupgrade/test-fixtures/valid/element-of-set/want/element-of-set.tf new file mode 100644 index 000000000..fcc6c4edf --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/element-of-set/want/element-of-set.tf @@ -0,0 +1,7 @@ +resource "test_instance" "a" { + subnet_ids = ["boop"] # this attribute takes a set of strings +} + +output "b" { + value = element(tolist(test_instance.a.subnet_ids), 0) +} diff --git a/configs/configupgrade/test-fixtures/valid/element-of-set/want/versions.tf b/configs/configupgrade/test-fixtures/valid/element-of-set/want/versions.tf new file mode 100644 index 000000000..d9b6f790b --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/element-of-set/want/versions.tf @@ -0,0 +1,3 @@ +terraform { + required_version = ">= 0.12" +} diff --git a/configs/configupgrade/upgrade_expr.go b/configs/configupgrade/upgrade_expr.go index b1e40450c..de4480af4 100644 --- a/configs/configupgrade/upgrade_expr.go +++ b/configs/configupgrade/upgrade_expr.go @@ -398,6 +398,23 @@ Value: buf.WriteByte(']') break Value } + case "element": + // We cannot replace element with index syntax safely in general + // because users may be relying on its special modulo wraparound + // behavior that the index syntax doesn't do. However, if it seems + // like the user is trying to use element with a set, we'll insert + // an explicit conversion to list to mimic the implicit conversion + // that we used to do as an unintended side-effect of how functions + // work in HIL. + if len(argExprs) > 0 { + argTy := an.InferExpressionType(argExprs[0], nil) + if argTy.IsSetType() { + newExpr := []byte(`tolist(`) + newExpr = append(newExpr, argExprs[0]...) + newExpr = append(newExpr, ')') + argExprs[0] = newExpr + } + } // HIL used some undocumented special functions to implement certain // operations, but since those were actually callable in real expressions diff --git a/configs/configupgrade/upgrade_test.go b/configs/configupgrade/upgrade_test.go index 92ca69f66..a0c90b21f 100644 --- a/configs/configupgrade/upgrade_test.go +++ b/configs/configupgrade/upgrade_test.go @@ -195,6 +195,7 @@ var testProviders = map[string]providers.Factory{ "image": {Type: cty.String, Optional: true}, "tags": {Type: cty.Map(cty.String), Optional: true}, "security_groups": {Type: cty.List(cty.String), Optional: true}, + "subnet_ids": {Type: cty.Set(cty.String), Optional: true}, }, BlockTypes: map[string]*configschema.NestedBlock{ "network": {