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

Header modification by k6/go: ++ Transfer-Encoding: chunked #2483

Open
kkriegkxs opened this issue Apr 6, 2022 · 6 comments
Open

Header modification by k6/go: ++ Transfer-Encoding: chunked #2483

kkriegkxs opened this issue Apr 6, 2022 · 6 comments
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API

Comments

@kkriegkxs
Copy link

kkriegkxs commented Apr 6, 2022

Brief summary

K6 (or go http client) chunks some SignalR POST calls (with empty payload) over SSL only

  • signalr/connect?transport=longPolling&clientProtocol=2.1&....
  • signalr/abort?transport=longPolling&clientProtocol=2.1&....

k6 version

37

OS

win 10

Docker version and image (if applicable)

No response

Steps to reproduce the problem

Not sure...
Here is the sample request structure:

response = http.post(`https://${__ENV.TARGET_HOST}/...../signalr/connect?transport=longPolling&clientProtocol=2.1&connectionToken=......&connectionData=.....`,
	``,
	{
		headers:
		{
			"Connection": `keep-alive`,
			"Content-Length": `0`,
			"Pragma": `no-cache`,
			"Cache-Control": `no-cache`,
			"sec-ch-ua": `"Chromium";v="97", " Not;A Brand";v="99"`,
			"Accept": `text/plain, */*; q=0.01`,
			"Content-Type": `application/x-www-form-urlencoded; charset=UTF-8`,
			"X-Requested-With": `XMLHttpRequest`,
			"sec-ch-ua-mobile": `?0`,
			"User-Agent": `Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/97.0.4692.71 Safari/537.36`,
			"sec-ch-ua-platform": `"Windows"`,
			"Origin": `https://${__ENV.TARGET_HOST}`,
			"Sec-Fetch-Site": `same-origin`,
			"Sec-Fetch-Mode": `cors`,
			"Sec-Fetch-Dest": `empty`,
			"Referer": `https://${__ENV.TARGET_HOST}/........aspx`,
			"Accept-Encoding": `gzip, deflate, br`,
			"Accept-Language": `en-US,en;q=0.9`,
		},
		timeout: "130s",
	tags: {
		threadID: `${exec.vu.idInTest}`,
	 },
	});

Expected behaviour

  • request headers contain Content-Length: 0
  • request headers do not contain Transfer-Encoding: chunked
  • response status 200

Works as expected on a non-SSL endpoint and on SSL endpoint via Fiddler

Actual behaviour

  • request header is added: Transfer-Encoding: chunked
  • request header is removed Content-Length: 0
  • response code 501

Occurs for a couple of SignalR POST calls over SSL only

  • signalr/connect?transport=longPolling&clientProtocol=2.1&....
  • signalr/abort?transport=longPolling&clientProtocol=2.1&....

Works fine when called over HTTP, however when switching to SSL (over reverse proxy), K6 (GO http client?) changes the headers, even when:

  • payload is "" (nothing to chunk)
  • Content-Length: 0 is set
  • Transfer-Encoding: deflate (an explicitly set a value)

When passing requests through Fiddler, everything comes out as it should (Content-Length: 0 and no Transfer-Encoding).
Interestingly, the app server actually receives "Transfer-Encoding: chunked , chunked " when Transfer-Encoding: chunked is sent (reverse proxy mishandles improper chunking)

@kkriegkxs kkriegkxs added the bug label Apr 6, 2022
@mstoykov
Copy link
Contributor

mstoykov commented Apr 7, 2022

Thanks for reporting this @kkriegkxs,

