-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: Add integration test for STS assume role credentials provider. #1839
Conversation
private func assumeRolePolicy(acctID: String) -> String { | ||
return """ | ||
{ | ||
"Version": "2012-10-17", | ||
"Statement": [ | ||
{ | ||
"Effect": "Allow", | ||
"Principal": { | ||
"AWS": "arn:aws:iam::\(acctID):root" | ||
}, | ||
"Action": "sts:AssumeRole" | ||
} | ||
] | ||
} | ||
""" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider extract this out to a helper function since assume role is also used in IMDS integration test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One in IMDS integ test is slightly different bc that one allows EC2 principal rather than account principal. I think it probably will require a function that needs multiple arguments with messy string concat logic just to handle the two cases.
But if more and more integration tests have this sort of duplicates, we can then consider refactoring integration test suite a bit and adding some utility class that can be used by any test for common uses like these.
private let identityPolicyName = "temp-allow-STS-getCallerIdentity" | ||
// JSON identity policy | ||
private let roleIdentityPolicy = """ | ||
{"Version": "2012-10-17","Statement": [{"Sid": "","Effect": "Allow", | ||
"Action": "sts:GetCallerIdentity","Resource": "*"}]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider creating a constant and moving these to another file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now this identity policy is only used by this test, so it makes sense to have it in same file as the tests for cohesion. When new integration tests down the line use identical policies, then we can consider refactoring it.
Issue #
2140
Description of changes
STSAssumeRoleAWSCredentialIdentityResolver
New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.