Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r/user_pool_domain - remove from state when deleted + move waiters to their own package #14732

Merged
merged 4 commits into from
Aug 19, 2020
Merged

r/user_pool_domain - remove from state when deleted + move waiters to their own package #14732

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59a1476
move cognito waiter to their own package
DrFaust92 Aug 19, 2020
edf495f
lint
DrFaust92 Aug 19, 2020
4ea0ddb
lint
DrFaust92 Aug 19, 2020
31b2f01
remove min timout
DrFaust92 Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions aws/internal/service/cognitoidentityprovider/waiter/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package waiter

import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/cognitoidentityprovider"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
)

// UserPoolDomainStatus fetches the Operation and its Status
func UserPoolDomainStatus(conn *cognitoidentityprovider.CognitoIdentityProvider, domain string) resource.StateRefreshFunc {
return func() (interface{}, string, error) {
input := &cognitoidentityprovider.DescribeUserPoolDomainInput{
Domain: aws.String(domain),
}

output, err := conn.DescribeUserPoolDomain(input)

if err != nil {
return nil, "", err
Copy link
Contributor

@ewbankkit ewbankkit Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a pattern of

const (
	userPoolDomainStatusNotFound = "NotFound"
	userPoolDomainStatusUnknown  = "Unknown"
)

...

		if err != nil {
			return nil, userPoolDomainStatusUnknown, err
		}

		if output == nil {
			return nil, userPoolDomainStatusNotFound, nil
		}

Although these statuses are never explicitly tested, I think having a value other than "" helps with logging etc.
Having the constant names lowercase ensures they're not used outside this package.

}

if output == nil {
return nil, "", nil
}

return output, aws.StringValue(output.DomainDescription.Status), nil
}
}
58 changes: 58 additions & 0 deletions aws/internal/service/cognitoidentityprovider/waiter/waiter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package waiter

import (
"time"

"github.com/aws/aws-sdk-go/service/cognitoidentityprovider"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
)

const (
// Maximum amount of time to wait for an Operation to return Success
UserPoolDomainDeleteTimeout = 1 * time.Minute
UserPoolDomainCreateTimeout = 1 * time.Minute
)

// UserPoolDomainDeleted waits for an Operation to return Success
func UserPoolDomainDeleted(conn *cognitoidentityprovider.CognitoIdentityProvider, domain string) (*cognitoidentityprovider.DescribeUserPoolDomainOutput, error) {
stateConf := &resource.StateChangeConf{
Pending: []string{
cognitoidentityprovider.DomainStatusTypeUpdating,
cognitoidentityprovider.DomainStatusTypeDeleting,
},
Target: []string{""},
Copy link
Contributor

@ewbankkit ewbankkit Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "correct" way to do this is Target: []string{},:

https://github.com/terraform-providers/terraform-provider-aws/blob/691d0645ceed9453a8fc77a88e7fb0fce29b1ad3/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource/state.go#L122-L131

This code runs when the StateRefreshFunc returns nil, "anything", nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this failed for me

        Error: error waiting for User Pool Domain (tf-acc-test-domain-3112558782247577448) deletion: unexpected state '', wanted target ''. last error: %!s(<nil>)

Copy link
Contributor

@ewbankkit ewbankkit Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess the AWS API may be returning a non-null response but the status is "".
Target: []string{""}, must be required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what's happening here:

2020/08/19 10:14:59 [DEBUG] [aws-sdk-go] DEBUG: Response cognito-idp/DescribeUserPoolDomain Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 200 OK
Connection: close
Content-Length: 24
Content-Type: application/x-amz-json-1.1
Date: Wed, 19 Aug 2020 14:14:59 GMT
X-Amzn-Requestid: abe5941d-5d1e-4153-a926-84cfbfce7f91


-----------------------------------------------------
2020/08/19 10:14:59 [DEBUG] [aws-sdk-go] {"DomainDescription":{}}

Refresh: UserPoolDomainStatus(conn, domain),
Timeout: UserPoolDomainDeleteTimeout,
}

outputRaw, err := stateConf.WaitForState()

if output, ok := outputRaw.(*cognitoidentityprovider.DescribeUserPoolDomainOutput); ok {
return output, err
}

return nil, err
}

func UserPoolDomainCreated(conn *cognitoidentityprovider.CognitoIdentityProvider, domain string, timeout time.Duration) (*cognitoidentityprovider.DescribeUserPoolDomainOutput, error) {
stateConf := &resource.StateChangeConf{
Pending: []string{
cognitoidentityprovider.DomainStatusTypeCreating,
cognitoidentityprovider.DomainStatusTypeUpdating,
},
Target: []string{
cognitoidentityprovider.DomainStatusTypeActive,
},
Refresh: UserPoolDomainStatus(conn, domain),
MinTimeout: UserPoolDomainCreateTimeout,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In StateChangeConf the MinTimeout field represents the minimum amount of time between refreshes, which the constant naming doesn't follow. I would suggest renaming the constant and providing a comment or removing this field and seeing if the resource still works as expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed! tests are passing

Timeout: timeout,
}

outputRaw, err := stateConf.WaitForState()

if output, ok := outputRaw.(*cognitoidentityprovider.DescribeUserPoolDomainOutput); ok {
return output, err
}

return nil, err
}
77 changes: 20 additions & 57 deletions aws/resource_aws_cognito_user_pool_domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/cognitoidentityprovider"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/cognitoidentityprovider/waiter"
)

