Skip to content

Commit

Permalink
tfsdk: Check ProposedNewState for State differences before marking un…
Browse files Browse the repository at this point in the history 
…knowns for Computed-only attributes in plans (#184)

* tfsdk: Check ProposedNewState for State differences before marking unknowns for Computed-only attributes in plans

Reference: #181

Previously, any `Computed` attributes would always trigger plan differences due to the unknown marking.

```
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:

  # hashicups_order.edu will be updated in-place
  ~ resource "hashicups_order" "edu" {
      ~ id           = "1" -> (known after apply)
      ~ items        = [
          ~ {
            ~ coffee   = {
              + description = (known after apply)
                id          = 3
              ~ image       = "/nomad.png" -> (known after apply)
              ~ name        = "Nomadicano" -> (known after apply)
              ~ price       = 150 -> (known after apply)
              ~ teaser      = "Drink one today and you will want to schedule another" -> (known after apply)
            }
              # (1 unchanged attribute hidden)

            ~ coffee   = {
              + description = (known after apply)
                id          = 1
              ~ image       = "/packer.png" -> (known after apply)
              ~ name        = "Packer Spiced Latte" -> (known after apply)
              ~ price       = 350 -> (known after apply)
              ~ teaser      = "Packed with goodness to spice up your images" -> (known after apply)
            }
              # (2 unchanged attributes hidden)
          },
        ]
      ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply)
    }

Plan: 0 to add, 1 to change, 0 to destroy.
```

This change limits the unknown marking to only be performed when there is actually a change between the `ProposedNewState` and `State`.

Without a configuration change:

```
No changes. Your infrastructure matches the configuration.
```

With a configuration change:

```
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:

  # hashicups_order.edu will be updated in-place
  ~ resource "hashicups_order" "edu" {
      ~ id           = "1" -> (known after apply)
      ~ items        = [
          ~ {
            ~ coffee   = {
              + description = (known after apply)
                id          = 3
              ~ image       = "/nomad.png" -> (known after apply)
              ~ name        = "Nomadicano" -> (known after apply)
              ~ price       = 150 -> (known after apply)
              ~ teaser      = "Drink one today and you will want to schedule another" -> (known after apply)
            }
              # (1 unchanged attribute hidden)

            ~ coffee   = {
              + description = (known after apply)
              ~ id          = 1 -> 2
              ~ image       = "/packer.png" -> (known after apply)
              ~ name        = "Packer Spiced Latte" -> (known after apply)
              ~ price       = 350 -> (known after apply)
              ~ teaser      = "Packed with goodness to spice up your images" -> (known after apply)
            }
              # (2 unchanged attributes hidden)
          },
        ]
      ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply)
    }

Plan: 0 to add, 1 to change, 0 to destroy.
```

As a followup, it may be necessary to invoke two passes of attribute and resource plan modifiers to ensure any changes captured by those modifiers triggers the unknown marking. As it stands now, a plan with no configuration changes, but changes due to plan modifiers will not correctly trigger the unknown marking. See also: #183


Co-authored-by: Katy Moe <katy@katy.moe>
  • Loading branch information
@bflad @kmoe
bflad and kmoe committed Sep 29, 2021
1 parent cf6f240 commit 7171bf2
Show file tree
Hide file tree
Showing 3 changed files with 474 additions and 72 deletions.
67 changes: 55 additions & 12 deletions tfsdk/serve.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -691,15 +691,57 @@ func (s *server) planResourceChange(ctx context.Context, req *tfprotov6.PlanReso
return
}

// first, mark any computed attributes that are null in the config as
// unknown in the plan, so providers have the choice to update them
// Execute any AttributePlanModifiers.
//
// do this first so that providers can override the unknown with a
// known value using any plan modifiers
// This pass is before any Computed-only attributes are marked as unknown
// to ensure any plan changes will trigger that behavior. These plan
// modifiers are run again after that marking to allow setting values
// and preventing extraneous plan differences.
//
// we only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point
if !plan.IsNull() {
// We only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point.
//
// TODO: Enabling this pass will generate the following test error:
//
// --- FAIL: TestServerPlanResourceChange/two_modifyplan_add_list_elem (0.00s)
// serve_test.go:3303: An unexpected error was encountered trying to read an attribute from the configuration. This is always an error in the provider. Please report the following to the provider developer:
//
// ElementKeyInt(1).AttributeName("name") still remains in the path: step cannot be applied to this value
//
// To fix this, (Config).GetAttribute() should return nil instead of the error.
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/183
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/150
// See also: https://github.com/hashicorp/terraform-plugin-framework/pull/167

// Execute any resource-level ModifyPlan method.
//
// This pass is before any Computed-only attributes are marked as unknown
// to ensure any plan changes will trigger that behavior. These plan
// modifiers be run again after that marking to allow setting values and
// preventing extraneous plan differences.
//
// TODO: Enabling this pass will generate the following test error:
//
// --- FAIL: TestServerPlanResourceChange/two_modifyplan_add_list_elem (0.00s)
// serve_test.go:3303: An unexpected error was encountered trying to read an attribute from the configuration. This is always an error in the provider. Please report the following to the provider developer:
//
// ElementKeyInt(1).AttributeName("name") still remains in the path: step cannot be applied to this value
//
// To fix this, (Config).GetAttribute() should return nil instead of the error.
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/183
// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/150
// See also: https://github.com/hashicorp/terraform-plugin-framework/pull/167

// After ensuring there are proposed changes, mark any computed attributes
// that are null in the config as unknown in the plan, so providers have
// the choice to update them.
//
// Later attribute and resource plan modifier passes can override the
// unknown with a known value using any plan modifiers.
//
// We only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point.
if !plan.IsNull() && !plan.Equal(state) {
modifiedPlan, err := tftypes.Transform(plan, markComputedNilsAsUnknown(ctx, config, resourceSchema))
if err != nil {
resp.Diagnostics.AddError(
Expand All @@ -711,8 +753,8 @@ func (s *server) planResourceChange(ctx context.Context, req *tfprotov6.PlanReso
plan = modifiedPlan
}

// next, execute any AttributePlanModifiers as long as there's a plan
// to modify.
// Execute any AttributePlanModifiers again. This allows overwriting
// any unknown values.
//
// We only do this if there's a plan to modify; otherwise, it
// represents a resource being deleted and there's no point.
Expand Down Expand Up @@ -766,17 +808,18 @@ func (s *server) planResourceChange(ctx context.Context, req *tfprotov6.PlanReso
}

resourceSchema.modifyAttributePlans(ctx, modifySchemaPlanReq, &modifySchemaPlanResp)
resp.RequiresReplace = modifySchemaPlanResp.RequiresReplace
resp.RequiresReplace = append(resp.RequiresReplace, modifySchemaPlanResp.RequiresReplace...)
plan = modifySchemaPlanResp.Plan.Raw
resp.Diagnostics = modifySchemaPlanResp.Diagnostics
if resp.Diagnostics.HasError() {
return
}
}

// third, execute any resource-level ModifyPlan method
// Execute any resource-level ModifyPlan method again. This allows
// overwriting any unknown values.
//
// we do this regardless of whether the plan is null or not, because we
// We do this regardless of whether the plan is null or not, because we
// want resources to be able to return diagnostics when planning to
// delete resources, e.g. to inform practitioners that the resource
// _can't_ be deleted in the API and will just be removed from
Expand Down
14 changes: 12 additions & 2 deletions tfsdk/serve_resource_attribute_plan_modifiers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ func (rt testServeResourceTypeAttributePlanModifiers) GetSchema(_ context.Contex
Type: types.StringType,
PlanModifiers: []AttributePlanModifier{testAttrDefaultValueModifier{}},
},
"computed_string_no_modifiers": {
Computed: true,
Type: types.StringType,
},
},
}, nil
}
Expand All @@ -114,6 +118,11 @@ var testServeResourceTypeAttributePlanModifiersSchema = &tfprotov6.Schema{
Version: 1,
Block: &tfprotov6.SchemaBlock{
Attributes: []*tfprotov6.SchemaAttribute{
{
Name: "computed_string_no_modifiers",
Computed: true,
Type: tftypes.String,
},
{
Name: "name",
Required: true,
Expand Down Expand Up @@ -173,8 +182,9 @@ var testServeResourceTypeAttributePlanModifiersSchema = &tfprotov6.Schema{

var testServeResourceTypeAttributePlanModifiersType = tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"name": tftypes.String,
"size": tftypes.Number,
"computed_string_no_modifiers": tftypes.String,
"name": tftypes.String,
"size": tftypes.Number,
"scratch_disk": tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"id": tftypes.String,
Expand Down

0 comments on commit 7171bf2

Please sign in to comment.