providers/aws: add tags for resource_aws_autoscaling_group #13574

s-urbaniak · 2017-04-12T13:59:30Z

The existing "tag" field on autoscaling groups is very limited in that it
is unable to accept a dynamic list of tag entries.

Other AWS resources don't have this restriction on tags because they work
directly on the map type.

AWS autoscaling groups on the other hand have an additional field
"propagate_at_launch" which is not usable with a pure map type.

This fixes it by introducing an additional field called "tags" which
allows specifying a list of maps. This preserves the possibility to
declare tags as with the "tag" field but additionally allows to
construct lists of maps using interpolation syntax.

/cc @philips @Quentin-M @sym3tri @alexsomesan

alexsomesan · 2017-04-12T16:04:01Z

👍
I'm working on the same project as @s-urbaniak.
We are building a Kubernetes deployment tool based on terraform. The cluster itself expects some predefined tags to detect underlying AWS resources, but we also want to allow the users to set their own custom tags on top, which we take in as TF vars.
In this scenario we needed to feed the tags attribute with a merged map of our own tags + the users'
All resources we use supported that, but the auto-scaling groups.

alexsomesan · 2017-04-24T15:48:11Z

@stack72 @radeksimko

Sorry for pinging directly. Is there any chance to get some initial feedback on this, so that we know if it's a feasible idea?
We're betting pretty strongly on this feature as part of our project. It would be really helpful to know if this change has a chance of making it in or we'd have to plan for vendoring it somehow on our own.

radeksimko · 2017-04-26T09:38:37Z

Hi @s-urbaniak
thanks for the PR.

I agree that dynamic tags for ASG is a reasonable use case and we should support it. 👍 👌

We didn't have a good support for complex data structures like list of maps in the past and the situation still isn't ideal but will improve over time.

There are some related caveats to mention:

We do not support schema-based validation for maps (yet) - so you can't perform validation for anything other than value type (bool/int/float/string)
We have been generally treating TypeMap as type suitable for maps with arbitrary keys and values, which isn't the case here as the API has very clear requirements - key, value, propagate_at_launch. This is also why it's very easy to cause the current code in this PR to crash (just leave out key or value in the config)
There is no way to enter a list of maps interactively in the CLI, although I doubt anyone would ever need to do it

Perhaps supporting schema validation for a map like this would be a solution:

Schema: map[string]*Schema{
	"validatableMap": {
		Type: TypeMap,
		Elem: map[string]*Schema{
			"key": {
				Type:         TypeString,
				ValidateFunc: validateKey,
			},
			"value": {
				Type:         TypeString,
				ValidateFunc: validateValue,
			},
			"propagate_at_launch": {
				Type: TypeBool,
			},
		},
	},
},

but this is not something to implement on the provider/resource side - at least not now as schema doesn't support such validation and we need to figure out whether/how we're going to support deeper nesting, i.e. map of maps etc.

I need to discuss this with other maintainers which work closer to the core to see what's the best way forward before I give you any specific suggestions.

Thanks for the patience.

s-urbaniak · 2017-04-26T16:11:35Z

Hi @radeksimko, thanks a lot for your detailed analysis!

We do not support schema-based validation for maps (yet) - so you can't perform validation for anything other than value type (bool/int/float/string)

Agreed, it would be very nice to add maps validation. I am fixing my local branch to simply ignore invalid map values for this use case.

We have been generally treating TypeMap as type suitable for maps with arbitrary keys and values, which isn't the case here as the API has very clear requirements - key, value, propagate_at_launch. This is also why it's very easy to cause the current code in this PR to crash (just leave out key or value in the config)

That is actually a very good catch! I am already working on fixing that very crash including tests.

There is no way to enter a list of maps interactively in the CLI, although I doubt anyone would ever need to do it

Our use case involves parametrization via configuration files, so indeed it does not apply.

I need to discuss this with other maintainers which work closer to the core to see what's the best way forward before I give you any specific suggestions.

That would be great to get some feedback on how to accomplish this feature! In the meanwhile I am working on fixing the mentioned edge cases.

Having said that it would be great to land this as a stop-gap solution once I fix the above edge cases :-)

s-urbaniak · 2017-04-26T17:02:14Z

@radeksimko I fixed the review comment about the potential crashes, also incorporating invalid values into the test. Do you mind to have another look?

