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 recursive locking in decision log plugin #1543

Closed
tsandall opened this issue Jul 3, 2019 · 0 comments · Fixed by #1544
Closed

Fix recursive locking in decision log plugin #1543

tsandall opened this issue Jul 3, 2019 · 0 comments · Fixed by #1544
Assignees
Labels

Comments

@tsandall
Copy link
Member

tsandall commented Jul 3, 2019

In v0.12 we added support for decision log masking. Those changes use a special log masking policy to obtain a set of paths to redact from decision log events. In those changes, the decision log plugin was extended to run policy evaluation without reusing the server's open read transaction. As a result, the plugin's evaluation context opens another read transaction (recursively). This lead to a deadlock if there are write transactions happening concurrently (because under the hood the storage layer just uses the sync.RWMutex primitive.)

@tsandall tsandall added the bug label Jul 3, 2019
@tsandall tsandall self-assigned this Jul 3, 2019
tsandall added a commit to tsandall/opa that referenced this issue Jul 3, 2019
These changes update the server to pass the server's open transaction
to the decision logger. This prevents the same goroutine from
recursively opening a new transcation when the log masking decision is
evaluated.

Alternatively we could update the server to close it's transaction
before logging the decision however this could lead to the log masking
decision being generated from a different policy revision. Another
alternative would be extend the storage layer to support recursive
transactions however this would be quite a bit more work.

We should investigate whether we can cheaply detect recursive
transactions in the store to avoid potential deadlocks in the future.

Also, delete opa binary that was accidentally committed to the repo.

Fixes open-policy-agent#1543

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
tsandall added a commit that referenced this issue Jul 3, 2019
These changes update the server to pass the server's open transaction
to the decision logger. This prevents the same goroutine from
recursively opening a new transcation when the log masking decision is
evaluated.

Alternatively we could update the server to close it's transaction
before logging the decision however this could lead to the log masking
decision being generated from a different policy revision. Another
alternative would be extend the storage layer to support recursive
transactions however this would be quite a bit more work.

We should investigate whether we can cheaply detect recursive
transactions in the store to avoid potential deadlocks in the future.

Also, delete opa binary that was accidentally committed to the repo.

Fixes #1543

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
tsandall added a commit to tsandall/opa that referenced this issue Jul 3, 2019
These changes update the server to pass the server's open transaction
to the decision logger. This prevents the same goroutine from
recursively opening a new transcation when the log masking decision is
evaluated.

Alternatively we could update the server to close it's transaction
before logging the decision however this could lead to the log masking
decision being generated from a different policy revision. Another
alternative would be extend the storage layer to support recursive
transactions however this would be quite a bit more work.

We should investigate whether we can cheaply detect recursive
transactions in the store to avoid potential deadlocks in the future.

Also, delete opa binary that was accidentally committed to the repo.

Fixes open-policy-agent#1543

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant