fix: avoid inconsistent state errors for vlans #642

ctreatma · 2024-04-04T21:26:29Z

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

codecov-commenter · 2024-04-04T21:29:20Z

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 59.76%. Comparing base (503d9e1) to head (bc903ac).

Files	Patch %	Lines
internal/resources/metal/vlan/models.go	75.00%	5 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #642       +/-   ##
===========================================
+ Coverage   37.91%   59.76%   +21.84%     
===========================================
  Files         120      120               
  Lines       19476    19491       +15     
===========================================
+ Hits         7384    11648     +4264     
+ Misses      11884     7285     -4599     
- Partials      208      558      +350

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

displague · 2024-04-05T15:52:21Z

internal/resources/metal/vlan/models.go

 	m.Vxlan = types.Int64Value(int64(vlan.VXLAN))
 	m.Facility = types.StringValue("")

+	if vlan.Description != "" {
+		m.Description = types.StringValue(vlan.Description)


Does this effectively prevent TF from recognizing an API cleared description?

Terraform initially set and continues to want: "foo"
API was updated to: ""
Terraform ignores the diff?

This same code is fine within the datasource parse.

VLAN doesn't have an update endpoint according to the spec & both SDKs (all attributes of VLAN resources are marked as requiring replacement, and have been for quite a while). I added a test, though, to confirm that the description is removed; if we ever add support for updating VLAN attributes, that will help confirm that description continues to work as expected.

displague · 2024-04-05T15:58:17Z

internal/resources/metal/vlan/resource_test.go

+			},
+			{
+				Config: testAccCheckMetalVlanConfig_metro(rs, upperMetro, "tfacc-vlan"),
+				Check: resource.ComposeTestCheckFunc(


Is there a check we can add to ensure that the resource was effectively recreated when the metro changed?

The model is written to throw an error if the metro received from the API is different from the one in state (other than casing): https://github.com/equinix/terraform-provider-equinix/pull/642/files#diff-8f6d46b17b852aeb7d85b49d7e72ea5359e1345953070c31a27b9264aa683f42R93-R96

If we weren't recreating the VLAN, we should hit that error and fail the test.

Thinking about this more: we should probably update the test to validate that the VLAN isn't recreated when we change the metro casing

Test has been updated to check that the VLAN ID stays the same when the metro casing changes; I validated by temporarily removing the CaseInsensitiveString plan modifier from the resource (so that casing changes are seen as requiring an apply) and confirming that the test fails.

ctreatma · 2024-04-08T15:40:59Z

internal/resources/metal/vlan/resource_test.go

+				Config: testAccCheckMetalVlanConfig_NoDescription(rs, metro),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckMetalVlanExists("equinix_metal_vlan.foovlan", &vlan),
+					resource.TestCheckNoResourceAttr(


This test checks that we don't get "inconsistent state" errors when description is omitted in the initial config

ctreatma · 2024-04-08T15:42:51Z

internal/resources/metal/vlan/resource_test.go

+			},
+			{
+				Config: testAccCheckMetalVlanConfig_NoDescription(rs, metro),
+				Check: resource.ComposeTestCheckFunc(


This test checks that we don't get "inconsistent state" errors when description is included in the initial config and then deleted later.

ctreatma · 2024-04-08T15:43:28Z

internal/resources/metal/vlan/resource_test.go

@@ -63,6 +77,89 @@ func TestAccMetalVlan_metro(t *testing.T) {
 						"equinix_metal_vlan.foovlan", "metro", metro),
 				),
 			},
+			{
+				Config:   testAccCheckMetalVlanConfig_metro(rs, strings.ToUpper(metro), "tfacc-vlan"),
+				PlanOnly: true,


This test step confirms that there are no changes to apply after changing the metro from sv to SV

Any reason you don't just use upperMetro here?

metro and upperMetro are being used in different ways, one is to use a different case as a starting point before converting case and the other is to have different metros to ensure that changing the metro field has the intended effect.

ctreatma · 2024-04-08T15:44:32Z

internal/resources/metal/vlan/resource_test.go

+				Config: testAccCheckMetalVlanConfig_metro(rs, upperMetro, "tfacc-vlan"),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("equinix_metal_vlan.foovlan", plancheck.ResourceActionDestroyBeforeCreate),


This plan check confirms that this test step will destroy and recreate the VLAN because there was a material change to the metro from Silicon Valley to Dallas

ctreatma · 2024-04-08T15:45:32Z

internal/resources/metal/vlan/resource_test.go

+			},
+			{
+				Config:   testAccCheckMetalVlanConfig_metro(rs, strings.ToLower(upperMetro), "tfacc-vlan"),
+				PlanOnly: true,


This test step confirms that there are no changes to apply after changing the metro from DA to da

And here you could use metro, right?

I can see how this test is hard to reason about; in the latest commit I tried changing the names of the upperMetro and metro variables in an attempt to clarify intent/usage. Let me know if that helped or no.

cprivitere

lgtm, but I'm not 100% up to speed on how these tests work. So maybe wait for @displague or @ocobles

cprivitere · 2024-04-08T16:09:27Z

=== NAME  TestAccMetalConnection_sharedRedundantVrf
    testing_new.go:91: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: Failed to delete Metal Connection d18ec3fc-e049-42a6-8f2a-7a248d0af2d1
        
        Timeout waiting for connection to be deleted
--- FAIL: TestAccMetalConnection_sharedRedundantVrf (79.21s)
FAIL
coverage: 67.9% of statements
FAIL	github.com/equinix/terraform-provider-equinix/internal/resources/metal/connection	79.228s

Failed test.

displague · 2024-04-08T16:41:54Z

This one is relevant but seems to have failed on availability

--- FAIL: TestAccMetalPort_hybridBondedVxlan (2.70s)

ctreatma · 2024-04-08T21:49:51Z

@cprivitere @displague I pushed up a minor change to the tests to try to make them more legible. The e2e tests are flaky; I re-ran them 3 more times in CI, but there were some failures each time. You can look at the output of each run and see the failures to decide if they're related to this change or not: https://github.com/equinix/terraform-provider-equinix/actions/runs/8604589925/job/23586041155?pr=642

displague · 2024-04-10T14:09:30Z

7/7 PASS: TestAccMetalVlan
0/7 FAIL: TestAccMetalVlan

ctreatma requested a review from a team as a code owner April 4, 2024 21:26

ctreatma temporarily deployed to internal April 4, 2024 21:26 — with GitHub Actions Inactive

displague reviewed Apr 5, 2024

View reviewed changes

ctreatma temporarily deployed to internal April 5, 2024 16:55 — with GitHub Actions Inactive

ctreatma added 3 commits April 5, 2024 14:25

fix: avoid inconsistent state errors for vlans

7718847

don't lowercase metro name in VLAN update function

9004801

add test to validate removing the VLAN description

db9e3c0

ctreatma force-pushed the fix-inconsistent-vlan branch from d2f31db to db9e3c0 Compare April 5, 2024 19:25

ctreatma temporarily deployed to internal April 5, 2024 19:25 — with GitHub Actions Inactive

ctreatma requested a deployment to internal April 5, 2024 19:30 — with GitHub Actions Abandoned

update test to check that VLAN isn't recreated when metro case changes

ec96d66

ctreatma temporarily deployed to internal April 5, 2024 19:55 — with GitHub Actions Inactive

use hashicorp-provided testing functions instead of checking VLAN UUIDs

bc903ac

ctreatma temporarily deployed to internal April 5, 2024 21:25 — with GitHub Actions Inactive

ctreatma commented Apr 8, 2024

View reviewed changes

cprivitere previously approved these changes Apr 8, 2024

View reviewed changes

rename test variables & annotate with comments

7ffb3a3

ctreatma temporarily deployed to internal April 8, 2024 17:59 — with GitHub Actions Inactive

ctreatma requested a deployment to internal April 8, 2024 20:04 — with GitHub Actions Abandoned

ctreatma temporarily deployed to internal April 8, 2024 20:45 — with GitHub Actions Inactive

ctreatma requested a deployment to internal April 8, 2024 21:24 — with GitHub Actions Abandoned

ctreatma requested a deployment to internal April 9, 2024 19:37 — with GitHub Actions Abandoned

displague approved these changes Apr 10, 2024

View reviewed changes

ctreatma merged commit 1e82c46 into main Apr 10, 2024
4 of 5 checks passed

ctreatma deleted the fix-inconsistent-vlan branch April 10, 2024 14:22

ctreatma mentioned this pull request Sep 26, 2024

refactor: migrate metal_vlan to equinix-sdk-go #782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid inconsistent state errors for vlans #642

fix: avoid inconsistent state errors for vlans #642

ctreatma commented Apr 4, 2024 •

edited by displague

Loading

codecov-commenter commented Apr 4, 2024 •

edited

Loading

displague Apr 5, 2024 •

edited

Loading

ctreatma Apr 5, 2024

displague Apr 5, 2024

ctreatma Apr 5, 2024

ctreatma Apr 5, 2024

ctreatma Apr 5, 2024

ctreatma Apr 8, 2024

ctreatma Apr 8, 2024

ctreatma Apr 8, 2024 •

edited

Loading

cprivitere Apr 8, 2024

displague Apr 8, 2024

ctreatma Apr 8, 2024

ctreatma Apr 8, 2024

cprivitere Apr 8, 2024

ctreatma Apr 8, 2024

cprivitere left a comment

cprivitere commented Apr 8, 2024

displague commented Apr 8, 2024

ctreatma commented Apr 8, 2024 •

edited

Loading

displague commented Apr 10, 2024

fix: avoid inconsistent state errors for vlans #642

fix: avoid inconsistent state errors for vlans #642

Conversation

ctreatma commented Apr 4, 2024 • edited by displague Loading

codecov-commenter commented Apr 4, 2024 • edited Loading

Codecov Report

displague Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctreatma Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cprivitere left a comment

Choose a reason for hiding this comment

cprivitere commented Apr 8, 2024

displague commented Apr 8, 2024

ctreatma commented Apr 8, 2024 • edited Loading

displague commented Apr 10, 2024

ctreatma commented Apr 4, 2024 •

edited by displague

Loading

codecov-commenter commented Apr 4, 2024 •

edited

Loading

displague Apr 5, 2024 •

edited

Loading

ctreatma Apr 8, 2024 •

edited

Loading

ctreatma commented Apr 8, 2024 •

edited

Loading