-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
d/aws_iam_session_context: New data source #19957
Conversation
} | ||
|
||
re := regexp.MustCompile(`^assumed-role/`) | ||
parsedARN.Resource = re.ReplaceAllString(parsedARN.Resource, "role/") |
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.
This will return the incorrect sourceArn for roles with paths. The sts assumed-role ARN doesn't include the path of the role. For instance, if you have IAM role ARN arn:aws:iam::{accountId}:role/{path}/{roleName}
and you assume that role , the assumed-role ARN will be arn:aws:sts::{accountId}:assumed-role/{roleName}/{sessionName}
. To derive the sourceARN, you would need to call getRole after parsing the role name from the sts ARN. The problem is then, the caller identity data source will then require the iam:GetRole
permission, which could theoretically break workflows.
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.
😞 absolutely right
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.
Some minor comments, otherwise LGTM
@@ -109,3 +111,22 @@ func Policies(conn *iam.IAM, arn, name, pathPrefix string) ([]*iam.Policy, error | |||
|
|||
return results, err | |||
} | |||
|
|||
// RoleByName returns a role's ARN given the role name | |||
func RoleByName(conn *iam.IAM, name string) (*iam.Role, error) { |
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.
Can this just be named Role
? This is just a wrapper on getRole
, and the above RO functions are named simply by 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.
Yes, love it
|
||
// roleNameSessionFromARN returns the role and session names in an ARN if any. | ||
// Otherwise, it returns empty strings. | ||
func roleNameSessionFromARN(rawARN string) (string, string) { |
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.
This function would ideally return an error if there is no session name to grab (i.e. its not a valid sts assumed-role arn), right?
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.
Yes, the problem is that good Go says we should return an error if we don't plan on dealing with it, which we don't.
d.Set("source_arn", arn) | ||
d.Set("session_name", "") | ||
d.Set("role_name", "") | ||
d.Set("role_path", "") |
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.
I don't think returning role_path
is necessary. Seems like an arbitrary field to choose to return from GetRole output. Maybe we could leave it off and if folks want more role context for this datasource we can always add it later?
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.
Removed.
if roleName, sessionName = roleNameSessionFromARN(arn); roleName == "" { | ||
d.Set("source_arn", arn) | ||
d.Set("session_name", "") | ||
d.Set("role_name", "") |
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.
The field name structure for source_arn
and role_name
seems inconsistent. Could we name them both with source
? Or we could even return arn
, role_arn
, role_name
and session_name
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.
the challenge here is if you're using an IAM user, you should still be able to get out a passed through user ARN from source_arn
.
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.
Yeah I see what you are saying. Don't love it, but can't think of a better way to do it.
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.
issuer_arn
?
return "", "" | ||
} | ||
|
||
if reRole.MatchString(parsedARN.Resource) && parsedARN.Service != "iam" { |
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.
My take on this is we should spit IAM role ARNs out just like we would any non-sts assumed-role ARN. If they want metadata on a role, they should use the aws_iam_role
data source.
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.
Since we have the role name, I figure I'd also verify IAM role - no cost.
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.
Its not costly, but I think it muddies the water a little on what this data source does. Its easiest to say if you pass in an assumed-role arn we'll give you back all of the metadata and if not you only get sourceArn. Once you add in a case for IAM role, I think the use case isn't quite as clear.
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.
Yeah, I can see that.
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.
Water un muddied
.changelog/19957.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:new-data-source | |||
aws_iam_assumed_role_source |
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.
aws_iam_assumed_role_source | |
aws_iam_session_context |
Type: schema.TypeString, | ||
Computed: true, | ||
}, | ||
"source_arn": { |
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.
Swap with session_name
for alphabetic ordering?
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.
it's now called issuer_name
and should be in the correct order
Tests after many refactors: GovCloud:
|
Co-authored-by: Kit Ewbank <Kit_Ewbank@hotmail.com>
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.
LGTM 🚀.
Commercial
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMSessionContext_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDataSourceIAMSessionContext_ -timeout 180m
=== RUN TestAccAWSDataSourceIAMSessionContext_basic
=== PAUSE TestAccAWSDataSourceIAMSessionContext_basic
=== RUN TestAccAWSDataSourceIAMSessionContext_withPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_withPath
=== RUN TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== RUN TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== RUN TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== CONT TestAccAWSDataSourceIAMSessionContext_basic
=== CONT TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== CONT TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== CONT TestAccAWSDataSourceIAMSessionContext_withPath
=== CONT TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (14.35s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (16.05s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (17.17s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (17.21s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (17.32s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 21.124s
GovCloud
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMSessionContext_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDataSourceIAMSessionContext_ -timeout 180m
=== RUN TestAccAWSDataSourceIAMSessionContext_basic
=== PAUSE TestAccAWSDataSourceIAMSessionContext_basic
=== RUN TestAccAWSDataSourceIAMSessionContext_withPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_withPath
=== RUN TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== RUN TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== RUN TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== CONT TestAccAWSDataSourceIAMSessionContext_basic
=== CONT TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== CONT TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== CONT TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== CONT TestAccAWSDataSourceIAMSessionContext_withPath
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (18.53s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (23.04s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (23.36s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (23.51s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (23.69s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 28.284s
Verified that one of our nightly CI tests that fails with an assumed role passes using data "aws_caller_identity" "current" {}
data "aws_iam_session_context" "current" {
arn = data.aws_caller_identity.current.arn
} % make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpcEndpointServiceAllowedPrincipal_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpcEndpointServiceAllowedPrincipal_ -timeout 180m
=== RUN TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== PAUSE TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== CONT TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
resource_aws_vpc_endpoint_service_allowed_principal_test.go:19: Step 1/1 error: Error running apply: exit status 1
2021/06/28 14:59:22 [DEBUG] Using modified User-Agent: Terraform/0.12.31 HashiCorp-terraform-exec/0.13.3
Error: Error creating VPC Endpoint Service allowed principal: InvalidPrincipal: Invalid Principal: 'arn:aws:sts::123456789012:assumed-role/terraform_team1_dev-developer/abc@example.com'
status code: 400, request id: e1a5d963-8952-41dd-a2dd-aa120a0ee9d2
on terraform_plugin_test.tf line 63, in resource "aws_vpc_endpoint_service_allowed_principal" "test":
63: resource "aws_vpc_endpoint_service_allowed_principal" "test" {
--- FAIL: TestAccAWSVpcEndpointServiceAllowedPrincipal_basic (258.09s)
FAIL
FAIL github.com/terraform-providers/terraform-provider-aws/aws 261.617s
FAIL
make: *** [testacc] Error 1 % make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpcEndpointServiceAllowedPrincipal_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpcEndpointServiceAllowedPrincipal_ -timeout 180m
=== RUN TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== PAUSE TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== CONT TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
--- PASS: TestAccAWSVpcEndpointServiceAllowedPrincipal_basic (227.45s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 230.947s |
This functionality has been released in v3.48.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! |
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. |
Community Note
Closes #19958
Special thanks to @dpowley for helping storyboard a solution!
Output from acceptance/unit testing (
us-west-2
):Output from acceptance/unit testing (GovCloud):