From 09ffac5e6dd5355c9aaf49c098942fa1e4fbed25 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 13 Oct 2021 19:42:31 +0200 Subject: [PATCH 01/11] s3 external storage listing rework Signed-off-by: Robin Appelman --- .github/workflows/s3-external.yml | 65 +++++ .../lib/Lib/Storage/AmazonS3.php | 243 +++++++++++------- .../Files/ObjectStore/S3ObjectTrait.php | 2 +- 3 files changed, 213 insertions(+), 97 deletions(-) create mode 100644 .github/workflows/s3-external.yml diff --git a/.github/workflows/s3-external.yml b/.github/workflows/s3-external.yml new file mode 100644 index 0000000000000..c51d070533d74 --- /dev/null +++ b/.github/workflows/s3-external.yml @@ -0,0 +1,65 @@ +name: S3 External storage +on: + push: + branches: + - master + - stable* + paths: + - 'apps/files_external/**' + pull_request: + paths: + - 'apps/files_external/**' + +env: + APP_NAME: files_external + +jobs: + s3-external-tests: + runs-on: ubuntu-latest + + strategy: + # do not stop on another job's failure + fail-fast: false + matrix: + php-versions: ['7.4', '8.0'] + + name: php${{ matrix.php-versions }}-${{ matrix.ftpd }} + + services: + minio: + image: minio/minio:RELEASE.2021-10-06T23-36-31Z + ports: + - "9000:9000" + + steps: + - name: Checkout server + uses: actions/checkout@v2 + with: + submodules: true + + - name: Set up php ${{ matrix.php-versions }} + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php-versions }} + tools: phpunit + extensions: mbstring, iconv, fileinfo, intl, sqlite, pdo_sqlite, zip, gd + + - name: Set up Nextcloud + run: | + mkdir data + ./occ maintenance:install --verbose --database=sqlite --database-name=nextcloud --database-host=127.0.0.1 --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass password + ./occ app:enable --force ${{ env.APP_NAME }} + php -S localhost:8080 & + - name: PHPUnit + run: | + echo " true,'hostname' => 'localhost','key' => 'minioadmin','secret' => 'minioadmin', 'bucket' => 'bucket', 'port' => 9000, 'use_ssl' => false, 'autocreate' => true, 'use_path_style' => true];" > apps/${{ env.APP_NAME }}/tests/config.amazons3.php + phpunit --configuration tests/phpunit-autotest-external.xml apps/files_external/tests/Storage/Amazons3Test.php + s3-external-summary: + runs-on: ubuntu-latest + needs: s3-external-tests + + if: always() + + steps: + - name: Summary status + run: if ${{ needs.s3-external-tests.result != 'success' }}; then exit 1; fi diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 1bdd11e39bd63..f85797d29f488 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -46,6 +46,7 @@ use Icewind\Streams\IteratorDirectory; use OC\Cache\CappedMemoryCache; use OC\Files\Cache\CacheEntry; +use OC\Files\Filesystem; use OC\Files\ObjectStore\S3ConnectionTrait; use OC\Files\ObjectStore\S3ObjectTrait; use OCP\Constants; @@ -71,6 +72,9 @@ public function needsPartFile() { /** @var IMimeTypeDetector */ private $mimeDetector; + /** @var bool|null */ + private $versioningEnabled = null; + public function __construct($parameters) { parent::__construct($parameters); $this->parseParams($parameters); @@ -120,12 +124,20 @@ private function invalidateCache($key) { unset($this->objectCache[$existingKey]); } } - unset($this->directoryCache[$key], $this->filesCache[$key]); + unset($this->filesCache[$key]); + $keys = array_keys($this->directoryCache->getData()); + $keyLength = strlen($key); + foreach ($keys as $existingKey) { + if (substr($existingKey, 0, $keyLength) === $key) { + unset($this->directoryCache[$existingKey]); + } + } + unset($this->directoryCache[$key]); } /** * @param $key - * @return Result|boolean + * @return array|false */ private function headObject($key) { if (!isset($this->objectCache[$key])) { @@ -133,7 +145,7 @@ private function headObject($key) { $this->objectCache[$key] = $this->getConnection()->headObject([ 'Bucket' => $this->bucket, 'Key' => $key - ]); + ])->toArray(); } catch (S3Exception $e) { if ($e->getStatusCode() >= 500) { throw $e; @@ -159,32 +171,44 @@ private function headObject($key) { * @throws \Exception */ private function doesDirectoryExist($path) { - if (!isset($this->directoryCache[$path])) { + if ($path === '.' || $path === '') { + return true; + } + + if (isset($this->directoryCache[$path])) { + return $this->directoryCache[$path]; + } + try { // Maybe this isn't an actual key, but a prefix. // Do a prefix listing of objects to determine. - try { - $result = $this->getConnection()->listObjects([ - 'Bucket' => $this->bucket, - 'Prefix' => rtrim($path, '/'), - 'MaxKeys' => 1, - 'Delimiter' => '/', - ]); + $result = $this->getConnection()->listObjectsV2([ + 'Bucket' => $this->bucket, + 'Prefix' => rtrim($path, '/'), + 'MaxKeys' => 1, + ]); - if ((isset($result['Contents'][0]['Key']) && $result['Contents'][0]['Key'] === rtrim($path, '/') . '/') - || isset($result['CommonPrefixes'])) { - $this->directoryCache[$path] = true; - } else { - $this->directoryCache[$path] = false; - } - } catch (S3Exception $e) { - if ($e->getStatusCode() === 403) { - $this->directoryCache[$path] = false; - } - throw $e; + if (isset($result['Contents'])) { + $this->directoryCache[$path] = true; + return true; } + + // empty directories have their own object + $object = $this->headObject($path); + + if ($object) { + $this->directoryCache[$path] = true; + return true; + } + } catch (S3Exception $e) { + if ($e->getStatusCode() === 403) { + $this->directoryCache[$path] = false; + } + throw $e; } - return $this->directoryCache[$path]; + + $this->directoryCache[$path] = false; + return false; } /** @@ -284,7 +308,9 @@ public function rmdir($path) { protected function clearBucket() { $this->clearCache(); try { - $this->getConnection()->clearBucket($this->bucket); + $this->getConnection()->clearBucket([ + "Bucket" => $this->bucket + ]); return true; // clearBucket() is not working with Ceph, so if it fails we try the slower approach } catch (\Exception $e) { @@ -318,7 +344,9 @@ private function batchDelete($path = null) { } // we reached the end when the list is no longer truncated } while ($objects['IsTruncated']); - $this->deleteObject($path); + if ($path !== '' && $path !== null) { + $this->deleteObject($path); + } } catch (S3Exception $e) { \OC::$server->getLogger()->logException($e, ['app' => 'files_external']); return false; @@ -327,54 +355,12 @@ private function batchDelete($path = null) { } public function opendir($path) { - $path = $this->normalizePath($path); - - if ($this->isRoot($path)) { - $path = ''; - } else { - $path .= '/'; - } - try { - $files = []; - $results = $this->getConnection()->getPaginator('ListObjects', [ - 'Bucket' => $this->bucket, - 'Delimiter' => '/', - 'Prefix' => $path, - ]); - - foreach ($results as $result) { - // sub folders - if (is_array($result['CommonPrefixes'])) { - foreach ($result['CommonPrefixes'] as $prefix) { - $directoryName = trim($prefix['Prefix'], '/'); - $files[] = substr($directoryName, strlen($path)); - $this->directoryCache[$directoryName] = true; - } - } - if (is_array($result['Contents'])) { - foreach ($result['Contents'] as $object) { - if (isset($object['Key']) && $object['Key'] === $path) { - // it's the directory itself, skip - continue; - } - $file = basename( - isset($object['Key']) ? $object['Key'] : $object['Prefix'] - ); - $files[] = $file; - - // store this information for later usage - $this->filesCache[$path . $file] = [ - 'ContentLength' => $object['Size'], - 'LastModified' => (string)$object['LastModified'], - ]; - } - } - } - - return IteratorDirectory::wrap($files); - } catch (S3Exception $e) { - \OC::$server->getLogger()->logException($e, ['app' => 'files_external']); + $content = iterator_to_array($this->getDirectoryContent($path)); + return IteratorDirectory::wrap(array_map(function (array $item) { + return $item['name']; + }, $content)); + } catch (S3Exception $e) { return false; } } @@ -382,33 +368,19 @@ public function opendir($path) { public function stat($path) { $path = $this->normalizePath($path); - try { - $stat = []; - if ($this->is_dir($path)) { - $cacheEntry = $this->getCache()->get($path); - if ($cacheEntry instanceof CacheEntry) { - $stat['size'] = $cacheEntry->getSize(); - $stat['mtime'] = $cacheEntry->getMTime(); - } else { - // Use dummy values - $stat['size'] = -1; // Pending - $stat['mtime'] = time(); - } - } else { - $stat['size'] = $this->getContentLength($path); - $stat['mtime'] = strtotime($this->getLastModified($path)); + if ($this->is_dir($path)) { + $stat = $this->getDirectoryMetaData($path); + } else { + $object = $this->headObject($path); + if ($object === false) { + return false; } - $stat['atime'] = time(); - - return $stat; - } catch (S3Exception $e) { - \OC::$server->getLogger()->logException($e, ['app' => 'files_external']); - return false; + $object["Key"] = $path; + $stat = $this->objectToMetaData($object); } - } + $stat['atime'] = time(); - public function hasUpdated($path, $time) { - return $this->getMountOption('filesystem_check_changes', 1) === 1 || parent::hasUpdated($path, $time); + return $stat; } /** @@ -711,4 +683,83 @@ public function writeBack($tmpFile, $path) { public static function checkDependencies() { return true; } + + public function getDirectoryContent($directory): \Traversable { + $path = $this->normalizePath($directory); + + if ($this->isRoot($path)) { + $path = ''; + } else { + $path .= '/'; + } + + $results = $this->getConnection()->getPaginator('ListObjectsV2', [ + 'Bucket' => $this->bucket, + 'Delimiter' => '/', + 'Prefix' => $path, + ]); + + foreach ($results as $result) { + // sub folders + if (is_array($result['CommonPrefixes'])) { + foreach ($result['CommonPrefixes'] as $prefix) { + $dir = $this->getDirectoryMetaData($prefix['Prefix']); + if ($dir) { + yield $dir; + } + } + } + if (is_array($result['Contents'])) { + foreach ($result['Contents'] as $object) { + $this->objectCache[$object['Key']] = $object; + if ($object['Key'] !== $path) { + yield $this->objectToMetaData($object); + } + } + } + } + } + + private function objectToMetaData(array $object): array { + return [ + 'name' => basename($object['Key']), + 'mimetype' => $this->mimeDetector->detectPath($object['Key']), + 'mtime' => strtotime($object['LastModified']), + 'storage_mtime' => strtotime($object['LastModified']), + 'etag' => $object['ETag'], + 'permissions' => Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE, + 'size' => (int)($object['Size'] ?? $object['ContentLength']), + ]; + } + + private function getDirectoryMetaData(string $path): ?array { + $path = trim($path, '/'); + // when versioning is enabled, delete markers are returned as part of CommonPrefixes + // resulting in "ghost" folders, verify that each folder actually exists + if ($this->versioningEnabled() && !$this->doesDirectoryExist($path)) { + return null; + } + $cacheEntry = $this->getCache()->get($path); + if ($cacheEntry instanceof CacheEntry) { + return $cacheEntry->getData(); + } else { + return [ + 'name' => basename($path), + 'mimetype' => 'httpd/unix-directory', + 'mtime' => time(), + 'storage_mtime' => time(), + 'etag' => uniqid(), + 'permissions' => Constants::PERMISSION_ALL, + 'size' => -1, + ]; + } + } + + public function versioningEnabled(): bool { + if ($this->versioningEnabled === null) { + $result = $this->getConnection()->getBucketVersioning(['Bucket' => $this->getBucket()]); + $this->versioningEnabled = $result->get('Status') === 'Enabled'; + } + return $this->versioningEnabled; + } } diff --git a/lib/private/Files/ObjectStore/S3ObjectTrait.php b/lib/private/Files/ObjectStore/S3ObjectTrait.php index c88246094ed81..01da7a88dc8f1 100644 --- a/lib/private/Files/ObjectStore/S3ObjectTrait.php +++ b/lib/private/Files/ObjectStore/S3ObjectTrait.php @@ -65,7 +65,7 @@ public function readObject($urn) { } $opts = [ 'http' => [ - 'protocol_version' => 1.1, + 'protocol_version' => $request->getProtocolVersion(), 'header' => $headers, ], ]; From 5e3c8b3af2f2b25fcadc9aaf3ed10afa6e1d63c4 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 14 Oct 2021 18:36:55 +0200 Subject: [PATCH 02/11] doesDirectoryExist fixes Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index f85797d29f488..1ad1cd699d3b6 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -46,7 +46,6 @@ use Icewind\Streams\IteratorDirectory; use OC\Cache\CappedMemoryCache; use OC\Files\Cache\CacheEntry; -use OC\Files\Filesystem; use OC\Files\ObjectStore\S3ConnectionTrait; use OC\Files\ObjectStore\S3ObjectTrait; use OCP\Constants; @@ -174,6 +173,7 @@ private function doesDirectoryExist($path) { if ($path === '.' || $path === '') { return true; } + $path = rtrim($path, '/') . '/'; if (isset($this->directoryCache[$path])) { return $this->directoryCache[$path]; @@ -183,7 +183,7 @@ private function doesDirectoryExist($path) { // Do a prefix listing of objects to determine. $result = $this->getConnection()->listObjectsV2([ 'Bucket' => $this->bucket, - 'Prefix' => rtrim($path, '/'), + 'Prefix' => $path, 'MaxKeys' => 1, ]); @@ -360,7 +360,7 @@ public function opendir($path) { return IteratorDirectory::wrap(array_map(function (array $item) { return $item['name']; }, $content)); - } catch (S3Exception $e) { + } catch (S3Exception $e) { return false; } } @@ -435,7 +435,7 @@ public function is_dir($path) { } try { - return $this->isRoot($path) || $this->doesDirectoryExist($path); + return $this->doesDirectoryExist($path); } catch (S3Exception $e) { \OC::$server->getLogger()->logException($e, ['app' => 'files_external']); return false; From 294af4275c3eaf8a40564edc4706ed7fab3f18c2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 14 Oct 2021 19:16:58 +0200 Subject: [PATCH 03/11] more reliable directory copy Signed-off-by: Robin Appelman --- .../lib/Lib/Storage/AmazonS3.php | 30 +++++++------------ 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 1ad1cd699d3b6..6fbe913ec7ce7 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -49,6 +49,7 @@ use OC\Files\ObjectStore\S3ConnectionTrait; use OC\Files\ObjectStore\S3ObjectTrait; use OCP\Constants; +use OCP\Files\FileInfo; use OCP\Files\IMimeTypeDetector; class AmazonS3 extends \OC\Files\Storage\Common { @@ -272,7 +273,7 @@ public function mkdir($path) { 'Bucket' => $this->bucket, 'Key' => $path . '/', 'Body' => '', - 'ContentType' => 'httpd/unix-directory' + 'ContentType' => FileInfo::MIMETYPE_FOLDER ]); $this->testTimeout(); } catch (S3Exception $e) { @@ -575,11 +576,11 @@ public function touch($path, $mtime = null) { return true; } - public function copy($path1, $path2) { + public function copy($path1, $path2, $isFile = null) { $path1 = $this->normalizePath($path1); $path2 = $this->normalizePath($path2); - if ($this->is_file($path1)) { + if ($isFile === true || $this->is_file($path1)) { try { $this->getConnection()->copyObject([ 'Bucket' => $this->bucket, @@ -595,28 +596,17 @@ public function copy($path1, $path2) { $this->remove($path2); try { - $this->getConnection()->copyObject([ - 'Bucket' => $this->bucket, - 'Key' => $path2 . '/', - 'CopySource' => S3Client::encodeKey($this->bucket . '/' . $path1 . '/') - ]); + $this->mkdir($path2); $this->testTimeout(); } catch (S3Exception $e) { \OC::$server->getLogger()->logException($e, ['app' => 'files_external']); return false; } - $dh = $this->opendir($path1); - if (is_resource($dh)) { - while (($file = readdir($dh)) !== false) { - if (\OC\Files\Filesystem::isIgnoredDir($file)) { - continue; - } - - $source = $path1 . '/' . $file; - $target = $path2 . '/' . $file; - $this->copy($source, $target); - } + foreach ($this->getDirectoryContent($path1) as $item) { + $source = $path1 . '/' . $item['name']; + $target = $path2 . '/' . $item['name']; + $this->copy($source, $target, $item['mimetype'] !== FileInfo::MIMETYPE_FOLDER); } } @@ -745,7 +735,7 @@ private function getDirectoryMetaData(string $path): ?array { } else { return [ 'name' => basename($path), - 'mimetype' => 'httpd/unix-directory', + 'mimetype' => FileInfo::MIMETYPE_FOLDER, 'mtime' => time(), 'storage_mtime' => time(), 'etag' => uniqid(), From 294b218895f446fe53bf9882c964d7cb49baf41d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 14 Oct 2021 17:28:32 +0200 Subject: [PATCH 04/11] ci Signed-off-by: Robin Appelman --- .github/workflows/s3-external.yml | 71 +++++++++++++++++-- .../tests/Storage/Amazons3Test.php | 6 ++ .../tests/Storage/VersionedAmazonS3Test.php | 43 +++++++++++ tests/lib/Files/Storage/Storage.php | 3 + 4 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 apps/files_external/tests/Storage/VersionedAmazonS3Test.php diff --git a/.github/workflows/s3-external.yml b/.github/workflows/s3-external.yml index c51d070533d74..497de81ae2f79 100644 --- a/.github/workflows/s3-external.yml +++ b/.github/workflows/s3-external.yml @@ -14,7 +14,7 @@ env: APP_NAME: files_external jobs: - s3-external-tests: + s3-external-tests-minio: runs-on: ubuntu-latest strategy: @@ -23,11 +23,14 @@ jobs: matrix: php-versions: ['7.4', '8.0'] - name: php${{ matrix.php-versions }}-${{ matrix.ftpd }} + name: php${{ matrix.php-versions }}-minio services: minio: - image: minio/minio:RELEASE.2021-10-06T23-36-31Z + env: + MINIO_ACCESS_KEY: minio + MINIO_SECRET_KEY: minio123 + image: bitnami/minio:2021.10.6 ports: - "9000:9000" @@ -52,14 +55,70 @@ jobs: php -S localhost:8080 & - name: PHPUnit run: | - echo " true,'hostname' => 'localhost','key' => 'minioadmin','secret' => 'minioadmin', 'bucket' => 'bucket', 'port' => 9000, 'use_ssl' => false, 'autocreate' => true, 'use_path_style' => true];" > apps/${{ env.APP_NAME }}/tests/config.amazons3.php + echo " true,'hostname' => 'localhost','key' => 'minio','secret' => 'minio123', 'bucket' => 'bucket', 'port' => 9000, 'use_ssl' => false, 'autocreate' => true, 'use_path_style' => true];" > apps/${{ env.APP_NAME }}/tests/config.amazons3.php phpunit --configuration tests/phpunit-autotest-external.xml apps/files_external/tests/Storage/Amazons3Test.php + phpunit --configuration tests/phpunit-autotest-external.xml apps/files_external/tests/Storage/VersionedAmazonS3Test.php + - name: S3 logs + if: always() + run: | + docker ps -a + docker logs $(docker ps -aq) + s3-external-tests-localstack: + runs-on: ubuntu-latest + + strategy: + # do not stop on another job's failure + fail-fast: false + matrix: + php-versions: ['7.4', '8.0'] + + name: php${{ matrix.php-versions }}-localstack + + services: + minio: + env: + SERVICES: s3 + DEBUG: 1 + image: localstack/localstack:0.12.7 + ports: + - "4566:4566" + + steps: + - name: Checkout server + uses: actions/checkout@v2 + with: + submodules: true + + - name: Set up php ${{ matrix.php-versions }} + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php-versions }} + tools: phpunit + extensions: mbstring, iconv, fileinfo, intl, sqlite, pdo_sqlite, zip, gd + + - name: Set up Nextcloud + run: | + mkdir data + ./occ maintenance:install --verbose --database=sqlite --database-name=nextcloud --database-host=127.0.0.1 --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass password + ./occ app:enable --force ${{ env.APP_NAME }} + php -S localhost:8080 & + - name: PHPUnit + run: | + echo " true,'hostname' => 'localhost','key' => 'ignored','secret' => 'ignored', 'bucket' => 'bucket', 'port' => 4566, 'use_ssl' => false, 'autocreate' => true, 'use_path_style' => true];" > apps/${{ env.APP_NAME }}/tests/config.amazons3.php + phpunit --configuration tests/phpunit-autotest-external.xml apps/files_external/tests/Storage/Amazons3Test.php + phpunit --configuration tests/phpunit-autotest-external.xml apps/files_external/tests/Storage/VersionedAmazonS3Test.php + - name: S3 logs + if: always() + run: | + docker ps -a + docker logs $(docker ps -aq) + s3-external-summary: runs-on: ubuntu-latest - needs: s3-external-tests + needs: [s3-external-tests-minio, s3-external-tests-localstack] if: always() steps: - name: Summary status - run: if ${{ needs.s3-external-tests.result != 'success' }}; then exit 1; fi + run: if ${{ needs.s3-external-tests-minio.result != 'success' }} || ${{ needs.s3-external-tests-localstack.result != 'success' }}; then exit 1; fi diff --git a/apps/files_external/tests/Storage/Amazons3Test.php b/apps/files_external/tests/Storage/Amazons3Test.php index c013d304cceb0..d231539fb54a6 100644 --- a/apps/files_external/tests/Storage/Amazons3Test.php +++ b/apps/files_external/tests/Storage/Amazons3Test.php @@ -38,6 +38,8 @@ */ class Amazons3Test extends \Test\Files\Storage\Storage { private $config; + /** @var AmazonS3 */ + protected $instance; protected function setUp(): void { parent::setUp(); @@ -60,4 +62,8 @@ protected function tearDown(): void { public function testStat() { $this->markTestSkipped('S3 doesn\'t update the parents folder mtime'); } + + public function testHashInFileName() { + $this->markTestSkipped('Localstack has a bug with hashes in filename'); + } } diff --git a/apps/files_external/tests/Storage/VersionedAmazonS3Test.php b/apps/files_external/tests/Storage/VersionedAmazonS3Test.php new file mode 100644 index 0000000000000..a16a9944d578b --- /dev/null +++ b/apps/files_external/tests/Storage/VersionedAmazonS3Test.php @@ -0,0 +1,43 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Files_External\Tests\Storage; + +/** + * @group DB + */ +class VersionedAmazonS3Test extends Amazons3Test { + protected function setUp(): void { + parent::setUp(); + try { + $this->instance->getConnection()->putBucketVersioning([ + 'Bucket' => $this->instance->getBucket(), + 'VersioningConfiguration' => [ + 'Status' => 'Enabled', + ], + ]); + } catch (\Exception $e) { + $this->markTestSkipped("s3 backend doesn't seem to support versioning"); + } + } +} diff --git a/tests/lib/Files/Storage/Storage.php b/tests/lib/Files/Storage/Storage.php index 9fae1a8484aaa..c4248b7e0da8c 100644 --- a/tests/lib/Files/Storage/Storage.php +++ b/tests/lib/Files/Storage/Storage.php @@ -498,6 +498,9 @@ public function testRenameDirectory() { $this->assertTrue($this->instance->file_exists('target/subfolder')); $this->assertTrue($this->instance->file_exists('target/subfolder/test.txt')); + $contents = iterator_to_array($this->instance->getDirectoryContent('')); + $this->assertCount(1, $contents); + $this->assertEquals('foo', $this->instance->file_get_contents('target/test1.txt')); $this->assertEquals('qwerty', $this->instance->file_get_contents('target/test2.txt')); $this->assertEquals('bar', $this->instance->file_get_contents('target/subfolder/test.txt')); From d3bd0b5a1bfd8125866a9a86cbb05d3d2c6e14bb Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 15 Oct 2021 14:54:09 +0200 Subject: [PATCH 05/11] optimize filetype for s3 directories a bit Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 6fbe913ec7ce7..c4c82c6a8795e 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -451,6 +451,9 @@ public function filetype($path) { } try { + if (isset($this->directoryCache[$path])) { + return 'dir'; + } if (isset($this->filesCache[$path]) || $this->headObject($path)) { return 'file'; } From 34637697e1310728d138aa4386f890dfc4ffd288 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 15 Oct 2021 15:15:56 +0200 Subject: [PATCH 06/11] remove old migration method Signed-off-by: Robin Appelman --- .../lib/Lib/Storage/AmazonS3.php | 31 ------------------- 1 file changed, 31 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index c4c82c6a8795e..9c7d6a554b3b5 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -212,37 +212,6 @@ private function doesDirectoryExist($path) { return false; } - /** - * Updates old storage ids (v0.2.1 and older) that are based on key and secret to new ones based on the bucket name. - * TODO Do this in a repair step. requires iterating over all users and loading the mount.json from their home - * - * @param array $params - */ - public function updateLegacyId(array $params) { - $oldId = 'amazon::' . $params['key'] . md5($params['secret']); - - // find by old id or bucket - $stmt = \OC::$server->getDatabaseConnection()->prepare( - 'SELECT `numeric_id`, `id` FROM `*PREFIX*storages` WHERE `id` IN (?, ?)' - ); - $stmt->execute([$oldId, $this->id]); - while ($row = $stmt->fetch()) { - $storages[$row['id']] = $row['numeric_id']; - } - - if (isset($storages[$this->id]) && isset($storages[$oldId])) { - // if both ids exist, delete the old storage and corresponding filecache entries - \OC\Files\Cache\Storage::remove($oldId); - } elseif (isset($storages[$oldId])) { - // if only the old id exists do an update - $stmt = \OC::$server->getDatabaseConnection()->prepare( - 'UPDATE `*PREFIX*storages` SET `id` = ? WHERE `id` = ?' - ); - $stmt->execute([$this->id, $oldId]); - } - // only the bucket based id may exist, do nothing - } - /** * Remove a file or folder * From 4bd08af2adf6742751aba089d28f6cb486e69e13 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 15 Oct 2021 16:02:57 +0200 Subject: [PATCH 07/11] more reliable hasUpdated for s3 Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 9c7d6a554b3b5..527a87aeb9a2b 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -724,4 +724,17 @@ public function versioningEnabled(): bool { } return $this->versioningEnabled; } + + public function hasUpdated($path, $time) { + // for files we can get the proper mtime + if ($path !== '' && $object = $this->headObject($path)) { + $stat = $this->objectToMetaData($object); + return $stat['mtime'] > $time; + } else { + // for directories, the only real option we have is to do a prefix listing and iterate over all objects + // however, since this is just as expensive as just re-scanning the directory, we can simply return true + // and have the scanner figure out if anything has actually changed + return true; + } + } } From 55346b5d6cb24d11157b7a2e73a91fda85bb9af5 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 15 Oct 2021 16:03:18 +0200 Subject: [PATCH 08/11] more reliable return value for Watcher::checkUpdate Signed-off-by: Robin Appelman --- lib/private/Files/Cache/Watcher.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/private/Files/Cache/Watcher.php b/lib/private/Files/Cache/Watcher.php index 1c1ce6d777bb2..acc76f263dc57 100644 --- a/lib/private/Files/Cache/Watcher.php +++ b/lib/private/Files/Cache/Watcher.php @@ -88,7 +88,14 @@ public function checkUpdate($path, $cachedEntry = null) { } if ($cachedEntry === false || $this->needsUpdate($path, $cachedEntry)) { $this->update($path, $cachedEntry); - return true; + + if ($cachedEntry === false) { + return true; + } else { + // storage backends can sometimes return false positives, only return true if the scanner actually found a change + $newEntry = $this->cache->get($path); + return $newEntry->getStorageMTime() > $cachedEntry->getStorageMTime(); + } } else { return false; } From 247e12da966aeceb833e8c60b833e089bee89f8a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 15 Oct 2021 16:23:39 +0200 Subject: [PATCH 09/11] always set Key field in `headObject` Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 527a87aeb9a2b..aa0afac15439f 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -154,6 +154,9 @@ private function headObject($key) { } } + if (is_array($this->objectCache[$key]) && !isset($this->objectCache[$key]["Key"])) { + $this->objectCache[$key]["Key"] = $key; + } return $this->objectCache[$key]; } @@ -345,7 +348,6 @@ public function stat($path) { if ($object === false) { return false; } - $object["Key"] = $path; $stat = $this->objectToMetaData($object); } $stat['atime'] = time(); From eb6e6e3a85a5d65215736998c916c5fe30d8271b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 19 Oct 2021 14:58:40 +0200 Subject: [PATCH 10/11] minor directory detect improvements Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/AmazonS3.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index aa0afac15439f..cb5fe134e6f2f 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -204,7 +204,7 @@ private function doesDirectoryExist($path) { return true; } } catch (S3Exception $e) { - if ($e->getStatusCode() === 403) { + if ($e->getStatusCode() >= 400 && $e->getStatusCode() < 500) { $this->directoryCache[$path] = false; } throw $e; @@ -422,7 +422,7 @@ public function filetype($path) { } try { - if (isset($this->directoryCache[$path])) { + if (isset($this->directoryCache[$path]) && $this->directoryCache[$path]) { return 'dir'; } if (isset($this->filesCache[$path]) || $this->headObject($path)) { From a1ca901e58e89a8cd7848910fd9700b4b6f5b402 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 19 Oct 2021 15:03:22 +0200 Subject: [PATCH 11/11] cache versioning enabled status Signed-off-by: Robin Appelman --- .../lib/Lib/Storage/AmazonS3.php | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index cb5fe134e6f2f..827fd63d1d655 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -51,6 +51,8 @@ use OCP\Constants; use OCP\Files\FileInfo; use OCP\Files\IMimeTypeDetector; +use OCP\ICacheFactory; +use OCP\IMemcache; class AmazonS3 extends \OC\Files\Storage\Common { use S3ConnectionTrait; @@ -75,6 +77,9 @@ public function needsPartFile() { /** @var bool|null */ private $versioningEnabled = null; + /** @var IMemcache */ + private $memCache; + public function __construct($parameters) { parent::__construct($parameters); $this->parseParams($parameters); @@ -82,6 +87,9 @@ public function __construct($parameters) { $this->directoryCache = new CappedMemoryCache(); $this->filesCache = new CappedMemoryCache(); $this->mimeDetector = \OC::$server->get(IMimeTypeDetector::class); + /** @var ICacheFactory $cacheFactory */ + $cacheFactory = \OC::$server->get(ICacheFactory::class); + $this->memCache = $cacheFactory->createLocal('s3-external'); } /** @@ -721,8 +729,14 @@ private function getDirectoryMetaData(string $path): ?array { public function versioningEnabled(): bool { if ($this->versioningEnabled === null) { - $result = $this->getConnection()->getBucketVersioning(['Bucket' => $this->getBucket()]); - $this->versioningEnabled = $result->get('Status') === 'Enabled'; + $cached = $this->memCache->get('versioning-enabled::' . $this->getBucket()); + if ($cached === null) { + $result = $this->getConnection()->getBucketVersioning(['Bucket' => $this->getBucket()]); + $this->versioningEnabled = $result->get('Status') === 'Enabled'; + $this->memCache->set('versioning-enabled::' . $this->getBucket(), $this->versioningEnabled, 60); + } else { + $this->versioningEnabled = $cached; + } } return $this->versioningEnabled; }