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

Add a nginx-unit -based image #1610

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ananace
Copy link

@ananace ananace commented Oct 14, 2021

This sets up a new variant for nginx-unit, based on their nginx-unit PHP image and the Nextcloud example in their documentation.

Probably related to #1063 - though ths solution is different than the one suggested there as nginx-unit already includes a FastCGI-based PHP solution.

Images for testing have been built at ghcr.io/ananace/docker-nextcloud - only x86_64 and aarch64 as that's what's available in the upstream unit image.

@ananace
Copy link
Author

ananace commented Dec 12, 2023

Pushed another version of this to do some more testing myself, also pushed a version as ananace/nextcloud-unit:27 - only amd64/aarch64 as that's what's available for the upstream unit image.

@ananace
Copy link
Author

ananace commented Dec 30, 2023

Continued work on things, it should now mostly match cache headers for nginx/apache as well. I wasn't able to get the v argument handling working - will be digging more into that later.

It's supposed to support setting cache control using an inline template like this, but any attempts to include inline templates like they describe cause unit to fail to load the configuration.

"response_headers": {
  "Cache-Control": "public, max-age=15778463, `${ arg_v ? 'immutable' : '' }`"
}

@J0WI J0WI requested review from tianon and J0WI January 9, 2024 22:14
@J0WI
Copy link
Contributor

J0WI commented Jan 9, 2024

generate-stackbrew-library.sh needs an update too to include it in the library file. Once this is done, a test similar to nextcloud-apache-run can be added.

@tianon
Copy link
Contributor

tianon commented Jan 10, 2024

The main thing that makes me nervous here is the reimplementation of the unit entrypoint script inside the nextcloud entrypoint script -- what I'd suggest is copying /usr/local/bin/docker-entrypoint.sh inside the Dockerfile before overwriting it to something like /usr/local/bin/unit-entrypoint.sh which can be invoked directly instead so it can remain the source-of-truth for the fiddly bits of initializing unit instead.

I started fiddling with how I might implement this in the WordPress image, and I came up with something like this:

RUN set -eux; \
	sed -e 's/docker-entrypoint/unit-entrypoint/g' /usr/local/bin/docker-entrypoint.sh > /usr/local/bin/unit-entrypoint.sh; \
	grep -F '/unit-entrypoint.d/' /usr/local/bin/unit-entrypoint.sh; \
	chmod +x /usr/local/bin/unit-entrypoint.sh
COPY wordpress-unit.json /unit-entrypoint.d/wordpress.json

This allows me to invoke unit-entrypoint.sh inside the new entrypoint to make sure Unit is initialized before we exec it.

I also don't know whether the --control bit from https://github.com/nginx/unit/blob/fb33ec86a3b6ca6a844dfa6980bb9e083094abec/pkg/docker/Dockerfile.php8.2#L89 is strictly necessary or whether that's the default value, but that's also something to consider.

@ananace
Copy link
Author

ananace commented Jan 11, 2024

@tianon When I did the first version of this, it wasn't at all possible to use the original unit entrypoint script due to how it was set up, but I should be able to retool to let it do the configuration through there now.

The control argument is only necessary if you want to keep exposing the control socket, to be able to dynamically reconfigure unit while it's running, something which nextcloud lacks any functionality for.

@tianon
Copy link
Contributor

tianon commented Jan 12, 2024

Ah, I see -- in case you're curious, I finished up my WordPress PoC and pushed it to docker-library/wordpress#875 😅

@ananace
Copy link
Author

ananace commented Jan 21, 2024

Retooled the nginx-unit image to use the upstream entrypoint, not sure about any generate-stackbrew-library.sh change though. I tried running it locally on my test setup and the generated output looked correct.
Though I've only been pushing the changes to the configuration and scripts, not the generated files, to keep the PR manageable - and it's likely that those are required for it to generate the correct library file.

@ananace ananace force-pushed the nginx-unit branch 2 times, most recently from 4a1bef0 to c196265 Compare May 10, 2024 11:56
@ananace
Copy link
Author

ananace commented May 10, 2024

Rebased the branch, added in an IP forward config block, and built new image tags for 27, 28, and 29 for testing.

@J0WI
Copy link
Contributor

J0WI commented May 13, 2024

Thanks for keeping this up to date. I just haven't merged it yet because I'm also following the progress in docker-library/wordpress#875.

Signed-off-by: Alexander Olofsson <[email protected]>
@ananace
Copy link
Author

ananace commented Nov 23, 2024

Pushed new 29 and 30 images to ghcr.io/ananace/docker-nextcloud - since the dockerhub rate limit is really annoying me at this point.

@joshtrichards joshtrichards linked an issue Nov 23, 2024 that may be closed by this pull request
@k4my4b
Copy link

k4my4b commented Dec 1, 2024

Continued work on things, it should now mostly match cache headers for nginx/apache as well. I wasn't able to get the v argument handling working - will be digging more into that later.

It's supposed to support setting cache control using an inline template like this, but any attempts to include inline templates like they describe cause unit to fail to load the configuration.

"response_headers": {
  "Cache-Control": "public, max-age=15778463, `${ arg_v ? 'immutable' : '' }`"
}

what you're after is this:

"response_headers": {
    "Cache-Control": "`public, max-age=15778463${vars.arg_v ? ', immutable' : ''}`"
}

Edit:
I forgot to mention that the Alpine package of Unit is compiled without njs support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add NGINX unit image / or use add to nextcloud-fpm
5 participants