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

S3 plugin #995

Closed
wants to merge 5 commits into from
Closed

S3 plugin #995

wants to merge 5 commits into from

Conversation

Shubham8287
Copy link
Contributor

@Shubham8287 Shubham8287 commented Jun 11, 2023

feat: update provisioner.proto to add methods for provisioning storage

feat: implemented provision_storage to create a bucket and return user credentials to access the bucket

This is a provisioner side change for task /attempt #668, Steps for provisioning storage look like -

Create a S3 bucket by appending an uuid to project_name to make it globally unique.
Create a User to interact with the bucket
Create a Policy and attach that to User
Return access keys for the user, so that resource builder can access the bucket.

* feat: update provisioner.proto to add methods for provisioning storage

* feat: implmented provision_storage to create bucket and return user credential to access bucket
@Shubham8287 Shubham8287 marked this pull request as draft June 11, 2023 20:03
@Shubham8287
Copy link
Contributor Author

draft implementation of plugin for #668

@jonaro00
Copy link
Member

I suppose this is paired with #917

@Shubham8287
Copy link
Contributor Author

yes but not ready to review yet :)

@iulianbarbu
Copy link
Contributor

Hey @Shubham8287 ! This PR slipped from our list of priorities lately. We'll be a bit low on bandwidth in the next weeks because of a bigger release and an offsite happening shortly, but we'll try to catch up on this in the meantime.

We thought that this PR will have a common portion shared with #910, regarding the AWS credentials management, so would be great to have alignment between the two. I haven't personally looked that much into how this is done on both PRs, and it would be great if you can share any thoughts you formed on this subject. Also, please check out #910 approach, would be great if you can spot something there that is conflicting with this PR needs, if any.

@Shubham8287
Copy link
Contributor Author

Shubham8287 commented Jun 17, 2023

We thought that this PR will have a common portion shared with #910, regarding the AWS credentials management, so would be great to have alignment between the two

@iulianbarbu thanks for making me aware of this PR, My approach -s3-plugin and the one mentioned in feat: add dynamodb resource is exactly same i.e to create identity, create policy and attach them, I will make changes to re-use aws cred code once that PR gets merged.

Also, through reading the PR, I am able to self-review my code to some extent which looks straight to the path as of now, I am happy about that. Would love to get my PR in the shuttle :).

@Shubham8287
Copy link
Contributor Author

I am holding this PR for #910 as it shares a common portion and I think that user policy part has some security concerns as well. Would resume it once we get other one merged or some better approach. Let me if this is fine to you folks.

@iulianbarbu
Copy link
Contributor

Thanks @Shubham8287! Indeed, at this time we have some security concerns that required postponing the DynamoDB/S3 PRs. We need to go back to the drawing board and see what architecture/design changes would need to be done to accommodate integrating these resources with Shuttle. BTW, thanks a lot for staying close to S3/DynamoDB development track. We'll try to give more updates about the way forward once we'll plan this internally.

@iulianbarbu
Copy link
Contributor

Closing this. Context provided here: #668 (comment).

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