From a95cf0205350b2b37b36514abbcfb0901b0461f0 Mon Sep 17 00:00:00 2001 From: Edwin Robbins Date: Tue, 23 Apr 2019 14:04:32 -0700 Subject: [PATCH 1/7] Make Unlock key delete conditional on being old leader's --- physical/dynamodb/dynamodb.go | 29 +++++++++++++++++++++++++---- vault/ha.go | 4 +++- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/physical/dynamodb/dynamodb.go b/physical/dynamodb/dynamodb.go index c827ea2595c5..0e800cafc83c 100644 --- a/physical/dynamodb/dynamodb.go +++ b/physical/dynamodb/dynamodb.go @@ -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))}, + "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, @@ -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) + } case <-done: ticker.Stop() return diff --git a/vault/ha.go b/vault/ha.go index ee6fe18a5849..386f03ad83a4 100644 --- a/vault/ha.go +++ b/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) + } c.heldHALock = nil } From 54d8c25a9e225dc0e99c740524eab602b07e6839 Mon Sep 17 00:00:00 2001 From: Edwin Robbins Date: Fri, 26 Apr 2019 13:59:09 -0700 Subject: [PATCH 2/7] Tidy up types and error usage --- physical/dynamodb/dynamodb.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/physical/dynamodb/dynamodb.go b/physical/dynamodb/dynamodb.go index 0e800cafc83c..243d17b02aaf 100644 --- a/physical/dynamodb/dynamodb.go +++ b/physical/dynamodb/dynamodb.go @@ -654,7 +654,7 @@ func (l *DynamoDBLock) periodicallyRenewLock(done chan struct{}) { // This should not renew the lock if the lock was deleted from under you. err := l.updateItem(false) if err != nil { - l.backend.logger.Error("error renewing leadership lock", err) + l.backend.logger.Error("error renewing leadership lock", "error", err) } case <-done: ticker.Stop() @@ -686,8 +686,8 @@ func (l *DynamoDBLock) updateItem(createIfMissing bool) error { _, err := l.backend.client.UpdateItem(&dynamodb.UpdateItemInput{ TableName: aws.String(l.backend.table), Key: map[string]*dynamodb.AttributeValue{ - "Path": &dynamodb.AttributeValue{S: aws.String(recordPathForVaultKey(l.key))}, - "Key": &dynamodb.AttributeValue{S: aws.String(recordKeyForVaultKey(l.key))}, + "Path": {S: aws.String(recordPathForVaultKey(l.key))}, + "Key": {S: aws.String(recordKeyForVaultKey(l.key))}, }, UpdateExpression: aws.String("SET #value=:value, #identity=:identity, #expires=:expires"), // If both key and path already exist, we can only write if @@ -703,10 +703,10 @@ func (l *DynamoDBLock) updateItem(createIfMissing bool) error { "#value": aws.String("Value"), }, ExpressionAttributeValues: map[string]*dynamodb.AttributeValue{ - ":identity": &dynamodb.AttributeValue{B: []byte(l.identity)}, - ":value": &dynamodb.AttributeValue{B: []byte(l.value)}, - ":now": &dynamodb.AttributeValue{N: aws.String(strconv.FormatInt(now.UnixNano(), 10))}, - ":expires": &dynamodb.AttributeValue{N: aws.String(strconv.FormatInt(now.Add(l.ttl).UnixNano(), 10))}, + ":identity": {B: []byte(l.identity)}, + ":value": {B: []byte(l.value)}, + ":now": {N: aws.String(strconv.FormatInt(now.UnixNano(), 10))}, + ":expires": {N: aws.String(strconv.FormatInt(now.Add(l.ttl).UnixNano(), 10))}, }, }) return err From 01bfce854e3690ecac11c6689394473d5c007488 Mon Sep 17 00:00:00 2001 From: Edwin Robbins Date: Mon, 29 Apr 2019 13:20:44 -0700 Subject: [PATCH 3/7] Catch conditional check error for Unlock() --- vault/ha.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/vault/ha.go b/vault/ha.go index 386f03ad83a4..721340ca5cab 100644 --- a/vault/ha.go +++ b/vault/ha.go @@ -6,6 +6,8 @@ import ( "crypto/x509" "errors" "fmt" + "github.com/aws/aws-sdk-go/aws/awserr" + "github.com/aws/aws-sdk-go/service/dynamodb" "sync/atomic" "time" @@ -577,7 +579,14 @@ func (c *Core) waitForLeadership(newLeaderCh chan func(), manualStepDownCh, stop } if err := c.heldHALock.Unlock(); err != nil { - c.logger.Error("unlocking HA lock failed", "error", err) + if err, ok := err.(awserr.Error); ok { + // Catch condition check failure, for case where unlock is called after + // new leader has already assumed the key ownership + if err.Code() != dynamodb.ErrCodeConditionalCheckFailedException { + c.logger.Error("unlocking HA lock failed", "error", err) + } + } + } c.heldHALock = nil } From 60b0e89fa116ca0979329d25697a68033ef60172 Mon Sep 17 00:00:00 2001 From: Edwin Robbins Date: Tue, 30 Apr 2019 11:36:12 -0700 Subject: [PATCH 4/7] Update error handling for conditional errors Push logic into dynamodb to keep specific logic contained to it. Centralize filtering and add to updateItem and Unlock --- physical/dynamodb/dynamodb.go | 25 +++++++++++++++++-------- vault/ha.go | 9 +-------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/physical/dynamodb/dynamodb.go b/physical/dynamodb/dynamodb.go index 243d17b02aaf..b42d523d9109 100644 --- a/physical/dynamodb/dynamodb.go +++ b/physical/dynamodb/dynamodb.go @@ -591,8 +591,7 @@ func (l *DynamoDBLock) Unlock() error { } _, err := l.backend.client.DeleteItem(deleteMyLock) - - return err + return filterError(err) } // Value checks whether or not the lock is held by any instance of DynamoDBLock, @@ -627,11 +626,7 @@ func (l *DynamoDBLock) tryToLock(stop, success chan struct{}, errors chan error) err := l.updateItem(true) if err != nil { if err, ok := err.(awserr.Error); ok { - // Don't report a condition check failure, this means that the lock - // is already being held. - if err.Code() != dynamodb.ErrCodeConditionalCheckFailedException { - errors <- err - } + errors <- err } else { // Its not an AWS error, and is probably not transient, bail out. errors <- err @@ -709,7 +704,8 @@ func (l *DynamoDBLock) updateItem(createIfMissing bool) error { ":expires": {N: aws.String(strconv.FormatInt(now.Add(l.ttl).UnixNano(), 10))}, }, }) - return err + + return filterError(err) } // watch checks whether the lock has changed in the @@ -852,3 +848,16 @@ func unescapeEmptyPath(s string) string { } return s } + +// filterError filters out expected errors which are expected +// under normal operation. +func filterError(err error) error{ + if err, ok := err.(awserr.Error); ok { + // Don't report a condition check failure, this means that the lock + // is already being held by another vault + if err.Code() == dynamodb.ErrCodeConditionalCheckFailedException { + return nil + } + } + return err +} diff --git a/vault/ha.go b/vault/ha.go index 721340ca5cab..0a9189ce7c2a 100644 --- a/vault/ha.go +++ b/vault/ha.go @@ -579,14 +579,7 @@ func (c *Core) waitForLeadership(newLeaderCh chan func(), manualStepDownCh, stop } if err := c.heldHALock.Unlock(); err != nil { - if err, ok := err.(awserr.Error); ok { - // Catch condition check failure, for case where unlock is called after - // new leader has already assumed the key ownership - if err.Code() != dynamodb.ErrCodeConditionalCheckFailedException { - c.logger.Error("unlocking HA lock failed", "error", err) - } - } - + c.logger.Error("unlocking HA lock failed", "error", err) } c.heldHALock = nil } From 3e2439216bbe8aec6bd49bbf298d6cb964d82987 Mon Sep 17 00:00:00 2001 From: Edwin Robbins Date: Tue, 30 Apr 2019 13:15:00 -0700 Subject: [PATCH 5/7] Remove no unused imports --- vault/ha.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/vault/ha.go b/vault/ha.go index 0a9189ce7c2a..386f03ad83a4 100644 --- a/vault/ha.go +++ b/vault/ha.go @@ -6,8 +6,6 @@ import ( "crypto/x509" "errors" "fmt" - "github.com/aws/aws-sdk-go/aws/awserr" - "github.com/aws/aws-sdk-go/service/dynamodb" "sync/atomic" "time" From 0aba117497edf485bee31cd4f5dec43c057d72bb Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Tue, 30 Apr 2019 14:14:47 -0700 Subject: [PATCH 6/7] Replace filterError with an error check --- physical/dynamodb/dynamodb.go | 43 ++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/physical/dynamodb/dynamodb.go b/physical/dynamodb/dynamodb.go index b42d523d9109..99b350a2e124 100644 --- a/physical/dynamodb/dynamodb.go +++ b/physical/dynamodb/dynamodb.go @@ -174,7 +174,7 @@ func NewDynamoDBBackend(conf map[string]string, logger log.Logger) (physical.Bac if dynamodbMaxRetryString == "" { dynamodbMaxRetryString = conf["dynamodb_max_retries"] } - var dynamodbMaxRetry int = aws.UseServiceDefaultRetries + var dynamodbMaxRetry = aws.UseServiceDefaultRetries if dynamodbMaxRetryString != "" { var err error dynamodbMaxRetry, err = strconv.Atoi(dynamodbMaxRetryString) @@ -579,19 +579,23 @@ func (l *DynamoDBLock) Unlock() error { TableName: &l.backend.table, ConditionExpression: &condition, Key: map[string]*dynamodb.AttributeValue{ - "Path": &dynamodb.AttributeValue{S: aws.String(recordPathForVaultKey(l.key))}, - "Key": &dynamodb.AttributeValue{S: aws.String(recordKeyForVaultKey(l.key))}, + "Path": {S: aws.String(recordPathForVaultKey(l.key))}, + "Key": {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)}, + ":identity": {B: []byte(l.identity)}, }, } - _, err := l.backend.client.DeleteItem(deleteMyLock) - return filterError(err) + _, err := l.backend.client.DeleteItem(deleteMyLock) + if isConditionCheckFailed(err) { + err = nil + } + + return err } // Value checks whether or not the lock is held by any instance of DynamoDBLock, @@ -626,7 +630,9 @@ func (l *DynamoDBLock) tryToLock(stop, success chan struct{}, errors chan error) err := l.updateItem(true) if err != nil { if err, ok := err.(awserr.Error); ok { - errors <- err + if !isConditionCheckFailed(err) { + errors <- err + } } else { // Its not an AWS error, and is probably not transient, bail out. errors <- err @@ -649,7 +655,9 @@ func (l *DynamoDBLock) periodicallyRenewLock(done chan struct{}) { // This should not renew the lock if the lock was deleted from under you. err := l.updateItem(false) if err != nil { - l.backend.logger.Error("error renewing leadership lock", "error", err) + if !isConditionCheckFailed(err) { + l.backend.logger.Error("error renewing leadership lock", "error", err) + } } case <-done: ticker.Stop() @@ -705,7 +713,7 @@ func (l *DynamoDBLock) updateItem(createIfMissing bool) error { }, }) - return filterError(err) + return err } // watch checks whether the lock has changed in the @@ -849,15 +857,14 @@ func unescapeEmptyPath(s string) string { return s } -// filterError filters out expected errors which are expected -// under normal operation. -func filterError(err error) error{ - if err, ok := err.(awserr.Error); ok { - // Don't report a condition check failure, this means that the lock - // is already being held by another vault - if err.Code() == dynamodb.ErrCodeConditionalCheckFailedException { - return nil +// isConditionCheckFailed tests whether err is an ErrCodeConditionalCheckFailedException +// from the AWS SDK. +func isConditionCheckFailed(err error) bool { + if err != nil { + if err, ok := err.(awserr.Error); ok { + return err.Code() == dynamodb.ErrCodeConditionalCheckFailedException } } - return err + + return false } From a81fa016d9c4536ed51b4a35767deeec3a563bde Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Tue, 30 Apr 2019 14:23:17 -0700 Subject: [PATCH 7/7] Restore comment --- physical/dynamodb/dynamodb.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/physical/dynamodb/dynamodb.go b/physical/dynamodb/dynamodb.go index 99b350a2e124..54a689b68fda 100644 --- a/physical/dynamodb/dynamodb.go +++ b/physical/dynamodb/dynamodb.go @@ -630,6 +630,8 @@ func (l *DynamoDBLock) tryToLock(stop, success chan struct{}, errors chan error) err := l.updateItem(true) if err != nil { if err, ok := err.(awserr.Error); ok { + // Don't report a condition check failure, this means that the lock + // is already being held. if !isConditionCheckFailed(err) { errors <- err }