Add maximum request duration (timeouts) for all requests except actua…

…l monitor and events requests (#28230) * fix paths for sys/monitor and sys/events * add changelog * add changelog * Update http/handler.go Co-authored-by: Violet Hynes <[email protected]> * added some modifications to check for timeout --------- Co-authored-by: Violet Hynes <[email protected]>
hashicorp · Aug 30, 2024 · 06fac16 · 06fac16
1 parent 8e1db67
commit 06fac16
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 3 deletions.
diff --git a/changelog/28230.txt b/changelog/28230.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+core: Fixed an issue where maximum request duration timeout was not being added to all requests containing strings sys/monitor and sys/events. With this change, timeout is now added to all requests except monitor and events endpoint. 
+```
diff --git a/command/agentproxyshared/cache/static_secret_cache_updater_test.go b/command/agentproxyshared/cache/static_secret_cache_updater_test.go
@@ -6,6 +6,7 @@ package cache
 import (
 	"context"
 	"fmt"
+	"os"
 	"sync"
 	syncatomic "sync/atomic"
 	"testing"
@@ -281,6 +282,8 @@ func TestOpenWebSocketConnection_AutoAuthSelfHeal(t *testing.T) {
 // works as expected with the default KVV1 mount, and then the connection can be used to receive an event.
 // This acts as more of an event system sanity check than a test of the updater
 // logic. It's still important coverage, though.
+// It also adds a client timeout of 1 second and checks that the connection does not timeout as this is a
+// streaming request.
 func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) {
 	if !constants.IsEnterprise {
 		t.Skip("test can only run on enterprise due to requiring the event notification system")
@@ -290,6 +293,11 @@ func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) {
 	cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{
 		HandlerFunc: vaulthttp.Handler,
 	})
+
+	oldClientTimeout := os.Getenv("VAULT_CLIENT_TIMEOUT")
+	os.Setenv("VAULT_CLIENT_TIMEOUT", "1")
+	defer os.Setenv("VAULT_CLIENT_TIMEOUT", oldClientTimeout)
+
 	client := cluster.Cores[0].Client
 
 	updater := testNewStaticSecretCacheUpdater(t, client)
@@ -318,6 +326,7 @@ func TestOpenWebSocketConnectionReceivesEventsDefaultMount(t *testing.T) {
 
 		// This method blocks until it gets a secret, so this test
 		// will only pass if we're receiving events correctly.
+		// It will fail here if the connection times out.
 		_, _, err = conn.Read(context.Background())
 		require.NoError(t, err)
 	}

diff --git a/http/handler.go b/http/handler.go
@@ -386,7 +386,9 @@ func wrapGenericHandler(core *vault.Core, h http.Handler, props *vault.HandlerPr
 		ctx := r.Context()
 		var cancelFunc context.CancelFunc
 		// Add our timeout, but not for the monitor or events endpoints, as they are streaming
-		if strings.HasSuffix(r.URL.Path, "sys/monitor") || strings.Contains(r.URL.Path, "sys/events") {
+		// Request URL path for sys/monitor looks like /v1/sys/monitor
+		// Request URL paths for event subscriptions look like /v1/sys/events/subscribe/{eventType}. Example: /v1/sys/events/subscribe/kv*
+		if r.URL.Path == "/v1/sys/monitor" || strings.HasPrefix(r.URL.Path, "/v1/sys/events/subscribe") {
 			ctx, cancelFunc = context.WithCancel(ctx)
 		} else {
 			ctx, cancelFunc = context.WithTimeout(ctx, maxRequestDuration)

diff --git a/http/sys_monitor_test.go b/http/sys_monitor_test.go
@@ -97,7 +97,8 @@ func TestSysMonitorStreamingLogs(t *testing.T) {
 			}
 			jsonLog := &jsonlog{}
 
-			timeCh := time.After(5 * time.Second)
+			// default timeout is 90 seconds
+			timeCh := time.After(120 * time.Second)
 
 			for {
 				select {
@@ -119,7 +120,7 @@ func TestSysMonitorStreamingLogs(t *testing.T) {
 						return
 					}
 				case <-timeCh:
-					t.Fatal("Failed to get a DEBUG message after 5 seconds")
+					t.Fatal("Failed to get a DEBUG message after 120 seconds")
 				}
 			}
 		})