vmware / terraform-provider-nsxt

Terraform VMware NSX-T provider
https://www.terraform.io/docs/providers/nsxt/
Other
123 stars 80 forks source link

nsxt_policy_project short_id is not marked ForceNew #1213

Open adarobin opened 2 months ago

adarobin commented 2 months ago

Describe the bug

The short_id field of the nsxt_policy_project is not marked with ForceNew. This causes an error to be returned by Terraform if you attempt to perform an apply with a change to this field.

Reproduction steps

  1. Attempt to change the short_id of an existing nsxt_policy_project

Expected behavior

A plan should show that the nsxt_policy_project needs to be recreated in order to change the short_id. An apply should not throw an error message.

Additional context

╷
│ Error:  Failed to update Project compute.services: short_id once set cannot be modified. (code 524234)
│ 
│   with module.compute_services_project.nsxt_policy_project.main,
│   on ../modules/miserver_nsx_project/main.tf line 1, in resource "nsxt_policy_project" "main":
│    1: resource "nsxt_policy_project" "main" {
│ 
╵
Releasing state lock. This may take a few moments...
ksamoray commented 2 months ago

Hi @adarobin, NSX API defines short_id as a value that can be assigned only during creation and cannot be updated. While ForceNew will enable "updating" this attribute, in reality the project object will be deleted and replaced with a new one. As this could impact the project's sub-objects (or more likely, fail - NSX will not perform a cascaded deletion if I'm not mistaken), I don't think that using ForceNew makes sense in this case. I'd rather leave this as a create-only attribute as it is in the NSX UI. @annakhm what's your opinion about this?

annakhm commented 2 months ago

I would agree ForceNew is an overkill here, recreating the whole project is likely not what the user had in mind when changing short_id. Unless the project is brand new with nothing created yet below it. @ksamoray do you think it is feasible to recreate the project from Update code in case its empty, and otherwise throw an error?

ksamoray commented 2 months ago

@annakhm it's never empty, if the value is not assigned by the user, NSX assigns a value, e.g the project_id if it's short enough and unique. Which is why this attribute is Computed: true

adarobin commented 2 months ago

It doesn't feel very Terraform-like for it to not propose a delete and recreate. There are other id-type fields in this provider that have ForceNew set on them (nsx_id is a prime example). If I attempt to change similar fields in other providers (i.e. project_id in google_project the plan will show that the resource will be destroyed and recreated.

recreating the whole project is likely not what the user had in mind when changing short_id

This is a true statement, but if I saw the plan propose a destroy and recreate I would have understood what was going on and figured out how I wanted to handle it. What I did not expect was a successful plan showing an update followed by an apply that threw an error. At the least, the plan should have thrown the error from my perspective.

annakhm commented 2 months ago

@annakhm it's never empty, if the value is not assigned by the user, NSX assigns a value, e.g the project_id if it's short enough and unique. Which is why this attribute is Computed: true

Sorry for bad phrasing, I meant empty project, not empty short_id

annakhm commented 2 months ago

It doesn't feel very Terraform-like for it to not propose a delete and recreate. There are other id-type fields in this provider that have ForceNew set on them (nsx_id is a prime example). If I attempt to change similar fields in other providers (i.e. project_id in google_project the plan will show that the resource will be destroyed and recreated.

recreating the whole project is likely not what the user had in mind when changing short_id

This is a true statement, but if I saw the plan propose a destroy and recreate I would have understood what was going on and figured out how I wanted to handle it. What I did not expect was a successful plan showing an update followed by an apply that threw an error. At the least, the plan should have thrown the error from my perspective.

@adarobin, thanks for your comment, good point about failing on plan stage. This could be a good compromise, however this doesn't seem feasible with terraform sdk that we use today, since we don't have access to the state from the context of validation function. Therefore, there is no way to determine whether value was updated.