310 - bug - netapp-ontap_nfs_service resource

[csahu9]

Issue #310
fixed issue with idempotency when default value for few params is not set in config

ACC test result:

[root@scs000952326 protocols]# TF_ACC=1 go test `go list ./... | grep -e provider` -v -run=TestAccNfsServiceResource
=== RUN   TestAccNfsServiceResourceAlias
--- PASS: TestAccNfsServiceResourceAlias (12.04s)
=== RUN   TestAccNfsServiceResource
--- PASS: TestAccNfsServiceResource (15.81s)
PASS
ok      github.com/netapp/terraform-provider-netapp-ontap/internal/provider/protocols   27.859s

[chuyich]

Agree to change UseStateForUnknown since there is a default value will be returned if it's not provided.
But why add stringplanmodifier.RequiresReplace() here?

[csahu9]

PlanModifiers: []planmodifier.String{stringplanmodifier.RequiresReplace()},
If you see the replaced snippet, this modifier was already there.
I just added []planmodifier.String{stringplanmodifier.UseStateForUnknown()

[chuyich]

I know this PR didn’t introduce RequiresReplace(), but since we’re touching these fields, can we confirm replacement is actually required?

The resource has update handling for these attributes, so if the API supports changing them in place, RequiresReplace() may not be the right schema behavior. If replacement really is required by the backend, a short note in the PR description would help.

[csahu9]

As the original request was related to the idempotency break, I didn't explicitly check the update pattern for these attributes.
Though I got your point; let me check the modification behaviour for these parameters.

[chuyich]

Same here. Why stringplanmodifier.RequiresReplace() is needed?

[csahu9]

answered in previous comment.

[chuyich]

I agree with adding UseStateForUnknown() for an Optional + Computed field if the API provides the effective default, but can you explain why the schema default was removed and the default is now injected manually in Create() instead?

Right now the behavior for this field is split between schema, create, update, and read paths. I’d like to understand whether this was necessary to avoid plan/apply drift, and if so, whether we should document that rationale in the PR description.

[csahu9]

Before this change, this attribute was causing a plan drift even if it was not mentioned in the config.
So, I had removed the default value and added it in POST operation.

[chuyich]

Same here, Why stringplanmodifier.RequiresReplace() is needed?

[csahu9]

answered in previous comment.

[chuyich]

Issue #310 describes a create failure with duplicate entry on protocol_access_rules.client_ip, but this PR appears to fix omitted/defaulted field handling for v4_id_domain, vstorage_enabled, and windows.map_unknown_uid_to_default_user. Please clarify how this PR fixes #310 specifically, or update the PR title/body/changelog if it actually addresses a different bug.

Defaulting strategy changed and needs explanation
The PR removes some schema defaults and replaces them with manual default injection in Create(), while also adding UseStateForUnknown(). That may be the right fix, but it changes how the resource behaves across schema/create/update/read, so I’d like a short explanation of why this approach was chosen over keeping framework defaults and normalizing through read/plan behavior.

Please add or test then share in the PR to idempotency-focused test
Since the stated problem is drift/idempotency when defaults are omitted, I’d like confidence for:

1. create with those fields omitted,
2. second plan shows no diff,
3. update path behaves correctly when values are omitted or unknown.

Please confirm RequiresReplace() is intentional
I understand this PR didn’t introduce those plan modifiers, but since these lines are being touched, it would be good to confirm those fields truly require replacement and are not updateable in place.

[csahu9]

Issue #310 describes a create failure with duplicate entry on protocol_access_rules.client_ip, but this PR appears to fix omitted/defaulted field handling for v4_id_domain, vstorage_enabled, and windows.map_unknown_uid_to_default_user. Please clarify how this PR fixes #310 specifically, or update the PR title/body/changelog if it actually addresses a different bug.

Defaulting strategy changed and needs explanation The PR removes some schema defaults and replaces them with manual default injection in Create(), while also adding UseStateForUnknown(). That may be the right fix, but it changes how the resource behaves across schema/create/update/read, so I’d like a short explanation of why this approach was chosen over keeping framework defaults and normalizing through read/plan behavior.

Please add or test then share in the PR to idempotency-focused test Since the stated problem is drift/idempotency when defaults are omitted, I’d like confidence for:

1. create with those fields omitted,
2. second plan shows no diff,
3. update path behaves correctly when values are omitted or unknown.

Please confirm RequiresReplace() is intentional I understand this PR didn’t introduce those plan modifiers, but since these lines are being touched, it would be good to confirm those fields truly require replacement and are not updateable in place.

In the associated github issue #310, I've commented twice that the issue reported by the user is not observed. Most probably the end user tried to create the same resource without importing the existing configuration into TF state. While trying to reproducing the error idempotency issue was observed for which I've created this PR as mentioned in comments in the original issue.

The PR description already mentions about the fixes being suggested as well as the change log.

The default values for a few attributes were causing state drift if those parameters are not set in the config, thus failing the idempotency. So, introduced these default values only during creation.

Let me test and revert back if RequiresReplace() modifier is actually required in the schema or not.

[csahu9]

Hi @chuyich I tested modifying below parameters manually using REST and they seem to be modifiable.
enabled, vstorage_enabled, and windows.map_unknown_uid_to_default_user
Should I add changes for such params by removing the modifier RequiresReplace()?

[csahu9]

I've tested the modified resource with below config (where the attributes holding default values are omitted) and it creates the resource as expected and also maintains idempotency.

resource "netapp-ontap_nfs_service" "protocols_nfs_service" {
  # required to know which system to interface with
  cx_profile_name = "test-cluster"
  svm_name = "svm1"
  enabled = true
  protocol = {
    v3_enabled = true
    v40_enabled = true
    v40_features = {
      acl_enabled = false
    }
  }
}

I've create another github issue #688, where I will check and create another PR by removing the RequiresReplace() modifier for the optional attributes, thus making those modifiable.