Adding State Checks for Known Type and Value, and Sensitive Checks #275

bendbennett · 2024-01-15T17:22:51Z

Reference: #266

This PR forms part of the changes outlined in #266, specifically it includes state checks for the following:

ExpectKnownValue
ExpectKnownOutputValue
ExpectKnownOutputValueAtPath
ExpectSensitiveValue

The implementation in this PR differs from the way in which checks for known type and value are currently being handled for plan checks, in that the ExpectKnown<Value|OutputValue|OutputValueAtPath> state checks allow for explicitly checking for a null value by using a newly introduced null known value check type. An alternative can be seen in the plan check implementations, where the ExpectKnown<Value|OutputValue|OutputValueAtPath> plan checks return an error if a null value is found, and instead there are explicit checks for null values (e.g., ExpectNullOutputValue).

In this context, a significant difference between tfjson.Plan and tfjson.State is the handling of null output values. In the case of tfjson.Plan, an output that specifies an attribute, for instance, which is null appears in the plan, whereas null output values do not appear in state (refer to Allow null value outputs to be present in json output). As a consequence, there is no way for state checks that leverage tfjson.State to discriminate between output values that are null, and output values that do not exist. However, it is worth noting that output values that reference, for instance, an object that has attributes which are null will appear in tfjson.State.

Together this raises the question of whether we want to:

Modify plan checks for ExpectKnown<Value|OutputValue|OutputValueAtPath> to handle detection of nulls (to bring them into line with the state checks for ExpectKnown<Value|OutputValue|OutputValueAtPath> in this PR).
Modify state checks for ExpectKnown<Value|OutputValue|OutputValueAtPath> in this PR to align their behaviour with the analogous plan checks (i.e., return an error when null is detected).
Depending upon how the known value checks are handled, decide whether we want to remove expect null value plan checks, or add expect null value state checks.

Further Considerations

The implementation in this PR runs state checks after "post-apply, post-refresh. Do we want to consider running state checks at any other points? I avoided adding state checks for the "refresh" mode as the refresh command is deprecated, but we could consider this if it is thought to valuable. Should we consider state checks for import?
We could also consider adding the equivalent state checks for ExpectEmptyPlan and ExpectNonEmptyPlan if there is possible value in having these built-in checks made available.

* Configuring when state checks are executed. * Testing that state checks are executed.

