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

There should be a different entrypoint.sh script for each setup (apache and fpm) #2091

Open
callmemagnus opened this issue Oct 29, 2023 · 8 comments
Labels
bug feature: auto config (environment variables) Auto configuring via environment variables

Comments

@callmemagnus
Copy link

I've been using the nextcloud docker images for a while now without any issue until recently. Not that the issue was introduced by a change here but because of my usage of the image.

I want to extend the image in order to add functionnality like the live update script for the fulltextsearch application.

To do so, I used the nextcloud:apache as base and added scripts and supervisor and ended the Dockerfile with a new e.g.

CMD ["supervisord", "-c", "supervisor.conf"]

It was "working" but broke some parts of Nextcloud because the entrypoint.sh script ran partially. The reason is because that script has strong expectations on the CMD of the image.

E.g. this expects the CMD to contain apache:

image

As I understand, this was probably made to simplify the management of the 3 different images that are handled by this repo but it makes extending the images more difficult.

Writing this ticket, I came up with the idea that I could probably wrap my supervisor command in a script named apache-whatever but that would not a "great" experience.

What do you think ?

@joshtrichards
Copy link
Member

joshtrichards commented Oct 30, 2023

Are you setting NEXTCLOUD_UPDATE=1 in your Dockerfile as documented for these situations?

https://github.com/nextcloud/docker#adding-features

(Though you may have a point about APACHE_DISABLE_REWRITE_IP specifically. Other scenarios should already be handled with NEXTCLOUD_UPDATE=1 though).

@joshtrichards joshtrichards added bug feature: auto config (environment variables) Auto configuring via environment variables labels Oct 30, 2023
@callmemagnus
Copy link
Author

Reading yet another time the entrypoint script, you are correct that I might have stumbled upon the only issue in that script :-)

Temporarily I implemented my idea but it feels wrong.

@joshtrichards
Copy link
Member

Feel up for creating a PR adding a NEXTCLOUD_UPDATE check to the APACHE_DISABLE_REWRITE_IP handler section?

I think it can be modeled somewhat after (slightly different logic to exclude fpm though I guess):

if expr "$1" : "apache" 1>/dev/null || [ "$1" = "php-fpm" ] || [ "${NEXTCLOUD_UPDATE:-0}" -eq 1 ]; then

@callmemagnus
Copy link
Author

Funnily enough, my wrong idea does not work as expr "/apache-whatever" : "apache" is not "true".

I'll look into the PR for this.

@callmemagnus
Copy link
Author

callmemagnus commented Nov 21, 2023

I tried something: not sure if it is PR ready yet (callmemagnus@b9adcad).

I'm not clear about how to make sure nothing is broken by this. Can you help me on that?

My solution is to provide a new variable NEXTCLOUD_DOCKER_MODE (either apache or php-fpm) and remove NEXTCLOUD_UPDATE when extending.

I wonder if that information could be "guessed" as we are extending either one or the other. If there is a way to know, I could rely on that to discriminate one from the other and we could fallback to the existing NEXTCLOUD_UPDATE=1.

@callmemagnus
Copy link
Author

@joshtrichards, did you get some time to look at my proposition ?

@EVOTk
Copy link

EVOTk commented Dec 19, 2023

Hello,

In order to customize the image, wouldn't it be simpler to have a command in entrypoint.sh that checks for the presence of a script file? Example custom.sh and, if present, execute it?

That way, you don't break what's currently in place. ( I'm thinking of existing installations )

Edit : Oh ! Sorry, that already seems to be the case!

https://github.com/nextcloud/docker/blob/37ee8cfdab231703b1adb5fa20abaeceec1b95cc/27/fpm/entrypoint.sh#L24C59-L24C59

@callmemagnus
Copy link
Author

Hello,

I'm extending the image to change the CMD used to start.
The current entry point script makes assumptions based on that command; changing it, removes the ability to disable the remote IP hack and I need that hack.

So, yes, I could run the command to disable it by myself but that would bring maintenance annoyance to make sure it has not changed.

The best solution would be my previous command, reason I'm asking for assistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature: auto config (environment variables) Auto configuring via environment variables
Projects
None yet
Development

No branches or pull requests

3 participants