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

Adds ability to configure S3 object stores via environment variables #1193

Closed

Conversation

adamjenkins1
Copy link
Contributor

Allows consumers of the Nextcloud docker image to configure S3 compatible object storage as Nextcloud's primary storage through environment variables. Supporting the configuration of this feature through the environment will make the image that much easier to leverage. Ref: https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/primary_storage.html#simple-storage-service-s3

@adamjenkins1
Copy link
Contributor Author

adamjenkins1 commented Jul 31, 2020

@pierreozoux, would you be able to take a look at this? Most of the content is from that previous commit you mentioned in #1135.

@J0WI
Copy link
Contributor

J0WI commented Aug 19, 2020

Nextcloud with S3 has some serious issues:
nextcloud/server#11826
nextcloud/server#14027
nextcloud/server#22077

@adamjenkins1
Copy link
Contributor Author

Nextcloud with S3 has some serious issues:
nextcloud/server#11826
nextcloud/server#14027
nextcloud/server#22077

The fact that encryption isn't working when Nextcloud's primary storage is a S3 object store is definitely an issue, but I don't see why that should mean that we shouldn't support this feature through the docker image.

@pierreozoux
Copy link
Member

Looks good to me and I agree with @adamjenkins1 Encryption is still beta.

@pierreozoux
Copy link
Member

@J0WI what do we need to merge this?

.config/s3.config.php Outdated Show resolved Hide resolved
Copy link
Contributor

@cuppett cuppett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid requiring HOST, the if block needs changed.

Another thought is should each entry getting added to the array independently? This could break if keys are added to the config array and empty string triggers unintended logic.

I'm thinking about my recent PR (nextcloud/server#20891), we'd want to leave out key and secret in this case to pick up the IAM role correctly in version 20+.

.config/s3.config.php Outdated Show resolved Hide resolved
.config/s3.config.php Outdated Show resolved Hide resolved
.config/s3.config.php Outdated Show resolved Hide resolved
.config/s3.config.php Outdated Show resolved Hide resolved
@pierreozoux
Copy link
Member

pierreozoux commented Aug 26, 2020

@cuppett the ideal would be the php code to read env var.

Do you want to care of that upstream in the serveur code? I Think it is the best place.

.config/s3.config.php Outdated Show resolved Hide resolved
@cuppett
Copy link
Contributor

cuppett commented Aug 26, 2020

I've done testing with various versions of this file. I was able to emulate the default behaviors of the param file wrt set/unset environment variables and trues/falses here:

https://github.com/cuppett/nextcloud-openshift-s2i/blob/master/autoconfig/objectstorage.config.php

A container with this available is here: quay.io/cuppett/nextcloud:latest (uses Apache, Nextcloud 19 and /var/www/html as the storage location)

Can we catch the PR code and doc stanza up to this?

@pierreozoux
Copy link
Member

@cuppett I think you are the most knowlegble about this, feel free to update things :) Or if you prefer create a new PR if it is faster, I'll just trust you and merge it :)

@cuppett
Copy link
Contributor

cuppett commented Aug 26, 2020

I've opened a PR for @adamjenkins1 (https://github.com/adamjenkins1/docker/pull/1/commits).

If he merges it pulls in a rebase & has all these updates. Should then show here, we can merge and closes the issues!

@cuppett
Copy link
Contributor

cuppett commented Aug 26, 2020

@pierreozoux We have it here now.

@cuppett cuppett self-requested a review August 26, 2020 17:47
Copy link
Contributor

@cuppett cuppett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have caught up to master with rebase and the cleanups.

@pierreozoux
Copy link
Member

pierreozoux commented Aug 26, 2020

Hum.. actually there are now 2 files objectstore and s3.
Plus I'd prefer to either have one commit, or 2 separate commit with the updated separated (easier to read)

As you want, if you don't mind, I'll redo the PR and put your names there. I'll take the objectstore file then.

@adamjenkins1
Copy link
Contributor Author

Hum.. actually there are now 2 files objectstore and s3.
Plus I'd prefer to either have one commit, or 2 separate commit with the updated separated (easier to read)

As you want, if you don't mind, I'll redo the PR and put your names there. I'll take the objectstore file then.

Yeah I just realized that. I can fix it.

Signed-off-by: Adam Jenkins <[email protected]>

Rebase and update S3 config (#1)

* Run update.sh

Signed-off-by: tilosp-bot <[email protected]>

* Run update.sh

Signed-off-by: tilosp-bot <[email protected]>

* Run update.sh

Signed-off-by: tilosp-bot <[email protected]>

* Adds ability to configure S3 object stores via environment variables

Signed-off-by: Adam Jenkins <[email protected]>

* Updates objectstore autoconfiguration to match server defaults

- usepath defaults to false (and is deprecated by AWS)
- autocreate is not used/necessary by the objectstorage code (always tries to create)

Signed-off-by: Stephen Cuppett <[email protected]>

* Dropping NULL check

Co-authored-by: tilosp-bot <[email protected]>
Co-authored-by: Tilo Spannagel <[email protected]>
Co-authored-by: Adam Jenkins <[email protected]>
Signed-off-by: Adam Jenkins <[email protected]>
@adamjenkins1
Copy link
Contributor Author

Is there anything else that needs to be done before we can merge this?

pierreozoux added a commit that referenced this pull request Aug 28, 2020
pierreozoux added a commit that referenced this pull request Aug 28, 2020
pierreozoux added a commit that referenced this pull request Aug 28, 2020
@pierreozoux
Copy link
Member

@adamjenkins1 sorry, it is just there was too many files in your commit, difficult to review, and not in sync with master.

I created a new PR with the code, and merged it.

Thanks @adamjenkins1 and @cuppett for your help on getting this done :)

i put you as coauthor, I hope that's fine :)

pierreozoux added a commit that referenced this pull request Aug 28, 2020
pierreozoux added a commit that referenced this pull request Aug 29, 2020
…1227)

* Adds ability to configure S3 object stores via environment variables

closes #1193 #1124 #1134

Co-authored-by: Adam Jenkins <[email protected]>
Co-authored-by: Stephen Cuppett <[email protected]>

* Update .config/s3.config.php

Co-authored-by: Adam Jenkins <[email protected]>
Co-authored-by: Stephen Cuppett <[email protected]>
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.

Configuring S3 (or compatible object storage) as Primary Storage Autoconfiguration for S3 as Primary Storage
4 participants