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

Disable br compression when no Accept-Encoding header is present #10178

Merged
merged 4 commits into from Jan 16, 2024

Conversation

robin-moser
Copy link
Contributor

@robin-moser robin-moser commented Oct 19, 2023

What does this PR do?

Fixes #9734

To ensure compatibility, it is recommended not to compress a request when there is no Accept-Encoding header present, even though RFC9110 (accept-encoding) would allow any compression.

Motivation

Stops traefik from breaking multiple applications that don't send a Accept-Encoding header and at the same time don't have any support for encoded http responses. Two breaking examples are yum/dnf and Zenarmor on Opnsense.

More

  • Added/updated tests
  • Added/updated documentation

@rjsocha
Copy link
Contributor

rjsocha commented Oct 30, 2023

The wording in RFC 2616, specifically in https://datatracker.ietf.org/doc/html/rfc2616#section-14.3 is similar to this

" If no Accept-Encoding field is present in a request, the server MAY
assume that the client will accept any content coding. In this case,
if "identity" is one of the available content-codings, then the
server SHOULD use the "identity" content-coding, unless it has
additional information that a different content-coding is meaningful
to the client.
"

In any case, I believe this option should be configurable. For example:

  label:
  ...
  - traefik.http.middlewares.test4compress.compress.disableCompressionOnMissingAcceptEncoding=true
  - traefik.http.middlewares.test4compress.compress.disableOnMissingAcceptEncoding=true
  - traefik.http.middlewares.test4compress.compress.noAcceptEncodingCompression=true

It should, by default, be disabled, following the semantics of RFC 9110.

@rtribotte rtribotte changed the title don't compress requests if not Accept-Encoding header is present Disable br compression when no Accept-Encoding header is present Nov 10, 2023
@robin-moser robin-moser force-pushed the missing-encoding-header-action branch 4 times, most recently from 1918191 to e3dfb4a Compare November 13, 2023 12:51
@rtribotte
Copy link
Member

The wording in RFC 2616, specifically in https://datatracker.ietf.org/doc/html/rfc2616#section-14.3 is similar to this

" If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. In this case, if "identity" is one of the available content-codings, then the server SHOULD use the "identity" content-coding, unless it has additional information that a different content-coding is meaningful to the client. "

In any case, I believe this option should be configurable. For example:

  label:
  ...
  - traefik.http.middlewares.test4compress.compress.disableCompressionOnMissingAcceptEncoding=true
  - traefik.http.middlewares.test4compress.compress.disableOnMissingAcceptEncoding=true
  - traefik.http.middlewares.test4compress.compress.noAcceptEncodingCompression=true

It should, by default, be disabled, following the semantics of RFC 9110.

Hello @rjsocha,

Thanks for this feedback. It seems to me that the need to introduce an option to control this behavior should be discussed in a dedicated issue first.

@rtribotte
Copy link
Member

Hello @robin-moser,

I have enabled the checks, and as you can see, the TestShouldCompressWhenNoAcceptEncodingHeader test should be adapted as well.

@robin-moser
Copy link
Contributor Author

Sorry, I missed that one!
I updated the unit tests, locally they are passing now.

Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@nmengin nmengin added priority/P1 need to be fixed in next release priority/P2 need to be fixed in the future labels Jan 8, 2024
pkg/middlewares/compress/compress.go Outdated Show resolved Hide resolved
@nmengin nmengin removed the priority/P2 need to be fixed in the future label Jan 11, 2024
@nmengin
Copy link
Contributor

nmengin commented Jan 16, 2024

Hey @robin-moser ,

We'd like to bring your feature in Traefik v3.0.
Do you need some help on it?

@robin-moser
Copy link
Contributor Author

Hi @nmengin,
sorry for the late reaction. I just committed the suggested documentation changes.

robin-moser and others added 4 commits January 16, 2024 14:18
To ensure compatibility, it is recommended not to compress a request when there is no Accept-Encoding header. See traefik#9734
Co-authored-by: Kevin Pollet <pollet.kevin@gmail.com>
Co-authored-by: Landry Benguigui <lbenguigui@gmail.com>
Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

Thanks 👍

Copy link
Contributor

@lbenguigui lbenguigui left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 7a315bb into traefik:v3.0 Jan 16, 2024
22 checks passed
@rtribotte rtribotte removed their assignment Jan 16, 2024
@robin-moser robin-moser deleted the missing-encoding-header-action branch January 18, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/middleware priority/P1 need to be fixed in next release size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants