Skip to content

Commit

Permalink
Variable renames and make lint
Browse files Browse the repository at this point in the history
  • Loading branch information
prasadborole1 committed Jul 12, 2022
1 parent 0ec17de commit e2b865b
Show file tree
Hide file tree
Showing 11 changed files with 54 additions and 62 deletions.
12 changes: 6 additions & 6 deletions cmd/spire-agent/cli/run/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ type experimentalConfig struct {
Flags fflag.RawConfig `hcl:"feature_flags"`

UnusedKeys []string `hcl:",unusedKeys"`
MaxSvidCacheSize int `hcl:"max_svid_cache_size"`
SVIDCacheExpiryPeriod string `hcl:"svid_cache_expiry_interval"`
SVIDCacheMaxSize int `hcl:"svid_cache_max_size"`
SVIDCacheExpiryPeriod string `hcl:"svid_cache_expiry_period"`
}

type Command struct {
Expand Down Expand Up @@ -394,10 +394,10 @@ func NewAgentConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool)
}
}

if c.Agent.Experimental.MaxSvidCacheSize < 0 {
return nil, fmt.Errorf("max_svid_cache_size should not be negative")
if c.Agent.Experimental.SVIDCacheMaxSize < 0 {
return nil, errors.New("svid_cache_max_size should not be negative")
}
ac.MaxSvidCacheSize = c.Agent.Experimental.MaxSvidCacheSize
ac.SVIDCacheMaxSize = c.Agent.Experimental.SVIDCacheMaxSize

if c.Agent.Experimental.SVIDCacheExpiryPeriod != "" {
var err error
Expand All @@ -406,7 +406,7 @@ func NewAgentConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool)
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")
return nil, errors.New("svid_cache_expiry_period should not be negative")
}
}

