diff --git a/internal/configs/resource.go b/internal/configs/resource.go index c54557921188..1f67c6c40f68 100644 --- a/internal/configs/resource.go +++ b/internal/configs/resource.go @@ -89,6 +89,12 @@ func (r *Resource) ProviderConfigAddr() addrs.LocalProviderConfig { } } +// HasCustomConditions returns true if and only if the resource has at least +// one author-specified custom condition. +func (r *Resource) HasCustomConditions() bool { + return len(r.Postconditions) != 0 || len(r.Preconditions) != 0 +} + func decodeResourceBlock(block *hcl.Block, override bool) (*Resource, hcl.Diagnostics) { var diags hcl.Diagnostics r := &Resource{ diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 821e435884cc..238616c7d5e1 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strings" + "sync" "testing" "github.com/davecgh/go-spew/spew" @@ -399,6 +400,384 @@ resource "test_resource" "b" { } } +func TestContext2Plan_dataResourceChecksManagedResourceChange(t *testing.T) { + // This tests the situation where the remote system contains data that + // isn't valid per a data resource postcondition, but that the + // configuration is destined to make the remote system valid during apply + // and so we must defer reading the data resource and checking its + // conditions until the apply step. + // + // This is an exception to the rule tested in + // TestContext2Plan_dataReferencesResourceIndirectly which is relevant + // whenever there's at least one precondition or postcondition attached + // to a data resource. + // + // See TestContext2Plan_managedResourceChecksOtherManagedResourceChange for + // an incorrect situation where a data resource is used only indirectly + // to drive a precondition elsewhere, which therefore doesn't achieve this + // special exception. + + p := testProvider("test") + p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + Provider: providers.Schema{ + Block: &configschema.Block{}, + }, + ResourceTypes: map[string]providers.Schema{ + "test_resource": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "valid": { + Type: cty.Bool, + Required: true, + }, + }, + }, + }, + }, + DataSources: map[string]providers.Schema{ + "test_data_source": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Required: true, + }, + "valid": { + Type: cty.Bool, + Computed: true, + }, + }, + }, + }, + }, + } + var mu sync.Mutex + validVal := cty.False + p.ReadResourceFn = func(req providers.ReadResourceRequest) (resp providers.ReadResourceResponse) { + // NOTE: This assumes that the prior state declared below will have + // "valid" set to false already, and thus will match validVal above. + resp.NewState = req.PriorState + return resp + } + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) (resp providers.ReadDataSourceResponse) { + cfg := req.Config.AsValueMap() + mu.Lock() + cfg["valid"] = validVal + mu.Unlock() + resp.State = cty.ObjectVal(cfg) + return resp + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) { + cfg := req.Config.AsValueMap() + prior := req.PriorState.AsValueMap() + resp.PlannedState = cty.ObjectVal(map[string]cty.Value{ + "id": prior["id"], + "valid": cfg["valid"], + }) + return resp + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { + planned := req.PlannedState.AsValueMap() + + mu.Lock() + validVal = planned["valid"] + mu.Unlock() + + resp.NewState = req.PlannedState + return resp + } + + m := testModuleInline(t, map[string]string{ + "main.tf": ` + +resource "test_resource" "a" { + valid = true +} + +locals { + # NOTE: We intentionally read through a local value here to make sure + # that this behavior still works even if there isn't a direct dependency + # between the data resource and the managed resource. + object_id = test_resource.a.id +} + +data "test_data_source" "a" { + id = local.object_id + + lifecycle { + postcondition { + condition = self.valid + error_message = "Not valid!" + } + } +} +`}) + + managedAddr := mustResourceInstanceAddr(`test_resource.a`) + dataAddr := mustResourceInstanceAddr(`data.test_data_source.a`) + + // This state is intended to represent the outcome of a previous apply that + // failed due to postcondition failure but had already updated the + // relevant object to be invalid. + // + // It could also potentially represent a similar situation where the + // previous apply succeeded but there has been a change outside of + // Terraform that made it invalid, although technically in that scenario + // the state data would become invalid only during the planning step. For + // our purposes here that's close enough because we don't have a real + // remote system in place anyway. + priorState := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + managedAddr, + &states.ResourceInstanceObjectSrc{ + // NOTE: "valid" is false here but is true in the configuration + // above, which is intended to represent that applying the + // configuration change would make this object become valid. + AttrsJSON: []byte(`{"id":"boop","valid":false}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, priorState, DefaultPlanOpts) + assertNoErrors(t, diags) + + if rc := plan.Changes.ResourceInstance(dataAddr); rc != nil { + if got, want := rc.Action, plans.Read; got != want { + t.Errorf("wrong action for %s\ngot: %s\nwant: %s", dataAddr, got, want) + } + if got, want := rc.ActionReason, plans.ResourceInstanceReadBecauseDependencyPending; got != want { + t.Errorf("wrong action reason for %s\ngot: %s\nwant: %s", dataAddr, got, want) + } + } else { + t.Fatalf("no planned change for %s", dataAddr) + } + + if rc := plan.Changes.ResourceInstance(managedAddr); rc != nil { + if got, want := rc.Action, plans.Update; got != want { + t.Errorf("wrong action for %s\ngot: %s\nwant: %s", managedAddr, got, want) + } + if got, want := rc.ActionReason, plans.ResourceInstanceChangeNoReason; got != want { + t.Errorf("wrong action reason for %s\ngot: %s\nwant: %s", managedAddr, got, want) + } + } else { + t.Fatalf("no planned change for %s", managedAddr) + } + + // This is primarily a plan-time test, since the special handling of + // data resources is a plan-time concern, but we'll still try applying the + // plan here just to make sure it's valid. + newState, diags := ctx.Apply(plan, m) + assertNoErrors(t, diags) + + if rs := newState.ResourceInstance(dataAddr); rs != nil { + if !rs.HasCurrent() { + t.Errorf("no final state for %s", dataAddr) + } + } else { + t.Errorf("no final state for %s", dataAddr) + } + + if rs := newState.ResourceInstance(managedAddr); rs != nil { + if !rs.HasCurrent() { + t.Errorf("no final state for %s", managedAddr) + } + } else { + t.Errorf("no final state for %s", managedAddr) + } + + if got, want := validVal, cty.True; got != want { + t.Errorf("wrong final valid value\ngot: %#v\nwant: %#v", got, want) + } + +} + +func TestContext2Plan_managedResourceChecksOtherManagedResourceChange(t *testing.T) { + // This tests the incorrect situation where a managed resource checks + // another managed resource indirectly via a data resource. + // This doesn't work because Terraform can't tell that the data resource + // outcome will be updated by a separate managed resource change and so + // we expect it to fail. + // This would ideally have worked except that we previously included a + // special case in the rules for data resources where they only consider + // direct dependencies when deciding whether to defer (except when the + // data resource itself has conditions) and so they can potentially + // read "too early" if the user creates the explicitly-not-recommended + // situation of a data resource and a managed resource in the same + // configuration both representing the same remote object. + + p := testProvider("test") + p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + Provider: providers.Schema{ + Block: &configschema.Block{}, + }, + ResourceTypes: map[string]providers.Schema{ + "test_resource": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Computed: true, + }, + "valid": { + Type: cty.Bool, + Required: true, + }, + }, + }, + }, + }, + DataSources: map[string]providers.Schema{ + "test_data_source": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Required: true, + }, + "valid": { + Type: cty.Bool, + Computed: true, + }, + }, + }, + }, + }, + } + var mu sync.Mutex + validVal := cty.False + p.ReadResourceFn = func(req providers.ReadResourceRequest) (resp providers.ReadResourceResponse) { + // NOTE: This assumes that the prior state declared below will have + // "valid" set to false already, and thus will match validVal above. + resp.NewState = req.PriorState + return resp + } + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) (resp providers.ReadDataSourceResponse) { + cfg := req.Config.AsValueMap() + if cfg["id"].AsString() == "main" { + mu.Lock() + cfg["valid"] = validVal + mu.Unlock() + } + resp.State = cty.ObjectVal(cfg) + return resp + } + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) { + cfg := req.Config.AsValueMap() + prior := req.PriorState.AsValueMap() + resp.PlannedState = cty.ObjectVal(map[string]cty.Value{ + "id": prior["id"], + "valid": cfg["valid"], + }) + return resp + } + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { + planned := req.PlannedState.AsValueMap() + + if planned["id"].AsString() == "main" { + mu.Lock() + validVal = planned["valid"] + mu.Unlock() + } + + resp.NewState = req.PlannedState + return resp + } + + m := testModuleInline(t, map[string]string{ + "main.tf": ` + +resource "test_resource" "a" { + valid = true +} + +locals { + # NOTE: We intentionally read through a local value here because a + # direct reference from data.test_data_source.a to test_resource.a would + # cause Terraform to defer the data resource to the apply phase due to + # there being a pending change for the managed resource. We're explicitly + # testing the failure case where the data resource read happens too + # eagerly, which is what results from the reference being only indirect + # so Terraform can't "see" that the data resource result might be affected + # by changes to the managed resource. + object_id = test_resource.a.id +} + +data "test_data_source" "a" { + id = local.object_id +} + +resource "test_resource" "b" { + valid = true + + lifecycle { + precondition { + condition = data.test_data_source.a.valid + error_message = "Not valid!" + } + } +} +`}) + + managedAddrA := mustResourceInstanceAddr(`test_resource.a`) + managedAddrB := mustResourceInstanceAddr(`test_resource.b`) + + // This state is intended to represent the outcome of a previous apply that + // failed due to postcondition failure but had already updated the + // relevant object to be invalid. + // + // It could also potentially represent a similar situation where the + // previous apply succeeded but there has been a change outside of + // Terraform that made it invalid, although technically in that scenario + // the state data would become invalid only during the planning step. For + // our purposes here that's close enough because we don't have a real + // remote system in place anyway. + priorState := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + managedAddrA, + &states.ResourceInstanceObjectSrc{ + // NOTE: "valid" is false here but is true in the configuration + // above, which is intended to represent that applying the + // configuration change would make this object become valid. + AttrsJSON: []byte(`{"id":"main","valid":false}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + s.SetResourceInstanceCurrent( + managedAddrB, + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"id":"checker","valid":true}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan(m, priorState, DefaultPlanOpts) + if !diags.HasErrors() { + t.Fatalf("unexpected successful plan; should've failed with non-passing precondition") + } + + if got, want := diags.Err().Error(), "Resource precondition failed: Not valid!"; !strings.Contains(got, want) { + t.Errorf("Missing expected error message\ngot: %s\nwant substring: %s", got, want) + } +} + func TestContext2Plan_destroyWithRefresh(t *testing.T) { m := testModuleInline(t, map[string]string{ "main.tf": ` diff --git a/internal/terraform/context_plan_test.go b/internal/terraform/context_plan_test.go index 3cacc8f0a0e9..d05c2eb75c8d 100644 --- a/internal/terraform/context_plan_test.go +++ b/internal/terraform/context_plan_test.go @@ -9,6 +9,7 @@ import ( "sort" "strings" "sync" + "sync/atomic" "testing" "github.com/davecgh/go-spew/spew" @@ -6260,7 +6261,13 @@ data "test_data_source" "d" { } } -func TestContext2Plan_dataReferencesResource(t *testing.T) { +func TestContext2Plan_dataReferencesResourceDirectly(t *testing.T) { + // When a data resource refers to a managed resource _directly_, any + // pending change for the managed resource will cause the data resource + // to be deferred to the apply step. + // See also TestContext2Plan_dataReferencesResourceIndirectly for the + // other case, where the reference is indirect. + p := testProvider("test") p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) (resp providers.ReadDataSourceResponse) { @@ -6319,6 +6326,79 @@ data "test_data_source" "e" { } } +func TestContext2Plan_dataReferencesResourceIndirectly(t *testing.T) { + // When a data resource refers to a managed resource indirectly, pending + // changes for the managed resource _do not_ cause the data resource to + // be deferred to apply. This is a pragmatic special case added for + // backward compatibility with the old situation where we would _always_ + // eagerly read data resources with known configurations, regardless of + // the plans for their dependencies. + // This test creates an indirection through a local value, but the same + // principle would apply for both input variable and output value + // indirection. + // + // See also TestContext2Plan_dataReferencesResourceDirectly for the + // other case, where the reference is direct. + // This special exception doesn't apply for a data resource that has + // custom conditions; see + // TestContext2Plan_dataResourceChecksManagedResourceChange for that + // situation. + + p := testProvider("test") + var applyCount int64 + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { + atomic.AddInt64(&applyCount, 1) + resp.NewState = req.PlannedState + return resp + } + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) (resp providers.ReadDataSourceResponse) { + if atomic.LoadInt64(&applyCount) == 0 { + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("data source read before managed resource apply")) + } else { + resp.State = req.Config + } + return resp + } + + m := testModuleInline(t, map[string]string{ + "main.tf": ` +locals { + x = "value" +} + +resource "test_resource" "a" { + value = local.x +} + +locals { + y = test_resource.a.value +} + +// test_resource.a.value would ideally cause a pending change for +// test_resource.a to defer this to the apply step, but we intentionally don't +// do that when it's indirect (through a local value, here) as a concession +// to backward compatibility. +data "test_data_source" "d" { + foo = local.y +} +`}) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + _, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + if !diags.HasErrors() { + t.Fatalf("successful plan; want an error") + } + + if got, want := diags.Err().Error(), "data source read before managed resource apply"; !strings.Contains(got, want) { + t.Errorf("Missing expected error message\ngot: %s\nwant substring: %s", got, want) + } +} + func TestContext2Plan_skipRefresh(t *testing.T) { p := testProvider("test") p.PlanResourceChangeFn = testDiffFn diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 675934935b66..d7a7af5df860 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1601,7 +1601,20 @@ func (n *NodeAbstractResourceInstance) dependenciesHavePendingChanges(ctx EvalCo // changes, since they won't show up as changes in the // configuration. changes := ctx.Changes() - for _, d := range n.dependsOn { + + depsToUse := n.dependsOn + + if n.Addr.Resource.Resource.Mode == addrs.DataResourceMode { + if n.Config.HasCustomConditions() { + // For a data resource with custom conditions we need to look at + // the full set of resource dependencies -- both direct and + // indirect -- because an upstream update might be what's needed + // in order to make a condition pass. + depsToUse = n.Dependencies + } + } + + for _, d := range depsToUse { if d.Resource.Mode == addrs.DataResourceMode { // Data sources have no external side effects, so they pose a need // to delay this read. If they do have a change planned, it must be diff --git a/website/docs/language/data-sources/index.mdx b/website/docs/language/data-sources/index.mdx index 6cfcf04712ec..d9ea8257fcd2 100644 --- a/website/docs/language/data-sources/index.mdx +++ b/website/docs/language/data-sources/index.mdx @@ -78,18 +78,23 @@ in more detail in the following sections. ## Data Resource Behavior -If the query constraint arguments for a data resource refer only to constant -values or values that are already known, the data resource will be read and its -state updated during Terraform's "refresh" phase, which runs prior to creating a plan. -This ensures that the retrieved data is available for use during planning and -so Terraform's plan will show the actual values obtained. - -Query constraint arguments may refer to values that cannot be determined until -after configuration is applied, such as the id of a managed resource that has -not been created yet. In this case, reading from the data source is deferred -until the apply phase, and any references to the results of the data resource -elsewhere in configuration will themselves be unknown until after the -configuration has been applied. +Terraform reads data resources during the planning phase when possible, but +announces in the plan when it must defer reading resources until the apply +phase to preserve the order of operations. Terraform defers reading data +resources in the following situations: +* At least one of the given arguments is a managed resource attribute or + other value that Terraform cannot predict until the apply step. +* The data resource depends directly on a managed resource that itself has + planned changes in the current plan. +* The data resource has + [custom conditions](#custom-condition-checks) + and it depends directly or indirectly on a managed resource that itself + has planned changes in the current plan. + +Refer to [Data Resource Dependencies](#data-resource-dependencies) for details +on what it means for a data resource to depend on other objects. Any resulting +attribute of such a data resource will be unknown during planning, so it cannot +be used in situations where values must be fully known. ## Local-only Data Sources @@ -118,7 +123,9 @@ In order to ensure that data sources are accessing the most up to date information possible in a wide variety of use cases, arguments directly referencing managed resources are treated the same as if the resource was listed in `depends_on`. This behavior can be avoided when desired by indirectly -referencing the managed resource values through a `local` value. +referencing the managed resource values through a `local` value, unless the +data resource itself has +[custom conditions](#custom-condition-checks). ~> **NOTE:** **In Terraform 0.12 and earlier**, due to the data resource behavior of deferring the read until the apply phase when depending on values that are not yet known, using `depends_on` with `data` resources will force the read to always be deferred to the apply phase, and therefore a configuration that uses `depends_on` with a `data` resource can never converge. Due to this behavior, we do not recommend using `depends_on` with data resources. diff --git a/website/docs/language/expressions/custom-conditions.mdx b/website/docs/language/expressions/custom-conditions.mdx index 6a3a3d36030d..b6fa0f1109de 100644 --- a/website/docs/language/expressions/custom-conditions.mdx +++ b/website/docs/language/expressions/custom-conditions.mdx @@ -92,11 +92,13 @@ The `lifecycle` block inside a `resource` or `data` block can include both `prec - Terraform evaluates `precondition` blocks after evaluating existing `count` and `for_each` arguments. This lets Terraform evaluate the precondition separately for each instance and then make `each.key`, `count.index`, etc. available to those conditions. Terraform also evaluates preconditions before evaluating the resource's configuration arguments. Preconditions can take precedence over argument evaluation errors. - Terraform evaluates `postcondition` blocks after planning and applying changes to a managed resource, or after reading from a data source. Postcondition failures prevent changes to other resources that depend on the failing resource. +In most cases, we do not recommend including both a `data` block and a `resource` block that both represent the same object in the same configuration. Doing so can prevent Terraform from understanding that the `data` block result can be affected by changes in the `resource` block. However, when you need to check a result of a `resource` block that the resource itself does not directly export, you can use a `data` block to check that object safely as long as you place the check as a direct `postcondition` of the `data` block. This tells Terraform that the `data` block is serving as a check of an object defined elsewhere, allowing Terraform to perform actions in the correct order. + #### Outputs An `output` block can include a `precondition` block. -Preconditions can serve a symmetrical purpose to input variable `validation` blocks. Whereas input variable validation checks assumptions the module makes about its inputs, preconditions check guarantees that the module makes about its outputs. You can use preconditions to prevent Terraform from saving an invalid new output value in the state. You can also use them to preserve the output value from the previous apply, if applicable. +Preconditions can serve a symmetrical purpose to input variable `validation` blocks. Whereas input variable validation checks assumptions the module makes about its inputs, preconditions check guarantees that the module makes about its outputs. You can use preconditions to prevent Terraform from saving an invalid new output value in the state. You can also use them to preserve a valid output value from the previous apply, if applicable. Terraform evaluates output value preconditions before evaluating the `value` expression to finalize the result. Preconditions can take precedence over potential errors in the `value` expression. @@ -144,16 +146,23 @@ data "aws_ebs_volume" "example" { name = "volume-id" values = [aws_instance.example.root_block_device.volume_id] } + + # Whenever a data resource is verifying the result of a managed resource + # declared in the same configuration, you MUST write the checks as + # postconditions of the data resource. This ensures Terraform will wait + # to read the data resource until after any changes to the managed resource + # have completed. + lifecycle { + # The EC2 instance will have an encrypted root volume. + postcondition { + condition = self.encrypted + error_message = "The server's root volume is not encrypted." + } + } } output "api_base_url" { value = "https://${aws_instance.example.private_dns}:8433/" - - # The EC2 instance will have an encrypted root volume. - precondition { - condition = data.aws_ebs_volume.example.encrypted - error_message = "The server's root volume is not encrypted." - } } ```