From a4973bc45aa7a4c4020249a58bb0dc7e4dd7cf39 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 c6843348e..26d291914 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 1a720cbf2..e36fde8c9 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 000000000..1db2ba623 --- /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 53910ea44..1949de04e 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 91e50c519..3487c5d0d 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