From 2c212c8cb37843ce58c02529e54e8018a56e601c Mon Sep 17 00:00:00 2001
From: Doron Lavi <doronlavi@google.com>
Date: Thu, 19 Dec 2024 12:17:51 +0300
Subject: [PATCH] Remove 'diffSuppressSourceRanges' func. It seems this
 function purpose is to suppress "0.0.0.0/0" to "" changes in source ranges.
 However: - 0.0.0.0/0 is not equivalent to "", as "" also include all IPv6
 addresses. - 0.0.0.0/0 also means that source ranges field is specified,
 which is required in some situations (e.g. there is no other source
 specified).

The current function also causes an issue in which removing a source range is not applied - added tests to address such situations.
---
 mmv1/products/compute/Firewall.yaml           |  1 -
 .../terraform/constants/firewall.tmpl         | 35 -----------
 .../resource_compute_firewall_test.go.tmpl    | 61 +++++++++++++++++++
 3 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/mmv1/products/compute/Firewall.yaml b/mmv1/products/compute/Firewall.yaml
index 0fe92be2c8d0..5be07ef06a6a 100644
--- a/mmv1/products/compute/Firewall.yaml
+++ b/mmv1/products/compute/Firewall.yaml
@@ -258,7 +258,6 @@ properties:
       apply. IPv4 or IPv6 ranges are supported. For INGRESS traffic, one of
       `source_ranges`, `source_tags` or `source_service_accounts` is required.
     is_set: true
-    diff_suppress_func: 'diffSuppressSourceRanges'
     item_type:
       type: String
   - name: 'sourceServiceAccounts'
diff --git a/mmv1/templates/terraform/constants/firewall.tmpl b/mmv1/templates/terraform/constants/firewall.tmpl
index af53f902e987..fab1df933feb 100644
--- a/mmv1/templates/terraform/constants/firewall.tmpl
+++ b/mmv1/templates/terraform/constants/firewall.tmpl
@@ -75,38 +75,3 @@ func resourceComputeFirewallSourceFieldsCustomizeDiff(_ context.Context, diff *s
 	return nil
 }
 
-func diffSuppressSourceRanges(k, old, new string, d *schema.ResourceData) bool {
-	if k == "source_ranges.#" {
-		if old == "1" && new == "0" {
-			// Allow diffing on the individual element if we are going from 1 -> 0
-			// this allows for diff suppress on ["0.0.0.0/0"] -> []
-			return true
-		}
-		// For any other source_ranges.# diff, don't suppress
-		return false
-	}
-	kLength := "source_ranges.#"
-	oldLength, newLength := d.GetChange(kLength)
-	oldInt, ok := oldLength.(int)
-
-	if !ok {
-		return false
-	}
-
-	newInt, ok := newLength.(int)
-	if !ok {
-		return false
-	}
-
-	// Diff suppress only should suppress removing the default range
-	// This should probably be newInt == 0, but due to Terraform core internals
-	// (bug?) values found via GetChange may not have the correct new value
-	// in some circumstances
-	if oldInt == 1 && newInt == 1 {
-		if old == "0.0.0.0/0" && new == "" {
-			return true
-		}
-	}
-	// For any other source_ranges value diff, don't suppress
-	return false
-}
\ No newline at end of file
diff --git a/mmv1/third_party/terraform/services/compute/resource_compute_firewall_test.go.tmpl b/mmv1/third_party/terraform/services/compute/resource_compute_firewall_test.go.tmpl
index d0576311d631..16fb81fb63db 100644
--- a/mmv1/third_party/terraform/services/compute/resource_compute_firewall_test.go.tmpl
+++ b/mmv1/third_party/terraform/services/compute/resource_compute_firewall_test.go.tmpl
@@ -94,6 +94,23 @@ func TestAccComputeFirewall_localRanges(t *testing.T) {
 				ImportState:       true,
 				ImportStateVerify: true,
 			},
+			{       
+				Config: testAccComputeFirewall_localRangesRemoveSource(networkName, firewallName),	
+			},
+			{       
+				ResourceName:      "google_compute_firewall.foobar",
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+			{      
+				Config: testAccComputeFirewall_localRangesRemoveDest(networkName, firewallName),
+			},
+			{       
+				ResourceName:      "google_compute_firewall.foobar",
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+
 		},
 	})
 }
@@ -380,6 +397,50 @@ resource "google_compute_firewall" "foobar" {
 `, network, firewall)
 }
 
+func testAccComputeFirewall_localRangesRemoveSource(network, firewall string) string {
+        return fmt.Sprintf(`
+resource "google_compute_network" "foobar" {
+  name                    = "%s"
+  auto_create_subnetworks = false
+}
+
+resource "google_compute_firewall" "foobar" {
+  name        = "%s"
+  description = "Resource created for Terraform acceptance testing"
+  network     = google_compute_network.foobar.name
+  source_tags = ["foo"]
+
+  destination_ranges = ["10.0.0.0/8"]
+
+  allow {
+    protocol = "icmp"
+  }
+}
+`, network, firewall)
+}
+
+func testAccComputeFirewall_localRangesRemoveDest(network, firewall string) string {
+        return fmt.Sprintf(`
+resource "google_compute_network" "foobar" {
+  name                    = "%s"
+  auto_create_subnetworks = false
+}
+        
+resource "google_compute_firewall" "foobar" {
+  name        = "%s"
+  description = "Resource created for Terraform acceptance testing"
+  network     = google_compute_network.foobar.name
+  source_tags = ["foo"]
+  
+  source_ranges = ["10.0.0.0/8"]
+
+  allow {
+    protocol = "icmp"
+  }
+}
+`, network, firewall)
+}
+
 func testAccComputeFirewall_update(network, firewall string) string {
 	return fmt.Sprintf(`
 resource "google_compute_network" "foobar" {