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

fix(brotli-plugin): Skip brotli compression for upstream compressed response #10740

Merged
merged 12 commits into from
Jan 12, 2024

Conversation

SilentEntity
Copy link
Contributor

@SilentEntity SilentEntity commented Jan 2, 2024


Skipping the brotli compression if the upstream response is already compressed.

The Problem

The problem occurs when the upstream service sends compressed data/response either in gzip or deflate. Apisix tries to compress data in brotli which is already compressed and, in turn, returns encoded data that can not be used by the browser or application.

Possible Solutions

There are two options to solve the problem first to skip the further compression and return with the upstream compressed response. The second is to decompress the upstream response and then compress using brotli, which is not viable on apisix gateway, it increases the overhead of the gateway and results in increasing latency and computation requirements.

This PR solves the Issue by skipping the brotli compression if the upstream response is compressed.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@SilentEntity SilentEntity changed the title Skip brotli compression fix(brotli-plugin): Skip brotli compression for upstream compressed response Jan 2, 2024
@SilentEntity SilentEntity marked this pull request as ready for review January 3, 2024 05:29
@monkeyDluffy6017
Copy link
Contributor

Could you add some test cases?

@monkeyDluffy6017
Copy link
Contributor

I think it's reasonable, multiple compressions may cause unnecessary trouble

@monkeyDluffy6017
Copy link
Contributor

please make the ci pass

@monkeyDluffy6017 monkeyDluffy6017 added wait for update wait for the author's response in this issue/PR and removed need test cases labels Jan 5, 2024
@SilentEntity
Copy link
Contributor Author

okay, it's ready

@SilentEntity
Copy link
Contributor Author

The typo is fixed. It will pass in CI.

@monkeyDluffy6017
Copy link
Contributor

The ci still failed

@SilentEntity
Copy link
Contributor Author

Hi @monkeyDluffy6017, please check now

@monkeyDluffy6017 monkeyDluffy6017 removed wait for update wait for the author's response in this issue/PR user responded labels Jan 11, 2024
@monkeyDluffy6017 monkeyDluffy6017 merged commit 781e8f6 into apache:master Jan 12, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Skip brotli compression if upstream response is compressed
3 participants