Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws_autoscaling_group "interpolated tags" example contains non-standard usage that won't upgrade well to 0.12 #7655

Closed
apparentlymart opened this issue Feb 22, 2019 · 5 comments
Assignees
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. documentation Introduces or discusses updates to documentation. service/ec2 Issues and PRs that pertain to the ec2 service.
Milestone

Comments

@apparentlymart
Copy link
Contributor

apparentlymart commented Feb 22, 2019

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

The documentation for aws_autoscaling_group currently includes an "Interpolated tags" example, as follows:

variable "extra_tags" {
  default = [
    {
      key                 = "Foo"
      value               = "Bar"
      propagate_at_launch = true
    },
    {
      key                 = "Baz"
      value               = "Bam"
      propagate_at_launch = true
    },
  ]
}

resource "aws_autoscaling_group" "bar" {
  name                 = "foobar3-terraform-test"
  max_size             = 5
  min_size             = 2
  launch_configuration = "${aws_launch_configuration.foobar.name}"
  vpc_zone_identifier  = ["${aws_subnet.example1.id}", "${aws_subnet.example2.id}"]

  tags = [
    {
      key                 = "explicit1"
      value               = "value1"
      propagate_at_launch = true
    },
    {
      key                 = "explicit2"
      value               = "value2"
      propagate_at_launch = true
    },
  ]

  tags = ["${concat(
    list(
      map("key", "interpolation1", "value", "value3", "propagate_at_launch", true),
      map("key", "interpolation2", "value", "value4", "propagate_at_launch", true)
    ),
    var.extra_tags)
  }"]
}

I understand from the history that this tags argument (defined as a list of maps attribute, rather than as a nested block) was added alongside tag to allow for this sort of dynamic construction. The example above is relying on the accidental flexibility that resulted from the lossy nature of configuration decoding in Terraform 0.11, where defining the same attribute more than once ended up being treated the same as defining a nested block more than once. Such usage is no longer allowed in Terraform 0.12, where we allow each argument to be defined only once.

A better way to write this for Terraform 0.11 would be to show only a single dynamic attribute definition with all of the elements together in the same expression, which I think is also subjectively clearer because it doesn't leave the unfamiliar reader wondering about the meaning of a repeated definition of the same argument:

  tags = ["${concat(
    list(
      map("key", "explicit1", "value", "value1", "propagate_at_launch", "true"),
      map("key", "explicit2", "value", "value2", "propagate_at_launch", "true")
    ),
    var.extra_tags
  )}"]
}

Because the Terraform v0.12 upgrade tool has a requirement of preserving existing declaration order and comments it will not combine these two arguments together and will instead just migrate both of them and produce an error after upgrade. I think that the error that results is the best we can do to handle this unusual case for existing users, but I'm hoping we can also update the documentation to reflect a usage pattern that will migrate correctly, such as the above.

We might also consider in this particular case having separate examples for Terraform 0.11 and 0.12, because I think the idiomatic way to write this for 0.12 would be to stick to using the tag nested block type and ignore the tags attribute altogether:

variable "extra_tags" {
  type = map(string)
  default = {
    Foo = "Bar"
    Baz = "Bam"
  }
}

resource "aws_autoscaling_group" "bar" {
  name                 = "foobar3-terraform-test"
  max_size             = 5
  min_size             = 2
  launch_configuration = aws_launch_configuration.foobar.name
  vpc_zone_identifier  = [aws_subnet.example1.id, aws_subnet.example2.id]

  tag {
    key                 = "explicit1"
    value               = "value1"
    propagate_at_launch = true
  }
  tag {
    key                 = "explicit2"
    value               = "value2"
    propagate_at_launch = true
  }
  dynamic "tag" {
    for_each = var.extra_tags
    content {
      key   = tag.key
      value = tag.value

      propagate_at_launch = true
    }
  }
}

Affected Resources

  • aws_autoscaling_group

Additional Context

The following is the result from upgrading the example exactly as given in the documentation using terraform 0.12upgrade:

