Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add locking when adding aliases to existing entities #4965

Merged
merged 4 commits into from
Jul 25, 2018

Conversation

chrishoffman
Copy link
Contributor

Fixes #4870

@chrishoffman chrishoffman added this to the 0.10.4 milestone Jul 20, 2018
@@ -332,6 +332,11 @@ func (i *IdentityStore) pathEntityIDUpdate() framework.OperationFunc {
return logical.ErrorResponse("missing entity id"), nil
}

// Acquire the lock to modify the entity storage entry
lock := locksutil.LockForKey(i.entityLocks, entityID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to add this in pathEntityRegister too since it also calls handleEntityUpdateComon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only case left in pathEntityRegister is new entities since when the id is provided it comes through this method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but you changed the function in handleEntityUpdateCommon to use a non-locking version, and now if it goes through pathEntityRegister and don't provide an ID that function is called without a lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added back the lock on a new entity by calling upsertEntity for new entities.

@@ -206,19 +206,10 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
}

// Get entity id
canonicalID := d.Get("entity_id").(string)
canonicalID := d.Get("canonical_id").(string)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also put a deprecation notice for entity_id's pathHelp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

lockKeys = append(lockKeys, existingEntity.ID)
}

if canonicalID != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move lockKeys up to the top and add in the canonicalID up then, if we have one, you can pull the LocksForKeys and lock/unlock lines out of this if/else

@@ -256,6 +247,51 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
return nil, err
}

var existingEntity *identity.Entity
var lockKeys []string
if !newAlias {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part should be after the locks are grabbed. The MemDB stuff happens in a transaction but it's still possible for that transaction to say, for instance, that there is no existingEntity, then have such an entity be created after the locking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we don't know the id of the entity to grab the lock for. This is what I alluded to as still a race but since we have to look it up, I'm not sure the alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to alleviate this issue would be to add locks for alias IDs. I'll went down that path at one point but pulled it out since it was only really used here. I think that would fully take care of this issue and will do that.

if existingEntity == nil {
return nil, fmt.Errorf("alias is not associated with an entity")
}
lockKeys = append(lockKeys, existingEntity.ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that canonical_id and existingEntity.ID can be the same ID, i think they can if you're updating the alias and keeping the same entityID. If so i think the below lock acquisition will deadlock

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it looks like the LocksForKeys function will dedup them for you

@@ -256,8 +258,46 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
return nil, err
}

var existingEntity *identity.Entity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to grab the alias lock before the MemDBAliasByFactors call above? Otherwise what happens if an automatic call is registering an alias at the same time as this manual call?

if entity == nil {
return logical.ErrorResponse("invalid canonical ID"), nil
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this it should probably check again to see whether the Alias is still a part of this entity.

@jefferai jefferai merged commit 8152811 into master Jul 25, 2018
@jefferai jefferai deleted the identity-alias-race branch July 25, 2018 02:02
Copy link
Member

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants