Merge branch '15453-default-is-not-a-remote'

fixes #15453

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/lib/controller/fed_collections.go b/lib/controller/fed_collections.go
index 07daf2f90ef28b3199e856c93134aa5b6975fab3..b7b1e64483f926c2da706541f233482fafeae384 100644 (file)
--- 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 63e801b22c7a2bebd5a6a35fdefa4835c1775c1d..3bcafacd2c8bc47bdce87bb6908169c710662c6d 100644 (file)
--- 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 169b1f79614bd9b0391542ae71771dac87e0d95d..c654277fea2dff8616b948eda8ce8dbeb76c75c0 100644 (file)
--- 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()