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: drop deprecated filter #15867

Merged
merged 18 commits into from Jun 9, 2021
Merged

Conversation

rojkov
Copy link
Member

@rojkov rojkov commented Apr 7, 2021

Commit Message: gzip: drop deprecated filter
Additional Description:
The HTTP Gzip filter has been disabled in v1.17 by default. Now we can remove it completely. Also the code common for the gzip filter and for the generic compressor filter is moved to source/extensions/filters/http/compressor.

Risk Level: Low
Testing: unit tests
Docs Changes: removed the page about the Gzip filter
Release Notes: added a note about the removal to current.rst

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15867 was opened by rojkov.

see: more, trace.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
@rojkov
Copy link
Member Author

rojkov commented Apr 7, 2021

I guess this PR should wait until API v2 is completely dropped to make the API check pass.

@htuch
Copy link
Member

htuch commented Apr 8, 2021

@rojkov you can't delete protos from the APIs, even after the v2 turndown. But you can remove proxy-side support.

@rojkov
Copy link
Member Author

rojkov commented Apr 8, 2021

@htuch I put all the protos back (v2 and v3), but now protodoc is failing when I run ./docs/build.sh with "Did you forget to add 'envoy.filters.http.gzip' to source/extensions/extensions_build_config.bzl?"

Should I put an empty BUILD as a stub for the removed extension?

@htuch
Copy link
Member

htuch commented Apr 9, 2021

@rojkov this is probably the first time we've removed an extension that still has API coverage. @phlax could you help @rojkov with this one? I feel this belongs in the docs ownership wheelhouse ;-)

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
…te-v4

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
@phlax
Copy link
Member

phlax commented Apr 20, 2021

hi @rojkov

I guess this PR should wait until API v2 is completely dropped to make the API check pass.

i have just started working on that here #16077 - although im not sure how quickly it will land

@rojkov
Copy link
Member Author

rojkov commented Apr 20, 2021

@phlax Thanks! No hurry, it's a tech debt PR.

rojkov added 2 commits May 6, 2021 11:34
…te-v4

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
@phlax
Copy link
Member

phlax commented May 6, 2021

now protodoc is failing ... Should I put an empty BUILD as a stub for the removed extension?

im not sure exactly how best to keep the proto files around but not include them (mho is that they should be removed as historical versions of the repo still contain them)

how they would be kept before they should be included is by adding a not-implemented-hide annotation - might work - not sure what the post-deprecation policy is for these yet

…te-v4

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
@rojkov rojkov requested a review from junr03 as a code owner June 1, 2021 08:14
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
@phlax
Copy link
Member

phlax commented Jun 1, 2021

@rojkov im not totally understanding - is gzip support removed completely?

there is a pr here #16440 to demonstrate gzip usage which i had approved - im wondering if this pr makes it obsolete

@phlax
Copy link
Member

phlax commented Jun 1, 2021

ah wait i see - there is still a gzip extension - its just been removed from http.filters - ill check the pr again for compat

@rojkov
Copy link
Member Author

rojkov commented Jun 1, 2021

@phlax gzip support (through the generic compressor filter) is still there. This PR removes the old gzip filter, deprecated long time ago.

@phlax
Copy link
Member

phlax commented Jun 1, 2021

got it - thanks - seems like the pr should be gtm - ill merge it now

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
@junr03 junr03 assigned phlax and htuch Jun 2, 2021
@rojkov
Copy link
Member Author

rojkov commented Jun 4, 2021

@markdroth Could you please have a look? This PR seems to be ready for review.

…te-v4

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@repokitteh-read-only repokitteh-read-only bot removed the api label Jun 9, 2021
@htuch
Copy link
Member

htuch commented Jun 9, 2021

/lgtm v2-freeze

@htuch htuch merged commit dc2cdde into envoyproxy:main Jun 9, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
The HTTP Gzip filter has been disabled in v1.17 by default. Now we can remove it completely. Also the code common for the gzip filter and for the generic compressor filter is moved to source/extensions/filters/http/compressor.

Risk Level: Low
Testing: unit tests
Docs Changes: removed the page about the Gzip filter
Release Notes: added a note about the removal to current.rst

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
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.

None yet

4 participants