refactor(php-fpm): rework php-fpm backend selection to reduce complexity #63

SamJUK · 2025-10-12T22:07:48Z

Note: Have not throughly tested it yet, mainly looking for feedback on the approach before investing the additional time.

Related PR within the docker compose repository - wardenenv/warden#889

Rough concept PR for reworking the PHP fastcgi backend selection to hopefully resolve a few issues.
Specially around the need to delete cookies when disabling debug containers to avoid 502 errors.

Add a debug header x-backend to improve the visibility of the backend selection
Rework the fastcgi_backend map to account for enabled status of each service/container to avoid 502 errors when disabling containers like SPX or XDEBUG
Rework the fastcgi_backend map to use named labels & multiple lines to improve maintainability

Should resolve the following issues:
wardenenv/warden#820 (comment)
wardenenv/warden#822

ilnytskyi · 2025-10-13T06:30:10Z

Looks good.

Not sure if we need all headers for debug every time, as they may be not readable for developers who don't dig into warden internals. But it's actually good for debugging.

Maybe we should consider hiding them under some request header like

curl ... -H 'X-WARDEN-UPSTREM-RULES: 1'

SamJUK · 2025-10-13T09:49:52Z

Agreed, they are primarily there just to aid with testing the change before release. Plan is to remove them before the merge, unless general consensus is to keep them behind a feature flag.

I feel the x-backend header could be worth keeping to improve visibility, although might be better suited to a entry in the access log rather than the header. (Although don't think we declare a custom access log format currently)

bap14 · 2025-10-13T12:45:20Z

nginx/Dockerfile

These changes are unnecessary. We don't need to use the environment variables for this.

Dropped the changes to the Dockerfile

bap14 · 2025-10-13T12:47:21Z

nginx/etc/nginx/conf.d/default.conf

+BF:$WARDEN_BLACKFIRE:$http_X_BLACKFIRE_QUERY;
+DBG:$PHP_XDEBUG_3:$cookie_XDEBUG_SESSION$cookie_XDEBUG_PROFILE$cookie_XDEBUG_TRACE$arg_XDEBUG_SESSION$arg_XDEBUG_SESSION_START;
+SPX:$WARDEN_PHP_SPX:$cookie_SPX_ENABLED$cookie_SPX_KEY$arg_SPX_ENABLED$arg_SPX_KEY$arg_SPX_UI_URI;


We should ignore the environment files, but keep the prefix so it's easier to match. I like the idea of the prefix, but I think it would be enough to have "~BF:.+;" or "~SPX:.+;"

Dropped the enabled checks, but kept the prefixes for the backends

bap14 · 2025-10-13T12:48:20Z

nginx/etc/nginx/conf.d/default.conf

+    "~DBG:1:.+;" ${NGINX_UPSTREAM_DEBUG_HOST}:${NGINX_UPSTREAM_DEBUG_PORT};
+    "~BF:1:.+;" ${NGINX_UPSTREAM_BLACKFIRE_HOST}:${NGINX_UPSTREAM_BLACKFIRE_PORT};
+    "~SPX:1:.+;" ${NGINX_UPSTREAM_SPX_HOST}:${NGINX_UPSTREAM_SPX_PORT};


Remove the :1 from the pattern, as we shouldn't need to use the environment variables.

bap14 · 2025-10-13T12:48:39Z

nginx/etc/nginx/conf.d/default.conf

+    # TESTING PURPOSES - Show evaluated backend conditions
+    add_header x-backend-blackfire "BF:$WARDEN_BLACKFIRE:$http_X_BLACKFIRE_QUERY;" always;
+    add_header x-backend-xdebug "DBG:$PHP_XDEBUG_3:$cookie_XDEBUG_SESSION$cookie_XDEBUG_PROFILE$cookie_XDEBUG_TRACE$arg_XDEBUG_SESSION$arg_XDEBUG_SESSION_START;" always;
+    add_header x-backend-spx "SPX:$WARDEN_PHP_SPX:$cookie_SPX_ENABLED$cookie_SPX_KEY$arg_SPX_ENABLED$arg_SPX_KEY$arg_SPX_UI_URI;" always;


Go ahead and remove these. If they're for debugging, they shouldn't be in the PR.

bap14 · 2025-10-13T12:53:49Z

I like where you're going with the PR, but I don't think the 502 errors are wrong to appear. warden env up should be bringing up a normal and debug version of php-fpm. If you specifically disable the debug version and then try to use it, that's user error not the system being wrong. I also fear this would lead to seeing a request be handled normally and leading to confusion why debugging isn't stopping at the breakpoint. While the header will be helpful, I think a more distinct 50x error is more appropriate.

SamJUK · 2025-10-13T20:25:01Z

Main direction of the PR was to reduce the complexity around enabling/disabling the SPX container and have it just work.
I know there was already some confusion with the current 502 page wardenenv/warden#820 (comment)

From my perspective, I treat the feature flag as a source of truth. After turning off the flag, I don't expect to have to manually cleanup my environment. And vice versa, when a feature I am expecting is missing, its one of the first things i check.

I think if we do decided to stick with the 502 route, we'll need a custom 502 page like @ihor-sviziev suggested, to help visualise why the 502 happened with a easy way to remove the left over cookies. wardenenv/warden#822 (comment)

I think its really just a case of deciding what will be the best UX.
I've slapped together a quick POC of the custom 502 approach.

https://github.com/wardenenv/images/compare/main...SamJUK:feat/cookie-helper?expand=1

bap14 · 2025-10-14T15:08:18Z

I'd rather support a dynamic 502 page. To me the issue you're trying to solve with the environment value is a user-error. You have to tag the request to be debug/spx/blackfire using cookies, why would disabling it be done somewhere else instead of just deleting the cookies? Feels like we're solving the wrong problem with this approach.

navarr · 2025-10-23T17:29:52Z

I'd also rather the dynamic 502 page. It's not exactly user error, but it's also not the kind of thing we want to silently change on developers (e.g. turning on SPX, but SPX isn't on, but you get a page, and you can't figure out why SPX isn't working). That kind of silence makes it really difficult to determine what's happening.

A dynamic 502 for the case where its been turned off but the cookie still exists is infinitely better.

And honestly I don't see any reason why your POC couldn't be considered complete.

SamJUK · 2025-10-25T18:42:38Z

Thanks for the feedback both.

I've had the 502 page build running locally for the last 2 weeks, and to be fair, its worked well from a UX perspective.
I'll improve the styling of it to give it a more finished look, and open a dedicated PR for it.

I'll also rebase this PR to drop the enabled checks, and reduce the scope of this PR to just include the QOL changes to the fastcgi_backend map for container selection.

SamJUK · 2025-10-25T18:58:50Z

PR rebased, and title updated to reflect new scope of the PR

This was referenced Oct 12, 2025

[WIP] fix(php-fpm): rework php-fpm backend selection wardenenv/warden#889

Closed

[Command] Disabling PHP SPX causes a 502 bad gateway wardenenv/warden#822

Closed

bap14 suggested changes Oct 13, 2025

View reviewed changes

refactor(php-fpm): refactor php-fpm backend map to reduce complexity

26ee1a0

SamJUK force-pushed the fix/rework-fpm-backend-selection branch from 8cc585a to 26ee1a0 Compare October 25, 2025 18:55

SamJUK changed the title ~~[WIP] fix(php-fpm): rework php-fpm backend selection~~ refactor(php-fpm): rework php-fpm backend selection to reduce complexity Oct 25, 2025

navarr approved these changes Oct 25, 2025

View reviewed changes

navarr merged commit 86fd672 into wardenenv:main Oct 25, 2025

Uh oh!

refactor(php-fpm): rework php-fpm backend selection to reduce complexity #63

refactor(php-fpm): rework php-fpm backend selection to reduce complexity #63

Uh oh!

Conversation

SamJUK commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilnytskyi commented Oct 13, 2025

Uh oh!

SamJUK commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bap14 commented Oct 13, 2025

Uh oh!

SamJUK commented Oct 13, 2025

Uh oh!

bap14 commented Oct 14, 2025

Uh oh!

navarr commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamJUK commented Oct 25, 2025

Uh oh!

SamJUK commented Oct 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SamJUK commented Oct 12, 2025 •

edited

Loading

navarr commented Oct 23, 2025 •

edited

Loading