Add cookie support to WebSockets

[imiric]

This is a continuation of #1650, with some minor cleanup.

Closes #1226

[imiric]

I tried adding a test for replace, but it seems that cookies in the global cookie jar (http.cookieJar()) are not propagated to ws.connect(). I.e. this script doesn't send the cookie:

import http from 'k6/http';
import ws from 'k6/ws';

export default function () {
  const jar = http.cookieJar();
  jar.set("ws://127.0.0.1:8080/", "Session", "123");
  var res = ws.connect("ws://127.0.0.1:8080/", null, function(socket){
    socket.on("message", function (data){
      console.log(data);
      socket.close();
    });
  });
}

So my question is: should it? Based on this comment probably not as it would be a breaking change. But should we then have an equivalent ws.cookieJar()?

[mstoykov]

I think it should and I also think that is the better first change.

Unfortunately the web socket library we use takes only jars and as adding both a cookie and using the jar should probably not add the cookie to the jar you will need to:

1. create a new jar to copy the values from the provide one (or the default one) and add the cookie to that
2. set any cookies set by the websocket one back in the original jar

Given that ... I am more for not having a way to specify a cookie but to

1. have a way to specify a jar
2. use the default one by default ;)

And then whoever wants to specify a Cookie can just add it to the jar and pass the jar or continue setting the Header ... both of which arguably aren't that much harder especially as I would expect the bigger problem will be that you would've made an HTTP request it would set you the cookie in the jar, but then that won't be used by the websocket. And the someone less likely variant of websocket HTTP call setting a cookie that isn';t set on the default jar

[imiric]

@mstoykov I'm a bit confused by the changes you want 😅 Can you summarize them as commit messages? :)

FWIW gorilla/websocket does support setting a Cookie header and not just via a jar. See Dialer.DialContext. There's an issue about the header overriding the jar, but IMO this seems like desired behavior.

So the way I see it to handle this "properly" and achieve feature-parity with how it works for http we should:

• Add a per-VU WS-only cookie jar (ws.cookieJar()) from which we will copy to the Dialer.Jar for each new connection.

• Keep this params approach to override cookies per connection, maybe without using Dialer.Jar but simply setting it as a header.

• Allow passing a standalone jar via params (new ws.CookieJar()) that will be set to Dialer.Jar after merging any cookies set via params and in the global jar. We should document how overriding works to avoid surprises.

I would've preferred if all this could work with http.cookieJar(), since the only reason we need a WS approach is to avoid breaking existing scripts, but hopefully we can unify this when we refactor WS and after the new HTTP API.

WDYT? cc: @na--

EDIT: Needless to say, I doubt this will be tested and ready for v0.33.0 so we might want to postpone it.

Also this expands on the scope of #1226 which merely wanted it passed via params. So we could merge a variation of this change and figure out jar handling later.(?)

[mstoykov]

FWIW gorilla/websocket does support setting a Cookie header and not just via a jar. See Dialer.DialContext. There's an issue about the header overriding the jar, but IMO this seems like desired behavior.

As mentioned in this comment from the "fix" this seems like the correct behaviour ;). And I agree with that.

So the way I see it to handle this "properly" and achieve feature-parity with how it works for http we should:

This is IMO the wrong thing to try to do given #1923 which literally spells out how not ... good and confusing the cookie works for HTTP. Also it was provoked by me trying to work on this exact PR/issue.

IMO this should :

1. Use the default cookiejar from lib.state by default.
2. be able to take Params.jar which is http.Cookiejar .. .we can probably move it and keep it's old name as well ... although a bit finicky.
3. Params.headers.Cookie to overwrite any of the above (so no change I think)
4. Params.Cookies should not be supported, at least for now. (look at how the merging works for HTTP and you will see it as PITA, and likely is ... completely not useful as I will try to explain below.)

Reasons:

1. I would expect that if I do an HTTP request and I then make a websocket one they will use the same jars. I would argue this will remove the reason for most of the people requesting this, after all the reports are about authenticating, usually with a session which likely got received through and HTTP request.
2. Having a way to specify another jar lets them set cookies in a more uniform way even if they don't use HTTP request to get the cookie
3. The header Cookie and how the order in it works is not very well standardized, so having us merge stuff into it "somehow" just makes things confusing IMO and requires reading way too much documentation from the user to learn how it works (also we will need to write it). IMO if we have a way to get the cookies for a given URL out of the cookie jar (which we do), have a way to modify those values and then seriliaze them to a Cookie value so people can do stuff like

var jar  = ... // cookiejar
var cookies = jar.cookiesForURL(myurl);
cookies[mycoolCookie]="specialValue";
var headers = {Cookie: serializeToCookieValue(cookies)};

will remove the whole need for having Cookie in the params. It might be 2 lines more or something like that, but given how much cleaner it will be and less confusing IMO that should've been what was done instead of the current merging of cookies between params.jar and params.cookies that is happening ...

I would've preferred if all this could work with http.cookieJar(), since the only reason we need a WS approach is to avoid breaking existing scripts, but hopefully we can unify this when we refactor WS and after the new HTTP API.

I see no reason why not do it now ... or just not do these changes at all for now ;) .

This has been the case forever and even now users can always just set the Cookie header. The main benefit of providing a jar IMO is that it makes things less confusing when you need to get a cookie from HTTP request and give it to the websocket one (which is still HTTP first) and , secondly, for when a websocket connection set cookies ... which currently needs to be parsed by the user somehow.

The only useful thing about params.Cookie currently is:

1. it lets you set a Cookie for every redirect after that ... which probably is a bug now that I write it
2. It lets you to maybe overwrite a cookie set in the jar given that those two match
3. It lets you merge easier with jar cookies
4. It lets you set cookies easier then setting the header.

The problems with those are:

1. this IMO is probably not what anyone expects ?!?
2. it only works if you set the replace:true part which makes the syntax horrible and I doubt there are many people who even know about this or use it. (it also means that it will change if the jar suddenly gets the same cookie for some reason)
3. this is probably the most useful one, but even this one can probably be done easier and IMO more consistently with some js code
4. great ... unless look at 2. at which point it doesn't.

The whole problem IMO comes from the fact that understanding how the 3 ways interact and interact with redirects makes the whole thing way too confusing to the point that even the (technically small) improvements that it does in the case 2-4 likely get removed by the fact that anyone who needs to set cookies specifically as the usual cookie jar support doesn't work will also hit one of the many problems listed above in some respect.

[codecov-commenter]

Codecov Report

Merging #2052 (3778915) into master (86315c6) will decrease coverage by 0.13%.
The diff coverage is 61.40%.

@@            Coverage Diff             @@
##           master    #2052      +/-   ##
==========================================
- Coverage   71.72%   71.58%   -0.14%     
==========================================
  Files         179      177       -2     
  Lines       14100    14148      +48     
==========================================
+ Hits        10113    10128      +15     
- Misses       3349     3372      +23     
- Partials      638      648      +10     

Flag	Coverage Δ
ubuntu	71.58% <61.40%> (-0.05%)	⬇️
windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
js/modules/k6/ws/ws.go	79.38% <57.44%> (-3.71%)	⬇️
lib/testutils/httpmultibin/httpmultibin.go	89.24% <80.00%> (-0.76%)	⬇️
js/common/initenv.go	66.66% <0.00%> (-33.34%)	⬇️
lib/executor/vu_handle.go	93.33% <0.00%> (-3.81%)	⬇️
lib/netext/httpext/error_codes.go	94.52% <0.00%> (-2.74%)	⬇️
js/initcontext.go	90.42% <0.00%> (-2.13%)	⬇️
output/influxdb/config.go	41.81% <0.00%> (-1.82%)	⬇️
js/runner.go	81.30% <0.00%> (-0.57%)	⬇️
cmd/ui_windows.go
lib/netext/httpext/error_codes_syscall_windows.go
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86315c6...3778915. Read the comment docs.

[mstoykov]

Given that the original issue was about jars, which is now fixed and the IMO way to confusing way the Cookie header + cookiejar + cookies param work, I would prefer if we drop the adding the param and close this PR

IMO just adding the header is nearly the same amount of work as adding the param, for the user and IMO is a lot less confusing.