resource/aws_ram: Fix eventual consistency problems #17032

shuheiktgw · 2021-01-09T00:00:11Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #13494
Closes #15187
Closes #16578
Closes #17658
Closes #18332

Release note for CHANGELOG:

* resource/aws_ram_resource_share_accepter: Fix eventual consistency problems

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsRamResourceShareAccepter_*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsRamResourceShareAccepter_* -timeout 120m
=== RUN   TestAccAwsRamResourceShareAccepter_basic
=== PAUSE TestAccAwsRamResourceShareAccepter_basic
=== CONT  TestAccAwsRamResourceShareAccepter_basic
--- PASS: TestAccAwsRamResourceShareAccepter_basic (79.33s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	81.162s

$ make testacc TESTARGS='-run=TestAccAwsRamResourceShare_*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsRamResourceShare_* -timeout 120m
=== RUN   TestAccAwsRamResourceShareAccepter_basic
=== PAUSE TestAccAwsRamResourceShareAccepter_basic
=== RUN   TestAccAwsRamResourceShare_basic
=== PAUSE TestAccAwsRamResourceShare_basic
=== RUN   TestAccAwsRamResourceShare_AllowExternalPrincipals
=== PAUSE TestAccAwsRamResourceShare_AllowExternalPrincipals
=== RUN   TestAccAwsRamResourceShare_Name
=== PAUSE TestAccAwsRamResourceShare_Name
=== RUN   TestAccAwsRamResourceShare_Tags
=== PAUSE TestAccAwsRamResourceShare_Tags
=== CONT  TestAccAwsRamResourceShareAccepter_basic
=== CONT  TestAccAwsRamResourceShare_Name
=== CONT  TestAccAwsRamResourceShare_AllowExternalPrincipals
=== CONT  TestAccAwsRamResourceShare_Tags
=== CONT  TestAccAwsRamResourceShare_basic
--- PASS: TestAccAwsRamResourceShare_basic (42.74s)
--- PASS: TestAccAwsRamResourceShare_Name (69.01s)
--- PASS: TestAccAwsRamResourceShare_AllowExternalPrincipals (69.10s)
--- PASS: TestAccAwsRamResourceShareAccepter_basic (90.10s)
--- PASS: TestAccAwsRamResourceShare_Tags (92.77s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	94.688s

Thank you for your review! 👍

dthvt · 2021-01-09T01:53:08Z

aws/resource_aws_ram_resource_share_accepter.go

+		i, err := findResourceShareInvitation(conn, resourceShareARN, status)
+		invitation = i


Not a golang expert, but is there a reason i is used here and then immediately assigned to invitation instead of just using invitation, err := ?

No, actually you cannot substitute the returned output for the outer invitation using :=...

Ah, TIL. Thanks!

pkleanthous · 2021-01-20T18:01:17Z

Hey guys,

Can you please merge this PR?

datacenter-dylan · 2021-04-14T15:05:12Z

Any estimate on when we can expect this to be merged?

lorengordon · 2021-04-21T17:18:21Z

This PR might also fix the race condition I reported here: #18332

lorengordon · 2021-04-24T17:12:13Z

@YakDriver I built the aws provider using this branch and can confirm it does appear to also address the race condition I reported in #18332 (and probably #17658 and #15187, as well as the two linked in the PR description #13494 and #16578). At least, I looped over 100 builds without encountering the failure...

YakDriver · 2021-04-28T13:33:43Z

@shuheiktgw My friend, thank you for this excellent PR. Unfortunately, I believe due to account settings changes on our side, I cannot currently test this properly. Rather than wait for us to straighten out our account problems, if you (and @lorengordon) can help test/fix my new changes, that would be very helpful! Make sure to pull my latest changes to your branch. Please let me know if you have any questions.

What I'm currently getting from the acceptance tests (GovCloud):

--- FAIL: TestAccAwsRamResourceAssociation_basic (18.17s)        // failure predates this PR (Hashi test account settings?)
--- FAIL: TestAccAwsRamResourceAssociation_disappears (17.97s)   // failure predates this PR (Hashi test account settings?)
--- FAIL: TestAccAwsRamResourceShareAccepter_basic (14.37s)      // failure predates this PR (Hashi test account settings?)
--- FAIL: TestAccAwsRamResourceShareAccepter_disappears (14.72s) // new test (Hashi test account settings?)
--- PASS: TestAccAwsRamPrincipalAssociation_basic (25.72s)
--- PASS: TestAccAwsRamPrincipalAssociation_disappears (22.62s)
--- PASS: TestAccAwsRamResourceShare_AllowExternalPrincipals (36.17s)
--- PASS: TestAccAwsRamResourceShare_basic (22.94s)
--- PASS: TestAccAwsRamResourceShare_Name (36.05s)
--- PASS: TestAccAwsRamResourceShare_Tags (48.68s)

Output from acceptance tests on commercial (us-west-2):

--- FAIL: TestAccAwsRamResourceAssociation_basic (15.42s)         // failure predates this PR (Hashi test account settings?)
--- FAIL: TestAccAwsRamResourceAssociation_disappears (15.35s)    // failure predates this PR (Hashi test account settings?)
--- FAIL: TestAccAwsRamResourceShareAccepter_basic (11.42s)       // failure predates this PR (Hashi test account settings?)
--- FAIL: TestAccAwsRamResourceShareAccepter_disappears (130.89s) // new test (Hashi test account settings?)
--- PASS: TestAccAwsRamPrincipalAssociation_basic (20.38s)
--- PASS: TestAccAwsRamPrincipalAssociation_disappears (14.36s)   // sometimes fails
--- PASS: TestAccAwsRamResourceShare_AllowExternalPrincipals (26.88s)
--- PASS: TestAccAwsRamResourceShare_basic (17.45s)
--- PASS: TestAccAwsRamResourceShare_Name (27.91s)
--- PASS: TestAccAwsRamResourceShare_Tags (35.73s)

lorengordon · 2021-04-28T13:53:57Z

@YakDriver running my test now, the loop takes a long time, so probably will report back tomorrow.

ghost · 2021-04-30T15:25:06Z

This has been released in version 3.38.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

lorengordon · 2021-04-30T15:48:11Z

fwiw, closing the loop, finally made it through my looped tests for the race condition. didn't encounter it even once in 100 executions of the test config, so 🤞 i'm pretty sure this is solid!

github-actions · 2021-05-31T02:48:14Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

shuheiktgw requested a review from a team as a code owner January 9, 2021 00:00

ghost added size/M Managed by automation to categorize the size of a PR. service/ram Issues and PRs that pertain to the ram service. labels Jan 9, 2021

github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Jan 9, 2021

This was referenced Jan 9, 2021

TGW multi-provider aws_ram_resource_share_accepter cannot find invitation #13494

Closed

TestAccAwsRamResourceShareAccepter_basic fails #16578

Closed

shuheiktgw force-pushed the fix_ram_resource_share_accepter branch 2 times, most recently from 1035942 to db54722 Compare January 9, 2021 00:22

dthvt reviewed Jan 9, 2021

View reviewed changes

shuheiktgw force-pushed the fix_ram_resource_share_accepter branch from db54722 to 19293a7 Compare January 17, 2021 04:08

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Jan 17, 2021

shuheiktgw force-pushed the fix_ram_resource_share_accepter branch from 19293a7 to fa70c5e Compare January 17, 2021 11:11

Base automatically changed from master to main January 23, 2021 01:00

YakDriver self-assigned this Apr 22, 2021

Fix aws_ram_resource_share_accepter eventual consistency problems

559e521

YakDriver force-pushed the fix_ram_resource_share_accepter branch from fa70c5e to 559e521 Compare April 27, 2021 18:36

YakDriver added 4 commits April 28, 2021 09:21

internal/ram: Move waiters to internal

baba2f2

r/ram_principal_association: Fix eventual consistency, disappears

a6b5cee

tests/r/ram_principal_associaton: Fix disappears

7bb030e

r/ram: Add changelog

e1cdba1

YakDriver added 3 commits April 28, 2021 09:24

r/resource_share_accepter: Minor fixes

f704ab2

tests/r/ram_resource_share_accepter: Minor fixes

85857ea

tests/r/ram_resource_share: Minor fixes

4876792

ghost added the tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. label Apr 28, 2021

r/ram: Update changelog

c117bc4

YakDriver changed the title ~~resource/aws_ram_resource_share_accepter: Fix eventual consistency problems~~ resource/aws_ram: Fix eventual consistency problems Apr 28, 2021

tests/r/ram_resource_share_accepter: Make consistent

d17f769

YakDriver removed the needs-triage Waiting for first response or review from a maintainer. label Apr 28, 2021

YakDriver added this to the v3.38.0 milestone Apr 29, 2021

YakDriver merged commit 16e9af1 into hashicorp:main Apr 29, 2021

github-actions bot locked as resolved and limited conversation to collaborators May 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_ram: Fix eventual consistency problems #17032

resource/aws_ram: Fix eventual consistency problems #17032

shuheiktgw commented Jan 9, 2021 •

edited by YakDriver

Loading

dthvt Jan 9, 2021

shuheiktgw Jan 17, 2021

dthvt Jan 18, 2021

pkleanthous commented Jan 20, 2021

datacenter-dylan commented Apr 14, 2021

lorengordon commented Apr 21, 2021 •

edited

Loading

lorengordon commented Apr 24, 2021

YakDriver commented Apr 28, 2021 •

edited

Loading

lorengordon commented Apr 28, 2021

ghost commented Apr 30, 2021

lorengordon commented Apr 30, 2021

github-actions bot commented May 31, 2021

		i, err := findResourceShareInvitation(conn, resourceShareARN, status)
		invitation = i

resource/aws_ram: Fix eventual consistency problems #17032

resource/aws_ram: Fix eventual consistency problems #17032

Conversation

shuheiktgw commented Jan 9, 2021 • edited by YakDriver Loading

Community Note

dthvt Jan 9, 2021

Choose a reason for hiding this comment

shuheiktgw Jan 17, 2021

Choose a reason for hiding this comment

dthvt Jan 18, 2021

Choose a reason for hiding this comment

pkleanthous commented Jan 20, 2021

datacenter-dylan commented Apr 14, 2021

lorengordon commented Apr 21, 2021 • edited Loading

lorengordon commented Apr 24, 2021

YakDriver commented Apr 28, 2021 • edited Loading

lorengordon commented Apr 28, 2021

ghost commented Apr 30, 2021

lorengordon commented Apr 30, 2021

github-actions bot commented May 31, 2021

shuheiktgw commented Jan 9, 2021 •

edited by YakDriver

Loading

lorengordon commented Apr 21, 2021 •

edited

Loading

YakDriver commented Apr 28, 2021 •

edited

Loading