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

Make Unlock key delete conditional on being old leader's #6637

Merged
merged 7 commits into from
May 1, 2019
Merged

Make Unlock key delete conditional on being old leader's #6637

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a95cf02
Make Unlock key delete conditional on being old leader's
EdwinRobbins Apr 23, 2019
54d8c25
Tidy up types and error usage
EdwinRobbins Apr 26, 2019
01bfce8
Catch conditional check error for Unlock()
EdwinRobbins Apr 29, 2019
60b0e89
Update error handling for conditional errors
EdwinRobbins Apr 30, 2019
3e24392
Remove no unused imports
EdwinRobbins Apr 30, 2019
0aba117
Replace filterError with an error check
Apr 30, 2019
a81fa01
Restore comment
Apr 30, 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
29 changes: 25 additions & 4 deletions physical/dynamodb/dynamodb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,10 +571,28 @@ func (l *DynamoDBLock) Unlock() error {
}

l.held = false
if err := l.backend.Delete(context.Background(), l.key); err != nil {
return err

// Conditionally delete after check that the key is actually this Vault's and
// not been already claimed by another leader
condition := "#identity = :identity"
deleteMyLock := &dynamodb.DeleteItemInput{
TableName: &l.backend.table,
ConditionExpression: &condition,
Key: map[string]*dynamodb.AttributeValue{
"Path": &dynamodb.AttributeValue{S: aws.String(recordPathForVaultKey(l.key))},
Copy link
Contributor

Choose a reason for hiding this comment

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

The type (&dynamodb.AttributeValue) isn't required here, or on 583 & 589.

"Key": &dynamodb.AttributeValue{S: aws.String(recordKeyForVaultKey(l.key))},
},
ExpressionAttributeNames: map[string]*string{
"#identity": aws.String("Identity"),
},
ExpressionAttributeValues: map[string]*dynamodb.AttributeValue{
":identity": &dynamodb.AttributeValue{B: []byte(l.identity)},
},
}
return nil

_, err := l.backend.client.DeleteItem(deleteMyLock)

return err
}

// Value checks whether or not the lock is held by any instance of DynamoDBLock,
Expand Down Expand Up @@ -634,7 +652,10 @@ func (l *DynamoDBLock) periodicallyRenewLock(done chan struct{}) {
select {
case <-ticker.C:
// This should not renew the lock if the lock was deleted from under you.
l.updateItem(false)
err := l.updateItem(false)
if err != nil {
l.backend.logger.Error("error renewing leadership lock", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

hclog takes a message and then label/value pairs, so this needs to be like: Error("my message", "error", "err")

}
case <-done:
ticker.Stop()
return
Expand Down
4 changes: 3 additions & 1 deletion vault/ha.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,9 @@ func (c *Core) waitForLeadership(newLeaderCh chan func(), manualStepDownCh, stop
c.logger.Error("clearing leader advertisement failed", "error", err)
}

c.heldHALock.Unlock()
if err := c.heldHALock.Unlock(); err != nil {
c.logger.Error("unlocking HA lock failed", "error", err)
Copy link
Contributor

@kalafut kalafut Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

hclog takes a message and then label/value pairs, so this needs to be like: Error("my message", "error", "err")

That said, I'm hesitant about this change until we understand what all of the HA backends are returning. For example even DynamoDB would need to have some additional guards around the error from Unlock(), like the check for dynamodb.ErrCodeConditionalCheckFailedException elsewhere in the file, or else you end up with [ERROR] log entries that are really just expected artifacts from that DB/SDK.

To be clear I do like the idea here, but we may want to run some additional tests with other backends lest customers start seeing errors. Thoughts, @briankassouf ?

Copy link
Contributor

Choose a reason for hiding this comment

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

After some discussion, we decided that this line is OK to leave in. But please add a check to Unlock() (which calls your modified Delete) to catch the ConditionCheck error and not report it as an error, similar to that handling here:

vault/physical/dynamodb/dynamodb.go

Line 632 in 54d8c25

if err.Code() != dynamodb.ErrCodeConditionalCheckFailedException {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think this case is a bit different from the tryToLock one because in that case it is speculatively attempting to update and has the expectation that it can reasonably fail. Whereas for Unlock() this shouldn't really expected unless something aberrant has happened.

Though as long as Vault will always attempt unlock when it steps down, even if it knows there is a new leader already, it's reasonable to not report the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is interesting b/c I've seen this occur in Unlock at least once, but that was during a simulated network outage. Much more common (and I didn't mention this previously) is [ERROR] storage.dynamodb: error renewing leadership lock: error="ConditionalCheckFailedException: .... This occurs with your current build during operator step-down. So a couple of things:

  1. This error catch should be just in dynamodb.go, not ha.go which is backend agnostic. e.g. move to after you call DeleteItem() in the dynamo implementation of Unlock().
  2. We also need to catch the one triggering the "renewing leadership lock" message. Both tryToLock and periodicallyRenewLock rely updateItem, so I think centralizing the check in updateItem is reasonable. i.e. if the error is the ConditionCheck, just make it nil instead.
  3. (Optional) A filterError() method that just does the transformation of err->nil if ConditionCheck (or a no-op if no) might benefit 1 and 2, and keeps it clean if other such benign errors ever are added.

I appreciate the effort on this! I'd like to get this merged fairly soon, so if you need any help I'm able to push to your branch as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those all make sense to me and should be taken care of now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alas, my suggested refactor was too much. The presence of an error is needed from updateItem() for correct tryToLock behavior. I've changed this slightly to restore that, and now it seems to work fine. There is no flapping of the active node, spurious errors aren't showing up in the logs, a situations like the lock file being deleted or network being lost are handled OK. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes look good to me.

}
c.heldHALock = nil
}

Expand Down