google_compute_router: dynamic block leads to undesired diff in plan

alxdrl commented 3 years ago

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 me too comments, 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.
If an issue is assigned to the modular-magician user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If an issue is assigned to a user, that user is claiming responsibility for the issue. If an issue is assigned to hashibot, a community member has claimed the issue already.

Terraform Version

Terraform v0.13.7
+ provider registry.terraform.io/hashicorp/google v3.71.0
+ provider registry.terraform.io/hashicorp/google-beta v3.71.0

Affected Resource(s)

google_compute_router

Terraform Configuration Files

Original configuration with individual advertised_ip_ranges blocks

resource "google_compute_router" "r1" {
  name    = "r1"
  network = "default"

  bgp {
    asn               = 12345
    advertise_mode    = "CUSTOM"
    advertised_groups = ["ALL_SUBNETS"]
    advertised_ip_ranges {
      range = "35.199.192.0/19"
    }
    advertised_ip_ranges {
      range = "10.240.24.0/22"
    }
  }
}

Update configuration with single dynamic advertised_ip_ranges block

resource "google_compute_router" "r1" {
  name    = "r1"
  network = "default"

  bgp {
    asn               = 12345
    advertise_mode    = "CUSTOM"
    advertised_groups = ["ALL_SUBNETS"]
    dynamic advertised_ip_ranges {
      for_each = toset([ "35.199.192.0/19", "10.240.24.0/22" ])
      content {
        range = advertised_ip_ranges.value
      }
    }
  }
}

Debug Output

N/A

Panic Output

N/A

Expected Behavior

No change to plan

------------------------------------------------------------------------

No changes. Infrastructure is up-to-date.

This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, no
actions need to be performed.

Actual Behavior

Plan shows resource modification should apply

 # google_compute_router.r1 will be updated in-place
  ~ resource "google_compute_router" "r1" {
        creation_timestamp = "2021-01-27T05:06:12.863-08:00"
        id                 = "projects/my-project-1234/regions/europe-west1/routers/r1"
        name               = "r1"
        network            = "https://www.googleapis.com/compute/v1/projects/my-project-1234/global/networks/default"
        project            = "my-project-1234"
        region             = "europe-west1"
        self_link          = "https://www.googleapis.com/compute/v1/projects/my-project-1234/regions/europe-west1/routers/r1"

      ~ bgp {
            advertise_mode    = "CUSTOM"
            advertised_groups = [
                "ALL_SUBNETS",
            ]
            asn               = 12345

          ~ advertised_ip_ranges {
              ~ range = "35.199.192.0/19" -> "10.240.24.0/22"
            }
          ~ advertised_ip_ranges {
              ~ range = "10.240.24.0/22" -> "35.199.192.0/19"
            }
        }
    }

Steps to Reproduce

change advertised_ip_ranges blocks to a single dynamic block in the google_compute_router resource
terraform apply

Important Factoids

diff still shows up even after accepting and applying the change
changing the order of values in the set doesn't help

References

N/A

b/318814729

alxdrl commented 3 years ago

Hi @edwardmedia, should advertised_ip_ranges schema definition be of TypeSet instead of TypeList there ? or ?

edwardmedia commented 3 years ago

@aderuelle yes. I think so.

slevenick commented 3 years ago

Hm, yeah that should probably be a set in the schema. You should be able to fix your diff by using tolist rather than toset and ordering your list to match the way the API is returning these fields.

joe-a-t commented 3 years ago

@slevenick and @edwardmedia re-ordering the items on our side doesn't work since we are using a dynamic block. Is there any update on fixing this or can I just make a PR myself to https://github.com/hashicorp/terraform-provider-google/blob/master/google/resource_compute_router.go#L110 changing List to Set and get it reviewed/merged?

slevenick commented 3 years ago

Changing from toset to tolist and reordering it doesn't fix the issue? It should work even with a dynamic block, at least according to my testing.

Changing from a List to a Set in the schema is possible, but may be considered a breaking change as it breaks interpolation on the field

joe-a-t commented 3 years ago

We use it from within a module and given the way that we pass around/transform the relevant variable, a list is not working for us. I'm a bit confused why fixing the schema would be considered a breaking change. Related to https://github.com/hashicorp/terraform/issues/28803 I've seen a bunch of other providers/resources make this exact list to set schema change without issues or having to declare it a breaking change.

