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

Add optional param for AWS IAM role assumption #143

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

hhamalai
Copy link
Contributor

@hhamalai hhamalai commented Nov 5, 2020

Fixes #142

Proposed Changes

  • add flag to specify role arn
  • if role arn is specified, use access keys to assume the role with arn provided

Description

Allows specifying role ARN which, if specified, is assumed with STS. All S3 operations are executed with this role.

Checklist

  • Read the CONTRIBUTING document.
  • Read the CODE OF CONDUCT document.
  • Ensure your code follows the code style of this project.
  • Ensure CI and all other PR checks are green OR
    • Code compiles correctly.
    • Created tests which fail without the change (if possible).
    • All new and existing tests passed.
  • Add your changes to Unreleased section of CHANGELOG.
  • Improve and update the README (if necessary).

Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Looking good. Thanks a lot for the contribution.

Is there way to test thing with our setup? We have minio, it'd be good to have tests.

storage/backend/s3/config.go Outdated Show resolved Hide resolved
@hhamalai
Copy link
Contributor Author

hhamalai commented Nov 6, 2020

Is there way to test thing with our setup? We have minio, it'd be good to have tests.

I didn't see any authentication related tests in the code, at least for AWS. What's your take on mocked unit tests, that would be plausible, but now when thinking about it, I can also check the minio support for role assumption as documented here https://github.com/minio/minio/blob/master/docs/sts/assume-role.md

@kakkoyun
Copy link
Contributor

kakkoyun commented Nov 6, 2020

@hhamalai
We have rudimentary happy path tests in the form of high-level e2e tests in here

func setupS3(t *testing.T, c *Config, name string) {

What we can either extend those or even better add a specific case in here https://github.com/meltwater/drone-cache/blob/master/storage/backend/s3/s3_test.go

Even a simple one to prevent regressions would suffice.

@hhamalai hhamalai force-pushed the add_assume_role_param branch 6 times, most recently from 86f4bd5 to 431299d Compare November 9, 2020 16:47
@hhamalai
Copy link
Contributor Author

hhamalai commented Nov 9, 2020

Hi, I added the test case, unfortunately it required quite a bit of too many changes. I am still not too familiar with minio, so take the following findings with a grain of salt:

  1. need newer version minio in order to support STS operations
  2. need a creation of users an policies on minio as the admin user cannot use STS (in docker-compose & drone yaml)

kakkoyun
kakkoyun previously approved these changes Nov 10, 2020
Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Wow, what a great PR! Thank you very much for this contribution and level of code quality.

It looks good to me. I have only requested a single change concerning package imports. If you can fix it I'll merge this immediately.

storage/backend/s3/s3_test.go Outdated Show resolved Hide resolved
storage/backend/s3/s3_test.go Outdated Show resolved Hide resolved
Upgrade minio version and add user & policy initialization

Fix permissions

Organize imports
@kakkoyun kakkoyun merged commit f32236f into meltwater:master Nov 10, 2020
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.

Allow specifying AWS IAM role to assume with IAM user keys
2 participants