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

Move create entry "similarity" check inside of DataStore #2475

Merged
merged 2 commits into from
Aug 30, 2021

Conversation

azdagron
Copy link
Member

The entry similarity check happens only on the API level, but not within the same transaction that the entry creation takes place in. This causes a small window where two concurrent requests for similar entries can result in both being created.

This PR moves the similarity check down to the SQL DataStore layer inside of the transaction that creates the entry. It also introduces a new DataStore function CreateOrReturnRegistrationEntry that returns the existing entry, that is useful for the API, but otherwise allows us to not have to update the existing CreateRegistrationEntry callers at this time. Both CreateRegistrationEntry and CreateOrReturnRegistrationEntry go through the same code paths and differ only in how they treat the case when a similar entry exists. CreateRegistrationEntry fails with AlreadyExists, while CreateOrReturnRegistrationEntry returns the existing entry along with a bool indicating that it is an existing entry.

This PR does NOT address the issue that UpdateRegistrationEntry can end up creating two "similar" entries.

The entry similarity check happens only on the API level, but not within the
same transaction that the entry creation takes place in. This causes a small
window where two concurrent requests for similar entries can result in both
being created.

This PR moves the similarity check down to the SQL DataStore layer inside of
the transaction that creates the entry. It also introduces a new DataStore
function CreateOrReturnRegistrationEntry that returns the existing entry, that
is useful for the API, but otherwise allows us to not have to update the
existing CreateRegistrationEntry callers at this time. Both
CreateRegistrationEntry and CreateOrReturnRegistrationEntry go through the same
code paths and differ only in how they treat the case when a similar entry
exists. CreateRegistrationEntry fails with AlreadyExists, while
CreateOrReturnRegistrationEntry returns the existing entry along with a bool
indicating that it is an existing entry.

This PR does NOT address the issue that UpdateRegistrationEntry can end up
creating two "similar" entries.

Signed-off-by: Andrew Harding <aharding@vmware.com>
Signed-off-by: Andrew Harding <aharding@vmware.com>
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

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

Thank you for this @azdagron!
Changes look good to me 🎉

@azdagron azdagron merged commit c01d0b6 into spiffe:main Aug 30, 2021
@azdagron azdagron deleted the fix-similar-entry-creation branch August 30, 2021 14:39
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.

2 participants