resource/aws_kms_grant: Retries read until principals return valid ARNs

.changelog/29245.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_kms_grant: Retries until valid principal ARNs are returned instead of not updating state
+```

.ci/.semgrep.yml

@@ -226,8 +226,6 @@ rules:
     paths:
       include:
         - internal/
-      exclude:
-        - internal/service/apigateway/integration.go
     patterns:
       - pattern-either:
           - pattern: |

.ci/semgrep/aws/arn.yml

@@ -0,0 +1,10 @@
+rules:
+  - id: prefer-isarn-to-stringshasprefix
+    languages: [go]
+    message: Prefer `aws.IsARN` to `strings.HasPrefix`
+    patterns:
+      - pattern: strings.HasPrefix($STR, $ARN)
+      - metavariable-regex:
+          metavariable: $ARN
+          regex: ^\"arn:[^"]*\"$
+    severity: WARNING

.github/workflows/semgrep-ci.yml

@@ -28,6 +28,8 @@ jobs:
           semgrep $COMMON_PARAMS \
             --config .ci/.semgrep.yml \
             --config .ci/semgrep/acctest/ \
+            --config .ci/semgrep/aws/ \
+            --config .ci/semgrep/migrate/ \
             --config 'r/dgryski.semgrep-go.badnilguard' \
             --config 'r/dgryski.semgrep-go.errnilcheck' \
             --config 'r/dgryski.semgrep-go.marshaljson' \

GNUmakefile

@@ -275,6 +275,7 @@ semall:
 		--config .ci/.semgrep-service-name2.yml \
 		--config .ci/.semgrep-service-name3.yml \
 		--config .ci/semgrep/acctest/ \
+		--config .ci/semgrep/aws/ \
 		--config .ci/semgrep/migrate/ \
 		--config 'r/dgryski.semgrep-go.badnilguard' \
 		--config 'r/dgryski.semgrep-go.errnilcheck' \

internal/service/apigateway/integration.go

@@ -279,7 +279,7 @@ func resourceIntegrationRead(ctx context.Context, d *schema.ResourceData, meta i
 	d.Set("cache_namespace", integration.CacheNamespace)
 	d.Set("connection_id", integration.ConnectionId)
 	d.Set("connection_type", apigateway.ConnectionTypeInternet)
-	if integration.ConnectionType != nil {
+	if integration.ConnectionType != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check
 		d.Set("connection_type", integration.ConnectionType)
 	}
 	d.Set("content_handling", integration.ContentHandling)

internal/service/iam/group_policy_attachment_test.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/aws/aws-sdk-go-v2/aws/arn"
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/iam"
 	sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
@@ -50,7 +51,7 @@ func TestAccIAMGroupPolicyAttachment_basic(t *testing.T) {
 						return fmt.Errorf("expected 1 state: %#v", s)
 					}
 					rs := s[0]
-					if !strings.HasPrefix(rs.Attributes["policy_arn"], "arn:") {
+					if !arn.IsARN(rs.Attributes["policy_arn"]) {
 						return fmt.Errorf("expected policy_arn attribute to be set and begin with arn:, received: %s", rs.Attributes["policy_arn"])
 					}
 					return nil

internal/service/iam/role_policy_attachment_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
 	sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
@@ -52,7 +53,7 @@ func TestAccIAMRolePolicyAttachment_basic(t *testing.T) {
 
 					rs := s[0]
 
-					if !strings.HasPrefix(rs.Attributes["policy_arn"], "arn:") {
+					if !arn.IsARN(rs.Attributes["policy_arn"]) {
 						return fmt.Errorf("expected policy_arn attribute to be set and begin with arn:, received: %s", rs.Attributes["policy_arn"])
 					}
 

internal/service/iam/user_policy_attachment_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/iam"
 	sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
@@ -50,7 +51,7 @@ func TestAccIAMUserPolicyAttachment_basic(t *testing.T) {
 
 					rs := s[0]
 
-					if !strings.HasPrefix(rs.Attributes["policy_arn"], "arn:") {
+					if !arn.IsARN(rs.Attributes["policy_arn"]) {
 						return fmt.Errorf("expected policy_arn attribute to be set and begin with arn:, received: %s", rs.Attributes["policy_arn"])
 					}
 

internal/service/iam/wait.go

@@ -2,10 +2,10 @@ package iam
 
 import (
 	"context"
-	"strings"
 	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
@@ -26,10 +26,14 @@ const (
 )
 
 func waitRoleARNIsNotUniqueID(ctx context.Context, conn *iam.IAM, id string, role *iam.Role) (*iam.Role, error) {
+	if arn.IsARN(aws.StringValue(role.Arn)) {
+		return role, nil
+	}
+
 	stateConf := &resource.StateChangeConf{
 		Pending:                   []string{RoleStatusARNIsUniqueID, RoleStatusNotFound},
 		Target:                    []string{RoleStatusARNIsARN},
-		Refresh:                   statusRoleCreate(ctx, conn, id, role),
+		Refresh:                   statusRoleCreate(ctx, conn, id),
 		Timeout:                   propagationTimeout,
 		NotFoundChecks:            10,
 		ContinuousTargetOccurence: 5,
@@ -44,13 +48,9 @@ func waitRoleARNIsNotUniqueID(ctx context.Context, conn *iam.IAM, id string, rol
 	return nil, err
 }
 
-func statusRoleCreate(ctx context.Context, conn *iam.IAM, id string, role *iam.Role) resource.StateRefreshFunc {
+func statusRoleCreate(ctx context.Context, conn *iam.IAM, id string) resource.StateRefreshFunc {
 	return func() (interface{}, string, error) {
-		if strings.HasPrefix(aws.StringValue(role.Arn), "arn:") {
-			return role, RoleStatusARNIsARN, nil
-		}
-
-		output, err := FindRoleByName(ctx, conn, id)
+		role, err := FindRoleByName(ctx, conn, id)
 
 		if tfresource.NotFound(err) {
 			return nil, RoleStatusNotFound, nil
@@ -60,11 +60,11 @@ func statusRoleCreate(ctx context.Context, conn *iam.IAM, id string, role *iam.R
 			return nil, "", err
 		}
 
-		if strings.HasPrefix(aws.StringValue(output.Arn), "arn:") {
-			return output, RoleStatusARNIsARN, nil
+		if arn.IsARN(aws.StringValue(role.Arn)) {
+			return role, RoleStatusARNIsARN, nil
 		}
 
-		return output, RoleStatusARNIsUniqueID, nil
+		return role, RoleStatusARNIsUniqueID, nil
 	}
 }
 

internal/service/kms/grant.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/kms"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
@@ -164,11 +165,8 @@ func resourceGrantCreate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Error Codes: https://docs.aws.amazon.com/sdk-for-go/api/service/kms/#KMS.CreateGrant
 			// Under some circumstances a newly created IAM Role doesn't show up and causes
 			// an InvalidArnException to be thrown.
-			if tfawserr.ErrCodeEquals(err, kms.ErrCodeDependencyTimeoutException) ||
-				tfawserr.ErrCodeEquals(err, kms.ErrCodeInternalException) ||
-				tfawserr.ErrCodeEquals(err, kms.ErrCodeInvalidArnException) {
-				return resource.RetryableError(
-					fmt.Errorf("creating KMS Grant for Key (%s), retrying: %w", keyId, err))
+			if tfawserr.ErrCodeEquals(err, kms.ErrCodeDependencyTimeoutException, kms.ErrCodeInternalException, kms.ErrCodeInvalidArnException) {
+				return resource.RetryableError(err)
 			}
 			return resource.NonRetryableError(err)
 		}
@@ -216,35 +214,22 @@ func resourceGrantRead(ctx context.Context, d *schema.ResourceData, meta interfa
 		return diags
 	}
 
-	// The grant sometimes contains principals that identified by their unique id: "AROAJYCVIVUZIMTXXXXX"
-	// instead of "arn:aws:...", in this case don't update the state file
-	if strings.HasPrefix(*grant.GranteePrincipal, "arn:aws") {
+	if grant.GranteePrincipal != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check
 		d.Set("grantee_principal", grant.GranteePrincipal)
-	} else {
-		log.Printf(
-			"[WARN] Unable to update grantee principal state %s for grant id %s for key id %s.",
-			*grant.GranteePrincipal, grantId, keyId)
-	}
-
-	if grant.RetiringPrincipal != nil {
-		if strings.HasPrefix(*grant.RetiringPrincipal, "arn:aws") {
-			d.Set("retiring_principal", grant.RetiringPrincipal)
-		} else {
-			log.Printf(
-				"[WARN] Unable to update retiring principal state %s for grant id %s for key id %s",
-				*grant.RetiringPrincipal, grantId, keyId)
-		}
+	}
+	if grant.RetiringPrincipal != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check
+		d.Set("retiring_principal", grant.RetiringPrincipal)
 	}
 
 	if err := d.Set("operations", aws.StringValueSlice(grant.Operations)); err != nil {
-		log.Printf("[DEBUG] Error setting operations for grant %s with error %s", grantId, err)
+		return sdkdiag.AppendErrorf(diags, "setting operations: %s", err)
 	}
 	if aws.StringValue(grant.Name) != "" {
 		d.Set("name", grant.Name)
 	}
 	if grant.Constraints != nil {
 		if err := d.Set("constraints", flattenGrantConstraints(grant.Constraints)); err != nil {
-			log.Printf("[DEBUG] Error setting constraints for grant %s with error %s", grantId, err)
+			return sdkdiag.AppendErrorf(diags, "setting constraints: %s", err)
 		}
 	}
 
@@ -324,6 +309,18 @@ func findGrantByIdRetry(ctx context.Context, conn *kms.KMS, keyId string, grantI
 			return resource.NonRetryableError(err)
 		}
 
+		if principal := aws.StringValue(grant.GranteePrincipal); principal != "" {
+			if !arn.IsARN(principal) {
+				return resource.RetryableError(fmt.Errorf("grantee principal is invalid: %q", principal))
+			}
+		}
+
+		if principal := aws.StringValue(grant.RetiringPrincipal); principal != "" {
+			if !arn.IsARN(principal) {
+				return resource.RetryableError(fmt.Errorf("retiring principal is invalid: %q", principal))
+			}
+		}
+
 		return nil
 	})
 	if tfresource.TimedOut(err) {
@@ -523,7 +520,7 @@ func flattenGrantConstraints(constraint *kms.GrantConstraints) *schema.Set {
 }
 
 func decodeGrantID(id string) (string, string, error) {
-	if strings.HasPrefix(id, "arn:aws") {
+	if arn.IsARN(id) {
 		arnParts := strings.Split(id, "/")
 		if len(arnParts) != 2 {
 			return "", "", fmt.Errorf("unexpected format of ARN (%q), expected KeyID:GrantID", id)

internal/service/kms/grant_test.go

@@ -34,8 +34,8 @@ func TestAccKMSGrant_basic(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "operations.#", "2"),
 					resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Encrypt"),
 					resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Decrypt"),
-					resource.TestCheckResourceAttrSet(resourceName, "grantee_principal"),
-					resource.TestCheckResourceAttrSet(resourceName, "key_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "grantee_principal", "aws_iam_role.test", "arn"),
+					resource.TestCheckResourceAttrPair(resourceName, "key_id", "aws_kms_key.test", "key_id"),
 				),
 			},
 			{
@@ -112,7 +112,7 @@ func TestAccKMSGrant_withRetiringPrincipal(t *testing.T) {
 				Config: testAccGrantConfig_retiringPrincipal(rName),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckGrantExists(resourceName),
-					resource.TestCheckResourceAttrSet(resourceName, "retiring_principal"),
+					resource.TestCheckResourceAttrPair(resourceName, "retiring_principal", "aws_iam_role.test", "arn"),
 				),
 			},
 			{
@@ -174,8 +174,8 @@ func TestAccKMSGrant_arn(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "operations.#", "2"),
 					resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Encrypt"),
 					resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Decrypt"),
-					resource.TestCheckResourceAttrSet(resourceName, "grantee_principal"),
-					resource.TestCheckResourceAttrSet(resourceName, "key_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "grantee_principal", "aws_iam_role.test", "arn"),
+					resource.TestCheckResourceAttrPair(resourceName, "key_id", "aws_kms_key.test", "arn"),
 				),
 			},
 			{