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

Apply results in an incorrect Update for TypeSet with an unexpected empty element #895

Closed
prashantv opened this issue Feb 25, 2022 · 4 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@prashantv
Copy link

prashantv commented Feb 25, 2022

SDK version

github.com/hashicorp/terraform-plugin-sdk/v2 v2.10.1

Relevant provider source code

Repo with a full provider + repro instructions: https://github.com/prashantv/terraform-set-null-repro

Relevant provider code:
https://github.com/prashantv/terraform-set-null-repro/blob/main/internal/provider/service_resource.go

Terraform Configuration Files

This configuration refers to a dummy resource that was created to repro the issue without any external dependencies.
See https://github.com/prashantv/terraform-set-null-repro for details.

To reproduce the issue, namespace is changed to some other value, and then applied. The apply results in an unexpected job set that contains 2 elements, where one is empty.

resource "tftest_service" "s" {
  name    = "svc1"
  job {
    namespace = "NS1"
    search_tags = {
      "foo" = "bar"
    }
  }
}
...

Debug Output

https://gist.github.com/prashantv/b7847ee5e35fa0167b7580cd10598faf

Expected Behavior

Apply to send the provider a single element in the "job" set.

Actual Behavior

Apply causes an Update that has 2 elements in the "job" set, where the first is empty, formatting the panic message:

panic: jobSet has more elements than expected: [
  map[namespace: search_tags:map[]]    ## unexpected empty element in set
  map[namespace:NS2 search_tags:map[foo:bar]]
]

Steps to Reproduce

To reproduce this issue, a repository is created with a specific dummy provider (that has no external dependencies), and with exact repro steps,
https://github.com/prashantv/terraform-set-null-repro

The custom provider is required, as this issue requires a specific setup:

  • TypeSet field which contains a nested schema (rather than just a simple primitive like string)
  • In the nested schema, one of the fields is a TypeMap, and the TypeMap has values specified in the .tf file

References

Originally filed on the terraform repo, but directed to the SDK,
hashicorp/terraform#30581

Additional Information

In case it helps anyone else, we mitigated this issue by using a TypeList instead of TypeSet, so a field change doesn't result in the whole object changing, which seems to avoid the specific bug.

@disaster37
Copy link

Same matter for me

@ewbankkit
Copy link
Contributor

Similar: #588.

@bflad
Copy link
Member

bflad commented Mar 21, 2022

Hi @prashantv 👋 Thank you for raising this and sorry you ran into trouble here. This is certainly unexpected behavior.

It appears this is a similar class of problem as #652, so to consolidate efforts and discussions, I'm going to close this issue in preference of the existing one. Please follow (and potentially 👍 react to) that issue for further updates.

@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, 2022
prashantv added a commit to prashantv/terraform-plugin-sdk that referenced this issue Aug 30, 2022
Fixes hashicorp#895

When calculating a diff, elements in sets are tracked as a delete of all
old attributes with `NewRemoved = true`, and a create of all new
attributes. When `DiffSuppressFunc` is used for an attribute, the
`ResourceAttrDiff` is replaced with `New = Old` (no-op):
https://github.com/hashicorp/terraform-plugin-sdk/blob/fa8d313665945816f6eb6c79182e4abdc1540504/helper/schema/schema.go#L1144

However, this also drops all other metadata about the attr diff, such as
`NewRemoved`. This ends up affecting the `InstanceDiff.applyBlockDiff`
which expects to drop removed items:
https://github.com/hashicorp/terraform-plugin-sdk/blob/e14d3b611f2e257d2a0862e5ea0f90ea96fd5bf8/terraform/diff.go#L207

All attributes of the removed set element get removed here, except those
with `DiffSuppressFunc` since the `NewRemoved` attribute was dropped.

Instead of dropping the metadata about the attr diff, clone the original
attr diff, and just set `New = Old`.

The added test fails without this fix:
```
=== RUN   TestSchemaMap_Diff/30-Set_with_DiffSuppressFunc
    schema_test.go:3188: expected:
        *terraform.InstanceDiff{ [...]"rule.80.duration":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:false, NewRemoved:true, [...]

        got:
        *terraform.InstanceDiff{ [...]"rule.80.duration":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:false, [...] NewRemoved:false, [...]
```

Previously, `NewRemoved` was set to false for the sustain field even though it belonged to an element being removed.
prashantv added a commit to prashantv/terraform-plugin-sdk that referenced this issue Aug 30, 2022
Fixes hashicorp#895

When calculating a diff, elements in sets are tracked as a delete of all
old attributes with `NewRemoved = true`, and a create of all new
attributes. When `DiffSuppressFunc` is used for an attribute, the
`ResourceAttrDiff` is replaced with `New = Old` (no-op):
https://github.com/hashicorp/terraform-plugin-sdk/blob/fa8d313665945816f6eb6c79182e4abdc1540504/helper/schema/schema.go#L1144

However, this also drops all other metadata about the attr diff, such as
`NewRemoved`. This ends up affecting the `InstanceDiff.applyBlockDiff`
which expects to drop removed items:
https://github.com/hashicorp/terraform-plugin-sdk/blob/e14d3b611f2e257d2a0862e5ea0f90ea96fd5bf8/terraform/diff.go#L207

All attributes of the removed set element get removed here, except those
with `DiffSuppressFunc` since the `NewRemoved` attribute was dropped.

Instead of dropping the metadata about the attr diff, clone the original
attr diff, and just set `New = Old`.

The added test fails without this fix:
```
=== RUN   TestSchemaMap_Diff/30-Set_with_DiffSuppressFunc
    schema_test.go:3188: expected:
        *terraform.InstanceDiff{ [...]"rule.80.duration":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:false, NewRemoved:true, [...]

        got:
        *terraform.InstanceDiff{ [...]"rule.80.duration":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:false, [...] NewRemoved:false, [...]
```

Previously, `NewRemoved` was set to false for the sustain field even though it belonged to an element being removed.
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

No branches or pull requests

4 participants