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

containers definition - allow to define caddy_routes #3192

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

szaimen
Copy link
Collaborator

@szaimen szaimen commented Aug 17, 2023

Address part of #3889

todo

  • merge it with containersfetcher
  • retreive it on container start
  • merge it as env
  • geneerate the files as imports during startup of apache
  • store them in tmpfs volume instead of in volume? (if so, remove some of the logic again from startup...
  • remove it from update-yaml.sh
  • maybe also use it for default apps? improves testing

@szaimen szaimen added 2. developing Work in progress enhancement New feature or request labels Aug 17, 2023
@szaimen szaimen added this to the next milestone Aug 17, 2023
@szaimen szaimen modified the milestones: v7.1.0, next, v7.1.1 Aug 31, 2023
@szaimen szaimen modified the milestones: v7.2.0, next, v7.2.1 Sep 13, 2023
@szaimen szaimen modified the milestones: v7.3.0, next Sep 21, 2023
@szaimen szaimen force-pushed the enh/noid/allow-define-containers-definition branch from 082d37c to 475c28e Compare September 27, 2023 12:31
@szaimen szaimen removed this from the next milestone Sep 27, 2023
@docjyJ docjyJ mentioned this pull request Mar 19, 2024
7 tasks
szaimen added a commit that referenced this pull request May 24, 2024
szaimen added a commit that referenced this pull request May 24, 2024
In favour of #3192
Signed-off-by: Simon L <[email protected]>
szaimen and others added 3 commits July 14, 2024 14:44
@docjyJ docjyJ force-pushed the enh/noid/allow-define-containers-definition branch from ddf198f to 70224e6 Compare July 14, 2024 13:28
@docjyJ
Copy link
Collaborator

docjyJ commented Jul 14, 2024

Testing script for caddy.sh

#!/bin/bash

export CADDY_ROUTES="|/browser,0,nextcloud-aio-collabora,9980;/hosting,0,nextcloud-aio-collabora,9980;/cool,0,nextcloud-aio-collabora,9980;/push,1,nextcloud-aio-notify-push,7867;/standalone-signaling,1,nextcloud-aio-talk,8081@mail.|,0,[email protected].|/a/b,0,server;/a/b/c,0,server;,0,server;/a,0,server"
export APACHE_PORT="443"
export APACHE_HOST="localhost"
export TRUSTED_DOMAINS="{\$ADDITIONAL_TRUSTED_DOMAIN},{\$NC_DOMAIN}"

export CADDY_ROUTES PROTOCOL NC_DOMAIN APACHE_PORT TRUSTED_DOMAINS

bash caddyfile.sh

Output :

{
    auto_https disable_redirects

    storage file_system {
        root /mnt/data/caddy
    }

    servers {
        trusted_proxies static 127.0.0.0/16
    }

    log {
        level ERROR
    }
}


https://verif.sort.{$ADDITIONAL_TRUSTED_DOMAIN}:443,
https://verif.sort.{$NC_DOMAIN}:443 {
    header -Server
    header -X-Powered-By

    route /a/b/c/* {
        
        reverse_proxy server
    }

    route /a/b/* {
        
        reverse_proxy server
    }

    route /a/* {
        
        reverse_proxy server
    }

    route {
        
        reverse_proxy server
    }

}

https://mail.{$ADDITIONAL_TRUSTED_DOMAIN}:443,
https://mail.{$NC_DOMAIN}:443 {
    header -Server
    header -X-Powered-By

    route {
        
        reverse_proxy nextcloud-aio-stalwart
    }

}

https://{$ADDITIONAL_TRUSTED_DOMAIN}:443,
https://{$NC_DOMAIN}:443 {
    header -Server
    header -X-Powered-By

    route /standalone-signaling/* {
        uri strip_prefix /standalone-signaling
        reverse_proxy nextcloud-aio-talk:8081
    }

    route /push/* {
        uri strip_prefix /push
        reverse_proxy nextcloud-aio-notify-push:7867
    }

    route /hosting/* {
        
        reverse_proxy nextcloud-aio-collabora:9980
    }

    route /cool/* {
        
        reverse_proxy nextcloud-aio-collabora:9980
    }

    route /browser/* {
        
        reverse_proxy nextcloud-aio-collabora:9980
    }

    route {
        header Strict-Transport-Security max-age=31536000;
        reverse_proxy localhost:8000
    }
    redir /.well-known/carddav /remote.php/dav/ 301
    redir /.well-known/caldav /remote.php/dav/ 301

    tls {
        issuer acme {
            disable_http_challenge
        }
    }
}

@docjyJ
Copy link
Collaborator

docjyJ commented Jul 14, 2024

What do you think @szaimen?

I tried to make it as generic as possible.

Known issue, subdomains are incompatible with trusted proxy mode.

Copy link
Collaborator Author

@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.

Hi @docjyJ , thanks a lot for working on this!

I had a look at your changes and I fear I have some general problems with this which is why I didnt push it further...

  1. We need to keep the Caddyfile as fallback for cases where the mastercontainer is not there (see the manual-install)
  2. I'd for now like to remove sub_domain from the implementation as this was actually only intended for routes --> since this then limits the use-case to pretty much to the already bundled services, I didn't really pushed this forward... However we could in theory move the onlyoffice container then to community-containers to make it easier to discover... But I am not certain about this yet.

Containers/apache/Caddyfile Outdated Show resolved Hide resolved
php/containers-schema.json Outdated Show resolved Hide resolved
Signed-off-by: Jean-Yves <[email protected]>
Signed-off-by: Jean-Yves <[email protected]>
Signed-off-by: Jean-Yves <[email protected]>
Signed-off-by: Jean-Yves <[email protected]>
@docjyJ
Copy link
Collaborator

docjyJ commented Jul 16, 2024

Hi!

I attempted to fix the issues you mentioned.

Don't hesitate to give feedback!

Take care,

Signed-off-by: Jean-Yves <[email protected]>
@docjyJ docjyJ marked this pull request as ready for review July 30, 2024 17:30
@docjyJ
Copy link
Collaborator

docjyJ commented Jul 30, 2024

@szaimen ready for review

@docjyJ docjyJ added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 30, 2024
@szaimen
Copy link
Collaborator Author

szaimen commented Aug 1, 2024

Hi @docjyJ sorry for my late reply.

So as I said, I would like to keep the Caddyfile as fallback if it is used with the manual-install.

You could adjust the logic in start.sh to copy the file from there if the variable CADDY_ROUTES was not found. Additionally, we then need to keep the env variables:
image


However I wonder how useful this change then is in the end if we need to maintain two places and keep it in sync without a real benefit at least in the current state? Sorry for bringing this only up so late 🙈

@szaimen
Copy link
Collaborator Author

szaimen commented Aug 1, 2024

This PR will make sense as soon as we have a real use-case for it, e.g. when a community-container needs such a route. As long as that is not the case, I think it does not make much sense to merge this. I am sorry 🙈

@docjyJ
Copy link
Collaborator

docjyJ commented Aug 1, 2024

No worries, I understand. It will come in handy someday.

By the way, what do you think of using Caddy without Apache: https://help.nextcloud.com/t/the-optimal-fastest-nextcloud-fpm-docker-setup-with-caddy-as-webserver-and-https-proxy/110515

I don't know how much it can help make AIO faster, but it could be interesting.

@docjyJ docjyJ marked this pull request as draft August 1, 2024 11:18
@szaimen
Copy link
Collaborator Author

szaimen commented Aug 2, 2024

By the way, what do you think of using Caddy without Apache: https://help.nextcloud.com/t/the-optimal-fastest-nextcloud-fpm-docker-setup-with-caddy-as-webserver-and-https-proxy/110515

I don't know how much it can help make AIO faster, but it could be interesting.

I have seen this and looks interesting but only Apache is officially supported: https://docs.nextcloud.com/server/latest/admin_manual/installation/nginx.html#nginx-configuration

Also only Apache can read the shipped htaccess files, e.g. this one, so we would need to keep the config always manually up-to-date which adds some maintenance burden...

However would of course be interesting to see if it potentially speeds up things...

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 2, 2024
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.

2 participants