Expand Down
28 changes: 14 additions & 14 deletions cmd/spire-agent/cli/run/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ func TestNewAgentConfig(t *testing.T) {
},
},
{
msg: "svid_cache_expiry_interval parses a duration",
msg: "svid_cache_expiry_period parses a duration",
input: func(c *Config) {
c.Agent.Experimental.SVIDCacheExpiryPeriod = "1s50ms"
},
Expand All @@ -709,7 +709,7 @@ func TestNewAgentConfig(t *testing.T) {
},
},
{
msg: "svid_cache_expiry_interval is negative",
msg: "svid_cache_expiry_period is negative",
expectError: true,
input: func(c *Config) {
c.Agent.Experimental.SVIDCacheExpiryPeriod = "-1s50ms"
Expand All @@ -719,7 +719,7 @@ func TestNewAgentConfig(t *testing.T) {
},
},
{
msg: "invalid svid_cache_expiry_interval returns an error",
msg: "invalid svid_cache_expiry_period returns an error",
expectError: true,
input: func(c *Config) {
c.Agent.Experimental.SVIDCacheExpiryPeriod = "moo"
Expand All @@ -729,44 +729,44 @@ func TestNewAgentConfig(t *testing.T) {
},
},
{
msg: "svid_cache_expiry_interval is not set",
msg: "svid_cache_expiry_period is not set",
input: func(c *Config) {
},
test: func(t *testing.T, c *agent.Config) {
require.EqualValues(t, 0, c.SVIDCacheExpiryPeriod)
},
},
{
msg: "max_svid_cache_size is set",
msg: "svid_cache_max_size is set",
input: func(c *Config) {
c.Agent.Experimental.MaxSvidCacheSize = 100
c.Agent.Experimental.SVIDCacheMaxSize = 100
},
test: func(t *testing.T, c *agent.Config) {
require.EqualValues(t, 100, c.MaxSvidCacheSize)
require.EqualValues(t, 100, c.SVIDCacheMaxSize)
},
},
{
msg: "max_svid_cache_size is not set",
msg: "svid_cache_max_size is not set",
input: func(c *Config) {
},
test: func(t *testing.T, c *agent.Config) {
require.EqualValues(t, 0, c.MaxSvidCacheSize)
require.EqualValues(t, 0, c.SVIDCacheMaxSize)
},
},
{
msg: "max_svid_cache_size is zero",
msg: "svid_cache_max_size is zero",
input: func(c *Config) {
c.Agent.Experimental.MaxSvidCacheSize = 0
c.Agent.Experimental.SVIDCacheMaxSize = 0
},
test: func(t *testing.T, c *agent.Config) {
require.EqualValues(t, 0, c.MaxSvidCacheSize)
require.EqualValues(t, 0, c.SVIDCacheMaxSize)
},
},
{
msg: "max_svid_cache_size is negative",
msg: "svid_cache_max_size is negative",
expectError: true,
input: func(c *Config) {
c.Agent.Experimental.MaxSvidCacheSize = -10
c.Agent.Experimental.SVIDCacheMaxSize = -10
},
test: func(t *testing.T, c *agent.Config) {
require.Nil(t, c)
Expand Down
4 changes: 2 additions & 2 deletions pkg/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func (a *Agent) attest(ctx context.Context, cat catalog.Catalog, metrics telemet

func (a *Agent) newManager(ctx context.Context, cat catalog.Catalog, metrics telemetry.Metrics, as *node_attestor.AttestationResult, cache *storecache.Cache) (manager.Manager, error) {
config := &manager.Config{
SVID: as.SVID,
SVID: as.SVID,
SVIDKey: as.Key,
Bundle: as.Bundle,
Catalog: cat,
Expand All @@ -210,7 +210,7 @@ func (a *Agent) newManager(ctx context.Context, cat catalog.Catalog, metrics tel
BundleCachePath: a.bundleCachePath(),
SVIDCachePath: a.agentSVIDPath(),
SyncInterval: a.c.SyncInterval,
MaxSvidCacheSize: a.c.MaxSvidCacheSize,
MaxSvidCacheSize: a.c.SVIDCacheMaxSize,
SVIDCacheExpiryPeriod: a.c.SVIDCacheExpiryPeriod,
SVIDStoreCache: cache,
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/agent/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ type Config struct {
// SyncInterval controls how often the agent sync synchronizer waits
SyncInterval time.Duration

// MaxSvidCacheSize is a soft limit of max number of SVIDs that would be stored in cache
MaxSvidCacheSize int
// SVIDCacheMaxSize is a soft limit of max number of SVIDs that would be stored in cache
SVIDCacheMaxSize int

// SVIDCacheExpiryPeriod is a period after which svids that don't have subscribers will be removed from cache
// SVIDCacheExpiryPeriod is a period after which SVIDs that don't have subscribers will be removed from cache
SVIDCacheExpiryPeriod time.Duration

// Trust domain and associated CA bundle
Expand Down
25 changes: 13 additions & 12 deletions pkg/agent/manager/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
)

const (
DefaultMaxSvidCacheSize = 1000
DefaultSVIDCacheMaxSize = 1000
DefaultSVIDCacheExpiryPeriod = 1 * time.Hour
)

Expand Down Expand Up @@ -145,8 +145,8 @@ type Cache struct {
// svids are stored by entry IDs
svids map[string]*X509SVID

// maxSVIDCacheSize is a soft limit of max number of SVIDs that would be stored in cache
maxSvidCacheSize int
// svidCacheMaxSize is a soft limit of max number of SVIDs that would be stored in cache
svidCacheMaxSize int

// svidCacheExpiryPeriod is a period after which svids that don't have subscribers will be removed from cache
svidCacheExpiryPeriod time.Duration
Expand All @@ -161,9 +161,9 @@ 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 {
maxSvidCacheSize = DefaultMaxSvidCacheSize
svidCacheMaxSize int, svidCacheExpiryPeriod time.Duration, clk clock.Clock) *Cache {
if svidCacheMaxSize <= 0 {
svidCacheMaxSize = DefaultSVIDCacheMaxSize
}

if svidCacheExpiryPeriod <= 0 {
Expand All @@ -184,7 +184,7 @@ func New(log logrus.FieldLogger, trustDomain spiffeid.TrustDomain, bundle *Bundl
trustDomain: bundle,
},
svids: make(map[string]*X509SVID),
maxSvidCacheSize: maxSvidCacheSize,
svidCacheMaxSize: svidCacheMaxSize,
svidCacheExpiryPeriod: svidCacheExpiryPeriod,
clk: clk,
}
Expand Down Expand Up @@ -431,7 +431,7 @@ func (c *Cache) UpdateEntries(update *UpdateEntries, checkSVID func(*common.Regi

// entries with active subscribers which are not cached will be put in staleEntries map
activeSubs, recordsWithLastAccessTime := c.syncSVIDs()
extraSize := len(c.svids) - c.maxSvidCacheSize
extraSize := len(c.svids) - c.svidCacheMaxSize

// delete svids without subscribers and which have not been accessed since svidCacheExpiryTime
if extraSize > 0 {
Expand All @@ -441,7 +441,7 @@ func (c *Cache) UpdateEntries(update *UpdateEntries, checkSVID func(*common.Regi
svidCacheExpiryTime := now.Add(-1 * c.svidCacheExpiryPeriod).UnixMilli()
for _, record := range recordsWithLastAccessTime {
if extraSize <= 0 {
// no need to delete SVIDs any further as cache size <= maxSvidCacheSize
// no need to delete SVIDs any further as cache size <= svidCacheMaxSize
break
}
if _, ok := c.svids[record.id]; ok {
Expand Down Expand Up @@ -589,9 +589,10 @@ func (c *Cache) updateLastAccessTimestamp(selectors []*common.Selector) {
records, recordsDone := c.getRecordsForSelectors(set)
defer recordsDone()

now := c.clk.Now().UnixMilli()
for record := range records {
// Set lastAccessTimestamp so that svid LRU cache can be cleaned based on this timestamp
record.lastAccessTimestamp = c.clk.Now().UnixMilli()
record.lastAccessTimestamp = now
}
}

Expand Down Expand Up @@ -623,9 +624,9 @@ func (c *Cache) syncSVIDs() (map[string]struct{}, []record) {
i++
}

remainderSize := c.maxSvidCacheSize - len(c.svids)
remainderSize := c.svidCacheMaxSize - len(c.svids)
// add records which are not cached for remainder of cache size
for id, _ := range c.records {
for id := range c.records {
if len(c.staleEntries) >= remainderSize {
break
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/agent/manager/cache/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ func TestMaxSVIDCacheSize(t *testing.T) {
clk := clock.NewMock()
cache := newTestCacheWithConfig(10, 1*time.Minute, clk)

// create entries more than maxSvidCacheSize
// create entries more than svidCacheMaxSize
updateEntries := createUpdateEntries(12, makeBundles(bundleV1))
cache.UpdateEntries(updateEntries, nil)

Expand All @@ -732,7 +732,7 @@ func TestMaxSVIDCacheSize(t *testing.T) {
require.Len(t, cache.GetStaleEntries(), 0)
assert.Equal(t, 10, cache.CountSVIDs())

// Validate that active subscriber will still get SVID even if SVID count is at maxSvidCacheSize
// Validate that active subscriber will still get SVID even if SVID count is at svidCacheMaxSize
foo := makeRegistrationEntry("FOO", "foo")
updateEntries.RegistrationEntries[foo.EntryId] = foo

Expand Down Expand Up @@ -799,12 +799,12 @@ func TestNotify(t *testing.T) {
func TestNewCache(t *testing.T) {
// negative values
cache := newTestCacheWithConfig(-5, -5, clock.NewMock())
require.Equal(t, DefaultMaxSvidCacheSize, cache.maxSvidCacheSize)
require.Equal(t, DefaultSVIDCacheMaxSize, cache.svidCacheMaxSize)
require.Equal(t, DefaultSVIDCacheExpiryPeriod, cache.svidCacheExpiryPeriod)

// zero values
cache = newTestCacheWithConfig(0, 0, clock.NewMock())
require.Equal(t, DefaultMaxSvidCacheSize, cache.maxSvidCacheSize)
require.Equal(t, DefaultSVIDCacheMaxSize, cache.svidCacheMaxSize)
require.Equal(t, DefaultSVIDCacheExpiryPeriod, cache.svidCacheExpiryPeriod)
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/agent/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,8 @@ func (m *manager) SubscribeToCacheChanges(selectors cache.Selectors) cache.Subsc
if m.cache.Notify(selectors) {
return subscriber
}
select {
case <-m.clk.After(backoff.NextBackOff()):
}
// wait until next backoff interval
<-m.clk.After(backoff.NextBackOff())
}
}

Expand Down
18 changes: 5 additions & 13 deletions pkg/agent/manager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,7 @@ func TestSynchronizationClearsExpiredSVIDCache(t *testing.T) {

require.NoError(t, m.synchronize(context.Background()))

// Make sure svid count is MaxSvidCacheSize and remaining SVIDs are deleted from cache
// Make sure svid count is SVIDCacheMaxSize and remaining SVIDs are deleted from cache
require.Equal(t, 1, m.CountSVIDs())
}

Expand Down Expand Up @@ -947,12 +947,8 @@ func TestSubscribersWaitForSVID(t *testing.T) {
sub1 := m.SubscribeToCacheChanges(cache.Selectors{{Type: "unix", Value: "uid:1111"}})
defer sub1.Finish()
u := <-sub1.Updates()
if len(u.Identities) != 2 {
t.Fatalf("expected 2 SVIDs, got: %d", len(u.Identities))
}
if !u.Bundle.EqualTo(c.Bundle) {
t.Fatal("bundles were expected to be equal")
}
require.Len(t, u.Identities, 2)
require.Equal(t, c.Bundle, u.Bundle)
}()

wg.Add(1)
Expand All @@ -962,12 +958,8 @@ func TestSubscribersWaitForSVID(t *testing.T) {
cache.Selectors{{Type: "spiffe_id", Value: "spiffe://example.org/spire/agent/join_token/abcd"}})
defer sub2.Finish()
u := <-sub2.Updates()
if len(u.Identities) != 1 {
t.Fatalf("expected 1 SVID, got: %d", len(u.Identities))
}
if !u.Bundle.EqualTo(c.Bundle) {
t.Fatal("bundles were expected to be equal")
}
require.Len(t, u.Identities, 1)
require.Equal(t, c.Bundle, u.Bundle)
}()

wg.Wait()
Expand Down
2 changes: 1 addition & 1 deletion test/integration/setup/debugagent/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func run() error {
func agentEndpoints(ctx context.Context) error {
s, err := retrieveDebugPage(ctx)
if err == nil {
log.Printf("Debug info: %s", string(s))
log.Printf("Debug info: %s", s)
}
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions test/integration/suites/fetch-svids/07-fetch-svids
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ docker-compose exec -u 1001 -T spire-agent \
check-svid-count "spire-agent" 17

# Call agent debug endpoints and check if svid count is equal to 12
# 17(svid-count) - 5(svids from entries with uuid 1002 will be removed first after svid_cache_expiry_interval) = 12
# 17(svid-count) - 5(svids from entries with uuid 1002 will be removed first after svid_cache_expiry_period) = 12
check-svid-count "spire-agent" 12

# Call agent debug endpoints and check if svid count is equal to 8
# 12 - 8(cache size) = 4 extra svids from entries with uuid 1001 will be removed after svid_cache_expiry_interval
# 12 - 8(cache size) = 4 extra svids from entries with uuid 1001 will be removed after svid_cache_expiry_period
check-svid-count "spire-agent" $CACHESIZE
4 changes: 2 additions & 2 deletions test/integration/suites/fetch-svids/conf/agent/agent.conf
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ agent {
trust_domain = "domain.test"
admin_socket_path = "/opt/debug.sock"
experimental {
max_svid_cache_size = 8
svid_cache_expiry_interval = "30s"
svid_cache_max_size = 8
svid_cache_expiry_period = "30s"
}
}

Expand Down

0 comments on commit e2b865b

Please sign in to comment.