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

Use Credentials (similar to minio-go) to enable AWS IAM #817

Merged
merged 21 commits into from
Jan 23, 2020

Conversation

febg
Copy link
Contributor

@febg febg commented Oct 29, 2019

First attempt to fix #814 it gives an idea about the design and how I tried to follow the minio-go implementation

@febg
Copy link
Contributor Author

febg commented Nov 5, 2019

@harshavardhana Any updates on feedback for this?

@harshavardhana
Copy link
Member

Not yet, we are currently busy with other priorities we will get back to you soon @febg

@hootener
Copy link

@harshavardhana This PR was submitted by the Codecov team. We make frequent use of this library in our Enterprise product and would love to bring minio-py's credential management code up to par with other minio SDK implementations (go, etc.).

We actively field minio (and minio-py) as a storage interface solution for Codecov Enterprise to some of our largest enterprise clients. We're submitting this PR on their behalf as AWS IAM support is routinely cited as a bottleneck to minio adoption by these companies. Is there anything we can do on our end to help expedite the review of this PR? Happy to help in any way we can.

@harshavardhana
Copy link
Member

@hootener there is nothing you need to do, we internally are neck deep busy with some deliverables and haven't found sufficient time to spend on this.

We would need a minimum of another 3 weeks before we can review this PR unfortunately.

@hootener
Copy link

@harshavardhana That's fine.

Even the ballpark estimate of when work can start gives me something to take back. So I'll let other stakeholders know there's at least a three week backlog before this can be reviewed. I'll check back for traction on this PR then. Thanks.

minio/api.py Outdated Show resolved Hide resolved
minio/credentials/aws_iam.py Outdated Show resolved Hide resolved
minio/credentials/file_aws_credentials.py Show resolved Hide resolved
@harshavardhana
Copy link
Member

@febg can you get this PR out of WIP and ready for final review? or do you wish to iterate on this further?

@febg febg changed the title [WIP] Use Credentials (similar to minio-go) to enable AWS IAM Use Credentials (similar to minio-go) to enable AWS IAM Dec 19, 2019
@febg
Copy link
Contributor Author

febg commented Dec 19, 2019

@harshavardhana @devgrok should be ready for final review! Thanks!

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Some comments, not tested yet

minio/api.py Outdated Show resolved Hide resolved
minio/credentials/file_minio_client.py Outdated Show resolved Hide resolved
@Praveenrajmani
Copy link
Collaborator

@febg Can you add an example in examples/ folder?

@febg
Copy link
Contributor Author

febg commented Jan 2, 2020

@Praveenrajmani should I add one example per every credentials provider?

Eg.

  • aws_iam_crendentials_example
  • chain_credentials_example
  • env_minio_credentials_example
  • file_minio_credentials_example
  • env_aws_credentials_example
  • etc...?

I was thinking on only doing aws_iam_crendentials and chain_credentials examples since it gets repetitive and it is quite intuitive

@harshavardhana
Copy link
Member

@Praveenrajmani @febg two examples should be enough for now.

minio/api.py Show resolved Hide resolved
Praveenrajmani
Praveenrajmani previously approved these changes Jan 9, 2020
Copy link
Collaborator

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

If we are not supporting IMDSv2, can we mention that in the docs? Or validate the version if possible @febg? LGTM otherwise

@febg
Copy link
Contributor Author

febg commented Jan 10, 2020

@devgrok I applied your patch! Thanks for figuring it out!

Praveenrajmani
Praveenrajmani previously approved these changes Jan 22, 2020
Copy link
Collaborator

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

LGTM

@Praveenrajmani
Copy link
Collaborator

PTAL @vadmeste @harshavardhana

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

Apart from license header fixes the rest of the PR LGTM - thanks for your patience @febg - we really appreciate all the work 🍰

examples/aws_credentials.py Outdated Show resolved Hide resolved
examples/chain_credentials.py Outdated Show resolved Hide resolved
minio/credentials/__init__.py Outdated Show resolved Hide resolved
minio/credentials/aws_iam.py Outdated Show resolved Hide resolved
minio/credentials/chain.py Outdated Show resolved Hide resolved
minio/credentials/static.py Outdated Show resolved Hide resolved
tests/unit/chain_provider_test.py Outdated Show resolved Hide resolved
tests/unit/credentials_test.py Outdated Show resolved Hide resolved
tests/unit/env_aws_provider_test.py Outdated Show resolved Hide resolved
tests/unit/env_minio_provider_test.py Outdated Show resolved Hide resolved
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.

Use Credentials (similar to minio-go) to enable AWS IAM
7 participants