Merge Identity Entities if two claim the same alias #5075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Fixes #5000 |
vishalnayak
reviewed
Aug 9, 2018
| } | ||
| } | ||
|
|
||
| result := make([]string, 0, len(all)) |
There was a problem hiding this comment.
Will making a slice of len(all) and using index to insert elements into it be any better? Or will append be a no-op in terms of new allocations when the capacity matches the desired size?
There was a problem hiding this comment.
It's a noop, no new allocations in this case.
vault/identity_store_util.go
Outdated
| @@ -176,20 +176,25 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { | |||
| } | |||
|
|
|||
| for _, item := range bucket.Items { | |||
| i.lock.Lock() | |||
There was a problem hiding this comment.
Do we want to grab this lock before the for loop to cover loading of all the items in the bucket?
| // If the entity from which we are merging from was already a merged | ||
| // entity, transfer over the Merged set to the entity we are | ||
| // merging into. | ||
| toEntity.MergedEntityIDs = append(toEntity.MergedEntityIDs, fromEntity.MergedEntityIDs...) |
There was a problem hiding this comment.
Can we use strutil.MergeSlices here as well?
There was a problem hiding this comment.
We could -- the main reason I did it for policies was for deduplication -- that shouldn't be a problem here. Don't think it really matters.
vishalnayak
previously approved these changes
Aug 9, 2018
jefferai
force-pushed
the
merge-on-conflict
branch
from
1e213d1
to
dcd3343
Compare
Past bugs/race conditions meant two entities could be created each claiming the same alias. There are planned longer term fixes for this (outside of the race condition being fixed in 0.10.4) that involve changing the data model, but this is an immediate workaround that has the same net effect: if two entities claim the same alias, assume they were created due to this race condition and merge them. In this situation, also automatically merge policies so we don't lose e.g. RGPs.
jefferai
force-pushed
the
merge-on-conflict
branch
from
dcd3343
to
bf864e5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Past bugs/race conditions meant two entities could be created each
claiming the same alias. There are planned longer term fixes for this
(outside of the race condition being fixed in 0.10.4) that involve
changing the data model, but this is an immediate workaround that has
the same net effect: if two entities claim the same alias, assume they
were created due to this race condition and merge them.
In this situation, also automatically merge policies so we don't lose
e.g. RGPs.