-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
Remove nginx bwdserver #3635
Merged
Merged
Remove nginx bwdserver #3635
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's gone in http2 anyway, and most connections to the load balancer are probably http2
(Unclear if they should be)
…anyway But now it's consistent
The predeployment checks CI job failed - that's fine, there's a different config and we'll just have bwdserver down for a few minutes. Since it's prerelease it doesn't matter. |
It's defined a few lines later
StachuDotNet
approved these changes
Mar 26, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part 1 of #3630
This removes nginx from the BwdServer. We needed nginx for a couple of reasons in the past that are no longer necessary or can be handled in other (better) ways:
we needed some form of way to protect the app from expensive requests without processing them or recompiling and redeploying the whole thing. nginx gave us a quick way to update the config and restart the containers - instead we can use cloud armor, which is better at it
we needed to support requests with missing content-lengths, as users like to make POST requests from curl sometimes and getting the right content-length is annoying. Now kestrel supports omitting the content-length header
we needed to proxy requests to other servers for darklang.com, but now we've split the ApiServer from the BwdServer so it's not as important
we needed to do gzip on large returned files. Admittedly nginx is better at that than kestrel but it can still be done in kestrel
ultimately, i want to remove it as it's "yet another thing to understand" and I'd rather remove a complex step from our infra
This removes nginx from bwdserver only, including removing the deployment/k8s config files.
One major problem was that the test suite had a lot of files with the wrong content-length, which nginx papered over. So a lot of this PR is making sure the content-length is right in all the test files, and adding support to the test suite to make sure of that.