This seems to be go stdlib thing. As "" is still something (it isn't nil) k6 makes a zero length body and sets Content-Length to 0 the latter of which stdlib take to mean that it should do chunk encode.

To fix it you can just put null instead of "" at which point no body will be given to the stdlib and it won't "chunked" encode.

But it might be a good idea if we do the same for an empty string as well 🤔.

This is reproducible with:

import http from "k6/http";
export default () => {
  http.post("https://httpbin.test.k6.io", "");
}

The headers don't make any difference

@na-- na-- added evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API labels Apr 7, 2022
@na--
Copy link
Member

na-- commented Apr 7, 2022

🤔 If you run this script with both null and "" for the request body:

import http from "k6/http";

export let options = {
    httpDebug: 'full'
};

export default () => {
    let resp = http.post("https://httpbin.org/anything", '');
    console.log(resp.body);
}

It seems that:

  • a Content-Length: 0 header will be added when the body is null
  • a Transfer-Encoding: chunked header will be added when the body is "" (an empty string), but, crucially, no content-length header will be added 😕 (which is contrary to the claim above that "k6 makes a zero length body and sets Content-Length to 0")

Both of these behaviors seem weird and unexpected to me... 😕 If they were exactly reversed, I'd have probably been fine in keeping them as they are and just documenting them. It makes some sense for k6 to emit Contet-Lenght: 0 when you pass it an explicitly empty string for a body, right? But that is not the case, so the current behavior does indeed seem like a bug... 😞 However, given the many issues and corner cases when it comes to handling request bodies in the current k6 HTTP API (#2311, #1382, #1571, etc.), and the fact that fixing this might be considered as a minor breaking change, I am not sure if we shouldn't wait until the new HTTP API (#2461) to properly fix this? 🤔

@mstoykov
Copy link
Contributor

mstoykov commented Apr 7, 2022

which is contrary to the claim above

we do set it

preq.Req.ContentLength = int64(preq.Body.Len()) // This will make Go set the content-length header
it just isn't sent, probably because of the chunked encoding.

@na--
Copy link
Member

na-- commented Apr 7, 2022

From https://pkg.go.dev/net/http#Request.Write

If Body is present, Content-Length is <= 0 and TransferEncoding hasn't been set to "identity", Write adds "Transfer-Encoding: chunked" to the header. Body is closed after it is sent.

Maybe we should explicitly specify Transfer-Encoding: identity if compression isn't enabled? 🤔 Though, given that Transfer-Encoding: chunked isn't a thing in HTTP/2, I wonder how big of a problem this is... 🤔

@kkriegkxs, how did you notice this issue? Were there any side-effects from Transfer-Encoding: chunked in your case, or did you just notice an extra header while inspecting the traffic?

@kkriegkxs
Copy link
Author

@na-- & @mstoykov: Transfer-Encoding: identity did exactly nothing, however nulling the body fixed it (regretfully it did not occur to me to try it before...). I saw the setting of the length header but as @mstoykov said, it is not sent due to chunked.
IMO philosophically speaking in this context empty=null with length 0 header and should be wrapped as such in POST(), so if anything the issue is that length header not controlling the chunking - so the easiest fix is to outline this behavior in the documentation.

post( url, [body], [params] )

PARAMETER TYPE DESCRIPTION
url string / HTTP URL Request URL (e.g. http://example.com).
body string / object / ArrayBuffer Request body; objects will be x-www-form-urlencoded.
params (optional) object Params object containing additional request parameters
Which I interpret as body shouldn't be null but rather empty (i.e. "" body & length 0 ==> null). But I guess empty resulting in chunking _kinda makes some sense_ as an explicit override but then it should not override any headers set explicitly... It is an easy fix for me because my code is 100% generated programmatically :) (Upon closer examination of about 100k lines of my k6 code I have found only about a dozen or so of occurrences - all SignalR)

@na--: no, there was a problem in SignalR (longpoll) resulting in code 501s. It took me a while (and #2482) to figure out what was happening. So I guess it is probably a more common issue than one might think (though I don't know how k6 recorder renders JS is such case).
Also between the load-balancer and the reverse proxy, Transfer-Encoding: chunked sent by the script somehow changed to Transfer-Encoding: chunked, chunked as received by IIS. This along with correct behavior through Fiddler makes me think k6 is probably in the wrong here.

@kkriegkxs
Copy link
Author

Oh, btw, per Mozilla:
Transfer-Encoding: chunked | compress | deflate | gzip but Accept-Encoding: gzip | compress | deflate | br | identity | *
Tried it anyway in case it's an undocumented spec :)
To sum up the long-winded previous reply: IMO body = null is an acceptable solution as long as it's documented but personally I would equate "" body to null if length = 0 (i.e. explicit manual mode should not auto-handle anything)

@na-- na-- added this to the v0.39.0 milestone Apr 18, 2022
@mstoykov mstoykov modified the milestones: v0.39.0, v0.40.0 Jun 15, 2022
@mstoykov mstoykov modified the milestones: v0.40.0, v0.41.0 Aug 30, 2022
@na-- na-- modified the milestones: v0.41.0, TBD Oct 28, 2022
@codebien codebien removed this from the TBD milestone Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API
Projects
None yet
Development

No branches or pull requests

4 participants