slevenick commented 3 years ago

We technically consider it a breaking change because it can break user configs that interpolate based on the current ordering of the List field. Changing to a set changes the order which will cause diffs for anyone using interpolation on that field.

That said, it is definitely something that we would consider doing. It would likely involve changing the behavior in google_compute_router_peer as well, as that is essentially the same resource

duxbuse commented 1 year ago

Yeah this is a huge pain, I even tried to sort the list myself but since map keys are always iterated in lexicographical order it makes 100.0.0.0 come before 2.0.0.0.

edit: here is a way to make it sortable by 0 padding the values, and disregarding any eg./23 cidr annotations.

join(".",[ for x in split(".", "10.116.192.0/19") : format("%03d", replace(x, "/^(\\d{1,3})/\\d{1,2}$/", "$1")) ])

"10.116.192.0/19" -> "010.116.192.000"

SarahFrench commented 6 months ago

This issue is going to be addressed in the short term by https://github.com/GoogleCloudPlatform/magic-modules/pull/10572, and in the long term by making the field a Set (https://github.com/GoogleCloudPlatform/magic-modules/pull/10118#issuecomment-2088319514). I'll mark this issue as something for v6.0

LucaPrete commented 5 months ago

This should be fixed now. Should we close it @SarahFrench ?

SarahFrench commented 5 months ago

I left the issue open so it reminds the team to make the long-term fix of changing the field to a Set as part of the 6.0.0 release - I'll double check that's still the plan and close this issue if that's not the case

Edit: we're ok to change to a Set as the reordering added in https://github.com/GoogleCloudPlatform/magic-modules/pull/10572 solves the problem for end users but means that some testing tools aren't as usable, such as ImportStateVerify.

LucaPrete commented 5 months ago

sgtm @SarahFrench FYI I opened another PR with the fix as per your comment. Worst case we can use that one in the major release.

SarahFrench commented 5 months ago

Thanks! I left a note on that PR about needing to remove the list reordering added in https://github.com/GoogleCloudPlatform/magic-modules/pull/10572 but I'll otherwise leave review to the assigned reviewer

philip-harvey commented 5 months ago

This PR, https://github.com/GoogleCloudPlatform/magic-modules/pull/10572 seems to have caused constant drift, every apply Terraform tried to re-order order advertised_ip_ranges properties according to the order in config

LucaPrete commented 5 months ago

This PR, GoogleCloudPlatform/magic-modules#10572 seems to have caused constant drift, every apply Terraform tried to re-order order advertised_ip_ranges properties according to the order in config

Hi @philip-harvey ! This is definitely bad and undesired. Can you please open a bug for this and let me know there how to reproduce it? I'm surprised our tests didn't get this..

bahag-klickst commented 4 months ago

Hej @philip-harvey

Did you already open a bug for this? If not, I can also do it as I have a working config which shows the permadiff. @LucaPrete What do you need from my side, if the bug is not yet opened?

LucaPrete commented 4 months ago

Can you please open a bug here https://github.com/hashicorp/terraform-provider-google/issues and put a configuration to replicate the issue?

Thanks!

Il giorno lun 17 giu 2024 alle ore 16:04 Tim Klicks < @.***> ha scritto:

Hej @philip-harvey https://github.com/philip-harvey

Did you already open a bug for this? If not, I can also do it as I have a working config which shows the permadiff. @LucaPrete https://github.com/LucaPrete What do you need from my side, if the bug is not yet opened?

— Reply to this email directly, view it on GitHub https://github.com/hashicorp/terraform-provider-google/issues/9353#issuecomment-2173486860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARY7UDLZJIRCYWRLEHEVLTZH3UHJAVCNFSM46QW3X5KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJXGM2DQNRYGYYA . You are receiving this because you were mentioned.Message ID: @.***>

bahag-klickst commented 4 months ago

@LucaPrete I have just created https://github.com/hashicorp/terraform-provider-google/issues/18472 Hope it includes everything you need. If not, feel free to reach out to me

Maarc-D commented 2 months ago

Same kind of unexpected behaviour for consumer_accept_lists on google_compute_service_attachment :/

hashicorp / terraform-provider-google