add firewall logging controls (#3780) (#2310)

* add firewall logging controls

* make backward compatible

* check enable_logging in expand

* update docs

* update expand logic to fix failing test

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

Author: modular-magician

.changelog/3780.txt

@@ -0,0 +1,6 @@
+```release-note:enhancement
+compute: added `logConfig.metadata` to `google_compute_firewall`, defining this will enable logging.
+```
+```release-note:deprecation
+compute: deprecated `enableLogging` on `google_compute_firewall`, define `logConfig.metadata` to enable logging instead.
+```

google-beta/resource_compute_firewall.go

@@ -27,7 +27,6 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/helper/hashcode"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
-	"google.golang.org/api/googleapi"
 )
 
 func resourceComputeFirewallRuleHash(v interface{}) int {
@@ -53,6 +52,30 @@ func compareCaseInsensitive(k, old, new string, d *schema.ResourceData) bool {
 	return strings.ToLower(old) == strings.ToLower(new)
 }
 
+func diffSuppressEnableLogging(k, old, new string, d *schema.ResourceData) bool {
+	if k == "log_config.#" {
+		if new == "0" && d.Get("enable_logging").(bool) {
+			return true
+		}
+	}
+
+	return false
+}
+
+func resourceComputeFirewallEnableLoggingCustomizeDiff(diff *schema.ResourceDiff, v interface{}) error {
+	enableLogging, enableExists := diff.GetOkExists("enable_logging")
+	if !enableExists {
+		return nil
+	}
+
+	logConfigExists := diff.Get("log_config.#").(int) != 0
+	if logConfigExists && enableLogging == false {
+		return fmt.Errorf("log_config cannot be defined when enable_logging is false")
+	}
+
+	return nil
+}
+
 func resourceComputeFirewall() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceComputeFirewallCreate,
@@ -72,6 +95,7 @@ func resourceComputeFirewall() *schema.Resource {
 
 		SchemaVersion: 1,
 		MigrateState:  resourceComputeFirewallMigrateState,
+		CustomizeDiff: resourceComputeFirewallEnableLoggingCustomizeDiff,
 
 		Schema: map[string]*schema.Schema{
 			"name": {
@@ -150,14 +174,24 @@ network it is associated with. When set to true, the firewall rule is
 not enforced and the network behaves as if it did not exist. If this
 is unspecified, the firewall rule will be enabled.`,
 			},
-			"enable_logging": {
-				Type:     schema.TypeBool,
-				Optional: true,
-				Description: `This field denotes whether to enable logging for a particular
-firewall rule. If logging is enabled, logs will be exported to
-Stackdriver.`,
+			"log_config": {
+				Type:             schema.TypeList,
+				Optional:         true,
+				DiffSuppressFunc: diffSuppressEnableLogging,
+				Description: `This field denotes the logging options for a particular firewall rule.
+If defined, logging is enabled, and logs will be exported to Cloud Logging.`,
+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"metadata": {
+							Type:         schema.TypeString,
+							Required:     true,
+							ValidateFunc: validation.StringInSlice([]string{"EXCLUDE_ALL_METADATA", "INCLUDE_ALL_METADATA"}, false),
+							Description:  `This field denotes whether to include or exclude metadata for firewall logs. Possible values: ["EXCLUDE_ALL_METADATA", "INCLUDE_ALL_METADATA"]`,
+						},
+					},
+				},
 			},
-
 			"priority": {
 				Type:         schema.TypeInt,
 				Optional:     true,
@@ -261,6 +295,13 @@ instances on the specified network.`,
 				Computed:    true,
 				Description: `Creation timestamp in RFC3339 text format.`,
 			},
+			"enable_logging": {
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Computed:    true,
+				Deprecated:  "Deprecated in favor of log_config",
+				Description: "This field denotes whether to enable logging for a particular firewall rule. If logging is enabled, logs will be exported to Stackdriver.",
+			},
 			"project": {
 				Type:     schema.TypeString,
 				Optional: true,
@@ -375,10 +416,10 @@ func resourceComputeFirewallCreate(d *schema.ResourceData, meta interface{}) err
 	} else if v, ok := d.GetOkExists("disabled"); ok || !reflect.DeepEqual(v, disabledProp) {
 		obj["disabled"] = disabledProp
 	}
-	logConfigProp, err := expandComputeFirewallLogConfig(nil, d, config)
+	logConfigProp, err := expandComputeFirewallLogConfig(d.Get("log_config"), d, config)
 	if err != nil {
 		return err
-	} else if !isEmptyValue(reflect.ValueOf(logConfigProp)) {
+	} else if v, ok := d.GetOkExists("log_config"); ok || !reflect.DeepEqual(v, logConfigProp) {
 		obj["logConfig"] = logConfigProp
 	}
 	nameProp, err := expandComputeFirewallName(d.Get("name"), d, config)
@@ -509,18 +550,8 @@ func resourceComputeFirewallRead(d *schema.ResourceData, meta interface{}) error
 	if err := d.Set("disabled", flattenComputeFirewallDisabled(res["disabled"], d, config)); err != nil {
 		return fmt.Errorf("Error reading Firewall: %s", err)
 	}
-	// Terraform must set the top level schema field, but since this object contains collapsed properties
-	// it's difficult to know what the top level should be. Instead we just loop over the map returned from flatten.
-	if flattenedProp := flattenComputeFirewallLogConfig(res["logConfig"], d, config); flattenedProp != nil {
-		if gerr, ok := flattenedProp.(*googleapi.Error); ok {
-			return fmt.Errorf("Error reading Firewall: %s", gerr)
-		}
-		casted := flattenedProp.([]interface{})[0]
-		if casted != nil {
-			for k, v := range casted.(map[string]interface{}) {
-				d.Set(k, v)
-			}
-		}
+	if err := d.Set("log_config", flattenComputeFirewallLogConfig(res["logConfig"], d, config)); err != nil {
+		return fmt.Errorf("Error reading Firewall: %s", err)
 	}
 	if err := d.Set("name", flattenComputeFirewallName(res["name"], d, config)); err != nil {
 		return fmt.Errorf("Error reading Firewall: %s", err)
@@ -592,10 +623,10 @@ func resourceComputeFirewallUpdate(d *schema.ResourceData, meta interface{}) err
 	} else if v, ok := d.GetOkExists("disabled"); ok || !reflect.DeepEqual(v, disabledProp) {
 		obj["disabled"] = disabledProp
 	}
-	logConfigProp, err := expandComputeFirewallLogConfig(nil, d, config)
+	logConfigProp, err := expandComputeFirewallLogConfig(d.Get("log_config"), d, config)
 	if err != nil {
 		return err
-	} else if !isEmptyValue(reflect.ValueOf(logConfigProp)) {
+	} else if v, ok := d.GetOkExists("log_config"); ok || !reflect.DeepEqual(v, logConfigProp) {
 		obj["logConfig"] = logConfigProp
 	}
 	networkProp, err := expandComputeFirewallNetwork(d.Get("network"), d, config)
@@ -802,14 +833,16 @@ func flattenComputeFirewallLogConfig(v interface{}, d *schema.ResourceData, conf
 	if len(original) == 0 {
 		return nil
 	}
+
+	v, ok := original["enable"]
+	if ok && !v.(bool) {
+		return nil
+	}
+
 	transformed := make(map[string]interface{})
-	transformed["enable_logging"] =
-		flattenComputeFirewallLogConfigEnableLogging(original["enable"], d, config)
+	transformed["metadata"] = original["metadata"]
 	return []interface{}{transformed}
 }
-func flattenComputeFirewallLogConfigEnableLogging(v interface{}, d *schema.ResourceData, config *Config) interface{} {
-	return v
-}
 
 func flattenComputeFirewallName(v interface{}, d *schema.ResourceData, config *Config) interface{} {
 	return v
@@ -968,19 +1001,23 @@ func expandComputeFirewallDisabled(v interface{}, d TerraformResourceData, confi
 }
 
 func expandComputeFirewallLogConfig(v interface{}, d TerraformResourceData, config *Config) (interface{}, error) {
+	l := v.([]interface{})
 	transformed := make(map[string]interface{})
-	transformedEnableLogging, err := expandComputeFirewallLogConfigEnableLogging(d.Get("enable_logging"), d, config)
-	if err != nil {
-		return nil, err
-	} else {
-		transformed["enable"] = transformedEnableLogging
+
+	if len(l) == 0 || l[0] == nil {
+		// send enable = enable_logging value to ensure correct logging status if there is no config
+		transformed["enable"] = d.Get("enable_logging").(bool)
+		return transformed, nil
 	}
 
-	return transformed, nil
-}
+	raw := l[0]
+	original := raw.(map[string]interface{})
 
-func expandComputeFirewallLogConfigEnableLogging(v interface{}, d TerraformResourceData, config *Config) (interface{}, error) {
-	return v, nil
+	// The log_config block is specified, so logging should be enabled
+	transformed["enable"] = true
+	transformed["metadata"] = original["metadata"]
+
+	return transformed, nil
 }
 
 func expandComputeFirewallName(v interface{}, d TerraformResourceData, config *Config) (interface{}, error) {

google-beta/resource_compute_firewall_test.go

@@ -207,23 +207,31 @@ func TestAccComputeFirewall_enableLogging(t *testing.T) {
 		CheckDestroy: testAccCheckComputeFirewallDestroyProducer(t),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccComputeFirewall_enableLogging(networkName, firewallName, false),
+				Config: testAccComputeFirewall_enableLogging(networkName, firewallName, ""),
 			},
 			{
 				ResourceName:      "google_compute_firewall.foobar",
 				ImportState:       true,
 				ImportStateVerify: true,
 			},
 			{
-				Config: testAccComputeFirewall_enableLogging(networkName, firewallName, true),
+				Config: testAccComputeFirewall_enableLogging(networkName, firewallName, "INCLUDE_ALL_METADATA"),
 			},
 			{
 				ResourceName:      "google_compute_firewall.foobar",
 				ImportState:       true,
 				ImportStateVerify: true,
 			},
 			{
-				Config: testAccComputeFirewall_enableLogging(networkName, firewallName, false),
+				Config: testAccComputeFirewall_enableLogging(networkName, firewallName, "EXCLUDE_ALL_METADATA"),
+			},
+			{
+				ResourceName:      "google_compute_firewall.foobar",
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+			{
+				Config: testAccComputeFirewall_enableLogging(networkName, firewallName, ""),
 			},
 			{
 				ResourceName:      "google_compute_firewall.foobar",
@@ -411,10 +419,13 @@ resource "google_compute_firewall" "foobar" {
 `, network, firewall)
 }
 
-func testAccComputeFirewall_enableLogging(network, firewall string, enableLogging bool) string {
+func testAccComputeFirewall_enableLogging(network, firewall, logging string) string {
 	enableLoggingCfg := ""
-	if enableLogging {
-		enableLoggingCfg = "enable_logging= true"
+	if logging != "" {
+		enableLoggingCfg = fmt.Sprintf(`log_config {
+		  metadata = "%s"
+		}
+		`, logging)
 	}
 	return fmt.Sprintf(`
 resource "google_compute_network" "foobar" {

website/docs/r/compute_firewall.html.markdown

@@ -136,11 +136,10 @@ The following arguments are supported:
   not enforced and the network behaves as if it did not exist. If this
   is unspecified, the firewall rule will be enabled.
 
-* `enable_logging` -
+* `log_config` -
   (Optional)
-  This field denotes whether to enable logging for a particular
-  firewall rule. If logging is enabled, logs will be exported to
-  Stackdriver.
+  This field denotes the logging options for a particular firewall rule.
+  If defined, logging is enabled, and logs will be exported to Cloud Logging.  Structure is documented below.
 
 * `priority` -
   (Optional)
@@ -208,6 +207,9 @@ The following arguments are supported:
     If it is not provided, the provider project is used.
 
 
+* `enable_logging` - (Optional, Deprecated) This field denotes whether to enable logging for a particular firewall rule.
+If logging is enabled, logs will be exported to Stackdriver. Deprecated in favor of `log_config`
+
 The `allow` block supports:
 
 * `protocol` -
@@ -244,6 +246,16 @@ The `deny` block supports:
   Example inputs include: ["22"], ["80","443"], and
   ["12345-12349"].
 
+The `log_config` block supports:
+
+* `metadata` -
+  (Required)
+  This field denotes whether to include or exclude metadata for firewall logs.
+
+  Possible values are:
+  * `EXCLUDE_ALL_METADATA`
+  * `INCLUDE_ALL_METADATA`
+
 ## Attributes Reference
 
 In addition to the arguments listed above, the following computed attributes are exported: