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

AronNovak
Copy link
Contributor

@AronNovak AronNovak 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
Copy link
Contributor Author

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

@rfay
Copy link
Member

rfay 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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@rfay
Copy link
Member

rfay 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
Copy link
Member

rfay 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 nginx-dont-hide-upstream-errors branch from f9b5b6d to ecb9aa1 Compare May 1, 2019 22:34
@rfay
Copy link
Member

rfay 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
Copy link
Contributor Author

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

@AronNovak AronNovak force-pushed the nginx-dont-hide-upstream-errors branch from ddf6865 to d436c44 Compare May 2, 2019 03:37
@rfay
Copy link
Member

rfay 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 nginx-dont-hide-upstream-errors branch from f204597 to 7aca178 Compare May 2, 2019 17:56
@rfay
Copy link
Member

rfay 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).

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

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 ddev:master May 3, 2019
@AronNovak AronNovak deleted the nginx-dont-hide-upstream-errors branch May 4, 2019 04:31
@rfay rfay mentioned this pull request May 9, 2019
8 tasks
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.

None yet

2 participants