Re-deprecate enable_flow_logs (#13485) (#22111)

[upstream:644ea10a2b33f2863a1edcdc2fb3986bf8ca0518]

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

Author: modular-magician

.changelog/13485.txt

@@ -0,0 +1,6 @@
+```release-note:deprecation
+compute: deprecated `enable_flow_logs` in favor of `log_config`.  If `log_config` is present, flow logs are enabled and `enable_flow_logs` can be safely removed.
+```
+```release-note:bug
+compute: fixed a regression in `google_compute_subnetwork` where setting `log_config` would not enable flow logs without `enable_flow_logs` also being set to true. To enable or disable flow logs, please use `log_config`. `enable_flow_logs` is now deprecated and will be removed in the next major release.
+```

google/services/compute/resource_compute_subnetwork.go

@@ -88,11 +88,13 @@ func sendSecondaryIpRangeIfEmptyDiff(_ context.Context, diff *schema.ResourceDif
 	return nil
 }
 
-// DiffSuppressFunc for fields inside `log_config`.
+// DiffSuppressFunc for `log_config`.
 func subnetworkLogConfigDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
-	// If the enable_flow_logs is not enabled, we don't need to check for differences.
-	if enable_flow_logs := d.Get("enable_flow_logs"); !enable_flow_logs.(bool) {
-		return true
+	// If enable_flow_logs is enabled and log_config is not set, ignore the diff
+	if enable_flow_logs := d.Get("enable_flow_logs"); enable_flow_logs.(bool) {
+		logConfig := d.GetRawConfig().GetAttr("log_config")
+		logConfigIsEmpty := logConfig.IsNull() || logConfig.LengthInt() == 0
+		return logConfigIsEmpty
 	}
 
 	return false
@@ -153,9 +155,11 @@ you create the resource. This field can be set only at resource
 creation time.`,
 			},
 			"enable_flow_logs": {
-				Type:     schema.TypeBool,
-				Computed: true,
-				Optional: true,
+				Type:       schema.TypeBool,
+				Computed:   true,
+				Optional:   true,
+				Deprecated: "This field is being removed in favor of log_config. If log_config is present, flow logs are enabled.",
+				ForceNew:   true,
 				Description: `Whether to enable flow logging for this subnetwork. If this field is not explicitly set,
 it will not appear in get listings. If not set the default behavior is determined by the
 org policy, if there is no org policy specified, then it will default to disabled.
@@ -203,7 +207,6 @@ cannot enable direct path. Possible values: ["EXTERNAL", "INTERNAL"]`,
 			},
 			"log_config": {
 				Type:             schema.TypeList,
-				Computed:         true,
 				Optional:         true,
 				DiffSuppressFunc: subnetworkLogConfigDiffSuppress,
 				Description: `This field denotes the VPC flow logging options for this subnetwork. If
@@ -594,7 +597,7 @@ func resourceComputeSubnetworkCreate(d *schema.ResourceData, meta interface{}) e
 	enableFlowLogsProp, err := expandComputeSubnetworkEnableFlowLogs(d.Get("enable_flow_logs"), d, config)
 	if err != nil {
 		return err
-	} else if v, ok := d.GetOkExists("enable_flow_logs"); ok || !reflect.DeepEqual(v, enableFlowLogsProp) {
+	} else if v, ok := d.GetOkExists("enable_flow_logs"); !tpgresource.IsEmptyValue(reflect.ValueOf(enableFlowLogsProp)) && (ok || !reflect.DeepEqual(v, enableFlowLogsProp)) {
 		obj["enableFlowLogs"] = enableFlowLogsProp
 	}
 
@@ -880,7 +883,7 @@ func resourceComputeSubnetworkUpdate(d *schema.ResourceData, meta interface{}) e
 			return err
 		}
 	}
-	if d.HasChange("private_ipv6_google_access") || d.HasChange("stack_type") || d.HasChange("ipv6_access_type") || d.HasChange("enable_flow_logs") {
+	if d.HasChange("private_ipv6_google_access") || d.HasChange("stack_type") || d.HasChange("ipv6_access_type") {
 		obj := make(map[string]interface{})
 
 		getUrl, err := tpgresource.ReplaceVars(d, config, "{{ComputeBasePath}}projects/{{project}}/regions/{{region}}/subnetworks/{{name}}")
@@ -924,12 +927,6 @@ func resourceComputeSubnetworkUpdate(d *schema.ResourceData, meta interface{}) e
 		} else if v, ok := d.GetOkExists("ipv6_access_type"); !tpgresource.IsEmptyValue(reflect.ValueOf(v)) && (ok || !reflect.DeepEqual(v, ipv6AccessTypeProp)) {
 			obj["ipv6AccessType"] = ipv6AccessTypeProp
 		}
-		enableFlowLogsProp, err := expandComputeSubnetworkEnableFlowLogs(d.Get("enable_flow_logs"), d, config)
-		if err != nil {
-			return err
-		} else if v, ok := d.GetOkExists("enable_flow_logs"); ok || !reflect.DeepEqual(v, enableFlowLogsProp) {
-			obj["enableFlowLogs"] = enableFlowLogsProp
-		}
 
 		url, err := tpgresource.ReplaceVars(d, config, "{{ComputeBasePath}}projects/{{project}}/regions/{{region}}/subnetworks/{{name}}")
 		if err != nil {
@@ -1676,9 +1673,8 @@ func expandComputeSubnetworkLogConfig(v interface{}, d tpgresource.TerraformReso
 			return nil, nil
 		}
 
-		// set enable field basing on the enable_flow_logs field. It's needed for case when enable_flow_logs
-		// is set to true to avoid conflict with the API. In that case API will return default values for log_config
-		transformed["enable"] = d.Get("enable_flow_logs")
+		// send enable = false to ensure logging is disabled if there is no config
+		transformed["enable"] = false
 		return transformed, nil
 	}
 

google/services/compute/resource_compute_subnetwork_test.go

@@ -493,66 +493,6 @@ func TestAccComputeSubnetwork_internal_ipv6(t *testing.T) {
 	})
 }
 
-func TestAccComputeSubnetwork_enableFlowLogs(t *testing.T) {
-	t.Parallel()
-	var subnetwork compute.Subnetwork
-
-	cnName := fmt.Sprintf("tf-test-%s", acctest.RandString(t, 10))
-	subnetworkName := fmt.Sprintf("tf-test-%s", acctest.RandString(t, 10))
-
-	acctest.VcrTest(t, resource.TestCase{
-		PreCheck:                 func() { acctest.AccTestPreCheck(t) },
-		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
-		CheckDestroy:             testAccCheckComputeSubnetworkDestroyProducer(t),
-		Steps: []resource.TestStep{
-			{
-				Config: testAccComputeSubnetwork_enableFlowLogs(cnName, subnetworkName, true),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckComputeSubnetworkExists(t, "google_compute_subnetwork.subnetwork", &subnetwork),
-					testAccCheckComputeSubnetworkIfLogConfigExists(&subnetwork, "google_compute_subnetwork.subnetwork"),
-					testAccCheckComputeSubnetworkLogConfig(&subnetwork, "log_config.0.enable", "true"),
-					resource.TestCheckResourceAttr("google_compute_subnetwork.subnetwork", "enable_flow_logs", "true"),
-				),
-			},
-			{
-				ResourceName:      "google_compute_subnetwork.subnetwork",
-				ImportState:       true,
-				ImportStateVerify: true,
-			},
-			{
-				Config: testAccComputeSubnetwork_enableFlowLogs_with_logConfig(cnName, subnetworkName, true),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckComputeSubnetworkExists(t, "google_compute_subnetwork.subnetwork", &subnetwork),
-					resource.TestCheckResourceAttr("google_compute_subnetwork.subnetwork", "enable_flow_logs", "true"),
-					testAccCheckComputeSubnetworkLogConfig(&subnetwork, "log_config.0.enable", "true"),
-					testAccCheckComputeSubnetworkLogConfig(&subnetwork, "log_config.0.aggregation_interval", "INTERVAL_1_MIN"),
-					testAccCheckComputeSubnetworkLogConfig(&subnetwork, "log_config.0.metadata", "INCLUDE_ALL_METADATA"),
-				),
-			},
-			{
-				ResourceName:      "google_compute_subnetwork.subnetwork",
-				ImportState:       true,
-				ImportStateVerify: true,
-			},
-			{
-				Config: testAccComputeSubnetwork_enableFlowLogs(cnName, subnetworkName, false),
-				Check: resource.ComposeTestCheckFunc(
-					testAccCheckComputeSubnetworkExists(t, "google_compute_subnetwork.subnetwork", &subnetwork),
-					resource.TestCheckResourceAttr("google_compute_subnetwork.subnetwork", "enable_flow_logs", "false"),
-					testAccCheckComputeSubnetworkLogConfig(&subnetwork, "log_config.0.enable", "false"),
-					testAccCheckComputeSubnetworkLogConfig(&subnetwork, "log_config.0.aggregation_interval", "INTERVAL_1_MIN"),
-					testAccCheckComputeSubnetworkLogConfig(&subnetwork, "log_config.0.metadata", "INCLUDE_ALL_METADATA"),
-				),
-			},
-			{
-				ResourceName:      "google_compute_subnetwork.subnetwork",
-				ImportState:       true,
-				ImportStateVerify: true,
-			},
-		},
-	})
-}
-
 func testAccCheckComputeSubnetworkExists(t *testing.T, n string, subnetwork *compute.Subnetwork) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		rs, ok := s.RootModule().Resources[n]
@@ -613,111 +553,6 @@ func testAccCheckComputeSubnetworkHasNotSecondaryIpRange(subnetwork *compute.Sub
 	}
 }
 
-func testAccCheckComputeSubnetworkIfLogConfigExists(subnetwork *compute.Subnetwork, resourceName string) resource.TestCheckFunc {
-	return func(s *terraform.State) error {
-		// Retrieve the resource state using a fixed resource name
-		rs, ok := s.RootModule().Resources[resourceName]
-		if !ok {
-			return fmt.Errorf("resource google_compute_subnetwork.subnetwork not found in state")
-		}
-
-		if rs.Primary.ID == "" {
-			return fmt.Errorf("resource ID is not set")
-		}
-
-		// Ensure that the log_config exists in the API response.
-		if subnetwork.LogConfig == nil {
-			return fmt.Errorf("no log_config exists in subnetwork")
-		}
-
-		stateAttrs := rs.Primary.Attributes
-
-		// Check aggregation_interval.
-		aggInterval, ok := stateAttrs["log_config.0.aggregation_interval"]
-		if !ok {
-			return fmt.Errorf("aggregation_interval not found in state")
-		}
-		if subnetwork.LogConfig.AggregationInterval != aggInterval {
-			return fmt.Errorf("aggregation_interval mismatch: expected %s, got %s", aggInterval, subnetwork.LogConfig.AggregationInterval)
-		}
-
-		// Check flow_sampling.
-		fsState, ok := stateAttrs["log_config.0.flow_sampling"]
-		if !ok {
-			return fmt.Errorf("flow_sampling not found in state")
-		}
-		actualFS := fmt.Sprintf("%g", subnetwork.LogConfig.FlowSampling)
-		if actualFS != fsState {
-			return fmt.Errorf("flow_sampling mismatch: expected %s, got %s", fsState, actualFS)
-		}
-
-		// Check metadata.
-		metadata, ok := stateAttrs["log_config.0.metadata"]
-		if !ok {
-			return fmt.Errorf("metadata not found in state")
-		}
-		if subnetwork.LogConfig.Metadata != metadata {
-			return fmt.Errorf("metadata mismatch: expected %s, got %s", metadata, subnetwork.LogConfig.Metadata)
-		}
-
-		// Optionally, check filter_expr if it exists.
-		if subnetwork.LogConfig.FilterExpr != "" {
-			filterExpr, ok := stateAttrs["log_config.0.filter_expr"]
-			if !ok {
-				return fmt.Errorf("filter_expr is set in API but not found in state")
-			}
-			if subnetwork.LogConfig.FilterExpr != filterExpr {
-				return fmt.Errorf("filter_expr mismatch: expected %s, got %s", filterExpr, subnetwork.LogConfig.FilterExpr)
-			}
-		}
-
-		// Optionally, check metadata_fields if present.
-		if len(subnetwork.LogConfig.MetadataFields) > 0 {
-			if _, ok := stateAttrs["log_config.0.metadata_fields"]; !ok {
-				return fmt.Errorf("metadata_fields are set in API but not found in state")
-			}
-		}
-
-		return nil
-	}
-}
-
-func testAccCheckComputeSubnetworkLogConfig(subnetwork *compute.Subnetwork, key, value string) resource.TestCheckFunc {
-	return func(s *terraform.State) error {
-		if subnetwork.LogConfig == nil {
-			return fmt.Errorf("no log_config found")
-		}
-
-		switch key {
-		case "log_config.0.enable":
-			if subnetwork.LogConfig.Enable != (value == "true") {
-				return fmt.Errorf("expected %s to be '%s', got '%t'", key, value, subnetwork.LogConfig.Enable)
-			}
-		case "log_config.0.aggregation_interval":
-			if subnetwork.LogConfig.AggregationInterval != value {
-				return fmt.Errorf("expected %s to be '%s', got '%s'", key, value, subnetwork.LogConfig.AggregationInterval)
-			}
-		case "log_config.0.metadata":
-			if subnetwork.LogConfig.Metadata != value {
-				return fmt.Errorf("expected %s to be '%s', got '%s'", key, value, subnetwork.LogConfig.Metadata)
-			}
-		case "log_config.0.flow_sampling":
-			flowSamplingStr := fmt.Sprintf("%g", subnetwork.LogConfig.FlowSampling)
-			if flowSamplingStr != value {
-				return fmt.Errorf("expected %s to be '%s', got '%s'", key, value, flowSamplingStr)
-			}
-		case "log_config.0.filterExpr":
-			if subnetwork.LogConfig.FilterExpr != value {
-				return fmt.Errorf("expected %s to be '%s', got '%s'", key, value, subnetwork.LogConfig.FilterExpr)
-			}
-		default:
-			return fmt.Errorf("unknown log_config key: %s", key)
-		}
-
-		return nil
-	}
-}
-
 func testAccComputeSubnetwork_basic(cnName, subnetwork1Name, subnetwork2Name, subnetwork3Name string) string {
 	return fmt.Sprintf(`
 resource "google_compute_network" "custom-test" {
@@ -1195,42 +1030,3 @@ resource "google_compute_subnetwork" "subnetwork" {
 }
 `, cnName, subnetworkName)
 }
