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

EdwinRobbins · 2019-04-25T00:12:28Z

No description provided.

hashicorp-cla · 2019-04-25T00:12:35Z

All committers have signed the CLA.

kalafut · 2019-04-25T16:24:46Z

@EdwinRobbins Thanks. I was was able to reproduce the issue and I think your fix is the right approach. I have some feedback on the PR, but first I'd like to get the CLA sorted out. Can you rewrite your commit(s) using an email address that is part of your Github profile?

EdwinRobbins · 2019-04-26T01:04:23Z

@kalafut swapped over to my Github profile address; CLA should be all sorted.

kalafut

This works well and I think the core logic is correct. Just some smaller items to change.

kalafut · 2019-04-26T15:38:48Z

physical/dynamodb/dynamodb.go

-			l.updateItem(false)
+			err := l.updateItem(false)
+			if err != nil {
+				l.backend.logger.Error("error renewing leadership lock", err)


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

kalafut · 2019-04-26T15:38:57Z

vault/ha.go

@@ -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)


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 ?

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 {

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.

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:

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().

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.

(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.

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

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.

Changes look good to me.

kalafut · 2019-04-26T16:14:04Z

physical/dynamodb/dynamodb.go

+		TableName:           &l.backend.table,
+		ConditionExpression: &condition,
+		Key: map[string]*dynamodb.AttributeValue{
+			"Path": &dynamodb.AttributeValue{S: aws.String(recordPathForVaultKey(l.key))},


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

Push logic into dynamodb to keep specific logic contained to it. Centralize filtering and add to updateItem and Unlock

jefferai · 2019-04-30T20:03:04Z

vault/ha.go

@@ -6,6 +6,8 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
+	"github.com/aws/aws-sdk-go/aws/awserr"


What's the reason for these imports?

Ah, that should have been removed when I moved the DynamoDB specific logic back into dyanamodb.go.

kalafut · 2019-04-30T22:55:21Z

Another engineer is going to do a re-review but I think we're in good shape now.

kalafut · 2019-05-01T00:46:03Z

Thanks for your work on this!

* master: Fixed Typo [Doc]: PKI Fix allowed_uri_sans spelling mistake (#6660) DynamoDB: Make Unlock key delete conditional on being old leader's (#6637) Fix hook by using env to discover the correct location of bash as sh doesn't have [[

…6637)

EdwinRobbins force-pushed the erobbins/fix-dynamodb-leader-delete branch from 9f455b9 to 092c1a8 Compare April 25, 2019 00:15

EdwinRobbins mentioned this pull request Apr 25, 2019

Vault leader thrashing when backed by DynamoDB #6636

Closed

kalafut self-assigned this Apr 25, 2019

Make Unlock key delete conditional on being old leader's

a95cf02

EdwinRobbins force-pushed the erobbins/fix-dynamodb-leader-delete branch from 092c1a8 to a95cf02 Compare April 26, 2019 00:58

kalafut suggested changes Apr 26, 2019

View reviewed changes

EdwinRobbins added 3 commits April 26, 2019 14:46

Tidy up types and error usage

54d8c25

Catch conditional check error for Unlock()

01bfce8

Update error handling for conditional errors

60b0e89

Push logic into dynamodb to keep specific logic contained to it. Centralize filtering and add to updateItem and Unlock

jefferai reviewed Apr 30, 2019

View reviewed changes

EdwinRobbins and others added 3 commits April 30, 2019 13:16

Remove no unused imports

3e24392

Replace filterError with an error check

0aba117

Restore comment

a81fa01

ncabatoff approved these changes May 1, 2019

View reviewed changes

kalafut approved these changes May 1, 2019

View reviewed changes

kalafut merged commit fdcf68d into hashicorp:master May 1, 2019

briankassouf pushed a commit that referenced this pull request Jun 4, 2019

DynamoDB: Make Unlock key delete conditional on being old leader's (#…

d931d70

…6637)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EdwinRobbins commented Apr 25, 2019

hashicorp-cla commented Apr 25, 2019 •

edited

Loading

kalafut commented Apr 25, 2019

EdwinRobbins commented Apr 26, 2019

kalafut left a comment

kalafut Apr 26, 2019

kalafut Apr 26, 2019 •

edited

Loading

kalafut Apr 26, 2019

EdwinRobbins Apr 29, 2019

kalafut Apr 30, 2019

EdwinRobbins Apr 30, 2019

kalafut Apr 30, 2019

EdwinRobbins Apr 30, 2019

kalafut Apr 26, 2019

jefferai Apr 30, 2019

EdwinRobbins Apr 30, 2019

kalafut commented Apr 30, 2019

kalafut commented May 1, 2019

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

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

Conversation

EdwinRobbins commented Apr 25, 2019

hashicorp-cla commented Apr 25, 2019 • edited Loading

kalafut commented Apr 25, 2019

EdwinRobbins commented Apr 26, 2019

kalafut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut commented Apr 30, 2019

kalafut commented May 1, 2019

hashicorp-cla commented Apr 25, 2019 •

edited

Loading

kalafut Apr 26, 2019 •

edited

Loading