deps: replace gzip handler #11843
Merged
deps: replace gzip handler #11843
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
shoenig
force-pushed
the
deps-swap-gzip-handler
branch
from
39d4638
to
56c5e99
Compare
shoenig
force-pushed
the
deps-swap-gzip-handler
branch
from
56c5e99
to
6a91c89
Compare
| @@ -97,8 +97,8 @@ github.com/LK4D4/joincontext v0.0.0-20171026170139-1724345da6d5/go.mod h1:nxQPcN | |||
| github.com/Microsoft/hcsshim v0.8.7/go.mod h1:OHd7sQqRFrYd3RmSgbgji+ctCwkbq2wbEYNSzOYtcBQ= | |||
| github.com/Microsoft/hcsshim v0.8.9 h1:VrfodqvztU8YSOvygU+DN1BGaSGxmrNfqOv5oOuX2Bk= | |||
| github.com/Microsoft/hcsshim v0.8.9/go.mod h1:5692vkUqntj1idxauYlpoINNKeqCiG6Sg38RRsjT5y8= | |||
| github.com/NYTimes/gziphandler v1.0.0 h1:OswZCvpiFsNRCbeapdJxDuikAqVXTgV7XAht8S9olZo= | |||
| github.com/NYTimes/gziphandler v1.0.0/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= | |||
| github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ= | |||
There was a problem hiding this comment.
Shouldn't these be removed?
| @@ -299,6 +299,8 @@ github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL | |||
| github.com/fatih/color v1.13.0 h1:8LOYc1KYPPmyKMuN8QV2DNRWNbLo6LZ0iLs8+mlH53w= | |||
| github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk= | |||
| github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M= | |||
| github.com/felixge/httpsnoop v1.0.1 h1:lvB5Jl89CsZtGIWuTcDM1E/vkVs49/Ml7JJe07l8SPQ= | |||
There was a problem hiding this comment.
Am I correct that this is indirectly required by Gorilla or did we add these?
|
@DerekStrickland both of these lines are from the Dependencies that actually get included can be described with And we can also see when transitives are not part of the build, e.g. |
|
Oh my bad! Sorry for the noise. |
This has been pinned since the Go modules migration, because the nytimes gzip handler was modified in version v1.1.0 in a way that is no longer compatible. Pretty sure it is this commit: nytimes/gziphandler@c551b6c Instead use handler.CompressHandler from gorilla, which is a web toolkit we already make use of for other things.
After swapping gzip handler to use the gorilla library, we must account for a quirk in how zero/minimal length response bodies are delivered. The previous gzip handler was configured to compress all responses regardless of size - even if the data was zero length or below the network MTU. This behavior changed in [v1.1.0](nytimes/gziphandler@c551b6c#diff-de723e6602cc2f16f7a9d85fd89d69954edc12a49134dab8901b10ee06d1879d) which is why we could not upgrade. The Nomad HTTP Client mutates the http.Response.Body object, making a strong assumption that if the Content-Encoding header is set to "gzip", the response will be readable via gzip decoder. This is no longer true for the nytimes gzip handler, and is also not true for the gorilla gzip handler. It seems in practice this only makes a difference on the /v1/operator/license endpoint which returns an empty response in OSS Nomad. The fix here is to simply not wrap the response body reader if we encounter an io.EOF while creating the gzip reader - indicating there is no data to decode.
shoenig
force-pushed
the
deps-swap-gzip-handler
branch
from
6a91c89
to
e2ab168
Compare
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
(going in 1.3.0)
After swapping gzip handler to use the gorilla library, we
must account for a quirk in how zero/minimal length response
bodies are delivered.
The previous gzip handler was configured to compress all responses
regardless of size - even if the data was zero length or below the
network MTU. This behavior changed in v1.1.0
which is why we could not upgrade.
The Nomad HTTP Client mutates the http.Response.Body object, making
a strong assumption that if the Content-Encoding header is set to "gzip",
the response will be readable via gzip decoder. This is no longer true
for the nytimes gzip handler, and is also not true for the gorilla gzip
handler.
It seems in practice this only makes a difference on the /v1/operator/license
endpoint which returns an empty response in OSS Nomad.
The fix here is to simply not wrap the response body reader if we
encounter an io.EOF while creating the gzip reader - indicating there
is no data to decode.