From d4eed572ed8244dbfdcab11bfc0039cee326c364 Mon Sep 17 00:00:00 2001 From: Modular Magician Date: Thu, 16 Nov 2023 16:23:19 +0000 Subject: [PATCH] [#16400] Fix and simplify InternalIpDiffSuppress function (#9423) * [#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 [upstream:06aa6a62bc9aed26c0c890c7bd41625ebca87173] Signed-off-by: Modular Magician --- .changelog/9423.txt | 3 + google/tpgresource/common_diff_suppress.go | 45 +++++++--- .../tpgresource/common_diff_suppress_test.go | 82 ++++++++++++++----- 3 files changed, 98 insertions(+), 32 deletions(-) create mode 100644 .changelog/9423.txt diff --git a/.changelog/9423.txt b/.changelog/9423.txt new file mode 100644 index 00000000000..d281cafd4b6 --- /dev/null +++ b/.changelog/9423.txt @@ -0,0 +1,3 @@ +```release-note:bug +Fix (and simplify) InternalIpDiffSuppress function to suppress drifts for IPv6 with and without netmask. +``` diff --git a/google/tpgresource/common_diff_suppress.go b/google/tpgresource/common_diff_suppress.go index fdcf280c459..a4e104cdb4f 100644 --- a/google/tpgresource/common_diff_suppress.go +++ b/google/tpgresource/common_diff_suppress.go @@ -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 diff --git a/google/tpgresource/common_diff_suppress_test.go b/google/tpgresource/common_diff_suppress_test.go index 26427bc31af..0ad87e0ca18 100644 --- a/google/tpgresource/common_diff_suppress_test.go +++ b/google/tpgresource/common_diff_suppress_test.go @@ -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 {