resource "aws_autoscaling_group" "bar" {
  name                 = "foobar3-terraform-test"
  max_size             = 5
  min_size             = 2
  launch_configuration = aws_launch_configuration.foobar.name
  vpc_zone_identifier  = [aws_subnet.example1.id, aws_subnet.example2.id]

  tags = [
    {
      key                 = "explicit1"
      value               = "value1"
      propagate_at_launch = true
    },
    {
      key                 = "explicit2"
      value               = "value2"
      propagate_at_launch = true
    },
  ]

  # TF-UPGRADE-TODO: In Terraform v0.10 and earlier, it was sometimes necessary to
  # force an interpolation expression to be interpreted as a list by wrapping it
  # in an extra set of list brackets. That form was supported for compatibilty in
  # v0.11, but is no longer supported in Terraform v0.12.
  #
  # If the expression in the following list itself returns a list, remove the
  # brackets to avoid interpretation as a list of lists. If the expression
  # returns a single list item then leave it as-is and remove this TODO comment.
  tags = [
    concat(
      [
        {
          "key"                 = "interpolation1"
          "value"               = "value3"
          "propagate_at_launch" = true
        },
        {
          "key"                 = "interpolation2"
          "value"               = "value4"
          "propagate_at_launch" = true
        },
      ],
      var.extra_tags,
    ),
  ]
}

As noted above, it produces two definitions of tags = because of the requirement to maintain declaration order and any attached comments, even though a subsequent terraform validate would detect that second definition as an error.

This shows another interesting quirk of this particular example where the upgrade tool is unable to definitively prove whether or not the expression given in tags is an example of the legacy usage pattern of wrapping a list expression in a redundant set of list brackets, and so it generates a TF-UPGRADE-TODO to prompt the user to decide. In this case it looks like it would be necessary to remove that extra level of brackets to make this work. The upgrade tool is able to make a decision on this in most normal cases, but this one is a little too complicated so it takes the safe path of prompting the user to review it.

Interestingly, writing the original example using the tag block type rather than the tags attribute -- while retaining everything else unchanged -- produces a more complete result because the upgrade tool can recognize it as a usage of the pre-0.12 hack of treating a block type as if it were an attribute:

resource "aws_autoscaling_group" "bar" {
  name                 = "foobar3-terraform-test"
  max_size             = 5
  min_size             = 2
  launch_configuration = aws_launch_configuration.foobar.name
  vpc_zone_identifier  = [aws_subnet.example1.id, aws_subnet.example2.id]

  tag {
    key                 = "explicit1"
    value               = "value1"
    propagate_at_launch = true
  }
  tag {
    key                 = "explicit2"
    value               = "value2"
    propagate_at_launch = true
  }

  dynamic "tag" {
    for_each = [concat(
      [
        {
          "key"                 = "interpolation1"
          "value"               = "value3"
          "propagate_at_launch" = true
        },
        {
          "key"                 = "interpolation2"
          "value"               = "value4"
          "propagate_at_launch" = true
        },
      ],
      var.extra_tags,
    )]
    content {
      # TF-UPGRADE-TODO: The automatic upgrade tool can't predict
      # which keys might be set in maps assigned here, so it has
      # produced a comprehensive set here. Consider simplifying
      # this after confirming which keys can be set in practice.

      key                 = tag.value.key
      propagate_at_launch = tag.value.propagate_at_launch
      value               = tag.value.value
    }
  }
}

This result is not perfect by any means either... it includes more than strictly necessary in the dynamic block because it is being conservative about maintaining what it considers to be a dynamic expression. But this would, at least, be parsed as expected by a subsequent terraform validate since the multiple definitions are of distinct tag blocks rather than the tags attribute.

Since the existing example will undoubtedly already have been used as the basis for real configurations the difference above is pretty academic and not something we can really do anything about now, but I thought it was interesting additional context to see exactly how the upgrade tool is understanding this right now.

@apparentlymart apparentlymart added documentation Introduces or discusses updates to documentation. service/ec2 Issues and PRs that pertain to the ec2 service. labels Feb 22, 2019
@bflad
Copy link
Contributor

bflad commented Jul 13, 2020

I think our preference will be for removing the tags argument (set of maps) in a future major version of the Terraform AWS Provider. I'm going to mark this issue for that consideration. 👍

@bflad bflad added this to the v4.0.0 milestone Jul 13, 2020
@anGie44 anGie44 self-assigned this Jan 19, 2022
@anGie44 anGie44 added the breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. label Jan 19, 2022
@anGie44 anGie44 modified the milestones: v4.0.0, v5.0.0 Jan 19, 2022
@anGie44
Copy link
Contributor

anGie44 commented Feb 2, 2022

The tags argument has been deprecated as part of v4.0 (#22663) and will be eventually removed in the next major release.

@jar-b
Copy link
Member

jar-b commented May 23, 2023

Closed by #30842, merged to main via #31392

@jar-b jar-b closed this as completed May 23, 2023
@github-actions
Copy link

This functionality has been released in v5.0.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. documentation Introduces or discusses updates to documentation. service/ec2 Issues and PRs that pertain to the ec2 service.
Projects
None yet
Development

No branches or pull requests

5 participants