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

Connect mastercontainer to docker network specified by APACHE_ADDITIONAL_NETWORK #5539

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

apparle
Copy link
Collaborator

@apparle apparle commented Nov 7, 2024

This is an extension of #5484 to allow for mastercontainer's admin page to also be behind a reverse proxy (for security, authentication etc.) instead of just an unsecured localhost port. For example, I've my reverse proxy set up to point https://aio-nextcloud.mydomain.com to https://nextcloud-aio-mastercontainer:8080 for the admin page.

Since the user is specifying APACHE_ADDITIONAL_NETWORK as a means to connect the reverse proxy, it is natural to use the same information for mastercontainer's reverse proxy setup as well. Yes in theory, the user can create this connection by docker commands or docker compose file (I do this today), but then the user must specify not just the network for proxy but also create nextcloud-aio doing something like below:

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

This isn't easy or obvious for newcomer, and it'd be much more seamless if handled automatically. FWIW, there's some precedence for mastercontainer manipulating its own network -- the default compose.yml suggests network_mode: bridge, then mastercontainer connects itself to nextcloud-aio and then later disconnects itself from the bridge through a cron job. Since we're manipulating the network, it's a relatively incremental extension to connect mastercontainer with reverse proxy with minimal downsides.
And the start.sh change also has the nice side-benefit of doing an early error check with clear error message if the specified network has issues.

After this change, 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. And potentially authentication schemes as well.
  2. User sets up 2 different routes:
    • Some aio URL (eg: https://aio-nextcloud.mydomain.com) pointing to https://nextcloud-aio-mastercontainer:8080 for the admin page.
    • Main nextcloud URL (eg: 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.)

@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature or request labels Nov 7, 2024
@szaimen szaimen added this to the next milestone Nov 7, 2024
@szaimen szaimen requested review from docjyJ and szaimen November 7, 2024 12:45
@szaimen szaimen modified the milestones: v9.9.0, next Nov 7, 2024
@apparle apparle requested a review from docjyJ November 12, 2024 01:23
Copy link
Collaborator

@docjyJ docjyJ left a comment

Choose a reason for hiding this comment

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

LGTM

@szaimen
Copy link
Collaborator

szaimen commented Dec 17, 2024

Yes in theory, the user can create this connection by docker commands or docker compose file (I do this today), but then the user must specify not just the network for proxy but also create nextcloud-aio doing something like below:

No, actually the container will always be connected to a nextcloud-aio network even if you don't specify it in the docker-compose file (as soon as you open the aio-interface). So this is a non-argument.

And sorry for my late reply btw...

Copy link
Collaborator

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Honestly I still dislike this idea. I think it is better to simply add docs on the topic and add the external network via docker-compose if you really want to. The mastercontainer will only remove the default bridge network and add itself to the nextcloud-aio network (and will even create it if it does not exist) but will not modify the network config any further.

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 17, 2024
@szaimen szaimen removed this from the next milestone Dec 17, 2024
@apparle
Copy link
Collaborator Author

apparle commented Dec 17, 2024

Yes in theory, the user can create this connection by docker commands or docker compose file (I do this today), but then the user must specify not just the network for proxy but also create nextcloud-aio doing something like below:

No, actually the container will always be connected to a nextcloud-aio network even if you don't specify it in the docker-compose file (as soon as you open the aio-interface). So this is a non-argument.

And sorry for my late reply btw...

Maybe this was the case some time ago, because I remember this being stated in the default compose file itself. If this is no more a requirement then I agree that directly specifying the frontend network in compose file is better than an environment variable. Let me do some testing.
I'm traveling so it'll take some time before I can test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants