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

support deflate encoding reader #76551

Merged
merged 1 commit into from May 31, 2019

Conversation

@JieJhih
Copy link
Member

commented Apr 13, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
support deflate encoding reader
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

proxy/transport: Support Content-Encoding: deflate
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Hi @JieJhih. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@roycaihw

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

/assign @lavalamp

@@ -232,6 +233,15 @@ func (t *Transport) rewriteResponse(req *http.Request, resp *http.Response) (*ht
defer gzw.Close()
writer = gzw
// TODO: support flate, other encodings.
case "deflate":

This comment has been minimized.

Copy link
@lavalamp

lavalamp Apr 15, 2019

Member

This whole response rewriting thing was a huge mistake, I'm not super excited to add bells and whistles. @deads2k thoughts? I guess this is at least a small, targeted change...

Please update the TODO statement and add a test.

This comment has been minimized.

Copy link
@JieJhih

JieJhih Apr 19, 2019

Author Member

Updated
Thanks

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

ping @lavalamp PTAL.

@dims

This comment has been minimized.

Copy link
Member

commented May 15, 2019

/ok-to-test
/milestone v1.15
/assign @liggitt

@dims

This comment has been minimized.

Copy link
Member

commented May 15, 2019

/kind feature
/priority important-longterm

errFn(v, err)
}

if getResponse.Body == nil {

This comment has been minimized.

Copy link
@liggitt

liggitt May 15, 2019

Member

this isn't actually testing the encoding worked correctly

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 15, 2019

Author Member

@liggitt Updated. PTAL
Thanks!!

}
encodeType := []string{"gzip", "deflate"}
resp := &http.Response{
Body: ioutil.NopCloser(bytes.NewBufferString("Unit test")),

This comment has been minimized.

Copy link
@liggitt

liggitt May 15, 2019

Member

the body has to actually be gzip or deflate-encoded for the rewriters to work properly

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

ping @liggitt PTAL.

@xmudrii

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Hello! We are starting the code freeze for 1.15 tomorrow EOD. Is this PR still planned for the 1.15 cycle?

gzw.Flush()
flw.Write([]byte(expectedDeflate))
flw.Flush()
resp := []http.Response{

This comment has been minimized.

Copy link
@lavalamp

lavalamp May 29, 2019

Member

It is better (more understandable, less error prone) to make one table with a struct type than to keep parallel arrays.

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 29, 2019

Author Member

@lavalamp Thanks!! Updated. PTAL


switch v {
case "deflate":
s, _ := ioutil.ReadAll(flate.NewReader(gotResponse.Body))

This comment has been minimized.

Copy link
@lavalamp

lavalamp May 29, 2019

Member

Package a reader factory function in the table, too. We shouldn't have switches like this inside the loop.

Body: ioutil.NopCloser(gzipbuf),
},
{
Body: ioutil.NopCloser(flatebuf),

This comment has been minimized.

Copy link
@lavalamp

lavalamp May 29, 2019

Member

Instead of premaking the test payload, I recommend putting a writer factory function in the table. Then the for loop can test whatever body it wants.

I think you should test a long body as well as this short one.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels May 29, 2019

}{
{
encodeType: "gzip",
excepted: []string{

This comment has been minimized.

Copy link
@lavalamp

lavalamp May 29, 2019

Member
  1. I think you mean "expected" :)
  2. This can be defined outside of the table and doesn't need to be repeated / different for each encoding type.

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 29, 2019

Author Member

Oops, I didn't notice that. Updated, thanks for the reminder.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels May 29, 2019

PathPrepend: "/proxy/node/node1:10250",
}
expected := []string{
"short body test: gzip test",

This comment has been minimized.

Copy link
@lavalamp

lavalamp May 29, 2019

Member

last set of comments, sorry :)

  1. remove gzip reference :)
  2. suggest making a realllllly big one with e.g. strings.Repeat("a",4097)
  3. please squash, thanks.

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 29, 2019

Author Member

@lavalamp Thanks, squashed and pushed.

This comment has been minimized.

Copy link
@lavalamp

lavalamp May 29, 2019

Member

thanks, just waiting for tests now.

This comment has been minimized.

Copy link
@JieJhih

JieJhih May 30, 2019

Author Member

@lavalamp The tests all passed now.

support deflate encoding reader
add unit test for rewrite response

response body using encode

refactor test case to struct

move expected to global

add long body test

@JieJhih JieJhih force-pushed the JieJhih:proxy/transport branch from 3f67d2e to 9337ed7 May 29, 2019

@liggitt liggitt removed their assignment May 30, 2019

@lavalamp

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label May 30, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JieJhih, lavalamp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@JieJhih

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 7929c15 into kubernetes:master May 31, 2019

21 checks passed

cla/linuxfoundation JieJhih authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.