16133: Don't take multiple federation hops.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 2aebc0e9707496807f96fa809fc24e111e5950e2..42083cb83df10918cdbc9f14869d0c2e49e51442 100644 (file)
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -120,8 +120,13 @@ func (conn *Conn) chooseBackend(id string) backend {
 // or "" for the local backend.
 //
 // A non-nil error means all backends failed.
-func (conn *Conn) tryLocalThenRemotes(ctx context.Context, fn func(context.Context, string, backend) error) error {
-       if err := fn(ctx, "", conn.local); err == nil || errStatus(err) != http.StatusNotFound {
+func (conn *Conn) tryLocalThenRemotes(ctx context.Context, forwardedFor string, fn func(context.Context, string, backend) error) error {
+       if err := fn(ctx, "", conn.local); err == nil || errStatus(err) != http.StatusNotFound || forwardedFor != "" {
+               // Note: forwardedFor != "" means this request came
+               // from a remote cluster, so we don't take a second
+               // hop. This avoids cycles, redundant calls to a
+               // mutually reachable remote, and use of double-salted
+               // tokens.
                return err
        }
 
@@ -218,8 +223,6 @@ func (conn *Conn) Login(ctx context.Context, options arvados.LoginOptions) (arva
 }
 
 func (conn *Conn) CollectionGet(ctx context.Context, options arvados.GetOptions) (arvados.Collection, error) {
-       downstream := options.ForwardedFor
-       options.ForwardedFor = conn.cluster.ClusterID + "-" + downstream
        if len(options.UUID) == 27 {
                // UUID is really a UUID
                c, err := conn.chooseBackend(options.UUID).CollectionGet(ctx, options)
@@ -230,17 +233,10 @@ func (conn *Conn) CollectionGet(ctx context.Context, options arvados.GetOptions)
        } else {
                // UUID is a PDH
                first := make(chan arvados.Collection, 1)
-               err := conn.tryLocalThenRemotes(ctx, func(ctx context.Context, remoteID string, be backend) error {
-                       if remoteID != "" && downstream != "" {
-                               // If remoteID isn't in downstream, we
-                               // might find the collection by taking
-                               // another hop, but we don't bother:
-                               // token salting and blob signature
-                               // rewriting don't work over multiple
-                               // hops.
-                               return notFoundError{}
-                       }
-                       c, err := be.CollectionGet(ctx, options)
+               err := conn.tryLocalThenRemotes(ctx, options.ForwardedFor, func(ctx context.Context, remoteID string, be backend) error {
+                       remoteOpts := options
+                       remoteOpts.ForwardedFor = conn.cluster.ClusterID + "-" + options.ForwardedFor
+                       c, err := be.CollectionGet(ctx, remoteOpts)
                        if err != nil {
                                return err
                        }