provider/aws: serialize SG rule access to fix race condition #3965
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
| // The initial use case is to let aws_security_group_rule resources serialize | ||
| // their access to individual security groups based on SG ID. | ||
| type MutexKV struct { | ||
| sync.Mutex |
There was a problem hiding this comment.
I normally don't embed these in public structs, as Lock() and Unlock() become public as well.
|
@phinze this looks good, just a few comments above! |
phinze
force-pushed
the
b-aws-sg-rules-v2-race
branch
2 times, most recently
from
e0486f5
to
bb0a9b4
Compare
|
@phinze This looks great. I would recommend moving the MutexKV to a helper package, but otherwise 👍 Also consider some tests for it. |
Because `aws_security_group_rule` resources are an abstraction on top of Security Groups, they must interact with the AWS Security Group APIs in a pattern that often results in lots of parallel requests interacting with the same security group. We've found that this pattern can trigger race conditions resulting in inconsistent behavior, including: * Rules that report as created but don't actually exist on AWS's side * Rules that show up in AWS but don't register as being created locally, resulting in follow up attempts to authorize the rule failing w/ Duplicate errors Here, we introduce a per-SG mutex that must be held by any security group before it is allowed to interact with AWS APIs. This protects the space between `DescribeSecurityGroup` and `Authorize*` / `Revoke*` calls, ensuring that no other rules interact with the SG during that span. The included test exposes the race by applying a security group with lots of rules, which based on the dependency graph can all be handled in parallel. This fails most of the time without the new locking behavior. I've omitted the mutex from `Read`, since it is only called during the Refresh walk when no changes are being made, meaning a bunch of parallel `DescribeSecurityGroup` API calls should be consistent in that case.
phinze
force-pushed
the
b-aws-sg-rules-v2-race
branch
from
bb0a9b4
to
6b6b5a4
Compare
|
Moved to helper w/ unit tests. 👍 |
|
🚢 |
phinze
added a commit
that referenced
this pull request
Nov 18, 2015
provider/aws: serialize SG rule access to fix race condition
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Because
aws_security_group_ruleresources are an abstraction on top ofSecurity Groups, they must interact with the AWS Security Group APIs in
a pattern that often results in lots of parallel requests interacting
with the same security group.
We've found that this pattern can trigger race conditions resulting in
inconsistent behavior, including:
locally, resulting in follow up attempts to authorize the rule
failing w/ Duplicate errors
Here, we introduce a per-SG mutex that must be held by any security
group before it is allowed to interact with AWS APIs. This protects the
space between
DescribeSecurityGroupandAuthorize*/Revoke*calls, ensuring that no other rules interact with the SG during that
span.
The included test exposes the race by applying a security group with
lots of rules, which based on the dependency graph can all be handled in
parallel. This fails most of the time without the new locking behavior.
I've omitted the mutex from
Read, since it is only called during theRefresh walk when no changes are being made, meaning a bunch of parallel
DescribeSecurityGroupAPI calls should be consistent in that case.