[Proof of Concept] Default tags implementation: provider and subnet/vpc resources #17974

anGie44 · 2021-03-08T04:51:10Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Output from acceptance testing:

--- PASS: TestAccAWSProvider_Region_AwsGovCloudUs (15.28s)
--- PASS: TestAccAWSProvider_Region_AwsSC2S (15.50s)
--- PASS: TestAccAWSProvider_Region_AwsCommercial (15.53s)
--- PASS: TestAccAWSProvider_Region_AwsC2S (15.78s)
--- PASS: TestAccAWSProvider_Region_AwsChina (16.42s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_None (19.23s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_One (19.51s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_Multiple (19.58s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_One (19.80s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_One (19.92s)
--- PASS: TestAccAWSProvider_IgnoreTags_EmptyConfigurationBlock (19.95s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_None (19.96s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_Multiple (19.97s)
--- PASS: TestAccAWSProvider_Endpoints (19.97s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_None (19.96s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_Multiple (20.19s)
--- PASS: TestAccAWSProvider_DefaultAndIgnoreTags_EmptyConfigurationBlocks (20.24s)
--- PASS: TestAccAWSProvider_AssumeRole_Empty (22.44s)

--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_duplicateTag (1.90s)
--- PASS: TestAccAWSSubnet_defaultTags_updateToProviderOnly (36.35s)
--- PASS: TestAccAWSSubnet_defaultTags_updateToResourceOnly (39.64s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_nonOverlappingTag (48.80s)
--- PASS: TestAccAWSSubnet_defaultTags_providerOnly (50.92s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_overlappingTag (51.19s)
--- PASS: TestAccAWSSubnet_defaultAndIgnoreTags (45.00s)

--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_duplicateTag (2.27s)
--- PASS: TestAccAWSVpc_defaultTags_updateToProviderOnly (31.94s)
--- PASS: TestAccAWSVpc_defaultTags_updateToResourceOnly (33.68s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_overlappingTag (47.60s)
--- PASS: TestAccAWSVpc_defaultTags_providerOnly (47.63s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_nonOverlappingTag (47.75s)
--- PASS: TestAccAWSVpc_defaultAndIgnoreTags (29.82s)

bflad · 2021-03-15T13:52:24Z

Pending some potential changes noted over in #17941 (review)

anGie44 · 2021-03-15T21:14:28Z

aws/tags.go

+		return fmt.Errorf(`"tags" are identical to those in the "default_tags" configuration block of the provider: please de-duplicate and try again`)
+	}
+
+	allTags := defaultTagsConfig.MergeTags(resourceTags).IgnoreConfig(ignoreTagsConfig)


to accommodate cases where both (default and ignore are configured). do you think this is a valid use case @bflad ? related tests are in each resource's test file with the suffix defaultAndIgnoreTags

I think configuring both is certainly valid as long as the tag keys do not overlap. For example, if I'm working in engineering and running Kubernetes I may want to do something like:

provider "aws" { default_tags { tags = { CostCenter = "Engineering" } } ignore_tags { key_prefixes = ["kubernetes.io/"] } }

Maybe we should error in the overlapping keys case, if we do not already. Does not need to be in the initial pull request though. 😄

bflad

This looks really good, I'm stepping into a meeting for an hour, but all I have left to do is run the testing at this point to verify. 👍

.changelog/17974.txt

bflad · 2021-03-16T14:37:31Z

aws/internal/keyvaluetags/key_value_tags_test.go

@@ -4,6 +4,254 @@ import (
 	"testing"
 )

+func TestKeyValueTagsDefaultConfigMergeTags(t *testing.T) {
+	testCases := []struct {


bflad · 2021-03-16T14:48:27Z

aws/resource_aws_subnet.go

-	if err := d.Set("tags", keyvaluetags.Ec2KeyValueTags(subnet.Tags).IgnoreAws().IgnoreConfig(ignoreTagsConfig).Map()); err != nil {
+	tags := keyvaluetags.Ec2KeyValueTags(subnet.Tags).IgnoreAws().IgnoreConfig(ignoreTagsConfig)
+
+	//lintignore:AWSR002


We can figure out how to adjust this linter after this pull request, even if that means creating a separate function to bundle this code up -- I personally really like the split here so tags and tags_all values are straightforward to 👀 in the d.Set() calls.

sounds good! i'll make an in issue for it and follow-up 👍

bflad · 2021-03-16T14:56:19Z

aws/resource_aws_subnet_test.go

+	})
+}
+
+func TestAccAWSSubnet_defaultTags_providerAndResource_overlappingTag(t *testing.T) {


Love all these test cases

bflad · 2021-03-16T14:58:09Z

aws/tags.go

+	if defaultTagsConfig.TagsEqual(resourceTags) {
+		return fmt.Errorf(`"tags" are identical to those in the "default_tags" configuration block of the provider: please de-duplicate and try again`)
+	}


I think this is a good error to have when all tags are equivalent 👍

bflad

Two nits on the changelog entries, otherwise this is a fantastic start. Great work. 🚀

Output from acceptance testing in AWS Commercial:

--- PASS: TestAccAWSProvider_AssumeRole_Empty (47.43s)
--- PASS: TestAccAWSProvider_DefaultAndIgnoreTags_EmptyConfigurationBlocks (23.95s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_Multiple (39.35s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_None (43.78s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_One (32.28s)
--- PASS: TestAccAWSProvider_Endpoints (40.52s)
--- PASS: TestAccAWSProvider_IgnoreTags_EmptyConfigurationBlock (30.92s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_Multiple (38.83s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_None (31.18s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_One (32.62s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_Multiple (39.85s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_None (35.49s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_One (36.41s)
--- PASS: TestAccAWSProvider_Region_AwsC2S (33.25s)
--- PASS: TestAccAWSProvider_Region_AwsChina (27.06s)
--- PASS: TestAccAWSProvider_Region_AwsCommercial (31.68s)
--- PASS: TestAccAWSProvider_Region_AwsGovCloudUs (36.04s)
--- PASS: TestAccAWSProvider_Region_AwsSC2S (33.29s)

--- PASS: TestAccAWSSubnet_availabilityZoneId (62.10s)
--- PASS: TestAccAWSSubnet_basic (66.30s)
--- PASS: TestAccAWSSubnet_defaultAndIgnoreTags (172.35s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_duplicateTag (11.92s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_nonOverlappingTag (198.12s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_overlappingTag (197.36s)
--- PASS: TestAccAWSSubnet_defaultTags_providerOnly (185.18s)
--- PASS: TestAccAWSSubnet_defaultTags_updateToProviderOnly (135.01s)
--- PASS: TestAccAWSSubnet_defaultTags_updateToResourceOnly (136.87s)
--- PASS: TestAccAWSSubnet_disappears (71.35s)
--- PASS: TestAccAWSSubnet_enableIpv6 (123.25s)
--- PASS: TestAccAWSSubnet_ignoreTags (173.12s)
--- PASS: TestAccAWSSubnet_ipv6 (119.99s)
--- PASS: TestAccAWSSubnet_MapPublicIpOnLaunch (234.20s)
--- PASS: TestAccAWSSubnet_tags (140.78s)
--- SKIP: TestAccAWSSubnet_CustomerOwnedIpv4Pool (1.04s)
--- SKIP: TestAccAWSSubnet_MapCustomerOwnedIpOnLaunch (0.71s)
--- SKIP: TestAccAWSSubnet_outpost (1.02s)

--- PASS: TestAccAWSVpc_AssignGeneratedIpv6CidrBlock (161.38s)
--- PASS: TestAccAWSVpc_basic (84.09s)
--- PASS: TestAccAWSVpc_bothDnsOptionsSet (103.70s)
--- PASS: TestAccAWSVpc_classiclinkDnsSupportOptionSet (90.72s)
--- PASS: TestAccAWSVpc_classiclinkOptionSet (89.79s)
--- PASS: TestAccAWSVpc_coreMismatchedDiffs (73.00s)
--- PASS: TestAccAWSVpc_defaultAndIgnoreTags (84.82s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_duplicateTag (11.01s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_nonOverlappingTag (187.70s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_overlappingTag (88.86s)
--- PASS: TestAccAWSVpc_defaultTags_providerOnly (155.12s)
--- PASS: TestAccAWSVpc_defaultTags_updateToProviderOnly (131.24s)
--- PASS: TestAccAWSVpc_defaultTags_updateToResourceOnly (131.38s)
--- PASS: TestAccAWSVpc_DisabledDnsSupport (101.98s)
--- PASS: TestAccAWSVpc_disappears (57.68s)
--- PASS: TestAccAWSVpc_ignoreTags (93.11s)
--- PASS: TestAccAWSVpc_tags (211.03s)
--- PASS: TestAccAWSVpc_Tenancy (161.25s)
--- PASS: TestAccAWSVpc_update (150.61s)

Output from acceptance testing in AWS GovCloud (US):

--- PASS: TestAccAWSProvider_AssumeRole_Empty (54.05s)
--- PASS: TestAccAWSProvider_DefaultAndIgnoreTags_EmptyConfigurationBlocks (43.18s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_Multiple (32.95s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_None (21.45s)
--- PASS: TestAccAWSProvider_DefaultTags_Tags_One (37.71s)
--- PASS: TestAccAWSProvider_Endpoints (40.39s)
--- PASS: TestAccAWSProvider_IgnoreTags_EmptyConfigurationBlock (38.03s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_Multiple (31.33s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_None (32.15s)
--- PASS: TestAccAWSProvider_IgnoreTags_KeyPrefixes_One (35.19s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_Multiple (40.27s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_None (32.84s)
--- PASS: TestAccAWSProvider_IgnoreTags_Keys_One (42.61s)
--- PASS: TestAccAWSProvider_Region_AwsC2S (31.91s)
--- PASS: TestAccAWSProvider_Region_AwsChina (27.52s)
--- PASS: TestAccAWSProvider_Region_AwsCommercial (18.89s)
--- PASS: TestAccAWSProvider_Region_AwsGovCloudUs (20.20s)
--- PASS: TestAccAWSProvider_Region_AwsSC2S (19.41s)

--- PASS: TestAccAWSSubnet_availabilityZoneId (53.68s)
--- PASS: TestAccAWSSubnet_basic (103.10s)
--- PASS: TestAccAWSSubnet_defaultAndIgnoreTags (174.14s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_duplicateTag (14.71s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_nonOverlappingTag (172.37s)
--- PASS: TestAccAWSSubnet_defaultTags_providerAndResource_overlappingTag (180.57s)
--- PASS: TestAccAWSSubnet_defaultTags_providerOnly (181.37s)
--- PASS: TestAccAWSSubnet_defaultTags_updateToProviderOnly (135.37s)
--- PASS: TestAccAWSSubnet_defaultTags_updateToResourceOnly (140.56s)
--- PASS: TestAccAWSSubnet_disappears (77.51s)
--- PASS: TestAccAWSSubnet_enableIpv6 (128.65s)
--- PASS: TestAccAWSSubnet_ignoreTags (176.63s)
--- PASS: TestAccAWSSubnet_ipv6 (121.96s)
--- PASS: TestAccAWSSubnet_MapPublicIpOnLaunch (179.33s)
--- PASS: TestAccAWSSubnet_tags (245.77s)
--- SKIP: TestAccAWSSubnet_CustomerOwnedIpv4Pool (1.99s)
--- SKIP: TestAccAWSSubnet_MapCustomerOwnedIpOnLaunch (0.81s)
--- SKIP: TestAccAWSSubnet_outpost (2.42s)

--- FAIL: TestAccAWSVpc_classiclinkDnsSupportOptionSet (34.06s) # https://github.com/hashicorp/terraform-provider-aws/issues/17460
--- FAIL: TestAccAWSVpc_classiclinkOptionSet (35.36s) # https://github.com/hashicorp/terraform-provider-aws/issues/17460
--- PASS: TestAccAWSVpc_AssignGeneratedIpv6CidrBlock (212.69s)
--- PASS: TestAccAWSVpc_basic (83.60s)
--- PASS: TestAccAWSVpc_bothDnsOptionsSet (99.42s)
--- PASS: TestAccAWSVpc_coreMismatchedDiffs (75.31s)
--- PASS: TestAccAWSVpc_defaultAndIgnoreTags (163.31s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_duplicateTag (6.40s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_nonOverlappingTag (175.66s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_overlappingTag (172.40s)
--- PASS: TestAccAWSVpc_defaultTags_providerOnly (146.26s)
--- PASS: TestAccAWSVpc_defaultTags_updateToProviderOnly (108.09s)
--- PASS: TestAccAWSVpc_defaultTags_updateToResourceOnly (115.28s)
--- PASS: TestAccAWSVpc_DisabledDnsSupport (100.36s)
--- PASS: TestAccAWSVpc_disappears (57.28s)
--- PASS: TestAccAWSVpc_ignoreTags (154.45s)
--- PASS: TestAccAWSVpc_tags (206.08s)
--- PASS: TestAccAWSVpc_Tenancy (214.24s)
--- PASS: TestAccAWSVpc_update (151.29s)

Co-authored-by: Brian Flad <bflad417@gmail.com>

ghost · 2021-03-18T23:51:37Z

This has been released in version 3.33.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 for triage. Thanks!

ghost · 2021-04-17T17:10:41Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

anGie44 force-pushed the f-resource-default-tags-poc branch 2 times, most recently from 750191d to 82242d3 Compare March 8, 2021 05:17

poc resource-level default tags support; vpc and subnet resources

7d85dac

anGie44 force-pushed the f-resource-default-tags-poc branch from 82242d3 to 7d85dac Compare March 8, 2021 05:20

anGie44 mentioned this pull request Mar 8, 2021

[Proof of Concept] provider: default tags implementation #17941

Closed

anGie44 marked this pull request as ready for review March 8, 2021 05:27

anGie44 requested a review from a team as a code owner March 8, 2021 05:27

anGie44 added the enhancement Requests to existing resources that expand the functionality or scope. label Mar 8, 2021

bflad self-assigned this Mar 15, 2021

anGie44 changed the title ~~[Proof of Concept] aws_subnet/aws_vpc: Default tags implementation~~ [Proof of Concept] Default tags implementation: provider and subnet/vpc resources Mar 15, 2021

update per CR comments in #17941

1beda81

anGie44 commented Mar 15, 2021

View reviewed changes

anGie44 added the proposal Proposes new design or functionality. label Mar 15, 2021

bflad reviewed Mar 16, 2021

View reviewed changes

bflad approved these changes Mar 16, 2021

View reviewed changes

anGie44 and others added 2 commits March 17, 2021 11:14

Update .changelog/17974.txt entry syntax

a4386d2

Co-authored-by: Brian Flad <bflad417@gmail.com>

Update .changelog/17974.txt entry syntax

6560fef

Co-authored-by: Brian Flad <bflad417@gmail.com>

anGie44 added this to the v3.33.0 milestone Mar 18, 2021

anGie44 merged commit f77fe16 into main Mar 18, 2021

anGie44 deleted the f-resource-default-tags-poc branch March 18, 2021 14:25

github-actions bot pushed a commit that referenced this pull request Mar 18, 2021

Update CHANGELOG.md for #17974

9a2a1d1

kennyche mentioned this pull request Mar 18, 2021

aws_route53_vpc_association_authorization - is this working as intended? #18176

Closed

antonbabenko mentioned this pull request Apr 9, 2021

Add support for default_tags in provider terraform-aws-modules/terraform-aws-vpc#625

Closed

ghost locked as resolved and limited conversation to collaborators Apr 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proof of Concept] Default tags implementation: provider and subnet/vpc resources #17974

[Proof of Concept] Default tags implementation: provider and subnet/vpc resources #17974

anGie44 commented Mar 8, 2021 •

edited

Loading

bflad commented Mar 15, 2021

anGie44 Mar 15, 2021 •

edited

Loading

bflad Mar 16, 2021

bflad left a comment

bflad Mar 16, 2021

bflad Mar 16, 2021

anGie44 Mar 17, 2021

bflad Mar 16, 2021

bflad Mar 16, 2021

bflad left a comment

ghost commented Mar 18, 2021

ghost commented Apr 17, 2021

[Proof of Concept] Default tags implementation: provider and subnet/vpc resources #17974

[Proof of Concept] Default tags implementation: provider and subnet/vpc resources #17974

Conversation

anGie44 commented Mar 8, 2021 • edited Loading

Community Note

bflad commented Mar 15, 2021

anGie44 Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

bflad Mar 16, 2021

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad Mar 16, 2021

Choose a reason for hiding this comment

bflad Mar 16, 2021

Choose a reason for hiding this comment

anGie44 Mar 17, 2021

Choose a reason for hiding this comment

bflad Mar 16, 2021

Choose a reason for hiding this comment

bflad Mar 16, 2021

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

ghost commented Mar 18, 2021

ghost commented Apr 17, 2021

anGie44 commented Mar 8, 2021 •

edited

Loading

anGie44 Mar 15, 2021 •

edited

Loading