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

Fix MySQL isolation level for read-modify-write ops #2150

Merged
merged 3 commits into from
Mar 12, 2021

Conversation

azdagron
Copy link
Member

Certain operations in SPIRE are read-modify-write, i.e., they read rows, process columns on those rows, and then update the rows as part of a single transaction. This is for sensitive operations like bundle appending/pruning.

We've relied on the REPEATABLE READ isolation level to provide us the consistency guarantees we need over these kind of operations operation, namely that which prevents concurrent transactions from updating the same target row(s). This is to prevent one transaction from undoing the action just performed by another. For example, if two servers attempt to append key material to the bundle concurrently, we don't want the append op from one to accidentally "undo" the append of the other, as they are both updating the same bundle row.

We chose REPEATABLE READ based on an assumption that PostgreSQL behavior was universal.

Unfortunately MySQL REPEATABLE READ is weaker than that of PostgreSQL. While, PostgreSQL fails concurrent transactions that try to update the same target row, MySQL does not. For MySQL, we need SERIALIZABLE, which is the same as REPEATABLE READ except that it converts SELECT to SELECT ... LOCK FOR SHARE MODE which "sets a shared lock that permits other transactions to read the examined rows but not to update or delete them", which gives us the result we need.

Fixes: #2149

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

@rturner3 rturner3 left a comment

Choose a reason for hiding this comment

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

I think we might need to do the same thing for updating of attested nodes

pkg/server/plugin/datastore/sql/sql.go Outdated Show resolved Hide resolved
pkg/server/plugin/datastore/sql/sql.go Show resolved Hide resolved
@azdagron
Copy link
Member Author

Good catch! I've updated the PR.

@azdagron
Copy link
Member Author

Github is struggling. Commit is not reflected here yet.

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

@rturner3 rturner3 left a comment

Choose a reason for hiding this comment

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

🚀

@azdagron azdagron merged commit b1b3b5a into spiffe:master Mar 12, 2021
@azdagron azdagron deleted the fix-isolation-levels branch March 12, 2021 20:05
@azdagron azdagron added this to the 0.12.2 milestone Mar 23, 2021
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.

sometimes spire-server fails to update the CA information in SQL-DB and configmap bundle
2 participants