s-urbaniak · 2017-04-27T12:46:31Z

@radeksimko I have been brainstorming with @alexsomesan how we can come up with a more sensible solution while still being idiomatic with the current type system and its limitations:

we leave the new attribute tags as a map like all the other resources. The bonus would then be that tag key/values from other AWS resources then can be immediately reused for autoscaling groups. Since the autoscaling group additionally has the information propagate_at_launch we could provide this information as an additional list of key values which will have this attribute set to true.

A sample configuration would then look like this:

resource "aws_autoscaling_group" "masters" {
  tag {
    key = "some"
    value = "thing"
    propagate_at_launch = true
  }

  tags = {
    foo = "value1"
    bar = "value2"
    baz = "value3"
  }

  propagate_at_launch = ["foo", "bar"]
}

In the example above the tag keys some, foo and bar would gain the setting propagate_at_launch = true, the tag key baz would gain the setting propagate_at_launch = false, because this key is not present in the list.

What do you think?

joshuaspence · 2017-04-27T21:05:58Z

Nice, I like that solution.

alexsomesan · 2017-04-28T08:06:41Z

Just pitching in to highlight the fact that this latter approach using plain maps is exactly in line with the current tagging syntax of the other resources, which also don't validate the tags map.

s-urbaniak · 2017-05-02T15:20:08Z

Hello @radeksimko, just kindly pinging if you had the chance to reiterate on the comments above. Sorry for the noise, and thanks a lot!

radeksimko

Hi @s-urbaniak
thank you for the patience. I took some time to discuss the problem with a few folks from the team which are more involved in the core/schema part of the codebase.

The conclusion is that we more or less agree this is not an ideal solution due to potential confusions in naming (tag & tags) and we generally perceive it as a workaround for currently limited HCL capabilities in Terraform.

That said we already have solid mechanisms in place which will allow us to deprecate one or the other field in the future gracefully when the HCL capabilities become more ideal, so it's not set in the stone forever and that is why we're happy to proceed with this PR.

I left you some questions/comments there - please have a look and let me know what you think.

radeksimko · 2017-05-08T10:02:42Z

builtin/providers/aws/autoscaling_tags.go

+	}
+
+	if v, ok := attr["propagate_at_launch"].(string); ok {
+		propagate_at_launch, _ = strconv.ParseBool(v)


Is there any specific reason to ignore this error here?
Also - perhaps a nitpick - but we prefer camelCase for internal variables rather than under_score.

The same reasoning as above, "ignore on error" and keep going. I am happy to introduce returning an error and also camelCase'ing.

radeksimko · 2017-05-08T10:45:44Z

builtin/providers/aws/autoscaling_tags.go

+	}
+
+	if _, ok := attr["value"]; !ok {
+		return nil


Is there any reason to ignore such case as it's an invalid block of code? I think it would make more sense to just return error in case that key, value or propagate_at_launch are not defined.

There have been no strong reasons, other than "ignore on error". I will introduce returning an error then.

radeksimko · 2017-05-08T10:48:48Z

builtin/providers/aws/resource_aws_autoscaling_group.go

+				Type:     schema.TypeList,
+				Optional: true,
+				Elem:     &schema.Schema{Type: schema.TypeMap},
+			},


Is there any reason we should accept both fields and deal with all kinds of potential edge cases related to that? If not, would you mind making them mutually exclusive via ConflictsWith?

Letting tag and tags be able to merge together just makes it nice when declaring default tags using tag and bringing in additional tags in like so: https://github.com/coreos/tectonic-installer/blob/45dfc2b/modules/aws/master-asg/master.tf#L35-L47.

I have no strong opinion on letting this be mutually exclusive, but from a logical standpoint I don't see why they should be.

I let you have the final word on this one :-)

Oh, since you mentioned if I can make them mutually exclusive: please take a look at my example posted above and let me know if you stand with your opinion to make it mutually exclusive; again I am happy to do so.

I see what do you mean - static, predefined tags combined with more tags from a variable.
I think this should work just fine

tags = [ { key = "foo" value = "bar" propagate_at_launch = true }, { key = "lorem" value = "ipsum" propagate_at_launch = false }, "${var.additional_tags}" ]

The main reason to make those mutually exclusive is to avoid having to document and deal with edge cases like duplicate tags or even conflicting ones (which one is the point of truth) and also to avoid any potential confusion in the heads of users - i.e. I don't want users to have to think about edge cases at all.

Also we don't have an acceptance test for importing ASGs yet, but I'm pretty confident it would fail in this PR because you're assuming that there's a config available - i.e. GetOk() in Read() decides which field to populate and during import none of those fields will be available, so none will be populated.

We need to decide which field is going to be the point of truth for this context. I'd vote for the existing tag field to avoid breaking changes.

I'm going to create the mentioned test and cc you in the PR, just for context.

Actually we do have a test for it, just in a different file I was expecting and it is failing on this branch:

=== RUN TestAccAWSAutoScalingGroup_importBasic --- FAIL: TestAccAWSAutoScalingGroup_importBasic (206.37s) testing.go:280: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected. (map[string]string) { } (map[string]string) (len=13) { (string) (len=5) "tag.#": (string) (len=1) "1", (string) (len=18) "tag.4237349706.key": (string) (len=7) "FromTag", (string) (len=34) "tag.4237349706.propagate_at_launch": (string) (len=4) "true", (string) (len=20) "tag.4237349706.value": (string) (len=6) "value1", (string) (len=6) "tags.#": (string) (len=1) "2", (string) (len=8) "tags.0.%": (string) (len=1) "3", (string) (len=10) "tags.0.key": (string) (len=9) "FromTags1", (string) (len=26) "tags.0.propagate_at_launch": (string) (len=1) "1", (string) (len=12) "tags.0.value": (string) (len=6) "value2", (string) (len=8) "tags.1.%": (string) (len=1) "3", (string) (len=10) "tags.1.key": (string) (len=9) "FromTags2", (string) (len=26) "tags.1.propagate_at_launch": (string) (len=1) "1", (string) (len=12) "tags.1.value": (string) (len=6) "value3" }

radeksimko · 2017-05-08T10:57:31Z

we leave the new attribute tags as a map like all the other resources.

I see where you're coming from with that idea, but I'm not too inclined to it.

Terraform should make users aware that ASG tags are somewhat special - i.e. require propagate_at_launch. I think that splitting this into 2 fields would make it more difficult to validate and manage in general. It would be too easy to forget updating one or the other field.

Also I take it as benefit if we can stick to the conventions in the upstream API - i.e. list of maps as people coming from other tools or generating HCL/JSON may find it useful.

s-urbaniak · 2017-05-08T12:37:50Z

@radeksimko No worries at all for the late reply! I appreciate your effort in looking at this! Regarding my second suggestion it was just a suggested workaround. We do like the current solution better too.

I will post an update on this PR tomorrow which will address your review comments, just please let me know your final decision on the mutual exclusiveness on tag vs. tags.

radeksimko · 2017-05-08T16:44:45Z

@s-urbaniak Thank you for a quick response. Just to add one important thing - can you make sure you run all relevant (that is all ASG) acceptance tests and check they're all passing with this PR?

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAutoScalingGroup_'

s-urbaniak · 2017-05-08T16:58:38Z

@radeksimko definitely, thanks for the pointer on those tests, I already am half-through the comments locally (error handling) and will make sure those tests pass too.

s-urbaniak · 2017-05-11T16:12:39Z

@radeksimko PTAL, I think I addressed all the review comments.

