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.

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

equinix_metal_connection field vlans should be forcenew #222

Closed
displague opened this issue Jul 8, 2022 · 5 comments
Closed

equinix_metal_connection field vlans should be forcenew #222

displague opened this issue Jul 8, 2022 · 5 comments
Milestone

Comments

@displague
Copy link
Member

The vlans field of equinix_metal_connection can not be updated. If a user changes this field the resource must be recreated.
What happens today is the change is silently updated in state without making remote changes.

https://github.com/equinix/terraform-provider-equinix/blob/master/equinix/resource_metal_connection.go#L141

@ctreatma
Copy link
Contributor

@ocobleseqx and @displague we discussed this issue earlier today, and I believe we concluded that it is possible to update VLANs for a shared connection, so we don't have to force recreate a connection when the vlans attribute changes. My understanding is that, when a customer creates a shared metal connection without specifying vlans, some other resources are created implicitly, and in order to update VLANs, a customer would have to import those "hidden" resources into terraform state & update them before updating the connection vlans. Does that sound like an accurate summary of the conversation? Are there any additional details or edge cases that are important to call out? One detail I'm not clear on is what kind of resources are created implicitly; is it a VLAN and a virtual circuit? One or the other?

One alternative I suggested, which could allow us to avoid recreating a shared metal connection when vlans is changed and also avoid customers importing a separate resource if/when they want to change vlans, is to make that a required attribute rather than implicitly creating other resources when a user creates a shared metal connection and does not specify vlans, but I think there were questions as to whether we can even specify vlans at creation time for a shared connection?

@ocobles
Copy link
Contributor

ocobles commented Nov 22, 2022

VLAN must remain optional since they can just be defined for dedicated connection(DC) and this resource supports the creation of both shared and dedicated connections. Each shared connection (SC) comes with a single Virtual Circuit (VC) (one virtual circuit for each port if you are creating a redundant connection) and you cannot have more than 1 VC and you cannot delete/add a different VC to the connection . For DC you are able to add multiple VCs, using equinix_metal_virtual_circuit, and there's no pre-created VC. Therefore, there's no terraform resource to update the VC of a SC unless you import the pre-created VC into an equinix_metal_virtual_circuit. Moreover, The creation of a SC is completed and ready to use only once the connection is also configured in the Fabric side, in terraform you will need to create an equinix_ecx_l2_connection using a one-time generated token. So, forcing a new equinix_metal_connection will imply that users also re-create the equinix_ecx_l2_connection.

This conversation took place because in the API there are different endpoints to create the SC and to update a VC, and we try to avoid hidden magic in the behavior of the terraform provider, and each resource must be a representation of an object/API endpoint. In this case, I think that recreating the connection instead of just updating the VLAN in the pre-created VC can cause several difficulties for the end user, for example, there are customers where the Metal resources and Fabric resources are managed by different departments or even different companies. However, that can backfire for users coming from the API and will also work against our journey of achieving an auto-generated terraform provider, where all resources will be just a representation of the API specs.

To sum up, in my opinion, I'd go ahead with this change and add the force_new, since there's a bug that must be fixed (...the change is silently updated in state without making remote changes). However, it will be nice to improve the documentation and explain users the alternatives they have if they want to update the VLANs and they cannot simply recreate the connection resources.

In addition to that, we don't have enough information today about the limitations and inconveniences that can cause a user forcing the recreation of the connection resources (on both Metal and Fabric sides). That is something we can change and work on in the future once we receive feedback or any special requests.

@ctreatma
Copy link
Contributor

The more I think about this, the more I'm concerned about adopting ForceNew for all changes to the vlans attribute. It sounds like adopting that would introduce new problems for dedicated connections, even if it eliminates the state mismatch. Having poked at this a little bit more, I see 2 problems to fix here:

  1. The vlans attribute is not refreshed when the state is refreshed; this means that when I apply a change to vlans, the state updates without the infrastructure updating, and then the next plan shows no changes, and it also means that if I change the vlans manually in the web console, terraform won't detect that.
  2. ForceNew on all changes to the vlans attribute sounds too broad, based on your description above? It sounds like it would be preferable to keep the current behavior for dedicated connections, but force recreation if vlans changes for a shared connection (we can do that by customizing the diff).

@ocobles
Copy link
Contributor

ocobles commented Nov 22, 2022

VLANS field is only used with shared connection, not dedicated

@ctreatma
Copy link
Contributor

Superseded by #272

displague added a commit that referenced this issue Mar 4, 2024
)

The Metal API was recently updated to change the behavior of shared
interconnections as follows:

- VLANs must be specified at creation for shared connections
- VLANs cannot be updated for shared connections

This updates the connection resource to align with the new API behavior
so that users can catch configuration errors earlier.

Note this rolls back #273, which was written to address #222.

This also includes a fix for #595
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants