Merge pull request #20339 from hashicorp/b-elasticache-user-update

r/elasticache_user: wait for modifying/active state to avoid inconsistent result

.changelog/20339.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+aws/resource_aws_elasticache_user: Correctly handle user modifications and deletion
+```

aws/internal/service/elasticache/finder/finder.go

@@ -196,11 +196,6 @@ func ElastiCacheUserById(conn *elasticache.ElastiCache, userID string) (*elastic
 			Message: "empty result",
 		}
 	case 1:
-		if aws.StringValue(out.Users[0].Status) != "active" {
-			return nil, &resource.NotFoundError{
-				Message: "empty result",
-			}
-		}
 		return out.Users[0], nil
 	default:
 		return nil, &resource.NotFoundError{

aws/internal/service/elasticache/waiter/status.go

@@ -15,6 +15,10 @@ const (
 	ReplicationGroupStatusDeleting     = "deleting"
 	ReplicationGroupStatusCreateFailed = "create-failed"
 	ReplicationGroupStatusSnapshotting = "snapshotting"
+
+	UserStatusActive    = "active"
+	UserStatusDeleting  = "deleting"
+	UserStatusModifying = "modifying"
 )
 
 // ReplicationGroupStatus fetches the Replication Group and its Status
@@ -125,3 +129,20 @@ func GlobalReplicationGroupMemberStatus(conn *elasticache.ElastiCache, globalRep
 		return member, aws.StringValue(member.Status), nil
 	}
 }
+
+// UserStatus fetches the ElastiCache user and its Status
+func UserStatus(conn *elasticache.ElastiCache, userId string) resource.StateRefreshFunc {
+	return func() (interface{}, string, error) {
+		user, err := finder.ElastiCacheUserById(conn, userId)
+
+		if tfresource.NotFound(err) {
+			return nil, "", nil
+		}
+
+		if err != nil {
+			return nil, "", err
+		}
+
+		return user, aws.StringValue(user.Status), nil
+	}
+}

aws/internal/service/elasticache/waiter/waiter.go

@@ -17,6 +17,9 @@ const (
 
 	replicationGroupDeletedMinTimeout = 10 * time.Second
 	replicationGroupDeletedDelay      = 30 * time.Second
+
+	UserActiveTimeout  = 5 * time.Minute
+	UserDeletedTimeout = 5 * time.Minute
 )
 
 // ReplicationGroupAvailable waits for a ReplicationGroup to return Available
@@ -230,3 +233,31 @@ func GlobalReplicationGroupMemberDetached(conn *elasticache.ElastiCache, globalR
 	}
 	return nil, err
 }
+
+// UserActive waits for an ElastiCache user to reach an active state after modifications
+func UserActive(conn *elasticache.ElastiCache, userId string) error {
+	stateConf := &resource.StateChangeConf{
+		Pending: []string{UserStatusModifying},
+		Target:  []string{UserStatusActive},
+		Refresh: UserStatus(conn, userId),
+		Timeout: UserActiveTimeout,
+	}
+
+	_, err := stateConf.WaitForState()
+
+	return err
+}
+
+// UserDeleted waits for an ElastiCache user to be deleted
+func UserDeleted(conn *elasticache.ElastiCache, userId string) error {
+	stateConf := &resource.StateChangeConf{
+		Pending: []string{UserStatusDeleting},
+		Target:  []string{},
+		Refresh: UserStatus(conn, userId),
+		Timeout: UserDeletedTimeout,
+	}
+
+	_, err := stateConf.WaitForState()
+
+	return err
+}

aws/resource_aws_elasticache_user.go

@@ -13,6 +13,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
 	"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/elasticache/finder"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/elasticache/waiter"
 	"github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource"
 )
 
@@ -112,8 +113,8 @@ func resourceAwsElasticacheUserRead(d *schema.ResourceData, meta interface{}) er
 
 	resp, err := finder.ElastiCacheUserById(conn, d.Id())
 	if !d.IsNewResource() && (tfresource.NotFound(err) || isAWSErr(err, elasticache.ErrCodeUserNotFoundFault, "")) {
+		log.Printf("[WARN] ElastiCache User (%s) not found, removing from state", d.Id())
 		d.SetId("")
-		log.Printf("[DEBUG] ElastiCache User (%s) not found", d.Id())
 		return nil
 	}
 
@@ -159,7 +160,7 @@ func resourceAwsElasticacheUserUpdate(d *schema.ResourceData, meta interface{})
 
 	if d.HasChangeExcept("tags_all") {
 		req := &elasticache.ModifyUserInput{
-			UserId: aws.String(d.Get("user_id").(string)),
+			UserId: aws.String(d.Id()),
 		}
 
 		if d.HasChange("access_string") {
@@ -180,7 +181,11 @@ func resourceAwsElasticacheUserUpdate(d *schema.ResourceData, meta interface{})
 		if hasChange {
 			_, err := conn.ModifyUser(req)
 			if err != nil {
-				return fmt.Errorf("error updating ElastiCache User (%q): %w", d.Id(), err)
+				return fmt.Errorf("error updating ElastiCache User (%s): %w", d.Id(), err)
+			}
+
+			if err := waiter.UserActive(conn, d.Id()); err != nil {
+				return fmt.Errorf("error waiting for ElastiCache User (%s) to be modified: %w", d.Id(), err)
 			}
 		}
 
@@ -206,11 +211,20 @@ func resourceAwsElasticacheUserDelete(d *schema.ResourceData, meta interface{})
 	}
 
 	_, err := conn.DeleteUser(input)
+
+	if tfawserr.ErrCodeEquals(err, elasticache.ErrCodeUserNotFoundFault) {
+		return nil
+	}
+
 	if err != nil {
+		return fmt.Errorf("error deleting ElastiCache User (%s): %w", d.Id(), err)
+	}
+
+	if err := waiter.UserDeleted(conn, d.Id()); err != nil {
 		if tfawserr.ErrCodeEquals(err, elasticache.ErrCodeUserNotFoundFault) {
 			return nil
 		}
-		return fmt.Errorf("ElastiCache User cannot be deleted: %w", err)
+		return fmt.Errorf("error waiting for ElastiCache User (%s) to be deleted: %w", d.Id(), err)
 	}
 
 	return nil

aws/resource_aws_elasticache_user_test.go

@@ -1,11 +1,11 @@
 package aws
 
 import (
-	//"errors"
 	"fmt"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/service/elasticache"
+	"github.com/hashicorp/aws-sdk-go-base/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -48,6 +48,43 @@ func TestAccAWSElasticacheUser_basic(t *testing.T) {
 	})
 }
 
+func TestAccAWSElasticacheUser_update(t *testing.T) {
+	var user elasticache.User
+	rName := acctest.RandomWithPrefix("tf-acc")
+	resourceName := "aws_elasticache_user.test"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		ErrorCheck:   testAccErrorCheck(t, elasticache.EndpointsID),
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSElasticacheUserDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccAWSElasticacheUserConfigBasic(rName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSElasticacheUserExists(resourceName, &user),
+				),
+			},
+			{
+				Config: testAccAWSElasticacheUserConfigUpdate(rName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSElasticacheUserExists(resourceName, &user),
+					resource.TestCheckResourceAttr(resourceName, "access_string", "on ~* +@all"),
+				),
+			},
+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"no_password_required",
+					"passwords",
+				},
+			},
+		},
+	})
+}
+
 func TestAccAWSElasticacheUser_tags(t *testing.T) {
 	var user elasticache.User
 	rName := acctest.RandomWithPrefix("tf-acc")
@@ -133,14 +170,19 @@ func testAccCheckAWSElasticacheUserDestroyWithProvider(s *terraform.State, provi
 			continue
 		}
 
-		_, err := finder.ElastiCacheUserById(conn, rs.Primary.ID)
+		user, err := finder.ElastiCacheUserById(conn, rs.Primary.ID)
+
+		if tfawserr.ErrCodeEquals(err, elasticache.ErrCodeUserNotFoundFault) || tfresource.NotFound(err) {
+			continue
+		}
+
 		if err != nil {
-			if tfresource.NotFound(err) {
-				return nil
-			}
+			return err
 		}
 
-		return err
+		if user != nil {
+			return fmt.Errorf("Elasticache User (%s) still exists", rs.Primary.ID)
+		}
 	}
 
 	return nil
@@ -186,6 +228,18 @@ resource "aws_elasticache_user" "test" {
 `, rName))
 }
 
+func testAccAWSElasticacheUserConfigUpdate(rName string) string {
+	return composeConfig(testAccAvailableAZsNoOptInConfig(), fmt.Sprintf(`
+resource "aws_elasticache_user" "test" {
+  user_id       = %[1]q
+  user_name     = "username1"
+  access_string = "on ~* +@all"
+  engine        = "REDIS"
+  passwords     = ["password123456789"]
+}
+`, rName))
+}
+
 func testAccAWSElasticacheUserConfigTags(rName, tagKey, tagValue string) string {
 	return composeConfig(testAccAvailableAZsNoOptInConfig(), fmt.Sprintf(`
 resource "aws_elasticache_user" "test" {

website/docs/r/elasticache_user.html.markdown

@@ -26,7 +26,7 @@ resource "aws_elasticache_user" "test" {
 
 The following arguments are required:
 
-* `access_string` - (Required) Access permissions string used for this user.
+* `access_string` - (Required) Access permissions string used for this user. See [Specifying Permissions Using an Access String](https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/Clusters.RBAC.html#Access-string) for more details.
 * `engine` - (Required) The current supported value is `REDIS`.
 * `user_id` - (Required) The ID of the user.
 * `user_name` - (Required) The username of the user.