Address comments: update comments, new unit tests and var renames

Signed-off-by: Prasad Borole <[email protected]>

cmd/spire-agent/cli/api/common.go

@@ -13,6 +13,8 @@ import (
 	"google.golang.org/grpc/metadata"
 )
 
+const commandTimeout = 5 * time.Second
+
 type workloadClient struct {
 	workload.SpiffeWorkloadAPIClient
 	timeout time.Duration
@@ -71,7 +73,7 @@ func adaptCommand(env *cli.Env, clientsMaker workloadClientMaker, cmd command) *
 		clientsMaker: clientsMaker,
 		cmd:          cmd,
 		env:          env,
-		timeout:      cli.DurationFlag(2 * time.Second),
+		timeout:      cli.DurationFlag(commandTimeout),
 	}
 
 	fs := flag.NewFlagSet(cmd.name(), flag.ContinueOnError)

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

@@ -394,15 +394,20 @@ func NewAgentConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool)
 		}
 	}
 
-	if c.Agent.Experimental.MaxSvidCacheSize != 0 {
-		ac.MaxSvidCacheSize = c.Agent.Experimental.MaxSvidCacheSize
+	if c.Agent.Experimental.MaxSvidCacheSize < 0 {
+		return nil, fmt.Errorf("max_svid_cache_size should not be negative")
 	}
+	ac.MaxSvidCacheSize = c.Agent.Experimental.MaxSvidCacheSize
+
 	if c.Agent.Experimental.SVIDCacheExpiryPeriod != "" {
 		var err error
 		ac.SVIDCacheExpiryPeriod, err = time.ParseDuration(c.Agent.Experimental.SVIDCacheExpiryPeriod)
 		if err != nil {
 			return nil, fmt.Errorf("could not parse svid cache expiry interval: %w", err)
 		}
+		if ac.SVIDCacheExpiryPeriod < 0 {
+			return nil, fmt.Errorf("svid_cache_expiry_interval should not be negative")
+		}
 	}
 
 	serverHostPort := net.JoinHostPort(c.Agent.ServerAddress, strconv.Itoa(c.Agent.ServerPort))

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

@@ -708,6 +708,16 @@ func TestNewAgentConfig(t *testing.T) {
 				require.EqualValues(t, 1050000000, c.SVIDCacheExpiryPeriod)
 			},
 		},
+		{
+			msg:         "svid_cache_expiry_interval is negative",
+			expectError: true,
+			input: func(c *Config) {
+				c.Agent.Experimental.SVIDCacheExpiryPeriod = "-1s50ms"
+			},
+			test: func(t *testing.T, c *agent.Config) {
+				require.Nil(t, c)
+			},
+		},
 		{
 			msg:         "invalid svid_cache_expiry_interval returns an error",
 			expectError: true,
@@ -743,6 +753,25 @@ func TestNewAgentConfig(t *testing.T) {
 				require.EqualValues(t, 0, c.MaxSvidCacheSize)
 			},
 		},
