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

feat: Add the ability to use a web identity token file #240

Merged
merged 9 commits into from
Aug 3, 2021

Conversation

nesta219
Copy link
Contributor

Issue #, if available: :

#124

Description of changes:

I would like to be able to use a github action to consume a web identity token file on an EKS worker node. I have a self-hosted runner which does not run as root and thus it does not automatically get IRSA credentials, so my current workaround is to use a bash script which handles the assume role portions, but it doesn't neatly export the credentials the way this workflow does. This pull request would allow users of this workflow to consume the web identity token file when assuming a role in a much cleaner fashion.

@paragbhingre
Copy link
Contributor

@nesta219 Thank you for contributing this feature. We will take a look at it and get back to you soon.

action.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@allisaurus allisaurus left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, @nesta219 !

Aside from light documentation feedback, I'm hoping you can add error handle re: missing files, and also wondering if you can provide some evidence of fully testing the change?

NOTE: If you're not familiar with how to run actions from a fork, it should just take running npm package, pushing the result to your fork as a new commit, and pointing your actions workflow at nesta219/configure-aws-credentials@master (but I'd suggest doing this in a branch other than the one associated with this PR, since our merge automation will attempt to generate the dist for you once it's approved)

action.yml Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@nesta219
Copy link
Contributor Author

Very reasonable suggestions, I will update the docs and I'll test using the action from my fork on a real EKS node as well

@nesta219
Copy link
Contributor Author

@allisaurus updated based on your feedback, and I also tested this with a real self-hosted github actions runner in EKS and was able to assume the web identity role using the token file

@nesta219
Copy link
Contributor Author

Screen Shot 2021-07-29 at 3 26 28 PM

@nesta219
Copy link
Contributor Author

Note in the screen shot that the decisions to use role-duration-seconds: 3600 and role-skip-session-tagging: true were made because of some restrictions that our runner role has, and are not related to the actual GH action itself

@nesta219
Copy link
Contributor Author

nesta219 commented Jul 29, 2021

One further comment:
The nodejs assumeRoleWithWebIdentity function doesn't support the Tags parameter (the tags come from the JWT stored in the web identity token file as opposed to the request body). If you try to pass Tags, it will error with Unexpected key 'Tags' found in params, so I adjusted the logic to exclude this parameter from the request if we are using a web identity token file.

@nesta219 nesta219 requested a review from allisaurus July 30, 2021 13:02
Copy link
Contributor

@allisaurus allisaurus left a comment

Choose a reason for hiding this comment

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

Nice catch re: tags and thanks very much for the testing @nesta219 !
I have one comment re: rearranging the logic for removing/logging the lack of tags, but overall this looks great and I'm excited to get it in.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@nesta219 nesta219 requested a review from allisaurus August 2, 2021 13:44
Copy link
Contributor

@allisaurus allisaurus left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Thanks again for working on this @nesta219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants