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

ddev-webserver Nginx - do not intercept 400 errors #1555

Merged
merged 8 commits into from May 3, 2019

Conversation

Projects
None yet
2 participants
@AronNovak
Copy link
Contributor

commented Apr 30, 2019

supersedes #1534

The Problem/Issue/Bug:

The issue is that currently Nginx swallows all the upstream error messages. This is particularly painful for instance for webservices where the error message would be crucial to get in addition to the status code.

How this PR Solves The Problem:

It changes the default configuration of Nginx to pass all the upstream errors.

Manual Testing Instructions:

Put a tiny PHP script inside the container:

header($_SERVER['SERVER_PROTOCOL'] . ' 500 Internal Server Error', true, 500);
echo "moo";

moo should be visible when accessing the script.

Automated Testing Overview:

Right now the PR does not contain tests, if it's approved that it's a wanted change, it should have a test that contains a dummy script that provides error and it tries to access that URL via wget˛and see if the body of the request contains the output from PHP.

Release/Deployment notes:

DDEV users need to be warned that the default behavior changes, it alters the behaviors of the applications in case of HTTP error responses.

Related: #1367 (comment) neos needed this change.

@AronNovak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@rfay Here's the reworked PR, ready for a last review.

@rfay

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Wow thanks. I didn't expect you to do that. Thanks. BTW, you could have just rebased the other one, would have saved some effort. But thanks! I pushed the updated ddev-router.

@AronNovak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@rfay Yep, rebasing works, but I do not prefer force pushes and with so many conflicts, it's the same order of magnitude of work in my opinion.

@AronNovak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Hm, it seems it needs a little work due to the CI failure.

@rfay

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

You had used the same tag, so that image wouldn't have passed tests, etc. It's pretty confused now. I tried to restart all of them and it's really confused.

@rfay

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Looks like you used intercept_errors instead of fastcgi_intercept_errors :)

@rfay rfay added this to the v1.8.0 milestone May 1, 2019

@rfay rfay force-pushed the AronNovak:nginx-dont-hide-upstream-errors branch from f9b5b6d to ecb9aa1 May 1, 2019

@rfay

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I took the liberty of fixing the problems and rebasing this, hope you'll take a look and make sure you're OK with it @AronNovak

@AronNovak

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@rfay Thanks, the change looks good of course, now I try to understand the CI failure.

@AronNovak AronNovak force-pushed the AronNovak:nginx-dont-hide-upstream-errors branch from ddf6865 to d436c44 May 2, 2019

@rfay

This comment has been minimized.

Copy link
Member

commented May 2, 2019

I think the TestConfigOverrideDetection failure was a (now) obsolete nginx-site.conf in that test. Pushed a commit to update it.

@rfay rfay force-pushed the AronNovak:nginx-dont-hide-upstream-errors branch from f204597 to 7aca178 May 2, 2019

@rfay

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Rebased again; it seems the pushed ddev-webserver image didn't match the tests that were being run against it (php overrides).

@rfay

rfay approved these changes May 3, 2019

Copy link
Member

left a comment

I think this is good to go. Thanks for championing this, I'm sure it's been in people's way from time to time.

@rfay rfay merged commit 81c4b2e into drud:master May 3, 2019

10 checks passed

buildkite/ddev-containers-windows Build #1705 passed (28 minutes, 27 seconds)
Details
buildkite/ddev-macos Build #2088 passed (42 minutes, 51 seconds)
Details
buildkite/ddev-macos-use-nfsmount Build #313 passed (1 hour, 23 minutes, 45 seconds)
Details
buildkite/ddev-windows-apache-fpm Build #1273 passed (1 hour, 8 minutes, 2 seconds)
Details
buildkite/ddev-windows-dockerforwindows Build #2243 passed (1 hour, 3 minutes, 21 seconds)
Details
buildkite/ddev-windows-dockerforwindows-nfs Build #392 passed (1 hour, 5 minutes, 41 seconds)
Details
buildkite/ddev-windows-dockertoolbox Build #1851 passed (1 hour, 13 minutes, 21 seconds)
Details
buildkite/ddev-windows-dockertoolbox-nfs Build #388 passed (1 hour, 38 minutes, 15 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
normal_build_and_test Workflow: normal_build_and_test
Details

@AronNovak AronNovak deleted the AronNovak:nginx-dont-hide-upstream-errors branch May 4, 2019

@rfay rfay referenced this pull request May 9, 2019

Closed

v1.8.0 Release Checklist Due 2019-05-14 #1533

8 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.