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.

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/iam_service_specific_credential - new resource #16185

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

DrFaust92
Copy link
Collaborator

@DrFaust92 DrFaust92 commented Nov 13, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #3233

Release note for CHANGELOG:

Output from acceptance testing:

$  make testacc TESTS=TestAccServiceSpecificCredential_ PKG=iam
--- PASS: TestAccServiceSpecificCredential_disappears (36.11s)
--- PASS: TestAccServiceSpecificCredential_basic (41.21s)
--- PASS: TestAccServiceSpecificCredential_multi (41.70s)
--- PASS: TestAccServiceSpecificCredential_status (93.78s)
$  make testacc TESTS=TestAccIAMUser_ PKG=iam
--- PASS: TestAccIAMUser_disappears (30.64s)
--- PASS: TestAccIAMUser_ForceDestroy_signingCertificate (45.27s)
--- PASS: TestAccIAMUser_ForceDestroy_serviceSpecificCred (46.38s)
--- PASS: TestAccIAMUser_ForceDestroy_accessKey (46.43s)
--- PASS: TestAccIAMUser_ForceDestroy_sshKey (46.80s)
--- PASS: TestAccIAMUser_ForceDestroy_mfaDevice (48.31s)
--- PASS: TestAccIAMUser_ForceDestroy_loginProfile (49.13s)
--- PASS: TestAccIAMUser_basic (63.65s)
--- PASS: TestAccIAMUser_pathChange (64.20s)
--- PASS: TestAccIAMUser_tags (65.18s)
--- PASS: TestAccIAMUser_nameChange (65.44s)
--- PASS: TestAccIAMUser_permissionsBoundary (142.00s)

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/iam Issues and PRs that pertain to the iam service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 13, 2020
@DrFaust92 DrFaust92 marked this pull request as ready for review November 13, 2020 21:17
@DrFaust92 DrFaust92 requested a review from a team as a code owner November 13, 2020 21:17
@DrFaust92 DrFaust92 added the new-resource Introduces a new resource. label Dec 8, 2020
@quinyx-tjeerd
Copy link

quinyx-tjeerd commented Jan 12, 2021

@bflad / @anGie44 can we maybe get this reviewed? with AWS Cassandra Keyspaces out this becomes quite mandatory to be able to adhere to IaC.
There's a group of people who'd love to see this feature here #3233 😅

Base automatically changed from master to main January 23, 2021 00:59

cred := out.ServiceSpecificCredential

d.SetId(fmt.Sprintf("%s:%s", aws.StringValue(cred.ServiceName), aws.StringValue(cred.UserName)))
Copy link

Choose a reason for hiding this comment

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

Can we use ServiceSpecificCredentialId instead?
An IAM user can have multiple service specific credentials for the same service, for example:

