Disable background cache refresh for Connect Leaf Certs

The rationale behind removing them is that all of our own code (xDS, builtin connect proxy) use the cache notification mechanism. This ensures that the blocking fetch behind the scenes is always executing. Therefore the only way you might go to get a certificate and have to wait is when 1) the request has never been made for that cert before or 2) you are using the v1/agent/connect/ca/leaf API for retrieving the cert yourself.

In the first case, the refresh change doesn’t alter the behavior. In the second case, it can be mitigated by using blocking queries with that API which just like normal cache notification mechanism will cause the blocking fetch to be initiated and to get leaf certs as soon as needed.

If you are not using blocking queries, or Envoy/xDS, or the builtin connect proxy but are retrieving the certs yourself then the HTTP endpoint might take a little longer to respond.

This also renames the RefreshTimeout field on the register options to QueryTimeout to more accurately reflect that it is used for any type that supports blocking queries.

agent/agent_endpoint_test.go

@@ -5247,6 +5247,8 @@ func TestAgentConnectCALeafCert_good(t *testing.T) {
 	assert.Equal(fmt.Sprintf("%d", issued.ModifyIndex),
 		resp.Header().Get("X-Consul-Index"))
 
+	index := resp.Header().Get("X-Consul-Index")
+
 	// Test caching
 	{
 		// Fetch it again
@@ -5259,39 +5261,25 @@ func TestAgentConnectCALeafCert_good(t *testing.T) {
 		require.Equal("HIT", resp.Header().Get("X-Cache"))
 	}
 
-	// Test that caching is updated in the background
+	// Issue a blocking query to ensure that the cert gets updated appropriately
 	{
 		// Set a new CA
 		ca := connect.TestCAConfigSet(t, a, nil)
 
-		retry.Run(t, func(r *retry.R) {
-			resp := httptest.NewRecorder()
-			// Try and sign again (note no index/wait arg since cache should update in
-			// background even if we aren't actively blocking)
-			obj, err := a.srv.AgentConnectCALeafCert(resp, req)
-			r.Check(err)
+		resp := httptest.NewRecorder()
+		req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test?index="+index, nil)
+		obj, err := a.srv.AgentConnectCALeafCert(resp, req)
+		require.NoError(err)
+		issued2 := obj.(*structs.IssuedCert)
+		require.NotEqual(issued.CertPEM, issued2.CertPEM)
+		require.NotEqual(issued.PrivateKeyPEM, issued2.PrivateKeyPEM)
 
-			issued2 := obj.(*structs.IssuedCert)
-			if issued.CertPEM == issued2.CertPEM {
-				r.Fatalf("leaf has not updated")
-			}
+		// Verify that the cert is signed by the new CA
+		requireLeafValidUnderCA(t, issued2, ca)
 
-			// Got a new leaf. Sanity check it's a whole new key as well as different
-			// cert.
-			if issued.PrivateKeyPEM == issued2.PrivateKeyPEM {
-				r.Fatalf("new leaf has same private key as before")
-			}
-
-			// Verify that the cert is signed by the new CA
-			requireLeafValidUnderCA(t, issued2, ca)
-
-			// Should be a cache hit! The data should've updated in the cache
-			// in the background so this should've been fetched directly from
-			// the cache.
-			if resp.Header().Get("X-Cache") != "HIT" {
-				r.Fatalf("should be a cache hit")
-			}
-		})
+		// Should not be a cache hit! The data was updated in response to the blocking
+		// query being made.
+		require.Equal("MISS", resp.Header().Get("X-Cache"))
 	}
 }
 

agent/cache-types/connect_ca_leaf.go

@@ -50,7 +50,7 @@ const caChangeJitterWindow = 30 * time.Second
 // ConnectCALeaf supports fetching and generating Connect leaf
 // certificates.
 type ConnectCALeaf struct {
-	RegisterOptionsBlockingRefresh
+	RegisterOptionsBlockingNoRefresh
 	caIndex uint64 // Current index for CA roots
 
 	// rootWatchMu protects access to the rootWatchSubscribers map and

agent/cache-types/options.go

@@ -18,7 +18,17 @@ func (r RegisterOptionsBlockingRefresh) RegisterOptions() cache.RegisterOptions
 		Refresh:          true,
 		SupportsBlocking: true,
 		RefreshTimer:     0 * time.Second,
-		RefreshTimeout:   10 * time.Minute,
+		QueryTimeout:     10 * time.Minute,
+	}
+}
+
+type RegisterOptionsBlockingNoRefresh struct{}
+
+func (r RegisterOptionsBlockingNoRefresh) RegisterOptions() cache.RegisterOptions {
+	return cache.RegisterOptions{
+		Refresh:          false,
+		SupportsBlocking: true,
+		QueryTimeout:     10 * time.Minute,
 	}
 }
 

agent/cache/cache.go

