Make cloud API requests idempotent #1208

cuonglm · 2019-10-21T03:58:28Z

When k6 requests to cloud API timeout, k6 will retry the request a
few times. This can be problematic, because the retrying request can
cause user's test start twice.

This PR tries to fix that problem by using a session token for each POST
request. With this token, cloud API can know that the request is the
retried one, and avoid creating new test, and return the old one.

Speaking in other way, k6 POST request to cloud API becomes idempotent.

Fixes #1205

codecov-io · 2019-10-21T04:16:16Z

Codecov Report

Merging #1208 into master will decrease coverage by <.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1208      +/-   ##
==========================================
- Coverage   73.65%   73.65%   -0.01%     
==========================================
  Files         147      147              
  Lines       10641    10656      +15     
==========================================
+ Hits         7838     7849      +11     
- Misses       2344     2346       +2     
- Partials      459      461       +2

Impacted Files	Coverage Δ
stats/cloud/client.go	`75.96% <88.88%> (+2.39%)`	⬆️
core/engine.go	`93.77% <0%> (-0.96%)`	⬇️
js/modules/k6/html/html.go	`85.08% <0%> (-0.06%)`	⬇️
lib/testutils/httpmultibin/httpmultibin.go	`92.53% <0%> (-0.06%)`	⬇️

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 5cda323...7452ffc. Read the comment docs.

mstoykov · 2019-10-21T05:45:55Z

stats/cloud/api_test.go

+func TestSessionToken(t *testing.T) {
+	sessionToken := ""
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPost {
+			gotK6SessionToken := r.Header.Get(k6SessionTokenHeader)
+			if sessionToken == "" {
+				sessionToken = gotK6SessionToken
+			} else {
+				assert.NotEmpty(t, gotK6SessionToken)
+				assert.Equal(t, sessionToken, gotK6SessionToken)
+			}
+		}
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	client := NewClient("token", server.URL, "1.0")
+	client.retryInterval = 1 * time.Millisecond
+	req, err := client.NewRequest(http.MethodPost, server.URL, nil)
+	assert.NoError(t, err)
+	req.Header.Set(k6SessionTokenHeader, "xxx")
+	assert.NoError(t, client.Do(req, nil))
+
+	req, err = client.NewRequest(http.MethodGet, server.URL, nil)
+	assert.NoError(t, err)
+	assert.NoError(t, client.Do(req, nil))
+}


Not certain what this test adds that the others you have changed don't already do ...

@mstoykov this test for 2 case:

POST request with user supplies session token success.

Non-POST request success without session token.

it doesn't test that the token that is set manually is the one that is used

it doesn't test that the Non-POST request don't send a token
So in both cases those are tested by other tests already

@mstoykov Fair point, but I think it just does mean that the test does not do the actual meaningful test, not mean both cases those are tested by other tests already

I'm sure because you can check code cov log, I add the test above because code cov complains about missing of 2 above cases.

Anyway, updated PR coming.

mstoykov · 2019-10-21T05:52:14Z

stats/cloud/client.go

+	if req.Method != http.MethodPost {
+		return nil
+	}
+	if req.Header.Get(k6SessionTokenHeader) != "" {
+		return nil
+	}


I don't think either of those or necessary:

This can be used for all requests not only POSTs

I don't see why calling setSessionToken should not overwrite an old token (maybe the tests were failing

If each of 1 or 2 is not true we can just not call setSessionToken inside Client.Do but insted in Client.CreateTestRun

@mstoykov

This can be used for all requests not only POSTs

Yes, but idempotency should be applied for POST only. There's nothing you should do with other requests. For k6, generating wasted HTTP header, which backend does not handle increase both bandwidth and load for our cloud API server

I don't see why calling setSessionToken should not overwrite an old token (maybe the tests were failing

The point is that if the caller can specify its session token id, instead of relying on our generating one. See TestSessionToken for example

If each of 1 or 2 is not true we can just not call setSessionToken inside Client.Do but insted in Client.CreateTestRun

Yes, but it's not well design to just handle specific case for CreateTestRun. In future, if we introduce more Post endpoint which need idempotency, this design works for all of them.

The problem is that ... literally we have 1 POST we need this for ... and while there is possibility we might add it to others, this is not the case today and I find it unlikely.
Also the method name is ... lying it's not just setting a SessionToken, it's setting only if ... some conditions are met.
To be honest I think generateSessionToken and k6SessionTokenHeader should become a public use them in CreateTestRun and let people if want to to set them in a custom request and send it ... which is also unlikely use case but still ... it would be possible.
If we ever want to do it for all Posts I would argue that the if method == POST should be in the body of the client.Do not in some function

If we ever want to do it for all Posts I would argue that the if method == POST should be in the body of the client.Do not in some function

Fair point, this change and with satori uuid, the code looks much cleaner

mstoykov · 2019-10-21T05:53:29Z

stats/cloud/client.go

+	sessionToken, err := uuid.NewV4()
+	if err != nil {
+		return "", err
+	}
+
+	return sessionToken.String(), nil


I really don't like that now we have an error that we have to check that

we have no way of fixing

it's very unlikely that crypto/rand#Read will return an error >.>

But unfortunately I don't have any proposals ...

I often use https://github.com/satori/go.uuid in all my previous projects. Since when I see current uuid package inside vendor already, I don't want to touch it in this PR.

We can ignore it for now and I will create separate issue for changing uuid package.

How do you think @na-- @imiric ?

I don't think changing to a different uuid package will help us with the error ... the problem is crypto.rand#Read can return an error and all of those use them AFAIK.
My argument is that we don't need crypto/rand at all and the non crypto one always returns a nil error :). But I don't think it's that big of problem ... I just really dislike that we added another error that is

unlikely

we have 0 things we can do, but report it or retry ... maybe

can't test

@mstoykov not sure what's your point. My point is that satori/go.uuid does not return error when creating new uuid string, so we don't have to check for error.

Hmm, maybe I should change the current uuid package, as I see it is not used anywhere in current code base (not sure why it existed in vendor)

Okay ... so apparently that lib you just added has a last version which panics on error and the very next commit (nearly 2 years ago) adds an error to be returned, which is what is shown in the godoc.

I am very strongly against adding another UUID lib ... just for this.

I am also very opposed to panicing. I have no idea when crypto/rand could return an error but still.

For me using crypto/rand for this particular token is an overkill .. if it wasn't for the fact we add an error I would've not cared :) , but that is what is bugging me - the error(or panic with this new lib), not the fact we use crypto for it

@mstoykov note that even you use math/rand, its rand.Read still returns an error, too. So why bother to use crypto/rand

@mstoykov Here's what I often do:

func generateSessionToken() string { // 16 hex characters b := make([]byte, 8) if _, err := rand.Read(b); err != nil { panic(err) } return hex.EncodeToString(b) }

as per the docs it always return a nil error, so we can ignore it safely knowing it's never going to happen.

@mstoykov good point 👍

stats/cloud/client.go

na-- · 2019-10-23T13:35:37Z

As we spoke on the call, I'm completely walking back from my desire of a session-id 😞 We don't currently need it, and it would unnecessarily complicate the distributed execution, so the per-request idempotency key would be sufficient for now.

cuonglm · 2019-10-24T11:39:29Z

As we spoke on the call, I'm completely walking back from my desire of a session-id 😞 We don't currently need it, and it would unnecessarily complicate the distributed execution, so the per-request idempotency key would be sufficient for now.

For clarifying, "per-request" means "per-POST-request" or every requests?

na-- · 2019-10-24T11:59:37Z

As we spoke on the call, I'm completely walking back from my desire of a session-id disappointed We don't currently need it, and it would unnecessarily complicate the distributed execution, so the per-request idempotency key would be sufficient for now.

For clarifying, "per-request" means "per-POST-request" or every requests?

Per non-GET/non-HEAD request, since these should be idempotent by default 😉 And while we currently have only POST requests IIRC, we might have PUT or DELETE requests in the future, where this could also be useful

mstoykov · 2019-10-24T12:03:37Z

I am still of the opinion that the session token should be set by each and every constructor of the corresponding request not in Client.Do.

na-- · 2019-10-24T12:30:47Z

@mstoykov, let's not call it session token, since that implies it's persistent for the whole session (which we agreed we're not going to do now, since it would complicate distributed execution and is not needed to fix the particular problem). Rather, let's call the new header Idempotency-Key, borrowing from https://stripe.com/docs/api/idempotent_requests (kudos to @cuonglm for sending me that link).

But yeah, it'd probably be more visible and understandable if we're setting that header value in the request constructor, not in the Do() method...

stats/cloud/api_test.go

na-- · 2019-10-24T20:23:54Z

stats/cloud/client.go

@@ -94,6 +100,10 @@ func (c *Client) Do(req *http.Request, v interface{}) error {
 		}
 	}

+	if shouldAddIdempotencyKey(req) {


Like we mentioned in the PR global comments, this seems better suited for the NewRequest() method

I against it, as we may want the caller to supply its own Idempotency-Key. Also, I have a patch local here, which will move all request header preparation to here instead of in Client.do, but it will go after this PR merged.

Fair enough, but the user would also be able to supply their own idempotency key even if we handle this in NewRequest(), it would just be done by deliberately overwriting whatever we generated there.

stats/cloud/api_test.go

stats/cloud/client.go

na-- · 2019-10-25T06:09:44Z

stats/cloud/client.go

@@ -94,6 +100,10 @@ func (c *Client) Do(req *http.Request, v interface{}) error {
 		}
 	}

+	if shouldAddIdempotencyKey(req) {


Fair enough, but the user would also be able to supply their own idempotency key even if we handle this in NewRequest(), it would just be done by deliberately overwriting whatever we generated there.

stats/cloud/client.go

na--

LGTM now 😄 Just please squash it to one or two (timeout increase+the rest) commits. And maybe add a //nolint:gosec in randomStrHex() so that the linter doesn't complain.

When k6 requests to cloud API timeout, k6 will retry the request a few times. This can be problematic, because the retrying request can cause user's test start twice. This PR tries to fix that problem by using an idempotency key for each request. With this, cloud API can avoid handling duplicated requests. Fixes #1205

stats/cloud/client.go

stats/cloud: increase default request timeout

05d7b84

cuonglm force-pushed the feature/1205 branch from 7eef5d4 to c3c9c06 Compare October 21, 2019 04:02

cuonglm added this to the v0.26.0 milestone Oct 21, 2019

cuonglm requested review from mstoykov, na-- and imiric October 21, 2019 04:34

mstoykov requested changes Oct 21, 2019

View reviewed changes

cuonglm requested a review from mstoykov October 21, 2019 09:03

na-- requested changes Oct 23, 2019

View reviewed changes

stats/cloud/client.go Outdated Show resolved Hide resolved

cuonglm requested a review from na-- October 23, 2019 11:02

golangcibot reviewed Oct 23, 2019

View reviewed changes

stats/cloud/client.go Outdated Show resolved Hide resolved

stats/cloud/client.go Show resolved Hide resolved

na-- requested changes Oct 25, 2019

View reviewed changes

cuonglm requested a review from na-- October 25, 2019 06:25

golangcibot reviewed Oct 25, 2019

View reviewed changes

stats/cloud/client.go Outdated Show resolved Hide resolved

na-- previously approved these changes Oct 25, 2019

View reviewed changes

cuonglm dismissed na--’s stale review via 593d85b October 25, 2019 06:48

cuonglm force-pushed the feature/1205 branch from 95dfad1 to 593d85b Compare October 25, 2019 06:48

cuonglm force-pushed the feature/1205 branch from 593d85b to 7452ffc Compare October 25, 2019 06:50

cuonglm requested a review from na-- October 25, 2019 06:50

mstoykov reviewed Oct 25, 2019

View reviewed changes

stats/cloud/client.go Show resolved Hide resolved

na-- approved these changes Oct 25, 2019

View reviewed changes

cuonglm requested a review from mstoykov October 25, 2019 06:59

cuonglm changed the title ~~Make cloud API post request idempotent~~ Make cloud API requests idempotent Oct 25, 2019

mstoykov approved these changes Oct 25, 2019

View reviewed changes

cuonglm merged commit bbe7794 into master Oct 25, 2019

cuonglm deleted the feature/1205 branch October 25, 2019 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make cloud API requests idempotent #1208

Make cloud API requests idempotent #1208

cuonglm commented Oct 21, 2019

codecov-io commented Oct 21, 2019 •

edited

mstoykov Oct 21, 2019

cuonglm Oct 21, 2019

mstoykov Oct 21, 2019

cuonglm Oct 21, 2019

mstoykov Oct 21, 2019

cuonglm Oct 21, 2019 •

edited

mstoykov Oct 21, 2019

cuonglm Oct 21, 2019 •

edited

mstoykov Oct 21, 2019

cuonglm Oct 21, 2019

mstoykov Oct 21, 2019

cuonglm Oct 21, 2019 •

edited

cuonglm Oct 21, 2019

mstoykov Oct 21, 2019

cuonglm Oct 21, 2019 •

edited

cuonglm Oct 21, 2019

mstoykov Oct 21, 2019 •

edited

cuonglm Oct 21, 2019

na-- commented Oct 23, 2019

cuonglm commented Oct 24, 2019

na-- commented Oct 24, 2019

mstoykov commented Oct 24, 2019

na-- commented Oct 24, 2019 •

edited

na-- Oct 24, 2019

cuonglm Oct 25, 2019

na-- Oct 25, 2019

cuonglm Oct 25, 2019

na-- Oct 25, 2019

na-- left a comment

Make cloud API requests idempotent #1208

Make cloud API requests idempotent #1208

Conversation

cuonglm commented Oct 21, 2019

codecov-io commented Oct 21, 2019 • edited

Codecov Report

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

cuonglm Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuonglm Oct 21, 2019 • edited

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

cuonglm Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuonglm Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- commented Oct 23, 2019

cuonglm commented Oct 24, 2019

na-- commented Oct 24, 2019

mstoykov commented Oct 24, 2019

na-- commented Oct 24, 2019 • edited

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

na-- left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 21, 2019 •

edited

cuonglm Oct 21, 2019 •

edited

cuonglm Oct 21, 2019 •

edited

cuonglm Oct 21, 2019 •

edited

cuonglm Oct 21, 2019 •

edited

mstoykov Oct 21, 2019 •

edited

na-- commented Oct 24, 2019 •

edited