Here is the output of the acceptance test. Without deeper investigation I believe the failed TestAccAWSAutoScalingGroup_ALB_TargetGroups is unrelated but I need to verify this tomorrow.

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAutoScalingGroup_'
==> Checking that code complies with gofmt requirements...
which: no stringer in (/home/sur/go/bin:/home/sur/go/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl)
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/11 17:11:49 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAutoScalingGroup_ -timeout 120m
=== RUN   TestAccAWSAutoScalingGroup_importBasic
--- PASS: TestAccAWSAutoScalingGroup_importBasic (305.58s)
=== RUN   TestAccAWSAutoScalingGroup_basic
--- PASS: TestAccAWSAutoScalingGroup_basic (287.32s)
=== RUN   TestAccAWSAutoScalingGroup_namePrefix
--- PASS: TestAccAWSAutoScalingGroup_namePrefix (59.83s)
=== RUN   TestAccAWSAutoScalingGroup_autoGeneratedName
--- PASS: TestAccAWSAutoScalingGroup_autoGeneratedName (50.09s)
=== RUN   TestAccAWSAutoScalingGroup_terminationPolicies
--- PASS: TestAccAWSAutoScalingGroup_terminationPolicies (143.87s)
=== RUN   TestAccAWSAutoScalingGroup_tags
--- PASS: TestAccAWSAutoScalingGroup_tags (293.79s)
=== RUN   TestAccAWSAutoScalingGroup_VpcUpdates
--- PASS: TestAccAWSAutoScalingGroup_VpcUpdates (142.76s)
=== RUN   TestAccAWSAutoScalingGroup_WithLoadBalancer
--- PASS: TestAccAWSAutoScalingGroup_WithLoadBalancer (422.11s)
=== RUN   TestAccAWSAutoScalingGroup_withPlacementGroup
--- PASS: TestAccAWSAutoScalingGroup_withPlacementGroup (181.87s)
=== RUN   TestAccAWSAutoScalingGroup_enablingMetrics
--- PASS: TestAccAWSAutoScalingGroup_enablingMetrics (215.47s)
=== RUN   TestAccAWSAutoScalingGroup_suspendingProcesses
--- PASS: TestAccAWSAutoScalingGroup_suspendingProcesses (231.12s)
=== RUN   TestAccAWSAutoScalingGroup_withMetrics
--- PASS: TestAccAWSAutoScalingGroup_withMetrics (87.56s)
=== RUN   TestAccAWSAutoScalingGroup_ALB_TargetGroups
--- FAIL: TestAccAWSAutoScalingGroup_ALB_TargetGroups (50.46s)
	testing.go:280: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_launch_configuration.foobar: 1 error(s) occurred:
		
		* aws_launch_configuration.foobar: Error creating launch configuration: ValidationError: AMI cannot be described
			status code: 400, request id: 25c54697-3662-11e7-87ef-0968d0bf68c0
=== RUN   TestAccAWSAutoScalingGroup_initialLifecycleHook
--- PASS: TestAccAWSAutoScalingGroup_initialLifecycleHook (264.13s)
=== RUN   TestAccAWSAutoScalingGroup_ALB_TargetGroups_ELBCapacity
--- PASS: TestAccAWSAutoScalingGroup_ALB_TargetGroups_ELBCapacity (347.24s)
FAIL
exit status 1
FAIL	github.com/hashicorp/terraform/builtin/providers/aws	3083.209s
make: *** [Makefile:49: testacc] Error 1

The existing "tag" field on autoscaling groups is very limited in that it cannot be used in conjunction with interpolation preventing from adding dynamic tag entries. Other AWS resources don't have this restriction on tags because they work directly on the map type. AWS autoscaling groups on the other hand have an additional field "propagate_at_launch" which is not usable with a pure map type. This fixes it by introducing an additional field called "tags" which allows specifying a list of maps. This preserves the possibility to declare tags as with the "tag" field but additionally allows to construct lists of maps using interpolation syntax.

radeksimko · 2017-05-16T08:31:00Z

Hi @s-urbaniak
thank you for making those changes. This is now looking good to me.

I'm just waiting for acceptance tests to finish and if all pass I'll merge it.

s-urbaniak · 2017-05-16T08:36:15Z

@radeksimko Great news, thanks a ton!

Since upstream wants them mutually exclusive. See hashicorp/terraform#13574 (comment) for a rationale.

luisdavim · 2018-01-16T09:36:56Z

in what version of terraform will this be available? the example posted above by @s-urbaniak doesn't work with 0.11.1

bflad · 2018-01-16T11:26:18Z

@luisdavim aws_autoscaling_group tagging has been available for a few months now, since at least Terraform 0.10 which split providers like AWS away from core, announcement here, and the first version of the split terraform-provider-aws 0.1.0.

You can double check your current core and provider versions with terraform version and upgrade following the instructions available in the Provider Versions section of the documentation.

If you're still having trouble, I would suggest opening an issue in the terraform-provider-aws repository.

Hope this helps!

ghost · 2020-04-05T02:13:14Z

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.

s-urbaniak mentioned this pull request Apr 12, 2017

aws: allow custom tags for AWS resources coreos/tectonic-installer#218

Merged

s-urbaniak force-pushed the tags-master branch from 6cb1fbc to 9245819 Compare April 12, 2017 14:14

Quentin-M mentioned this pull request Apr 14, 2017

