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

Re-add gzip compression of HTTP server responses #6699

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

adriansmares
Copy link
Contributor

@adriansmares adriansmares commented Nov 9, 2023

Summary

References #4921

This PR reintroduces the gzip compression of server responses, if the client signals that it supports it. We had this functionality by default between 2019 and 2021 as part of the stack, but it was removed as part of the Echo framework removal, and never reintroduced back, until now.

On the client side we now have support for zstd as well. Should we have any regressions related to this reintroduction, we can disable both client and server side support using an experimental feature flag.

If the changes are accepted, the experimental flags should be removed as part of the v3.29.0 release.

Changes

  • Add HTTP client support for both gzip and zstd compression of server responses.
  • Add HTTP server support for gzip compression of server responses.
    • This change does not apply to the interop server.

Testing

Local testing of the stack. One can manually check that the server side compression is working by looking in the browser developer tools network tab:
image

Notice that the amount transferred is lower than the content size, and that the Content-Encoding is set to gzip.

Regressions

Our first public v3 bug report (#82, followed up by #84) was related to gzip compression due to a mis-chaining of Flush calls - this bug does not exist in gzhttp, so we are safe.

A second consideration is BREACH - in short, we don't really reflect user input into API call contents, such that the compressed result size could leak something about page contents. Our CSRF tokens are also generated on a per request basis, so you cannot brute force the CSRF token by repeatedly loading the page.

gzhttp is capable of using HTB, which is a general purpose amelioration of the BREACH attack, but it seems that it breaks Content-Type detection so until that issue is cleared up we'll not enable the random content padding.

Notes for Reviewers

The experimental tracking issues will be created after the PR is merged.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@adriansmares adriansmares added this to the v3.28.1 milestone Nov 9, 2023
@adriansmares adriansmares self-assigned this Nov 9, 2023
@adriansmares adriansmares marked this pull request as ready for review November 10, 2023 09:16
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

Nice!

@adriansmares adriansmares merged commit 7a41da8 into v3.28 Nov 13, 2023
13 checks passed
@adriansmares adriansmares deleted the feature/gzip-experimental branch November 13, 2023 12:57
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.

2 participants