Skip to content
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.

Sign up for GitHub

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

Fix "not master" error in mongo plugin #7743

Closed
wants to merge 7 commits into from
Closed

Fix "not master" error in mongo plugin #7743

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8b0f654
Fix not master error in mongo plugin
zhecksum Oct 26, 2019
10d84a1
Close connection and reconnect after primary step down
zhecksum Oct 26, 2019
95f52fd
Close connection without trying to acquire lock again
zhecksum Oct 26, 2019
a94e333
Fix a deadlock in RevokeUser
zhecksum Oct 26, 2019
22c154d
Rewrite error message when failing to close non-primary
zhecksum Oct 26, 2019
4932579
Reinstate the Lock() in Close()
zhecksum Oct 31, 2019
016a8b4
Merge branch 'master' of https://github.com/hashicorp/vault
zhecksum Oct 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions plugins/database/mongodb/connection_producer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,7 @@ func (c *mongoDBConnectionProducer) Connection(_ context.Context) (interface{},
return c.session, nil
}

// Close terminates the database connection.
func (c *mongoDBConnectionProducer) Close() error {
c.Lock()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locking inside Close() always produces a deadlock, as all of the calling functions (from mongodb.go) have locked the mutex already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close can be called externally which is why we need to lock.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I see two possibilities for dealing with that then.

  1. Unlocking before calling it, and then locking again once done. (downside being that the lock needs to be acquired twice, and that another process can interfere inbetween the locking and unlocking)

  2. Not calling it at all, and just closing the session by referencing the mongoDBConnectionProducer in mongodb.go. I had a commit where I did exactly this, and it worked ok.

What do you prefer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calvn Close now locks again!

defer c.Unlock()

if c.session != nil {
c.session.Close()
Expand Down
30 changes: 29 additions & 1 deletion plugins/database/mongodb/mongodb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,20 @@ func (m *MongoDB) CreateUser(ctx context.Context, statements dbplugin.Statements
case err == nil:
case err == io.EOF, strings.Contains(err.Error(), "EOF"):
// Call getConnection to reset and retry query if we get an EOF error on first attempt.
session, err := m.getConnection(ctx)
if err != nil {
return "", "", err
}
err = session.DB(mongoCS.DB).Run(createUserCmd, nil)
if err != nil {
return "", "", err
}
case strings.Contains(err.Error(), "not master"):
// Close connection and reconnect if connected node is not primary.
if err := m.Close(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to call Close here, getConnection should take care of closing the session. Do we also want to retry the query afterwards? If so, we might just combine the EOF case and this one into one (similar to what you're doing in RevokeUser.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does getConnection really close the session in this case though? What happens if the session is not nil already and the ping succeeds?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see getConnection calls Connection() in connection_producer.go which only closes the session if the session is not nil and the ping fails.

The ping won't fail though, as the node is still up (it's just not the primary). I don't see a way to check whether it's master with the ping command.

return "", "", errwrap.Wrapf("error closing EOF'd mongo connection: {{err}}", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set the session to nil afterwards?


session, err := m.getConnection(ctx)
if err != nil {
return "", "", err
Expand Down Expand Up @@ -190,6 +204,20 @@ func (m *MongoDB) SetCredentials(ctx context.Context, statements dbplugin.Statem
case err == nil:
case err == io.EOF, strings.Contains(err.Error(), "EOF"):
// Call getConnection to reset and retry query if we get an EOF error on first attempt.
session, err := m.getConnection(ctx)
if err != nil {
return "", "", err
}
err = session.DB(dialInfo.Database).UpsertUser(&mongoUser)
if err != nil {
return "", "", err
}
case strings.Contains(err.Error(), "not master"):
// Close connection and reconnect if connected node is not primary.
if err := m.Close(); err != nil {
return "", "", errwrap.Wrapf("error closing EOF'd mongo connection: {{err}}", err)
}

session, err := m.getConnection(ctx)
if err != nil {
return "", "", err
Expand Down Expand Up @@ -251,7 +279,7 @@ func (m *MongoDB) RevokeUser(ctx context.Context, statements dbplugin.Statements
err = session.DB(db).RemoveUser(username)
switch {
case err == nil, err == mgo.ErrNotFound:
case err == io.EOF, strings.Contains(err.Error(), "EOF"):
case err == io.EOF, strings.Contains(err.Error(), "EOF"), strings.Contains(err.Error(), "not master"):
if err := m.Close(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we've not bumped into a deadlock here, but we should remove this Close call.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I removed it and replcaed it with a different way of closing that doesn't lead to deadlock.

return errwrap.Wrapf("error closing EOF'd mongo connection: {{err}}", err)
}
Expand Down