Merge pull request #41253 from hashicorp/td-framework-wrapper-tags-cu…

…stomizediff

Tech debt: Remove transparent tagging-only `CustomizeDiff` implementations for Plugin SDK resources

.ci/semgrep/pluginsdk/customdiff.yml

@@ -0,0 +1,24 @@
+rules:
+  - id: simplify-customizediff-all-single
+    languages: [go]
+    message: Simplify CustomizeDiff All
+    paths:
+      include:
+        - "internal/service/*/*.go"
+      exclude:
+        - "internal/service/*/*_test.go"
+    patterns:
+      - pattern-regex: CustomizeDiff:\s+customdiff\.All\(\s*[a-zA-Z0-9]+,?\s*\)
+    severity: WARNING
+
+  - id: simplify-customizediff-sequence-single
+    languages: [go]
+    message: Simplify CustomizeDiff Sequence
+    paths:
+      include:
+        - "internal/service/*/*.go"
+      exclude:
+        - "internal/service/*/*_test.go"
+    patterns:
+      - pattern-regex: CustomizeDiff:\s+customdiff\.Sequence\(\s*[a-zA-Z0-9]+,?\s*\)
+    severity: WARNING

docs/resource-tagging.md

@@ -163,22 +163,9 @@ The `tags_all` attribute contains a union of the tags set directly on the resour
     }
     ```
 
-Add a `CustomizeDiff` function (Terraform Plugin SDK V2) to ensure tagging diffs are handled appropriately.
-This function handles the combination of tags set on the resource and default tags, and must be set for tagging to function properly.
-
-=== "Terraform Plugin SDK V2"
-    ```go
-    func ResourceExample() *schema.Resource {
-      return &schema.Resource{
-        /* ... other configuration ... */
-        CustomizeDiff: verify.SetTagsDiff,
-      }
-    }
-    ```
-
 ### Transparent Tagging
 
-Most services can use a facility we call _transparent_ (or _implicit_) _tagging_, where the majority of resource tagging functionality is implemented using code located in the provider's runtime packages (see `internal/provider/intercept.go` and `internal/provider/fwprovider/intercept.go` for details) and not in the resource's CRUD handler functions. Resource implementers opt-in to transparent tagging by adding an _annotation_ (a specially formatted Go comment) to the resource's factory function (similar to the [resource self-registration mechanism](add-a-new-resource.md)).
+All service that support tagging use a facility we call _transparent_ (or _implicit_) _tagging_, where the majority of resource tagging functionality is implemented using code located in the provider's runtime packages (see `internal/provider/intercept.go` and `internal/provider/fwprovider/intercept.go` for details) and not in the resource's CRUD handler functions. Resource implementers opt-in to transparent tagging by adding an _annotation_ (a specially formatted Go comment) to the resource's factory function (similar to the [resource self-registration mechanism](add-a-new-resource.md)).
 
 === "Terraform Plugin Framework (Preferred)"
     ```go
@@ -284,7 +271,7 @@ In the resource `Update` operation, only non-`tags` updates need to be done as t
 
 === "Terraform Plugin SDK V2"
     ```go
-    if d.HasChangesExcept("tags", "tags_all") {
+    if d.HasChangesExcept(names.AttrTags, names.AttrTagsAll) {
       ...
     }
     ```

internal/provider/provider.go

@@ -402,8 +402,9 @@ func New(ctx context.Context) (*schema.Provider, error) {
 
 					return ctx
 				},
-				interceptors: interceptors,
-				typeName:     typeName,
+				interceptors:           interceptors,
+				typeName:               typeName,
+				usesTransparentTagging: v.Tags != nil,
 			}
 			wrapResource(r, opts)
 			provider.ResourcesMap[typeName] = r

internal/provider/wrap.go

@@ -5,8 +5,13 @@ package provider
 
 import (
 	"context"
+	"fmt"
 
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/hashicorp/terraform-provider-aws/internal/conns"
+	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
+	"github.com/hashicorp/terraform-provider-aws/names"
 )
 
 // contextFunc augments Context.
