From cba76d06c86ee8a01f5c22b8196ae0a0872dd230 Mon Sep 17 00:00:00 2001 From: Ryn Daniels Date: Thu, 20 Jun 2019 14:04:15 +0200 Subject: [PATCH 1/6] Cleaning up some resource retries for cloudwatch functions --- ...esource_aws_cloudwatch_event_permission.go | 79 ++++++++++++++++--- aws/resource_aws_cloudwatch_event_rule.go | 36 +++++---- ...resource_aws_cloudwatch_log_destination.go | 40 +++++----- ..._aws_cloudwatch_log_subscription_filter.go | 41 +++++----- 4 files changed, 131 insertions(+), 65 deletions(-) diff --git a/aws/resource_aws_cloudwatch_event_permission.go b/aws/resource_aws_cloudwatch_event_permission.go index e72b8c5d6ed..eb8b7546eba 100644 --- a/aws/resource_aws_cloudwatch_event_permission.go +++ b/aws/resource_aws_cloudwatch_event_permission.go @@ -103,41 +103,98 @@ func resourceAwsCloudWatchEventPermissionRead(d *schema.ResourceData, meta inter var policyStatement *CloudWatchEventPermissionPolicyStatement // Especially with concurrent PutPermission calls there can be a slight delay - err := resource.Retry(1*time.Minute, func() *resource.RetryError { + var out *events.DescribeEventBusOutput + var err error + err = resource.Retry(1*time.Minute, func() *resource.RetryError { log.Printf("[DEBUG] Reading CloudWatch Events bus: %s", input) - debo, err := conn.DescribeEventBus(&input) + out, err = conn.DescribeEventBus(&input) + if err != nil { return resource.NonRetryableError(fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error())) } - if debo.Policy == nil { + if out.Policy == nil { return resource.RetryableError(&resource.NotFoundError{ Message: fmt.Sprintf("CloudWatch Events permission %q not found"+ "in given results from DescribeEventBus", d.Id()), - LastResponse: debo, + LastResponse: out, LastRequest: input, }) } + return nil + }) + if isResourceTimeoutError(err) { + out, err = conn.DescribeEventBus(&input) + } + if err != nil { + return fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error()) + } - err = json.Unmarshal([]byte(*debo.Policy), &policyDoc) - if err != nil { - return resource.NonRetryableError(fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error())) - } + err = json.Unmarshal([]byte(*out.Policy), &policyDoc) + if err != nil { + return fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error()) + } + err = resource.Retry(1*time.Minute, func() *resource.RetryError { policyStatement, err = findCloudWatchEventPermissionPolicyStatementByID(&policyDoc, d.Id()) - return resource.RetryableError(err) + if err != nil { + return resource.RetryableError(err) + } + return nil }) + if isResourceTimeoutError(err) { + policyStatement, err = findCloudWatchEventPermissionPolicyStatementByID(&policyDoc, d.Id()) + } if err != nil { - // Missing statement inside valid policy if nfErr, ok := err.(*resource.NotFoundError); ok { log.Printf("[WARN] %s", nfErr) d.SetId("") return nil } - return err } + /* err := resource.Retry(1*time.Minute, func() *resource.RetryError { + log.Printf("[DEBUG] Reading CloudWatch Events bus: %s", input) + debo, err := conn.DescribeEventBus(&input) + if err != nil { + return resource.NonRetryableError(fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error())) + } + + if debo.Policy == nil { + return resource.RetryableError(&resource.NotFoundError{ + Message: fmt.Sprintf("CloudWatch Events permission %q not found"+ + "in given results from DescribeEventBus", d.Id()), + LastResponse: debo, + LastRequest: input, + }) + } + + // TODO maybe this should be out of the block? + err = json.Unmarshal([]byte(*debo.Policy), &policyDoc) + if err != nil { + return resource.NonRetryableError(fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error())) + } + + // TODO This feels like it should be a separate retry block + policyStatement, err = findCloudWatchEventPermissionPolicyStatementByID(&policyDoc, d.Id()) + // TODO also what if err is nil here? + return resource.RetryableError(err) + }) + + if err != nil { + // Missing statement inside valid policy + // TODO use isAWSErr here + if nfErr, ok := err.(*resource.NotFoundError); ok { + log.Printf("[WARN] %s", nfErr) + d.SetId("") + return nil + } + + return err + } + */ + d.Set("action", policyStatement.Action) if err := d.Set("condition", flattenCloudWatchEventPermissionPolicyStatementCondition(policyStatement.Condition)); err != nil { diff --git a/aws/resource_aws_cloudwatch_event_rule.go b/aws/resource_aws_cloudwatch_event_rule.go index 853830ed711..200af79efc2 100644 --- a/aws/resource_aws_cloudwatch_event_rule.go +++ b/aws/resource_aws_cloudwatch_event_rule.go @@ -3,7 +3,6 @@ package aws import ( "fmt" "log" - "regexp" "time" "github.com/aws/aws-sdk-go/aws" @@ -99,22 +98,23 @@ func resourceAwsCloudWatchEventRuleCreate(d *schema.ResourceData, meta interface // IAM Roles take some time to propagate var out *events.PutRuleOutput err = resource.Retry(30*time.Second, func() *resource.RetryError { - var err error out, err = conn.PutRule(input) - pattern := regexp.MustCompile(`cannot be assumed by principal '[a-z]+\.amazonaws\.com'\.$`) + + if isAWSErr(err, "ValidationException", "cannot be assumed by principal") { + log.Printf("[DEBUG] Retrying update of CloudWatch Event Rule %q", *input.Name) + return resource.RetryableError(err) + } if err != nil { - if awsErr, ok := err.(awserr.Error); ok { - if awsErr.Code() == "ValidationException" && pattern.MatchString(awsErr.Message()) { - log.Printf("[DEBUG] Retrying creation of CloudWatch Event Rule %q", *input.Name) - return resource.RetryableError(err) - } - } return resource.NonRetryableError(err) } return nil }) + if isResourceTimeoutError(err) { + _, err = conn.PutRule(input) + } + if err != nil { - return fmt.Errorf("Creating CloudWatch Event Rule failed: %s", err) + return fmt.Errorf("Updating CloudWatch Event Rule failed: %s", err) } d.Set("arn", out.RuleArn) @@ -197,18 +197,20 @@ func resourceAwsCloudWatchEventRuleUpdate(d *schema.ResourceData, meta interface // IAM Roles take some time to propagate err = resource.Retry(30*time.Second, func() *resource.RetryError { _, err := conn.PutRule(input) - pattern := regexp.MustCompile(`cannot be assumed by principal '[a-z]+\.amazonaws\.com'\.$`) + + if isAWSErr(err, "ValidationException", "cannot be assumed by principal") { + log.Printf("[DEBUG] Retrying update of CloudWatch Event Rule %q", *input.Name) + return resource.RetryableError(err) + } if err != nil { - if awsErr, ok := err.(awserr.Error); ok { - if awsErr.Code() == "ValidationException" && pattern.MatchString(awsErr.Message()) { - log.Printf("[DEBUG] Retrying update of CloudWatch Event Rule %q", *input.Name) - return resource.RetryableError(err) - } - } return resource.NonRetryableError(err) } return nil }) + if isResourceTimeoutError(err) { + _, err = conn.PutRule(input) + } + if err != nil { return fmt.Errorf("Updating CloudWatch Event Rule failed: %s", err) } diff --git a/aws/resource_aws_cloudwatch_log_destination.go b/aws/resource_aws_cloudwatch_log_destination.go index e995cf62e16..ab900d843ad 100644 --- a/aws/resource_aws_cloudwatch_log_destination.go +++ b/aws/resource_aws_cloudwatch_log_destination.go @@ -2,11 +2,9 @@ package aws import ( "fmt" - "strings" "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/cloudwatchlogs" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" @@ -64,28 +62,34 @@ func resourceAwsCloudWatchLogDestinationPut(d *schema.ResourceData, meta interfa TargetArn: aws.String(target_arn), } - return resource.Retry(3*time.Minute, func() *resource.RetryError { - resp, err := conn.PutDestination(params) + var resp *cloudwatchlogs.PutDestinationOutput + var err error + err = resource.Retry(3*time.Minute, func() *resource.RetryError { + resp, err = conn.PutDestination(params) - if err == nil { - d.SetId(name) - d.Set("arn", *resp.Destination.Arn) - } - - awsErr, ok := err.(awserr.Error) - if !ok { + if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "Could not deliver test message to specified") { return resource.RetryableError(err) } - - if awsErr.Code() == "InvalidParameterException" { - if strings.Contains(awsErr.Message(), "Could not deliver test message to specified") { - return resource.RetryableError(err) - } + if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "") { return resource.NonRetryableError(err) } - - return resource.NonRetryableError(err) + if isAWSErr(err, "", "") { + return resource.NonRetryableError(err) + } + if err != nil { + return resource.RetryableError(err) + } + return nil }) + if isResourceTimeoutError(err) { + resp, err = conn.PutDestination(params) + } + if err != nil { + return fmt.Errorf("Error putting cloudwatch log destination: %s", err) + } + d.SetId(name) + d.Set("arn", *resp.Destination.Arn) + return nil } func resourceAwsCloudWatchLogDestinationRead(d *schema.ResourceData, meta interface{}) error { diff --git a/aws/resource_aws_cloudwatch_log_subscription_filter.go b/aws/resource_aws_cloudwatch_log_subscription_filter.go index fa6985d57d5..cb4d726817d 100644 --- a/aws/resource_aws_cloudwatch_log_subscription_filter.go +++ b/aws/resource_aws_cloudwatch_log_subscription_filter.go @@ -64,32 +64,35 @@ func resourceAwsCloudwatchLogSubscriptionFilterCreate(d *schema.ResourceData, me params := getAwsCloudWatchLogsSubscriptionFilterInput(d) log.Printf("[DEBUG] Creating SubscriptionFilter %#v", params) - return resource.Retry(5*time.Minute, func() *resource.RetryError { + err := resource.Retry(5*time.Minute, func() *resource.RetryError { _, err := conn.PutSubscriptionFilter(¶ms) - if err == nil { - d.SetId(cloudwatchLogsSubscriptionFilterId(d.Get("log_group_name").(string))) - log.Printf("[DEBUG] Cloudwatch logs subscription %q created", d.Id()) + if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "Could not deliver test message to specified") { + return resource.RetryableError(err) } - - awsErr, ok := err.(awserr.Error) - if !ok { + if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "Could not execute the lambda function") { return resource.RetryableError(err) } - - if awsErr.Code() == "InvalidParameterException" { - log.Printf("[DEBUG] Caught message: %q, code: %q: Retrying", awsErr.Message(), awsErr.Code()) - if strings.Contains(awsErr.Message(), "Could not deliver test message to specified") { - return resource.RetryableError(err) - } - if strings.Contains(awsErr.Message(), "Could not execute the lambda function") { - return resource.RetryableError(err) - } - resource.NonRetryableError(err) + if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "") { + return resource.NonRetryableError(err) } - - return resource.NonRetryableError(err) + if isAWSErr(err, "", "") { + return resource.NonRetryableError(err) + } + return resource.RetryableError(err) }) + + if isResourceTimeoutError(err) { + _, err = conn.PutSubscriptionFilter(¶ms) + } + + if err != nil { + return fmt.Errorf("Error creating Cloudwatch log subscription filter: %s", err) + } + + d.SetId(cloudwatchLogsSubscriptionFilterId(d.Get("log_group_name").(string))) + log.Printf("[DEBUG] Cloudwatch logs subscription %q created", d.Id()) + return nil } func resourceAwsCloudwatchLogSubscriptionFilterUpdate(d *schema.ResourceData, meta interface{}) error { From 5184446b2f5d183d2eba1b6a2c8634de0909e34e Mon Sep 17 00:00:00 2001 From: Ryn Daniels Date: Thu, 20 Jun 2019 14:56:12 +0200 Subject: [PATCH 2/6] Clean up the old error handling --- ...esource_aws_cloudwatch_event_permission.go | 41 ------------------- 1 file changed, 41 deletions(-) diff --git a/aws/resource_aws_cloudwatch_event_permission.go b/aws/resource_aws_cloudwatch_event_permission.go index eb8b7546eba..acdc43bcb86 100644 --- a/aws/resource_aws_cloudwatch_event_permission.go +++ b/aws/resource_aws_cloudwatch_event_permission.go @@ -154,47 +154,6 @@ func resourceAwsCloudWatchEventPermissionRead(d *schema.ResourceData, meta inter return err } - /* err := resource.Retry(1*time.Minute, func() *resource.RetryError { - log.Printf("[DEBUG] Reading CloudWatch Events bus: %s", input) - debo, err := conn.DescribeEventBus(&input) - if err != nil { - return resource.NonRetryableError(fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error())) - } - - if debo.Policy == nil { - return resource.RetryableError(&resource.NotFoundError{ - Message: fmt.Sprintf("CloudWatch Events permission %q not found"+ - "in given results from DescribeEventBus", d.Id()), - LastResponse: debo, - LastRequest: input, - }) - } - - // TODO maybe this should be out of the block? - err = json.Unmarshal([]byte(*debo.Policy), &policyDoc) - if err != nil { - return resource.NonRetryableError(fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error())) - } - - // TODO This feels like it should be a separate retry block - policyStatement, err = findCloudWatchEventPermissionPolicyStatementByID(&policyDoc, d.Id()) - // TODO also what if err is nil here? - return resource.RetryableError(err) - }) - - if err != nil { - // Missing statement inside valid policy - // TODO use isAWSErr here - if nfErr, ok := err.(*resource.NotFoundError); ok { - log.Printf("[WARN] %s", nfErr) - d.SetId("") - return nil - } - - return err - } - */ - d.Set("action", policyStatement.Action) if err := d.Set("condition", flattenCloudWatchEventPermissionPolicyStatementCondition(policyStatement.Condition)); err != nil { From 86263d264f674e2eabd0d2a0807fa71acf1f8651 Mon Sep 17 00:00:00 2001 From: Ryn Daniels Date: Fri, 21 Jun 2019 17:03:22 +0200 Subject: [PATCH 3/6] Update aws/resource_aws_cloudwatch_log_destination.go Co-Authored-By: Brian Flad --- aws/resource_aws_cloudwatch_log_destination.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_cloudwatch_log_destination.go b/aws/resource_aws_cloudwatch_log_destination.go index ab900d843ad..0259d91d9c5 100644 --- a/aws/resource_aws_cloudwatch_log_destination.go +++ b/aws/resource_aws_cloudwatch_log_destination.go @@ -88,7 +88,7 @@ func resourceAwsCloudWatchLogDestinationPut(d *schema.ResourceData, meta interfa return fmt.Errorf("Error putting cloudwatch log destination: %s", err) } d.SetId(name) - d.Set("arn", *resp.Destination.Arn) + d.Set("arn", resp.Destination.Arn) return nil } From 98e09fd41e1537fbc15f09002dc9a2f156213b7e Mon Sep 17 00:00:00 2001 From: Ryn Daniels Date: Fri, 21 Jun 2019 17:14:02 +0200 Subject: [PATCH 4/6] changes to AWSErr handling from code review --- aws/resource_aws_cloudwatch_log_destination.go | 8 +------- aws/resource_aws_cloudwatch_log_subscription_filter.go | 7 ++----- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/aws/resource_aws_cloudwatch_log_destination.go b/aws/resource_aws_cloudwatch_log_destination.go index 0259d91d9c5..ab2eda1abbf 100644 --- a/aws/resource_aws_cloudwatch_log_destination.go +++ b/aws/resource_aws_cloudwatch_log_destination.go @@ -70,14 +70,8 @@ func resourceAwsCloudWatchLogDestinationPut(d *schema.ResourceData, meta interfa if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "Could not deliver test message to specified") { return resource.RetryableError(err) } - if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "") { - return resource.NonRetryableError(err) - } - if isAWSErr(err, "", "") { - return resource.NonRetryableError(err) - } if err != nil { - return resource.RetryableError(err) + return resource.NonRetryableError(err) } return nil }) diff --git a/aws/resource_aws_cloudwatch_log_subscription_filter.go b/aws/resource_aws_cloudwatch_log_subscription_filter.go index cb4d726817d..acbafac3a1f 100644 --- a/aws/resource_aws_cloudwatch_log_subscription_filter.go +++ b/aws/resource_aws_cloudwatch_log_subscription_filter.go @@ -73,13 +73,10 @@ func resourceAwsCloudwatchLogSubscriptionFilterCreate(d *schema.ResourceData, me if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "Could not execute the lambda function") { return resource.RetryableError(err) } - if isAWSErr(err, cloudwatchlogs.ErrCodeInvalidParameterException, "") { + if err != nil { return resource.NonRetryableError(err) } - if isAWSErr(err, "", "") { - return resource.NonRetryableError(err) - } - return resource.RetryableError(err) + return nil }) if isResourceTimeoutError(err) { From c823a730c313ade319c51d952699365d5b1f131f Mon Sep 17 00:00:00 2001 From: Ryn Daniels Date: Fri, 5 Jul 2019 17:15:15 +0200 Subject: [PATCH 5/6] Update event permission read based on code review --- ...esource_aws_cloudwatch_event_permission.go | 69 +++++++++---------- 1 file changed, 33 insertions(+), 36 deletions(-) diff --git a/aws/resource_aws_cloudwatch_event_permission.go b/aws/resource_aws_cloudwatch_event_permission.go index acdc43bcb86..e70b8cbecdd 100644 --- a/aws/resource_aws_cloudwatch_event_permission.go +++ b/aws/resource_aws_cloudwatch_event_permission.go @@ -99,58 +99,36 @@ func resourceAwsCloudWatchEventPermissionCreate(d *schema.ResourceData, meta int func resourceAwsCloudWatchEventPermissionRead(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).cloudwatcheventsconn input := events.DescribeEventBusInput{} - var policyDoc CloudWatchEventPermissionPolicyDoc + var output *events.DescribeEventBusOutput var policyStatement *CloudWatchEventPermissionPolicyStatement // Especially with concurrent PutPermission calls there can be a slight delay - var out *events.DescribeEventBusOutput var err error err = resource.Retry(1*time.Minute, func() *resource.RetryError { log.Printf("[DEBUG] Reading CloudWatch Events bus: %s", input) - out, err = conn.DescribeEventBus(&input) - + output, err := conn.DescribeEventBus(&input) if err != nil { return resource.NonRetryableError(fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error())) } - if out.Policy == nil { - return resource.RetryableError(&resource.NotFoundError{ - Message: fmt.Sprintf("CloudWatch Events permission %q not found"+ - "in given results from DescribeEventBus", d.Id()), - LastResponse: out, - LastRequest: input, - }) - } - return nil + policyStatement, err = getPolicyStatement(output, d.Id()) + return resource.RetryableError(err) }) - if isResourceTimeoutError(err) { - out, err = conn.DescribeEventBus(&input) - } - if err != nil { - return fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error()) - } - err = json.Unmarshal([]byte(*out.Policy), &policyDoc) - if err != nil { - return fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error()) + if isResourceTimeoutError(err) { + output, err = conn.DescribeEventBus(&input) + if output != nil { + policyStatement, err = getPolicyStatement(output, d.Id()) + } } - err = resource.Retry(1*time.Minute, func() *resource.RetryError { - policyStatement, err = findCloudWatchEventPermissionPolicyStatementByID(&policyDoc, d.Id()) - if err != nil { - return resource.RetryableError(err) - } + if isResourceNotFoundError(err) { + log.Printf("[WARN] %s", err) + d.SetId("") return nil - }) - if isResourceTimeoutError(err) { - policyStatement, err = findCloudWatchEventPermissionPolicyStatementByID(&policyDoc, d.Id()) } if err != nil { - if nfErr, ok := err.(*resource.NotFoundError); ok { - log.Printf("[WARN] %s", nfErr) - d.SetId("") - return nil - } + // Missing statement inside valid policy return err } @@ -167,7 +145,7 @@ func resourceAwsCloudWatchEventPermissionRead(d *schema.ResourceData, meta inter principalMap := policyStatement.Principal.(map[string]interface{}) policyARN, err := arn.Parse(principalMap["AWS"].(string)) if err != nil { - return fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err.Error()) + return fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", d.Id(), err) } d.Set("principal", policyARN.AccountID) } @@ -176,6 +154,25 @@ func resourceAwsCloudWatchEventPermissionRead(d *schema.ResourceData, meta inter return nil } +func getPolicyStatement(output *events.DescribeEventBusOutput, statementID string) (*CloudWatchEventPermissionPolicyStatement, error) { + var policyDoc CloudWatchEventPermissionPolicyDoc + + if output == nil || output.Policy == nil { + return nil, &resource.NotFoundError{ + Message: fmt.Sprintf("CloudWatch Events permission %q not found"+ + "in given results from DescribeEventBus", statementID), + LastResponse: output, + } + } + + err := json.Unmarshal([]byte(*output.Policy), &policyDoc) + if err != nil { + return nil, fmt.Errorf("Reading CloudWatch Events permission '%s' failed: %s", statementID, err) + } + + return findCloudWatchEventPermissionPolicyStatementByID(&policyDoc, statementID) +} + func resourceAwsCloudWatchEventPermissionUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).cloudwatcheventsconn From f6e475bdcf42d991e60be9b686ea6d8955ae239b Mon Sep 17 00:00:00 2001 From: Ryn Daniels Date: Fri, 5 Jul 2019 18:48:45 +0200 Subject: [PATCH 6/6] Appease the linting gods --- aws/resource_aws_cloudwatch_event_permission.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/aws/resource_aws_cloudwatch_event_permission.go b/aws/resource_aws_cloudwatch_event_permission.go index e70b8cbecdd..be61ac40bdc 100644 --- a/aws/resource_aws_cloudwatch_event_permission.go +++ b/aws/resource_aws_cloudwatch_event_permission.go @@ -103,8 +103,7 @@ func resourceAwsCloudWatchEventPermissionRead(d *schema.ResourceData, meta inter var policyStatement *CloudWatchEventPermissionPolicyStatement // Especially with concurrent PutPermission calls there can be a slight delay - var err error - err = resource.Retry(1*time.Minute, func() *resource.RetryError { + err := resource.Retry(1*time.Minute, func() *resource.RetryError { log.Printf("[DEBUG] Reading CloudWatch Events bus: %s", input) output, err := conn.DescribeEventBus(&input) if err != nil {