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 initial "NGINX Unit" based image #875

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

Conversation

tianon
Copy link
Member

@tianon tianon commented Jan 12, 2024

The Unit configuration I've included here is based on the example provided by the Unit documentation (https://unit.nginx.org/howto/wordpress/), although I did spend time understanding it before blindly trusting it. 😄

The biggest 😬 here (IMO) is that WordPress itself does not know about Unit yet, so the permalink handling is a bit off -- I've added a hack in wp-config-docker.php (specifically in the Unit images only) which works around it by teaching WordPress that Unit is effectively NGINX (which does work and is accurate enough for what WordPress needs), but that should probably be an issue or PR to the WordPress community instead.

(FYI @thresheek - maybe you have more context or know someone who does about WordPress recognizing Unit natively and not injecting /index.php into permalinks when it's the server software? 👀)

The Unit configuration I've included here is based on the example provided by the Unit documentation (https://unit.nginx.org/howto/wordpress/), although I did spend time understanding it before blindly trusting it. 😄

The biggest 😬 here (IMO) is that WordPress itself does not know about Unit yet, so the permalink handling is a bit off -- I've added a hack in `wp-config-docker.php` (specifically in the Unit images only) which works around it by teaching WordPress that Unit is effectively NGINX (which does work and is accurate enough for what WordPress needs), but that should probably be an issue or PR to the WordPress community instead.
@tianon
Copy link
Member Author

tianon commented Jan 12, 2024

Diff:
$ diff -u <(bashbrew cat wordpress) <(bashbrew cat <(./generate-stackbrew-library.sh))
--- /dev/fd/63	2024-01-12 09:16:41.475066580 -0800
+++ /dev/fd/62	2024-01-12 09:16:41.479066614 -0800
@@ -3,47 +3,52 @@
 
 Tags: 6.4.2-php8.1-apache, 6.4-php8.1-apache, 6-php8.1-apache, php8.1-apache, 6.4.2-php8.1, 6.4-php8.1, 6-php8.1, php8.1
 Architectures: amd64, arm32v5, arm32v7, arm64v8, i386, mips64le, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.1/apache
 
 Tags: 6.4.2-php8.1-fpm, 6.4-php8.1-fpm, 6-php8.1-fpm, php8.1-fpm
 Architectures: amd64, arm32v5, arm32v7, arm64v8, i386, mips64le, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.1/fpm
 
 Tags: 6.4.2-php8.1-fpm-alpine, 6.4-php8.1-fpm-alpine, 6-php8.1-fpm-alpine, php8.1-fpm-alpine
 Architectures: amd64, arm32v6, arm32v7, arm64v8, i386, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.1/fpm-alpine
 
 Tags: 6.4.2-apache, 6.4-apache, 6-apache, apache, 6.4.2, 6.4, 6, latest, 6.4.2-php8.2-apache, 6.4-php8.2-apache, 6-php8.2-apache, php8.2-apache, 6.4.2-php8.2, 6.4-php8.2, 6-php8.2, php8.2
 Architectures: amd64, arm32v5, arm32v7, arm64v8, i386, mips64le, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.2/apache
 
 Tags: 6.4.2-fpm, 6.4-fpm, 6-fpm, fpm, 6.4.2-php8.2-fpm, 6.4-php8.2-fpm, 6-php8.2-fpm, php8.2-fpm
 Architectures: amd64, arm32v5, arm32v7, arm64v8, i386, mips64le, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.2/fpm
 
 Tags: 6.4.2-fpm-alpine, 6.4-fpm-alpine, 6-fpm-alpine, fpm-alpine, 6.4.2-php8.2-fpm-alpine, 6.4-php8.2-fpm-alpine, 6-php8.2-fpm-alpine, php8.2-fpm-alpine
 Architectures: amd64, arm32v6, arm32v7, arm64v8, i386, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.2/fpm-alpine
 
+Tags: 6.4.2-unit, 6.4-unit, 6-unit, unit, 6.4.2-php8.2-unit, 6.4-php8.2-unit, 6-php8.2-unit, php8.2-unit
+Architectures: amd64, arm64v8
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
+Directory: latest/php8.2/unit
+
 Tags: 6.4.2-php8.3-apache, 6.4-php8.3-apache, 6-php8.3-apache, php8.3-apache, 6.4.2-php8.3, 6.4-php8.3, 6-php8.3, php8.3
 Architectures: amd64, arm32v5, arm32v7, arm64v8, i386, mips64le, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.3/apache
 
 Tags: 6.4.2-php8.3-fpm, 6.4-php8.3-fpm, 6-php8.3-fpm, php8.3-fpm
 Architectures: amd64, arm32v5, arm32v7, arm64v8, i386, mips64le, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.3/fpm
 
 Tags: 6.4.2-php8.3-fpm-alpine, 6.4-php8.3-fpm-alpine, 6-php8.3-fpm-alpine, php8.3-fpm-alpine
 Architectures: amd64, arm32v6, arm32v7, arm64v8, i386, ppc64le, s390x
-GitCommit: ac65dab91d64f611e4fa89b5e92903e163d24572
+GitCommit: 3610d02f059743d924bcda64dcf0ffe839bd904e
 Directory: latest/php8.3/fpm-alpine
 
 Tags: cli-2.9.0-php8.1, cli-2.9-php8.1, cli-2-php8.1, cli-php8.1

@tianon
Copy link
Member Author

tianon commented Jan 12, 2024

What I think we need to resolve in order to merge this:

  1. a new test to make sure it works correctly (and continues to do so)

  2. figure out what to do about the $is_nginx hack I added in wp-config -- that's not a real long-term solution and if we want to merge this, we should have a conversation with the WordPress community first (or find where one has already happened and implement any more official workarounds than my personal hack)

@tianon
Copy link
Member Author

tianon commented Jan 12, 2024

For 1., I guess that's really adapting the existing apache test to be slightly more generic and assigning it to both variants.

@thresheek
Copy link

That's pretty cool @tianon! Paging @javorszky for assistance - I believe they have been involved with Unit/Wordpress integration lately on our team.

@javorszky
Copy link

@tianon hello! I'll be on this on Monday morning GMT timezone :) I'm super excited about this!

@tianon
Copy link
Member Author

tianon commented Jan 12, 2024

Here's the simplified version of my current hack (for you to review Monday ❤️), since this PR probably isn't the easiest thing in the world to read. 😂

I'm essentially injecting this at the end of wp-config.php (because it includes wp-settings.php which includes vars.php which is what sets $is_nginx, so if I put it at the end, I can supplement that value with additional detection without patching WordPress itself):

// ...

/** Sets up WordPress vars and included files. */
require_once ABSPATH . 'wp-settings.php';

// WordPress does not support NGINX Unit (yet), so we have to do a little hackery to let it know that Unit supports nice permalinks (and prevent it from insisting on injecting "/index.php" in all the permalink options) -- the simplest way is teaching it to recognize Unit as if it were NGINX (which is only semi-true, but true enough for these purposes)
$is_nginx = $is_nginx || str_starts_with($_SERVER["SERVER_SOFTWARE"], "Unit/");
// see also https://github.com/WordPress/WordPress/blob/39f7f558d91afdd2f3afc7f3b049a6a800cd3f80/wp-includes/vars.php#L127

More useful backlinks (ie, how I ended up at the conclusion I needed to patch $is_nginx):

I guess a more "upstream-friendly" hack/workaround would be implementing a proper got_url_rewrite filter, but I'm not sure whether I can inject one of those just via wp-config.php or whether that'd have to be a proper plugin (which also isn't a bad idea for non-Docker users which we could recommend users of this image install instead of my hacks). Of course, I still think the ideal solution is probably fixes in WordPress itself to handle this case correctly, but I don't know whether that's already been attempted or whether there's any upstream appetite for that. 😅

{
apache: [ "apache2-foreground" ],
# https://github.com/nginx/unit/blob/fb33ec86a3b6ca6a844dfa6980bb9e083094abec/pkg/docker/Dockerfile.php8.2#L89
unit: [ "unitd", "--no-daemon", "--control", "unix:/var/run/control.unit.sock" ],
Copy link
Member Author

Choose a reason for hiding this comment

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

nextcloud/docker#1610 (comment) makes an excellent case for probably removing --control here 😅

@javorszky
Copy link

javorszky commented Jan 15, 2024

Neat, I had some time to look into this. From what I understand this repository is responsible for generating the individual Dockerfiles for WordPress at the different tags.

There are three immediate unit-specific observations:

  • the default unit config json needs to have a processes: 2 or
    "processes": {
    	"idle_timeout": 30,
    	"max": 50,
    	"spare": 1
    }
    See Wordpress is unable to perform loopback requests nginx/unit#1041 (comment) for reasoning for that one. The important point is that the number of processes should be more than 1
  • WordPress multisite is currently not supported due to a bug in unit itself. That one is documented here: wordpress subdirectory multisite setup can't get it to work nginx/unit#916. There's a POC fix for it, but the unit team is still figuring out how best to handle the request uri having the correct value persisted.
    • As an aside, when WordPress runs in multisite mode, the unit config that we have on our documentation page is not enough. There's a working (with the POC solution) unit config in the comments for issue 916
  • WordPress, from my experience at least running it on the main branch and the POC branch for the issue-916 fix, is absolutely totally fine with pretty permalinks

For upstream work that we need the WordPress folks, I think we need to add two things:

  1. a way for WP to detect that it's running Unit, much like it recognised Nginx and Apache, and
  2. a way for WordPress to reconfigure unit for multisite with the included multisite config file, once 1041 is solved and a unit release contains that one
    • this one won't matter if unit was started without the --control flag

@javorszky
Copy link

Hey @tianon,

Small update on where things are on our side:

  • the PR to fix the issue in Unit is still open, we need to do more testing and fix a few other existing tests (HTTP: Ensure REQUEST_URI immutability. nginx/unit#1162). Once it's fixed and merged and the new version is released with this in it, this PR can be updated to pull in the correct Unit version
  • we're writing a documentation on how to configure unit so the multisite setup works for both subdirectory and subdomain installs, and that needs to be published in the WordPress Handbook so users have a reference about it
  • There's an open trac ticket with a relevant code changeset so WordPress can recognise it's running behind a Unit server and link to the documentation page (https://core.trac.wordpress.org/ticket/60582)

This PR is an active item in our planning and we keep an eye on it 🙂

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

Successfully merging this pull request may close these issues.

3 participants