From ede92cea76edfa758976ec1bcc9ba48e08bde0c9 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Tue, 6 Jun 2023 10:47:09 -0400 Subject: [PATCH 1/6] add ability to identify source of incoming tag --- internal/tags/key_value_tags.go | 75 +++++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/internal/tags/key_value_tags.go b/internal/tags/key_value_tags.go index dfb8b5de6c4..095c74eba0d 100644 --- a/internal/tags/key_value_tags.go +++ b/internal/tags/key_value_tags.go @@ -749,27 +749,37 @@ type schemaResourceData interface { GetRawState() cty.Value } +// tagSource is an enum that identifiers the source of the tag +type tagSource int + +const ( + configuration tagSource = iota + plan + state +) + +// configTag contains the value and source of the incoming tag +type configTag struct { + value string + source tagSource +} + +// ResolveDuplicates resolves differences between incoming tags, defaultTags, and ignoreConfig func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *DefaultConfig, ignoreConfig *IgnoreConfig, d schemaResourceData) KeyValueTags { // remove default config. t := tags.RemoveDefaultConfig(defaultConfig) + cf := d.GetRawConfig() + configExists := !cf.IsNull() && cf.IsKnown() + result := make(map[string]string) for k, v := range t { result[k] = v.ValueString() } - configTags := make(map[string]string) - if config := d.GetRawPlan(); !config.IsNull() && config.IsKnown() { - c := config.GetAttr("tags") - if !c.IsNull() && c.IsKnown() { - for k, v := range c.AsValueMap() { - configTags[k] = v.AsString() - } - } - } - - if config := d.GetRawConfig(); !config.IsNull() && config.IsKnown() { - c := config.GetAttr("tags") + configTags := make(map[string]configTag) + if configExists { + c := cf.GetAttr("tags") // if the config is null just return the incoming tags // no duplicates to calculate @@ -782,19 +792,39 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D if _, ok := configTags[k]; !ok { // config tags can be null values. Ignore. if !v.IsNull() { - configTags[k] = v.AsString() + configTags[k] = configTag{ + value: v.AsString(), + source: configuration, + } + } + } + } + } + } + + if config := d.GetRawPlan(); !config.IsNull() && config.IsKnown() { + c := config.GetAttr("tags") + if !c.IsNull() && c.IsKnown() { + for k, v := range c.AsValueMap() { + if _, ok := configTags[k]; !ok { + configTags[k] = configTag{ + value: v.AsString(), + source: plan, } } } } } - if state := d.GetRawState(); !state.IsNull() && state.IsKnown() { - c := state.GetAttr("tags") + if st := d.GetRawState(); !st.IsNull() && st.IsKnown() { + c := st.GetAttr("tags") if !c.IsNull() { for k, v := range c.AsValueMap() { if _, ok := configTags[k]; !ok { - configTags[k] = v.AsString() + configTags[k] = configTag{ + value: v.AsString(), + source: state, + } } } } @@ -803,8 +833,16 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D for k, v := range configTags { if _, ok := result[k]; !ok { if defaultConfig != nil { - if val, ok := defaultConfig.Tags[k]; ok && val.ValueString() == v { - result[k] = v + if val, ok := defaultConfig.Tags[k]; ok && val.ValueString() == v.value { + // config does not exist during a refresh. + // set duplicate values from other sources for refresh diff calculation + if !configExists { + result[k] = v.value + } else { + if v.source == configuration { + result[k] = v.value + } + } } } } @@ -813,6 +851,7 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D return New(ctx, result).IgnoreConfig(ignoreConfig) } +// ResolveDuplicatesFramework resolves differences between incoming tags, defaultTags, and ignoreConfig func (tags KeyValueTags) ResolveDuplicatesFramework(ctx context.Context, defaultConfig *DefaultConfig, ignoreConfig *IgnoreConfig, resp *resource.ReadResponse, diags fwdiag.Diagnostics) KeyValueTags { // remove default config. t := tags.RemoveDefaultConfig(defaultConfig) From 7c7d61786077ada65a9a53b7bf110d4bf43d8c6a Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Tue, 6 Jun 2023 10:47:53 -0400 Subject: [PATCH 2/6] use tags consts --- internal/tags/key_value_tags.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/tags/key_value_tags.go b/internal/tags/key_value_tags.go index 095c74eba0d..48dc4d11b9a 100644 --- a/internal/tags/key_value_tags.go +++ b/internal/tags/key_value_tags.go @@ -779,7 +779,7 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D configTags := make(map[string]configTag) if configExists { - c := cf.GetAttr("tags") + c := cf.GetAttr(names.AttrTags) // if the config is null just return the incoming tags // no duplicates to calculate @@ -803,7 +803,7 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D } if config := d.GetRawPlan(); !config.IsNull() && config.IsKnown() { - c := config.GetAttr("tags") + c := config.GetAttr(names.AttrTags) if !c.IsNull() && c.IsKnown() { for k, v := range c.AsValueMap() { if _, ok := configTags[k]; !ok { @@ -817,7 +817,7 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D } if st := d.GetRawState(); !st.IsNull() && st.IsKnown() { - c := st.GetAttr("tags") + c := st.GetAttr(names.AttrTags) if !c.IsNull() { for k, v := range c.AsValueMap() { if _, ok := configTags[k]; !ok { From fbe9916b98e726a9745c91b012cdf964941e2074 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Tue, 6 Jun 2023 11:05:59 -0400 Subject: [PATCH 3/6] refactor: normalizeTagsFromRaw() --- internal/tags/key_value_tags.go | 47 +++++++++++++-------------------- 1 file changed, 18 insertions(+), 29 deletions(-) diff --git a/internal/tags/key_value_tags.go b/internal/tags/key_value_tags.go index 48dc4d11b9a..372e5e70613 100644 --- a/internal/tags/key_value_tags.go +++ b/internal/tags/key_value_tags.go @@ -788,45 +788,21 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D } if !c.IsNull() && c.IsKnown() { - for k, v := range c.AsValueMap() { - if _, ok := configTags[k]; !ok { - // config tags can be null values. Ignore. - if !v.IsNull() { - configTags[k] = configTag{ - value: v.AsString(), - source: configuration, - } - } - } - } + normalizeTagsFromRaw(c.AsValueMap(), configTags, configuration) } } - if config := d.GetRawPlan(); !config.IsNull() && config.IsKnown() { - c := config.GetAttr(names.AttrTags) + if pl := d.GetRawPlan(); !pl.IsNull() && pl.IsKnown() { + c := pl.GetAttr(names.AttrTags) if !c.IsNull() && c.IsKnown() { - for k, v := range c.AsValueMap() { - if _, ok := configTags[k]; !ok { - configTags[k] = configTag{ - value: v.AsString(), - source: plan, - } - } - } + normalizeTagsFromRaw(c.AsValueMap(), configTags, plan) } } if st := d.GetRawState(); !st.IsNull() && st.IsKnown() { c := st.GetAttr(names.AttrTags) if !c.IsNull() { - for k, v := range c.AsValueMap() { - if _, ok := configTags[k]; !ok { - configTags[k] = configTag{ - value: v.AsString(), - source: state, - } - } - } + normalizeTagsFromRaw(c.AsValueMap(), configTags, state) } } @@ -899,3 +875,16 @@ func ToSnakeCase(str string) string { result = regexp.MustCompile("([a-z0-9])([A-Z])").ReplaceAllString(result, "${1}_${2}") return strings.ToLower(result) } + +func normalizeTagsFromRaw(m map[string]cty.Value, incoming map[string]configTag, source tagSource) { + for k, v := range m { + if !v.IsNull() { + if _, ok := incoming[k]; !ok { + incoming[k] = configTag{ + value: v.AsString(), + source: source, + } + } + } + } +} From 21799f74db12d1360f49ae571eebe0c5b35d18f3 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Tue, 6 Jun 2023 11:37:36 -0400 Subject: [PATCH 4/6] add test to checking moving dupliate tags --- internal/service/ec2/vpc_test.go | 46 ++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/internal/service/ec2/vpc_test.go b/internal/service/ec2/vpc_test.go index 571eedf1356..d5258670e23 100644 --- a/internal/service/ec2/vpc_test.go +++ b/internal/service/ec2/vpc_test.go @@ -523,6 +523,52 @@ func TestAccVPC_DefaultTagsProviderAndResource_duplicateTag(t *testing.T) { }) } +func TestAccVPC_DefaultTagsProviderAndResource_moveDuplicateTags(t *testing.T) { + ctx := acctest.Context(t) + var vpc ec2.Vpc + resourceName := "aws_vpc.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, ec2.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: nil, + Steps: []resource.TestStep{ + { + Config: acctest.ConfigCompose( + testAccVPCConfig_tags1("overlapkey", "overlapvalue"), + ), + Check: resource.ComposeTestCheckFunc( + acctest.CheckVPCExists(ctx, resourceName, &vpc), + resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + ), + }, + { + Config: acctest.ConfigCompose( + testAccVPCConfig_basic, + acctest.ConfigDefaultTags_Tags1("overlapkey", "overlapvalue"), + ), + Check: resource.ComposeTestCheckFunc( + acctest.CheckVPCExists(ctx, resourceName, &vpc), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + ), + }, + { + Config: acctest.ConfigCompose( + testAccVPCConfig_tags1("overlapkey", "overlapvalue"), + ), + Check: resource.ComposeTestCheckFunc( + acctest.CheckVPCExists(ctx, resourceName, &vpc), + resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + ), + }, + }, + }) +} + // TestAccVPC_DynamicResourceTagsMergedWithLocals_ignoreChanges ensures computed "tags_all" // attributes are correctly determined when the provider-level default_tags block // is left unused and resource tags (merged with local.tags) are only known at apply time, From 86c04b5b31736d374bec6ac98eea833499af8fae Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Wed, 7 Jun 2023 12:37:24 -0400 Subject: [PATCH 5/6] add CHANGELOG entry --- .changelog/31794.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/31794.txt diff --git a/.changelog/31794.txt b/.changelog/31794.txt new file mode 100644 index 00000000000..2cb06c5aa43 --- /dev/null +++ b/.changelog/31794.txt @@ -0,0 +1,3 @@ +```release-note:bug +provider/default_tags: Fix perpetual diff when identical tags are moved from `default_tags` to resource `tags`, and vice versa +``` \ No newline at end of file From 3f98ce47ec7a9b3046a87dbb40f357aa3921d849 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Wed, 7 Jun 2023 12:48:40 -0400 Subject: [PATCH 6/6] correct CHANGELOG entry --- .changelog/{31794.txt => 31826.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{31794.txt => 31826.txt} (100%) diff --git a/.changelog/31794.txt b/.changelog/31826.txt similarity index 100% rename from .changelog/31794.txt rename to .changelog/31826.txt