From 147743b0ff61897348feebb766f589225f41355d Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 18 Jan 2019 15:53:56 -0500 Subject: [PATCH 01/14] Merge entities during unseal only on the primary --- vault/identity_store_test.go | 152 ++++++++++++++++++++++++++--------- vault/identity_store_util.go | 3 +- 2 files changed, 115 insertions(+), 40 deletions(-) diff --git a/vault/identity_store_test.go b/vault/identity_store_test.go index ae8a23f86a1a..347c67b5b4cd 100644 --- a/vault/identity_store_test.go +++ b/vault/identity_store_test.go @@ -15,6 +15,98 @@ import ( "github.com/hashicorp/vault/logical" ) +func TestIdentityStore_UnsealingWhenConflictingAliasNames(t *testing.T) { + err := AddTestCredentialBackend("github", credGithub.Factory) + if err != nil { + t.Fatalf("err: %s", err) + } + + c, unsealKey, root := TestCoreUnsealed(t) + + meGH := &MountEntry{ + Table: credentialTableType, + Path: "github/", + Type: "github", + Description: "github auth", + } + + err = c.enableCredential(namespace.RootContext(nil), meGH) + if err != nil { + t.Fatal(err) + } + + alias := &identity.Alias{ + ID: "alias1", + CanonicalID: "entity1", + MountType: "github", + MountAccessor: meGH.Accessor, + Name: "githubuser", + } + entity := &identity.Entity{ + ID: "entity1", + Name: "name1", + Policies: []string{"foo", "bar"}, + Aliases: []*identity.Alias{ + alias, + }, + NamespaceID: namespace.RootNamespaceID, + } + entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID) + + err = c.identityStore.upsertEntity(namespace.RootContext(nil), entity, nil, true) + if err != nil { + t.Fatal(err) + } + + alias2 := &identity.Alias{ + ID: "alias2", + CanonicalID: "entity2", + MountType: "github", + MountAccessor: meGH.Accessor, + Name: "GITHUBUSER", + } + entity2 := &identity.Entity{ + ID: "entity2", + Name: "name2", + Policies: []string{"foo", "bar"}, + Aliases: []*identity.Alias{ + alias2, + }, + NamespaceID: namespace.RootNamespaceID, + } + entity2.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity2.ID) + + // Persist the second entity directly without the regular flow. This will skip + // merging of these enties. + entity2Any, err := ptypes.MarshalAny(entity2) + if err != nil { + t.Fatal(err) + } + item := &storagepacker.Item{ + ID: entity2.ID, + Message: entity2Any, + } + if err = c.identityStore.entityPacker.PutItem(item); err != nil { + t.Fatal(err) + } + + // Seal and ensure that unseal works + if err = c.Seal(root); err != nil { + t.Fatal(err) + } + + var unsealed bool + for i := 0; i < 3; i++ { + unsealed, err = c.Unseal(unsealKey[i]) + if err != nil { + t.Fatal(err) + } + } + if !unsealed { + t.Fatal("still sealed") + } +} + func TestIdentityStore_EntityIDPassthrough(t *testing.T) { // Enable GitHub auth and initialize ctx := namespace.RootContext(nil) @@ -381,7 +473,7 @@ func TestIdentityStore_MergeConflictingAliases(t *testing.T) { t.Fatalf("err: %s", err) } - c, unsealKey, root := TestCoreUnsealed(t) + c, _, _ := TestCoreUnsealed(t) meGH := &MountEntry{ Table: credentialTableType, @@ -409,54 +501,36 @@ func TestIdentityStore_MergeConflictingAliases(t *testing.T) { Aliases: []*identity.Alias{ alias, }, + NamespaceID: namespace.RootNamespaceID, } entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID) - // Now add the alias to two entities, skipping all existing checking by - // writing directly - entityAny, err := ptypes.MarshalAny(entity) + err = c.identityStore.upsertEntity(namespace.RootContext(nil), entity, nil, true) if err != nil { t.Fatal(err) } - item := &storagepacker.Item{ - ID: entity.ID, - Message: entityAny, - } - if err = c.identityStore.entityPacker.PutItem(item); err != nil { - t.Fatal(err) - } - entity.ID = "entity2" - entity.Name = "name2" - entity.Policies = []string{"bar", "baz"} - alias.ID = "alias2" - alias.CanonicalID = "entity2" - entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID) - entityAny, err = ptypes.MarshalAny(entity) - if err != nil { - t.Fatal(err) - } - item = &storagepacker.Item{ - ID: entity.ID, - Message: entityAny, + alias2 := &identity.Alias{ + ID: "alias2", + CanonicalID: "entity2", + MountType: "github", + MountAccessor: meGH.Accessor, + Name: "githubuser", } - if err = c.identityStore.entityPacker.PutItem(item); err != nil { - t.Fatal(err) + entity2 := &identity.Entity{ + ID: "entity2", + Name: "name2", + Policies: []string{"bar", "baz"}, + Aliases: []*identity.Alias{ + alias2, + }, + NamespaceID: namespace.RootNamespaceID, } - // Seal and unseal. If things are broken, we will now fail to unseal. - if err = c.Seal(root); err != nil { - t.Fatal(err) - } + entity2.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity2.ID) - var unsealed bool - for i := 0; i < 3; i++ { - unsealed, err = c.Unseal(unsealKey[i]) - if err != nil { - t.Fatal(err) - } - } - if !unsealed { - t.Fatal("still sealed") + err = c.identityStore.upsertEntity(namespace.RootContext(nil), entity2, nil, true) + if err != nil { + t.Fatal(err) } newEntity, err := c.identityStore.CreateOrFetchEntity(namespace.RootContext(nil), &logical.Alias{ diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index f186b36f94e8..6a6dd2675d80 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -332,7 +332,7 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // Otherwise it's still tied to previousEntity and fall through // into merging fallthrough - default: + case persist && !i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary): i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true) switch { @@ -341,6 +341,7 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e case intErr != nil: return intErr } + // The entity and aliases will be loaded into memdb and persisted // as a result of the merge so we are done here return nil From b12a5588711c15162cb94b7187af730db8122d0e Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 18 Jan 2019 18:17:19 -0500 Subject: [PATCH 02/14] Add another guard check --- vault/identity_store_util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 6a6dd2675d80..92552466af23 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -349,7 +349,7 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e if strutil.StrListContains(aliasFactors, i.sanitizeName(alias.Name)+alias.MountAccessor) { i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases") - if !i.disableLowerCasedNames { + if !persist && !i.disableLowerCasedNames { return errDuplicateIdentityName } } From dee8f9caa8aaab7352a7991cbf938677620ee6a9 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 1 Feb 2019 16:37:13 -0500 Subject: [PATCH 03/14] Add perf standby to the check --- vault/identity_store_util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 92552466af23..2ce7c52e169b 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -332,7 +332,7 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // Otherwise it's still tied to previousEntity and fall through // into merging fallthrough - case persist && !i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary): + case persist && !i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary|consts.ReplicationPerformanceStandby): i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true) switch { From 162d093f352085ef585f14f628d7c91029961d32 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Mon, 4 Feb 2019 13:27:58 -0500 Subject: [PATCH 04/14] Make primary to not differ from case-insensitivity status w.r.t secondaries --- vault/identity_store_util.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 2ce7c52e169b..999c5d2e17ff 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -332,8 +332,14 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // Otherwise it's still tied to previousEntity and fall through // into merging fallthrough - case persist && !i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary|consts.ReplicationPerformanceStandby): + default: + if !persist { + i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases") + return errDuplicateIdentityName + } + i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) + respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true) switch { case respErr != nil: From da27715954bdfd3f81266e2933e8da325aeb4a8c Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Mon, 4 Feb 2019 13:59:38 -0500 Subject: [PATCH 05/14] Ensure mutual exclusivity between loading and invalidations --- vault/identity_store_util.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 999c5d2e17ff..8c62681ba8ae 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -333,7 +333,20 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // into merging fallthrough default: - if !persist { + // `persist` will be `false` during startup and invalidations. + // During startup, if there are conflicting alias names, log the + // fact and bring up identity store to operate in case-sensitive + // manner by returning errDuplicateIdentityName, and by setting + // disableLowerCasedNames to `true`. This implies that, by the time + // invalidations are happening, only 2 things can happen: 1) + // disableLowerCasedNames is `false` and when that's the case this + // default case will never be hit. 2) This default case will be hit + // but only when disableLowerCasedNames is set to `true`. Hence + // invalidations workflow will execute the `mergeEntity` call while + // the startup workflow will not. That's the desired behavior as + // the `mergeEntity` call will result in a read-only error in + // secondaries during startup causing unseal failure. + if !persist && !i.disableLowerCasedNames { i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases") return errDuplicateIdentityName } From 3fb55b6fcce702566df28b8cb422bcf86ad54611 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Mon, 4 Feb 2019 16:02:09 -0500 Subject: [PATCH 06/14] Both primary and secondaries won't persist during startup and invalidations --- vault/identity_store_util.go | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 8c62681ba8ae..ea239b16ebc6 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -333,22 +333,16 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // into merging fallthrough default: - // `persist` will be `false` during startup and invalidations. - // During startup, if there are conflicting alias names, log the - // fact and bring up identity store to operate in case-sensitive - // manner by returning errDuplicateIdentityName, and by setting - // disableLowerCasedNames to `true`. This implies that, by the time - // invalidations are happening, only 2 things can happen: 1) - // disableLowerCasedNames is `false` and when that's the case this - // default case will never be hit. 2) This default case will be hit - // but only when disableLowerCasedNames is set to `true`. Hence - // invalidations workflow will execute the `mergeEntity` call while - // the startup workflow will not. That's the desired behavior as - // the `mergeEntity` call will result in a read-only error in - // secondaries during startup causing unseal failure. - if !persist && !i.disableLowerCasedNames { + // During startup and invalidations, log the conflicts and don't + // hit the storage, regardless of being primary or secondary. Also, + // ensure that case-insensitivity is assumed only after all the + // conflicts are resolved. + if !persist { i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases") - return errDuplicateIdentityName + if !i.disableLowerCasedNames { + return errDuplicateIdentityName + } + break } i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) From 4df3d3e4769be44056cddfaa395f7b437470d0ce Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Mon, 4 Feb 2019 16:46:17 -0500 Subject: [PATCH 07/14] Allow primary to persist when loading case sensitively --- vault/identity_store_util.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index ea239b16ebc6..4bba47acf684 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -333,16 +333,17 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // into merging fallthrough default: - // During startup and invalidations, log the conflicts and don't - // hit the storage, regardless of being primary or secondary. Also, - // ensure that case-insensitivity is assumed only after all the - // conflicts are resolved. if !persist { i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases") if !i.disableLowerCasedNames { return errDuplicateIdentityName } - break + + // At this point, identity store is operating case-sensitively. + // Persisting is allowed only on the primary. + if i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.ReplicationState().HasState(consts.ReplicationPerformanceStandby) { + break + } } i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) From df847393371f0dc4e4ed098f55308fca4fcf7123 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Mon, 4 Feb 2019 20:13:09 -0500 Subject: [PATCH 08/14] Using core.perfStandby --- vault/identity_store_util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 4bba47acf684..dcab0edfed79 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -341,7 +341,7 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // At this point, identity store is operating case-sensitively. // Persisting is allowed only on the primary. - if i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.ReplicationState().HasState(consts.ReplicationPerformanceStandby) { + if i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby { break } } From 50205132626fb4ed8f3d68f836d9c908114fd733 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 7 Feb 2019 10:46:32 -0500 Subject: [PATCH 09/14] Add a tweak in core for testing --- vault/core.go | 4 ++++ vault/identity_store_util.go | 25 ++++++++++++++++--------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/vault/core.go b/vault/core.go index fb6250fc799b..aa7b4a38b79c 100644 --- a/vault/core.go +++ b/vault/core.go @@ -416,6 +416,10 @@ type Core struct { // Can be toggled atomically to cause the core to never try to become // active, or give up active as soon as it gets it neverBecomeActive *uint32 + + // loadCaseSensitiveIdentityStore enforces the loading of identity store + // artifacts in a case sensitive manner. To be used only in testing. + loadCaseSensitiveIdentityStore bool } // CoreConfig is used to parameterize a core diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index dcab0edfed79..56930d60e886 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -24,28 +24,35 @@ var ( errDuplicateIdentityName = errors.New("duplicate identity name") ) +func (c *Core) SetLoadCaseSensitiveIdentityStore(caseSensitive bool) { + c.loadCaseSensitiveIdentityStore = caseSensitive +} + func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error { if c.identityStore == nil { c.logger.Warn("identity store is not setup, skipping loading") return nil } + var err error loadFunc := func(context.Context) error { - err := c.identityStore.loadEntities(ctx) + err = c.identityStore.loadEntities(ctx) if err != nil { return err } return c.identityStore.loadGroups(ctx) } - // Load everything when memdb is set to operate on lower cased names - err := loadFunc(ctx) - switch { - case err == nil: - // If it succeeds, all is well - return nil - case err != nil && !errwrap.Contains(err, errDuplicateIdentityName.Error()): - return err + if !c.loadCaseSensitiveIdentityStore { + // Load everything when memdb is set to operate on lower cased names + err = loadFunc(ctx) + switch { + case err == nil: + // If it succeeds, all is well + return nil + case err != nil && !errwrap.Contains(err, errDuplicateIdentityName.Error()): + return err + } } c.identityStore.logger.Warn("enabling case sensitive identity names") From 5ac6370210f1ac58fd8de56acfd6694c989b0bec Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 7 Feb 2019 11:50:54 -0500 Subject: [PATCH 10/14] Address review feedback --- vault/identity_store_util.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 56930d60e886..a47071f7c8a3 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -34,9 +34,8 @@ func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error { return nil } - var err error loadFunc := func(context.Context) error { - err = c.identityStore.loadEntities(ctx) + err := c.identityStore.loadEntities(ctx) if err != nil { return err } @@ -45,7 +44,7 @@ func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error { if !c.loadCaseSensitiveIdentityStore { // Load everything when memdb is set to operate on lower cased names - err = loadFunc(ctx) + err := loadFunc(ctx) switch { case err == nil: // If it succeeds, all is well @@ -63,8 +62,7 @@ func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error { // Swap the memdb instance by the one which operates on case sensitive // names, hence obviating the need to unload anything that's already // loaded. - err = c.identityStore.resetDB(ctx) - if err != nil { + if err := c.identityStore.resetDB(ctx); err != nil { return err } From f2d0e83552a36052379ae46d45cecefaab784843 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 7 Feb 2019 13:23:30 -0500 Subject: [PATCH 11/14] update memdb but not storage in secondaries --- vault/identity_store_entities.go | 41 ++++++++++++++++++-------------- vault/identity_store_util.go | 8 +------ 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/vault/identity_store_entities.go b/vault/identity_store_entities.go index 4cfadb6873a1..5ee9a707ac48 100644 --- a/vault/identity_store_entities.go +++ b/vault/identity_store_entities.go @@ -9,6 +9,7 @@ import ( "github.com/golang/protobuf/ptypes" "github.com/hashicorp/errwrap" memdb "github.com/hashicorp/go-memdb" + "github.com/hashicorp/vault/helper/consts" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/storagepacker" @@ -155,7 +156,7 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc { return nil, err } - userErr, intErr := i.mergeEntity(ctx, txn, toEntity, fromEntityIDs, force, true, false) + userErr, intErr := i.mergeEntity(ctx, txn, toEntity, fromEntityIDs, force, true, false, true) if userErr != nil { return logical.ErrorResponse(userErr.Error()), nil } @@ -604,7 +605,7 @@ func (i *IdentityStore) handlePathEntityListCommon(ctx context.Context, req *log return logical.ListResponseWithInfo(keys, entityInfo), nil } -func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntity *identity.Entity, fromEntityIDs []string, force, grabLock, mergePolicies bool) (error, error) { +func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntity *identity.Entity, fromEntityIDs []string, force, grabLock, mergePolicies, persist bool) (error, error) { if grabLock { i.lock.Lock() defer i.lock.Unlock() @@ -704,10 +705,12 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit return nil, err } - // Delete the entity which we are merging from in storage - err = i.entityPacker.DeleteItem(fromEntity.ID) - if err != nil { - return nil, err + if persist && !(i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby) { + // Delete the entity which we are merging from in storage + err = i.entityPacker.DeleteItem(fromEntity.ID) + if err != nil { + return nil, err + } } } @@ -717,19 +720,21 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit return nil, err } - // Persist the entity which we are merging to - toEntityAsAny, err := ptypes.MarshalAny(toEntity) - if err != nil { - return nil, err - } - item := &storagepacker.Item{ - ID: toEntity.ID, - Message: toEntityAsAny, - } + if persist && !(i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby) { + // Persist the entity which we are merging to + toEntityAsAny, err := ptypes.MarshalAny(toEntity) + if err != nil { + return nil, err + } + item := &storagepacker.Item{ + ID: toEntity.ID, + Message: toEntityAsAny, + } - err = i.entityPacker.PutItem(item) - if err != nil { - return nil, err + err = i.entityPacker.PutItem(item) + if err != nil { + return nil, err + } } return nil, nil diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index a47071f7c8a3..df7bc0ef6328 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -343,17 +343,11 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e if !i.disableLowerCasedNames { return errDuplicateIdentityName } - - // At this point, identity store is operating case-sensitively. - // Persisting is allowed only on the primary. - if i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby { - break - } } i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) - respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true) + respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true, persist) switch { case respErr != nil: return respErr From 41a47c2be1b7d254cdc4c720a8352f88506009a1 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 7 Feb 2019 15:03:04 -0500 Subject: [PATCH 12/14] Wire all the things directly do mergeEntity --- vault/identity_store_util.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index df7bc0ef6328..1554b439cd92 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -338,13 +338,6 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e // into merging fallthrough default: - if !persist { - i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases") - if !i.disableLowerCasedNames { - return errDuplicateIdentityName - } - } - i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true, persist) From d862f84025d49aa1f62834805a3fefc816734219 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 7 Feb 2019 16:40:49 -0500 Subject: [PATCH 13/14] Fix persist behavior --- vault/identity_store_util.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 1554b439cd92..56956a482d46 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -355,7 +355,7 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e if strutil.StrListContains(aliasFactors, i.sanitizeName(alias.Name)+alias.MountAccessor) { i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases") - if !persist && !i.disableLowerCasedNames { + if !i.disableLowerCasedNames { return errDuplicateIdentityName } } @@ -370,23 +370,25 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e } // If previous entity is set, update it in MemDB and persist it - if previousEntity != nil && persist { + if previousEntity != nil { err = i.MemDBUpsertEntityInTxn(txn, previousEntity) if err != nil { return err } - // Persist the previous entity object - marshaledPreviousEntity, err := ptypes.MarshalAny(previousEntity) - if err != nil { - return err - } - err = i.entityPacker.PutItem(&storagepacker.Item{ - ID: previousEntity.ID, - Message: marshaledPreviousEntity, - }) - if err != nil { - return err + if persist { + // Persist the previous entity object + marshaledPreviousEntity, err := ptypes.MarshalAny(previousEntity) + if err != nil { + return err + } + err = i.entityPacker.PutItem(&storagepacker.Item{ + ID: previousEntity.ID, + Message: marshaledPreviousEntity, + }) + if err != nil { + return err + } } } From 4ac25c6be080ca967ca0735163a4853a42e91be1 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Fri, 8 Feb 2019 09:28:30 -0500 Subject: [PATCH 14/14] Address review feedback --- vault/identity_store_entities.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/vault/identity_store_entities.go b/vault/identity_store_entities.go index 5ee9a707ac48..e57a1e7972ff 100644 --- a/vault/identity_store_entities.go +++ b/vault/identity_store_entities.go @@ -652,6 +652,7 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit } } + isPerfSecondaryOrStandby := i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby for _, fromEntityID := range fromEntityIDs { if fromEntityID == toEntity.ID { return errors.New("to_entity_id should not be present in from_entity_ids"), nil @@ -705,7 +706,7 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit return nil, err } - if persist && !(i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby) { + if persist && !isPerfSecondaryOrStandby { // Delete the entity which we are merging from in storage err = i.entityPacker.DeleteItem(fromEntity.ID) if err != nil { @@ -720,7 +721,7 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit return nil, err } - if persist && !(i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby) { + if persist && !isPerfSecondaryOrStandby { // Persist the entity which we are merging to toEntityAsAny, err := ptypes.MarshalAny(toEntity) if err != nil {