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

Conditional compression based on request Content-Type #5721

Merged
merged 2 commits into from Oct 31, 2019

Conversation

ldez
Copy link
Member

@ldez ldez commented Oct 25, 2019

What does this PR do?

Allow to exclude some Content-Type of the compression.

Motivation

Fixes #2576
Closes #5120

More

  • Added/updated tests
  • Added/updated documentation

Copy link
Member

@jbdoumenjou jbdoumenjou left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

It does not work on my test:

I got the following error message multiple times

mime: no media type" middlewareName=jenkins-compress@docker middlewareType=Compress

For reproduction: I used the following example:

version: '3'

services:
  proxy:
    # Built with "make build-image"
    # from https://github.com/containous/traefik/pull/5721/commits/d6a2d32b5c306141312811dac74d5a9da5a53c61
    image: 76025322bc49 
    command:
      - --providers.docker
      - --entrypoints.web.address=:80
      - --api.insecure=true
    ports:
      - "80:80"
      - "8080:8080"
    labels:
      - traefik.enable=false
    volumes:
      - /var/run/docker.sock:/var/run/docker.sock:ro

  cloudbees-jenkins:
    image: cloudbees/jenkins-enterprise:latest
    restart: unless-stopped
    labels:
      - traefik.http.routers.jenkins.rule=Host(`jenkins.localhost`)
      - traefik.http.services.jenkins.loadbalancer.server.port=8080
      - traefik.http.middlewares.jenkins-compress.compress=true
      - traefik.http.middlewares.jenkins-compress.compress.excludedcontenttypes=application/javascript
      - traefik.http.routers.jenkins.middlewares=jenkins-compress

I expect any requests to application/json file to NOT be compressed ,
but it is still gzipped:

Screenshot 2019-10-28 at 15 53 44

Di I do something wrong?

@ldez
Copy link
Member Author

ldez commented Oct 28, 2019

application/javascript != application/json

@dduportal
Copy link
Contributor

After an exchange:

  • The "application/javascript != application/json" is an error on my previous GH message, but the configuration was always application/javascript.
  • Error on my side is the expectation: Expecting the Content-Type Header of responses from backend to be the switch, while it the incoming request. Fix: Even though it is correctly written in the proposed documentation, we added a 2nd mention to the "incoming request" to be sure (a warning would have been heavy).
  • The message about empty content-type is expected as my requests did not have any, but we decided to move it as "debug" level to avoid flooding end-users.

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ldez ldez force-pushed the feature/compress-content-types branch from b957668 to a09be6c Compare October 29, 2019 11:15
Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

@traefiker traefiker force-pushed the feature/compress-content-types branch from a09be6c to d29a4e1 Compare October 30, 2019 17:34
@traefiker traefiker merged commit 3410541 into traefik:master Oct 31, 2019
v2 automation moved this from To review to Done Oct 31, 2019
@ldez ldez deleted the feature/compress-content-types branch October 31, 2019 13:11
@ldez ldez changed the title Conditionnal compression based on Content-Type Conditional compression based on Content-Type Nov 5, 2019
@ldez ldez changed the title Conditional compression based on Content-Type Conditional compression based on request Content-Type Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v2
Done
Development

Successfully merging this pull request may close these issues.

Problem with text/event-stream when compress=true
5 participants