From: Tom Clegg <tom@curii.com>
Date: Wed, 15 Mar 2023 18:28:16 +0000 (-0400)
Subject: 20187: Check that the discovery doc is really a discovery doc.
X-Git-Tag: 2.6.0~11^2~6
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/b8d80c01a51ce3d271f838231b90e70bf4460d0e

20187: Check that the discovery doc is really a discovery doc.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---

diff --git a/lib/controller/handler.go b/lib/controller/handler.go
index b3d850ac04..c213c897c3 100644
--- a/lib/controller/handler.go
+++ b/lib/controller/handler.go
@@ -171,7 +171,7 @@ func (h *Handler) setup() {
 		Name: "arvados-controller",
 	}
 	h.cache = map[string]*cacheEnt{
-		"/discovery/v1/apis/arvados/v1/rest": &cacheEnt{},
+		"/discovery/v1/apis/arvados/v1/rest": &cacheEnt{validateDD: true},
 	}
 
 	go h.trashSweepWorker()
@@ -222,6 +222,7 @@ func (h *Handler) limitLogCreateRequests(w http.ResponseWriter, req *http.Reques
 // cacheEnt implements a basic stale-while-revalidate cache, suitable
 // for the Arvados discovery document.
 type cacheEnt struct {
+	validateDD   bool
 	mtx          sync.Mutex
 	header       http.Header
 	body         []byte
@@ -277,7 +278,14 @@ func (ent *cacheEnt) refresh(path string, do func(*http.Request) (*http.Response
 			header[k] = v
 		}
 	}
-	if mediatype, _, err := mime.ParseMediaType(header.Get("Content-Type")); err == nil && mediatype == "application/json" {
+	if ent.validateDD {
+		var dd arvados.DiscoveryDocument
+		if err := json.Unmarshal(body, &dd); err != nil {
+			return nil, nil, fmt.Errorf("error decoding JSON response: %w", err)
+		} else if dd.BasePath == "" {
+			return nil, nil, errors.New("error in discovery document: no value for basePath")
+		}
+	} else if mediatype, _, err := mime.ParseMediaType(header.Get("Content-Type")); err == nil && mediatype == "application/json" {
 		if !json.Valid(body) {
 			return nil, nil, errors.New("invalid JSON encoding in response")
 		}
diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go
index 4a7e1ad78a..02b72dc459 100644
--- a/lib/controller/handler_test.go
+++ b/lib/controller/handler_test.go
@@ -131,8 +131,12 @@ func (s *HandlerSuite) TestDiscoveryDocCache(c *check.C) {
 		return &wg
 	}
 	clearCache := func() {
-		for path := range s.handler.cache {
-			s.handler.cache[path] = &cacheEnt{}
+		for _, ent := range s.handler.cache {
+			ent.refreshLock.Lock()
+			ent.mtx.Lock()
+			ent.body, ent.header, ent.refreshAfter = nil, nil, time.Time{}
+			ent.mtx.Unlock()
+			ent.refreshLock.Unlock()
 		}
 	}
 	expireCache := func() {
@@ -188,11 +192,15 @@ func (s *HandlerSuite) TestDiscoveryDocCache(c *check.C) {
 	}
 	c.Check(countRailsReqs(), check.Equals, reqsBefore+1)
 
-	// Configure railsSpy to return an error when wantError==true.
-	var wantError bool
+	// Configure railsSpy to return an error or bad content
+	// depending on flags.
+	var wantError, wantBadContent bool
 	s.railsSpy.Director = func(req *http.Request) {
 		if wantError {
 			req.Method = "MAKE-COFFEE"
+		} else if wantBadContent {
+			req.URL.Path = "/_health/ping"
+			req.Header.Set("Authorization", "Bearer "+arvadostest.ManagementToken)
 		}
 	}
 
@@ -208,9 +216,16 @@ func (s *HandlerSuite) TestDiscoveryDocCache(c *check.C) {
 	wg.Wait()
 	c.Check(countRailsReqs(), check.Equals, reqsBefore+5)
 
+	// Response status is OK but body is not a discovery document
+	wantError = false
+	wantBadContent = true
+	reqsBefore = countRailsReqs()
+	c.Check(getDD(), check.Equals, http.StatusBadGateway)
+	c.Check(countRailsReqs(), check.Equals, reqsBefore+1)
+
 	// Error condition clears => caller gets OK, cache is warmed
 	// up
-	wantError = false
+	wantBadContent = false
 	reqsBefore = countRailsReqs()
 	getDDConcurrently(5, http.StatusOK, check.Commentf("success after errors at startup")).Wait()
 	c.Check(countRailsReqs(), check.Equals, reqsBefore+1)