{
    "ServiceSpecificCredentials": [
        {
            "UserName": "some.user",
            "Status": "Active",
            "ServiceUserName": "some.user-at-322414443281",
            "CreateDate": "2021-02-04T17:47:09Z",
            "ServiceSpecificCredentialId": "ABC125",
            "ServiceName": "cassandra.amazonaws.com"
        },
        {
            "UserName": "some.user",
            "Status": "Active",
            "ServiceUserName": "some.user+1-at-1234567890",
            "CreateDate": "2021-02-04T19:59:25Z",
            "ServiceSpecificCredentialId": "ABC123",
            "ServiceName": "cassandra.amazonaws.com"
        }
    ]
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ListServiceSpecificCredentials function takes user name and service name parameters so we cant use it.
but assuming a user can have more than key per service i will need to add the ServiceSpecificCredentialId as part of the Id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

taking it back it doesnt seems that a single user can have multi keys (per service) so keeping as is

Copy link

@jverce jverce Jul 31, 2021

Choose a reason for hiding this comment

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

Actually it can, and it's by design so that users can rotate their credentials without having to wait for them to expire.

Here's an example of the output for my account:

  • Command

    aws iam list-service-specific-credentials
  • Output

    {
        "ServiceSpecificCredentials": [
            {
                "UserName": "jverce",
                "Status": "Active",
                "ServiceUserName": "jverce-at-081124132640",
                "CreateDate": "2021-07-31T00:20:18+00:00",
                "ServiceSpecificCredentialId": "ACCARFY3AA4QGT4YVGPGT",
                "ServiceName": "cassandra.amazonaws.com"
            },
            {
                "UserName": "jverce",
                "Status": "Active",
                "ServiceUserName": "jverce+1-at-081124132640",
                "CreateDate": "2021-07-31T00:20:25+00:00",
                "ServiceSpecificCredentialId": "ACCARFY3AA4QFRQLHXTYW",
                "ServiceName": "cassandra.amazonaws.com"
            },
            {
                "UserName": "jverce",
                "Status": "Active",
                "ServiceUserName": "jverce-at-081124132640",
                "CreateDate": "2021-07-31T00:23:35+00:00",
                "ServiceSpecificCredentialId": "ACCARFY3AA4QIUPNNJKHQ",
                "ServiceName": "codecommit.amazonaws.com"
            },
            {
                "UserName": "jverce",
                "Status": "Active",
                "ServiceUserName": "jverce+1-at-081124132640",
                "CreateDate": "2021-07-31T00:23:38+00:00",
                "ServiceSpecificCredentialId": "ACCARFY3AA4QI4CCNKJV7",
                "ServiceName": "codecommit.amazonaws.com"
            }
        ]
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added cred is as part of id to allow for multiple keys under same user

func resourceAwsIamServiceSpecificCredentialRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).iamconn

serviceName, userName, err := decodeAwsIamServiceSpecificCredential(d.Id())
Copy link

Choose a reason for hiding this comment

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

decodeAwsIamServiceSpecificCredential wouldn't be necessary if we stick to ServiceSpecificCredentialId, correct?

Comment on lines 122 to 124
if len(out.ServiceSpecificCredentials) > 1 {
return fmt.Errorf("error reading IAM Service Specific Credential: multiple results found, try adjusting search criteria")
}
Copy link

Choose a reason for hiding this comment

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

We should allow for this use case. The ID is already available through d.Id() if we use ServiceSpecificCredentialId. We'd then cycle through this slice (which length is at most 2) and return that credential.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ill add ServiceSpecificCredentialId as part of the id (cant be used alone) but either way this acts more as a guard and this check is implemented in several resources

Copy link

Choose a reason for hiding this comment

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

Yeah, I see your point, that the user won't have access to this ID to specify it later on.
Maybe we can pick one of the credentials based whether they are active or not? And if both are active, just pick the latest one (based on .ServiceSpecificCredentials[].CreatedDate)?

It seems like the use case for AWS customers to have 2 credentials is to be able to rotate them without disruption, so it would make sense to go with that approach IMHO. What do you think?

Copy link

Choose a reason for hiding this comment

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

Other than the "status" there isn't anything else for the customer to in terms of mutation (e.g. permissions or access to other services), since those are determined for the IAM user itself, so the proposed heuristic shouldn't present any major risks AFAICT.


cred := out.ServiceSpecificCredentials[0]

d.Set("service_specific_credential_id", cred.ServiceSpecificCredentialId)
Copy link

Choose a reason for hiding this comment

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

Related, this would be set during the Create step.

conn := meta.(*AWSClient).iamconn

input := &iam.DeleteServiceSpecificCredentialInput{
ServiceSpecificCredentialId: aws.String(d.Get("service_specific_credential_id").(string)),
Copy link

Choose a reason for hiding this comment

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

Same here, we'd just do d.Id()

aws/resource_aws_iam_service_specific_credential.go Outdated Show resolved Hide resolved
@ghost ghost added the documentation Introduces or discusses updates to documentation. label Mar 11, 2021
@DrFaust92
Copy link
Collaborator Author

@jverce ill answer here instead of per comment as its the same answer. im voting to keep the current id struct as you can have only 2 keys per user + service, and there isnt a way to distinct in TF on when to create a new key or not. managing several iam keys per above combo introduces too much maintenance burden and room for errors (if for some reason the 2 key limitation is broken)

at the end of the day part of the reason for this resource is to better scope credentials of iam users.

@sethbacon
Copy link

Anyone looking at moving this forward?

@zhelding
Copy link
Contributor

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

@bixu
Copy link

bixu commented Nov 2, 2021

@DrFaust92, let me know via a mention if this gets merged and you want a user to "kick the tires". My team is very interested :)

@sanarena
Copy link

Any update on this one?

Copy link
Collaborator

@AdamTylerLynch AdamTylerLynch left a comment

Choose a reason for hiding this comment

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

A few changes requested.

internal/service/iam/service_specific_credential_test.go Outdated Show resolved Hide resolved
internal/service/iam/service_specific_credential_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the sweeper Pertains to changes to or issues with the sweeper. label Feb 10, 2022
Copy link
Collaborator

@AdamTylerLynch AdamTylerLynch left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@DrFaust92
Copy link
Collaborator Author

Consideration for reviewer, i might want to add deleting creds for a user when deleting a user (via some force_delete flag)

@gdavison
Copy link
Contributor

Yes, since the force_destroy flag deletes a bunch of other resources linked to the user, this should be added as well

@DrFaust92
Copy link
Collaborator Author

@gdavison, Thanks! added that to force_destory logic + test

Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM 🚀.

Commercial
% make testacc TESTARGS='-run=TestAccServiceSpecificCredential_\|TestAccIAMUser_' PKG=iam
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/iam/... -v -count 1 -parallel 20  -run=TestAccServiceSpecificCredential_\|TestAccIAMUser_ -timeout 180m
=== RUN   TestAccServiceSpecificCredential_basic
=== PAUSE TestAccServiceSpecificCredential_basic
=== RUN   TestAccServiceSpecificCredential_multi
=== PAUSE TestAccServiceSpecificCredential_multi
=== RUN   TestAccServiceSpecificCredential_status
=== PAUSE TestAccServiceSpecificCredential_status
=== RUN   TestAccServiceSpecificCredential_disappears
=== PAUSE TestAccServiceSpecificCredential_disappears
=== RUN   TestAccIAMUser_basic
=== PAUSE TestAccIAMUser_basic
=== RUN   TestAccIAMUser_disappears
=== PAUSE TestAccIAMUser_disappears
=== RUN   TestAccIAMUser_ForceDestroy_accessKey
=== PAUSE TestAccIAMUser_ForceDestroy_accessKey
=== RUN   TestAccIAMUser_ForceDestroy_loginProfile
=== PAUSE TestAccIAMUser_ForceDestroy_loginProfile
=== RUN   TestAccIAMUser_ForceDestroy_mfaDevice
=== PAUSE TestAccIAMUser_ForceDestroy_mfaDevice
=== RUN   TestAccIAMUser_ForceDestroy_sshKey
=== PAUSE TestAccIAMUser_ForceDestroy_sshKey
=== RUN   TestAccIAMUser_ForceDestroy_serviceSpecificCred
=== PAUSE TestAccIAMUser_ForceDestroy_serviceSpecificCred
=== RUN   TestAccIAMUser_ForceDestroy_signingCertificate
=== PAUSE TestAccIAMUser_ForceDestroy_signingCertificate
=== RUN   TestAccIAMUser_nameChange
=== PAUSE TestAccIAMUser_nameChange
=== RUN   TestAccIAMUser_pathChange
=== PAUSE TestAccIAMUser_pathChange
=== RUN   TestAccIAMUser_permissionsBoundary
=== PAUSE TestAccIAMUser_permissionsBoundary
=== RUN   TestAccIAMUser_tags
=== PAUSE TestAccIAMUser_tags
=== CONT  TestAccServiceSpecificCredential_basic
=== CONT  TestAccIAMUser_ForceDestroy_mfaDevice
=== CONT  TestAccIAMUser_pathChange
=== CONT  TestAccIAMUser_basic
=== CONT  TestAccServiceSpecificCredential_disappears
=== CONT  TestAccIAMUser_nameChange
=== CONT  TestAccIAMUser_tags
=== CONT  TestAccIAMUser_permissionsBoundary
=== CONT  TestAccIAMUser_ForceDestroy_signingCertificate
=== CONT  TestAccIAMUser_ForceDestroy_serviceSpecificCred
=== CONT  TestAccIAMUser_ForceDestroy_sshKey
=== CONT  TestAccIAMUser_ForceDestroy_accessKey
=== CONT  TestAccIAMUser_ForceDestroy_loginProfile
=== CONT  TestAccServiceSpecificCredential_multi
=== CONT  TestAccServiceSpecificCredential_status
=== CONT  TestAccIAMUser_disappears
--- PASS: TestAccIAMUser_disappears (30.29s)
--- PASS: TestAccServiceSpecificCredential_disappears (38.34s)
--- PASS: TestAccServiceSpecificCredential_basic (44.91s)
--- PASS: TestAccIAMUser_ForceDestroy_signingCertificate (45.55s)
--- PASS: TestAccIAMUser_ForceDestroy_serviceSpecificCred (45.66s)
--- PASS: TestAccIAMUser_ForceDestroy_sshKey (45.66s)
--- PASS: TestAccIAMUser_ForceDestroy_mfaDevice (45.70s)
--- PASS: TestAccIAMUser_ForceDestroy_accessKey (45.76s)
--- PASS: TestAccServiceSpecificCredential_multi (51.02s)
--- PASS: TestAccIAMUser_ForceDestroy_loginProfile (51.05s)
--- PASS: TestAccIAMUser_basic (56.20s)
--- PASS: TestAccIAMUser_pathChange (60.71s)
--- PASS: TestAccIAMUser_tags (60.86s)
--- PASS: TestAccIAMUser_nameChange (61.00s)
--- PASS: TestAccServiceSpecificCredential_status (75.02s)
--- PASS: TestAccIAMUser_permissionsBoundary (97.01s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/iam	100.994s
GovCloud
% make testacc TESTARGS='-run=TestAccServiceSpecificCredential_\|TestAccIAMUser_' PKG=iam
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/iam/... -v -count 1 -parallel 20  -run=TestAccServiceSpecificCredential_\|TestAccIAMUser_ -timeout 180m
=== RUN   TestAccServiceSpecificCredential_basic
=== PAUSE TestAccServiceSpecificCredential_basic
=== RUN   TestAccServiceSpecificCredential_multi
=== PAUSE TestAccServiceSpecificCredential_multi
=== RUN   TestAccServiceSpecificCredential_status
=== PAUSE TestAccServiceSpecificCredential_status
=== RUN   TestAccServiceSpecificCredential_disappears
=== PAUSE TestAccServiceSpecificCredential_disappears
=== RUN   TestAccIAMUser_basic
=== PAUSE TestAccIAMUser_basic
=== RUN   TestAccIAMUser_disappears
=== PAUSE TestAccIAMUser_disappears
=== RUN   TestAccIAMUser_ForceDestroy_accessKey
=== PAUSE TestAccIAMUser_ForceDestroy_accessKey
=== RUN   TestAccIAMUser_ForceDestroy_loginProfile
=== PAUSE TestAccIAMUser_ForceDestroy_loginProfile
=== RUN   TestAccIAMUser_ForceDestroy_mfaDevice
=== PAUSE TestAccIAMUser_ForceDestroy_mfaDevice
=== RUN   TestAccIAMUser_ForceDestroy_sshKey
=== PAUSE TestAccIAMUser_ForceDestroy_sshKey
=== RUN   TestAccIAMUser_ForceDestroy_serviceSpecificCred
=== PAUSE TestAccIAMUser_ForceDestroy_serviceSpecificCred
=== RUN   TestAccIAMUser_ForceDestroy_signingCertificate
=== PAUSE TestAccIAMUser_ForceDestroy_signingCertificate
=== RUN   TestAccIAMUser_nameChange
=== PAUSE TestAccIAMUser_nameChange
=== RUN   TestAccIAMUser_pathChange
=== PAUSE TestAccIAMUser_pathChange
=== RUN   TestAccIAMUser_permissionsBoundary
=== PAUSE TestAccIAMUser_permissionsBoundary
=== RUN   TestAccIAMUser_tags
=== PAUSE TestAccIAMUser_tags
=== CONT  TestAccServiceSpecificCredential_basic
=== CONT  TestAccIAMUser_ForceDestroy_mfaDevice
=== CONT  TestAccIAMUser_ForceDestroy_accessKey
=== CONT  TestAccServiceSpecificCredential_multi
=== CONT  TestAccIAMUser_basic
=== CONT  TestAccIAMUser_ForceDestroy_loginProfile
=== CONT  TestAccServiceSpecificCredential_disappears
=== CONT  TestAccIAMUser_permissionsBoundary
=== CONT  TestAccServiceSpecificCredential_status
=== CONT  TestAccIAMUser_ForceDestroy_signingCertificate
=== CONT  TestAccIAMUser_ForceDestroy_serviceSpecificCred
=== CONT  TestAccIAMUser_nameChange
=== CONT  TestAccIAMUser_tags
=== CONT  TestAccIAMUser_ForceDestroy_sshKey
=== CONT  TestAccIAMUser_disappears
=== CONT  TestAccIAMUser_pathChange
--- PASS: TestAccIAMUser_disappears (33.58s)
--- PASS: TestAccServiceSpecificCredential_disappears (37.80s)
--- PASS: TestAccServiceSpecificCredential_basic (43.92s)
--- PASS: TestAccServiceSpecificCredential_multi (44.18s)
--- PASS: TestAccIAMUser_ForceDestroy_loginProfile (46.02s)
--- PASS: TestAccIAMUser_ForceDestroy_accessKey (46.38s)
--- PASS: TestAccIAMUser_ForceDestroy_signingCertificate (46.53s)
--- PASS: TestAccIAMUser_ForceDestroy_sshKey (46.55s)
--- PASS: TestAccIAMUser_ForceDestroy_serviceSpecificCred (46.64s)
--- PASS: TestAccIAMUser_ForceDestroy_mfaDevice (46.96s)
--- PASS: TestAccIAMUser_pathChange (57.01s)
--- PASS: TestAccIAMUser_nameChange (57.04s)
--- PASS: TestAccIAMUser_basic (60.10s)
--- PASS: TestAccIAMUser_tags (60.17s)
--- PASS: TestAccServiceSpecificCredential_status (76.12s)
--- PASS: TestAccIAMUser_permissionsBoundary (104.34s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/iam	107.830s

@ewbankkit ewbankkit merged commit fd082c0 into hashicorp:main Feb 11, 2022
@github-actions github-actions bot added this to the v4.1.0 milestone Feb 11, 2022
@DrFaust92 DrFaust92 deleted the iam_service_spec_creds branch February 12, 2022 12:49
@github-actions
Copy link

This functionality has been released in v4.1.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/iam Issues and PRs that pertain to the iam service. size/XL Managed by automation to categorize the size of a PR. sweeper Pertains to changes to or issues with the sweeper. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: IAM Service Specific credentials
10 participants