-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
resource/aws_security_group: Fix Missing Id Error #3892
Conversation
Make security groups more resilient to eventual consistency errors by adding retries on the existence function in read and update. See: hashicorp/terraform#6991
I'll run the acceptance test suite for you now. 😄 Will take a little bit. |
Test failure looks like an unrelated eventual consistency issue.
|
@bflad Thank you for running the acceptance tests for me. |
aws/resource_aws_security_group.go
Outdated
@@ -342,11 +332,12 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er | |||
func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) error { | |||
conn := meta.(*AWSClient).ec2conn | |||
|
|||
sgRaw, _, err := SGStateRefreshFunc(conn, d.Id())() | |||
sgRaw, err := waitForSgToExist(conn, d.Id(), d.Timeout(schema.TimeoutRead)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in both the read/update cases we will want to check d.IsNewResource()
before using the waitForSgToExist
logic -- otherwise people will have to wait for the entire read timeout when it is removed outside Terraform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, will fix. Sorry for not considering that case.
@bflad Updated. Was torn on whether to put this new logic in the function or not, ended up putting it outside the function because I thought it would be harder to reason about the behavior with it in the function. But this duplicates a few lines. |
@bflad Would you be able to look at this again today? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - let's get this in 🚀
Test failures unrelated:
Tests failed: 3 (3 new), passed: 52
=== RUN TestAccAWSSecurityGroupRule_ExpectInvalidTypeError
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (2.22s)
=== RUN TestAccAWSSecurityGroupRule_ExpectInvalidCIDR
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (2.45s)
=== RUN TestAccAWSSecurityGroupRule_Issue5310
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (17.12s)
=== RUN TestAccAWSSecurityGroupRule_Ingress_Classic
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (20.01s)
=== RUN TestAccAWSSecurityGroupRule_MultiIngress
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (46.52s)
=== RUN TestAccAWSSecurityGroupRule_Egress
--- PASS: TestAccAWSSecurityGroupRule_Egress (49.44s)
=== RUN TestAccAWSSecurityGroupRule_IngressDescription
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (36.86s)
=== RUN TestAccAWSSecurityGroupRule_EgressDescription
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (84.77s)
=== RUN TestAccAWSSecurityGroupRule_IngressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (90.23s)
=== RUN TestAccAWSSecurityGroupRule_Ingress_Protocol
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (137.30s)
=== RUN TestAccAWSSecurityGroupRule_PartialMatching_Source
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (152.82s)
=== RUN TestAccAWSSecurityGroupRule_SelfSource
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (168.60s)
=== RUN TestAccAWSSecurityGroupRule_Ingress_Ipv6
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (173.52s)
=== RUN TestAccAWSSecurityGroup_importIPRangeAndSecurityGroupWithSameRules
--- PASS: TestAccAWSSecurityGroup_importIPRangeAndSecurityGroupWithSameRules (184.56s)
=== RUN TestAccAWSSecurityGroupRule_Ingress_VPC
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (185.14s)
=== RUN TestAccAWSSecurityGroup_importIPRangesWithSameRules
--- PASS: TestAccAWSSecurityGroup_importIPRangesWithSameRules (188.84s)
=== RUN TestAccAWSSecurityGroup_namePrefix
--- PASS: TestAccAWSSecurityGroup_namePrefix (11.47s)
=== RUN TestAccAWSSecurityGroup_importSelf
--- PASS: TestAccAWSSecurityGroup_importSelf (208.33s)
=== RUN TestAccAWSSecurityGroup_importSourceSecurityGroup
--- PASS: TestAccAWSSecurityGroup_importSourceSecurityGroup (234.12s)
=== RUN TestAccAWSSecurityGroup_importPrefixList
--- PASS: TestAccAWSSecurityGroup_importPrefixList (243.07s)
=== RUN TestAccAWSSecurityGroupRule_PartialMatching_basic
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (249.42s)
=== RUN TestAccAWSSecurityGroupRule_SelfReference
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (274.68s)
=== RUN TestAccAWSSecurityGroup_tagsCreatedFirst
--- PASS: TestAccAWSSecurityGroup_tagsCreatedFirst (103.54s)
=== RUN TestAccAWSSecurityGroup_ChangeRuleDescription
--- FAIL: TestAccAWSSecurityGroup_ChangeRuleDescription (28.14s)
testing.go:518: Step 0 error: Check failed: Check 3/3 error: aws_security_group.web: Attribute 'egress.2129912301.description' not found
=== RUN TestAccAWSSecurityGroup_DefaultEgress_Classic
--- PASS: TestAccAWSSecurityGroup_DefaultEgress_Classic (11.64s)
=== RUN TestAccAWSSecurityGroup_basic
--- PASS: TestAccAWSSecurityGroup_basic (203.72s)
=== RUN TestAccAWSSecurityGroup_generatedName
--- FAIL: TestAccAWSSecurityGroup_generatedName (35.14s)
testing.go:518: Step 0 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: aws_security_group.web
egress.#: "0" => "1"
egress.3629188364.cidr_blocks.#: "0" => "1"
egress.3629188364.cidr_blocks.0: "" => "10.0.0.0/8"
egress.3629188364.description: "" => ""
egress.3629188364.from_port: "" => "80"
egress.3629188364.ipv6_cidr_blocks.#: "0" => "0"
egress.3629188364.prefix_list_ids.#: "0" => "0"
egress.3629188364.protocol: "" => "tcp"
egress.3629188364.security_groups.#: "0" => "0"
egress.3629188364.self: "" => "false"
egress.3629188364.to_port: "" => "8000"
=== RUN TestAccAWSSecurityGroup_invalidCIDRBlock
--- PASS: TestAccAWSSecurityGroup_invalidCIDRBlock (3.05s)
=== RUN TestAccAWSSecurityGroup_vpcProtoNumIngress
--- PASS: TestAccAWSSecurityGroup_vpcProtoNumIngress (150.39s)
=== RUN TestAccAWSSecurityGroupRule_EgressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (312.24s)
=== RUN TestAccAWSSecurityGroup_drift
--- PASS: TestAccAWSSecurityGroup_drift (72.42s)
=== RUN TestAccAWSSecurityGroup_Change
--- PASS: TestAccAWSSecurityGroup_Change (129.71s)
=== RUN TestAccAWSSecurityGroup_ipv6
--- PASS: TestAccAWSSecurityGroup_ipv6 (203.49s)
=== RUN TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic (16.47s)
=== RUN TestAccAWSSecurityGroup_vpc
--- PASS: TestAccAWSSecurityGroup_vpc (208.47s)
=== RUN TestAccAWSSecurityGroup_drift_complex
--- FAIL: TestAccAWSSecurityGroup_drift_complex (123.49s)
testing.go:518: Step 0 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: aws_security_group.web
egress.#: "0" => "3"
egress.341908820.cidr_blocks.#: "0" => "0"
egress.341908820.description: "" => ""
egress.341908820.from_port: "" => "22"
egress.341908820.ipv6_cidr_blocks.#: "0" => "0"
egress.341908820.prefix_list_ids.#: "0" => "0"
egress.341908820.protocol: "" => "tcp"
egress.341908820.security_groups.#: "0" => "1"
egress.341908820.security_groups.882545645: "" => "sg-f237c88c"
egress.341908820.self: "" => "false"
egress.341908820.to_port: "" => "22"
egress.3629188364.cidr_blocks.#: "0" => "1"
egress.3629188364.cidr_blocks.0: "" => "10.0.0.0/8"
egress.3629188364.description: "" => ""
egress.3629188364.from_port: "" => "80"
egress.3629188364.ipv6_cidr_blocks.#: "0" => "0"
egress.3629188364.prefix_list_ids.#: "0" => "0"
egress.3629188364.protocol: "" => "tcp"
egress.3629188364.security_groups.#: "0" => "0"
egress.3629188364.self: "" => "false"
egress.3629188364.to_port: "" => "8000"
egress.657243763.cidr_blocks.#: "0" => "1"
egress.657243763.cidr_blocks.0: "" => "206.0.0.0/8"
egress.657243763.description: "" => ""
egress.657243763.from_port: "" => "80"
egress.657243763.ipv6_cidr_blocks.#: "0" => "0"
egress.657243763.prefix_list_ids.#: "0" => "0"
egress.657243763.protocol: "" => "tcp"
egress.657243763.security_groups.#: "0" => "0"
egress.657243763.self: "" => "false"
egress.657243763.to_port: "" => "8000"
=== RUN TestAccAWSSecurityGroup_self
--- PASS: TestAccAWSSecurityGroup_self (252.01s)
=== RUN TestAccAWSSecurityGroup_emptyRuleDescription
--- PASS: TestAccAWSSecurityGroup_emptyRuleDescription (67.36s)
=== RUN TestAccAWSSecurityGroup_tags
--- PASS: TestAccAWSSecurityGroup_tags (131.92s)
=== RUN TestAccAWSSecurityGroup_vpcNegOneIngress
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (283.12s)
=== RUN TestAccAWSSecurityGroup_DefaultEgress_VPC
--- PASS: TestAccAWSSecurityGroup_DefaultEgress_VPC (246.54s)
=== RUN TestAccAWSSecurityGroup_importBasic
--- PASS: TestAccAWSSecurityGroup_importBasic (528.17s)
=== RUN TestAccAWSSecurityGroupRule_PrefixListEgress
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (585.82s)
=== RUN TestAccAWSSecurityGroup_MultiIngress
--- PASS: TestAccAWSSecurityGroup_MultiIngress (369.39s)
=== RUN TestAccAWSSecurityGroup_failWithDiffMismatch
--- PASS: TestAccAWSSecurityGroup_failWithDiffMismatch (217.59s)
=== RUN TestAccAWSSecurityGroup_CIDRandGroups
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (289.18s)
=== RUN TestAccAWSSecurityGroup_ipv4andipv6Egress
--- PASS: TestAccAWSSecurityGroup_ipv4andipv6Egress (311.26s)
=== RUN TestAccAWSSecurityGroup_egressWithPrefixList
--- PASS: TestAccAWSSecurityGroup_egressWithPrefixList (318.70s)
=== RUN TestAccAWSSecurityGroup_ingressWithCidrAndSGs
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs (329.87s)
=== RUN TestAccAWSSecurityGroup_basicRuleDescription
--- PASS: TestAccAWSSecurityGroup_basicRuleDescription (552.78s)
=== RUN TestAccAWSSecurityGroupRule_MultiDescription
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (653.61s)
=== RUN TestAccAWSSecurityGroupRule_Race
--- PASS: TestAccAWSSecurityGroupRule_Race (783.07s)
=== RUN TestAccAWSSecurityGroup_importIpv6
--- PASS: TestAccAWSSecurityGroup_importIpv6 (849.05s)
=== RUN TestAccAWSSecurityGroup_forceRevokeRules_false
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_false (1087.52s)
=== RUN TestAccAWSSecurityGroup_forceRevokeRules_true
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_true (1221.43s)
@bflad Awesome, thanks. |
This has been released in version 1.13.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Issue Details
I've been running into an error with security groups quite frequently recently, where I have one security group referencing the id of another and then causing terraform to fail due to an error like this:
This issue is documented here: hashicorp/terraform#6991 and has been open for two years. I have a debug log from a failure, and it seems like the following is causing the issue:
SGStateRefreshFunc
without any retries hereHowever, if AWS is having a bad day
1.
could return the SG, but then2.
will returnnil
(since it has no retries) which will then cause the SG to be removed from the state here.I tested this was the issue by moving the
d.SetId("")
outside of theif
statement, and it produced the above error.PR Details
So to fix this have added a call to the
waitForState()
function encapsulated in a common function shared amongstCreate, Read, and Update
, which basically adds retries to the previous call sites toSGStateRefreshFunc
.Testing
I ran the unit tests and acceptance tests, however my VPC is not setup properly and some acceptance tests fail with failures such as:
I'd appreciate if you could run the acceptance tests for me.