func resourceAwsCognitoUserPoolDomain() *schema.Resource {
Expand Down Expand Up @@ -83,37 +83,13 @@ func resourceAwsCognitoUserPoolDomainCreate(d *schema.ResourceData, meta interfa

_, err := conn.CreateUserPoolDomain(params)
if err != nil {
return fmt.Errorf("Error creating Cognito User Pool Domain: %s", err)
return fmt.Errorf("Error creating Cognito User Pool Domain: %w", err)
}

d.SetId(domain)

stateConf := resource.StateChangeConf{
Pending: []string{
cognitoidentityprovider.DomainStatusTypeCreating,
cognitoidentityprovider.DomainStatusTypeUpdating,
},
Target: []string{
cognitoidentityprovider.DomainStatusTypeActive,
},
MinTimeout: 1 * time.Minute,
Timeout: timeout,
Refresh: func() (interface{}, string, error) {
domain, err := conn.DescribeUserPoolDomain(&cognitoidentityprovider.DescribeUserPoolDomainInput{
Domain: aws.String(d.Get("domain").(string)),
})
if err != nil {
return 42, "", err
}

desc := domain.DomainDescription

return domain, *desc.Status, nil
},
}
_, err = stateConf.WaitForState()
if err != nil {
return err
if _, err := waiter.UserPoolDomainCreated(conn, d.Id(), timeout); err != nil {
return fmt.Errorf("error waiting for User Pool Domain (%s) creation: %w", d.Id(), err)
}

return resourceAwsCognitoUserPoolDomainRead(d, meta)
Expand All @@ -127,7 +103,7 @@ func resourceAwsCognitoUserPoolDomainRead(d *schema.ResourceData, meta interface
Domain: aws.String(d.Id()),
})
if err != nil {
if isAWSErr(err, "ResourceNotFoundException", "") {
if isAWSErr(err, cognitoidentityprovider.ErrCodeResourceNotFoundException, "") {
log.Printf("[WARN] Cognito User Pool Domain %q not found, removing from state", d.Id())
d.SetId("")
return nil
Expand All @@ -137,6 +113,12 @@ func resourceAwsCognitoUserPoolDomainRead(d *schema.ResourceData, meta interface

desc := domain.DomainDescription

if desc.Status == nil {
log.Printf("[WARN] Cognito User Pool Domain %q not found, removing from state", d.Id())
d.SetId("")
return nil
}

d.Set("domain", d.Id())
d.Set("certificate_arn", "")
if desc.CustomDomainConfig != nil {
Expand All @@ -160,35 +142,16 @@ func resourceAwsCognitoUserPoolDomainDelete(d *schema.ResourceData, meta interfa
UserPoolId: aws.String(d.Get("user_pool_id").(string)),
})
if err != nil {
return err
return fmt.Errorf("Error deleting User Pool Domain: %w", err)
}

stateConf := resource.StateChangeConf{
Pending: []string{
cognitoidentityprovider.DomainStatusTypeUpdating,
cognitoidentityprovider.DomainStatusTypeDeleting,
},
Target: []string{""},
Timeout: 1 * time.Minute,
Refresh: func() (interface{}, string, error) {
domain, err := conn.DescribeUserPoolDomain(&cognitoidentityprovider.DescribeUserPoolDomainInput{
Domain: aws.String(d.Id()),
})
if err != nil {
if isAWSErr(err, "ResourceNotFoundException", "") {
return 42, "", nil
}
return 42, "", err
}

desc := domain.DomainDescription
if desc.Status == nil {
return 42, "", nil
}

return domain, *desc.Status, nil
},
if _, err := waiter.UserPoolDomainDeleted(conn, d.Id()); err != nil {
if isAWSErr(err, cognitoidentityprovider.ErrCodeResourceNotFoundException, "") {
return nil
}
return fmt.Errorf("error waiting for User Pool Domain (%s) deletion: %w", d.Id(), err)
}
_, err = stateConf.WaitForState()
return err

return nil

}
24 changes: 23 additions & 1 deletion aws/resource_aws_cognito_user_pool_domain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,28 @@ func TestAccAWSCognitoUserPoolDomain_custom(t *testing.T) {
})
}

func TestAccAWSCognitoUserPoolDomain_disappears(t *testing.T) {
domainName := fmt.Sprintf("tf-acc-test-domain-%d", acctest.RandInt())
poolName := fmt.Sprintf("tf-acc-test-pool-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
resourceName := "aws_cognito_user_pool_domain.main"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSCognitoIdentityProvider(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSCognitoUserPoolDomainDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSCognitoUserPoolDomainConfig_basic(domainName, poolName),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAWSCognitoUserPoolDomainExists(resourceName),
testAccCheckResourceDisappears(testAccProvider, resourceAwsCognitoUserPoolDomain(), resourceName),
),
ExpectNonEmptyPlan: true,
},
},
})
}

func testAccCheckAWSCognitoUserPoolDomainExists(n string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
Expand Down Expand Up @@ -185,7 +207,7 @@ func testAccCheckAWSCognitoUserPoolDomainDestroy(s *terraform.State) error {
})

if err != nil {
if isAWSErr(err, "ResourceNotFoundException", "") {
if isAWSErr(err, cognitoidentityprovider.ErrCodeResourceNotFoundException, "") {
return nil
}
return err
Expand Down