Skip to content

Commit

Permalink
[#16400] Fix and simplify InternalIpDiffSuppress function (#9423) (#1…
Browse files Browse the repository at this point in the history 
…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>
  • Loading branch information
@modular-magician
modular-magician authored Nov 16, 2023
1 parent f9b45fb commit 0f78291
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 32 deletions.
3 changes: 3 additions & 0 deletions .changelog/9423.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
Fix (and simplify) InternalIpDiffSuppress function to suppress drifts for IPv6 with and without netmask.
```
45 changes: 34 additions & 11 deletions google/tpgresource/common_diff_suppress.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,25 +180,48 @@ func TimestampDiffSuppress(format string) schema.SchemaDiffSuppressFunc {
}
}

// suppress diff when saved is Ipv4 and Ipv6 format while new is required a reference
// this happens for an internal ip for Private Services Connect
// Suppresses diff for IPv4 and IPv6 different formats.
// It also suppresses diffs if an IP is changing to a reference.
func InternalIpDiffSuppress(_, old, new string, _ *schema.ResourceData) bool {
olds := strings.Split(old, "/")
news := strings.Split(new, "/")
addr_equality := false
netmask_equality := false

if len(olds) == 2 {
if len(news) == 2 {
return bytes.Equal(net.ParseIP(olds[0]), net.ParseIP(news[0])) && olds[1] == news[1]
addr_netmask_old := strings.Split(old, "/")
addr_netmask_new := strings.Split(new, "/")

// Check if old or new are IPs (with or without netmask)
var addr_old net.IP
if net.ParseIP(addr_netmask_old[0]) == nil {
addr_old = net.ParseIP(old)
} else {
addr_old = net.ParseIP(addr_netmask_old[0])
}
var addr_new net.IP
if net.ParseIP(addr_netmask_new[0]) == nil {
addr_new = net.ParseIP(new)
} else {
addr_new = net.ParseIP(addr_netmask_new[0])
}

if addr_old != nil {
if addr_new == nil {
// old is an IP and new is a reference
addr_equality = true
} else {
return (net.ParseIP(olds[0]) != nil) && (net.ParseIP(new) == nil)
// old and new are IP addresses
addr_equality = bytes.Equal(addr_old, addr_new)
}
}

if (net.ParseIP(old) != nil) && (net.ParseIP(new) != nil) {
return bytes.Equal(net.ParseIP(old), net.ParseIP(new))
// If old and new both have a netmask compare them, otherwise suppress
// This is not technically correct but prevents the permadiff described in https://github.com/hashicorp/terraform-provider-google/issues/16400
if (len(addr_netmask_old)) == 2 && (len(addr_netmask_new) == 2) {
netmask_equality = addr_netmask_old[1] == addr_netmask_new[1]
} else {
netmask_equality = true
}

return (net.ParseIP(old) != nil) && (net.ParseIP(new) == nil)
return addr_equality && netmask_equality
}

// Suppress diffs for duration format. ex "60.0s" and "60s" same
Expand Down
82 changes: 61 additions & 21 deletions google/tpgresource/common_diff_suppress_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,56 +292,96 @@ func TestInternalIpDiffSuppress(t *testing.T) {
Old, New string
ExpectDiffSuppress bool
}{
"same1 ipv6s": {
"suppress - same long and short ipv6 IPs without netmask": {
Old: "2600:1900:4020:31cd:8000:0:0:0",
New: "2600:1900:4020:31cd:8000::",
ExpectDiffSuppress: true,
},
"same2 ipv6s": {
"suppress - long and short ipv6 IPs with netmask": {
Old: "2600:1900:4020:31cd:8000:0:0:0/96",
New: "2600:1900:4020:31cd:8000::/96",
ExpectDiffSuppress: true,
},
"same3 ipv6s": {
"suppress - long ipv6 IP with netmask and short ipv6 IP without netmask": {
Old: "2600:1900:4020:31cd:8000:0:0:0/96",
New: "https://www.googleapis.com/compute/v1/projects/myproject/regions/us-central1/addresses/myaddress",
New: "2600:1900:4020:31cd:8000::",
ExpectDiffSuppress: true,
},
"different1 ipv6s": {
"suppress - long ipv6 IP without netmask and short ipv6 IP with netmask": {
Old: "2600:1900:4020:31cd:8000:0:0:0",
New: "2600:1900:4020:31cd:8001::",
ExpectDiffSuppress: false,
New: "2600:1900:4020:31cd:8000::/96",
ExpectDiffSuppress: true,
},
"different2 ipv6s": {
"suppress - long ipv6 IP with netmask and reference": {
Old: "2600:1900:4020:31cd:8000:0:0:0/96",
New: "projects/project_id/regions/region/addresses/address-name",
ExpectDiffSuppress: true,
},
"suppress - long ipv6 IP without netmask and reference": {
Old: "2600:1900:4020:31cd:8000:0:0:0",
New: "2600:1900:4020:31cd:8000:0:0:8000",
ExpectDiffSuppress: false,
New: "projects/project_id/regions/region/addresses/address-name",
ExpectDiffSuppress: true,
},
"different3 ipv6s": {
"do not suppress - ipv6 IPs different netmask": {
Old: "2600:1900:4020:31cd:8000:0:0:0/96",
New: "2600:1900:4020:31cd:8001::/96",
New: "2600:1900:4020:31cd:8000:0:0:0/95",
ExpectDiffSuppress: false,
},
"different ipv4s": {
Old: "1.2.3.4",
New: "1.2.3.5",
"do not suppress - reference and ipv6 IP with netmask": {
Old: "projects/project_id/regions/region/addresses/address-name",
New: "2600:1900:4020:31cd:8000:0:0:0/96",
ExpectDiffSuppress: false,
},
"do not suppress - ipv6 IPs - 1": {
Old: "2600:1900:4020:31cd:8000:0:0:0",
New: "2600:1900:4020:31cd:8001::",
ExpectDiffSuppress: false,
},
"do not suppress - ipv6 IPs - 2": {
Old: "2600:1900:4020:31cd:8000:0:0:0",
New: "2600:1900:4020:31cd:8000:0:0:8000",
ExpectDiffSuppress: false,
},
"same ipv4s": {
"suppress - ipv4 IPs": {
Old: "1.2.3.4",
New: "1.2.3.4",
ExpectDiffSuppress: true,
},
"ipv4 vs id": {
"suppress - ipv4 IP without netmask and ipv4 IP with netmask": {
Old: "1.2.3.4",
New: "google_compute_address.my_ipv4_addr.address",
New: "1.2.3.4/24",
ExpectDiffSuppress: true,
},
"ipv6 vs id": {
Old: "2600:1900:4020:31cd:8000:0:0:0",
New: "google_compute_address.my_ipv6_addr.address",
"suppress - ipv4 IP without netmask and reference": {
Old: "1.2.3.4",
New: "projects/project_id/regions/region/addresses/address-name",
ExpectDiffSuppress: true,
},
"do not suppress - reference and ipv4 IP without netmask": {
Old: "projects/project_id/regions/region/addresses/address-name",
New: "1.2.3.4",
ExpectDiffSuppress: false,
},
"do not suppress - different ipv4 IPs": {
Old: "1.2.3.4",
New: "1.2.3.5",
ExpectDiffSuppress: false,
},
"do not suppress - ipv4 IPs different netmask": {
Old: "1.2.3.4/24",
New: "1.2.3.5/25",
ExpectDiffSuppress: false,
},
"do not suppress - different references": {
Old: "projects/project_id/regions/region/addresses/address-name",
New: "projects/project_id/regions/region/addresses/address-name-1",
ExpectDiffSuppress: false,
},
"do not suppress - same references": {
Old: "projects/project_id/regions/region/addresses/address-name",
New: "projects/project_id/regions/region/addresses/address-name",
ExpectDiffSuppress: false,
},
}

for tn, tc := range cases {
Expand Down

0 comments on commit 0f78291

Please sign in to comment.