sdk/ldaputil: add connection_timeout configurable #20144
Conversation
jasonodonnell
requested review from
ltcarbonell,
elliesterner,
davidadeleon and
a team
There was a problem hiding this comment.
Looks good; I provided an alternative option for setting the timeout since it appears that the Dial* methods we're using are deprecated.
| // Default timeout in the pacakge is 60 seconds, which we default to on our | ||
| // end. This is useful if you want to take advantage of the URL list to increase | ||
| // availability of LDAP. | ||
| ldap.DefaultTimeout = time.Duration(cfg.ConnectionTimeout) * time.Second |
There was a problem hiding this comment.
Instead of changing the external package's DefaultTmeout, there is an ldap.DialURL that allows for a DialWithDialer option where we can set the timeout.
There was a problem hiding this comment.
I looked into this during implementation but I think it will require a lot more changes to this helper library instead of modifying the Dial wrapper. That seemed a lot more risky even though I don't take changing an exported package variable lightly.
There was a problem hiding this comment.
@jasonodonnell it looks like this is triggering the race detector in tests, since two tests running in parallel modify ldap.DefaultTimeout. https://github.com/hashicorp/vault/actions/runs/4697499717/jobs/8329395640. I can remove the parallel tests as a workaround, but this is perhaps more reason to switch to DialURL
There was a problem hiding this comment.
Looking into this. Thanks!
* sdk/ldaputil: add connection_timeout configurable * changelog * Update doc * Fix test * Change default to 30s
* sdk/ldaputil: add connection_timeout configurable * changelog * Update doc * Fix test * Change default to 30s
* sdk/ldaputil: add connection_timeout configurable * changelog * Update doc * Fix test * Change default to 30s
* sdk/ldaputil: add connection_timeout configurable * changelog * Update doc * Fix test * Change default to 30s
The go-ldap package sets a 60 second timeout in their package for dial attempts which aligns with Vault's 60 second client timeout. Since these timeouts are the same if the first value in our LDAP config
urlis unavailable, no other URLs in the list will be tried and all connections will fail.This adds a new LDAP config
connection_timeoutwhich allows operators to override this timeout value. By setting this value lower you can leverage the backup URLs in the config.This is not the same as the existing config
request_timeoutwhich tunes timeout values after a connection is made to LDAP.A workaround exists where clients can set
VAULT_CLIENT_TIMEOUT. By adding this config though we can set timeouts for all user connections and may be a better overall experience.