Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid inconsistent state errors for vlans #642

Merged
merged 6 commits into from
Apr 10, 2024
Merged

fix: avoid inconsistent state errors for vlans #642

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7718847
fix: avoid inconsistent state errors for vlans
ctreatma Apr 4, 2024
9004801
don't lowercase metro name in VLAN update function
ctreatma Apr 5, 2024
db9e3c0
add test to validate removing the VLAN description
ctreatma Apr 5, 2024
ec96d66
update test to check that VLAN isn't recreated when metro case changes
ctreatma Apr 5, 2024
bc903ac
use hashicorp-provided testing functions instead of checking VLAN UUIDs
ctreatma Apr 5, 2024
7ffb3a3
rename test variables & annotate with comments
ctreatma Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions internal/resources/metal/vlan/models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
}
Expand All @@ -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))
Expand All @@ -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)
Copy link
Member

@displague displague Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if vlan.Project.ID != "" {
m.ProjectID = types.StringValue(vlan.Project.ID)
}
Expand All @@ -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
}
5 changes: 2 additions & 3 deletions internal/resources/metal/vlan/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
97 changes: 97 additions & 0 deletions internal/resources/metal/vlan/resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -46,6 +59,7 @@ func TestAccMetalVlan_metro(t *testing.T) {
var vlan packngo.VirtualNetwork
rs := acctest.RandString(10)
metro := "sv"
upperMetro := "DA"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acceptance.TestAccPreCheckMetal(t) },
Expand All @@ -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,
Copy link
Contributor Author

@ctreatma ctreatma Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you don't just use upperMetro here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
},
Check: resource.ComposeTestCheckFunc(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

testAccCheckMetalVlanExists("equinix_metal_vlan.foovlan", &vlan),
resource.TestCheckResourceAttr(
"equinix_metal_vlan.foovlan", "description", "tfacc-vlan"),
resource.TestCheckResourceAttr(
"equinix_metal_vlan.foovlan", "metro", upperMetro),
),
},
{
Config: testAccCheckMetalVlanConfig_metro(rs, strings.ToLower(upperMetro), "tfacc-vlan"),
PlanOnly: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here you could use metro, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
},
})
}

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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"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"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acceptance.TestAccPreCheckMetal(t) },
ExternalProviders: acceptance.TestExternalProviders,
ProtoV5ProviderFactories: acceptance.ProtoV5ProviderFactories,
CheckDestroy: testAccMetalVlanCheckDestroyed,
Steps: []resource.TestStep{
{
Config: testAccCheckMetalVlanConfig_metro(rs, metro, "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", metro),
),
},
{
Config: testAccCheckMetalVlanConfig_NoDescription(rs, metro),
Check: resource.ComposeTestCheckFunc(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

testAccCheckMetalVlanExists("equinix_metal_vlan.foovlan", &vlan),
resource.TestCheckNoResourceAttr(
"equinix_metal_vlan.foovlan", "description"),
resource.TestCheckResourceAttr(
"equinix_metal_vlan.foovlan", "metro", metro),
),
},
},
})
}
Expand Down
Loading