From cad3268089b7fbbb07ba8f175d1d3858fd7b8683 Mon Sep 17 00:00:00 2001 From: Nuru Date: Wed, 11 May 2022 11:11:39 -0700 Subject: [PATCH] Handle `self = false`, add warning about `compact` and `sort` (#33) --- README.md | 55 ++++++++++++++++++++++++++++++++++----- README.yaml | 47 +++++++++++++++++++++++++++++++-- docs/terraform.md | 4 +-- examples/complete/main.tf | 8 +++--- normalize.tf | 18 ++++++++----- variables.tf | 6 +++-- 6 files changed, 116 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 872234e..1a31dea 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,15 @@ object do not all have to be the same type. The "type" of an object is itself an object: the keys are the same, and the values are the types of the values in the object. So although `{ foo = "bar", baz = {} }` and `{ foo = "bar", baz = [] }` are both objects, -they are not of the same type. This means you cannot put them both in the same list or the same map, +they are not of the same type, and you can get error messages like + +``` +Error: Inconsistent conditional result types +The true and false result expressions must have consistent types. The given +expressions are object and object, respectively. +``` + +This means you cannot put them both in the same list or the same map, even though you can put them in a single tuple or object. Similarly, and closer to the problem at hand, @@ -137,23 +145,29 @@ cidr_rule = { } ``` is not the same type as + ```hcl self_rule = { type = "ingress" self = true } ``` + This means you cannot put both of those in the same list. + ```hcl rules = tolist([local.cidr_rule, local.self_rule]) ``` + Generates the error + ```text Invalid value for "v" parameter: cannot convert tuple to list of any single type. ``` You could make them the same type and put them in a list, like this: + ```hcl rules = tolist([{ type = "ingress" @@ -166,9 +180,11 @@ rules = tolist([{ self = true }]) ``` + That remains an option for you when generating the rules, and is probably better when you have full control over all the rules. However, what if some of the rules are coming from a source outside of your control? You cannot simply add those rules to your list. So, what to do? Create an object whose attributes' values can be of different types. + ```hcl { mine = local.my_rules, theirs = var.their_rules } ``` @@ -266,7 +282,32 @@ You can avoid this for the most part by providing the optional keys. Rules with changed if their keys do not change and the rules themselves do not change, except in the case of `rule_matrix`, where the rules are still dependent on the order of the security groups in `source_security_group_ids`. You can avoid this by using `rules` instead of `rule_matrix` when you have -more than one security group in the list. +more than one security group in the list. You cannot avoid this by sorting the +`source_security_group_ids`, because that leads to the "Invalid `for_each` argument" error. + +##### Invalid for_each argument +You can supply a number of rules as inputs to this module, and they (usually) get transformed into +`aws_security_group_rule` resources. However, Terraform works in 2 steps: a `plan` step where it +calculates the changes to be made, and an `apply` step where it makes the changes. This is so you +can review and approve the plan before changing anything. One big limitation of this approach is +that it requires that Terraform be able to count the number of resources to create without the +benefit of any data generated during the `apply` phase. So if you try to generate a rule based +on something you are creating at the same time, you can get an error like + +``` +Error: Invalid for_each argument +The "for_each" value depends on resource attributes that cannot be determined until apply, +so Terraform cannot predict how many instances will be created. +``` + +This module uses lists to minimize the chance of that happening, as all it needs to know +is the length of the list, not the values in it, but this error still can +happen for subtle reasons. Most commonly, using a function like `compact` on a list +will cause the length to become unknown (since the values have to be checked and `null`s removed). +In the case of `source_security_group_ids`, just sorting the list using `sort` +will cause this error. If you run into this error, check for functions like `compact` somewhere +in the chain that produces the list and remove them if you find them. + ##### WARNINGS and Caveats @@ -474,8 +515,8 @@ Available targets: | [regex\_replace\_chars](#input\_regex\_replace\_chars) | Terraform regular expression (regex) string.
Characters matching the regex will be removed from the ID elements.
If not set, `"/[^a-zA-Z0-9-]/"` is used to remove all characters other than hyphens, letters and digits. | `string` | `null` | no | | [revoke\_rules\_on\_delete](#input\_revoke\_rules\_on\_delete) | Instruct Terraform to revoke all of the Security Group's attached ingress and egress rules before deleting
the security group itself. This is normally not needed. | `bool` | `false` | no | | [rule\_matrix](#input\_rule\_matrix) | A convenient way to apply the same set of rules to a set of subjects. See README for details. | `any` | `[]` | no | -| [rules](#input\_rules) | A list of Security Group rule objects. All elements of a list must be exactly the same type;
use `rules_map` if you want to supply multiple lists of different types.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule . | `list(any)` | `[]` | no | -| [rules\_map](#input\_rules\_map) | A map-like object of lists of Security Group rule objects. All elements of a list must be exactly the same type,
so this input accepts an object with keys (attributes) whose values are lists so you can separate different
types into different lists and still pass them into one input. Keys must be known at "plan" time.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule . | `any` | `{}` | no | +| [rules](#input\_rules) | A list of Security Group rule objects. All elements of a list must be exactly the same type;
use `rules_map` if you want to supply multiple lists of different types.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see the `security_group_rule` [documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule).
\_\_\_Note:\_\_\_ The length of the list must be known at plan time.
This means you cannot use functions like `compact` or `sort` when computing the list. | `list(any)` | `[]` | no | +| [rules\_map](#input\_rules\_map) | A map-like object of lists of Security Group rule objects. All elements of a list must be exactly the same type,
so this input accepts an object with keys (attributes) whose values are lists so you can separate different
types into different lists and still pass them into one input. Keys must be known at "plan" time.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see the `security_group_rule` [documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule). | `any` | `{}` | no | | [security\_group\_create\_timeout](#input\_security\_group\_create\_timeout) | How long to wait for the security group to be created. | `string` | `"10m"` | no | | [security\_group\_delete\_timeout](#input\_security\_group\_delete\_timeout) | How long to retry on `DependencyViolation` errors during security group deletion from
lingering ENIs left by certain AWS services such as Elastic Load Balancing. | `string` | `"15m"` | no | | [security\_group\_description](#input\_security\_group\_description) | The description to assign to the created Security Group.
Warning: Changing the description causes the security group to be replaced. | `string` | `"Managed by Terraform"` | no | @@ -652,12 +693,14 @@ Check out [our other projects][github], [follow us on twitter][twitter], [apply ### Contributors -| [![Erik Osterman][osterman_avatar]][osterman_homepage]
[Erik Osterman][osterman_homepage] | [![Vladimir][SweetOps_avatar]][SweetOps_homepage]
[Vladimir][SweetOps_homepage] | [![RB][nitrocode_avatar]][nitrocode_homepage]
[RB][nitrocode_homepage] | [![Andriy Knysh][aknysh_avatar]][aknysh_homepage]
[Andriy Knysh][aknysh_homepage] | -|---|---|---|---| +| [![Erik Osterman][osterman_avatar]][osterman_homepage]
[Erik Osterman][osterman_homepage] | [![Nuru][Nuru_avatar]][Nuru_homepage]
[Nuru][Nuru_homepage] | [![Vladimir][SweetOps_avatar]][SweetOps_homepage]
[Vladimir][SweetOps_homepage] | [![RB][nitrocode_avatar]][nitrocode_homepage]
[RB][nitrocode_homepage] | [![Andriy Knysh][aknysh_avatar]][aknysh_homepage]
[Andriy Knysh][aknysh_homepage] | +|---|---|---|---|---| [osterman_homepage]: https://github.com/osterman [osterman_avatar]: https://img.cloudposse.com/150x150/https://github.com/osterman.png + [Nuru_homepage]: https://github.com/Nuru + [Nuru_avatar]: https://img.cloudposse.com/150x150/https://github.com/Nuru.png [SweetOps_homepage]: https://github.com/SweetOps [SweetOps_avatar]: https://img.cloudposse.com/150x150/https://github.com/SweetOps.png [nitrocode_homepage]: https://github.com/nitrocode diff --git a/README.yaml b/README.yaml index a88531b..b86eecc 100644 --- a/README.yaml +++ b/README.yaml @@ -93,7 +93,15 @@ usage: |- The "type" of an object is itself an object: the keys are the same, and the values are the types of the values in the object. So although `{ foo = "bar", baz = {} }` and `{ foo = "bar", baz = [] }` are both objects, - they are not of the same type. This means you cannot put them both in the same list or the same map, + they are not of the same type, and you can get error messages like + + ``` + Error: Inconsistent conditional result types + The true and false result expressions must have consistent types. The given + expressions are object and object, respectively. + ``` + + This means you cannot put them both in the same list or the same map, even though you can put them in a single tuple or object. Similarly, and closer to the problem at hand, @@ -104,23 +112,29 @@ usage: |- } ``` is not the same type as + ```hcl self_rule = { type = "ingress" self = true } ``` + This means you cannot put both of those in the same list. + ```hcl rules = tolist([local.cidr_rule, local.self_rule]) ``` + Generates the error + ```text Invalid value for "v" parameter: cannot convert tuple to list of any single type. ``` You could make them the same type and put them in a list, like this: + ```hcl rules = tolist([{ type = "ingress" @@ -133,9 +147,11 @@ usage: |- self = true }]) ``` + That remains an option for you when generating the rules, and is probably better when you have full control over all the rules. However, what if some of the rules are coming from a source outside of your control? You cannot simply add those rules to your list. So, what to do? Create an object whose attributes' values can be of different types. + ```hcl { mine = local.my_rules, theirs = var.their_rules } ``` @@ -233,7 +249,32 @@ usage: |- changed if their keys do not change and the rules themselves do not change, except in the case of `rule_matrix`, where the rules are still dependent on the order of the security groups in `source_security_group_ids`. You can avoid this by using `rules` instead of `rule_matrix` when you have - more than one security group in the list. + more than one security group in the list. You cannot avoid this by sorting the + `source_security_group_ids`, because that leads to the "Invalid `for_each` argument" error. + + ##### Invalid for_each argument + You can supply a number of rules as inputs to this module, and they (usually) get transformed into + `aws_security_group_rule` resources. However, Terraform works in 2 steps: a `plan` step where it + calculates the changes to be made, and an `apply` step where it makes the changes. This is so you + can review and approve the plan before changing anything. One big limitation of this approach is + that it requires that Terraform be able to count the number of resources to create without the + benefit of any data generated during the `apply` phase. So if you try to generate a rule based + on something you are creating at the same time, you can get an error like + + ``` + Error: Invalid for_each argument + The "for_each" value depends on resource attributes that cannot be determined until apply, + so Terraform cannot predict how many instances will be created. + ``` + + This module uses lists to minimize the chance of that happening, as all it needs to know + is the length of the list, not the values in it, but this error still can + happen for subtle reasons. Most commonly, using a function like `compact` on a list + will cause the length to become unknown (since the values have to be checked and `null`s removed). + In the case of `source_security_group_ids`, just sorting the list using `sort` + will cause this error. If you run into this error, check for functions like `compact` somewhere + in the chain that produces the list and remove them if you find them. + ##### WARNINGS and Caveats @@ -387,6 +428,8 @@ include: contributors: - name: "Erik Osterman" github: "osterman" + - name: "Nuru" + github: "Nuru" - name: "Vladimir" github: "SweetOps" - name: "RB" diff --git a/docs/terraform.md b/docs/terraform.md index db124f5..940d0f3 100644 --- a/docs/terraform.md +++ b/docs/terraform.md @@ -50,8 +50,8 @@ | [regex\_replace\_chars](#input\_regex\_replace\_chars) | Terraform regular expression (regex) string.
Characters matching the regex will be removed from the ID elements.
If not set, `"/[^a-zA-Z0-9-]/"` is used to remove all characters other than hyphens, letters and digits. | `string` | `null` | no | | [revoke\_rules\_on\_delete](#input\_revoke\_rules\_on\_delete) | Instruct Terraform to revoke all of the Security Group's attached ingress and egress rules before deleting
the security group itself. This is normally not needed. | `bool` | `false` | no | | [rule\_matrix](#input\_rule\_matrix) | A convenient way to apply the same set of rules to a set of subjects. See README for details. | `any` | `[]` | no | -| [rules](#input\_rules) | A list of Security Group rule objects. All elements of a list must be exactly the same type;
use `rules_map` if you want to supply multiple lists of different types.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule . | `list(any)` | `[]` | no | -| [rules\_map](#input\_rules\_map) | A map-like object of lists of Security Group rule objects. All elements of a list must be exactly the same type,
so this input accepts an object with keys (attributes) whose values are lists so you can separate different
types into different lists and still pass them into one input. Keys must be known at "plan" time.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule . | `any` | `{}` | no | +| [rules](#input\_rules) | A list of Security Group rule objects. All elements of a list must be exactly the same type;
use `rules_map` if you want to supply multiple lists of different types.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see the `security_group_rule` [documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule).
\_\_\_Note:\_\_\_ The length of the list must be known at plan time.
This means you cannot use functions like `compact` or `sort` when computing the list. | `list(any)` | `[]` | no | +| [rules\_map](#input\_rules\_map) | A map-like object of lists of Security Group rule objects. All elements of a list must be exactly the same type,
so this input accepts an object with keys (attributes) whose values are lists so you can separate different
types into different lists and still pass them into one input. Keys must be known at "plan" time.
The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource,
except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique
and known at "plan" time.
To get more info see the `security_group_rule` [documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule). | `any` | `{}` | no | | [security\_group\_create\_timeout](#input\_security\_group\_create\_timeout) | How long to wait for the security group to be created. | `string` | `"10m"` | no | | [security\_group\_delete\_timeout](#input\_security\_group\_delete\_timeout) | How long to retry on `DependencyViolation` errors during security group deletion from
lingering ENIs left by certain AWS services such as Elastic Load Balancing. | `string` | `"15m"` | no | | [security\_group\_description](#input\_security\_group\_description) | The description to assign to the created Security Group.
Warning: Changing the description causes the security group to be replaced. | `string` | `"Managed by Terraform"` | no | diff --git a/examples/complete/main.tf b/examples/complete/main.tf index a3c993c..96d2b2f 100644 --- a/examples/complete/main.tf +++ b/examples/complete/main.tf @@ -10,7 +10,7 @@ provider "aws" { module "vpc" { source = "cloudposse/vpc/aws" - version = "v0.25.0" + version = "v1.1.0" cidr_block = "10.0.0.0/24" @@ -56,7 +56,7 @@ module "new_security_group" { # In TF 0.14 and later (through 1.0.x) if the length of the cidr_blocks # list is not available at plan time, the module breaks. cidr_blocks = random_integer.coin.result > 1 ? ["10.0.0.0/16"] : ["10.0.0.0/24"] - ipv6_cidr_blocks = [module.vpc.ipv6_cidr_block] + ipv6_cidr_blocks = [module.vpc.vpc_ipv6_cidr_block] prefix_list_ids = [] # Making `self` derived should break `count`, as it legitimately makes @@ -92,10 +92,10 @@ module "new_security_group" { to_port = 443 protocol = "tcp" cidr_blocks = ["10.0.0.0/8"] - ipv6_cidr_blocks = [module.vpc.ipv6_cidr_block] # ["::/0"] # + ipv6_cidr_blocks = [module.vpc.vpc_ipv6_cidr_block] # ["::/0"] # source_security_group_id = null description = "Discrete HTTPS ingress by CIDR" - self = null + self = false }] }, { new-sg = [{ # no key provided diff --git a/normalize.tf b/normalize.tf index 28a4686..4e23fd4 100644 --- a/normalize.tf +++ b/normalize.tf @@ -27,7 +27,9 @@ locals { source_security_group_id = lookup(rule, "source_security_group_id", null) security_groups = [] - self = lookup(rule, "self", null) + # self conflicts with other arguments, so it must either be + # null or true (in which case we split it out into a separate rule) + self = lookup(rule, "self", null) == true ? true : null }]])...) : [] # in rule_matrix and inline rules, a single rule can have a list of security groups @@ -51,7 +53,9 @@ locals { source_security_group_id = null security_groups = lookup(subject, "source_security_group_ids", []) - self = lookup(subject, "self", null) + # self conflicts with other arguments, so it must either be + # null or true (in which case we split it out into a separate rule) + self = lookup(rule, "self", null) == true ? true : null }]])...) : [] allow_egress_rule = { @@ -93,14 +97,16 @@ locals { cidr_blocks = [] ipv6_cidr_blocks = [] prefix_list_ids = [] - self = rule.self + self = true security_groups = [] source_security_group_id = null - # To preserve count and order of rules, create rules for `false` if though they do nothing, - # so that toggling to true does not have ripple effects. - } if rule.self != null] + # To preserve count and order of rules, we would like to create rules for `false` + # even though they do nothing, but an empty rule is not allowed, so we have to + # create the rule only when `self` is `true`. + # We use `== true` because `self` could be `null` and `if null` is not allowed. + } if rule.self == true] other_rules = local.inline ? [] : [for rule in local.norm_matrix : { key = "${rule.key}#cidr" diff --git a/variables.tf b/variables.tf index ccfaee2..9ebd409 100644 --- a/variables.tf +++ b/variables.tf @@ -66,7 +66,9 @@ variable "rules" { The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource, except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique and known at "plan" time. - To get more info see https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule . + To get more info see the `security_group_rule` [documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule). + ___Note:___ The length of the list must be known at plan time. + This means you cannot use functions like `compact` or `sort` when computing the list. EOT } @@ -80,7 +82,7 @@ variable "rules_map" { The keys and values of the Security Group rule objects are fully compatible with the `aws_security_group_rule` resource, except for `security_group_id` which will be ignored, and the optional "key" which, if provided, must be unique and known at "plan" time. - To get more info see https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule . + To get more info see the `security_group_rule` [documentation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule). EOT }