Skip to content

FEATURE REQUEST: Support for direct uploading of files to S3 #3530

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

Closed
kcoop opened this issue Feb 19, 2017 · 22 comments
Closed

FEATURE REQUEST: Support for direct uploading of files to S3 #3530

kcoop opened this issue Feb 19, 2017 · 22 comments

Comments

@kcoop
Copy link

kcoop commented Feb 19, 2017

A VERY significant amount of processing time on my servers is spent uploading and forwarding files to S3 using the S3 adapter. These uploaded files need no processing, so it's really wasteful to involve the parse server.

It would be great to have a mechanism between the client and server whereby the client PFFile or ParseFile sent a request to parse server for a destination URL and credentials, then did a direct POST to the destination URL.

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

This might also work well in conjunction with #3056.

@flovilmart
Copy link
Contributor

How do you suggest we keep those credentials safe? I'm all open for suggestions. Perhaps using the body stream would be more efficient instead of consuming the request.

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

Haven't fully researched it, but I don't think you'd need to. Amazon provides temporary authorization through Security Token Services: http://docs.aws.amazon.com/STS/latest/APIReference/Welcome.html

@flovilmart
Copy link
Contributor

flovilmart commented Feb 19, 2017

but I don't think you'd need to

the S3 identity responsible for uploading content to your bucket should remain private (unless you provide short lived access). Besides, if you want to bypass parse-server, you'd need to implement an S3 client in your app, provide the tokens securely to the clients (or the identity with write access to the bucket, probably involving a secret).

I'm open for it, #3056 would be most likely required, actually, with that PR, nothing else would be required. I'm not sure we'd modify all the SDK's to include this behaviour.

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

I guess you'd expose the end point in the destination URL (no more than read access), but not the identity. If I understand correctly, the whole point of STS is to provide temporary auth tokens that don't expose the identity itself. The server would use its private identity to request short lived single use tokens, then pass them along to the client.

If you just went with #3056, then implementing this would require embedding the S3 identity in the client, a serious security hole.

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

I recognize this describes work spanning both the server and the clients. It would be a big performance boost for those of us using S3 however.

@flovilmart
Copy link
Contributor

There's nothing better than the S3 SDK to upload to S3, I'm not sure adding a dependency to ALL client SDK's to the S3 SDK is a terrific idea.

However, #3056 would likely enable an upload to pretty much anything and then keep a reference in parse-server as a PFFile.

Then you can implement any security you want around your S3 credentials, putting AWS_KEY/SECRET in your bundle, serve your S3 credentials from the config or a cloud function that would be responsible to negociate short lived tokens.

what do you think?

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

Mmm, good point about the S3 SDK dependency. Heavy.

With what you're describing though, everyone ends up rewriting the same thing, losing all the niceties of PFFile uploading, and adding the learning curve of S3 & STS. It would be great if there was a drop in component on the client that included the S3 SDK, and added this functionality transparently to PFFile/ParseFile. I could imagine a similar one for Azure, etc.

@flovilmart
Copy link
Contributor

the component you're talking about is a one-liner function that wraps the S3 SDK upload call and produces a URL... I'M not sure that 'deserves' a module, nor but feel free to implement one :)

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

But it isn't integrated with PFFile, you'd have to write a cloud function that managed the STS tokens, etc. It's not a one-liner.

@flovilmart
Copy link
Contributor

But it isn't integrated with PFFile

It would be 'trivial' to integrate it a-posteriori, wether it's iOS, or JS through extension for the former or prototype for the latter.

TBH, I have some trouble wrapping my head around it, and I am not sure of the necessity of the parse-server close integration. Can you tell me why this absolutely should belong to the SDK's and parse-server because I don't see it now.

However I'm fully open for facilitating such feature, that's why I highly consider #3056, if you wanna get started on it, I would happily review it and merge it ASAP.

@flovilmart
Copy link
Contributor

@kcoop AFAICT, this would require client side integration more than server side, what you need is on iOS for ex:

  • the ability to set the 'url' on a PFFile
  • a custom upload controller to fit your needs

