From ba17c0f517c97b87d17b173774f9abf2dc9b1cbc Mon Sep 17 00:00:00 2001 From: Jimmy Date: Thu, 18 Jul 2024 10:12:21 -0700 Subject: [PATCH 1/7] feat: add a concurrency test --- client_test.go | 66 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 5 deletions(-) diff --git a/client_test.go b/client_test.go index 8d84adc..2f3bc7c 100644 --- a/client_test.go +++ b/client_test.go @@ -75,9 +75,9 @@ func buildServer(code int, body []byte, a func(r *http.Request)) *httptest.Serve })) } -func buildConcurrentServer(code int, t *testing.T, a func(r *http.Request) []byte) *httptest.Server { +func buildConcurrentServer(a func(r *http.Request) (resp []byte, code int)) *httptest.Server { return httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - resp := a(r) + resp, code := a(r) w.WriteHeader(code) w.Header().Set("Content-Type", "application/json") w.Write(resp) @@ -357,7 +357,7 @@ func TestPutAccess(t *testing.T) { func TestConcurrentDeletes(t *testing.T) { var ops uint64 - srv := buildConcurrentServer(200, t, func(r *http.Request) []byte { + srv := buildConcurrentServer(func(r *http.Request) (resp []byte, code int) { if r.Method != "DELETE" { t.Fatalf("%s is not DELETE", r.Method) } @@ -366,7 +366,6 @@ func TestConcurrentDeletes(t *testing.T) { t.Fatalf("%s is not the path for testkey1 or testkey2", r.URL.Path) } atomic.AddUint64(&ops, 1) - var resp []byte var err error if ops%2 == 0 { resp, err = buildGoodResponse("") @@ -379,7 +378,7 @@ func TestConcurrentDeletes(t *testing.T) { t.Fatalf("%s is not nil", err) } } - return resp + return resp, 200 }) defer srv.Close() @@ -401,6 +400,62 @@ func TestConcurrentDeletes(t *testing.T) { } } +func TestConcurrentAddVersion(t *testing.T) { + var ops uint64 + expected := uint64(123) + expected2 := uint64(124) + srv := buildConcurrentServer(func(r *http.Request) (resp []byte, code int) { + if r.Method != "POST" { + t.Fatalf("%s is not POST", r.Method) + } + if r.URL.Path != "/v0/keys/testkey1/versions/" { + t.Fatalf("%s is not the path for testkey1", r.URL.Path) + } + atomic.AddUint64(&ops, 1) + var err error + switch ops { + case 1: + resp, err = buildGoodResponse(expected) + code = 200 + case 2: + resp, err = buildInternalServerErrorResponse(0) + code = 500 + case 3: + resp, err = buildGoodResponse(expected2) + code = 200 + default: + } + if err != nil { + t.Fatalf("%s is not nil", err) + } + return resp, code + }) + defer srv.Close() + + cli := MockClient(srv.Listener.Addr().String(), "") + + // Put a new version of the same key in succession + respData, err := cli.AddVersion("testkey1", []byte("data")) + if err != nil { + t.Fatalf("%s is not nil", err) + } + if respData != expected { + t.Fatalf("expected %d but got %d", expected, respData) + } + respData, err = cli.AddVersion("testkey1", []byte("data")) + if err != nil { + t.Fatalf("%s is not nil", err) + } + if respData != expected2 { + t.Fatalf("expected %d but got %d", expected2, respData) + } + + // Verify that our atomic counter was incremented 3 times + if ops != 3 { + t.Fatalf("%d total client attempts is not 3", ops) + } +} + func TestGetKeyWithStatus(t *testing.T) { expected := Key{ ID: "testkey", @@ -511,6 +566,7 @@ func TestGetInvalidKeys(t *testing.T) { } func TestNewFileClient(t *testing.T) { + t.Skip() _, err := NewFileClient("ThisKeyDoesNotExistSoWeExpectAnError") if (err.Error() != "error getting knox key ThisKeyDoesNotExistSoWeExpectAnError. error: exit status 1") && (err.Error() != "error getting knox key ThisKeyDoesNotExistSoWeExpectAnError. error: exec: \"knox\": executable file not found in $PATH") { t.Fatal("Unexpected error", err.Error()) From 15147cf4cb0151eca67a7eed2f09f3fe091e9d26 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Thu, 18 Jul 2024 10:22:35 -0700 Subject: [PATCH 2/7] fix: remove skip test --- client_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/client_test.go b/client_test.go index 2f3bc7c..5c1a810 100644 --- a/client_test.go +++ b/client_test.go @@ -566,7 +566,6 @@ func TestGetInvalidKeys(t *testing.T) { } func TestNewFileClient(t *testing.T) { - t.Skip() _, err := NewFileClient("ThisKeyDoesNotExistSoWeExpectAnError") if (err.Error() != "error getting knox key ThisKeyDoesNotExistSoWeExpectAnError. error: exit status 1") && (err.Error() != "error getting knox key ThisKeyDoesNotExistSoWeExpectAnError. error: exec: \"knox\": executable file not found in $PATH") { t.Fatal("Unexpected error", err.Error()) From e5a518d0bec9e2c5346952b22f2bb3c0994f1d07 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Thu, 18 Jul 2024 11:12:18 -0700 Subject: [PATCH 3/7] fix: comment --- server/routes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/routes.go b/server/routes.go index 032d5b5..7de21ba 100644 --- a/server/routes.go +++ b/server/routes.go @@ -341,7 +341,7 @@ func putAccessHandler(m KeyManager, principal knox.Principal, parameters map[str // postVersionHandler creates a new key version. This version is immediately // added as an Active key. -// The route for this handler is PUT /v0/keys//versions/ +// The route for this handler is POST /v0/keys//versions/ // The principal needs Write access. func postVersionHandler(m KeyManager, principal knox.Principal, parameters map[string]string) (interface{}, *HTTPError) { From 438da7709c01d4ddd68ebf2fe193aaa02c269092 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Thu, 18 Jul 2024 12:07:23 -0700 Subject: [PATCH 4/7] fix: test case should return nil --- client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client_test.go b/client_test.go index 5c1a810..125da35 100644 --- a/client_test.go +++ b/client_test.go @@ -418,7 +418,7 @@ func TestConcurrentAddVersion(t *testing.T) { resp, err = buildGoodResponse(expected) code = 200 case 2: - resp, err = buildInternalServerErrorResponse(0) + resp, err = buildInternalServerErrorResponse(nil) code = 500 case 3: resp, err = buildGoodResponse(expected2) From d5b10e682c8d5f75e8c5c88c70a1f636eee6457b Mon Sep 17 00:00:00 2001 From: Jimmy Date: Thu, 18 Jul 2024 12:11:00 -0700 Subject: [PATCH 5/7] fix: don't write outparam if null --- client.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/client.go b/client.go index 0588021..0f6a104 100644 --- a/client.go +++ b/client.go @@ -535,8 +535,18 @@ func getHTTPResp(cli HTTP, r *http.Request, resp *Response) error { } defer w.Body.Close() + prevRespData := resp.Data decoder := json.NewDecoder(w.Body) - return decoder.Decode(resp) + err = decoder.Decode(resp) + if err != nil { + return err + } + + if resp.Data == nil { + resp.Data = prevRespData + } + + return nil } // MockClient builds a client that ignores certs and talks to the given host. From 98453581daad65cd948284866d2e945684658f66 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Thu, 18 Jul 2024 12:18:37 -0700 Subject: [PATCH 6/7] chore: clean up code a bit --- client.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/client.go b/client.go index 0f6a104..089a769 100644 --- a/client.go +++ b/client.go @@ -536,8 +536,7 @@ func getHTTPResp(cli HTTP, r *http.Request, resp *Response) error { defer w.Body.Close() prevRespData := resp.Data - decoder := json.NewDecoder(w.Body) - err = decoder.Decode(resp) + err = json.NewDecoder(w.Body).Decode(resp) if err != nil { return err } From 178022ab96a572d1a83bdafe84d55158c4112dff Mon Sep 17 00:00:00 2001 From: Jimmy Date: Thu, 18 Jul 2024 12:20:23 -0700 Subject: [PATCH 7/7] chore: add comment explaining rational for why we must reset resp.Data --- client.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client.go b/client.go index 089a769..7fe2171 100644 --- a/client.go +++ b/client.go @@ -541,6 +541,8 @@ func getHTTPResp(cli HTTP, r *http.Request, resp *Response) error { return err } + // NOTE: in case of error, the server may return the data is nil; we must not accept this value but keep + // the other Response values. This is because if it is set to nil, our outpointer writing would fail and do nothing on retry if resp.Data == nil { resp.Data = prevRespData }