Remove timeout logic from ReadRaw functions and add ReadRawWithContext (#18708)

Removing the timeout logic from raw-response functions and adding documentation comments. The following functions are affected:

- `ReadRaw`
- `ReadRawWithContext` (newly added)
- `ReadRawWithData`
- `ReadRawWithDataWithContext`

The previous logic of using `ctx, _ = c.c.withConfiguredTimeout(ctx)` could cause a potential [context leak](https://pkg.go.dev/context):

> Failing to call the CancelFunc leaks the child and its children until the parent is canceled or the timer fires. The go vet tool checks that CancelFuncs are used on all control-flow paths.

Cancelling the context would have caused more issues since the context would be cancelled before the request body is closed.

Resolves: #18658

api/client.go

@@ -114,7 +114,11 @@ type Config struct {
 	// of three tries).
 	MaxRetries int
 
-	// Timeout is for setting custom timeout parameter in the HttpClient
+	// Timeout, given a non-negative value, will apply the request timeout
+	// to each request function unless an earlier deadline is passed to the
+	// request function through context.Context. Note that this timeout is
+	// not applicable to Logical().ReadRaw* (raw response) functions.
+	// Defaults to 60 seconds.
 	Timeout time.Duration
 
 	// If there is an error when creating the configuration, this will be the

api/logical.go

@@ -69,20 +69,46 @@ func (c *Logical) ReadWithDataWithContext(ctx context.Context, path string, data
 	return c.ParseRawResponseAndCloseBody(resp, err)
 }
 
+// ReadRaw attempts to read the value stored at the given Vault path
+// (without '/v1/' prefix) and returns a raw *http.Response.
+//
+// Note: the raw-response functions do not respect the client-configured
+// request timeout; if a timeout is desired, please use ReadRawWithContext
+// instead and set the timeout through context.WithTimeout or context.WithDeadline.
 func (c *Logical) ReadRaw(path string) (*Response, error) {
-	return c.ReadRawWithData(path, nil)
+	return c.ReadRawWithDataWithContext(context.Background(), path, nil)
 }
 
+// ReadRawWithContext attempts to read the value stored at the give Vault path
+// (without '/v1/' prefix) and returns a raw *http.Response.
+//
+// Note: the raw-response functions do not respect the client-configured
+// request timeout; if a timeout is desired, please set it through
+// context.WithTimeout or context.WithDeadline.
+func (c *Logical) ReadRawWithContext(ctx context.Context, path string) (*Response, error) {
+	return c.ReadRawWithDataWithContext(ctx, path, nil)
+}
+
+// ReadRawWithData attempts to read the value stored at the given Vault
+// path (without '/v1/' prefix) and returns a raw *http.Response. The 'data' map
+// is added as query parameters to the request.
+//
+// Note: the raw-response functions do not respect the client-configured
+// request timeout; if a timeout is desired, please use
+// ReadRawWithDataWithContext instead and set the timeout through
+// context.WithTimeout or context.WithDeadline.
 func (c *Logical) ReadRawWithData(path string, data map[string][]string) (*Response, error) {
 	return c.ReadRawWithDataWithContext(context.Background(), path, data)
 }
 
+// ReadRawWithDataWithContext attempts to read the value stored at the given
+// Vault path (without '/v1/' prefix) and returns a raw *http.Response. The 'data'
+// map is added as query parameters to the request.
+//
+// Note: the raw-response functions do not respect the client-configured
+// request timeout; if a timeout is desired, please set it through
+// context.WithTimeout or context.WithDeadline.
 func (c *Logical) ReadRawWithDataWithContext(ctx context.Context, path string, data map[string][]string) (*Response, error) {
-	// See note in client.go, RawRequestWithContext for why we do not call
-	// Cancel here. The difference between these two methods are that the
-	// former takes a Request object directly, whereas this builds one
-	// up for the caller.
-	ctx, _ = c.c.withConfiguredTimeout(ctx)
 	return c.readRawWithDataWithContext(ctx, path, data)
 }
 

changelog/18708.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+api: Remove timeout logic from ReadRaw functions and add ReadRawWithContext
+```

command/healthcheck/healthcheck.go

@@ -25,6 +25,7 @@
 package healthcheck
 
 import (
+	"context"
 	"fmt"
 	"strings"
 
@@ -162,7 +163,11 @@ func (e *Executor) FetchIfNotFetched(op logical.Operation, rawPath string) (*Pat
 		return nil, fmt.Errorf("unknown operation: %v on %v", op, path)
 	}
 
-	response, err := e.Client.Logical().ReadRawWithData(path, data)
+	// client.ReadRaw* methods require a manual timeout override
+	ctx, cancel := context.WithTimeout(context.Background(), e.Client.ClientTimeout())
+	defer cancel()
+
+	response, err := e.Client.Logical().ReadRawWithDataWithContext(ctx, path, data)
 	ret.Response = response
 	if err != nil {
 		ret.FetchError = err

command/read.go

@@ -1,6 +1,7 @@
 package command
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"os"
@@ -77,6 +78,10 @@ func (c *ReadCommand) Run(args []string) int {
 		return 2
 	}
 
+	// client.ReadRaw* methods require a manual timeout override
+	ctx, cancel := context.WithTimeout(context.Background(), client.ClientTimeout())
+	defer cancel()
+
 	// Pull our fake stdin if needed
 	stdin := (io.Reader)(os.Stdin)
 	if c.testStdin != nil {
@@ -92,7 +97,7 @@ func (c *ReadCommand) Run(args []string) int {
 	}
 
 	if Format(c.UI) != "raw" {
-		secret, err := client.Logical().ReadWithData(path, data)
+		secret, err := client.Logical().ReadWithDataWithContext(ctx, path, data)
 		if err != nil {
 			c.UI.Error(fmt.Sprintf("Error reading %s: %s", path, err))
 			return 2
@@ -109,7 +114,7 @@ func (c *ReadCommand) Run(args []string) int {
 		return OutputSecret(c.UI, secret)
 	}
 
-	resp, err := client.Logical().ReadRawWithData(path, data)
+	resp, err := client.Logical().ReadRawWithDataWithContext(ctx, path, data)
 	if err != nil {
 		c.UI.Error(fmt.Sprintf("Error reading: %s: %s", path, err))
 		return 2