7200: The incomplete response when no such prefix exists will be "\n". Update keepcli...

Also, added some more comments and golint checks for the newly added code.

diff --git a/sdk/go/keepclient/keepclient.go b/sdk/go/keepclient/keepclient.go
index a7522eb2e6b4a2b4deec5bd019cd9658fb9b9e4b..c7a543c6f1508a723ce54396beeb9d558cd290e5 100644 (file)
--- a/sdk/go/keepclient/keepclient.go
+++ b/sdk/go/keepclient/keepclient.go
@@ -28,9 +28,15 @@ var OversizeBlockError = errors.New("Exceeded maximum block size (" + strconv.It
 var MissingArvadosApiHost = errors.New("Missing required environment variable ARVADOS_API_HOST")
 var MissingArvadosApiToken = errors.New("Missing required environment variable ARVADOS_API_TOKEN")
 var InvalidLocatorError = errors.New("Invalid locator")
-var NoSuchKeepServer = errors.New("No keep server matching the given UUID is found")
-var GetIndexError = errors.New("Error getting index")
-var IncompleteIndexError = errors.New("Got incomplete index")
+
+// ErrorGetIndex is the generic GetIndex error
+var ErrorGetIndex = errors.New("Error getting index")
+
+// ErrorNoSuchKeepServer is returned when GetIndex is invoked with a UUID with no matching keep server
+var ErrorNoSuchKeepServer = errors.New("No keep server matching the given UUID is found")
+
+// ErrorIncompleteIndex is returned when the Index response does not end with a new empty line
+var ErrorIncompleteIndex = errors.New("Got incomplete index")
 
 const X_Keep_Desired_Replicas = "X-Keep-Desired-Replicas"
 const X_Keep_Replicas_Stored = "X-Keep-Replicas-Stored"
@@ -194,7 +200,7 @@ func (kc *KeepClient) GetIndex(keepServiceUUID, prefix string) (io.Reader, error
        url := kc.LocalRoots()[keepServiceUUID]
        if url == "" {
                log.Printf("No such keep server found: %v", keepServiceUUID)
-               return nil, NoSuchKeepServer
+               return nil, ErrorNoSuchKeepServer
        }
 
        url += "/index"
@@ -205,33 +211,35 @@ func (kc *KeepClient) GetIndex(keepServiceUUID, prefix string) (io.Reader, error
        req, err := http.NewRequest("GET", url, nil)
        if err != nil {
                log.Printf("GET index error: %v", err)
-               return nil, GetIndexError
+               return nil, ErrorGetIndex
        }
 
        req.Header.Add("Authorization", fmt.Sprintf("OAuth2 %s", kc.Arvados.ApiToken))
        resp, err := kc.Client.Do(req)
        if err != nil || resp.StatusCode != http.StatusOK {
                log.Printf("GET index error: %v; status code: %v", err, resp.StatusCode)
-               return nil, GetIndexError
+               return nil, ErrorGetIndex
        }
 
-       var respbody []byte
+       var respBody []byte
        if resp.Body != nil {
-               respbody, err = ioutil.ReadAll(resp.Body)
+               respBody, err = ioutil.ReadAll(resp.Body)
                if err != nil {
                        log.Printf("GET index error: %v", err)
-                       return nil, GetIndexError
+                       return nil, ErrorGetIndex
                }
 
                // Got index; verify that it is complete
-               if !strings.HasSuffix(string(respbody), "\n\n") {
+               // The response should be "\n" if no locators matched the prefix
+               // Else, it should be a list of locators followed by a blank line
+               if (!strings.HasSuffix(string(respBody), "\n\n")) && (string(respBody) != "\n") {
                        log.Printf("Got incomplete index for %v", url)
-                       return nil, IncompleteIndexError
+                       return nil, ErrorIncompleteIndex
                }
        }
 
-       // Got complete index or "" if no locators matching prefix
-       return strings.NewReader(string(respbody)), nil
+       // Got complete index
+       return strings.NewReader(string(respBody)), nil
 }
 
 // LocalRoots() returns the map of local (i.e., disk and proxy) Keep

diff --git a/sdk/go/keepclient/keepclient_test.go b/sdk/go/keepclient/keepclient_test.go
index f83bee14a7cc551b29e86432ef8c847d7da04147..588037314b8609b907e5d2adddab2c2235c6234a 100644 (file)
--- a/sdk/go/keepclient/keepclient_test.go
+++ b/sdk/go/keepclient/keepclient_test.go
@@ -1064,10 +1064,10 @@ func (s *StandaloneSuite) TestGetIndexWithNoSuchServer(c *C) {
 func (s *StandaloneSuite) TestGetIndexWithNoSuchPrefix(c *C) {
        st := StubGetIndexHandler{
                c,
-               "/index/xyz",
+               "/index/abcd",
                "abc123",
                http.StatusOK,
-               []byte("")}
+               []byte("\n")}
 
        ks := RunFakeKeepServer(st)
        defer ks.listener.Close()
@@ -1077,6 +1077,10 @@ func (s *StandaloneSuite) TestGetIndexWithNoSuchPrefix(c *C) {
        arv.ApiToken = "abc123"
        kc.SetServiceRoots(map[string]string{"x": ks.url}, map[string]string{ks.url: ""}, nil)
 
-       _, err = kc.GetIndex("x", "xyz")
-       c.Check((err != nil), Equals, true)
+       r, err := kc.GetIndex("x", "abcd")
+       c.Check(err, Equals, nil)
+
+       content, err2 := ioutil.ReadAll(r)
+       c.Check(err2, Equals, nil)
+       c.Check(content, DeepEquals, st.body)
 }

diff --git a/services/keepproxy/keepproxy.go b/services/keepproxy/keepproxy.go
index 788cbfcdcf17ac018c9a445602d8e853f8b3b6fd..865212d747691f6423e5b1701575f15206f2abd4 100644 (file)
--- a/services/keepproxy/keepproxy.go
+++ b/services/keepproxy/keepproxy.go
@@ -273,7 +273,7 @@ func MakeRESTRouter(
                rest.Handle(`/index`, IndexHandler{kc, t}).Methods("GET")
 
                // List blocks whose hash has the given prefix
-               rest.Handle(`/index/{prefix}`, IndexHandler{kc, t}).Methods("GET")
+               rest.Handle(`/index/{prefix:[0-9a-f]{0,32}}`, IndexHandler{kc, t}).Methods("GET")
        }
 
        if enable_put {
@@ -494,7 +494,14 @@ func (this PutBlockHandler) ServeHTTP(resp http.ResponseWriter, req *http.Reques
        }
 }
 
-func (this IndexHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
+// ServeHTTP implemenation for IndexHandler
+// Supports only GET requests for /index/{prefix:[0-9a-f]{0,32}}
+// For each keep server found in LocalRoots:
+//   Invokes GetIndex using keepclient
+//   Expects "complete" response (terminating with blank new line)
+//   Aborts on any errors
+// Concatenates responses from all those keep servers and returns
+func (handler IndexHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
        SetCorsHeaders(resp)
 
        prefix := mux.Vars(req)["prefix"]
@@ -507,11 +514,11 @@ func (this IndexHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request)
                }
        }()
 
-       kc := *this.KeepClient
+       kc := *handler.KeepClient
 
        var pass bool
        var tok string
-       if pass, tok = CheckAuthorizationHeader(kc, this.ApiTokenCache, req); !pass {
+       if pass, tok = CheckAuthorizationHeader(kc, handler.ApiTokenCache, req); !pass {
                status, err = http.StatusForbidden, BadAuthorizationHeader
                return
        }
@@ -526,7 +533,7 @@ func (this IndexHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request)
 
        switch req.Method {
        case "GET":
-               for uuid, _ := range kc.LocalRoots() {
+               for uuid := range kc.LocalRoots() {
                        reader, err = kc.GetIndex(uuid, prefix)
                        if err != nil {
                                break
@@ -539,12 +546,17 @@ func (this IndexHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request)
                        }
 
                        // Got index; verify that it is complete
-                       if !strings.HasSuffix(string(readBytes), "\n\n") {
+                       // The response should be "\n" if no locators matched the prefix
+                       // Else, it should be a list of locators followed by a blank line
+                       if (!strings.HasSuffix(string(readBytes), "\n\n")) && (string(readBytes) != "\n") {
                                err = errors.New("Got incomplete index")
                        }
 
+                       // Trim the extra empty new line found in response from each server
                        indexResp = append(indexResp, (readBytes[0 : len(readBytes)-1])...)
                }
+
+               // Append empty line at the end of concatenation of all server responses
                indexResp = append(indexResp, ([]byte("\n"))...)
        default:
                status, err = http.StatusNotImplemented, MethodNotSupported

diff --git a/services/keepproxy/keepproxy_test.go b/services/keepproxy/keepproxy_test.go
index a08d1b603929424259ff0a65ad6eb7d80e227c97..1afe37343f1628fc688e156e1447da195383e42d 100644 (file)
--- a/services/keepproxy/keepproxy_test.go
+++ b/services/keepproxy/keepproxy_test.go
@@ -407,6 +407,12 @@ func (s *ServerRequiredSuite) TestStripHint(c *C) {
 
 }
 
+// Test GetIndex
+//   Put one block, with 2 replicas
+//   With no prefix (expect the block locator, twice)
+//   With an existing prefix (expect the block locator, twice)
+//   With a valid but non-existing prefix (expect "\n")
+//   With an invalid prefix (expect error)
 func (s *ServerRequiredSuite) TestGetIndex(c *C) {
        kc := runProxy(c, []string{"keepproxy"}, 28852, false)
        waitForListener()
@@ -434,7 +440,7 @@ func (s *ServerRequiredSuite) TestGetIndex(c *C) {
        count := 0
        for _, locator := range locators {
                if strings.HasPrefix(locator, hash) {
-                       count += 1
+                       count++
                }
        }
        c.Assert(2, Equals, count)
@@ -447,12 +453,18 @@ func (s *ServerRequiredSuite) TestGetIndex(c *C) {
        count = 0
        for _, locator := range locators {
                if strings.HasPrefix(locator, hash) {
-                       count += 1
+                       count++
                }
        }
        c.Assert(2, Equals, count)
 
-       // GetIndex with no such prefix
+       // GetIndex with valid but no such prefix
+       indexReader, err = kc.GetIndex("proxy", "abcd")
+       c.Assert(err, Equals, nil)
+       indexResp, err = ioutil.ReadAll(indexReader)
+       c.Assert(string(indexResp), Equals, "\n")
+
+       // GetIndex with invalid prefix
        indexReader, err = kc.GetIndex("proxy", "xyz")
        c.Assert((err != nil), Equals, true)
 }

diff --git a/services/keepstore/handler_test.go b/services/keepstore/handler_test.go
index dd8366671b6b056f8e4161cfae8169cd6a086cc6..ba923cad768abc6fe7d906bc82bc67b0bef6276d 100644 (file)
--- a/services/keepstore/handler_test.go
+++ b/services/keepstore/handler_test.go
@@ -321,6 +321,16 @@ func TestIndexHandler(t *testing.T) {
                uri:      "/index/" + TestHash[0:3],
                apiToken: dataManagerToken,
        }
+       superuserNoSuchPrefixReq := &RequestTester{
+               method:   "GET",
+               uri:      "/index/abcd",
+               apiToken: dataManagerToken,
+       }
+       superuserInvalidPrefixReq := &RequestTester{
+               method:   "GET",
+               uri:      "/index/xyz",
+               apiToken: dataManagerToken,
+       }
 
        // -------------------------------------------------------------
        // Only the superuser should be allowed to issue /index requests.
@@ -407,6 +417,26 @@ func TestIndexHandler(t *testing.T) {
                        "permissions on, superuser /index/prefix request: expected %s, got:\n%s",
                        expected, response.Body.String())
        }
+
+       // superuser /index/{no-such-prefix} request
+       // => OK
+       response = IssueRequest(superuserNoSuchPrefixReq)
+       ExpectStatusCode(t,
+               "permissions on, superuser request",
+               http.StatusOK,
+               response)
+
+       if "\n" != response.Body.String() {
+               t.Errorf("Expected empty response for %s. Found %s", superuserNoSuchPrefixReq.uri, response.Body.String())
+       }
+
+       // superuser /index/{invalid-prefix} request
+       // => StatusBadRequest
+       response = IssueRequest(superuserInvalidPrefixReq)
+       ExpectStatusCode(t,
+               "permissions on, superuser request",
+               http.StatusBadRequest,
+               response)
 }
 
 // TestDeleteHandler