…(apply) tests (#266)

…or checking of null values (#266)

…#266)

#266)

…null checking (#266)

…n checks (#266)

austinvalle

Everything looks great to me, good work @bendbennett! Left a couple comments but nothing major.

Love all the docs as well ❤️

plancheck/expect_known_output_value.go

plancheck/expect_known_output_value_at_path.go

plancheck/expect_known_value.go

plancheck/expect_null_output_value.go

knownvalue/null.go

austinvalle · 2024-01-19T22:46:19Z

knownvalue/null.go

+
+// CheckValue determines whether the passed value is of nil.
+func (v nullExact) CheckValue(other any) error {
+	if other != nil {


I vaguely remember checking an any/interface{} for nil and I believe it's not as straightforward as this for things like maps. (I don't know if the underlying tfjson implementation will expose this problem, so it may not be relevant)

This test will fail:

And delve shows it slightly different, but still should be considered null:

This article explains the problem a bit, may have to dip into some reflection if we need to cover nil cases like maps and slices.

https://vitaneri.com/posts/check-for-nil-interface-in-go

Thanks for raising this.

I've examined the values returned when using the tfjson implementation.

With a schema that looks as follows:

func (e *exampleResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { resp.Schema = schema.Schema{ Attributes: map[string]schema.Attribute{ /* ... */ "list_attribute": schema.ListAttribute{ Optional: true, /* ... */ }, "map_attribute": schema.MapAttribute{ Optional: true, /* ... */ }, "object_attribute": schema.ObjectAttribute{ Optional: true, AttributeTypes: map[string]attr.Type{ /* ... */ }, }, "set_attribute": schema.SetAttribute{ Optional: true, /* ... */ }, "list_nested_attribute": schema.ListNestedAttribute{ Optional: true, NestedObject: schema.NestedAttributeObject{ Attributes: map[string]schema.Attribute{ /* ... */ }, }, }, "map_nested_attribute": schema.MapNestedAttribute{ Optional: true, NestedObject: schema.NestedAttributeObject{ Attributes: map[string]schema.Attribute{ /* ... */ }, }, }, "set_nested_attribute": schema.SetNestedAttribute{ Optional: true, NestedObject: schema.NestedAttributeObject{ Attributes: map[string]schema.Attribute{ /* ... */ }, }, }, "single_nested_attribute": schema.SingleNestedAttribute{ Optional: true, Attributes: map[string]schema.Attribute{ /* ... */ }, }, }, Blocks: map[string]schema.Block{ "list_nested_block": schema.ListNestedBlock{ NestedObject: schema.NestedBlockObject{ Attributes: map[string]schema.Attribute{ /* ... */ }, Blocks: map[string]schema.Block{ "list_nested_nested_block": schema.ListNestedBlock{ NestedObject: schema.NestedBlockObject{ Attributes: map[string]schema.Attribute{ /* ... */ }, }, }, }, }, }, "set_nested_block": schema.SetNestedBlock{ NestedObject: schema.NestedBlockObject{ Attributes: map[string]schema.Attribute{ /* ... */ }, Blocks: map[string]schema.Block{ "set_nested_nested_block": schema.SetNestedBlock{ NestedObject: schema.NestedBlockObject{ Attributes: map[string]schema.Attribute{ /* ... */ }, }, }, }, }, }, "single_nested_block": schema.SingleNestedBlock{ Attributes: map[string]schema.Attribute{ /* ... */ }, }, }, } }

The plan output looks as follows:

{ "format_version": "1.2", "terraform_version": "1.7.0", "planned_values": { "root_module": { "resources": [ { "address": "example_resource.example", "mode": "managed", "type": "example_resource", "name": "example", "provider_name": "registry.terraform.io/bendbennett/playground", "schema_version": 0, "values": { "list_attribute": null, "list_nested_attribute": null, "list_nested_block": [], "map_attribute": null, "map_nested_attribute": null, "object_attribute": null, "set_attribute": null, "set_nested_attribute": null, "set_nested_block": [], "single_nested_attribute": null, "single_nested_block": null, }, "sensitive_values": { "list_nested_block": [], "set_nested_block": [] } } ] } }, "resource_changes": [ { "address": "example_resource.example", "mode": "managed", "type": "example_resource", "name": "example", "provider_name": "registry.terraform.io/bendbennett/playground", "change": { "actions": [ "create" ], "before": null, "after": { "list_attribute": null, "list_nested_attribute": null, "list_nested_block": [], "map_attribute": null, "map_nested_attribute": null, "object_attribute": null, "set_attribute": null, "set_nested_attribute": null, "set_nested_block": [], "single_nested_attribute": null, "single_nested_block": null, }, "after_unknown": { "id": true, "list_nested_block": [], "set_nested_block": [] }, "before_sensitive": false, "after_sensitive": { "list_nested_block": [], "set_nested_block": [] } } } ], "configuration": { "provider_config": { "example": { "name": "example", "full_name": "registry.terraform.io/bendbennett/playground" }, "playground": { "name": "playground", "full_name": "registry.terraform.io/bendbennett/playground" } }, "root_module": { "resources": [ { "address": "example_resource.example", "mode": "managed", "type": "example_resource", "name": "example", "provider_config_key": "example", "schema_version": 0 } ] } }, "timestamp": "2024-01-22T10:12:40Z", "errored": false }

The state looks as follows:

{ "version": 4, "terraform_version": "1.7.0", "serial": 1, "lineage": "2c8dadf4-11dd-22b9-c419-5d07aa808890", "outputs": {}, "resources": [ { "mode": "managed", "type": "example_resource", "name": "example", "provider": "provider[\"registry.terraform.io/bendbennett/playground\"]", "instances": [ { "schema_version": 0, "attributes": { "bool_attribute": null, "float64_attribute": null, "id": "example-id", "int64_attribute": null, "list_attribute": null, "list_nested_attribute": null, "list_nested_block": [], "map_attribute": null, "map_nested_attribute": null, "number_attribute": null, "object_attribute": null, "set_attribute": null, "set_nested_attribute": null, "set_nested_block": [], "single_nested_attribute": null, "single_nested_block": null, "string_attribute": null }, "sensitive_attributes": [] } ] } ], "check_results": null }

Debugging during test execution shows the following:

I've added some additional test coverage to the following:

TestExpectKnownValue_CheckState_AttributeValueNull

TestExpectKnownOutputValueAtPath_CheckState_AttributeValueNull

TestExpectKnownValue_CheckPlan_AttributeValueNull

TestExpectKnownOutputValueAtPath_CheckPlan_AttributeValueNull

TestExpectKnownOutputValue_CheckPlan_AttributeValueNull

Today I learned! I think we have historically avoided these sorts of issues by not performing (re-)assignment like that article shows. Hopefully we can rely on the static analysis tooling to catch the situation if for some reason it does get introduced and the unit testing does not catch it, because I'm a proponent of simpler == better where possible.

Thanks for adding the tests, good to know it shouldn't be a problem for us 🙂

I'm a proponent of simpler == better where possible.

Agreed 👍🏻

statecheck/expect_known_output_value.go

statecheck/expect_known_value.go

tfversion/versions.go

…d `ExpectNullOutputValueAtPath` plan checks (#266)

bflad

Overall very thrilled with the direction here! Great job. 🚀

bflad · 2024-01-22T14:47:30Z

knownvalue/null.go

+
+// NullExact returns a Check for asserting equality nil
+// and the value passed to the CheckValue method.
+func NullExact() nullExact {


Bikeshed (I'm so sorry): Are there other checks that could be run against a null value? Maybe this can be just Null if there is only the possibility for the value being null or not? I guess you could also say the same for boolean checks, but all the other typed checks could have additional checks. 🤷

Good point. Agree with the suggested naming change here. I've made the following changes:

NullExact => Null

nullExact => null

BoolExact => bool

boolExact => boolValue

The docs have been updated in accordance with these changes.

bflad · 2024-01-22T15:33:07Z

helper/resource/testing.go

@@ -795,6 +803,10 @@ type RefreshPlanChecks struct {
 	PostRefresh []plancheck.PlanCheck
 }

+// ConfigStateChecks runs all state checks in the slice. This occurs after the apply and refresh of a Config test are run.
+// All errors by state checks in this slice are aggregated, reported, and will result in a test failure.
+type ConfigStateChecks []statecheck.StateCheck


Open question: do we need this additional exported type? I'm guessing this was for a little bit of consistency with ConfigPlanChecks, however that is there because we need a structure to capture the various times plan checks could be ran. In my experience, typically you would introduce something like this if there were plans to attach methods to the type, but since there are not any here right now, I'm curious if there were future plans for that. Another option would also be to put this type in the statecheck package, so all the implementation details live in one place. I'm just wondering if we should treat this similar to how the plan checks do use []plancheck.PlanCheck directly once its at that "level". Thanks!

You're assumption is correct, the exported ConfigStateChecks type was purely for consistency. I've removed the type and replaced with the usage of []statecheck.StateCheck throughout.

bflad · 2024-01-22T15:39:57Z

helper/resource/testing_new_config.go

+	// Run post-apply, post-refresh state checks
+	if len(step.ConfigStateChecks) > 0 {


Historically we have ran state checks immediately after apply and before the additional plan checks (e.g. line 181 area) -- do we want to also do that here for consistency/ease of migration? In reality, the timing of the checks will determine which errors developers might see first: whether it be unexpectedly non-empty plans after apply or whether their state assertions are incorrect.

I've moved the execution of the ConfigStateChecks so that they run immediately after any Check (i.e., line 210).

plancheck/expect_null_output_value.go

bflad · 2024-01-22T15:51:00Z

helper/resource/state_checks.go

+		result = append(result, resp.Error)
+	}
+
+	return errors.Join(result...)


No more resource.ComposeAggregateTestCheckFunc vs resource.ComposeTestCheckFunc debates!

github-actions · 2024-02-25T02:01:11Z

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

bendbennett added 15 commits January 15, 2024 17:03

Adding StateCheck interface (#266)

f67b3e7

* Configuring when state checks are executed. * Testing that state checks are executed.

Adding validation to ensure state checks are only defined for config …

eed2a12

…(apply) tests (#266)

Adding ExpectKnownValue state check (#266)

d4e31d3

Adding ExpectKnownOutputValue state check (#266)

4e3ca3a

Adding ExpectKnownOutputValueAtPath state check (#266)

d93aa82

Modifying ExpectKnown<Value|OutputValue|OutputValueAtPath> to allow f…

598dac8

…or checking of null values (#266)

Adding ExpectSensitiveValue state check (#266)

c78e3e8

Adding documentation for state checks and null known value check type (…

1e51693

…#266)

Adding to the documentation for the custom known value check (#266)

3d4acf4

Adding changelog entries (#266)

01fe9a9

Refactoring to use updated known value check types (#266)

fb9fed8

Correcting documentation for revised naming of known value check types (

7ff68bc

#266)

Renaming nul known value check (#266)

5d04859

Fixing tests (#266)

a79aea8

Adding address and path to state check errors (#266)

f5abf73

bendbennett added the enhancement New feature or request label Jan 15, 2024

bendbennett added this to the v1.7.0 milestone Jan 15, 2024

bendbennett requested a review from a team as a code owner January 15, 2024 17:22

bendbennett added 4 commits January 16, 2024 08:58

Fixing navigation (#266)

648730a

Fixing changelog entries

c74a9e8

Modifying ExpectKnown<Value|OutputValue|OutputValueAtPath> to handle …

178c2c4

…null checking (#266)

Deprecating ExpectNullOutputValue and ExpectNullOutputValueAtPath pla…

c542a70

…n checks (#266)

austinvalle approved these changes Jan 19, 2024

View reviewed changes

bendbennett added 7 commits January 22, 2024 08:19

Adding return statements (#266)

518c94c

Adding change log entry for deprecation of ExpectNullOutputValue an…

35ffc45

…d `ExpectNullOutputValueAtPath` plan checks (#266)

Modifying return value of nullExact.String() (#266)

31f8d5e

Renaming variable (#266)

031df21

Adding comment for Terraform v1.4.6 (#266)

6df33b9

Adding further tests for null exact known value type check (#266)

15330e3

Merge branch 'main' into bendbennett/issues-266

e4e96ac

Linting (#266)

a88a10d

bflad approved these changes Jan 22, 2024

View reviewed changes

bendbennett added 3 commits January 23, 2024 10:41

Renaming BoolExact to Bool, and NullExact to Null (#266)

6d8f112

Removing ConfigStateChecks type (#266)

04cf3c9

Move execution of ConfigStateChecks (#266)

173d4b2

bendbennett merged commit 1b803b4 into main Jan 25, 2024
29 checks passed

bendbennett deleted the bendbennett/issues-266 branch January 25, 2024 13:28

bendbennett mentioned this pull request Feb 22, 2024

Adding Provider-Defined Function Testing hashicorp/terraform-provider-corner#210

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding State Checks for Known Type and Value, and Sensitive Checks #275

Adding State Checks for Known Type and Value, and Sensitive Checks #275

bendbennett commented Jan 15, 2024 •

edited

Loading

austinvalle left a comment

austinvalle Jan 19, 2024

austinvalle Jan 19, 2024

bendbennett Jan 22, 2024

bendbennett Jan 22, 2024

bflad Jan 22, 2024

austinvalle Jan 22, 2024

bflad left a comment

bflad Jan 22, 2024

bendbennett Jan 23, 2024

bflad Jan 22, 2024

bendbennett Jan 23, 2024

bflad Jan 22, 2024

bendbennett Jan 23, 2024

bflad Jan 22, 2024

github-actions bot commented Feb 25, 2024

		// Run post-apply, post-refresh state checks
		if len(step.ConfigStateChecks) > 0 {

Adding State Checks for Known Type and Value, and Sensitive Checks #275

Adding State Checks for Known Type and Value, and Sensitive Checks #275

Conversation

bendbennett commented Jan 15, 2024 • edited Loading

Further Considerations

austinvalle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 25, 2024

bendbennett commented Jan 15, 2024 •

edited

Loading