diff --git a/.changelog/29245.txt b/.changelog/29245.txt new file mode 100644 index 00000000000..afe860821c6 --- /dev/null +++ b/.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 +``` diff --git a/.ci/.semgrep.yml b/.ci/.semgrep.yml index 79ac465d4dd..3f1094361e9 100644 --- a/.ci/.semgrep.yml +++ b/.ci/.semgrep.yml @@ -226,8 +226,6 @@ rules: paths: include: - internal/ - exclude: - - internal/service/apigateway/integration.go patterns: - pattern-either: - pattern: | diff --git a/.ci/semgrep/aws/arn.yml b/.ci/semgrep/aws/arn.yml new file mode 100644 index 00000000000..917c24f894e --- /dev/null +++ b/.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 diff --git a/.github/workflows/semgrep-ci.yml b/.github/workflows/semgrep-ci.yml index 596e01c208c..a3de3b19924 100644 --- a/.github/workflows/semgrep-ci.yml +++ b/.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' \ diff --git a/GNUmakefile b/GNUmakefile index 87a2b4f3f8d..2b93a2a4308 100644 --- a/GNUmakefile +++ b/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' \ diff --git a/internal/service/apigateway/integration.go b/internal/service/apigateway/integration.go index 46f572ef1d6..7d20febd758 100644 --- a/internal/service/apigateway/integration.go +++ b/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) diff --git a/internal/service/iam/group_policy_attachment_test.go b/internal/service/iam/group_policy_attachment_test.go index d77f947eecb..986d51e437d 100644 --- a/internal/service/iam/group_policy_attachment_test.go +++ b/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 diff --git a/internal/service/iam/role_policy_attachment_test.go b/internal/service/iam/role_policy_attachment_test.go index f4b08883019..09ebf25c8fd 100644 --- a/internal/service/iam/role_policy_attachment_test.go +++ b/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"]) } diff --git a/internal/service/iam/user_policy_attachment_test.go b/internal/service/iam/user_policy_attachment_test.go index 4875b263bec..9bb090e8c60 100644 --- a/internal/service/iam/user_policy_attachment_test.go +++ b/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"]) } diff --git a/internal/service/iam/wait.go b/internal/service/iam/wait.go index 428fb3958e8..72dd1b89270 100644 --- a/internal/service/iam/wait.go +++ b/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 } } diff --git a/internal/service/kms/grant.go b/internal/service/kms/grant.go index 78adb7e3ad3..9c358bc7985 100644 --- a/internal/service/kms/grant.go +++ b/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) diff --git a/internal/service/kms/grant_test.go b/internal/service/kms/grant_test.go index 5ac0bb4f089..b7e94e972a1 100644 --- a/internal/service/kms/grant_test.go +++ b/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"), ), }, {