Set `nextcloud.podSecurityContext.fsGroup` to `33` by default and allow users to configure it if needed. #379

jessebot · 2023-04-16T15:22:01Z

Pull Request

Description of the change

Changes default fsGroup to be 33. In all apache and regular fpm, the www-data user is 33.

The www-data user is 82 in all alpine images (including nginx), so I added a note in the README about it.

Previously we relied on if the user was using nginx.enabled which is alpine by default, but using nginx:alpine doesn't mean you're using nextcloud:fpm-alpine.

Benefits

If someone uses image.flavor of apache or fpm, the fsGroup should be 33 by default.

Possible drawbacks

If you used an alpine image, you will now have to manually set nextcloud.podSecurityContext.fsGroup to be 82 in your values.yaml.

Applicable issues

partially fixes nextcloud-nginx container crashlooping after securityContext update; /var/www/html/config always owned by root #335

Additional information

Checklist

DCO has been signed off on the commit.
Chart version bumped in Chart.yaml according to semver.
(optional) Variables are documented in the README.md

charts/nextcloud/templates/deployment.yaml

jessebot · 2023-04-23T20:26:16Z

just need to rebase to fix the commits, but this seems to be okay now I think

provokateurin · 2023-04-24T04:08:00Z

The name of the commit/PR is a bit misleading because fsGroup was 33 by default if using apache, so nothing changes for those cases.

jessebot · 2023-04-24T10:24:28Z

Fixed the PR and can fix the commits. does this work?

Always podSecurityContext.fsGroup to 33, even if nginx is enabled

Happy to change it to whatever makes most sense. Also, thank you again for all your patience through this. It's been a doozy 😅

jessebot · 2023-04-24T14:16:11Z

fixed commit message :)

jessebot · 2023-04-24T14:31:02Z

Looking into why CI is failing. Submitted this PR, which will at least help with the warnings.

jessebot · 2023-04-24T14:35:05Z

Perhaps it just doesn't like the git push --force-with-lease I did earlier? 🤔 It failed on listing changing, but passed on previous checks.

jessebot · 2023-04-24T14:55:42Z

Getting this error still in the linting step in the github workflow:

Run ct lint --config ct.yaml
  ct lint --config ct.yaml
  shell: /usr/bin/bash -e {0}
  env:
    CT_CONFIG_DIR: /opt/hostedtoolcache/ct/v3.8.0/x86_64/etc
    VIRTUAL_ENV: /opt/hostedtoolcache/ct/v3.8.0/x86_64/venv
Linting charts...
Error: failed linting charts: failed identifying charts to process: failed running process: exit status 128
------------------------------------------------------------------------------------------------------------------------
No chart changes detected.
------------------------------------------------------------------------------------------------------------------------
failed linting charts: failed identifying charts to process: failed running process: exit status 128
Error: Process completed with exit code 1.

maybe it has something to do with the fact that we changed the default branch from master to main, and when I opened this PR, it was against "master" 🤔 I could open a new PR and see if it persists? 🤷

jessebot · 2023-04-24T15:03:03Z

seems to be happening on another PR as well, checking something, going to do a test commit here

jessebot · 2023-04-24T15:31:23Z

ok, fixed the additional warnings in #387 after testing here. Will undo the commit here where #387 is merged, then rebase again, and rerequest review after the GHA check is passing.

jessebot · 2023-04-24T15:55:22Z

github actions queuing is so slow:

provokateurin · 2023-04-24T16:03:50Z

Yeah, Nextcloud uses a lot of GH actions and there is a max limit per org. I think some people are experimenting with adding our own runners to speed this up.

jessebot · 2023-04-24T16:12:01Z

Oh, thank you for letting me know! Then I will not try to cancel and re-run them. Sorry about doing that earlier. I only tried to cancel and rerun one of them after it'd been over 15 minutes, because I thought there was something wrong with my own rate limiting, because I do a lot of stuff on github. 😅

jessebot · 2023-04-24T16:12:47Z

CI finished, this is a working PR, finally!

charts/nextcloud/README.md

add note in README about alpine containers needing to change to 82 manually this ensures our defaults work when using image.flavor fpm or apache Signed-off-by: Jesse Hitch <[email protected]>

Signed-off-by: JesseBot <[email protected]> Signed-off-by: Jesse Hitch <[email protected]>

Signed-off-by: JesseBot <[email protected]>

jessebot · 2023-11-08T19:57:47Z

I haven't merged this because I switched to using the image.flavor: fpm-alpine exclusively, which doesn't have the issues with at least mounting PostgreSQL SSL certs, and then I started using s3 for the primary object store, so I stopped having to deal with backups running as root for nextcloud's persistent volumes, because I'm not using them anymore, which means I can't do a final test to make sure this is solid. If this is breaking anyone else's flow and they really need it, we can still resolve the conflicts and merge it, but I can't easily test this at this moment. I'll close this for now, but if others like, they can ask that I either reopen this, or submit their own PR. Apologies for all the back and forth Kate 💙

jessebot · 2023-11-29T15:06:52Z

ok, I have to begrudgingly reopen this because I want to solve #367 but if this is not merged, then I cannot test using the fpm container with no nginx container.

Signed-off-by: JesseBot <[email protected]>

wrenix · 2024-09-12T18:55:35Z

charts/nextcloud/README.md

@@ -136,7 +138,8 @@ The following table lists the configurable parameters of the nextcloud chart and
 | `nextcloud.extraVolumes`                                   | specify additional volumes for the NextCloud pod                                                    | `{}`                       |
 | `nextcloud.extraVolumeMounts`                              | specify additional volume mounts for the NextCloud pod                                              | `{}`                       |
 | `nextcloud.securityContext`                                | Optional security context for the NextCloud container                                               | `nil`                      |
-| `nextcloud.podSecurityContext`                             | Optional security context for the NextCloud pod (applies to all containers in the pod)              | `nil`                      |
+| `nextcloud.podSecurityContext`                             | Optional security context for the NextCloud pod (applies to all containers in the pod)              | `{fsgroup: 33}`            |
+| `nextcloud.podSecurityContext.fsGroup`                     | special supplemental group that applies to all containers in the NextCloud pod                      | `33`                       |


duplicated with the line above

should I combine the description of both? 🤔 Do you prefer we keep the parameter line of 141 or 142? (also congrats on being a collaborator now!! 🎉 )

wrenix · 2024-09-12T19:04:26Z

charts/nextcloud/values.yaml

+    # runAsUser: 33
+    # runAsGroup: 33
+    # runAsNonRoot: true
+    # readOnlyRootFilesystem: false


is not part of podSecurityContext

wrenix · 2024-09-12T19:05:45Z

charts/nextcloud/templates/deployment.yaml

@@ -352,19 +352,12 @@ spec:
        {{- toYaml . | nindent 8 }}
        {{- end }}
      securityContext:
+        # this is deprecated and will be removed in a future release - use nextcloud.podSecurityContext instead


Should we drop securityContext now? And announce it as a breaking change?

I'm ok with this

provokateurin reviewed Apr 16, 2023

View reviewed changes