search

nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.
GNU Affero General Public License v3.0
325 stars 264 forks source link

Sync config files from nextcloud/docker repo; Add S3/Swift object storage config; Add S3 ci test; Add `nextcloud.trustedDomains` #464

Closed jessebot closed 2 months ago

jessebot commented 10 months ago

Description of the change

[!Note] If you were already manually providing s3.config.php, reverse-proxy.config.php, swift.config.php, or upgrade-disable-web.config.php, you can disable us deploying a default file for you by setting the respective nextcloud.defaultConfigs.FILE to false. For instance, to disable the default s3.config.php, you would set nextcloud.defaultConfigs.s3.config.php=false.

Benefits

This lets users use both nextcloud.configs and still use the nextcloud/docker auto configuration via environment variables. Prior to this change, you had to use one or the other when it came to s3 as primary object storage.

Possible drawbacks

not sure 🤷 but always open to feedback :)

Applicable issues

Checklist

jessebot commented 10 months ago

Since it's taking a bit for my PR to be merged upstream, I'll try to spend some time on this later today to get it ready for merge without the encryption feature to start with.

Perhaps it also makes sense to have a nextcloud.objectStore.s3 section in the values.yaml like (maybe in a second PR as I'd need to play with the deployment template and add an s3 secret):

nextcloud:
  objectStore:
    s3:
      enabled: true
      # ignored if nextcloud.objectstore.s3.existingSecret is not empty string
      accessKey: ""
      # ignored if nextcloud.objectstore.s3.existingSecret is not empty string
      secretKey: ""
      # only required if you're not using AWS
      endpoint: ""
      # default port that can be changed based on your object store, e.g. for minio, you can use 9000
      port: "443"
      # this is the default in the nextcloud docs
      region: "eu-west-1"
      bucket: ""
      ssl: true
      # set to true if you are not using DNS for your buckets.
      usePathStyle: false
      existingSecret: ""
jessebot commented 10 months ago

I've removed the encryption key setting for now, so this could still be merged, but I haven't tested this, so it should still wait another week or two while I make sure it works without the encryption key.

NikoKS commented 7 months ago

any plan for this to be merged?

NikoKS commented 7 months ago

any blocker to this @jessebot

jessebot commented 5 months ago

No, I just was just really busy before. Let me take a look at the code now and see what's left to be done here :)

jessebot commented 2 months ago

gonna take a peak locally and push up some changes as https://github.com/nextcloud/docker/pull/2151 was merged so we now have encryption as well :)

jessebot commented 2 months ago

This should be ready for review now :) Unless #480 gets merged first, then I need to update this PR again.

provokateurin commented 2 months ago

I'd rather wait for the other PR, it has been open so long already.

jessebot commented 2 months ago

Sounds good to me :)

jessebot commented 2 months ago

As discussed in #480, I will refactor this to use the new config file format and then we can review and merge.

jessebot commented 2 months ago

Actually, in the interest of resolving https://github.com/nextcloud/helm/issues/477, should I just go ahead and add the following to the values?

nextcloud:
  objectStore:
    s3:
      enabled: true
      # ignored if nextcloud.objectstore.s3.existingSecret is not empty string
      accessKey: ""
      # ignored if nextcloud.objectstore.s3.existingSecret is not empty string
      secretKey: ""
      # only required if you're not using AWS
      endpoint: ""
      # default port that can be changed based on your object store, e.g. for minio, you can use 9000
      port: "443"
      # this is the default in the nextcloud docs
      region: "eu-west-1"
      bucket: ""
      ssl: true
      # set to true if you are not using DNS for your buckets.
      usePathStyle: false
      existingSecret: ""

Open to suggestions and feedback on better organizing this, but it feels like we could accommodate this, rather than making users manually set the env vars?

provokateurin commented 2 months ago

How about adding the other missing configs (especially https://github.com/nextcloud/docker/blob/master/.config/upgrade-disable-web.config.php) and checking if the other ones are up-to-date?

jessebot commented 2 months ago

How about adding the other missing configs (especially https://github.com/nextcloud/docker/blob/master/.config/upgrade-disable-web.config.php) and checking if the other ones are up-to-date?

Sure, why not :)

provokateurin commented 2 months ago

Open to suggestions and feedback on better organizing this, but it feels like we could accommodate this, rather than making users manually set the env vars?

Agreed, this is probably a commonly used setting and it's easier to do it this way. Internally it could also use the autoconfig env vars though or just it's own config map (I don't really have a preference, as long as both work fine).

jessebot commented 2 months ago

How about adding the other missing configs (especially https://github.com/nextcloud/docker/blob/master/.config/upgrade-disable-web.config.php) and checking if the other ones are up-to-date?

Sure, why not :)

ok, this part is done. All configs in our new charts/nextcloud/files/defaultConfigs dir match those in nextcloud/docker :)

I'll now work on adding the nextcloud.objectStore.s3 and I guess nextcloud.objectStore.swift while I'm at it? Although I have no way to really test swift at home, so I'd be adding it and ensuring basic tests pass, but I couldn't really support it very much as I don't know how to emulate swift locally, unlike s3, which we can install minio conditionally in the test_cases to verify all is working.

jessebot commented 2 months ago

OK, I know I need to fix that last commit not being signed (weird because I set my local git config to always sign, but whatever), but when that's done, do we want to add a test for s3 as primary storage? 🤔

Also, anything else stick out to you before I do the big squash into one commit?

jessebot commented 2 months ago

Looks good, a test would be very nice (for swift as well if that is somehow possible and you want to give it a try)

I can work on the S3 test, but Swift is OpenStack, which is a massive can of worms that I just don't have the time to commit to right now :( (If others in the community are reading this and are familiar with Swift, we'd love your contributions on this one!)

jessebot commented 2 months ago

This is the first attempt at adding an S3 test. One of the things I'm not actually sure how to do from the cli is add a file to nextcloud for a specific user? Open to suggestions on how to test that. 🙏

I found this: https://docs.nextcloud.com/server/18/user_manual/files/access_webdav.html#accessing-files-using-curl

I guess maybe we need to set a default password for the nextcloud admin user and then We already set a deafault user name and password, so I guess I can use that? 🤔

jessebot commented 2 months ago

I ended up adding a job to curl a file into place using: https://github.com/nextcloud/helm/blob/dc77117d71f73c7dc721a4cae5748ef54c080780/.github/test_upload_job.yaml#L18-L23

The job gets applied right after we install nextcloud using ct, we wait for it, and then tail the logs: https://github.com/nextcloud/helm/blob/dc77117d71f73c7dc721a4cae5748ef54c080780/.github/workflows/lint-test.yaml#L141-L146

Oh no, I just realized I forgot the -l on the for label on the selector for the kubectl logs 🤦 I'll fix that.

It should still get to through everything else though, so in the meantime the current set of jobs running should still give us insights :) https://github.com/nextcloud/helm/actions/runs/10077836913/job/27861738508

jessebot commented 2 months ago

When I try to do the curl in the test job, I get Access through untrusted domain. Well, it actually spits out all the following HTML which is hard to read, but here it is if you want:

click me for full HTML output ```html Nextcloud

Nextcloud

Access through untrusted domain

Please contact your administrator. If you are an administrator, edit the "trusted_domains" setting in config/config.php like the example in config.sample.php.


Further information how to configure this can be found in the documentation.

Nextcloud – a safe home for all your data

```

So, can I just add nextcloud.nextcloud.svc.cluster.local as a trusted domain? 🤔

provokateurin commented 2 months ago

Yeah either add that directly or add * which will just allow all domains which is fine for testing.

jessebot commented 2 months ago

OK, I've added nextcloud.trustedDomains, which has been an on and off suggestion anyway. It templates to NEXTCLOUD_TRUSTED_DOMAINS which nextcloud/docker recognizes, so seems fine to me 🤷 Update: fixed it so it only takes a custom value if you pass in a list, else it uses our default templating we already had in place

jessebot commented 2 months ago

I can't figure out why I can't get the final s3 test helm chart install to work anymore 🤔 I'll try again tomorrow.

jessebot commented 2 months ago

The test works!!! 🎉 🕺 💃 🥳
https://github.com/nextcloud/helm/actions/runs/10090274007/job/27899378354?pr=464#step:10:38

jessebot commented 2 months ago

OK! This is finally ready for review, @provokateurin :)

provokateurin commented 2 months ago

Very nice work btw :heart:

jessebot commented 2 months ago

Could you try to separate the changes into a few commits e.g.:

  1. update existing config files

  2. add new/missing config files

  3. add s3 and swift values.yaml

  4. add s3 test

It doesn't need to be exactly these commits, but if anything here breaks something down the road it will be a lot easier to bisect if it's not a single commit.

Sure, I can do that when I get home later. I assumed you'd ask for it to be in all one commit, so I did this to solve that, but also, since we typically do a squash and merge, does this mean you'd like me to do a normal merge commit after I finish teasing apart the commits? Happy to do it, just making sure :)

provokateurin commented 2 months ago

Well I prefer a clean history where every commit has a purpose and does only one thing. So neither fixup commits nor mega commits are great, just smallish concrete ones.

jessebot commented 2 months ago

OK, I think all the commits are reasonable now. It took me kind of a while to get it together, because splitting a commit can't really be done in any easy way, but like, we should be good now!

Oh, when merging, do you want to do a "merge commit" or "rebase and merge"?

provokateurin commented 2 months ago

Merge commit ;)

But honestly do what you find best, as long as it works for you :woman_shrugging:

jessebot commented 2 months ago

Okie dokie. I still need an approval, but then we can get this out the door :)