diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index 2316acab1a..fa34a8b271 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/agent/cache-types/connect_ca_leaf.go @@ -168,12 +168,18 @@ func (c *ConnectCALeaf) Fetch(opts cache.FetchOptions, req cache.Request) (cache // reached (on timeout ErrTimeout is returned on the channel). func (c *ConnectCALeaf) waitNewRootCA(datacenter string, ch chan<- error, timeout time.Duration) { + // We always want to block on at least an initial value. If this isn't + minIndex := atomic.LoadUint64(&c.caIndex) + if minIndex == 0 { + minIndex = 1 + } + // Fetch some new roots. This will block until our MinQueryIndex is // matched or the timeout is reached. rawRoots, err := c.Cache.Get(ConnectCARootName, &structs.DCSpecificRequest{ Datacenter: datacenter, QueryOptions: structs.QueryOptions{ - MinQueryIndex: atomic.LoadUint64(&c.caIndex), + MinQueryIndex: minIndex, MaxQueryTime: timeout, }, }) diff --git a/agent/cache/cache.go b/agent/cache/cache.go index 797f00697f..62a83dacde 100644 --- a/agent/cache/cache.go +++ b/agent/cache/cache.go @@ -326,6 +326,14 @@ func (c *Cache) fetch(t, key string, r Request, allowNew bool, attempt uint) (<- entry = cacheEntry{Valid: false, Waiter: make(chan struct{})} } + // We always specify an index greater than zero since index of zero + // means to always return immediately and we want to block if possible. + // Index 1 is always safe since Consul's own initialization always results + // in a higher index (around 10 or above). + if entry.Index == 0 { + entry.Index = 1 + } + // Set that we're fetching to true, which makes it so that future // identical calls to fetch will return the same waiter rather than // perform multiple fetches. diff --git a/agent/cache/cache_test.go b/agent/cache/cache_test.go index a1a3542607..0348ddc545 100644 --- a/agent/cache/cache_test.go +++ b/agent/cache/cache_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -413,6 +414,36 @@ func TestCacheGet_periodicRefreshErrorBackoff(t *testing.T) { require.True(t, actual < 10, fmt.Sprintf("actual: %d", actual)) } +// Test that fetching with no index actually sets to index to one, including +// for background refreshes. This ensures we don't end up with any index 0 +// loops. +func TestCacheGet_noIndexSetsOne(t *testing.T) { + t.Parallel() + + typ := TestType(t) + defer typ.AssertExpectations(t) + c := TestCache(t) + c.RegisterType("t", typ, &RegisterOptions{ + Refresh: true, + RefreshTimer: 0, + RefreshTimeout: 5 * time.Minute, + }) + + // Configure the type + fetchErr := fmt.Errorf("test fetch error") + typ.Static(FetchResult{Value: 42, Index: 0}, fetchErr).Run(func(args mock.Arguments) { + opts := args.Get(0).(FetchOptions) + assert.True(t, opts.MinIndex > 0, "minIndex > 0") + }) + + // Fetch + resultCh := TestCacheGetCh(t, c, "t", TestRequest(t, RequestInfo{Key: "hello"})) + TestCacheGetChResult(t, resultCh, 42) + + // Sleep a bit so background refresh happens + time.Sleep(100 * time.Millisecond) +} + // Test that the backend fetch sets the proper timeout. func TestCacheGet_fetchTimeout(t *testing.T) { t.Parallel()