-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Locking updates in database backend #3774
Conversation
unlockFunc() | ||
return nil, fmt.Errorf("cound not retrieve db with name: %s, got error: %s", role.DBName, err) | ||
// Check again | ||
db, ok = b.getDBObj(role.DBName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the createDBObj
function does this check at the beginning of the function, so doesn't necessarily need to happen here.
if err != nil { | ||
unlockFunc() | ||
b.closeIfShutdown(role.DBName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than giving up the lock and requiring it in the closeIfShutdown
function what if we unlocked after the closeIfShutdown
call. That'll protect against another thread getting the lock before we reset the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally went down that route but we don't always have the write lock when this method gets called.
LGTM; we'll accept the race condition for now. |
There was one place where the lock could potentially not be released. Specifically https://github.com/hashicorp/vault/blob/master/builtin/logical/database/secret_creds.go#L72.