diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index c9aef60e3..a2b96e969 100644 --- a/agent/agent_endpoint_test.go +++ b/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")) } } diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index c17780489..325423bec 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/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 diff --git a/agent/cache-types/options.go b/agent/cache-types/options.go index 6864258e8..5eb6cdd9b 100644 --- a/agent/cache-types/options.go +++ b/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, } } diff --git a/agent/cache/cache.go b/agent/cache/cache.go index 2b1f3c120..a216d4ab8 100644 --- a/agent/cache/cache.go +++ b/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{ diff --git a/agent/cache/cache_test.go b/agent/cache/cache_test.go index 5036a12ca..72422fb25 100644 --- a/agent/cache/cache_test.go +++ b/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) diff --git a/agent/proxycfg/testing.go b/agent/proxycfg/testing.go index 2a36eb192..910398c32 100644 --- a/agent/proxycfg/testing.go +++ b/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, } }