fix(#8179) Add support for nginx and haproxy returning json error pages - #8446
Conversation
mrjones-plip
left a comment
mrjones-plip
left a comment
There was a problem hiding this comment.
Excellent work - thanks very much @nydr ! This took a bit longer for me because I don't know some of the techniques you use inside nginx and docker - but they were fun to learn!
For the most part this PR is good to go. I was able to spin up a vanilla 4.2.0 CHT instance and a one with the nginx running the public.ecr.aws/medic/cht-nginx:4.3.0-dn-8179-nginx-json image. Smoke testing the web app worked fine in latest Chrome on Ubuntu 20.
As well, i wrote a little one liner and tried docker kill ... on the same service in each Core deployments. JSON was correctly returned! \o/
➜ nginx git:(dn/8179-nginx-json) ✗ echo "\n------------8179 branch------------";curl -H "Accept: application/json" https://192-168-68-26.local-ip.medicmobile.org:10451/api/v2/monitoring ; echo "\n\n-------------4.2.0-----------------" ; curl -H "Accept: application/json" https://192-168-68-26.local-ip.medicmobile.org:10450/api/v2/monitoring
------------8179 branch------------
{"version":{"app":"4.2.2.5489334896","node":"v16.20.1","couchdb":"2.3.1"},"couchdb":{"medic":{"name":"medic","update_sequence":117---SNIP-TRUNCATE-JSON--connected_users":{"count":1}}
-------------4.2.0-----------------
{"version":{"app":"4.2.0.5075680573","node":"v16.20.0","couchdb":"2.3.1"},"couchdb":{"medic":{"name":"medic","update_sequence":136---SNIP-TRUNCATE-JSON--connected_users":{"count":1}}
➜ nginx git:(dn/8179-nginx-json) ✗ docker kill test_nginx_error_pages_8446_api_1 four_two__yay_api_1
test_nginx_error_pages_8446_api_1
four_two__yay_api_1
➜ nginx git:(dn/8179-nginx-json) ✗ echo "\n------------8179 branch------------";curl -H "Accept: application/json" https://192-168-68-26.local-ip.medicmobile.org:10451/api/v2/monitoring ; echo "\n\n-------------4.2.0-----------------" ; curl -H "Accept: application/json" https://192-168-68-26.local-ip.medicmobile.org:10450/api/v2/monitoring
------------8179 branch------------
{
"type": "ERROR",
"error": "502 Bad Gateway",
"server": "nginx"
}
-------------4.2.0-----------------
<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx/1.19.6</center>
</body>
</html>
I suspect this PR would be good as is, but I had one major change request as well as some minor ones and a one nitpick. Oh, and some questions too! If the questions end up blocking too much, feel free to resolve them and we can just chat on slack/ad hoc vid call!
Thanks again!
|
Oh yes! I forgot to compliment you on the test coverage here ! Great to see this as a hopefully a new standard of PRs - we really could use it. |
dianabarsan
left a comment
dianabarsan
left a comment
There was a problem hiding this comment.
This is fantastic work, @nydr . I'm really impressed.
@mrjones-plip has done a great job adding comments, that I don't have anything further to request.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
nydr
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
garethbowen
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks for checking @mrjones-plip! I've updated the PR so it should pass now |
mrjones-plip
left a comment
mrjones-plip
left a comment
There was a problem hiding this comment.
Really good work here @nydr ! Thanks very much for working through my feedback.
ship it 🚢
Replace `as` with upper case format `AS`.
|
SonarCloud Quality Gate failed.
|
3 participants
Description
Resolves #8179 - nginx and haproxy should respect
Acceptheaders when responding with error templatesCode review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.