Add S3 objectstore backend

[icewind1991]

For using s3 as a primary storage

Also improves tests for primary objectstore by testing for ObjectStoreStorage independent from the backends

[mention-bot]

@icewind1991, thanks for your PR! By analyzing the annotation information on this pull request, we identified @butonic, @PVince81, @berendt and @Xenopathic to be potential reviewers

[MariusBluem]

Isnt that related to https://github.com/nextcloud/files_primary_s3 😁 @MorrisJobke

[MorrisJobke]

Isnt that related to https://github.com/nextcloud/files_primary_s3 😁 @MorrisJobke

I think most of it was only in one class and properly use more code in both cases makes sense ;)

[icewind1991]

@LukasReschke @MorrisJobke please review

[berendt]

I am happy to help with a OpenStack Swift integration. But I am not familiar with Amazon S3.

[rullzer]

I know it is the case for Swift as well. But could we please have this in a different app or something? I really would like to not have the amazon SDK as a 3rdparty dep for our main component (I would really love to rip out opencloud as well).

[icewind1991]

The main issue I can see with splitting this off is that both files_s3 (the would be new app) and files_external would both need the aws sdk as dependency, having them both ship with it adds maintenance work since both would be need to be kept in sync to prevent issues

[MariusBluem]

Can we copy the S3 SDK over to "3rdparty" as a single dependency which would provide the SDK for both 😁

[MorrisJobke]

Can we copy the S3 SDK over to "3rdparty" as a single dependency which would provide the SDK for both 😁

the problem then is, that an app depends on a 3rdparty library of core which is a major issue once you want to update the app independent from the core.

[MariusBluem]

But isnt the app in the core (so we can not update it Independent) ... 😁
...will there be a Migration path for local Data to S3?

[MariusBluem]

...any news on this? 😁 We've some requests about that in the community (forum,...) 😉

[Spacefish]

Guess files_external is part of the core. We should introduce a function in
files_external which scans for external storage drivers in other apps as
well. That would introduce inter app dependency though...
I wrote a Cassandra FS external storage driver (not ready yet) but I had to
put everything into files_external as well... Which is bad as you need PHP
extensions for it to work...

Marius Blüm notifications@github.com schrieb am Mo., 3. Okt. 2016, 16:48:

...any news on this 😁 We've some requests about that in the community
(forum,...) 😉

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#768 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAW7UZTeDXeu1uuGxAGmrm6Q0NihzBt5ks5qwRW9gaJpZM4JfEmS
.

[icewind1991]

I wrote a Cassandra FS external storage driver (not ready yet) but I had to
put everything into files_external as well... Which is bad as you need PHP
extensions for it to work...

Apps can already register new backends for external storage

see https://github.com/owncloud/files_external_ftp for an example

[benmichael]

Has S3 server side encryption been considered? It's easy to implement mostly just being a header x-amz-server-side-encryption on the PutObject request.

It means that the encryption module isn't need for encrypting data at rest for anyone who uses this, which solves the object store/encryption module overhead issue by simply removing the need for the encryption module.

I'm sure that a number of larger customers would find this extremely beneficial.

[jospoortvliet]

For those who would like to test this feature, this blog post has some simple steps for how to do it:
http://blog.jospoortvliet.com/2016/01/patching-owncloud-get-your-fix-now.html

Without testing, we can't really get this merged, sadly, so please - give it a try if you are interested in this feature!

[benmichael]

I set up a test instance, set up some theming, confirmed that all was working, then pulled the patch into it.

Files seems to work perfectly. I can add, remove, restore, share. One thing I noticed is that "new file" has disappeared. New directory and upload file are still there though.

Some issues encountered though:

• My themed logo and login background disappeared
  • The default works
• "new file" has disappeared when I click the upload button
  • New directory and upload file are still there
• All directories have the external directory icon, this should not be the case for primary storage
• Gallery app is gone. I understand that this is expected behaviour, but it's a sore loss.

Hope that helps

[icewind1991]

All directories have the external directory icon, this should not be the case for primary storage

My themed logo and login background disappeared

Great finds, should be fixed now

[icewind1991]

"new file" has disappeared when I click the upload button

Can you check if the texteditor app is still enabled (that provides that button)

[icewind1991]

Gallery app is gone. I understand that this is expected behaviour, but it's a sore loss.

Gallery still works for me, maybe it got disabled on accident

[benmichael]

@icewind1991 Great work. You were correct about the text and gallery apps, I think I did something stupid there. So the following is still giving me trouble:

• I can see the create new file, however it fails on create.
  • I can edit and save an existing text file
• The logo still isn't working
  • I uploading a logo just spins forever
  • If I disable s3 it uploads instantly
  • If I reenable with a logo in place, the logo disappears as before

Both of these feel like a permissions issue, however my IAM user has AmazonS3FullAccess permissions, so it's not on S3's side.

Below is what the log spits out:

Error index Exception: {"Exception":"OCP\\Files\\NotFoundException","Message":"","Code":0,"Trace":"#0 \/var\/www\/sthree\/lib\/private\/Files\/Node\/Node.php(223): OC\\Files\\Node\\Node->getFileInfo()\n#1 \/var\/www\/sthree\/lib\/private\/Files\/Node\/Node.php(106): OC\\Files\\Node\\Node->getPermissions()\n#2 \/var\/www\/sthree\/lib\/private\/Files\/Node\/File.php(92): OC\\Files\\Node\\Node->checkPermissions(3)\n#3 \/var\/www\/sthree\/apps\/theming\/lib\/Controller\/ThemingController.php(189): OC\\Files\\Node\\File->fopen('w')\n#4 [internal function]: OCA\\Theming\\Controller\\ThemingController->updateLogo()\n#5 \/var\/www\/sthree\/lib\/private\/AppFramework\/Http\/Dispatcher.php(160): call_user_func_array(Array, Array)\n#6 \/var\/www\/sthree\/lib\/private\/AppFramework\/Http\/Dispatcher.php(90): OC\\AppFramework\\Http\\Dispatcher->executeController(Object(OCA\\Theming\\Controller\\ThemingController), 'updateLogo')\n#7 \/var\/www\/sthree\/lib\/private\/AppFramework\/App.php(111): OC\\AppFramework\\Http\\Dispatcher->dispatch(Object(OCA\\Theming\\Controller\\ThemingController), 'updateLogo')\n#8 \/var\/www\/sthree\/lib\/private\/AppFramework\/Routing\/RouteActionHandler.php(47): OC\\AppFramework\\App::main('OCA\\\\Theming\\\\Con...', 'updateLogo', Object(OC\\AppFramework\\DependencyInjection\\DIContainer), Array)\n#9 [internal function]: OC\\AppFramework\\Routing\\RouteActionHandler->__invoke(Array)\n#10 \/var\/www\/sthree\/lib\/private\/Route\/Router.php(293): call_user_func(Object(OC\\AppFramework\\Routing\\RouteActionHandler), Array)\n#11 \/var\/www\/sthree\/lib\/base.php(983): OC\\Route\\Router->match('\/apps\/theming\/a...')\n#12 \/var\/www\/sthree\/index.php(48): OC::handleRequest()\n#13 {main}","File":"\/var\/www\/sthree\/lib\/private\/Files\/Node\/Node.php","Line":86} 2016-10-27T15:05:04+00:00 sthreetest Error objectstore Could not create object: A header you provided implies functionality that is not implemented 2016-10-27T15:05:04+00:00 sthreetest Error PHP file_exists() expects parameter 1 to be a valid path, resource given at /var/www/sthree/lib/public/AppFramework/Http/StreamResponse.php#57 2016-10-27T15:04:32+00:00 sthreetest

[icewind1991]

Text editor issues should be fixed now

With theming I was able to reproduce it, added some logging and then everything worked fine ☹️ so I'm not sure if that's fixed now or not, could you retest that part.

and thanks for the through testing @benmichael

[benmichael]

