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

Drift google_compute_forwarding_rule ip_address for IPv6 #16400

Closed
LucaPrete opened this issue Oct 30, 2023 · 9 comments
Closed

Drift google_compute_forwarding_rule ip_address for IPv6 #16400

LucaPrete opened this issue Oct 30, 2023 · 9 comments

Comments

@LucaPrete
Copy link
Contributor

LucaPrete commented Oct 30, 2023

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

1.6.2

Affected Resource(s)

  • google_compute_forwarding_rule

Terraform Configuration Files

resource "google_compute_forwarding_rule" "forwarding_rules" {
  provider    = google-beta
  project     = "my-prj"
  name        = "my-fwd-rule"
  region      = "europe-west8"
  ip_address  = "fd20:6c7:bb6a:f400:0:0:0:0/96"
  ip_protocol = "L3_DEFAULT"
  ip_version  = "IPV6"
  backend_service = google_compute_region_backend_service.default.self_link
  load_balancing_scheme = "INTERNAL"
  network               = "my-vpc"
  subnetwork            = "my-subnet"
  allow_global_access   = true
  all_ports             = true
}

Expected Behavior

No drifts are expected.

Actual Behavior

First apply goes well. Second apply/plan causes a drift, given the API returns the short version of the IPv6 address.
For example: ~ ip_address = "fd20:6c7:bb6a:f400:0:0:0:0/96" -> "fd20:6c7:bb6a:f400::" # forces replacement

Steps to Reproduce

Customize the TF I've given and run two times apply / apply+plan

b/308569731

@LucaPrete LucaPrete added the bug label Oct 30, 2023
@LucaPrete
Copy link
Contributor Author

This should be an easy fix in the TF provider. We did the same when we enabled IPv6 for VMs. I'll take care of it.

@github-actions github-actions bot added forward/review In review; remove label to forward service/compute-l7-load-balancer labels Oct 30, 2023
@edwardmedia edwardmedia removed the forward/review In review; remove label to forward label Oct 30, 2023
@LucaPrete
Copy link
Contributor Author

LucaPrete commented Oct 31, 2023

It seems I spoke too soon.... :)

I see that all IPv6 resources are already using the ipv6RepresentationDiffSuppress function, apparently only defined in
mmv1/templates/terraform/constants/router_bgp_peer.erb. Weird, this is consumed also by mmv1/third_party/terraform/services/compute/resource_compute_instance.go.erb, even if the two resource names apparently have nothing to do with each other.

On the other hand, the google_compute_forwarding_rule resource uses for the ipAddress field (which can be either ipv4 or ipv6), the InternalIpDiffSuppress function, defined in mmv1/third_party/terraform/tpgresource/common_diff_suppress.go.erb.

Imho, it would seem appropriate to move the ipv6RepresentationDiffSuppress function in a file/path that is not directly linked to a resource name. Then, I'd call this function from inside the InternalIPDiffSuppress function as well, only if the IP in input is IPv6.

Does any of this makes sense? Any idea on the possible design @rileykarson @edwardmedia @hao-nan-li ?

Thanks

@rileykarson
Copy link
Collaborator

Does your configuration include fd20:6c7:bb6a:f400:0:0:0:0/96 or fd20:6c7:bb6a:f400::? Based on your plan it should be fd20:6c7:bb6a:f400::, but your configuration says fd20:6c7:bb6a:f400:0:0:0:0/96.

A workaround in the meantime is to ensure that your config contains the canonical form (fd20:6c7:bb6a:f400:0:0:0:0/96) which I suspect you've already done based on the discrepancy.

What's a little unusual here is that https://cloud.google.com/compute/docs/reference/rest/v1/forwardingRules specifies that you should have specified an IPv6 address range, but I thought fd20:6c7:bb6a:f400:: was a literal address.

@rileykarson
Copy link
Collaborator

Imho, it would seem appropriate to move the ipv6RepresentationDiffSuppress function in a file/path that is not directly linked to a resource name. Then, I'd call this function from inside the InternalIPDiffSuppress function as well, only if the IP in input is IPv6.

That seems reasonable at first glance, although we should validate that there's no reason we'd want to avoid this approach during review.

@LucaPrete
Copy link
Contributor Author

Does your configuration include fd20:6c7:bb6a:f400:0:0:0:0/96 or fd20:6c7:bb6a:f400::? Based on your plan it should be fd20:6c7:bb6a:f400::, but your configuration says fd20:6c7:bb6a:f400:0:0:0:0/96.

A workaround in the meantime is to ensure that your config contains the canonical form (fd20:6c7:bb6a:f400:0:0:0:0/96) which I suspect you've already done based on the discrepancy.

What's a little unusual here is that https://cloud.google.com/compute/docs/reference/rest/v1/forwardingRules specifies that you should have specified an IPv6 address range, but I thought fd20:6c7:bb6a:f400:: was a literal address.

So, indeed that's not exactly what I have. Let me explain.
I haven't set the ip_address argument.

I guess the post returned the first time the full address (fd20:6c7:bb6a:f400:0:0:0:0/96), which was stored in the terraform state. The second time I ran apply the get returned the short address (fd20:6c7:bb6a:f400::), which caused the drift.
I will also open an internal bug mentioning the post and the get return different results. As a tactical solution I will 1) try to put the address fd20:6c7:bb6a:f400:: in the ip_address field, after the first apply. 2) work on the ipv6RepresentationDiffSuppress function.

@rileykarson
Copy link
Collaborator

rileykarson commented Nov 6, 2023

That's unusual- a config not including the value shouldn't diff at all, in theory.

@LucaPrete
Copy link
Contributor Author

LucaPrete commented Nov 6, 2023

In theory :)

I think I understood what happens. I'll add a few more data points:

  1. I confirm what stated above. I'm not passing any ip_address

  2. It seems InternalIpDiffSuppress already takes care of IPv6 diffs, similarly to what the ipv6RepresentationDiffSuppress does. The comment on top of the function imho is very difficult to understand.

  3. Most importantly, it seems the InternalIpDiffSuppress function doesn't cover the use case that proposed by the APIs. If you look at tests, we don't have a case where we evaluate whether we can consider equal a whole IP address with netmask and one shortened without netmask, which is exactly what I get from the APIs (fd20:6c7:bb6a:f400:0:0:0:0/96 --> fd20:6c7:bb6a:f400::)

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this issue Nov 16, 2023
…ashicorp#9423)

* [hashicorp#16400] Fix and simplify InternalIpDiffSuppress function

* Fixes as per PR comments

* Adding new tests for different netmasks

* fix test

* Fixes to unit tests

* Fixes per comments

* Comments fixes

---------

Co-authored-by: Luca Prete <lucaprete@google.com>
[upstream:06aa6a62bc9aed26c0c890c7bd41625ebca87173]

Signed-off-by: Modular Magician <magic-modules@google.com>
modular-magician added a commit that referenced this issue Nov 16, 2023
…6550)

* [#16400] Fix and simplify InternalIpDiffSuppress function

* Fixes as per PR comments

* Adding new tests for different netmasks

* fix test

* Fixes to unit tests

* Fixes per comments

* Comments fixes

---------


[upstream:06aa6a62bc9aed26c0c890c7bd41625ebca87173]

Signed-off-by: Modular Magician <magic-modules@google.com>
@melinath
Copy link
Collaborator

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 Dec 30, 2023
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

5 participants