From a1fd7cb6b44cf98c146c16f05428a0e820fbe33a Mon Sep 17 00:00:00 2001
From: Egon Elbre <egonelbre@gmail.com>
Date: Fri, 12 Apr 2019 10:28:27 -0400
Subject: [PATCH] don't cache failed satellite certificate fetch attempt
 (#1745)

* don't cache failing error

* fix test

* fix linter errors
---
 storagenode/piecestore/verification.go |  6 ++++
 storagenode/trust/service.go           | 19 +++++++---
 storagenode/trust/service_test.go      | 50 +++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/storagenode/piecestore/verification.go b/storagenode/piecestore/verification.go
index 8f18067eb..138499574 100644
--- a/storagenode/piecestore/verification.go
+++ b/storagenode/piecestore/verification.go
@@ -66,6 +66,9 @@ func (endpoint *Endpoint) VerifyOrderLimit(ctx context.Context, limit *pb.OrderL
 	}
 
 	if err := endpoint.VerifyOrderLimitSignature(ctx, limit); err != nil {
+		if err == context.Canceled {
+			return err
+		}
 		return ErrVerifyUntrusted.Wrap(err)
 	}
 
@@ -124,6 +127,9 @@ func (endpoint *Endpoint) VerifyPieceHash(ctx context.Context, peer *identity.Pe
 func (endpoint *Endpoint) VerifyOrderLimitSignature(ctx context.Context, limit *pb.OrderLimit2) error {
 	signee, err := endpoint.trust.GetSignee(ctx, limit.SatelliteId)
 	if err != nil {
+		if err == context.Canceled {
+			return err
+		}
 		return ErrVerifyUntrusted.New("unable to get signee: %v", err) // TODO: report grpc status bad message
 	}
 
diff --git a/storagenode/trust/service.go b/storagenode/trust/service.go
index 35a87864b..5711e8488 100644
--- a/storagenode/trust/service.go
+++ b/storagenode/trust/service.go
@@ -9,12 +9,17 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/zeebo/errs"
+
 	"storj.io/storj/pkg/auth/signing"
 	"storj.io/storj/pkg/identity"
 	"storj.io/storj/pkg/kademlia"
 	"storj.io/storj/pkg/storj"
 )
 
+// Error is the default error class
+var Error = errs.Class("trust:")
+
 // Pool implements different peer verifications.
 type Pool struct {
 	kademlia *kademlia.Kademlia
@@ -29,7 +34,6 @@ type Pool struct {
 type satelliteInfoCache struct {
 	once     sync.Once
 	identity *identity.PeerIdentity
-	err      error
 }
 
 // NewPool creates a new trust pool using kademlia to find certificates and with the specified list of trusted satellites.
@@ -119,12 +123,17 @@ func (pool *Pool) GetSignee(ctx context.Context, id storj.NodeID) (signing.Signe
 		}
 	}
 
+	identity, err := pool.kademlia.FetchPeerIdentity(nestedContext, id)
+	if err != nil {
+		if err == context.Canceled {
+			return nil, err
+		}
+		return nil, Error.Wrap(err)
+	}
+
 	info.once.Do(func() {
-		info.identity, info.err = pool.kademlia.FetchPeerIdentity(nestedContext, id)
+		info.identity = identity
 	})
 
-	if info.err != nil {
-		return nil, info.err
-	}
 	return signing.SigneeFromPeerIdentity(info.identity), nil
 }
diff --git a/storagenode/trust/service_test.go b/storagenode/trust/service_test.go
index dd792a384..828d9793b 100644
--- a/storagenode/trust/service_test.go
+++ b/storagenode/trust/service_test.go
@@ -5,25 +5,57 @@ package trust_test
 
 import (
 	"context"
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sync/errgroup"
+
 	"storj.io/storj/internal/testcontext"
 	"storj.io/storj/internal/testplanet"
 )
 
 func TestGetSignee(t *testing.T) {
-	testplanet.Run(t, testplanet.Config{
-		SatelliteCount: 1, StorageNodeCount: 1,
-	}, func(t *testing.T, ctx *testcontext.Context, planet *testplanet.Planet) {
-		trust := planet.StorageNodes[0].Storage2.Trust
+	ctx := testcontext.New(t)
+	defer ctx.Cleanup()
 
-		canceledContext, cancel := context.WithCancel(ctx)
-		cancel()
+	planet, err := testplanet.New(t, 1, 1, 0)
+	require.NoError(t, err)
+	defer ctx.Check(planet.Shutdown)
 
-		// GetSignee should succeed even on canceled context to avoid miscaching
+	planet.Start(ctx)
+
+	trust := planet.StorageNodes[0].Storage2.Trust
+
+	canceledContext, cancel := context.WithCancel(ctx)
+	cancel()
+
+	var group errgroup.Group
+	group.Go(func() error {
 		cert, err := trust.GetSignee(canceledContext, planet.Satellites[0].ID())
-		assert.NoError(t, err)
-		assert.NotNil(t, cert)
+		if err != context.Canceled {
+			return nil
+		}
+		if err != nil {
+			return err
+		}
+		if cert != nil {
+			return errors.New("got certificate")
+		}
+		return nil
 	})
+
+	group.Go(func() error {
+		cert, err := trust.GetSignee(ctx, planet.Satellites[0].ID())
+		if err != nil {
+			return err
+		}
+		if cert == nil {
+			return errors.New("didn't get certificate")
+		}
+		return nil
+	})
+
+	assert.NoError(t, group.Wait())
 }