@icewind1991 I'm afraid this latest patch won't patch correctly. Not sure if the problem is on my side or not.. I tried a clean git pull from master, then tried patching, and got the below:

Edit: just confirmed the issue with the patch using a fresh checkout at the tip of master (HEAD), at the v10.0.1 tag, and with a straight tar.bz2 download. Same issue, shown below, on all three. Is there a specific branch or commit I'm supposed to be patching against?

$ patch -p1 --dry-run < 768.patch 
checking file apps/files_external/lib/Lib/Storage/AmazonS3.php
checking file lib/private/Files/ObjectStore/S3ConnectionTrait.php
checking file lib/private/Files/ObjectStore/StorageObjectStore.php
checking file tests/lib/Files/ObjectStore/LocalTest.php
checking file tests/lib/Files/ObjectStore/ObjectStoreStorageTest.php
checking file tests/lib/Files/ObjectStore/ObjectStoreTest.php
checking file tests/lib/Files/ObjectStore/SwiftTest.php
checking file lib/private/Files/ObjectStore/S3.php
checking file tests/lib/Files/ObjectStore/S3Test.php
checking file lib/private/Files/FileInfo.php
checking file lib/private/Files/Mount/ObjectHomeMountProvider.php
checking file lib/private/AppFramework/Http/Output.php
checking file lib/public/AppFramework/Http/IOutput.php
checking file lib/public/AppFramework/Http/StreamResponse.php
checking file apps/theming/lib/Controller/ThemingController.php
can't find file to patch at input line 1350
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|
|From b8fecf47a650829c0d7e13b287e022e62369dbd0 Mon Sep 17 00:00:00 2001
|From: Robin Appelman <robin@icewind.nl>
|Date: Fri, 28 Oct 2016 16:35:39 +0200
|Subject: [PATCH 7/8] fopen s3 objects directly to work around unexplainable
| guzzle bug
|
|For some reason when a text file started with a valid hex character ([0-9a-f]) it would eat the text untill the first newline
|The new code does basically the same thing as guzzle/s3-sdk did only without wrapping everything in a guzzle stream
|
|Signed-off-by: Robin Appelman <robin@icewind.nl>
|---
| lib/private/Files/ObjectStore/S3.php | 51 +++++++++++++++---------------------
| 1 file changed, 21 insertions(+), 30 deletions(-)
|
|diff --git a/lib/private/Files/ObjectStore/S3.php b/lib/private/Files/ObjectStore/S3.php
|index 641c172..5251b47 100644
|--- a/lib/private/Files/ObjectStore/S3.php
|+++ b/lib/private/Files/ObjectStore/S3.php
--------------------------

[gjonespf]

Haven't tried the patch, but does this work with just s3 storage, or does it support s3 compatible storage? e.g.
https://www.minio.io/
Would be keen to see compatibility, as this would mean less lock in. Though I understand it adds some testing complications 😄

[nickvergessen]

if this is intended, it needs a comment why so

[icewind1991]

done

[nickvergessen]

Still don't see a comment or revert of this. Forgot to push?

[icewind1991]

now it's there

[icewind1991]

@MorrisJobke @nickvergessen can this be merged?

[icewind1991]

@MorrisJobke @nickvergessen @LukasReschke please review

[rullzer]

Tests are actually run in #2159

[MorrisJobke]

Works but only with authorization v2 and not the new v4 (Amazon S3 in Frankfurt only allows then new version). We should upgrade our S3 library to accomplish this.

👍 from me

[flypenguin]

Although I fear I know the answer - has the S3 library been upgraded or not? (We're in Frankfurt btw ;)

[MorrisJobke]

Although I fear I know the answer - has the S3 library been upgraded or not? (We're in Frankfurt btw ;)

Nope :( That's also an issue I pointed out 😢

[MorrisJobke]

Although I fear I know the answer - has the S3 library been upgraded or not? (We're in Frankfurt btw ;)

Could you open an ticket for that? Thanks

[powareverb]

@rusher81572 thanks for pointing this out. Looks initially positive.