From 908080bea0394c5d92c857494267af103292855e Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 27 Jul 2021 21:14:12 -0400 Subject: [PATCH 1/2] wait for modifying/active state to avoid inconsistent result --- .../service/elasticache/finder/finder.go | 5 -- .../service/elasticache/waiter/status.go | 21 ++++++ .../service/elasticache/waiter/waiter.go | 31 +++++++++ aws/resource_aws_elasticache_user.go | 22 +++++-- aws/resource_aws_elasticache_user_test.go | 66 +++++++++++++++++-- website/docs/r/elasticache_user.html.markdown | 2 +- 6 files changed, 131 insertions(+), 16 deletions(-) diff --git a/aws/internal/service/elasticache/finder/finder.go b/aws/internal/service/elasticache/finder/finder.go index d16478a2e69..fbf21abbc40 100644 --- a/aws/internal/service/elasticache/finder/finder.go +++ b/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{ diff --git a/aws/internal/service/elasticache/waiter/status.go b/aws/internal/service/elasticache/waiter/status.go index 9fb4fbd58dd..2199b185384 100644 --- a/aws/internal/service/elasticache/waiter/status.go +++ b/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 + } +} diff --git a/aws/internal/service/elasticache/waiter/waiter.go b/aws/internal/service/elasticache/waiter/waiter.go index d0af31d8afc..12a218e462d 100644 --- a/aws/internal/service/elasticache/waiter/waiter.go +++ b/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 +} diff --git a/aws/resource_aws_elasticache_user.go b/aws/resource_aws_elasticache_user.go index cd09f21fd29..20eb8139a96 100644 --- a/aws/resource_aws_elasticache_user.go +++ b/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 diff --git a/aws/resource_aws_elasticache_user_test.go b/aws/resource_aws_elasticache_user_test.go index f90b436f2dd..e7ca5feb545 100644 --- a/aws/resource_aws_elasticache_user_test.go +++ b/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" { diff --git a/website/docs/r/elasticache_user.html.markdown b/website/docs/r/elasticache_user.html.markdown index a344f326c90..4bbde40391f 100644 --- a/website/docs/r/elasticache_user.html.markdown +++ b/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. From 7f585f8bccea941fa219b95c2a2493c7a633ebb8 Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 27 Jul 2021 21:18:01 -0400 Subject: [PATCH 2/2] Add CHANGELOG for #20339 --- .changelog/20339.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/20339.txt diff --git a/.changelog/20339.txt b/.changelog/20339.txt new file mode 100644 index 00000000000..71ddbe49c98 --- /dev/null +++ b/.changelog/20339.txt @@ -0,0 +1,3 @@ +```release-note:bug +aws/resource_aws_elasticache_user: Correctly handle user modifications and deletion +``` \ No newline at end of file