@@ -40,9 +45,10 @@ func (w *wrappedDataSource) read(f schema.ReadContextFunc) schema.ReadContextFun
 
 type wrappedResourceOptions struct {
 	// bootstrapContext is run on all wrapped methods before any interceptors.
-	bootstrapContext contextFunc
-	interceptors     interceptorItems
-	typeName         string
+	bootstrapContext       contextFunc
+	interceptors           interceptorItems
+	typeName               string
+	usesTransparentTagging bool
 }
 
 // wrappedResource represents an interceptor dispatcher for a Plugin SDK v2 resource.
@@ -55,66 +61,146 @@ func wrapResource(r *schema.Resource, opts wrappedResourceOptions) {
 		opts: opts,
 	}
 
-	if v := r.CreateWithoutTimeout; v != nil {
-		r.CreateWithoutTimeout = w.create(v)
-	}
-	if v := r.ReadWithoutTimeout; v != nil {
-		r.ReadWithoutTimeout = w.read(v)
-	}
-	if v := r.UpdateWithoutTimeout; v != nil {
-		r.UpdateWithoutTimeout = w.update(v)
-	}
-	if v := r.DeleteWithoutTimeout; v != nil {
-		r.DeleteWithoutTimeout = w.delete(v)
-	}
+	r.CreateWithoutTimeout = w.create(r.CreateWithoutTimeout)
+	r.ReadWithoutTimeout = w.read(r.ReadWithoutTimeout)
+	r.UpdateWithoutTimeout = w.update(r.UpdateWithoutTimeout)
+	r.DeleteWithoutTimeout = w.delete(r.DeleteWithoutTimeout)
 	if v := r.Importer; v != nil {
-		if v := v.StateContext; v != nil {
-			r.Importer.StateContext = w.state(v)
-		}
-	}
-	if v := r.CustomizeDiff; v != nil {
-		r.CustomizeDiff = w.customizeDiff(v)
+		r.Importer.StateContext = w.state(v.StateContext)
 	}
