`computed` field producing spurious plan changes with framework but not SDK v2

[asaba-hashi]

Module version

github.com/hashicorp/terraform-plugin-framework v1.0.1

Relevant provider source code

PR for updating the provider to framework: https://github.com/JupiterOne/terraform-provider-jupiterone/pull/136/files#diff-516571d421fdf72766d23e79357696f48d9949132cfd700d30df5a26fa0dc8aa

These errors are occurred in the tests when moving from SDK v2 to Framework:

Framework:

		        "version": schema.Int64Attribute{
				Description: "Computed current version of the rule. Incremented each time the rule is updated.",
				Computed:    true,
			},

SDK:

			"version": {
				Type:        schema.TypeInt,
				Description: "Computed current version of the rule. Incremented each time the rule is updated.",
				Computed:    true,
			},

Terraform Configuration Files

This happens as part of the acceptance tests.

func TestRuleInstance_Basic(t *testing.T) {
	ctx := context.TODO()

	recorder, cleanup := setupCassettes(t.Name())
	defer cleanup(t)
	testHttpClient := cleanhttp.DefaultClient()
	testHttpClient.Transport = logging.NewTransport("JupiterOne", recorder)
	// testJ1Client is used for direct calls for CheckDestroy/etc.
	testJ1Client, err := (&JupiterOneProviderModel{httpClient: testHttpClient}).Client(ctx)
	if err != nil {
		t.Fatal("error configuring check client", err)
	}

	ruleName := "tf-provider-rule"
	resourceName := "jupiterone_rule.test"
	operations := getValidOperations()
	operationsUpdate := getValidOperationsWithoutConditions()
	resource.Test(t, resource.TestCase{
		PreCheck:                 func() { testAccPreCheck(t) },
		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories(testJ1Client),
		CheckDestroy:             testAccCheckRuleInstanceDestroy(ctx, resourceName, testJ1Client),
		Steps: []resource.TestStep{
			{
				Config: testRuleInstanceBasicConfigWithOperations(ruleName, operations),
				Check: resource.ComposeTestCheckFunc(
				),
			},
			{
				Config: testRuleInstanceBasicConfigWithOperations(ruleName, operationsUpdate),
				Check: resource.ComposeTestCheckFunc(
				),
			},
		},
	})
}

func testRuleInstanceBasicConfigWithOperations(rName string, operations string) string {
	return fmt.Sprintf(`
		resource "jupiterone_rule" "test" {
			name = %q
			description = "Test"
			spec_version = 1
			polling_interval = "ONE_WEEK"
			tags = ["tag1","tag2"]

			question {
				queries {
					name = "query0"
					query = "Find DataStore with classification=('critical' or 'sensitive' or 'confidential' or 'restricted') and encrypted!=true"
					version = "v1"
				}
			}

			outputs = [
				"queries.query0.total",
				"alertLevel"
			]

			operations = %q
		}
	`, rName, operations)
}

Debug Output

https://gist.github.com/asaba-hashi/275dd1d9fc6f774a2ae5629143acde30

Expected Behavior

The tests should have passed because no changes were detected in the plan for just the computed field.

Actual Behavior

The test failed with:

=== RUN   TestRuleInstance_Basic
    resource_rule_test.go:36: Step 1/2 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
        stdout
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # jupiterone_rule.test will be updated in-place
          ~ resource "jupiterone_rule" "test" {
                id               = "f2ad0219-549e-4f36-9569-35f8979c38fa"
                name             = "tf-provider-rule"
                tags             = [
                    "tag1",
                    "tag2",
                ]
              ~ version          = 1 -> (known after apply)
                # (5 unchanged attributes hidden)
        
                # (1 unchanged block hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.

Adding UseStateForUnknown() produces the following error:

			"version": schema.Int64Attribute{
				Description: "Computed current version of the rule. Incremented each time the rule is updated.",
				Computed:    true,
				PlanModifiers: []planmodifier.Int64{
					int64planmodifier.UseStateForUnknown(),
				},
			},

=== RUN   TestRuleInstance_Basic
    resource_rule_test.go:36: Step 2/2 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to jupiterone_rule.test, provider
        "provider[\"registry.terraform.io/hashicorp/jupiterone\"]" produced an
        unexpected new value: .version: was cty.NumberIntVal(1), but now
        cty.NumberIntVal(2).
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestRuleInstance_Basic (2.08s)

A full workaround can be implemented by providing a ModifyPlan() that will run after the field plan modifier:

// ModifyPlan is a workaround for unexpected behavior in the framework around
// the `computed: true` `version` field to make sure that it is only part of
// the plan if there is some other change in the resource.
//
// Based on the implementation of the Time resource:
// https://github.com/hashicorp/terraform-provider-time/blob/main/internal/provider/resource_time_rotating.go#L189-L234
func (*QuestionRuleResource) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) {
	// Plan does not need to be modified when the resource is being destroyed.
	if req.Plan.Raw.IsNull() {
		return
	}

	// Plan only needs modifying if the resource already exists as the purpose of
	// the plan modifier is to show updated attribute values on CLI.
	if req.State.Raw.IsNull() {
		return
	}

	var plan, state *RuleModel

	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
	resp.Diagnostics.Append(req.State.Get(ctx, &state)...)

	if resp.Diagnostics.HasError() {
		return
	}

	if !reflect.DeepEqual(plan, state) {
		resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, path.Root("version"),
			types.Int64Unknown())...)
	}
}

