fix: avoid inconsistent state errors for vlans (#642)

When the equinix_metal_vlan resource was migrated (#578) from terraform-plugin-sdk/v2 (SDKv2) to terraform-plugin-framework (framework), some unintended behavior was introduced around the description and metro attributes. For equinix_metal_vlan resources under SDKv2, description was an optional attribute and metro was case insensitive, but after migration from SDKv2 to framework, the description was set to `""` when the attribute was omitted, and metro was required to be lower-case to avoid terraform errors. This updates the VLAN model parsing function so that: - if there is already metro value configured in state and it is equal to the value received from the API or from the config, ignoring case, then the value that is already in state is used instead of the value from the API - if there is no description in the API response, the description property is omitted entirely instead of being set to an empty string `TestAccMetalVlan_NoDescription` has been added to validate the resource behavior when the description is omitted and `TestAccMetalVlan_metro` has been updated to validate behavior when metro is specified in uppercase. Fixes #633
equinix · Apr 10, 2024 · 1e82c46 · 1e82c46
1 parent 503d9e1
commit 1e82c46
Show file tree

Hide file tree

Showing 3 changed files with 132 additions and 11 deletions.
diff --git a/internal/resources/metal/vlan/models.go b/internal/resources/metal/vlan/models.go
@@ -2,10 +2,12 @@ package vlan
 
 import (
 	"context"
+	"fmt"
+	"strings"
+
 	"github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/packethost/packngo"
-	"strings"
 )
 
 type DataSourceModel struct {
@@ -19,13 +21,16 @@ type DataSourceModel struct {
 	AssignedDevicesIds types.List   `tfsdk:"assigned_devices_ids"`
 }
 
-func (m *DataSourceModel) parse(vlan *packngo.VirtualNetwork) diag.Diagnostics {
+func (m *DataSourceModel) parse(vlan *packngo.VirtualNetwork) (d diag.Diagnostics) {
 	m.ID = types.StringValue(vlan.ID)
 	m.VlanID = types.StringValue(vlan.ID)
-	m.Description = types.StringValue(vlan.Description)
 	m.Vxlan = types.Int64Value(int64(vlan.VXLAN))
 	m.Facility = types.StringValue("")
 
+	if vlan.Description != "" {
+		m.Description = types.StringValue(vlan.Description)
+	}
+
 	if vlan.Project.ID != "" {
 		m.ProjectID = types.StringValue(vlan.Project.ID)
 	}
@@ -36,7 +41,14 @@ func (m *DataSourceModel) parse(vlan *packngo.VirtualNetwork) diag.Diagnostics {
 	}
 
 	if vlan.Metro != nil {
-		m.Metro = types.StringValue(strings.ToLower(vlan.Metro.Code))
+		if m.Metro.IsNull() {
+			m.Metro = types.StringValue(vlan.Metro.Code)
+		} else if !strings.EqualFold(m.Metro.ValueString(), vlan.Metro.Code) {
+			d.AddWarning(
+				"unexpected value for metro",
+				fmt.Sprintf("expected vlan %v to have metro %v, but metro was %v",
+					m.ID, m.Metro, vlan.Metro.Code))
+		}
 	}
 
 	deviceIds := make([]types.String, 0, len(vlan.Instances))
@@ -56,12 +68,15 @@ type ResourceModel struct {
 	Description types.String `tfsdk:"description"`
 }
 
-func (m *ResourceModel) parse(vlan *packngo.VirtualNetwork) diag.Diagnostics {
+func (m *ResourceModel) parse(vlan *packngo.VirtualNetwork) (d diag.Diagnostics) {
 	m.ID = types.StringValue(vlan.ID)
-	m.Description = types.StringValue(vlan.Description)
 	m.Vxlan = types.Int64Value(int64(vlan.VXLAN))
 	m.Facility = types.StringValue("")
 
+	if vlan.Description != "" {
+		m.Description = types.StringValue(vlan.Description)
+	}
+
 	if vlan.Project.ID != "" {
 		m.ProjectID = types.StringValue(vlan.Project.ID)
 	}
@@ -72,8 +87,14 @@ func (m *ResourceModel) parse(vlan *packngo.VirtualNetwork) diag.Diagnostics {
 	}
 
 	if vlan.Metro != nil {
-		m.Metro = types.StringValue(strings.ToLower(vlan.Metro.Code))
+		if m.Metro.IsNull() {
+			m.Metro = types.StringValue(vlan.Metro.Code)
+		} else if !strings.EqualFold(m.Metro.ValueString(), vlan.Metro.Code) {
+			d.AddError(
+				"unexpected value for metro",
+				fmt.Sprintf("expected vlan %v to have metro %v, but metro was %v",
+					m.ID, m.Metro, vlan.Metro.Code))
+		}
 	}
-
 	return nil
 }
diff --git a/internal/resources/metal/vlan/resource.go b/internal/resources/metal/vlan/resource.go
@@ -4,10 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
+
 	equinix_errors "github.com/equinix/terraform-provider-equinix/internal/errors"
 	"github.com/equinix/terraform-provider-equinix/internal/framework"
-	"github.com/hashicorp/terraform-plugin-framework/types"
-	"strings"
 
 	"github.com/hashicorp/terraform-plugin-framework/resource"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema"
@@ -146,7 +146,6 @@ func (r *Resource) Update(ctx context.Context, req resource.UpdateRequest, resp
 		return
 	}
 
-	data.Metro = types.StringValue(strings.ToLower(data.Metro.ValueString()))
 	if diag := resp.State.Set(ctx, &data); diag.HasError() {
 		resp.Diagnostics.Append(diag...)
 		return

diff --git a/internal/resources/metal/vlan/resource_test.go b/internal/resources/metal/vlan/resource_test.go
@@ -29,6 +29,19 @@ resource "equinix_metal_vlan" "foovlan" {
 `, projSuffix, metro, desc)
 }
 
+func testAccCheckMetalVlanConfig_NoDescription(projSuffix, metro string) string {
+	return fmt.Sprintf(`
+resource "equinix_metal_project" "foobar" {
+    name = "tfacc-vlan-%s"
+}
+
+resource "equinix_metal_vlan" "foovlan" {
+    project_id = equinix_metal_project.foobar.id
+    metro = "%s"
+}
+`, projSuffix, metro)
+}
+
 func testAccCheckMetalVlanConfig_facility(projSuffix, facility, desc string) string {
 	return fmt.Sprintf(`
 resource "equinix_metal_project" "foobar" {
@@ -43,6 +56,84 @@ resource "equinix_metal_vlan" "foovlan" {
 }
 
 func TestAccMetalVlan_metro(t *testing.T) {
+	var vlan packngo.VirtualNetwork
+	rs := acctest.RandString(10)
+	lowerSiliconValley := "sv"
+	upperDallas := "DA"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acceptance.TestAccPreCheckMetal(t) },
+		ExternalProviders:        acceptance.TestExternalProviders,
+		ProtoV5ProviderFactories: acceptance.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccMetalVlanCheckDestroyed,
+		Steps: []resource.TestStep{
+			{
+				// Create VLAN with metro "sv" (lower-case)
+				Config: testAccCheckMetalVlanConfig_metro(rs, lowerSiliconValley, "tfacc-vlan"),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckMetalVlanExists("equinix_metal_vlan.foovlan", &vlan),
+					resource.TestCheckResourceAttr(
+						"equinix_metal_vlan.foovlan", "description", "tfacc-vlan"),
+					resource.TestCheckResourceAttr(
+						"equinix_metal_vlan.foovlan", "metro", lowerSiliconValley),
+				),
+			},
+			{
+				// Confirm no changes if metro is changed to "SV" (upper-case)
+				Config:   testAccCheckMetalVlanConfig_metro(rs, strings.ToUpper(lowerSiliconValley), "tfacc-vlan"),
+				PlanOnly: true,
+			},
+			{
+				// Recreate VLAN with metro "DA" (upper-case)
+				Config: testAccCheckMetalVlanConfig_metro(rs, upperDallas, "tfacc-vlan"),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("equinix_metal_vlan.foovlan", plancheck.ResourceActionDestroyBeforeCreate),
+					},
+				},
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckMetalVlanExists("equinix_metal_vlan.foovlan", &vlan),
+					resource.TestCheckResourceAttr(
+						"equinix_metal_vlan.foovlan", "description", "tfacc-vlan"),
+					resource.TestCheckResourceAttr(
+						"equinix_metal_vlan.foovlan", "metro", upperDallas),
+				),
+			},
+			{
+				// Confirm no changes if metro is changed to "da" (lower-case)
+				Config:   testAccCheckMetalVlanConfig_metro(rs, strings.ToLower(upperDallas), "tfacc-vlan"),
+				PlanOnly: true,
+			},
+		},
+	})
+}
+
+func TestAccMetalVlan_NoDescription(t *testing.T) {
+	var vlan packngo.VirtualNetwork
+	rs := acctest.RandString(10)
+	metro := "sv"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acceptance.TestAccPreCheckMetal(t) },
+		ExternalProviders:        acceptance.TestExternalProviders,
+		ProtoV5ProviderFactories: acceptance.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccMetalVlanCheckDestroyed,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccCheckMetalVlanConfig_NoDescription(rs, metro),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckMetalVlanExists("equinix_metal_vlan.foovlan", &vlan),
+					resource.TestCheckNoResourceAttr(
+						"equinix_metal_vlan.foovlan", "description"),
+					resource.TestCheckResourceAttr(
+						"equinix_metal_vlan.foovlan", "metro", metro),
+				),
+			},
+		},
+	})
+}
+
+func TestAccMetalVlan_RemoveDescription(t *testing.T) {
 	var vlan packngo.VirtualNetwork
 	rs := acctest.RandString(10)
 	metro := "sv"
@@ -63,6 +154,16 @@ func TestAccMetalVlan_metro(t *testing.T) {
 						"equinix_metal_vlan.foovlan", "metro", metro),
 				),
 			},
+			{
+				Config: testAccCheckMetalVlanConfig_NoDescription(rs, metro),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckMetalVlanExists("equinix_metal_vlan.foovlan", &vlan),
+					resource.TestCheckNoResourceAttr(
+						"equinix_metal_vlan.foovlan", "description"),
+					resource.TestCheckResourceAttr(
+						"equinix_metal_vlan.foovlan", "metro", metro),
+				),
+			},
 		},
 	})
 }