-
-func testAccComputeSubnetwork_enableFlowLogs(cnName, subnetworkName string, enableFlowLogs bool) string {
-	return fmt.Sprintf(`
-resource "google_compute_network" "custom-test" {
-  name                    = "%s"
-  auto_create_subnetworks = false
-}
-
-resource "google_compute_subnetwork" "subnetwork" {
-  name          = "%s"
-  ip_cidr_range = "10.0.0.0/16"
-  region        = "us-central1"
-  network       = google_compute_network.custom-test.self_link
-  enable_flow_logs = %t
-}
-`, cnName, subnetworkName, enableFlowLogs)
-}
-
-func testAccComputeSubnetwork_enableFlowLogs_with_logConfig(cnName, subnetworkName string, enableFlowLogs bool) string {
-	return fmt.Sprintf(`
-resource "google_compute_network" "custom-test" {
-  name                    = "%s"
-  auto_create_subnetworks = false
-}
-
-resource "google_compute_subnetwork" "subnetwork" {
-  name          = "%s"
-  ip_cidr_range = "10.0.0.0/16"
-  region        = "us-central1"
-  network       = google_compute_network.custom-test.self_link
-  enable_flow_logs = %t
-
-  log_config {
-    aggregation_interval = "INTERVAL_1_MIN"
-    metadata = "INCLUDE_ALL_METADATA"
-  }
-}
-`, cnName, subnetworkName, enableFlowLogs)
-}

website/docs/r/compute_subnetwork.html.markdown

@@ -457,12 +457,14 @@ The following arguments are supported:
   via BGP even if their destinations match existing subnet ranges.
 
 * `enable_flow_logs` -
-  (Optional)
+  (Optional, Deprecated)
   Whether to enable flow logging for this subnetwork. If this field is not explicitly set,
   it will not appear in get listings. If not set the default behavior is determined by the
   org policy, if there is no org policy specified, then it will default to disabled.
   This field isn't supported if the subnet purpose field is set to REGIONAL_MANAGED_PROXY.
 
+  ~> **Warning:** This field is being removed in favor of log_config. If log_config is present, flow logs are enabled.
+
 * `project` - (Optional) The ID of the project in which the resource belongs.
     If it is not provided, the provider project is used.