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

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

Open
SamJUK opened this issue Nov 21, 2024 · 6 comments
Open

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

SamJUK opened this issue Nov 21, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@SamJUK
Copy link
Contributor

SamJUK commented Nov 21, 2024

Version of Warden

in-dev

Operating System and Installation Method

MacOS 15 - Git Main Branch

Describe the Bug

@monteshot has reported, When attempting to disable SPX after usage, you receive a Nginx 502 bad gateway.
See the following comment: #820 (comment)

image

To Reproduce

  1. Start a warden project with WARDEN_PHP_SPX=1 within the .env file
  2. Profile a page or two with SPX
  3. Disable SPX by updating the .env file to WARDEN_PHP_SPX=0
  4. Restart the environment warden env down && warden env up

Expected Behavior

Disabling PHP SPX via the .env, should not cause a 502 bad gateway

Additional context

Leading thought is that it will be cause by having the SPX_ENABLED or SPX_KEY cookies present.

Removing these cookies / using a different browser or incognito window should confirm this theory.

Additionally @ihor-sviziev has suggest manually disabling the profiler before stopping the environment: #820 (comment)

@SamJUK SamJUK added the bug Something isn't working label Nov 21, 2024
@SamJUK SamJUK mentioned this issue Nov 21, 2024
@SamJUK
Copy link
Contributor Author

SamJUK commented Nov 21, 2024

The PHP Debug container also works from a XDEBUG cookie IIRC, presumably this issue is not present there. Is there any configuration difference?

Is it worth updating the nginx config upstreams to include the core php-fpm container as a fallback when using a specialised image (php-spx, php-debug etc)?

@navarr
Copy link
Member

navarr commented Nov 21, 2024

I have a few different thoughts here. The one that would be best seems most difficult to do, potentially impossible which is that Nginx should only have the redirect functionality based on whether or not those features are enabled. It may potentially be doable with the env variables. But in general, I consider this expected behavior.

I think the problem here is that SPX isn't turned on/off in a browser extension like xdebug is, which makes it more confusing to the end user.

I'm wondering if we could display a custom nginx error page in this scenario that explains what's happening and allows the user to turn spx off.

@monteshot
Copy link
Contributor

Custom Nginx pages may reduce confusion(at least for me).

@navarr navarr moved this to 📋 Backlog in Warden Nov 21, 2024
@bap14
Copy link
Member

bap14 commented Nov 21, 2024

@navarr I think an possibility would be to to define server groups for xdebug and php-spx. Then in the group the primary server would be the debug server and the backup would be the normal php-fpm server.

upstream php-xdebug {
    server ${NGINX_UPSTREAM_BLACKFIRE_HOST}:${NGINX_UPSTREAM_BLACKFIRE_PORT};
    server ${NGINX_UPSTREAM_HOST}:${NGINX_UPSTREAM_PORT} backup;
}

upstream php-spx {
    server ${NGINX_UPSTREAM_SPX_HOST}:${NGINX_UPSTREAM_SPX_PORT};
    server ${NGINX_UPSTREAM_HOST}:${NGINX_UPSTREAM_PORT} backup;
}

Then in the map definition for $fastcgi_backend we just replace the debug hostnames with the upstream server groups: php-xdebug or php-spx. This would then let requests fall through to the non-debug services if the debug service isn't available.

@SamJUK
Copy link
Contributor Author

SamJUK commented Nov 21, 2024

@bap14 Attempted that earlier and Nginx fails to boot with a Host not found error for the SPX container. As far as I can tell, there is no way to prevent nginx from resolving the server entries on start up. Could be user error though.

@navarr feature flag ENV variables could potentially work, if we set them via the php spx / xdebug compose files. Although, I'm not sure if we can force a nginx container restart in the case of someone running warden env up -d --remove-orphans

Running with the error page idea, we could unset the cookies via headers & a 302 redirect. Although it definently lacks polish and feels a bit of a hacky workaround.

diff --git a/nginx/etc/nginx/conf.d/default.conf b/nginx/etc/nginx/conf.d/default.conf
index a40a502..901d94c 100644
--- a/nginx/etc/nginx/conf.d/default.conf
+++ b/nginx/etc/nginx/conf.d/default.conf
@@ -13,6 +13,11 @@ map "$http_X_BLACKFIRE_QUERY:$cookie_XDEBUG_SESSION$cookie_XDEBUG_PROFILE$cookie
     default ${NGINX_UPSTREAM_DEBUG_HOST}:${NGINX_UPSTREAM_DEBUG_PORT};
 }
 
+map "$cookie_SPX_ENABLED$cookie_SPX_KEY:$http_x_spx_reset" $bad_gateway_response {
+    "~.+:" @reset_spx;
+    default @standard_bad_gateway;
+}
+
 server {
     listen 80;
 
@@ -23,6 +28,21 @@ server {
     autoindex off;
     charset UTF-8;
 
+    error_page 502 = $bad_gateway_response;
+    location @reset_spx {
+      internal;
+      add_header X-SPX-RESET 1;
+      add_header Set-Cookie "SPX_ENABLED=;Path=/;expires=Thu, 01 Jan 1970 00:00:00 GMT";
+      add_header Set-Cookie "SPX_KEY=;Path=/;expires=Thu, 01 Jan 1970 00:00:00 GMT";
+      return 302 $scheme://$host$request_uri;
+    }
+
+    location @standard_bad_gateway {
+      internal;
+      add_header Content-Type text/html always;
+      return 502 '<html><head><title>502 Bad Gateway</title></head><body><center><h1>502 Bad Gateway</h1><hr><p>nginx</p></center>';
+    }
+
     include /etc/nginx/available.d/${NGINX_TEMPLATE};
     include /etc/nginx/default.d/*.conf;
 }

@ihor-sviziev
Copy link
Contributor

@SamJUK i like the idea in general, but instead of having remove cookies and redirect isn't a good idea since it will happen not clearly. Maybe we can do some small nice page with a JS-based cookie removal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 📋 Backlog
Development

No branches or pull requests

5 participants