@@ -172,7 +172,7 @@ type RegisterOptions struct {
 	// If this is zero, then data is refreshed immediately when a fetch
 	// is returned.
 	//
-	// Using different values for RefreshTimer and RefreshTimeout, various
+	// Using different values for RefreshTimer and QueryTimeout, various
 	// "refresh" mechanisms can be implemented:
 	//
 	//   * With a high timer duration and a low timeout, a timer-based
@@ -184,10 +184,10 @@ type RegisterOptions struct {
 	//
 	RefreshTimer time.Duration
 
-	// RefreshTimeout is the default value for the maximum query time for a fetch
+	// QueryTimeout is the default value for the maximum query time for a fetch
 	// operation. It is set as FetchOptions.Timeout so that cache.Type
 	// implementations can use it as the MaxQueryTime.
-	RefreshTimeout time.Duration
+	QueryTimeout time.Duration
 }
 
 // RegisterType registers a cacheable type.
@@ -475,7 +475,7 @@ func (c *Cache) fetch(key string, r getOptions, allowNew bool, attempt uint, ign
 		// keepalives are every 30 seconds so the RPC should fail if the packets are
 		// being blackholed for more than 30 seconds.
 		var connectedTimer *time.Timer
-		if tEntry.Opts.Refresh && entry.Index > 0 && tEntry.Opts.RefreshTimeout > 31*time.Second {
+		if tEntry.Opts.Refresh && entry.Index > 0 && tEntry.Opts.QueryTimeout > 31*time.Second {
 			connectedTimer = time.AfterFunc(31*time.Second, func() {
 				c.entriesLock.Lock()
 				defer c.entriesLock.Unlock()
@@ -491,7 +491,11 @@ func (c *Cache) fetch(key string, r getOptions, allowNew bool, attempt uint, ign
 		fOpts := FetchOptions{}
 		if tEntry.Opts.SupportsBlocking {
 			fOpts.MinIndex = entry.Index
-			fOpts.Timeout = tEntry.Opts.RefreshTimeout
+			fOpts.Timeout = tEntry.Opts.QueryTimeout
+
+			if fOpts.Timeout == 0 {
+				fOpts.Timeout = 10 * time.Minute
+			}
 		}
 		if entry.Valid {
 			fOpts.LastResult = &FetchResult{

agent/cache/cache_test.go

@@ -464,9 +464,9 @@ func TestCacheGet_periodicRefresh(t *testing.T) {
 
 	typ := &MockType{}
 	typ.On("RegisterOptions").Return(RegisterOptions{
-		Refresh:        true,
-		RefreshTimer:   100 * time.Millisecond,
-		RefreshTimeout: 5 * time.Minute,
+		Refresh:      true,
+		RefreshTimer: 100 * time.Millisecond,
+		QueryTimeout: 5 * time.Minute,
 	})
 	defer typ.AssertExpectations(t)
 	c := TestCache(t)
@@ -504,9 +504,9 @@ func TestCacheGet_periodicRefreshMultiple(t *testing.T) {
 
 	typ := &MockType{}
 	typ.On("RegisterOptions").Return(RegisterOptions{
-		Refresh:        true,
-		RefreshTimer:   0 * time.Millisecond,
-		RefreshTimeout: 5 * time.Minute,
+		Refresh:      true,
+		RefreshTimer: 0 * time.Millisecond,
+		QueryTimeout: 5 * time.Minute,
 	})
 	defer typ.AssertExpectations(t)
 	c := TestCache(t)
@@ -553,9 +553,9 @@ func TestCacheGet_periodicRefreshErrorBackoff(t *testing.T) {
 
 	typ := &MockType{}
 	typ.On("RegisterOptions").Return(RegisterOptions{
-		Refresh:        true,
-		RefreshTimer:   0,
-		RefreshTimeout: 5 * time.Minute,
+		Refresh:      true,
+		RefreshTimer: 0,
+		QueryTimeout: 5 * time.Minute,
 	})
 	defer typ.AssertExpectations(t)
 	c := TestCache(t)
@@ -595,9 +595,9 @@ func TestCacheGet_periodicRefreshBadRPCZeroIndexErrorBackoff(t *testing.T) {
 
 	typ := &MockType{}
 	typ.On("RegisterOptions").Return(RegisterOptions{
-		Refresh:        true,
-		RefreshTimer:   0,
-		RefreshTimeout: 5 * time.Minute,
+		Refresh:      true,
+		RefreshTimer: 0,
+		QueryTimeout: 5 * time.Minute,
 	})
 	defer typ.AssertExpectations(t)
 	c := TestCache(t)
@@ -642,7 +642,7 @@ func TestCacheGet_noIndexSetsOne(t *testing.T) {
 		SupportsBlocking: true,
 		Refresh:          true,
 		RefreshTimer:     0,
-		RefreshTimeout:   5 * time.Minute,
+		QueryTimeout:     5 * time.Minute,
 	})
 	defer typ.AssertExpectations(t)
 	c := TestCache(t)
@@ -702,7 +702,7 @@ func TestCacheGet_fetchTimeout(t *testing.T) {
 	typ := &MockType{}
 	timeout := 10 * time.Minute
 	typ.On("RegisterOptions").Return(RegisterOptions{
-		RefreshTimeout:   timeout,
+		QueryTimeout:     timeout,
 		SupportsBlocking: true,
 	})
 	defer typ.AssertExpectations(t)
@@ -954,9 +954,9 @@ func TestCacheGet_refreshAge(t *testing.T) {
 
 	typ := &MockType{}
 	typ.On("RegisterOptions").Return(RegisterOptions{
-		Refresh:        true,
-		RefreshTimer:   0,
-		RefreshTimeout: 5 * time.Minute,
+		Refresh:      true,
+		RefreshTimer: 0,
+		QueryTimeout: 5 * time.Minute,
 	})
 	defer typ.AssertExpectations(t)
 	c := TestCache(t)

agent/proxycfg/testing.go

@@ -1995,7 +1995,7 @@ func (ct *ControllableCacheType) RegisterOptions() cache.RegisterOptions {
 	return cache.RegisterOptions{
 		Refresh:          ct.blocking,
 		SupportsBlocking: ct.blocking,
-		RefreshTimeout:   10 * time.Minute,
+		QueryTimeout:     10 * time.Minute,
 	}
 }