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

Suppress diffs for trivial changes within sets #28281

Closed
dventer opened this issue Apr 5, 2021 · 4 comments
Closed

Suppress diffs for trivial changes within sets #28281

dventer opened this issue Apr 5, 2021 · 4 comments

Comments

@dventer
Copy link

dventer commented Apr 5, 2021

Terraform Version

Terraform v0.14.9
+ provider registry.terraform.io/hashicorp/google v3.62.0

Terraform Configuration Files

I add host_rule for hosts "beta.domain.tld"

resource "google_compute_url_map" "url_map_a" {
  project = "playground-public"
  name = "url-map-a"
  default_service = google_compute_backend_service.playground_a.id

  host_rule {
    hosts = ["sdwr.domain.tld"]
    path_matcher = "spaths"
  }

  host_rule {
    hosts = ["beta.domain.tld"]
    path_matcher = "bpaths"
  }

  path_matcher {
    name = "spaths"
    default_service = google_compute_backend_service.playground_sbs.id
  }

  path_matcher {
    name = "bpaths"
    default_service = google_compute_backend_service.playground_bbs.id
  }
...

Debug Output

- host_rule {
          - hosts        = [
              - "sdwr.domain.tld",
            ] -> null
          - path_matcher = "spaths" -> null
   }
+ host_rule {
          + hosts        = [
              + "beta.domain.tld",
            ]
          + path_matcher = "bpaths"
    }
 + host_rule {
          + hosts        = [
              + "sdwr.domain.tld",
            ]
          + path_matcher = "spaths"
    }

Crash Output

Expected Behavior

host_rule {
        hosts        = [
            "sdwr.domain.tld",
        ]
        path_matcher = "spaths"
}
+ host_rule {
          + hosts        = [
              + "beta.domain.tld",
            ]
          + path_matcher = "bpaths"
    }

Actual Behavior

Steps to Reproduce

  1. terraform plan
  2. terraform apply

Additional Context

We had open this issue last time, and I thought it will be fix in v0.14, but the problem still happen.
TL;DR
Every part of new host_rule i add, the terraform will re-sort the host_rule order. It will make the terraform looks like removing existing host_rule and then add back the existing host_rule and the new host_rule in the same time.
I am affraid to use this in production, because i have 39 host_rule.
I cannot imagine if i add the 40th host_rule and terraform shows it removes 39 host_rule and then add 39 host_rule back + 1 new host_rule...
Magicly, when i apply it, the existing hosts aren't removed. So, it's like the terraform removes them only in terraform side but not in google cloud side
I have opened it in Google Cloud Provider
host_rule uses TypeSet and i think The TypeSet behaviour is on terraform site not provider site.

References

@dventer dventer added bug new new issue not yet triaged labels Apr 5, 2021
@jbardin
Copy link
Member

jbardin commented Apr 5, 2021

Hi @dventer

Thanks for filing the issue. The explanation in #27018 is unfortunately still the same, and the underlying issue is with a combination of the provider and the provider SDK. The data that terraform is getting back from the provider is showing changes in the set values, and hence terraform must record those as changes.

While the diff output is pruned to create a more compact representation which sometimes leaves the impression that the underlying data is identical, there may be subtle differences and the raw data must not be altered in order to fulfill the API contract between the CLI and the provider. If the provider intends to make no changes to the resource, it is up to the provider to return the original data unchanged. With the legacy SDK this is often much easier said than done however, and until SDK can be fully upgraded to supporte the rich types in Terraform, many providers will have to work around these issues individually.

While we cannot solve this generally for all providers, it's possible that the example shown here could be alleviated somewhat by better heuristics in the diff output for eliminating trivial changes. Since there is nothing to be done in Terraform to prevent the actual changes coming from the provider, perhaps we could look into suppressing the output if it can be done without hindering more complex use cases.

@jbardin jbardin added cli enhancement and removed bug new new issue not yet triaged labels Apr 5, 2021
@jbardin jbardin changed the title TypeSet is always be reordered Suppress diffs for trivial changes within sets Apr 5, 2021
@c2thorn
Copy link

c2thorn commented Jun 25, 2021

Just to be clear, at the provider level there doesn't seem like a way to suppress the diff output. A DiffSuppressFunc compares a before/after on individual keys, but in the case of

- host_rule {
          - hosts        = [
              - "sdwr.domain.tld",
            ] -> null
          - path_matcher = "spaths" -> null
   }
...
+ host_rule {
          + hosts        = [
              + "sdwr.domain.tld",
            ]
          + path_matcher = "spaths"
    }

two separate keys exist. The providers could suppress the entire set output, but this isn't viable where there is an actual change.

I agree that this is mostly an SDK issue, and is pretty much hashicorp/terraform-plugin-sdk#477.

@apparentlymart
Copy link
Contributor

In the meantime since last discussion here, a few things have changed:

  • The modern Terraform plugin framework is now generally available and recommended for developing new providers. This framework was written for modern Terraform, and so properly supports the modern Terraform type system and can directly return set values in the way that modern Terraform expects.

  • As part of gradually prioritizing the needs of framework-based providers, we have phased out some of the concessions that Terraform CLI was making in its plan rendering to try to compensate for legacy SDK quirks, which means that the plan UI is now more likely to show what's really different in the case of SDK misbehavior like was being discussed in this issue.

    Since there are still many popular providers using the old SDK and that will remain true for the foreseeable future, we cannot yet phase out all of those compensations. However, we will continue making adjustments over time as the tradeoff shifts.

Since the root problem of this issue really belongs to the legacy SDK rather than to Terraform Core/CLI, there isn't really a clear definition for when this issue would be "done", and so I'm going to close it now with the expectation that we'll still continue making progress on this over time, as providers gradually adopt the modern plugin framework.

Thanks!

Copy link
Contributor

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 Jun 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants