small general cleanup

- stop workload x509 handler when stream done listening to prevent a
goroutine leak
- replace a few tickers used as timers with timers
- wired up Agent test suite and gave it an empty test to exercise at
least Setup and TearDown
- removed dead code
- increase idutil verification resilience if a new id type is added but
not fully implemented

replace workload ticker w/ timer

Signed-off-by: Andrew Harding <azdagron@gmail.com>

api/workload/backoff.go

@@ -13,8 +13,6 @@ type backoff struct {
 	mtx     *sync.Mutex
 	current time.Duration
 	timeout time.Duration
-
-	lastErr error
 }
 
 // newBackoff creates a new backoff struct with the requested timeout applied.
@@ -26,10 +24,10 @@ func newBackoff(timeout time.Duration) *backoff {
 	}
 }
 
-// ticker returns a tick channel configured for with the current backoff
+// timer returns a timer configured for with the current backoff
 // delay. Consumers can use this to wait for the appropriate period of time.
-func (b *backoff) ticker() <-chan time.Time {
-	return time.NewTicker(b.next()).C
+func (b *backoff) timer() *time.Timer {
+	return time.NewTimer(b.next())
 }
 
 // expired returns true if the backoff timer has exceeded the timeout value.

api/workload/backoff_test.go

@@ -49,9 +49,9 @@ func TestBackoff_Ticker(t *testing.T) {
 
 	bo.current = 1 * time.Millisecond
 	select {
-	case <-time.NewTicker(5 * time.Millisecond).C:
-		t.Errorf("ticker did not fire in time")
-	case <-bo.ticker():
+	case <-time.NewTimer(5 * time.Millisecond).C:
+		t.Errorf("timer did not fire in time")
+	case <-bo.timer().C:
 		break
 	}
 }

api/workload/x509_stream.go

@@ -42,6 +42,7 @@ func (x *x509Stream) listen() error {
 	bo := newBackoff(x.c.Timeout)
 
 	x.handler.start()
+	defer x.handler.stop()
 
 	for {
 		x.wlClient, err = x.newClient()
@@ -209,8 +210,11 @@ func (x *x509Stream) goAgain(bo *backoff) bool {
 		return false
 	}
 
+	timer := bo.timer()
+	defer timer.Stop()
+
 	select {
-	case <-bo.ticker():
+	case <-timer.C:
 		return true
 	case <-x.stopChan:
 		return false

cmd/spire-agent/cli/run/run.go

@@ -337,11 +337,3 @@ func parseTrustBundle(path string) ([]*x509.Certificate, error) {
 
 	return bundle, nil
 }
-
-func stringDefault(option string, defaultValue string) string {
-	if option == "" {
-		return defaultValue
-	}
-
-	return option
-}

functional/tools/workload/workload.go

@@ -13,8 +13,6 @@ import (
 	workload "github.com/spiffe/spire/proto/api/workload"
 )
 
-const cacheBusyRetrySeconds = 10
-
 // Workload is the component that consumes Workload API and renews certs
 type Workload struct {
 	workloadClient        workload.SpiffeWorkloadAPIClient
@@ -40,6 +38,7 @@ func (w *Workload) RunDaemon(ctx context.Context) error {
 
 	// Create timer for timeout
 	timeoutTimer := time.NewTimer(time.Second * time.Duration(w.timeout))
+	defer timeoutTimer.Stop()
 
 	stream, err := w.workloadClient.FetchX509SVID(ctx, &workload.X509SVIDRequest{})
 	if err != nil {

pkg/agent/agent_test.go

@@ -5,18 +5,20 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"testing"
 
 	"github.com/golang/mock/gomock"
 	"github.com/sirupsen/logrus/hooks/test"
-	"github.com/spiffe/spire/proto/common"
 	"github.com/spiffe/spire/test/mock/agent/catalog"
 	"github.com/spiffe/spire/test/mock/agent/manager"
 	"github.com/spiffe/spire/test/mock/proto/agent/keymanager"
 	"github.com/spiffe/spire/test/mock/proto/agent/nodeattestor"
 	"github.com/stretchr/testify/suite"
 )
 
-type selectors []*common.Selector
+func TestAgent(t *testing.T) {
+	suite.Run(t, new(AgentTestSuite))
+}
 
 type AgentTestSuite struct {
 	suite.Suite
@@ -40,7 +42,7 @@ func (s *AgentTestSuite) SetupTest() {
 
 	addr := &net.UnixAddr{Name: "./spire_api", Net: "unix"}
 	log, _ := test.NewNullLogger()
-	tempDir, err := ioutil.TempDir(os.TempDir(), "spire-test")
+	tempDir, err := ioutil.TempDir("", "spire-test")
 	s.Require().NoError(err)
 
 	config := &Config{
@@ -60,3 +62,7 @@ func (s *AgentTestSuite) TearDownTest() {
 	os.RemoveAll(s.agent.c.DataDir)
 	s.ctrl.Finish()
 }
+
+func (s *AgentTestSuite) TestSomething() {
+	// TODO: add meaningful test here.
+}

pkg/agent/attestor/node/node_test.go

@@ -44,7 +44,6 @@ type NodeAttestorTestSuite struct {
 	keyManager   *mock_keymanager.MockKeyManager
 	nodeClient   *mock_node.MockNodeClient
 	config       *Config
-	expectation  *node.X509SVIDUpdate
 }
 
 func (s *NodeAttestorTestSuite) SetupTest() {

pkg/agent/attestor/workload/workload.go

@@ -38,7 +38,6 @@ const (
 	workloadApi    = "workload_api"
 	workloadPid    = "workload_pid"
 	workloadAttDur = "workload_attestation_duration"
-	unknownName    = "unknown"
 )
 
 // Attest invokes all workload attestor plugins against the provided PID. If an error

pkg/agent/attestor/workload/workload_test.go

@@ -12,7 +12,6 @@ import (
 	"github.com/spiffe/spire/pkg/common/selector"
 	"github.com/spiffe/spire/pkg/common/telemetry"
 	"github.com/spiffe/spire/proto/agent/workloadattestor"
-	"github.com/spiffe/spire/proto/api/node"
 	"github.com/spiffe/spire/proto/common"
 	"github.com/spiffe/spire/test/fakes/fakeagentcatalog"
 	"github.com/spiffe/spire/test/mock/proto/agent/workloadattestor"
@@ -32,10 +31,9 @@ type WorkloadAttestorTestSuite struct {
 
 	ctrl *gomock.Controller
 
-	attestor    *attestor
-	expectation *node.X509SVIDUpdate
-	attestor1   *mock_workloadattestor.MockWorkloadAttestor
-	attestor2   *mock_workloadattestor.MockWorkloadAttestor
+	attestor  *attestor
+	attestor1 *mock_workloadattestor.MockWorkloadAttestor
+	attestor2 *mock_workloadattestor.MockWorkloadAttestor
 }
 
 func (s *WorkloadAttestorTestSuite) SetupTest() {

pkg/agent/manager/manager.go

@@ -57,9 +57,8 @@ type manager struct {
 	cache cache.Cache
 	svid  svid.Rotator
 
-	spiffeID       string
-	serverSPIFFEID string
-	serverAddr     net.Addr
+	spiffeID   string
+	serverAddr net.Addr
 
 	svidCachePath   string
 	bundleCachePath string

pkg/agent/manager/manager_test.go

@@ -772,7 +772,7 @@ type mockNodeAPIHandlerConfig struct {
 
 	// Callbacks used to build the response according to the request and state of mockNodeAPIHandler.
 	fetchX509SVID func(*mockNodeAPIHandler, *node.FetchX509SVIDRequest, node.Node_FetchX509SVIDServer) error
-	fetchJWTSVID func(*mockNodeAPIHandler, *node.FetchJWTSVIDRequest) (*node.FetchJWTSVIDResponse, error)
+	fetchJWTSVID  func(*mockNodeAPIHandler, *node.FetchJWTSVIDRequest) (*node.FetchJWTSVIDResponse, error)
 
 	svidTTL int
 }
@@ -794,8 +794,6 @@ type mockNodeAPIHandler struct {
 
 	// Counts the number of requests received from clients
 	reqCount int
-
-	delay time.Duration
 }
 
 func newMockNodeAPIHandler(config *mockNodeAPIHandlerConfig) *mockNodeAPIHandler {

pkg/common/idutil/spiffeid.go

@@ -90,6 +90,7 @@ func ValidateSpiffeIDURL(id *url.URL, mode ValidationMode) error {
 
 	// id type validation
 	switch options.idType {
+	case anyId:
 	case trustDomainId:
 		if id.Path != "" {
 			return validationError("path is not empty")
@@ -115,6 +116,8 @@ func ValidateSpiffeIDURL(id *url.URL, mode ValidationMode) error {
 		if !isAgentPath(id.Path) {
 			return validationError(`invalid path: expecting "/spire/agent/*"`)
 		}
+	default:
+		return validationError("internal error: unhandled id type %v", options.idType)
 	}
 
 	return nil

pkg/server/endpoints/endpoints_test.go

@@ -123,7 +123,7 @@ func (s *EndpointsTestSuite) TestGRPCHook() {
 
 	select {
 	case <-snitchChan:
-	case <-time.NewTicker(5 * time.Second).C:
+	case <-time.NewTimer(5 * time.Second).C:
 		s.T().Error("grpc hook did not fire")
 	}
 

pkg/server/endpoints/node/handler_test.go

@@ -43,7 +43,6 @@ var (
 
 type HandlerTestSuite struct {
 	suite.Suite
-	t                *testing.T
 	ctrl             *gomock.Controller
 	handler          *Handler
 	mockDataStore    *mock_datastore.MockDataStore

pkg/server/endpoints/registration/handler_test.go

@@ -19,7 +19,6 @@ import (
 
 type handlerTestSuite struct {
 	suite.Suite
-	t             *testing.T
 	ctrl          *gomock.Controller
 	handler       *Handler
 	mockDataStore *mock_datastore.MockDataStore

pkg/server/plugin/datastore/sql/sql.go

@@ -35,10 +35,6 @@ type configuration struct {
 	ConnectionString string `hcl:"connection_string" json:"connection_string"`
 }
 
-type database interface {
-	connect(string) (*gorm.DB, error)
-}
-
 type sqlPlugin struct {
 	db *gorm.DB
 
@@ -301,10 +297,10 @@ func (ds *sqlPlugin) CreateAttestedNodeEntry(ctx context.Context,
 
 	return &datastore.CreateAttestedNodeEntryResponse{
 		AttestedNodeEntry: &datastore.AttestedNodeEntry{
-			BaseSpiffeId:       model.SpiffeID,
-			AttestationDataType:   model.DataType,
-			CertSerialNumber:   model.SerialNumber,
-			CertExpirationDate: expiresAt.Format(datastore.TimeFormat),
+			BaseSpiffeId:        model.SpiffeID,
+			AttestationDataType: model.DataType,
+			CertSerialNumber:    model.SerialNumber,
+			CertExpirationDate:  expiresAt.Format(datastore.TimeFormat),
 		},
 	}, nil
 }
@@ -325,10 +321,10 @@ func (ds *sqlPlugin) FetchAttestedNodeEntry(ctx context.Context,
 	}
 	return &datastore.FetchAttestedNodeEntryResponse{
 		AttestedNodeEntry: &datastore.AttestedNodeEntry{
-			BaseSpiffeId:       model.SpiffeID,
-			AttestationDataType:   model.DataType,
-			CertSerialNumber:   model.SerialNumber,
-			CertExpirationDate: model.ExpiresAt.Format(datastore.TimeFormat),
+			BaseSpiffeId:        model.SpiffeID,
+			AttestationDataType: model.DataType,
+			CertSerialNumber:    model.SerialNumber,
+			CertExpirationDate:  model.ExpiresAt.Format(datastore.TimeFormat),
 		},
 	}, nil
 }
@@ -350,10 +346,10 @@ func (ds *sqlPlugin) FetchStaleNodeEntries(ctx context.Context,
 
 	for _, model := range models {
 		resp.AttestedNodeEntryList = append(resp.AttestedNodeEntryList, &datastore.AttestedNodeEntry{
-			BaseSpiffeId:       model.SpiffeID,
-			AttestationDataType:   model.DataType,
-			CertSerialNumber:   model.SerialNumber,
-			CertExpirationDate: model.ExpiresAt.Format(datastore.TimeFormat),
+			BaseSpiffeId:        model.SpiffeID,
+			AttestationDataType: model.DataType,
+			CertSerialNumber:    model.SerialNumber,
+			CertExpirationDate:  model.ExpiresAt.Format(datastore.TimeFormat),
 		})
 	}
 	return resp, nil
@@ -391,10 +387,10 @@ func (ds *sqlPlugin) UpdateAttestedNodeEntry(ctx context.Context,
 
 	return &datastore.UpdateAttestedNodeEntryResponse{
 		AttestedNodeEntry: &datastore.AttestedNodeEntry{
-			BaseSpiffeId:       model.SpiffeID,
-			AttestationDataType:   model.DataType,
-			CertSerialNumber:   model.SerialNumber,
-			CertExpirationDate: model.ExpiresAt.Format(datastore.TimeFormat),
+			BaseSpiffeId:        model.SpiffeID,
+			AttestationDataType: model.DataType,
+			CertSerialNumber:    model.SerialNumber,
+			CertExpirationDate:  model.ExpiresAt.Format(datastore.TimeFormat),
 		},
 	}, db.Commit().Error
 }
@@ -421,10 +417,10 @@ func (ds *sqlPlugin) DeleteAttestedNodeEntry(ctx context.Context,
 
 	return &datastore.DeleteAttestedNodeEntryResponse{
 		AttestedNodeEntry: &datastore.AttestedNodeEntry{
-			BaseSpiffeId:       model.SpiffeID,
-			AttestationDataType:   model.DataType,
-			CertSerialNumber:   model.SerialNumber,
-			CertExpirationDate: model.ExpiresAt.Format(datastore.TimeFormat),
+			BaseSpiffeId:        model.SpiffeID,
+			AttestationDataType: model.DataType,
+			CertSerialNumber:    model.SerialNumber,
+			CertExpirationDate:  model.ExpiresAt.Format(datastore.TimeFormat),
 		},
 	}, db.Commit().Error
 }

pkg/server/plugin/datastore/sql/sql_test.go

@@ -20,7 +20,6 @@ var (
 	ctx = context.Background()
 )
 
-type selectors []*common.Selector
 type regEntries []*common.RegistrationEntry
 
 func TestInvalidPluginConfiguration(t *testing.T) {

pkg/server/plugin/nodeattestor/jointoken/join_token_test.go

@@ -16,8 +16,6 @@ import (
 
 const (
 	config = `{"join_tokens":{"foo":600,"bar":1}, "trust_domain":"example.com"}`
-
-	spiffeId = "spiffe://example.com/spiffe/node-id/foobar"
 )
 
 var (

pkg/server/plugin/upstreamca/disk/disk.go

@@ -18,16 +18,6 @@ import (
 	"github.com/spiffe/spire/proto/server/upstreamca"
 )
 
-var (
-	pluginInfo = spi.GetPluginInfoResponse{
-		Description: "",
-		DateCreated: "",
-		Version:     "",
-		Author:      "",
-		Company:     "",
-	}
-)
-
 type Configuration struct {
 	TTL          string `hcl:"ttl" json:"ttl"` // time to live for generated certs
 	TrustDomain  string `hcl:"trust_domain" json:"trust_domain"`
@@ -40,7 +30,6 @@ type diskPlugin struct {
 
 	mtx        sync.RWMutex
 	cert       *x509.Certificate
-	keypair    *x509util.MemoryKeypair
 	upstreamCA *x509svid.UpstreamCA
 }