From 4ff90013cd74235007fce17ae102cf28dfc40a2c Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Thu, 20 Jun 2019 10:31:31 -0400 Subject: [PATCH] Use a role cache to avoid separate locking paths (#6926) * Use a role cache to avoid separate locking paths Due to the various locked/nonlocked paths we had a case where we weren't always checking for secondary status before trying to upgrade. This broadly simplifies things by using a cache to store the current role values (avoiding a lot of storage hits) and updating the cache on any write, delete, or invalidation. --- builtin/credential/aws/backend.go | 12 +- builtin/credential/aws/backend_test.go | 4 +- builtin/credential/aws/path_login.go | 8 +- builtin/credential/aws/path_login_test.go | 2 +- builtin/credential/aws/path_role.go | 172 ++++++++---------- builtin/credential/aws/path_role_tag.go | 4 +- builtin/credential/aws/path_role_test.go | 57 +++++- .../credential/aws/path_roletag_blacklist.go | 2 +- 8 files changed, 148 insertions(+), 113 deletions(-) diff --git a/builtin/credential/aws/backend.go b/builtin/credential/aws/backend.go index b9c124da81ae..0209e6773c05 100644 --- a/builtin/credential/aws/backend.go +++ b/builtin/credential/aws/backend.go @@ -3,6 +3,7 @@ package awsauth import ( "context" "fmt" + "strings" "sync" "time" @@ -76,6 +77,9 @@ type backend struct { // accounts using their IAM instance profile to get their credentials. defaultAWSAccountID string + // roleCache caches role entries to avoid locking headaches + roleCache *cache.Cache + resolveArnToUniqueIDFunc func(context.Context, logical.Storage, string) (string, error) } @@ -89,6 +93,7 @@ func Backend(conf *logical.BackendConfig) (*backend, error) { iamUserIdToArnCache: cache.New(7*24*time.Hour, 24*time.Hour), tidyBlacklistCASGuard: new(uint32), tidyWhitelistCASGuard: new(uint32), + roleCache: cache.New(cache.NoExpiration, cache.NoExpiration), } b.resolveArnToUniqueIDFunc = b.resolveArnToRealUniqueId @@ -201,13 +206,16 @@ func (b *backend) periodicFunc(ctx context.Context, req *logical.Request) error } func (b *backend) invalidate(ctx context.Context, key string) { - switch key { - case "config/client": + switch { + case key == "config/client": b.configMutex.Lock() defer b.configMutex.Unlock() b.flushCachedEC2Clients() b.flushCachedIAMClients() b.defaultAWSAccountID = "" + case strings.HasPrefix(key, "role"): + // TODO: We could make this better + b.roleCache.Flush() } } diff --git a/builtin/credential/aws/backend_test.go b/builtin/credential/aws/backend_test.go index 030bed7578b6..b8b947350b31 100644 --- a/builtin/credential/aws/backend_test.go +++ b/builtin/credential/aws/backend_test.go @@ -60,7 +60,7 @@ func TestBackend_CreateParseVerifyRoleTag(t *testing.T) { } // read the created role entry - roleEntry, err := b.lockedAWSRole(context.Background(), storage, "abcd-123") + roleEntry, err := b.role(context.Background(), storage, "abcd-123") if err != nil { t.Fatal(err) } @@ -127,7 +127,7 @@ func TestBackend_CreateParseVerifyRoleTag(t *testing.T) { } // get the entry of the newly created role entry - roleEntry2, err := b.lockedAWSRole(context.Background(), storage, "ami-6789") + roleEntry2, err := b.role(context.Background(), storage, "ami-6789") if err != nil { t.Fatal(err) } diff --git a/builtin/credential/aws/path_login.go b/builtin/credential/aws/path_login.go index 07168bfa1dbc..fa68e8baf436 100644 --- a/builtin/credential/aws/path_login.go +++ b/builtin/credential/aws/path_login.go @@ -597,7 +597,7 @@ func (b *backend) pathLoginUpdateEc2(ctx context.Context, req *logical.Request, } // Get the entry for the role used by the instance - roleEntry, err := b.lockedAWSRole(ctx, req.Storage, roleName) + roleEntry, err := b.role(ctx, req.Storage, roleName) if err != nil { return nil, err } @@ -951,7 +951,7 @@ func (b *backend) pathLoginRenewIam(ctx context.Context, req *logical.Request, d if roleName == "" { return nil, fmt.Errorf("error retrieving role_name during renewal") } - roleEntry, err := b.lockedAWSRole(ctx, req.Storage, roleName) + roleEntry, err := b.role(ctx, req.Storage, roleName) if err != nil { return nil, err } @@ -1079,7 +1079,7 @@ func (b *backend) pathLoginRenewEc2(ctx context.Context, req *logical.Request, d } // Ensure that role entry is not deleted - roleEntry, err := b.lockedAWSRole(ctx, req.Storage, storedIdentity.Role) + roleEntry, err := b.role(ctx, req.Storage, storedIdentity.Role) if err != nil { return nil, err } @@ -1205,7 +1205,7 @@ func (b *backend) pathLoginUpdateIam(ctx context.Context, req *logical.Request, roleName = entity.FriendlyName } - roleEntry, err := b.lockedAWSRole(ctx, req.Storage, roleName) + roleEntry, err := b.role(ctx, req.Storage, roleName) if err != nil { return nil, err } diff --git a/builtin/credential/aws/path_login_test.go b/builtin/credential/aws/path_login_test.go index a493770bc897..b6adf627f996 100644 --- a/builtin/credential/aws/path_login_test.go +++ b/builtin/credential/aws/path_login_test.go @@ -219,7 +219,7 @@ func TestBackend_pathLogin_IAMHeaders(t *testing.T) { AuthType: iamAuthType, } - if err := b.nonLockedSetAWSRole(context.Background(), storage, testValidRoleName, roleEntry); err != nil { + if err := b.setRole(context.Background(), storage, testValidRoleName, roleEntry); err != nil { t.Fatalf("failed to set entry: %s", err) } diff --git a/builtin/credential/aws/path_role.go b/builtin/credential/aws/path_role.go index d1b5545b6548..992d2e62b809 100644 --- a/builtin/credential/aws/path_role.go +++ b/builtin/credential/aws/path_role.go @@ -2,6 +2,7 @@ package awsauth import ( "context" + "errors" "fmt" "strings" "time" @@ -12,6 +13,7 @@ import ( "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/helper/policyutil" "github.com/hashicorp/vault/sdk/logical" + "github.com/mitchellh/copystructure" ) var ( @@ -218,82 +220,83 @@ func pathListRoles(b *backend) *framework.Path { // Establishes dichotomy of request operation between CreateOperation and UpdateOperation. // Returning 'true' forces an UpdateOperation, CreateOperation otherwise. func (b *backend) pathRoleExistenceCheck(ctx context.Context, req *logical.Request, data *framework.FieldData) (bool, error) { - entry, err := b.lockedAWSRole(ctx, req.Storage, strings.ToLower(data.Get("role").(string))) + entry, err := b.role(ctx, req.Storage, strings.ToLower(data.Get("role").(string))) if err != nil { return false, err } return entry != nil, nil } -// lockedAWSRole returns the properties set on the given role. This method -// acquires the read lock before reading the role from the storage. -func (b *backend) lockedAWSRole(ctx context.Context, s logical.Storage, roleName string) (*awsRoleEntry, error) { +// role fetches the role entry from cache, or loads from disk if necessary +func (b *backend) role(ctx context.Context, s logical.Storage, roleName string) (*awsRoleEntry, error) { if roleName == "" { return nil, fmt.Errorf("missing role name") } - b.roleMutex.RLock() - roleEntry, err := b.nonLockedAWSRole(ctx, s, roleName) - // we manually unlock rather than defer the unlock because we might need to grab - // a read/write lock in the upgrade path - b.roleMutex.RUnlock() + roleEntryRaw, found := b.roleCache.Get(roleName) + if found && roleEntryRaw != nil { + roleEntry, ok := roleEntryRaw.(*awsRoleEntry) + if !ok { + return nil, errors.New("could not convert role entry internally") + } + if roleEntry == nil { + return nil, errors.New("converted role entry is nil") + } + } + + // Not found, or was nil + b.roleMutex.Lock() + defer b.roleMutex.Unlock() + + return b.roleInternal(ctx, s, roleName) +} + +// roleInternal does not perform locking, and rechecks the cache, going to disk if necessar +func (b *backend) roleInternal(ctx context.Context, s logical.Storage, roleName string) (*awsRoleEntry, error) { + // Check cache again now that we have the lock + roleEntryRaw, found := b.roleCache.Get(roleName) + if found && roleEntryRaw != nil { + roleEntry, ok := roleEntryRaw.(*awsRoleEntry) + if !ok { + return nil, errors.New("could not convert role entry internally") + } + if roleEntry == nil { + return nil, errors.New("converted role entry is nil") + } + } + + // Fetch from storage + entry, err := s.Get(ctx, "role/"+strings.ToLower(roleName)) if err != nil { return nil, err } - if roleEntry == nil { + if entry == nil { return nil, nil } - needUpgrade, err := b.upgradeRoleEntry(ctx, s, roleEntry) + + result := new(awsRoleEntry) + if err := entry.DecodeJSON(result); err != nil { + return nil, err + } + + needUpgrade, err := b.upgradeRole(ctx, s, result) if err != nil { return nil, errwrap.Wrapf("error upgrading roleEntry: {{err}}", err) } if needUpgrade && (b.System().LocalMount() || !b.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary|consts.ReplicationPerformanceStandby)) { - b.roleMutex.Lock() - defer b.roleMutex.Unlock() - // Now that we have a R/W lock, we need to re-read the role entry in case it was - // written to between releasing the read lock and acquiring the write lock - roleEntry, err = b.nonLockedAWSRole(ctx, s, roleName) - if err != nil { - return nil, err - } - // somebody deleted the role, so no use in putting it back - if roleEntry == nil { - return nil, nil + if err = b.setRole(ctx, s, roleName, result); err != nil { + return nil, errwrap.Wrapf("error saving upgraded roleEntry: {{err}}", err) } - // now re-check to see if we need to upgrade - if needUpgrade, err = b.upgradeRoleEntry(ctx, s, roleEntry); err != nil { - return nil, errwrap.Wrapf("error upgrading roleEntry: {{err}}", err) - } - if needUpgrade { - if err = b.nonLockedSetAWSRole(ctx, s, roleName, roleEntry); err != nil { - return nil, errwrap.Wrapf("error saving upgraded roleEntry: {{err}}", err) - } - } - } - return roleEntry, nil -} - -// lockedSetAWSRole creates or updates a role in the storage. This method -// acquires the write lock before creating or updating the role at the storage. -func (b *backend) lockedSetAWSRole(ctx context.Context, s logical.Storage, roleName string, roleEntry *awsRoleEntry) error { - if roleName == "" { - return fmt.Errorf("missing role name") } - if roleEntry == nil { - return fmt.Errorf("nil role entry") - } + b.roleCache.SetDefault(roleName, result) - b.roleMutex.Lock() - defer b.roleMutex.Unlock() - - return b.nonLockedSetAWSRole(ctx, s, roleName, roleEntry) + return result, nil } -// nonLockedSetAWSRole creates or updates a role in the storage. This method -// does not acquire the write lock before reading the role from the storage. If -// locking is desired, use lockedSetAWSRole instead. -func (b *backend) nonLockedSetAWSRole(ctx context.Context, s logical.Storage, roleName string, +// setRole creates or updates a role in the storage. The caller must hold +// the write lock. +func (b *backend) setRole(ctx context.Context, s logical.Storage, roleName string, roleEntry *awsRoleEntry) error { if roleName == "" { return fmt.Errorf("missing role name") @@ -312,12 +315,14 @@ func (b *backend) nonLockedSetAWSRole(ctx context.Context, s logical.Storage, ro return err } + b.roleCache.SetDefault(roleName, roleEntry) + return nil } // If needed, updates the role entry and returns a bool indicating if it was updated // (and thus needs to be persisted) -func (b *backend) upgradeRoleEntry(ctx context.Context, s logical.Storage, roleEntry *awsRoleEntry) (bool, error) { +func (b *backend) upgradeRole(ctx context.Context, s logical.Storage, roleEntry *awsRoleEntry) (bool, error) { if roleEntry == nil { return false, fmt.Errorf("received nil roleEntry") } @@ -421,33 +426,6 @@ func (b *backend) upgradeRoleEntry(ctx context.Context, s logical.Storage, roleE return upgraded, nil } -// nonLockedAWSRole returns the properties set on the given role. This method -// does not acquire the read lock before reading the role from the storage. If -// locking is desired, use lockedAWSRole instead. -// This method also does NOT check to see if a role upgrade is required. It is -// the responsibility of the caller to check if a role upgrade is required and, -// if so, to upgrade the role -func (b *backend) nonLockedAWSRole(ctx context.Context, s logical.Storage, roleName string) (*awsRoleEntry, error) { - if roleName == "" { - return nil, fmt.Errorf("missing role name") - } - - entry, err := s.Get(ctx, "role/"+strings.ToLower(roleName)) - if err != nil { - return nil, err - } - if entry == nil { - return nil, nil - } - - var result awsRoleEntry - if err := entry.DecodeJSON(&result); err != nil { - return nil, err - } - - return &result, nil -} - // pathRoleDelete is used to delete the information registered for a given AMI ID. func (b *backend) pathRoleDelete(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { roleName := data.Get("role").(string) @@ -458,24 +436,29 @@ func (b *backend) pathRoleDelete(ctx context.Context, req *logical.Request, data b.roleMutex.Lock() defer b.roleMutex.Unlock() - return nil, req.Storage.Delete(ctx, "role/"+strings.ToLower(roleName)) + err := req.Storage.Delete(ctx, "role/"+strings.ToLower(roleName)) + if err != nil { + return nil, errwrap.Wrapf("error deleting role: {{err}}", err) + } + + b.roleCache.Delete(roleName) + + return nil, nil } // pathRoleList is used to list all the AMI IDs registered with Vault. func (b *backend) pathRoleList(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - b.roleMutex.RLock() - defer b.roleMutex.RUnlock() - roles, err := req.Storage.List(ctx, "role/") if err != nil { return nil, err } + return logical.ListResponse(roles), nil } // pathRoleRead is used to view the information registered for a given AMI ID. func (b *backend) pathRoleRead(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - roleEntry, err := b.lockedAWSRole(ctx, req.Storage, strings.ToLower(data.Get("role").(string))) + roleEntry, err := b.role(ctx, req.Storage, strings.ToLower(data.Get("role").(string))) if err != nil { return nil, err } @@ -498,7 +481,10 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request b.roleMutex.Lock() defer b.roleMutex.Unlock() - roleEntry, err := b.nonLockedAWSRole(ctx, req.Storage, roleName) + // We use the internal one here to ensure that we have fresh data and + // nobody else is concurrently modifying. This will also call the upgrade + // path on existing role entries. + roleEntry, err := b.roleInternal(ctx, req.Storage, roleName) if err != nil { return nil, err } @@ -512,16 +498,14 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request Version: currentRoleStorageVersion, } } else { - needUpdate, err := b.upgradeRoleEntry(ctx, req.Storage, roleEntry) + // We want to always use a copy so we aren't modifying items in the + // version in the cache while other users may be looking it up (or if + // we fail somewhere) + cp, err := copystructure.Copy(roleEntry) if err != nil { - return logical.ErrorResponse(fmt.Sprintf("failed to update roleEntry: %v", err)), nil - } - if needUpdate { - err = b.nonLockedSetAWSRole(ctx, req.Storage, roleName, roleEntry) - if err != nil { - return logical.ErrorResponse(fmt.Sprintf("failed to save upgraded roleEntry: %v", err)), nil - } + return nil, err } + roleEntry = cp.(*awsRoleEntry) } // Fetch and set the bound parameters. There can't be default values @@ -808,7 +792,7 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request } } - if err := b.nonLockedSetAWSRole(ctx, req.Storage, roleName, roleEntry); err != nil { + if err := b.setRole(ctx, req.Storage, roleName, roleEntry); err != nil { return nil, err } diff --git a/builtin/credential/aws/path_role_tag.go b/builtin/credential/aws/path_role_tag.go index 1b011edc5856..73b1e33d816e 100644 --- a/builtin/credential/aws/path_role_tag.go +++ b/builtin/credential/aws/path_role_tag.go @@ -77,7 +77,7 @@ func (b *backend) pathRoleTagUpdate(ctx context.Context, req *logical.Request, d } // Fetch the role entry - roleEntry, err := b.lockedAWSRole(ctx, req.Storage, roleName) + roleEntry, err := b.role(ctx, req.Storage, roleName) if err != nil { return nil, err } @@ -349,7 +349,7 @@ func (b *backend) parseAndVerifyRoleTagValue(ctx context.Context, s logical.Stor return nil, fmt.Errorf("missing role name") } - roleEntry, err := b.lockedAWSRole(ctx, s, rTag.Role) + roleEntry, err := b.role(ctx, s, rTag.Role) if err != nil { return nil, err } diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 150f32316997..a593d459685f 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -591,7 +591,6 @@ func TestAwsEc2_RoleCrud(t *testing.T) { } roleReq.Operation = logical.ReadOperation - resp, err = b.HandleRequest(context.Background(), roleReq) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("resp: %#v, err: %v", resp, err) @@ -625,42 +624,86 @@ func TestAwsEc2_RoleCrud(t *testing.T) { t.Fatal("role_id not found in repsonse") } expected["role_id"] = resp.Data["role_id"] - if diff := deep.Equal(expected, resp.Data); diff != nil { t.Fatal(diff) } roleData["bound_vpc_id"] = "newvpcid" roleReq.Operation = logical.UpdateOperation - resp, err = b.HandleRequest(context.Background(), roleReq) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("resp: %#v, err: %v", resp, err) } roleReq.Operation = logical.ReadOperation - resp, err = b.HandleRequest(context.Background(), roleReq) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("resp: %#v, err: %v", resp, err) } - expected["bound_vpc_id"] = []string{"newvpcid"} + if !reflect.DeepEqual(expected, resp.Data) { + t.Fatalf("bad: role data: expected: %#v\n actual: %#v", expected, resp.Data) + } + + // Create a new backend so we have a new cache (thus populating from disk). + // Then test reading (reading from disk + lock), writing, reading, + // deleting, reading. + b, err = Backend(config) + if err != nil { + t.Fatal(err) + } + err = b.Setup(context.Background(), config) + if err != nil { + t.Fatal(err) + } + + // Read again, make sure things are what we expect + resp, err = b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } if !reflect.DeepEqual(expected, resp.Data) { t.Fatalf("bad: role data: expected: %#v\n actual: %#v", expected, resp.Data) } - roleReq.Operation = logical.DeleteOperation + roleReq.Operation = logical.UpdateOperation + roleData["bound_ami_id"] = "testamiid2" + resp, err = b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } + roleReq.Operation = logical.ReadOperation resp, err = b.HandleRequest(context.Background(), roleReq) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("resp: %#v, err: %v", resp, err) } + expected["bound_ami_id"] = []string{"testamiid2"} + if diff := deep.Equal(expected, resp.Data); diff != nil { + t.Fatal(diff) + } + + // Delete which should remove from disk and also cache + roleReq.Operation = logical.DeleteOperation + resp, err = b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } if resp != nil { t.Fatalf("failed to delete role entry") } + + // Verify it was deleted, e.g. it isn't found in the role cache + roleReq.Operation = logical.ReadOperation + resp, err = b.HandleRequest(context.Background(), roleReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("resp: %#v, err: %v", resp, err) + } + if resp != nil { + t.Fatal("expected nil") + } } func TestAwsEc2_RoleDurationSeconds(t *testing.T) { @@ -742,7 +785,7 @@ func TestRoleEntryUpgradeV(t *testing.T) { Version: currentRoleStorageVersion, } - upgraded, err := b.upgradeRoleEntry(context.Background(), storage, roleEntryToUpgrade) + upgraded, err := b.upgradeRole(context.Background(), storage, roleEntryToUpgrade) if err != nil { t.Fatalf("error upgrading role entry: %#v", err) } diff --git a/builtin/credential/aws/path_roletag_blacklist.go b/builtin/credential/aws/path_roletag_blacklist.go index 88b79c388a8f..b87f4f58dc09 100644 --- a/builtin/credential/aws/path_roletag_blacklist.go +++ b/builtin/credential/aws/path_roletag_blacklist.go @@ -163,7 +163,7 @@ func (b *backend) pathRoletagBlacklistUpdate(ctx context.Context, req *logical.R } // Get the entry for the role mentioned in the role tag. - roleEntry, err := b.lockedAWSRole(ctx, req.Storage, rTag.Role) + roleEntry, err := b.role(ctx, req.Storage, rTag.Role) if err != nil { return nil, err }