From ef3e3eace250d13e1071f2b14d65f3ab60b72425 Mon Sep 17 00:00:00 2001 From: Anton Averchenkov <84287187+averche@users.noreply.github.com> Date: Tue, 17 Jan 2023 15:41:59 -0500 Subject: [PATCH] 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 | 6 ++++- api/logical.go | 38 +++++++++++++++++++++++++----- changelog/18708.txt | 3 +++ command/healthcheck/healthcheck.go | 7 +++++- command/read.go | 9 +++++-- 5 files changed, 53 insertions(+), 10 deletions(-) create mode 100644 changelog/18708.txt diff --git a/api/client.go b/api/client.go index c6843348e5..26d291914d 100644 --- a/api/client.go +++ b/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 diff --git a/api/logical.go b/api/logical.go index 1a720cbf2c..e36fde8c93 100644 --- a/api/logical.go +++ b/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) } diff --git a/changelog/18708.txt b/changelog/18708.txt new file mode 100644 index 0000000000..1db2ba6239 --- /dev/null +++ b/changelog/18708.txt @@ -0,0 +1,3 @@ +```release-note:bug +api: Remove timeout logic from ReadRaw functions and add ReadRawWithContext +``` diff --git a/command/healthcheck/healthcheck.go b/command/healthcheck/healthcheck.go index 53910ea446..1949de04e5 100644 --- a/command/healthcheck/healthcheck.go +++ b/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 diff --git a/command/read.go b/command/read.go index 91e50c519d..3487c5d0d7 100644 --- a/command/read.go +++ b/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