Specifying additional docker network for Apache container

[apparle]

This is a fix for #5482

[szaimen]

Thank you for your PR! :)

I have some remarks below

[szaimen]

Additionally, is it possible to add a check for this variable in start.sh so that only allowed characters are actually used?
See for example something like this but for strings:

all-in-one/Containers/mastercontainer/start.sh

Lines 179 to 188 in af975f6

	if [ -n "$APACHE_PORT" ]; then
	if ! check_if_number "$APACHE_PORT"; then
	print_red "You provided an Apache port but did not only use numbers.
	It is set to '$APACHE_PORT'."
	exit 1
	elif ! [ "$APACHE_PORT" -le 65535 ] || ! [ "$APACHE_PORT" -ge 1 ]; then
	print_red "The provided Apache port is invalid. It must be between 1 and 65535"
	exit 1
	fi
	fi

[apparle]

@szaimen I've made a bunch of changes incorporating your suggestions, added support for domaincheck container to make it all seamless and updated documentation. I've tested this to work starting from scratch, including the domain check, and it all works great.
Can you take a look?

PS: I was a little careless with my local commits messages so I manually squashed all the commits.

[docjyJ]

LGTM

[szaimen]

I have some additional suggestions.

Please also add your new env to https://github.com/nextcloud/all-in-one/blob/main/tests/QA/060-environmental-variables.md

[apparle]

I replied inline but apparently my comments are all "pending" and I didn't realize they aren't posted for everyone. I'm not very familiar with github UI and clearly my interpretation was wrong. Let me post comments directly, not inline.

[apparle]

The intent of both changes above is to allow for the master container to also be exposed via a reverse proxy. For example, I've my reverse proxy point https://aio-nextcloud.mydomain.com to https://nextcloud-aio-mastercontainer:8080 to access the admin page.

The default compose.yml suggests network_mode: bridge, then internally connects the mastercontainer to nextcloud-aio and then later disconnects the mastercontainer from the bridge through cron job. Since we're manipulating the network, if a user is specifying an additional network for proxy setup through APACHE_ADDITIONAL_NETWORK, it's more seamless to have mastercontainer also be accessible through the same connection.
Yes, the user can create this connection manually -- but then the user must specify not just the network for proxy but also create nextcloud-aio doing something like below, which is again not obvious for a naive user.

services:
  nextcloud-aio-mastercontainer:
    image: nextcloud/all-in-one:latest
...
    networks:
      - nextcloud-aio 
      - front_net

networks:
  front_net:
    external: true
  nextcloud-aio:
    name: nextcloud-aio
    driver: bridge

Instead, with the mastercontainer changes I've proposed, I imagine the end-user flow to be:

1. User sets up some reverse proxy container with SSL in a docker container. Eg: Nginx Proxy Manager with let's encrypt + DuckDNS is extremely simple GUI based and amateur proof. There's at least 3-4 tutorials I've seen on internet which are straight forward to follow.
2. User sets up 2 different routes (just blindly):
   • Some aio URL (https://aio-nextcloud.mydomain.com) pointing to https://nextcloud-aio-mastercontainer:8080 for the admin page.
   • Main nextcloud URL (https://nextcloud.mydomain.com) pointing to http://nextcloud-aio-apache:11000.
3. User launches the Nextcloud AIO with default compose file and specifies APACHE_ADDITIONAL_NETWORK=<the network name of reverse proxy>.
4. Follow all the steps in GUI, and because reverse proxy is already set up, everything including domaincheck just works.
   (FWIW, I do intend to post this as a guide somewhere after this pull request goes through.)

At a higher level, I suppose it's a software-architecture vs. user-experience question on how much do we do from within the mastercontainer for the sake of consistent user-experience & simplicity between both mastercontainer & apache container vs. leave it up to user to manipulate the compose file. Personally, I think this is a simple enough extension to mastercontainer with minimal downsides, and enables the super-simple reverse proxy + nextcloud setup that I described in above comment. And the start.sh change also has the nice side-benefit of doing an early error check with clear error message in red on the docker log, if the specified network has issues.

[szaimen]

Hi @apparle thanks for the detailed explanation!

I was wondering though if we could handle the mastercontainer networking change in a separate PR?
(I'd like to merge this PR here without the mastercontainer network change and then do the mastercontainer stuff in a follow-up)

Is that fine for you?

[apparle]

Hi @apparle thanks for the detailed explanation!

I was wondering though if we could handle the mastercontainer networking change in a separate PR? (I'd like to merge this PR here without the mastercontainer network change and then do the mastercontainer stuff in a follow-up)

Is that fine for you?

Sounds good, let me make that change.

[szaimen]

LGTM, thanks!

[szaimen]

This is now released with v9.9.0 Beta. Testing and feedback is welcome! See https://github.com/nextcloud/all-in-one#how-to-switch-the-channel

[ctag]

Just tested on :beta to fix a timeout issue setting up collabora. It worked for fixing the timeout in the settings page, but then I immediately ran into nextcloud/files_pdfviewer#1080 apparently and no documents will open for editing to allow me to test the editor.