Nil pointer dereference errors when setting partial values for vault.RetryConfiguration

[rohitss912]

Expected Behavior

We have the option of setting the custom RetryConfiguration in the client_options.go file via the func WithRetryConfiguration(configuration RetryConfiguration) ClientOption() function.

But, when using this function, what If we set only partial fields in the struct type RetryConfiguration struct that we would want to control like

client, err := vault.New(
vault.WithAddress(endpoint),
vault.WithRequestTimeout(30*time.Second),
vault.WithRetryConfiguration(vault.RetryConfiguration{
	RetryWaitMin: 2 * time.Second,
	RetryMax:     3,
	RetryWaitMax: 3 * time.Second,
}),
)

I would expect the HTTP calls to go through fine and the rest of fields of the type RetryConfiguration struct to be set to defaults if we aren't setting it similar to what func DefaultConfiguration() ClientConfiguration is doing.

Can the method func WithRetryConfiguration() not check the unset fields and set them to defaults?

Current Behavior

What is the current behavior?

Current snippet :

client, err := vault.New(
vault.WithAddress(endpoint),
vault.WithRequestTimeout(30*time.Second),
vault.WithRetryConfiguration(vault.RetryConfiguration{
	RetryWaitMin: 2 * time.Second,
	RetryMax:     3,
	RetryWaitMax: 3 * time.Second,
}),
)

is throwing errors complaining about Nil pointer dereference errors as the remaining fields CheckRetry, Backoff, ErrorHandler & Logger are unset during the http call.

Failure Information

Please include the version of Vault binary and the version of vault-client-go you're using.

vault-client-go-v0.3.3

Steps to Reproduce

Please provide detailed steps for reproducing the issue.

1. Setup the vault client with below snippet

client, err := vault.New(
vault.WithAddress(endpoint),
vault.WithRequestTimeout(30*time.Second),
vault.WithRetryConfiguration(vault.RetryConfiguration{
	RetryWaitMin: 2 * time.Second,
	RetryMax:     3,
	RetryWaitMax: 3 * time.Second,
}),
)

2. Perform a vault call like:

resp, err := client.System.ReadHealthStatus(ctx)

3. You should see errors like when the HTTP call is being made -

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12dfdd8]

Additional Information

Full Stack trace

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12dfdd8]

goroutine 1 [running]:
github.com/hashicorp/go-retryablehttp.(*Client).Do(0xc0001218f0, 0xc0000b2288)
	/Users/rohit.appu/go/pkg/mod/github.com/hashicorp/[email protected]/client.go:641 +0x678
github.com/hashicorp/vault-client-go.(*Client).do(0xc000122780, 0x0?, 0x0?)
	/Users/rohit.appu/go/pkg/mod/github.com/hashicorp/[email protected]/client_requests.go:445 +0x8e
github.com/hashicorp/vault-client-go.(*Client).send(0xc000122780, {0x170dba8?, 0xc00009ff50?}, 0x0?, 0x0?, {0x0, 0x0, 0x0?}, {0x0, 0x0, ...})
	/Users/rohit.appu/go/pkg/mod/github.com/hashicorp/[email protected]/client_requests.go:403 +0x11c
github.com/hashicorp/vault-client-go.sendRequestParseResponse[...]({0x170db70, 0xc0000a2000?}, 0xc000122780?, {0x1637f7f, 0x3}, {0x163bde1, _}, {0x0, 0x0}, _, ...)
	/Users/rohit.appu/go/pkg/mod/github.com/hashicorp/[email protected]/client_requests.go:260 +0x336
github.com/hashicorp/vault-client-go.(*System).ReadHealthStatus(0xc0001229d0, {0x170db70, 0xc0000a2000}, {0x0?, 0x14e3b40?, 0x77359400?})
	/Users/rohit.appu/go/pkg/mod/github.com/hashicorp/[email protected]/api_system.go:2723 +0x1f1
jcredctl/internal/vault.verifyConnectivity({0x170db70?, 0xc0000a2000?}, {0x1707d00?, 0xc0001229d0?})
	/Users/rohit.appu/Documents/Repositories/is-cloudbees-core/jcredctl/internal/vault/vault_base.go:106 +0x4b
jcredctl/internal/vault.New({0x0?, 0x0?}, {0x7ff7bfeff3c4, 0x1a})
	/Users/rohit.appu/Documents/Repositories/is-cloudbees-core/jcredctl/internal/vault/vault_base.go:37 +0xeb
jcredctl/cmd.InfoCmd.func1(0xc00012a600?, {0x16383cd?, 0x2?, 0x2?})
	/Users/rohit.appu/Documents/Repositories/is-cloudbees-core/jcredctl/cmd/info.go:21 +0x47
github.com/spf13/cobra.(*Command).execute(0xc00012a600, {0xc0000c2720, 0x2, 0x2})
	/Users/rohit.appu/go/pkg/mod/github.com/spf13/[email protected]/command.go:944 +0x847
github.com/spf13/cobra.(*Command).ExecuteC(0xc00012a000)
	/Users/rohit.appu/go/pkg/mod/github.com/spf13/[email protected]/command.go:1068 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/rohit.appu/go/pkg/mod/github.com/spf13/[email protected]/command.go:992
main.main()
	/Users/rohit.appu/Documents/Repositories/is-cloudbees-core/jcredctl/main.go:12 +0x1f

[rohitss912]

@achew22 Can you please check the above ^ if possible?

[averche]

Thanks for creating this issue, @rohitss912, and sorry for the delayed response as I was away on vacation.

I would expect the HTTP calls to go through fine and the rest of fields of the type RetryConfiguration struct to be set to defaults if we aren't setting it similar to what func DefaultConfiguration() ClientConfiguration is doing.

Yes, this makes sense to me. The fix should be relatively easy, I think.

[rohitss912]

Thank you for addressing this @averche !