Steps to Reproduce

1. Remove or comment out the workaround:
   1. (*RuleResource) ModifyPlan()
   2. UseStateForUnknown on the version field schema
2. make test.

References

[bflad]

Thank you so much for filing this detailed bug report, @asaba-hashi. I'm going to look into this particular case further, hopefully in the context of also implementing #627.

[bflad]

While trying out #627 logging, it looks like the operations attribute may be the culprit that is causing the framework to mark the plan with unknowns:

2023-01-19T19:19:08.333-0500 [DEBUG] sdk.framework: Detected value change between proposed new state and prior state: tf_req_id=47953e38-cd0b-1585-0b8a-dc7aa420caa3 tf_provider_addr=registry.terraform.io/hashicorp/jupiterone tf_rpc=PlanResourceChange tf_resource_type=jupiterone_rule tf_attribute_path=operations
2023-01-19T19:19:08.333-0500 [DEBUG] sdk.framework: Marking Computed attributes with null configuration values as unknown (known after apply) in the plan to prevent potential Terraform errors: tf_req_id=47953e38-cd0b-1585-0b8a-dc7aa420caa3 tf_provider_addr=registry.terraform.io/hashicorp/jupiterone tf_rpc=PlanResourceChange tf_resource_type=jupiterone_rule

Here's the schema from the time I pulled down the code:

			"operations": schema.StringAttribute{
				Description: "Actions that are executed when a corresponding condition is met.",
				Required:    true,
				// PlanModifiers currently tries to diff the json objects and
				// ignore formatting changes, but long term should probably
				// be a TODO for a more complete schema and encouraging
				// jsonencode() usage instead.
				PlanModifiers: []planmodifier.String{
					jsonIgnoreDiffPlanModifier(),
				},
				// TODO: similar to above, longer term is to define more of the
				// schema for HCL and encourage use of jsonencode()
				// Alternative: Look for a JSONString CustomType that comes
				// with it's own validation in it's marshalling
				Validators: []validator.String{
					jsonValidator{},
				},
			},

I'm suspecting that it may be JSON string normalization that may be causing the issue in this case. The current plan modification workaround you are using is a possible solution for now, however it does not scale. In these cases where possible, it's generally preferable to prevent the state from becoming out of sync with the configuration and therefore the next plan. This also prevents Terraform from potentially being noisy about drift detection.

The way you can do this is by adding a similar JSON normalization check to the Read method between the existing state and the incoming state (API response). Prefer to keep the existing state value instead of refreshing/replacing it if they are semantically equivalent.

---

#70 would be an enhancement so custom types (and/or attribute definitions) can signal semantic equality between two values. In this case, there would be a JSON string custom type (potentially provided by HashiCorp in the future) that would implement the JSON normalization logic. The framework could determine automatically during Set or SetAttribute that the new state and prior state JSON strings are actually "equivalent" and keep the prior state in this case, hence removing the difference.

This would be similar to the very recent DiffSuppressOnRefresh support in terraform-plugin-sdk.

[bflad]

This should be covered by #737, which will release with v1.3.0 soon. The https://developer.hashicorp.com/terraform/plugin/framework/handling-data/custom-types documentation page releasing with v1.3.0 will be revamped to cover how to set up a custom type with the logic to automatically handle situations like these. 👍

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.