+	r.CustomizeDiff = w.customizeDiff(r.CustomizeDiff)
 	for i, v := range r.StateUpgraders {
-		if v := v.Upgrade; v != nil {
-			r.StateUpgraders[i].Upgrade = w.stateUpgrade(v)
-		}
+		r.StateUpgraders[i].Upgrade = w.stateUpgrade(v.Upgrade)
 	}
 }
 
 func (w *wrappedResource) create(f schema.CreateContextFunc) schema.CreateContextFunc {
+	if f == nil {
+		return nil
+	}
+
 	return interceptedHandler(w.opts.bootstrapContext, w.opts.interceptors, f, Create)
 }
 
 func (w *wrappedResource) read(f schema.ReadContextFunc) schema.ReadContextFunc {
+	if f == nil {
+		return nil
+	}
+
 	return interceptedHandler(w.opts.bootstrapContext, w.opts.interceptors, f, Read)
 }
 
 func (w *wrappedResource) update(f schema.UpdateContextFunc) schema.UpdateContextFunc {
+	if f == nil {
+		return nil
+	}
+
 	return interceptedHandler(w.opts.bootstrapContext, w.opts.interceptors, f, Update)
 }
 
 func (w *wrappedResource) delete(f schema.DeleteContextFunc) schema.DeleteContextFunc {
+	if f == nil {
+		return nil
+	}
+
 	return interceptedHandler(w.opts.bootstrapContext, w.opts.interceptors, f, Delete)
 }
 
 func (w *wrappedResource) state(f schema.StateContextFunc) schema.StateContextFunc {
+	if f == nil {
+		return nil
+	}
+
 	return func(ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) {
 		ctx = w.opts.bootstrapContext(ctx, meta)
 		return f(ctx, d, meta)
 	}
 }
 
 func (w *wrappedResource) customizeDiff(f schema.CustomizeDiffFunc) schema.CustomizeDiffFunc {
-	return func(ctx context.Context, d *schema.ResourceDiff, meta any) error {
+	if w.opts.usesTransparentTagging {
+		if f == nil {
+			return w.customizeDiffWithBootstrappedContext(setTagsAll)
+		} else {
+			return w.customizeDiffWithBootstrappedContext(customdiff.Sequence(setTagsAll, f))
+		}
+	}
+
+	if f == nil {
+		return nil
+	}
+
+	return w.customizeDiffWithBootstrappedContext(f)
+}
+
+func (w *wrappedResource) customizeDiffWithBootstrappedContext(f schema.CustomizeDiffFunc) schema.CustomizeDiffFunc {
+	return func(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error {
 		ctx = w.opts.bootstrapContext(ctx, meta)
 		return f(ctx, d, meta)
 	}
 }
 
 func (w *wrappedResource) stateUpgrade(f schema.StateUpgradeFunc) schema.StateUpgradeFunc {
+	if f == nil {
+		return nil
+	}
+
 	return func(ctx context.Context, rawState map[string]interface{}, meta any) (map[string]interface{}, error) {
 		ctx = w.opts.bootstrapContext(ctx, meta)
 		return f(ctx, rawState, meta)
 	}
 }
+
+// setTagsAll is a CustomizeDiff function that calculates the new value for the `tags_all` attribute.
+func setTagsAll(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error {
+	c := meta.(*conns.AWSClient)
+
+	if !d.GetRawPlan().GetAttr(names.AttrTags).IsWhollyKnown() {
+		if err := d.SetNewComputed(names.AttrTagsAll); err != nil {
+			return fmt.Errorf("setting tags_all to Computed: %w", err)
+		}
+		return nil
+	}
+
+	newTags := tftags.New(ctx, d.Get(names.AttrTags).(map[string]interface{}))
+	allTags := c.DefaultTagsConfig(ctx).MergeTags(newTags).IgnoreConfig(c.IgnoreTagsConfig(ctx))
+	if d.HasChange(names.AttrTags) {
+		if newTags.HasZeroValue() {
+			if err := d.SetNewComputed(names.AttrTagsAll); err != nil {
+				return fmt.Errorf("setting tags_all to Computed: %w", err)
+			}
+		}
+
+		if len(allTags) > 0 && (!newTags.HasZeroValue() || !allTags.HasZeroValue()) {
+			if err := d.SetNew(names.AttrTagsAll, allTags.Map()); err != nil {
+				return fmt.Errorf("setting new tags_all diff: %w", err)
+			}
+		}
+
+		if len(allTags) == 0 {
+			if err := d.SetNew(names.AttrTagsAll, allTags.Map()); err != nil {
+				return fmt.Errorf("setting new tags_all diff: %w", err)
+			}
+		}
+	} else {
+		if len(allTags) > 0 && !allTags.HasZeroValue() {
+			if err := d.SetNew(names.AttrTagsAll, allTags.Map()); err != nil {
+				return fmt.Errorf("setting new tags_all diff: %w", err)
+			}
+			return nil
+		}
+
+		var newTagsAll tftags.KeyValueTags
+		if v, ok := d.Get(names.AttrTagsAll).(map[string]interface{}); ok {
+			newTagsAll = tftags.New(ctx, v)
+		}
+		if len(allTags) > 0 && !newTagsAll.DeepEqual(allTags) && allTags.HasZeroValue() {
+			if err := d.SetNewComputed(names.AttrTagsAll); err != nil {
+				return fmt.Errorf("setting tags_all to Computed: %w", err)
+			}
+			return nil
+		}
+	}
+
+	return nil
+}

internal/sdkv2/suppress.go

@@ -5,6 +5,7 @@ package sdkv2
 
 import (
 	"strings"
+	"time"
 
 	awspolicy "github.com/hashicorp/awspolicyequivalence"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -23,6 +24,20 @@ func SuppressEquivalentJSONDocuments(k, old, new string, d *schema.ResourceData)
 	return json.EqualStrings(old, new)
 }
 
+// SuppressEquivalentRoundedTime returns a difference suppression function that compares
+// two time value with the specified layout rounded to the specified duration.
+func SuppressEquivalentRoundedTime(layout string, d time.Duration) schema.SchemaDiffSuppressFunc {
+	return func(k, old, new string, _ *schema.ResourceData) bool {
+		if old, err := time.Parse(layout, old); err == nil {
+			if new, err := time.Parse(layout, new); err == nil {
+				return old.Round(d).Equal(new.Round(d))
+			}
+		}
+
+		return false
+	}
+}
+
 // SuppressEquivalentIAMPolicyDocuments provides custom difference suppression
 // for IAM policy documents in the given strings that are equivalent.
 func SuppressEquivalentIAMPolicyDocuments(k, old, new string, d *schema.ResourceData) bool {

internal/verify/diff_test.go → internal/sdkv2/suppress_test.go

@@ -1,7 +1,7 @@
 // Copyright (c) HashiCorp, Inc.
 // SPDX-License-Identifier: MPL-2.0
 
-package verify
+package sdkv2
 
 import (
 	"testing"

internal/service/accessanalyzer/analyzer.go

@@ -23,7 +23,6 @@ import (
 	"github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag"
 	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
 	"github.com/hashicorp/terraform-provider-aws/internal/tfresource"
-	"github.com/hashicorp/terraform-provider-aws/internal/verify"
 	"github.com/hashicorp/terraform-provider-aws/names"
 )
 
@@ -98,8 +97,6 @@ func resourceAnalyzer() *schema.Resource {
 				ValidateDiagFunc: enum.Validate[types.Type](),
 			},
 		},
-
-		CustomizeDiff: verify.SetTagsDiff,
 	}
 }
 

internal/service/acm/certificate.go

@@ -324,7 +324,6 @@ func resourceCertificate() *schema.Resource {
 
 				return nil
 			},
-			verify.SetTagsDiff,
 		),
 	}
 }

