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

* 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

Signed-off-by: Modular Magician <[email protected]>

Author: modular-magician

.changelog/5253.txt

@@ -0,0 +1,3 @@
+```release-note:breaking-change
+compute: required one of `source_tags`, `source_ranges` or `source_service_accounts` on INGRESS `google_compute_firewall` resources
+```

google/resource_compute_firewall.go

@@ -25,6 +25,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 )
@@ -76,6 +77,28 @@ func resourceComputeFirewallEnableLoggingCustomizeDiff(_ context.Context, diff *
 	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
+}
+
 func resourceComputeFirewall() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceComputeFirewallCreate,
@@ -95,7 +118,10 @@ func resourceComputeFirewall() *schema.Resource {
 
 		SchemaVersion: 1,
 		MigrateState:  resourceComputeFirewallMigrateState,
-		CustomizeDiff: resourceComputeFirewallEnableLoggingCustomizeDiff,
+		CustomizeDiff: customdiff.All(
+			resourceComputeFirewallEnableLoggingCustomizeDiff,
+			resourceComputeFirewallSourceFieldsCustomizeDiff,
+		),
 
 		Schema: map[string]*schema.Schema{
 			"name": {
@@ -164,7 +190,8 @@ must be expressed in CIDR format. Only IPv4 is supported.`,
 				Description: `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. Possible values: ["INGRESS", "EGRESS"]`,
+'source_ranges' OR 'source_tags'. For INGRESS traffic, one of 'source_ranges',
+'source_tags' or 'source_service_accounts' is required. Possible values: ["INGRESS", "EGRESS"]`,
 			},
 			"disabled": {
 				Type:     schema.TypeBool,

google/resource_compute_firewall_generated_test.go

@@ -110,6 +110,8 @@ resource "google_compute_firewall" "rules" {
     protocol  = "tcp"
     ports     = ["80", "8080", "1000-2000"]
   }
+
+  source_tags = ["foo"]
   target_tags = ["web"]
 }
 `, context)

google/resource_compute_firewall_test.go

@@ -2,6 +2,7 @@ package google
 
 import (
 	"fmt"
+	"regexp"
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
@@ -81,12 +82,8 @@ func TestAccComputeFirewall_noSource(t *testing.T) {
 		CheckDestroy: testAccCheckComputeFirewallDestroyProducer(t),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccComputeFirewall_noSource(networkName, firewallName),
-			},
-			{
-				ResourceName:      "google_compute_firewall.foobar",
-				ImportState:       true,
-				ImportStateVerify: true,
+				Config:      testAccComputeFirewall_noSource(networkName, firewallName),
+				ExpectError: regexp.MustCompile("one of source_tags, source_ranges, or source_service_accounts must be defined"),
 			},
 		},
 	})

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.
+

website/docs/r/compute_firewall.html.markdown

@@ -86,6 +86,8 @@ resource "google_compute_firewall" "rules" {
     protocol  = "tcp"
     ports     = ["80", "8080", "1000-2000"]
   }
+
+  source_tags = ["foo"]
   target_tags = ["web"]
 }
 ```
@@ -142,7 +144,8 @@ The following arguments are supported:
   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.
   Possible values are `INGRESS` and `EGRESS`.
 
 * `disabled` -