Add support for s3 storage classes

[fmenabe]

• Resolves: Setting of storage class for S3 #14031

Summary

This add support for s3 storage class in both primary objectstore and External Storage (files_external) application.

TODO

• Traductions for files_external app manually done for english (not sure that should be done manually) but not for other languages
• Unit tests? (not sure how to test it)
• Documentation
  • https://github.com/nextcloud/documentation/blob/master/admin_manual/configuration_files/primary_storage.rst#simple-storage-service-s3
  • https://github.com/nextcloud/documentation/blob/master/admin_manual/configuration_files/external_storage/amazons3.rst

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Documentation (manuals or wiki) has been updated or is not required

[kesselb]

Thanks for your pull request 👍

Traductions for files_external app manually done for english (not sure that should be done manually) but not for other languages

Please don't change the translation files.
A bot will update them automatically from Transifex once this pull request was merged.

[fmenabe]

Please don't change the translation files. A bot will update them automatically from Transifex once this pull request was merged.

I removed the translations from the commit

[artonge]

Beside minor nitpick, looks good to me

[artonge]

Why not use the same logic as for other params ?

Suggested change

	$this->storageClass = (isset($params['storageClass']) && !empty($params['storageClass'])) ? $params['storageClass'] : 'STANDARD';
	$this->storageClass = $params['storageClass'] ?? 'STANDARD';

[fmenabe]

That's what I've done at start ;-)

But in the external storage application (files_external), if the storage class is not specified, you get an empty class (an empty string), which is not a valid a storage class for the providers and generates an error. Not sure you can specified a default value for OCA\Files_External\Lib\DefinitionParameter?

It seemed to me more generic to ensure here that, if the storage class is not defined or is empty, we fallback to STANDARD.

[come-nc]

Suggested change

	$this->storageClass = (isset($params['storageClass']) && !empty($params['storageClass'])) ? $params['storageClass'] : 'STANDARD';
	$this->storageClass = !empty($params['storageClass']) ? $params['storageClass'] : 'STANDARD';

Then no need for isset, empty implies isset.

[fmenabe]

ok, the PR have been updated

[come-nc]

Some nitpick but otherwise looks fine.

[come-nc]

Suggested change

	'StorageClass' => $this->storageClass
	'StorageClass' => $this->storageClass,

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[aentwist]

Since this comes with no tests or documentation, I'll ask here - has anyone actually tested this? It isn't working for me, and I am trying to make it available on the Docker image. I can open an issue?

• new installation
• empty bucket
• config as correct as I can figure it should be after reading this code

-> everything gets STANDARD

Relevant config..

        "objectstore": {
            "class": "\\OC\\Files\\ObjectStore\\S3",
            "arguments": {
                "bucket": "redacted",
                "region": "us-east-1",
                "hostname": "",
                "port": "",
                "storageClass": "INTELLIGENT_TIERING",
                "objectPrefix": "urn:oid:",
                "autocreate": false,
                "use_ssl": true,
                "use_path_style": false,
                "legacy_auth": false
            }
        },

The log is empty

I am not trying to raise an issue here. I just need to verify that the usage above is as intended.

[fmenabe]

I've just retested on my dev environment (based on skjnldsv/docker-nextcloud-dev) and it "works for me". Switched the fork to v26.0.0 tag and it still works.

That's the config I used (seems pretty identical to what you paste and yes only storageClass parameter was added by this PR):

  'objectstore' => array(
    'class' => '\\OC\\Files\\ObjectStore\\S3',
    'arguments' => array(
      'bucket'     => '<BUCKET>',
      'key'        => '<KEY>',
      'secret'     => '<SECRET>',
      'hostname'   => 's3.fr-par.scw.cloud',
      'storageClass' => 'ONEZONE_IA',
      'port'       => '443',
      'use_ssl'    => true,
      'region'     => 'fr-par'
    )
  )

I also manually build a Docker image from nextcloud/docker applying something similar to nextcloud/docker#1933 but directly in 26/apache/config/s3.config.php directory and it also worked as expected.

$ git diff 26/apache/config/s3.config.php
diff --git a/26/apache/config/s3.config.php b/26/apache/config/s3.config.php
index aa3f4f5..4223d8a 100644
--- a/26/apache/config/s3.config.php
+++ b/26/apache/config/s3.config.php
@@ -15,6 +15,7 @@ if (getenv('OBJECTSTORE_S3_BUCKET')) {
         'hostname' => getenv('OBJECTSTORE_S3_HOST') ?: '',
         'port' => getenv('OBJECTSTORE_S3_PORT') ?: '',
         'objectPrefix' => getenv("OBJECTSTORE_S3_OBJECT_PREFIX") ? getenv("OBJECTSTORE_S3_OBJECT_PREFIX") : "urn:oid:",
+        'storageClass' => getenv("OBJECTSTORE_S3_STORAGE_CLASS") ? getenv("OBJECTSTORE_S3_STORAGE_CLASS") : "STANDARD",
         'autocreate' => (strtolower($autocreate) === 'false' || $autocreate == false) ? false : true,
         'use_ssl' => (strtolower($use_ssl) === 'false' || $use_ssl == false) ? false : true,
         // required for some non Amazon S3 implementations
@@ -24,4 +25,4 @@ if (getenv('OBJECTSTORE_S3_BUCKET')) {
       )
     )
   );
-} 
+}
$ cd 26/apache
$ docker build -t nextcloud-debug:26 .
$ docker-compose up

What I could suspect:

• somehow you're not using Nextcloud 26 and the patch is not there, or the image is not built with or don't use OBJECTSTORE_S3_STORAGE_CLASS environment variable for whatever reason
• the behaviour from your s3 provider is different and for some reason it falls back to STANDARD (would surprise me as from my tests, when the storage class was invalid, there was an invalid response from the provider and got 500/Internal Server Error)
• another bug I don't see

Last note: if storageClass is not set in the configuration, it forces STANDARD (cf #36075 (comment))

[aentwist]

Good grief man I didn't check the release version this made it into. I figured with CI/CDs, since this is a feature and not a breaking change, that it had already made it out after a couple months into Nextcloud 25 and the corresponding Docker image. Can confirm it works flawlessly in Nextcloud 26 using my setup described above + S3. Thanks for this sanity check, and for your work on this.