+		{
+			msg: "max_svid_cache_size is zero",
+			input: func(c *Config) {
+				c.Agent.Experimental.MaxSvidCacheSize = 0
+			},
+			test: func(t *testing.T, c *agent.Config) {
+				require.EqualValues(t, 0, c.MaxSvidCacheSize)
+			},
+		},
+		{
+			msg:         "max_svid_cache_size is negative",
+			expectError: true,
+			input: func(c *Config) {
+				c.Agent.Experimental.MaxSvidCacheSize = -10
+			},
+			test: func(t *testing.T, c *agent.Config) {
+				require.Nil(t, c)
+			},
+		},
 		{
 			msg: "allowed_foreign_jwt_claims provided",
 			input: func(c *Config) {

examples/load-gen.sh

@@ -0,0 +1,25 @@
+#!/bin/bash
+
+set -e
+
+SERVER=./bin/spire-server
+i=$1
+UNTIL=$(expr $1 + $2)
+#sleep 30
+
+while [ $i -le $UNTIL ]
+do
+   $SERVER entry create \
+  -parentID spiffe://example.org/host \
+  -spiffeID "spiffe://example.org/workload/$i" \
+  -selector "unix:uid:1001" &
+  ((i++))
+done
+
+#var="$($SERVER entry show -parentID spiffe://example.org/host | grep Entry | cut -d ":" -f2 | sed 's/^ *//')"
+#for val in $var; do
+#    $SERVER entry delete -entryID "$val"
+#done
+
+#  -selector "unix:uid:$(id -u)"
+#  -selector "unix:user:testing" &

pkg/agent/api/delegatedidentity/v1/service.go

@@ -83,9 +83,9 @@ func (s *Service) isCallerAuthorized(ctx context.Context, log logrus.FieldLogger
 	}
 
 	entries := s.manager.MatchingRegistrationEntries(callerSelectors)
-	numRegisteredIDs := len(entries)
+	numRegisteredEntries := len(entries)
 
-	if numRegisteredIDs == 0 {
+	if numRegisteredEntries == 0 {
 		log.Error("no identity issued")
 		return nil, status.Error(codes.PermissionDenied, "no identity issued")
 	}
@@ -98,8 +98,8 @@ func (s *Service) isCallerAuthorized(ctx context.Context, log logrus.FieldLogger
 
 	// caller has identity associeted with but none is authorized
 	log.WithFields(logrus.Fields{
-		"num_registered_ids": numRegisteredIDs,
-		"default_id":         entries[0].SpiffeId,
+		"num_registered_entries": numRegisteredEntries,
+		"default_id":             entries[0].SpiffeId,
 	}).Error("Permission denied; caller not configured as an authorized delegate.")
 
 	return nil, status.Error(codes.PermissionDenied, "caller not configured as an authorized delegate")

pkg/agent/manager/cache/cache.go

@@ -82,20 +82,20 @@ type X509SVID struct {
 // workloads) and registration entries that have that selector.
 //
 // The LRU-like SVID cache has configurable size limit and expiry period.
-// 1. Size limit of SVID cache is a soft limit which means if SVID has a subscriber present then
+// 1. Size limit of SVID cache is a soft limit. If SVID has a subscriber present then
 //    that SVID is never removed from cache.
 // 2. Least recently used SVIDs are removed from cache only after the cache expiry period has passed.
 //    This is done to reduce the overall cache churn.
 // 3. Last access timestamp for SVID cache entry is updated when a new subscriber is created
-// 4. When a new subscriber is created and if there is a cache miss
+// 4. When a new subscriber is created and there is a cache miss
 //    then subscriber needs to wait for next SVID sync event to receive WorkloadUpdate with newly minted SVID
 //
 // The advantage of above approach is that if agent has entry count less than cache size
 // then all SVIDs are cached at all times. If agent has entry count greater than cache size then
 // subscribers will continue to get SVID updates (potential delay for first WorkloadUpdate if cache miss)
 // and least used SVIDs will be removed from cache which will save memory usage.
-// It will allow agent to support large number of registrations.
-//
+// This allows agent to support environments where the active simultaneous workload count
+// is a small percentage of the large number of registrations assigned to the agent.
 //
 // When registration entries are added/updated/removed, the set of relevant
 // selectors are gathered and the indexes for those selectors are combed for
@@ -162,11 +162,11 @@ type StaleEntry struct {
 
 func New(log logrus.FieldLogger, trustDomain spiffeid.TrustDomain, bundle *Bundle, metrics telemetry.Metrics,
 	maxSvidCacheSize int, svidCacheExpiryPeriod time.Duration, clk clock.Clock) *Cache {
-	if maxSvidCacheSize == 0 {
+	if maxSvidCacheSize <= 0 {
 		maxSvidCacheSize = DefaultMaxSvidCacheSize
 	}
 
-	if svidCacheExpiryPeriod == 0 {
+	if svidCacheExpiryPeriod <= 0 {
 		svidCacheExpiryPeriod = DefaultSVIDCacheExpiryPeriod
 	}
 
@@ -602,6 +602,11 @@ func (c *Cache) syncSVIDs() (map[string]struct{}, []record) {
 	lastAccessTimestamps := make([]record, len(c.records))
 
 	i := 0
+	// iterate over all selectors from cached entries and obtain:
+	// 1. entries that have active subscribers
+	//   1.1 if those entries don't have corresponding SVID cached then put them in staleEntries
+	//       so that SVID will be cached in next sync
+	// 2. get lastAccessTimestamp of each entry
 	for id, record := range c.records {
 		for _, sel := range record.entry.Selectors {
 			if index, ok := c.selectors[makeSelector(sel)]; ok && index != nil {