google_compute_router: dynamic block leads to undesired diff in plan

[alxdrl]

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

1. change advertised_ip_ranges blocks to a single dynamic block in the google_compute_router resource
2. 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]

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

[edwardmedia]

@aderuelle yes. I think so.

[slevenick]

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]

@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]

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]

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 hashicorp/terraform#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]

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]

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]

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

[LucaPrete]

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

[SarahFrench]

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 GoogleCloudPlatform/magic-modules#10572 solves the problem for end users but means that some testing tools aren't as usable, such as ImportStateVerify.

[LucaPrete]

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]

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

[philip-harvey]

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

[LucaPrete]

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]

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]

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 <#9353 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARY7UDLZJIRCYWRLEHEVLTZH3UHJAVCNFSM46QW3X5KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJXGM2DQNRYGYYA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bahag-klickst]

@LucaPrete
I have just created #18472
Hope it includes everything you need.
If not, feel free to reach out to me

[Maarc-D]

Same kind of unexpected behaviour for consumer_accept_lists on google_compute_service_attachment :/