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

helper/schema: Remove timeouts during ImportResourceState #1146

Merged
merged 3 commits into from Feb 14, 2023

Conversation

bflad
Copy link
Member

@bflad bflad commented Feb 13, 2023

Closes #1145
Reference: hashicorp/terraform#32463

Terraform 1.3.8 and later now correctly handles null values for single nested blocks. This means Terraform will now report differences between a null block and known block with null values. This SDK only supported single nested blocks via its timeouts functionality.

This change is a very targeted removal of any potential timeouts block values in a resource state from the ImportResourceState RPC. Since configuration is not available during that RPC, it is never valid to return any data beyond null for that block. This will prevent unexpected differences on the first plan after import, where Terraform will report the block removal for configurations which do not contain the block.

New unit test failure prior to code updates:

--- FAIL: TestImportResourceState_Timeouts_Removed (0.00s)
    /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema/grpc_provider_test.go:1159: unexpected difference:   cty.Value(
        - 	{
        - 		ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}},
        - 		v:  map[string]any{"id": string("test"), "string_attribute": nil, "timeouts": nil},
        - 	},
        + 	{
        + 		ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}},
        + 		v: map[string]any{
        + 			"id":               string("test"),
        + 			"string_attribute": nil,
        + 			"timeouts":         map[string]any{"create": nil, "read": nil},
        + 		},
        + 	},
          )

Reference: #1145
Reference: hashicorp/terraform#32463

Terraform 1.3.8 and later now correctly handles null values for single nested blocks. This means Terraform will now report differences between a null block and known block with null values. This SDK only supported single nested blocks via its timeouts functionality.

This change is a very targeted removal of any potential `timeouts` block values in a resource state from the `ImportResourceState` RPC. Since configuration is not available during that RPC, it is never valid to return any data beyond null for that block. This will prevent unexpected differences on the first plan after import, where Terraform will report the block removal for configurations which do not contain the block.

New unit test failure prior to code updates:

```
--- FAIL: TestImportResourceState_Timeouts_Removed (0.00s)
    /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema/grpc_provider_test.go:1159: unexpected difference:   cty.Value(
        - 	{
        - 		ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}},
        - 		v:  map[string]any{"id": string("test"), "string_attribute": nil, "timeouts": nil},
        - 	},
        + 	{
        + 		ty: cty.Type{typeImpl: cty.typeObject{AttrTypes: map[string]cty.Type{...}}},
        + 		v: map[string]any{
        + 			"id":               string("test"),
        + 			"string_attribute": nil,
        + 			"timeouts":         map[string]any{"create": nil, "read": nil},
        + 		},
        + 	},
          )
```
@bflad bflad added the bug Something isn't working label Feb 13, 2023
@bflad bflad added this to the v2.25.0 milestone Feb 13, 2023
@bflad bflad requested a review from a team as a code owner February 13, 2023 20:00
@bflad bflad requested a review from jbardin February 13, 2023 20:03
@bflad
Copy link
Member Author

bflad commented Feb 13, 2023

This cannot be verified via provider acceptance testing on Terraform 1.3.8 as TestStep.ImportStateVerify is explicitly coded to ignore timeouts differences. 😖 Rather than dealing with removing that special testing logic, I verified this downstream by rebuilding terraform-provider-aws, using developer overrides, and running an aws_security_group import and plan.

$ terraform import aws_security_group.test sg-0329f3c7cf561ad15                                                       
aws_security_group.test: Importing from ID "sg-0329f3c7cf561ad15"...
aws_security_group.test: Import prepared!
  Prepared aws_security_group for import
aws_security_group.test: Refreshing state... [id=sg-0329f3c7cf561ad15]

Import successful!

The resources that were imported are shown above. These resources are now in
your Terraform state and will henceforth be managed by Terraform.

$ terraform plan

│ Warning: Provider development overrides are in effect

│ The following provider development overrides are set in the CLI configuration:
│  - hashicorp/aws in /Users/bflad/go/bin

│ The behavior may therefore not match any released version of the provider and applying changes may cause the state to become incompatible with published releases.

aws_security_group.test: Refreshing state... [id=sg-0329f3c7cf561ad15]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.

Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet the timeouts are being added in HCL2ValueFromFlatmap which doesn't make use of the schema, and NormalizeObjectFromLegacySDK doesn't try to strip them out because it's a technically valid value. One of those functions should have been where this was done originally, but it probably doesn't matter so much at this point.

bflad added a commit that referenced this pull request Feb 13, 2023
…eVerify

Reference: #576
Reference: #1146

This change reverts #576 as it prevents acceptance testing from detecting Terraform 1.3.8 and later potentially returning an unexpected plan after import involving the `timeouts` block. #1146 fixes the import logic to never include a known value for `timeouts`, which is what the acceptance testing was trying to say when this testing logic was not in place.

With this change, but without #1146 on a resource known to have the unexpected plan after import on Terraform 1.3.8:

```
=== CONT  TestAccVPCSecurityGroup_basic
    vpc_security_group_test.go:1002: Step 2/2 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.

          map[string]string{
        +       "timeouts.%": "2",
          }
--- FAIL: TestAccVPCSecurityGroup_basic (21.54s)
```

With this change and with #1146:

```
=== CONT  TestAccVPCSecurityGroup_basic
--- PASS: TestAccVPCSecurityGroup_basic (20.23s)
```

Verified both ways on Terraform 1.2.9 as well (as a smoke test for a Terraform version prior to 1.3.8).
bflad added a commit to hashicorp/terraform-plugin-testing that referenced this pull request Feb 13, 2023
…eVerify

Reference: hashicorp/terraform-plugin-sdk#1146
Reference: hashicorp/terraform-plugin-sdk#1147

This change reverts hashicorp/terraform-plugin-sdk#576 as it prevents acceptance testing from detecting Terraform 1.3.8 and later potentially returning an unexpected plan after import involving the `timeouts` block. hashicorp/terraform-plugin-sdk#1146 fixes the import logic to never include a known value for `timeouts`, which is what the acceptance testing was trying to say when this testing logic was not in place.

With this change, but without hashicorp/terraform-plugin-sdk#1146 on a resource known to have the unexpected plan after import on Terraform 1.3.8:

```
=== CONT  TestAccVPCSecurityGroup_basic
    vpc_security_group_test.go:1002: Step 2/2 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.

          map[string]string{
        +       "timeouts.%": "2",
          }
--- FAIL: TestAccVPCSecurityGroup_basic (21.54s)
```

With this change and with hashicorp/terraform-plugin-sdk#1146:

```
=== CONT  TestAccVPCSecurityGroup_basic
--- PASS: TestAccVPCSecurityGroup_basic (20.23s)
```

Verified both ways on Terraform 1.2.9 as well (as a smoke test for a Terraform version prior to 1.3.8).
@bflad bflad merged commit 3933b61 into main Feb 14, 2023
@bflad bflad deleted the bflad/no-timeouts-on-import branch February 14, 2023 14:17
@github-actions
Copy link

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 contributions.
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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImportResourceState Errantly Includes timeouts in State
2 participants