Compose full IAM ARN from role name or alias #169
Conversation
|
@dims @nckturner @jaypipes @micahhausler hey 👋🏻, could someone take a look at this PR? We'd like to contribute, we believe this might be helpful for others 🙂. The idea is to set only the role name in case you're not assuming a cross-account role, but it doesn't do harm if you would still leave the account id in the annotation. PTAL 🙇🏻 |
|
Can you squash the commits into reviewable chunks with vendor separate please? @zewolfe |
pkg/cache/cache.go
Outdated
|
|
||
| accountId = identity.AccountID | ||
| if strings.Contains(identity.Region, "cn-") { | ||
| partition = "aws-cn" |
There was a problem hiding this comment.
Needs to account for other paritions (aws-us-gov, aws-iso and aws-iso-b)
There was a problem hiding this comment.
Thanks will update them, we were not aware of all the partitions 😅
There was a problem hiding this comment.
There was a problem hiding this comment.
I added the other partitions.
main.go
Outdated
| metadataClient := ec2metadata.New(sess) | ||
| identity, err := metadataClient.GetInstanceIdentityDocument() | ||
| if err != nil { | ||
| klog.Fatalf("Error getting instance identity document: %v", err.Error()) |
There was a problem hiding this comment.
Does this introduce a new required dependency on EC2 metadata where there wasn't one before?
There was a problem hiding this comment.
Correct, this is a new dependency. We need to call instance identity document in order to get the account ID
There was a problem hiding this comment.
Can we only fail on this dependency if the feature is enabled?
There was a problem hiding this comment.
Ok should be done, I just wrapped a if condition around it 🙂
|
Thanks again for your input, I hope I addressed your comments. PTAL again ❤️ |
|
@nckturner @dims could you take a look again please? |
|
@nckturner @dims I know you're super busy, I'd would really like to see this move. Is there any chance you have a couple of minutes to have a look again or you might point me to another person who can take a look, I don't want to waste your time but I'm a bit clueless here how we can move forward. Thank you again ❤️ |
|
cc @nnmin-aws |
|
thanks @dims! I will review it today. |
|
/lgtm |
main.go
Outdated
| *tokenExpiration, | ||
| saInformer, | ||
| cmInformer, | ||
| identity, |
There was a problem hiding this comment.
Passing the entire instance identity document to the cache constructor seems excessive and leaky, can we instead create a struct which includes all the data required for this feature, called something like ComposeRoleArn, which contains Enabled, Region, Partition and AccountId?
There was a problem hiding this comment.
Ok I changed it slightly, I hope this is better now.
|
@njuettner Thanks for the update! Sorry I've been unresponsive, I have one more piece of feedback for you to address. |
|
@nckturner Addressed the feedback, thank you 👍🏻 |
|
@nckturner sorry for pinging you again, can you take a look again please? |
|
@njuettner thanks for doing that, taking a look. |
|
@nckturner Awesome, thank you 👍🏻. I committed your two suggestions. |
|
@nckturner if you have time again to have a final look that would be awesome 🙂 |
|
@nckturner any news 🙏🏻? |
|
@nckturner do you mind having a look? I think we can merge this PR 🙂 |
|
@njuettner apology for the delay as Nick is on leave. I am checking it. |
|
/lgtm |
|
@dims Could you please kindly help merge this PR as Nick is on leave? thank you! |
|
@nnmin-aws running the tests one last time |
|
@zewolfe can you please rebase and fix the broken test? |
Co-authored-by: Nicholas Turner <1205393+nckturner@users.noreply.github.com>
Co-authored-by: Nicholas Turner <1205393+nckturner@users.noreply.github.com>
|
@dims please take a look again 👍🏻 |
|
thanks @njuettner |
|
thank you @dims and @nckturner ❤️ |
|
and @nnmin-aws :) |
|
Hello @nnmin-aws and @dims, would it be possible to create a release with this change? |
|
apology for your inconvenience. we will work on this and let you know when new release is out |
|
@calvix the new release v0.5.0 is out. thank you |
Issue #, if available:
Reference: #78
Description of changes:
This PR introduces the ability to create a fully formed IAM ARN if just the role alias/name is specified. For example an input of
s3-readerwill result inarn:aws:iam::111122223333:role/s3-reader. The account ID is fetched from ec2 MetadataBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.