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

Gzip Response compression filter running before the transformation filter #6206

Closed
asayah opened this issue Mar 31, 2022 · 5 comments
Closed
Assignees
Labels
Size: S 1 - 3 days Type: Bug Something isn't working

Comments

@asayah
Copy link
Contributor

asayah commented Mar 31, 2022

Gloo Edge Version

1.11.x (beta)

Kubernetes Version

No response

Describe the bug

The gzip compression filter runs before the transformation filter on the response path, resulting in compromised data.

Steps to reproduce the bug

create a gzip filter + a response transformation

Expected Behavior

the transformation filter runs before the gzip filter, so the compression happens on the transformed data.

Additional Context

ps: something to keep in mind is that on request compression, the filter gzip should be put after the transformation (opposite to the response path)
but since we don't support request transformation, maybe not something to worry about for now

@asayah asayah added the Type: Bug Something isn't working label Mar 31, 2022
@chrisgaun
Copy link

The problem is Gloo is allowing transformations to introduce spaces on a GZIP blob, making it incompatible w/ any standard GZIP decompressor that expects the GZIP standard header data structure.

These spaces were added by an inja (response) transformation code where there was a leading & trailing space for the {{body}} (example below). Also post transformation, It looks like the JSON response is not trimmed for spaces before getting compressed. Inja transformations are widely used in response transformations, trimming leading & trailing blank spaces in JSON responses is needed...

responseTransforms:

  • responseTransformation:
    transformationTemplate:
    body:
    text: '{% if (header(":status") == "401") %}…

      {% else %} {{body()}} {% endif %}'
    

To fix this GZIP should be the very last filter

@sam-heilbron
Copy link
Contributor

The work required here depends a little on who the compression is for (the client, or the upstream).

Currently, the compression (gzip) filter is ordered after the transformation filter. The effect is that filter order is as such:

Request: client -> transformation -> gzip -> upstream
Response: upstream -> gzip -> transformation -> client

If the compression that we're supporting is for the client, then the gzip filter should be earlier in the filter chain so that it decompresses requests first and compresses responses last. If the compression that we're supporting is for the upstream, then we want the gzip filter later in the filter chain, so that we compress outgoing requests last and decompress them right after receiving them.

Part 1
Currently, gzip compression only applies to responses, and I believe is intended to be used to decompress large responses from upstream. If that is true, then we just need to order the gzip filter at an earlier stage than transformations so that it is executed after them
Timeline: 1 hour

Part 2
If we also want to support more thorough compression both for clients and upstreams, there is some more work to expose more attributes of the compression filter
Timeline: 2-3 days

Overall: I think the work to resolve this can be completed by just handling Part 1, and writing up an feature request for Part 2 if that is desired.

cc @chrisgaun @nrjpoddar

@sam-heilbron sam-heilbron added the Size: S 1 - 3 days label Apr 4, 2022
@nrjpoddar
Copy link

I would lean towards doing it the correct way and support flexible options for our users. From my experience sending compressed responses to the end clients is the more realistic use case for responses i.e. we should run transformation -> gzip compression.

@asayah
Copy link
Contributor Author

asayah commented Apr 6, 2022

hi @sam-heilbron , can you please add more details regarding part2? how exposing more compression settings (part2) help in this use-case

@jenshu
Copy link
Contributor

jenshu commented Apr 13, 2022

Fix is merged and will be available in gloo-ee v1.11.3 as well as the next 1.12-beta release

We went with option 1 since that fixes the issue at hand. If there's a use case / desire for more customization (option 2) we can open a new issue at that time.

@jenshu jenshu closed this as completed Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: S 1 - 3 days Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants