Add support for cookie jar to k6/ws #2193

mstoykov · 2021-10-21T14:51:20Z

This does require the ws module to get access to the http.CookieJar's
jar so we need to make it exported, but through the magic of js tags
we can make it not accessible from the js side.

mstoykov · 2021-10-21T14:55:29Z

IMO this fixes the major problem with cookies in the websocket and specifically #1226. Adding the cookies param as in HTTP IMO is not a good idea:

the merging in k6/http is already very ... strange and not really noticeable
I would argue setting a single cookie is likely not a common problem
Setting the Cookie header is almost the same amount of complication IMO and at least hopefully will be easier to explain how it's merged with the jar cookies

js/modules/k6/ws/ws_test.go

imiric · 2021-10-25T10:52:19Z

js/modules/k6/http/cookiejar.go

@@ -40,6 +40,12 @@ type HTTPCookieJar struct {
 	ctx *context.Context
 }

+// GetJarFromHTTPCookieJar is a helper function for modules outside of http to get the jar from inside the cookie jar
+// TODO figure out how to do this through only goja
+func GetJarFromHTTPCookieJar(jar *HTTPCookieJar) *cookiejar.Jar {


I think this would be more readable/convenient as a method on HTTPCookieJar. GetStdlibJar?

It's unfortunate that the ws module now depends on the http one, but hopefully we can clean this up in an upcoming refactor.

Yes, GetJarFromHTTPCookieJar needs an HTTPCookieJar anyway. By the way, I think the method name can be just: StdlibJar.

The whole point of this not being on the HTTPCookieJar is that I don't want this to be callable from inside the js code ;)

Fair enough:) http.StdlibJar() instead of `GetJarFromHTTPCookieJar‘? It’s already clear that you’re getting an HTTP jar.

inancgumus

LGTM, nice work! I have some minor suggestions.

inancgumus · 2021-10-25T13:14:46Z

js/modules/k6/ws/ws_test.go

@@ -91,7 +93,7 @@ func assertMetricEmittedCount(t *testing.T, metricName string, sampleContainers
 	assert.Equal(t, count, actualCount, "url %s emitted %s %d times, expected was %d times", url, metricName, actualCount, count)
 }

-func newRuntime(t testing.TB) (*httpmultibin.HTTPMultiBin, chan stats.SampleContainer, *goja.Runtime) {
+func newRuntime(t testing.TB) (*httpmultibin.HTTPMultiBin, chan stats.SampleContainer, *goja.Runtime, *context.Context) {


Can it be better if we make it:

func newRuntime(t testing.TB) runtimeState { return runtimeState{ ctx: ..., tb: ..., // httpmultibin.HTTPMultiBin samples: ..., rt: ..., }

It will be clearer and we won't need to discard some of the values if we don't need them like this:

tb, _, rt, _ := newRuntime(t)

inancgumus · 2021-10-25T13:18:12Z

js/modules/k6/ws/ws_test.go

+func TestCookieJar(t *testing.T) {
+	t.Parallel()
+	tb, samples, rt, ctxPtr := newRuntime(t)
+	sr := tb.Replacer.Replace


Instead sr := tb.Replacer.Replace, we can also move this into the runtimeState:

rs := newRuntime() _, err := rs.RunString(`...`) // this can call rs.rt.RunString(sr(`...`))

WDYT?

There are other places we need both the runtime and the replacer that aren't for running a script

inancgumus · 2021-10-25T13:30:31Z

js/modules/k6/ws/ws_test.go

+		conn, err := (&websocket.Upgrader{}).Upgrade(w, req, responseHeaders)
+		if err != nil {
+			t.Fatalf("/ws-echo-someheader cannot upgrade request: %v", err)
+			return


You don't need to return here.

inancgumus · 2021-10-25T13:31:53Z

js/modules/k6/ws/ws_test.go

+		})
+		var someheader = res.headers["Echo-Someheader"];
+		if (someheader != undefined) {
+			throw new Error("somehader is echoed back by test server even though it doesn't exist");


Tiny typo: someheader

This does require the ws module to get access to the http.CookieJar's jar so we need to make it exported, but through the magic of `js` tags we can make it not accessible from the js side.

inancgumus · 2021-10-25T16:06:06Z

js/modules/k6/http/cookiejar.go

@@ -36,7 +36,8 @@ import (

 // HTTPCookieJar is cookiejar.Jar wrapper to be used in js scripts
 type HTTPCookieJar struct {
-	jar *cookiejar.Jar
+	// js is to make it not be accessible from inside goja/js, the json is because it's used if we return it from setup
+	Jar *cookiejar.Jar `js:"-" json:"-"`


You came up with a better solution, cool!

github-actions bot requested review from imiric and inancgumus October 21, 2021 14:51

mstoykov force-pushed the wsJarSupport branch from 4b1ce3c to 8fc3c36 Compare October 21, 2021 14:59

imiric reviewed Oct 25, 2021

View reviewed changes

inancgumus requested changes Oct 25, 2021

View reviewed changes

Add newTestState to ws tests

ee45fa7

mstoykov force-pushed the wsJarSupport branch 2 times, most recently from 7d11be2 to 84547f3 Compare October 25, 2021 15:14

mstoykov requested review from inancgumus and imiric October 25, 2021 15:15

Add support for cookie jar to k6/ws

dc36cb2

This does require the ws module to get access to the http.CookieJar's jar so we need to make it exported, but through the magic of `js` tags we can make it not accessible from the js side.

mstoykov force-pushed the wsJarSupport branch from 84547f3 to dc36cb2 Compare October 25, 2021 15:33

inancgumus reviewed Oct 25, 2021

View reviewed changes

inancgumus approved these changes Oct 25, 2021

View reviewed changes

imiric approved these changes Oct 26, 2021

View reviewed changes

mstoykov merged commit a8f6b73 into master Oct 26, 2021

mstoykov deleted the wsJarSupport branch October 26, 2021 08:00

This was referenced Oct 26, 2021

Support passing CookieJar to WebSocket through Params #1226

Closed

Add cookie to ws #1650

Closed

mstoykov added this to the v0.35.0 milestone Oct 26, 2021

mstoykov mentioned this pull request Nov 17, 2021

Update k6/ws after 0.35.0 grafana/k6-docs#492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cookie jar to k6/ws #2193

Add support for cookie jar to k6/ws #2193

mstoykov commented Oct 21, 2021 •

edited

Loading

mstoykov commented Oct 21, 2021

imiric Oct 25, 2021

inancgumus Oct 25, 2021

mstoykov Oct 25, 2021

inancgumus Oct 25, 2021

inancgumus left a comment

inancgumus Oct 25, 2021

inancgumus Oct 25, 2021

mstoykov Oct 25, 2021

inancgumus Oct 25, 2021

inancgumus Oct 25, 2021

inancgumus Oct 25, 2021

Add support for cookie jar to k6/ws #2193

Add support for cookie jar to k6/ws #2193

Conversation

mstoykov commented Oct 21, 2021 • edited Loading

mstoykov commented Oct 21, 2021

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

inancgumus left a 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

Choose a reason for hiding this comment

mstoykov commented Oct 21, 2021 •

edited

Loading