This custom S3 upload controller should be configured with your own needs, that falls completely out of the scope of the server project.

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

Yes, exactly, it would be mostly done by the clients. A pluggable upload controller is what I meant by component on the clients.

From the server side though, it would still be useful to have a pluggable STS token generator. I imagine having a cluster of components for each file service: Android, iOS, and Javascript client controllers, and a server side token generator that worked in concert with the file module.

I'll look into making the upload controller pluggable in the different client projects, and see if there are any gotchas in making the url settable. For the server, any pointers for best practices for extending the S3Adapter in a separate module?

@flovilmart
Copy link
Contributor

flovilmart commented Feb 19, 2017

I don't see it as part of the server, for me that's probably a cloud function, or an HTTP endpoint, or a gPRC call, or anything else, but I don't see how it fits within parse-server's architecture and goal.

The fact that's you're using a short lived token is an implementation detail for S3. Your client upload controller will be pretty much isolated from Parse's ecosystem after all.

ex in Swift:

class S3Uploader {
    static let shared = S3Uploader()
    func upload(_ data: Data, toURL url: URL) -> BFTask<Bool> {
      let task = BFTask()
      negociateToken() { uploadToken in 
          upload(data, toURL: url, withToken: uploadToken) { (err, res) -> Void in
              if err != nil { task.resolve(true) } 
              else { task.reject(err) }
          }
      }
      return task
    }
}

extension PFFile {
   func save(withUploader uploader: S3Uploader = .shared) -> BFTask<PFFile> {
     let data = data
     return uploader.save(data, toURL: ...).continueWithBlock {
       return self
     }
   }
}

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

Ok, that's a workable one-off for my needs, since I'm already doing a lot of the other generalized work done in PFFile/ParseFile that this bypasses (e.g. progress updates and retry management).

It would be better of course to have the client libraries allow for a pluggable loader inserted into all the existing upload calls. And it would be nice to have the server side STS management be reusable - this is pretty general purpose for anyone using AWS.

Thanks for your help!

@flovilmart
Copy link
Contributor

And it would be nice to have the server side STS management be reusable - this is pretty general purpose for anyone using AWS.

Cloud functions are very indicated for that:

Parse.Cloud.define('stsToken', require('sts-token-provider'));

on iOS:

let s3Uploader = new S3Uploader(stsTokenFunction: 'stsToken')

I'm all for providing supporting a custom controller for uploading, I'M not sur I have the bandwidth for implementing it. If you're moving in that direction, I will welcome those changes :)

@kcoop
Copy link
Author

kcoop commented Feb 19, 2017

It's now on my (rather long) list. I'll let you when I have something concrete.

@flovilmart
Copy link
Contributor

Awesome! As this will ultimately land on the client SDK's I'm closing it now.

@kcoop
Copy link
Author

kcoop commented Feb 23, 2017

I've now implemented this for my own needs, and created a branch of the iOS SDK that adds a necessary pluggable uploader protocol PFFileUploadController and a property on PFFile (PFFile does local caching behind the scenes after an upload, which can't easily be managed with an externally defined category). Here's a link to my branch: https://github.com/kcoop/Parse-SDK-iOS-OSX/tree/upload-controller

It looks like the client projects are a bit rudderless right now, so I haven't submitted a pull request. I'd love to see this integrated - I hate forking. If you are interested in helping shepherd this on the clients, I can create gists with working code: I've implemented an STS authenticated uploading implementation of the protocol for iOS, and a cloud function module that implements the STS token retrieval, but don't have the time right now to package either of them.

I'll also be doing the same for Android when I get there.

@flovilmart
Copy link
Contributor

Why not open a PR on the iOS SDK?

@kcoop
Copy link
Author

kcoop commented Feb 24, 2017

The project looks a little... distracted right now. parse-community/Parse-SDK-iOS-OSX#1101

I'm very busy, and if it's just going to sit there...

@flovilmart
Copy link
Contributor

I have commit access to the repo. Some things are worth giving attention to it, and your PR is.
You've actually worked on something pretty meaningful, so let's go with it.

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

No branches or pull requests

3 participants
@flovilmart @kcoop and others