terraform: upstream custom plugins coreos/tectonic-installer#60

Closed

2 tasks

mbfrahry added enhancement provider/aws labels Apr 19, 2017

s-urbaniak force-pushed the tags-master branch 2 times, most recently from e47c178 to 82bda74 Compare April 26, 2017 16:54

radeksimko self-assigned this Apr 27, 2017

s-urbaniak mentioned this pull request May 8, 2017

AWS Terraform config doesn't work due to invalid aws_autoscaling_group argument coreos/tectonic-installer#571

Closed

radeksimko requested changes May 8, 2017

View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label May 8, 2017

s-urbaniak force-pushed the tags-master branch from 82bda74 to 8afe0ef Compare May 11, 2017 15:58

s-urbaniak force-pushed the tags-master branch from 8afe0ef to 72e2295 Compare May 12, 2017 05:38

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label May 16, 2017

radeksimko approved these changes May 16, 2017

View reviewed changes

radeksimko merged commit 399830f into hashicorp:master May 16, 2017

s-urbaniak pushed a commit to s-urbaniak/tectonic-installer that referenced this pull request May 19, 2017

modules/aws: convert tag entries into tags

736ae2f

Since upstream wants them mutually exclusive. See hashicorp/terraform#13574 (comment) for a rationale.

pgroudas mentioned this pull request May 22, 2017

[AWS] Pass tags to module creating an ASG #9098

Closed

This was referenced Jun 15, 2017

Revisiting AWS ASG Tagging #15298

Closed

Revisiting AWS ASG Tagging hashicorp/terraform-provider-aws#874

Closed

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

providers/aws: add tags for resource_aws_autoscaling_group #13574

providers/aws: add tags for resource_aws_autoscaling_group #13574

s-urbaniak commented Apr 12, 2017 •

edited

Loading

alexsomesan commented Apr 12, 2017 •

edited

Loading

alexsomesan commented Apr 24, 2017

radeksimko commented Apr 26, 2017

s-urbaniak commented Apr 26, 2017

s-urbaniak commented Apr 26, 2017

s-urbaniak commented Apr 27, 2017 •

edited

Loading

joshuaspence commented Apr 27, 2017

alexsomesan commented Apr 28, 2017

s-urbaniak commented May 2, 2017

radeksimko left a comment

radeksimko May 8, 2017

s-urbaniak May 8, 2017

radeksimko May 8, 2017

s-urbaniak May 8, 2017

radeksimko May 8, 2017

s-urbaniak May 8, 2017

s-urbaniak May 8, 2017

radeksimko May 8, 2017

radeksimko May 8, 2017

radeksimko commented May 8, 2017

s-urbaniak commented May 8, 2017

radeksimko commented May 8, 2017 •

edited

Loading

s-urbaniak commented May 8, 2017

s-urbaniak commented May 11, 2017

radeksimko commented May 16, 2017

s-urbaniak commented May 16, 2017

luisdavim commented Jan 16, 2018

bflad commented Jan 16, 2018

ghost commented Apr 5, 2020

providers/aws: add tags for resource_aws_autoscaling_group #13574

providers/aws: add tags for resource_aws_autoscaling_group #13574

Conversation

s-urbaniak commented Apr 12, 2017 • edited Loading

alexsomesan commented Apr 12, 2017 • edited Loading

alexsomesan commented Apr 24, 2017

radeksimko commented Apr 26, 2017

s-urbaniak commented Apr 26, 2017

s-urbaniak commented Apr 26, 2017

s-urbaniak commented Apr 27, 2017 • edited Loading

joshuaspence commented Apr 27, 2017

alexsomesan commented Apr 28, 2017

s-urbaniak commented May 2, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko commented May 8, 2017

s-urbaniak commented May 8, 2017

radeksimko commented May 8, 2017 • edited Loading

s-urbaniak commented May 8, 2017

s-urbaniak commented May 11, 2017

radeksimko commented May 16, 2017

s-urbaniak commented May 16, 2017

luisdavim commented Jan 16, 2018

bflad commented Jan 16, 2018

ghost commented Apr 5, 2020

s-urbaniak commented Apr 12, 2017 •

edited

Loading

alexsomesan commented Apr 12, 2017 •

edited

Loading

s-urbaniak commented Apr 27, 2017 •

edited

Loading

radeksimko commented May 8, 2017 •

edited

Loading