make one of the source_ parameters required for ingress firewall (#5253)

* make one of the source_ parameters required for ingress firewall * go back to checking not egress in customize diff * add to upgrade guide * update upgrade guide toc
GoogleCloudPlatform · Oct 21, 2021 · c59aeb2 · c59aeb2
1 parent a1f79ef
commit c59aeb2
Show file tree

Hide file tree

Showing 6 changed files with 40 additions and 8 deletions.
diff --git a/mmv1/products/compute/api.yaml b/mmv1/products/compute/api.yaml
@@ -3566,7 +3566,8 @@ objects:
           Direction of traffic to which this firewall applies; default is
           INGRESS. Note: For INGRESS traffic, it is NOT supported to specify
           destinationRanges; For EGRESS traffic, it is NOT supported to specify
-          sourceRanges OR sourceTags.
+          `source_ranges` OR `source_tags`. For INGRESS traffic, one of `source_ranges`,
+          `source_tags` or `source_service_accounts` is required.
         values:
           - :INGRESS
           - :EGRESS

diff --git a/mmv1/templates/terraform/constants/firewall.erb b/mmv1/templates/terraform/constants/firewall.erb
@@ -56,5 +56,27 @@ func resourceComputeFirewallEnableLoggingCustomizeDiff(_ context.Context, diff *
 		return fmt.Errorf("log_config cannot be defined when enable_logging is false")
 	}
 
+	return nil
+}
+
+// Per https://github.com/hashicorp/terraform-provider-google/issues/2924
+// Make one of the source_ parameters Required in ingress google_compute_firewall
+func resourceComputeFirewallSourceFieldsCustomizeDiff(_ context.Context, diff *schema.ResourceDiff, v interface{}) error {
+	direction := diff.Get("direction").(string)
+
+	if direction != "EGRESS" {
+		_, tagsOk := diff.GetOk("source_tags")
+		_, rangesOk := diff.GetOk("source_ranges")
+		_, sasOk := diff.GetOk("source_service_accounts")
+
+		_, tagsExist := diff.GetOkExists("source_tags")
+		// ranges is computed, but this is what we're trying to avoid, so we're not going to check this
+		_, sasExist := diff.GetOkExists("source_service_accounts")
+
+		if !tagsOk && !rangesOk && !sasOk && !tagsExist && !sasExist {
+			return fmt.Errorf("one of source_tags, source_ranges, or source_service_accounts must be defined")
+		}
+	}
+
 	return nil
 }
diff --git a/mmv1/templates/terraform/examples/firewall_with_target_tags.tf.erb b/mmv1/templates/terraform/examples/firewall_with_target_tags.tf.erb
@@ -9,6 +9,8 @@ resource "google_compute_firewall" "<%= ctx[:primary_resource_id] %>" {
     protocol  = "tcp"
     ports     = ["80", "8080", "1000-2000"]
   }
+
+  source_tags = ["foo"]
   target_tags = ["web"]
 }
 # [END vpc_firewall_create]
diff --git a/mmv1/templates/terraform/resource_definition/firewall.erb b/mmv1/templates/terraform/resource_definition/firewall.erb
@@ -14,4 +14,7 @@
 -%>
 SchemaVersion: 1,
 MigrateState:  resourceComputeFirewallMigrateState,
-CustomizeDiff: resourceComputeFirewallEnableLoggingCustomizeDiff,
+CustomizeDiff: customdiff.All(
+		resourceComputeFirewallEnableLoggingCustomizeDiff,
+		resourceComputeFirewallSourceFieldsCustomizeDiff,
+	),
diff --git a/mmv1/third_party/terraform/tests/resource_compute_firewall_test.go.erb b/mmv1/third_party/terraform/tests/resource_compute_firewall_test.go.erb
@@ -83,11 +83,7 @@ func TestAccComputeFirewall_noSource(t *testing.T) {
 		Steps: []resource.TestStep{
 			{
 				Config: testAccComputeFirewall_noSource(networkName, firewallName),
-			},
-			{
-				ResourceName:      "google_compute_firewall.foobar",
-				ImportState:       true,
-				ImportStateVerify: true,
+				ExpectError: regexp.MustCompile("one of source_tags, source_ranges, or source_service_accounts must be defined"),
 			},
 		},
 	})

diff --git a/mmv1/third_party/terraform/website/docs/guides/version_4_upgrade.html.markdown b/mmv1/third_party/terraform/website/docs/guides/version_4_upgrade.html.markdown
@@ -15,6 +15,8 @@ description: |-
     - [Runtime Configurator (`runtimeconfig`) resources have been removed from the GA provider](#runtime-configurator-runtimeconfig-resources-have-been-removed-from-the-ga-provider)
   - [Datasource: `google_product_resource`](#datasource-google_product_resource)
     - [Datasource-level change example](#datasource-level-change-example)
+  - [Resource: `google_compute_firewall`](#resource-google_compute_firewall)
+    - [One of `source_tags`, `source_ranges` or `source_service_accounts` are required on INGRESS firewalls](#one-of-source_tags-source_ranges-or-source_service_accounts-are-required-on-ingress-firewalls)
   - [Resource: `google_compute_instance_group_manager`](#resource-google_compute_instance_group_manager)
     - [`update_policy.min_ready_sec` is removed from the GA provider](#update_policymin_ready_sec-is-removed-from-the-ga-provider)
   - [Resource: `google_compute_region_instance_group_manager`](#resource-google_compute_region_instance_group_manager)
@@ -162,10 +164,15 @@ resource "google_runtimeconfig_config" "my-runtime-config" {
 
 Description of the change and how users should adjust their configuration (if needed).
 
+## Resource: `google_compute_firewall`
+
+### One of `source_tags`, `source_ranges` or `source_service_accounts` are required on INGRESS firewalls
+
+Previously, if all of these fields were left empty, the firewall defaulted to allowing traffic from 0.0.0.0/0, which is a suboptimal default.
+
 ## Resource: `google_compute_instance_group_manager`
 
 ### `update_policy.min_ready_sec` is removed from the GA provider
-
 This field was incorrectly included in the GA `google` provider in past releases.
 In order to continue to use the feature, add `provider = google-beta` to your
 resource definition.
@@ -225,3 +232,4 @@ resource "google_container_cluster" "cluster" {
 This field was incorrectly included in the GA `google` provider in past releases.
 In order to continue to use the feature, add `provider = google-beta` to your
 resource definition.
+