add gzip compression to GET and LIST requests #45666

ilackarms · 2017-05-11T15:42:08Z

Enable compressed response bodies for non-watch GET and LIST requests on API Objects.

What this PR does / why we need it: Adds compression via Accept-Encoding header, returns Content-Encoding header on responses (only supports gzip at this time). Enabled solely for GET and LIST requests which can return very large response bodies.

Special notes for your reviewer:

See #44164 for discussion.

Release note:

k8s-ci-robot · 2017-05-11T15:42:11Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2017-05-11T15:42:17Z

Hi @ilackarms. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gmarek · 2017-05-11T17:19:17Z

cc @kubernetes/sig-api-machinery-pr-reviews

deads2k · 2017-05-11T17:20:33Z

@k8s-bot ok to test

deads2k · 2017-05-11T17:23:02Z

staging/src/k8s.io/apiserver/pkg/server/filters/compression.go

+}
+
+const (
+	header_AcceptEncoding  = "Accept-Encoding"


kube convention doesn't use underscores in constant names.

deads2k · 2017-05-11T17:28:08Z

staging/src/k8s.io/apiserver/pkg/server/filters/compression.go

+func WithCompression(handler http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		//don't compress watches
+		if req.URL.Query().Get("watch") == "true" {


plumb through a RequestContextMapper from https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go#L188 and then use https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go#L229 and https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go#L35

With that info, you can whitelist instead of blacklist. Or you can blacklist more effectively.

thanks for the review, I'll look into this now

deads2k · 2017-05-11T17:30:28Z

staging/src/k8s.io/apiserver/pkg/server/filters/compression.go

+	case encoding_deflate:
+		compressor = zlib.NewWriter(w)
+	default:
+		panic(encoding + " not a valid compression type")


We prefer returning and error and letting the caller handle it. In this case, why wouldn't we degrade to a non-compressed response instead of dying?

This shouldn't be here at all; wantsCompression simply returns false if the user specifies a compression type that doesn't exactly match gzip or deflate. I'll fix this in my next commit

deads2k · 2017-05-11T17:31:51Z

staging/src/k8s.io/apiserver/pkg/server/filters/compression.go

+}
+
+func RestfulWithCompression(function restful.RouteFunction) restful.RouteFunction {
+	return restful.RouteFunction(func(request *restful.Request, response *restful.Response) {


I expected this to delegate to WithCompression

Does the following seem right to you? I feel like I'm messing with restful internals here, but this was how I implemented the delegation:

func RestfulWithCompression(function restful.RouteFunction) restful.RouteFunction { return restful.RouteFunction(func(request *restful.Request, response *restful.Response) { handler := WithCompression(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { response.ResponseWriter = w request.Request = req function(request, response) })) handler.ServeHTTP(response.ResponseWriter, request.Request) }) }

I didn't want to go with this as I was more comfortable being explicit with the code (despite their being code duplication)

Does the following seem right to you? I

Looks like it would work. Not pretty, but workable

deads2k · 2017-05-17T21:18:01Z

@k8s-bot gce etcd3 e2e test this

deads2k · 2017-05-17T21:18:14Z

squash, fix gofmt (verify) and I'll take another pass

deads2k · 2017-05-18T14:52:49Z

@k8s-bot gce etcd3 e2e test this

deads2k · 2017-05-18T14:54:43Z

staging/src/k8s.io/apiserver/pkg/server/filters/compression.go

+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		wantsCompression, encoding := wantsCompressedResponse(req)
+		if wantsCompression {
+			compressionWriter := NewCompressionResponseWriter(w, encoding)


@sttts I remember that this caused us problems in the general case, but here it's only getting added for get and list. Any concerns about wrapping here?

deads2k · 2017-06-05T13:19:05Z

@k8s-bot pull-kubernetes-unit test this

ilackarms · 2017-06-05T13:32:52Z

gating issue #46963

liggitt · 2017-06-05T14:50:21Z

go ahead and add a release note in the PR description

k8s-ci-robot · 2017-06-05T15:21:40Z

@ilackarms: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
Jenkins GCE etcd3 e2e	20b37cb1e092c2ddeb9a872f6f8678562e91dc06	link	`@k8s-bot gce etcd3 e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

deads2k · 2017-06-06T12:36:46Z

I updated links and issues, so this gets another shot in the queue. please update.

closes kubernetes#44164

smarterclayton · 2017-06-06T13:40:16Z

Submit queue, pay attention.

k8s-github-robot · 2017-06-06T14:43:02Z

Automatic merge from submit-queue

Automatic merge from submit-queue (batch tested with PRs 47883, 47179, 46966, 47982, 47945) Add feature gating to REST Compression **What this PR does / why we need it**: Adds feature gating to opt out of REST API compression **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #46963 **Special notes for your reviewer**: This PR is a fix / addendum to #45666 **Release note**: ```release-note ```

sppatel · 2017-08-04T17:38:07Z

Does anyone know what version of kubernetes this made it into? I'm using minikube that contains 1.7..0 and setting the Accept-encoding to gzip but I do not see the expected Content-Encoding response header after requesting a resource list.

ilackarms · 2017-08-04T18:16:17Z

@sppatel see #46966; this PR was reverted in favor of that one. I'm not sure which version it was merged into, but in that PR, compression is disabled by default. In order to enable it, you'll need to use the feature flag APIResponseCompression

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 11, 2017

k8s-github-robot assigned erictune and gmarek May 11, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 11, 2017

gmarek assigned smarterclayton May 11, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 11, 2017

gmarek assigned deads2k and unassigned gmarek May 11, 2017

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 11, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 11, 2017

deads2k reviewed May 11, 2017

View reviewed changes

ilackarms force-pushed the compression branch 2 times, most recently from bdf7eda to 0acde54 Compare May 13, 2017 14:17

ilackarms force-pushed the compression branch 3 times, most recently from e296af3 to 20b37cb Compare May 17, 2017 23:44

deads2k reviewed May 18, 2017

View reviewed changes

ilackarms force-pushed the compression branch from 5ecae4d to 428c18a Compare June 3, 2017 15:18

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 3, 2017

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017

ilackarms mentioned this pull request Jun 5, 2017

Add gating to API Compression handler #46963

Closed

ilackarms mentioned this pull request Jun 5, 2017

Add feature gating to REST Compression #46966

Merged

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2017

deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2017

add gzip compression to GET and LIST requests

fc650a5

closes kubernetes#44164

ilackarms force-pushed the compression branch from 428c18a to fc650a5 Compare June 6, 2017 12:43

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2017

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2017

k8s-github-robot merged commit cc568f6 into kubernetes:master Jun 6, 2017

ilackarms deleted the compression branch June 6, 2017 15:01

mikedanese mentioned this pull request Jun 8, 2017

cache mutation detector causes memory/cpu pressure at the end of long e2e runs (like pull-kubernetes-e2e-gce-etcd3) #47135

Closed

liggitt mentioned this pull request Jun 8, 2017

Revert "add gzip compression to GET and LIST requests" #47191

Merged

liggitt mentioned this pull request Jun 26, 2017

Do not return partial list of API/Groups on startup #48026

Closed

ilackarms mentioned this pull request Aug 27, 2017

Disable Requesting Compression by default on restclient #48477

Closed

cben mentioned this pull request Dec 27, 2017

Allow compression of json response body using Accept-Encoding header #110349

Closed

sftim mentioned this pull request Nov 25, 2021

Document API response compression kubernetes/website#30639

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add gzip compression to GET and LIST requests #45666

add gzip compression to GET and LIST requests #45666

ilackarms commented May 11, 2017 •

edited by deads2k

Loading

k8s-ci-robot commented May 11, 2017

k8s-ci-robot commented May 11, 2017

gmarek commented May 11, 2017

deads2k commented May 11, 2017

deads2k May 11, 2017

deads2k May 11, 2017

deads2k May 11, 2017 •

edited

Loading

ilackarms May 11, 2017

deads2k May 11, 2017

ilackarms May 11, 2017

deads2k May 11, 2017

ilackarms May 11, 2017

deads2k May 11, 2017

deads2k commented May 17, 2017

deads2k commented May 17, 2017

deads2k commented May 18, 2017

deads2k May 18, 2017

sttts May 19, 2017

deads2k commented Jun 5, 2017

ilackarms commented Jun 5, 2017

liggitt commented Jun 5, 2017

k8s-ci-robot commented Jun 5, 2017 •

edited

Loading

deads2k commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

sppatel commented Aug 4, 2017

ilackarms commented Aug 4, 2017

add gzip compression to GET and LIST requests #45666

add gzip compression to GET and LIST requests #45666

Conversation

ilackarms commented May 11, 2017 • edited by deads2k Loading

k8s-ci-robot commented May 11, 2017

k8s-ci-robot commented May 11, 2017

gmarek commented May 11, 2017

deads2k commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 17, 2017

deads2k commented May 17, 2017

deads2k commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 5, 2017

ilackarms commented Jun 5, 2017

liggitt commented Jun 5, 2017

k8s-ci-robot commented Jun 5, 2017 • edited Loading

deads2k commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

sppatel commented Aug 4, 2017

ilackarms commented Aug 4, 2017

ilackarms commented May 11, 2017 •

edited by deads2k

Loading

deads2k May 11, 2017 •

edited

Loading

k8s-ci-robot commented Jun 5, 2017 •

edited

Loading