feat(nextcloud): Enable including custom Nginx configurations and show X-Real-IP in Nextcloud logs when Nginx is enabled

[JulesTriomphe]

This PR adds an option to include custom Nginx configuration. It also configures Nextcloud logs to show the X-Real-IP header when Nginx is enabled.

[stavros-k]

I'm wondering if we should just add an include directive in the nginx config and let users provide their own configurations.
It will become cumbersome to maintain/update the list of IPs and I imagine if this gets in,
that a lot of such customization will be requested/added afterwards, which then each will need it's own set of options.

What do you think?

[JulesTriomphe]

That would be ideal. I would leave default Cloudflare configuration however since it's used by many users.

To reduce the maintenance burden, we can generate the Cloudflare IPs on pod startup.

So we could do something like this:

• Have an option in questions.yaml to include custom configuration. This would make visible an option to mount an extra volume at /etc/nginx/includes/
• Add an include statement below the location / block, in the server.http section like include /etc/nginx/includes/*.conf if the custom configuration option is selected
• Have an option in questions.yaml to set real IPs from Cloudflare, which would activate (i.e. mount) the script in /docker-entrypoint.d/. This script would generate the list of Cloudflare IPs with set_real_ip_from directives in a /etc/nginx/includes/10-set-real-ip-from-cloudflare.conf file and would end with real_ip_header CF-Connecting-IP following the docs
• Keep the apache2 config change as this helps in any case

The 10-... prefix will allow users to create configs that will be evaluated before the cloudflare one, since they are evaluated alphabetically.

What do you think ?

[stavros-k]

I'm not sure I want having such scripts (eg for updating CF ips).

---

I think nginx will ingore includes if there are not files matched. So we can add it unconditionally, then present a storage option in the UI for nginx confs.

As for the apache log formats, what is the default/current format?
What happens if x-real-ip is empty?

[JulesTriomphe]

I'm not sure I want having such scripts (eg for updating CF ips).

Sure, this can anyways be done outside the app, e.g. with a cronjob.

I think nginx will ingore includes if there are not files matched. So we can add it unconditionally, then present a storage option in the UI for nginx confs.

Sounds good.

As for the apache log formats, what is the default/current format?

This is what's currently configured in /etc/apache2/apache2.conf:

#
# The following directives define some format nicknames for use with
# a CustomLog directive.
#
# These deviate from the Common Log Format definitions in that they use %O
# (the actual bytes sent including headers) instead of %b (the size of the
# requested file), because the latter makes it impossible to detect partial
# requests.
#
# Note that the use of %{X-Forwarded-For}i instead of %h is not recommended.
# Use mod_remoteip instead.
#
LogFormat "%v:%p %h %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"" vhost_combined
LogFormat "%h %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"" combined
LogFormat "%h %l %u %t \"%r\" %>s %O" common
LogFormat "%{Referer}i -> %U" referer
LogFormat "%{User-agent}i" agent

The log format being used in logs is the combined one.

What happens if x-real-ip is empty?

It isn't, since we're sending the header from Nginx in nginx.conf:

proxy_set_header X-Real-IP         $remote_addr;

But if it is, then the (%{X-Real-IP}i) directive will simply become (), showing that something is missing.

[JulesTriomphe]

Just saw that nginx isn't always included: it's only included if a certificate ID is provided. So we should only change the Apache LogFormat directives when nginx is included.

[stavros-k]

I believe we can simplify this a lot.
for example, ix volumes makes no sense here

Also the mount toggle can be omitted and instead have a list of confs to mount.
simple strings.

Then on the compose template, we mount each one like this

/etc/nginx/includes/<idx>.conf (So it will also be loaded in the same order as defined.

[JulesTriomphe]

Thank you for the feedback. I've made the changes you suggested. Let me know if I understood them right.

[JulesTriomphe]

The tests are failing because the files (custom confs) don't exist. Do you have a way to create them before running the tests? Or should I remove the entries from the custom_confs list?

[stavros-k]

btw, take a look here #1351 (comment)
what do you think?

[JulesTriomphe]

btw, take a look here #1351 (comment) what do you think?

From experience, although bundling everything in the same image seems simpler, it ends up being more of a pain in the long run because the image becomes bloated, dependencies are mixed, so changes are harder, which makes maintenance harder too.
Either that or you need to keep the customisations to a minimum, but then you'll be driving users away because your offering won't fit their needs. I suppose most of your users like to tinker a bit (they are self-hosting TrueNAS after all, or some apps at least), so I doubt restricting their freedom will help you grow your user-base or the appeal of the offering.

[stavros-k]

btw, take a look here #1351 (comment) what do you think?

From experience, although bundling everything in the same image seems simpler, it ends up being more of a pain in the long run because the image becomes bloated, dependencies are mixed, so changes are harder, which makes maintenance harder too. Either that or you need to keep the customisations to a minimum, but then you'll be driving users away because your offering won't fit their needs. I suppose most of your users like to tinker a bit (they are self-hosting TrueNAS after all, or some apps at least), so I doubt restricting their freedom will help you grow your user-base or the appeal of the offering.

When I say dependencies I mean deps like pdlib, smbclient etc.
Not for example imaginary or collabora etc. Those will still be seperate containers

[JulesTriomphe]

Ah, I see. Makes sense then. While smaller images are usually better, for Nextcloud startup time is more important IMO. I don't see how this will be a breaking change though?

[stavros-k]

Ah, I see. Makes sense then. While smaller images are usually better, for Nextcloud startup time is more important IMO. I don't see how this will be a breaking change though?

setup scripts that will configure the instance declaratively. rootless. and due to the huge changes, it won't be able to guarantee non-breaking change.

[stavros-k]

Lets add this information on the PR description and remove this file.
There is no plan to have changelogs on the apps UI in the near future.

[JulesTriomphe]

Sure. I thought you would given that it's prepared already: https://github.com/truenas/apps_validation/blob/master/catalog_reader/app.py#L167

[stavros-k]

LGTM, lets address the changelog comment only and merge it.
Thansk