internal/service/acmpca/certificate_authority.go

@@ -331,8 +331,6 @@ func resourceCertificateAuthority() *schema.Resource {
 				ValidateDiagFunc: enum.Validate[types.CertificateAuthorityUsageMode](),
 			},
 		},
-
-		CustomizeDiff: verify.SetTagsDiff,
 	}
 }
 

internal/service/amp/workspace.go

@@ -46,7 +46,6 @@ func resourceWorkspace() *schema.Resource {
 			customdiff.ForceNewIfChange(names.AttrAlias, func(_ context.Context, old, new, meta interface{}) bool {
 				return old.(string) != "" && new.(string) == ""
 			}),
-			verify.SetTagsDiff,
 		),
 
 		Schema: map[string]*schema.Schema{

internal/service/amplify/app.go

@@ -42,7 +42,6 @@ func resourceApp() *schema.Resource {
 		},
 
 		CustomizeDiff: customdiff.Sequence(
-			verify.SetTagsDiff,
 			customdiff.ForceNewIfChange(names.AttrDescription, func(_ context.Context, old, new, meta interface{}) bool {
 				// Any existing value cannot be cleared.
 				return new.(string) == ""

internal/service/amplify/branch.go

@@ -42,8 +42,6 @@ func resourceBranch() *schema.Resource {
 			StateContext: schema.ImportStatePassthroughContext,
 		},
 
-		CustomizeDiff: verify.SetTagsDiff,
-
 		Schema: map[string]*schema.Schema{
 			"app_id": {
 				Type:     schema.TypeString,

internal/service/apigateway/api_key.go

@@ -21,7 +21,6 @@ import (
 	"github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag"
 	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
 	"github.com/hashicorp/terraform-provider-aws/internal/tfresource"
-	"github.com/hashicorp/terraform-provider-aws/internal/verify"
 	"github.com/hashicorp/terraform-provider-aws/names"
 )
 
@@ -81,8 +80,6 @@ func resourceAPIKey() *schema.Resource {
 				ValidateFunc: validation.StringLenBetween(20, 128),
 			},
 		},
-
-		CustomizeDiff: verify.SetTagsDiff,
 	}
 }
 

internal/service/apigateway/client_certificate.go

@@ -20,7 +20,6 @@ import (
 	"github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag"
 	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
 	"github.com/hashicorp/terraform-provider-aws/internal/tfresource"
-	"github.com/hashicorp/terraform-provider-aws/internal/verify"
 	"github.com/hashicorp/terraform-provider-aws/names"
 )
 
@@ -62,8 +61,6 @@ func resourceClientCertificate() *schema.Resource {
 			names.AttrTags:    tftags.TagsSchema(),
 			names.AttrTagsAll: tftags.TagsSchemaComputed(),
 		},
-
-		CustomizeDiff: verify.SetTagsDiff,
 	}
 }