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

Implement d.IsNewResource() Checks In Resource Read Functions #16796

Closed
bflad opened this issue Dec 16, 2020 · 7 comments · Fixed by #30157
Closed

Implement d.IsNewResource() Checks In Resource Read Functions #16796

bflad opened this issue Dec 16, 2020 · 7 comments · Fixed by #30157
Labels
bug Addresses a defect in current functionality. linter Pertains to changes to or issues with the various linters. provider Pertains to the provider itself, rather than any interaction with AWS. technical-debt Addresses areas of the codebase that need refactoring or redesign.

Comments

@bflad
Copy link
Contributor

bflad commented Dec 16, 2020

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue 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 issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

During resource creation, Terraform CLI expects either a properly applied state for the new resource or an error. To signal proper resource existence, the Terraform Plugin SDK uses an underlying resource identifier (set via d.SetId(/* some value */)). If for some reason the resource creation is returned without an error, but also without the resource identifier being set, Terraform CLI will return an error such as:

Error: Provider produced inconsistent result after apply

When applying changes to aws_sns_topic_subscription.sqs,
provider "registry.terraform.io/hashicorp/aws" produced an unexpected new
value: Root resource was present, but now absent.

This is a bug in the provider, which should be reported in the provider's own
issue tracker.

A typical pattern in resource implementations in the Create/CreateContext function is to return the Read/ReadContext function at the end to fill in the Terraform State for all attributes. Another typical pattern in resource implementations in the Read/ReadContext function is to remove the resource from the Terraform State if the remote system returns an error or status that indicates the remote resource no longer exists by explicitly calling d.SetId("") and returning no error. If the remote system is not strongly read-after-write consistent (eventually consistent), this means the resource creation can return no error and also return no resource state.

To prevent this type of Terraform CLI error, the resource implementation should also check against d.IsNewResource() before removing from the Terraform State and returning no error. If that check is true, then remote operation error (or one synthesized from the non-existent status) should be returned instead. While adding this check will not fix the resource implementation to handle the eventually consistent nature of the remote system, the error being returned will be less opaque for operators and code maintainers to troubleshoot.

In the Terraform AWS Provider, an initial fix for the Terraform CLI error will typically look like:

func resourceServiceThingCreate(d *schema.ResourceData, meta interface{}) error {
    /* ... */

    return resourceServiceThingRead(d, meta)
}

func resourceServiceThingRead(d *schema.ResourceData, meta interface{}) error {
    /* ... */

    output, err := conn.DescribeServiceThing(input)

    if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, "ResourceNotFoundException") {
        log.Printf("[WARN] {Service} {Thing} (%s) not found, removing from state", d.Id())
        d.SetId("")
        return nil
    }

    if err != nil {
        return fmt.Errorf("error reading {Service} {Thing} (%s): %w", d.Id(), err)
    }

    /* ... */
}

The Resource Lifecycle Retries documentation in the Contributing Guide also shows how to mitigate eventual consistency issues by retrying the remote operation for a short period of time.

To catch existing occurrences of this code problem and prevent it going forward, a semgrep rule will be added, e.g.

  - id: helper-schema-ResourceData-SetId-empty-without-IsNewResource-check
    languages: [go]
    message: Calling `d.SetId("")` should ensure `!d.IsNewResource()` is checked first
    paths:
      include:
        - aws/resource*.go
    patterns:
      - pattern-either:
        - pattern: |
            d.SetId("")
            ...
            return nil
      - pattern-not-inside: |
          if ... {
            if <... d.IsNewResource() ...> { ... }
            ...
            d.SetId("")
            ...
            return nil
          }
      - pattern-not-inside: |
          if <... d.IsNewResource() ...> { ... }
    severity: WARNING

Affected Resource(s)

https://gist.github.com/bflad/2c7a8df73a1e5e42d8d82af847f9f226

References

@bflad bflad added bug Addresses a defect in current functionality. technical-debt Addresses areas of the codebase that need refactoring or redesign. provider Pertains to the provider itself, rather than any interaction with AWS. linter Pertains to changes to or issues with the various linters. labels Dec 16, 2020
bflad added a commit that referenced this issue Jan 22, 2021
…uring creation and deletion

Reference: #13985
Reference: #16796

Fixing this as part of API Gateway service spike work. As mentioned in the issue this will not help mitigate the cause of the problem, but will at least give a better error message.

Output from acceptance testing:

```
--- PASS: TestAccAWSAPIGatewayMethodSettings_basic (25.35s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_disappears (247.81s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheDataEncrypted (147.06s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheTtlInSeconds (340.17s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CachingEnabled (448.46s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_DataTraceEnabled (216.61s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_LoggingLevel (55.96s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_MetricsEnabled (397.43s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_Multiple (692.67s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_RequireAuthorizationForCacheControl (178.96s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimit (118.97s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimitDisabledByDefault (86.39s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimit (515.72s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimitDisabledByDefault (484.35s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_UnauthorizedCacheControlHeaderStrategy (312.16s)
```
ewbankkit added a commit to ewbankkit/terraform-provider-aws that referenced this issue Jan 22, 2021
…und (hashicorp#15945).

r/aws_route: Implement 'd.IsNewResource()' checksin 'resourceAwsRouteRead' (hashicorp#16796).

Acceptance test output:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSRoute_basic\|TestAccAWSRoute_disappears' ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 2 -run=TestAccAWSRoute_basic\|TestAccAWSRoute_disappears -timeout 120m
=== RUN   TestAccAWSRoute_basic
=== PAUSE TestAccAWSRoute_basic
=== RUN   TestAccAWSRoute_disappears
=== PAUSE TestAccAWSRoute_disappears
=== RUN   TestAccAWSRoute_disappears_RouteTable
=== PAUSE TestAccAWSRoute_disappears_RouteTable
=== CONT  TestAccAWSRoute_basic
=== CONT  TestAccAWSRoute_disappears_RouteTable
--- PASS: TestAccAWSRoute_disappears_RouteTable (34.67s)
=== CONT  TestAccAWSRoute_disappears
--- PASS: TestAccAWSRoute_basic (36.39s)
--- PASS: TestAccAWSRoute_disappears (31.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.620s
bflad added a commit that referenced this issue Feb 3, 2021
…uring creation and deletion (#17234)

Reference: #13985
Reference: #16796

Fixing this as part of API Gateway service spike work. As mentioned in the issue this will not help mitigate the cause of the problem, but will at least give a better error message.

Output from acceptance testing:

```
--- PASS: TestAccAWSAPIGatewayMethodSettings_basic (25.35s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_disappears (247.81s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheDataEncrypted (147.06s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheTtlInSeconds (340.17s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CachingEnabled (448.46s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_DataTraceEnabled (216.61s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_LoggingLevel (55.96s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_MetricsEnabled (397.43s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_Multiple (692.67s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_RequireAuthorizationForCacheControl (178.96s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimit (118.97s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimitDisabledByDefault (86.39s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimit (515.72s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimitDisabledByDefault (484.35s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_UnauthorizedCacheControlHeaderStrategy (312.16s)
```
bflad added a commit that referenced this issue Feb 8, 2021
Reference: #12844
Reference: #15792
Reference: #16796

This page is meant to serve as a reference for all the related retry and waiting logic present in the provider. Further enhancements could discuss resource timeouts in general, however there is some future uncertainty with that functionality so it is currently omitted.
bflad added a commit that referenced this issue Feb 10, 2021
Reference: #12844
Reference: #15792
Reference: #16796

This page is meant to serve as a reference for all the related retry and waiting logic present in the provider. Further enhancements could discuss resource timeouts in general, however there is some future uncertainty with that functionality so it is currently omitted.
ewbankkit added a commit to ewbankkit/terraform-provider-aws that referenced this issue Feb 23, 2021
…f 'nil' when no application found (hashicorp#15945).

r/aws_kinesis_analytics_application: Implement 'd.IsNewResource()' checks in 'resourceAwsKinesisAnalyticsApplicationRead' (hashicorp#16796).

Acceptance test output:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSKinesisAnalyticsApplication_basic\|TestAccAWSKinesisAnalyticsApplication_disappears'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSKinesisAnalyticsApplication_basic\|TestAccAWSKinesisAnalyticsApplication_disappears -timeout 120m
=== RUN   TestAccAWSKinesisAnalyticsApplication_basic
=== PAUSE TestAccAWSKinesisAnalyticsApplication_basic
=== RUN   TestAccAWSKinesisAnalyticsApplication_disappears
=== PAUSE TestAccAWSKinesisAnalyticsApplication_disappears
=== CONT  TestAccAWSKinesisAnalyticsApplication_basic
=== CONT  TestAccAWSKinesisAnalyticsApplication_disappears
--- PASS: TestAccAWSKinesisAnalyticsApplication_basic (15.29s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_disappears (18.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	18.344s
bflad added a commit that referenced this issue Mar 2, 2021
… inconsistent result after apply` with actual error message

Reference: #16796
Reference: #17809

Output from acceptance testing:

```
--- PASS: TestAccAWSStorageGatewayUploadBuffer_basic (455.25s)
```
bflad added a commit that referenced this issue Mar 2, 2021
Reference: #16796

Changes:

- Migrate panic bug/crash labeling from hashibot to GitHub Actions
- Implement bug labeling for `Provider produced inconsistent result after apply` and `Invalid address to set` error messages
- Minor refactor of needs-triage issue labeling to github/issue-labeler for consistency
@ellisroll-b
Copy link

another form for security groups: resource/aws_security_group: error reading Security Group (sg-xxx): couldn't find resource,
Installed hashicorp/aws v3.65.0 (signed by HashiCorp)

staebler pushed a commit to staebler/terraform-provider-aws that referenced this issue Dec 15, 2021
…ency (hashicorp#18391)

* resource/aws_vpc: Handle read-after-create eventual consistency

Reference: hashicorp#16796

Output from acceptance testing in AWS Commercial:

```
--- PASS: TestAccAWSVpc_AssignGeneratedIpv6CidrBlock (82.88s)
--- PASS: TestAccAWSVpc_basic (41.94s)
--- PASS: TestAccAWSVpc_bothDnsOptionsSet (50.15s)
--- PASS: TestAccAWSVpc_classiclinkDnsSupportOptionSet (42.24s)
--- PASS: TestAccAWSVpc_classiclinkOptionSet (42.26s)
--- PASS: TestAccAWSVpc_coreMismatchedDiffs (29.27s)
--- PASS: TestAccAWSVpc_defaultAndIgnoreTags (62.73s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_duplicateTag (4.83s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_nonOverlappingTag (70.21s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_overlappingTag (69.97s)
--- PASS: TestAccAWSVpc_defaultTags_providerOnly (65.84s)
--- PASS: TestAccAWSVpc_defaultTags_updateToProviderOnly (55.23s)
--- PASS: TestAccAWSVpc_defaultTags_updateToResourceOnly (50.23s)
--- PASS: TestAccAWSVpc_DisabledDnsSupport (49.90s)
--- PASS: TestAccAWSVpc_disappears (22.42s)
--- PASS: TestAccAWSVpc_ignoreTags (64.60s)
--- PASS: TestAccAWSVpc_tags (82.89s)
--- PASS: TestAccAWSVpc_Tenancy (84.83s)
--- PASS: TestAccAWSVpc_update (67.73s)
```

Output from acceptance testing in AWS GovCloud (US):

```
--- PASS: TestAccAWSVpc_AssignGeneratedIpv6CidrBlock (85.77s)
--- PASS: TestAccAWSVpc_basic (36.99s)
--- PASS: TestAccAWSVpc_bothDnsOptionsSet (48.65s)
--- PASS: TestAccAWSVpc_coreMismatchedDiffs (32.11s)
--- PASS: TestAccAWSVpc_defaultAndIgnoreTags (63.25s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_duplicateTag (4.70s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_nonOverlappingTag (59.36s)
--- PASS: TestAccAWSVpc_defaultTags_providerAndResource_overlappingTag (59.09s)
--- PASS: TestAccAWSVpc_defaultTags_providerOnly (60.21s)
--- PASS: TestAccAWSVpc_defaultTags_updateToProviderOnly (46.79s)
--- PASS: TestAccAWSVpc_defaultTags_updateToResourceOnly (48.55s)
--- PASS: TestAccAWSVpc_DisabledDnsSupport (47.71s)
--- PASS: TestAccAWSVpc_disappears (23.64s)
--- PASS: TestAccAWSVpc_ignoreTags (62.21s)
--- PASS: TestAccAWSVpc_tags (82.23s)
--- PASS: TestAccAWSVpc_Tenancy (83.42s)
--- PASS: TestAccAWSVpc_update (66.22s)
--- SKIP: TestAccAWSVpc_classiclinkDnsSupportOptionSet (15.15s)
--- SKIP: TestAccAWSVpc_classiclinkOptionSet (15.15s)
```

* Update CHANGELOG for hashicorp#18391
staebler pushed a commit to staebler/terraform-provider-aws that referenced this issue Dec 15, 2021
…r-create eventual consistency (hashicorp#18472)

* resource/aws_vpc_dhcp_options_association: Handle read-after-create eventual consistency

Reference: hashicorp#16142
Reference: hashicorp#16796

Output from acceptance testing in AWS Commercial:

```
--- PASS: TestAccAWSDHCPOptionsAssociation_disappears_vpc (19.27s)
--- PASS: TestAccAWSDHCPOptionsAssociation_disappears_dhcp (25.61s)
--- PASS: TestAccAWSDHCPOptionsAssociation_basic (25.84s)
```

Output from acceptance testing in AWS GovCloud (US):

```
--- PASS: TestAccAWSDHCPOptionsAssociation_disappears_vpc (21.51s)
--- PASS: TestAccAWSDHCPOptionsAssociation_disappears_dhcp (27.55s)
--- PASS: TestAccAWSDHCPOptionsAssociation_basic (28.10s)
```

* Update CHANGELOG for hashicorp#18472

* r/vpc_dhcp_options_association: Tweak delete

Co-authored-by: Dirk Avery <dirk.avery@gmail.com>
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. linter Pertains to changes to or issues with the various linters. provider Pertains to the provider itself, rather than any interaction with AWS. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Projects
None yet
4 participants