From 1ba00901283d88fc7a5c82cabdad6e5183d4bb78 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tclegg@veritasgenetics.com>
Date: Wed, 7 Aug 2019 15:09:35 -0400
Subject: [PATCH] 15453: Don't use "*" as a real remote. Return 502 for remote
 errors.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>
---
 lib/controller/fed_collections.go | 10 ++++++----
 lib/controller/federation/conn.go |  8 ++++----
 lib/controller/federation_test.go | 17 ++++++++++++++++-
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/lib/controller/fed_collections.go b/lib/controller/fed_collections.go
index 07daf2f90e..b7b1e64483 100644
--- a/lib/controller/fed_collections.go
+++ b/lib/controller/fed_collections.go
@@ -232,6 +232,10 @@ func fetchRemoteCollectionByPDH(
 			// No need to query local cluster again
 			continue
 		}
+		if remoteID == "*" {
+			// This isn't a real remote cluster: it just sets defaults for unlisted remotes.
+			continue
+		}
 
 		wg.Add(1)
 		go func(remote string) {
@@ -293,10 +297,8 @@ func fetchRemoteCollectionByPDH(
 			var errors []string
 			for len(errorChan) > 0 {
 				err := <-errorChan
-				if httperr, ok := err.(HTTPError); ok {
-					if httperr.Code != http.StatusNotFound {
-						errorCode = http.StatusBadGateway
-					}
+				if httperr, ok := err.(HTTPError); !ok || httperr.Code != http.StatusNotFound {
+					errorCode = http.StatusBadGateway
 				}
 				errors = append(errors, err.Error())
 			}
diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 63e801b22c..3bcafacd2c 100644
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -142,8 +142,7 @@ func (conn *Conn) tryLocalThenRemotes(ctx context.Context, fn func(context.Conte
 	if all404 {
 		return notFoundError{}
 	}
-	// FIXME: choose appropriate HTTP status
-	return fmt.Errorf("errors: %v", errs)
+	return httpErrorf(http.StatusBadGateway, "errors: %v", errs)
 }
 
 func (conn *Conn) CollectionCreate(ctx context.Context, options arvados.CreateOptions) (arvados.Collection, error) {
@@ -206,8 +205,9 @@ func (conn *Conn) CollectionGet(ctx context.Context, options arvados.GetOptions)
 			// hash+size+hints; only hash+size need to
 			// match the computed PDH.
 			if pdh := portableDataHash(c.ManifestText); pdh != options.UUID && !strings.HasPrefix(options.UUID, pdh+"+") {
-				ctxlog.FromContext(ctx).Warnf("bad portable data hash %q received from remote %q (expected %q)", pdh, remoteID, options.UUID)
-				return notFoundError{}
+				err = httpErrorf(http.StatusBadGateway, "bad portable data hash %q received from remote %q (expected %q)", pdh, remoteID, options.UUID)
+				ctxlog.FromContext(ctx).Warn(err)
+				return err
 			}
 			if remoteID != "" {
 				c.ManifestText = rewriteManifest(c.ManifestText, remoteID)
diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go
index 169b1f7961..c654277fea 100644
--- a/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@ -83,6 +83,9 @@ func (s *FederationSuite) SetUpTest(c *check.C) {
 			Proxy:  true,
 			Scheme: "http",
 		},
+		"*": {
+			Scheme: "https",
+		},
 	}
 
 	c.Assert(s.testServer.Start(), check.IsNil)
@@ -467,6 +470,10 @@ func (s *FederationSuite) TestGetRemoteCollectionByPDH(c *check.C) {
 func (s *FederationSuite) TestGetCollectionByPDHError(c *check.C) {
 	defer s.localServiceReturns404(c).Close()
 
+	// zmock's normal response (200 with an empty body) would
+	// change the outcome from 404 to 502
+	delete(s.testHandler.Cluster.RemoteClusters, "zmock")
+
 	req := httptest.NewRequest("GET", "/arvados/v1/collections/99999999999999999999999999999999+99", nil)
 	req.Header.Set("Authorization", "Bearer "+arvadostest.ActiveToken)
 
@@ -479,6 +486,10 @@ func (s *FederationSuite) TestGetCollectionByPDHError(c *check.C) {
 func (s *FederationSuite) TestGetCollectionByPDHErrorBadHash(c *check.C) {
 	defer s.localServiceReturns404(c).Close()
 
+	// zmock's normal response (200 with an empty body) would
+	// change the outcome
+	delete(s.testHandler.Cluster.RemoteClusters, "zmock")
+
 	srv2 := &httpserver.Server{
 		Server: http.Server{
 			Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
@@ -512,7 +523,7 @@ func (s *FederationSuite) TestGetCollectionByPDHErrorBadHash(c *check.C) {
 	resp := s.testRequest(req).Result()
 	defer resp.Body.Close()
 
-	c.Check(resp.StatusCode, check.Equals, http.StatusNotFound)
+	c.Check(resp.StatusCode, check.Equals, http.StatusBadGateway)
 }
 
 func (s *FederationSuite) TestSaltedTokenGetCollectionByPDH(c *check.C) {
@@ -534,6 +545,10 @@ func (s *FederationSuite) TestSaltedTokenGetCollectionByPDH(c *check.C) {
 func (s *FederationSuite) TestSaltedTokenGetCollectionByPDHError(c *check.C) {
 	arvadostest.SetServiceURL(&s.testHandler.Cluster.Services.RailsAPI, "https://"+os.Getenv("ARVADOS_TEST_API_HOST"))
 
+	// zmock's normal response (200 with an empty body) would
+	// change the outcome
+	delete(s.testHandler.Cluster.RemoteClusters, "zmock")
+
 	req := httptest.NewRequest("GET", "/arvados/v1/collections/99999999999999999999999999999999+99", nil)
 	req.Header.Set("Authorization", "Bearer v2/zzzzz-gj3su-077z32aux8dg2s1/282d7d172b6cfdce364c5ed12ddf7417b2d00065")
 	resp := s.testRequest(req).Result()
-- 
2.30.2