Persist merged entities only on the primary #6075
Conversation
vault/identity_store_util.go
Outdated
| // The entity and aliases will be loaded into memdb and persisted | ||
| // as a result of the merge so we are done here | ||
| return nil | ||
| } | ||
|
|
||
| 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 { |
There was a problem hiding this comment.
Should this be an OR, not AND?
There was a problem hiding this comment.
It should be an AND.
There was a problem hiding this comment.
OK, so this will then cause memdb updates in this case, whereas before we would not do memdb updates and would simply return the error. Just making sure that's the correct behavior.
There was a problem hiding this comment.
Thanks for asking! The reason behind this change is just that disableLowerCasedNames error is captured only in the startup flow and no where else, and it only makes sense to get this error in that flow. But this function is not restricted to the login flow alone. So adding the guard of persist being set to false ensures that regular operations doesn't result in this error. This is not related to the core fix in this PR, and is something that I happened to notice.
vault/identity_store_util.go
Outdated
| @@ -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): | |||
There was a problem hiding this comment.
Should this be checking for performance standby too?
vault/identity_store_util.go
Outdated
|
|
||
| // 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) { |
There was a problem hiding this comment.
As I found out today, this won't do what you expect. Perf standby only appears in this call if you're going through a system view. You need to check c.perfStandby.
There was a problem hiding this comment.
Thanks a bunch! Fixed.
vishalnayak
force-pushed
the
entity-loading
branch
from
3e4493a to
df84739
Compare
vault/identity_store_util.go
Outdated
| loadFunc := func(context.Context) error { | ||
| err := c.identityStore.loadEntities(ctx) | ||
| err = c.identityStore.loadEntities(ctx) |
There was a problem hiding this comment.
Better to leave as := I think?
vault/identity_store_entities.go
Outdated
| ID: toEntity.ID, | ||
| Message: toEntityAsAny, | ||
| } | ||
| if persist && !(i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby) { |
There was a problem hiding this comment.
Minor nit, can you pull the replication state check into a var at the top of the function rather than have it twice?
vishalnayak
changed the title
No description provided.