From 9af67806fc600d10ef8a285c847aebd0a2a61627 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 31 Aug 2018 16:42:07 -0700 Subject: [PATCH] core: Prune placeholder objects from state after refresh Prior to our refactoring here, we were relying on a lucky coincidence for correct behavior of the plan walk following a refresh in the same run: - The refresh phase created placeholder objects in the state to represent any resource instance pending creation, to allow the interpolator to read attributes from them when evaluating "provider" and "data" blocks. In effect, the refresh walk is creating a partial plan that only covers creation actions, but was immediately discarding the actual diff entries and storing only the planned new state. - It happened that objects pending creation showed up in state with an empty ID value, since that only gets assigned by the provider during apply. - The Refresh function concluded by calling terraform.State.Prune, which deletes from the state any objects that have an empty ID value, which therefore prevented these temporary objects from surviving into the plan phase. After refactoring, we no longer have this special ID field on instance object state, and we instead rely on the Status field for tracking such things. We also no longer have an explicit "prune" step on state, since the state mutation methods themselves keep the structure pruned. To address this, here we introduce a new instance object status "planned", which is equivalent to having an empty ID value in the old world. We also introduce a new method on states.SyncState that deletes from the state any planned objects, which therefore replaces that portion of the old State.prune operation just for this refresh use-case. Finally, we are now expecting the expression evaluator to pull pending objects from the planned changeset rather than from the state directly, and so for correct results these placeholder resource creation changes must also be reported in a throwaway changeset during the refresh walk. The addition of states.ObjectPlanned also permits a previously-missing safety check in the expression evaluator to prevent us from relying on the incomplete value stored in state for a pending object, in the event that some bug prevents the real pending object from being written into the planned changeset. --- states/instance_object.go | 12 +++++++ states/sync.go | 53 ++++++++++++++++++++++++++++++ terraform/context.go | 20 +++++++++++ terraform/eval_diff.go | 8 ++++- terraform/evaluate.go | 26 +++++++++++++++ terraform/node_resource_refresh.go | 14 ++++++++ 6 files changed, 132 insertions(+), 1 deletion(-) diff --git a/states/instance_object.go b/states/instance_object.go index 484743051..be836053f 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -51,6 +51,18 @@ const ( // update, or delete operation. Since it cannot be moved into the // ObjectRead state, a tainted object must be replaced. ObjectTainted ObjectStatus = 'T' + + // ObjectPlanned is a special object status used only for the transient + // placeholder objects we place into state during the refresh and plan + // walks to stand in for objects that will be created during apply. + // + // Any object of this status must have a corresponding change recorded + // in the current plan, whose value must then be used in preference to + // the value stored in state when evaluating expressions. A planned + // object stored in state will be incomplete if any of its attributes are + // not yet known, and the plan must be consulted in order to "see" those + // unknown values, because the state is not able to represent them. + ObjectPlanned ObjectStatus = 'P' ) // Encode marshals the value within the receiver to produce a diff --git a/states/sync.go b/states/sync.go index 669d46162..617be07ca 100644 --- a/states/sync.go +++ b/states/sync.go @@ -336,6 +336,59 @@ func (s *SyncState) ForgetResourceInstanceDeposed(addr addrs.AbsResourceInstance ms.ForgetResourceInstanceDeposed(addr.Resource, key) } +// RemovePlannedResourceInstanceObjects removes from the state any resource +// instance objects that have the status ObjectPlanned, indiciating that they +// are just transient placeholders created during planning. +// +// Note that this does not restore any "ready" or "tainted" object that might +// have been present before the planned object was written. The only real use +// for this method is in preparing the state created during a refresh walk, +// where we run the planning step for certain instances just to create enough +// information to allow correct expression evaluation within provider and +// data resource blocks. Discarding planned instances in that case is okay +// because the refresh phase only creates planned objects to stand in for +// objects that don't exist yet, and thus the planned object must have been +// absent before by definition. +func (s *SyncState) RemovePlannedResourceInstanceObjects() { + // TODO: Merge together the refresh and plan phases into a single walk, + // so we can remove the need to create this "partial plan" during refresh + // that we then need to clean up before proceeding. + + s.lock.Lock() + defer s.lock.Unlock() + + for _, ms := range s.state.Modules { + moduleAddr := ms.Addr + + for _, rs := range ms.Resources { + resAddr := rs.Addr + + for ik, is := range rs.Instances { + instAddr := resAddr.Instance(ik) + + if is.Current != nil && is.Current.Status == ObjectPlanned { + // Setting the current instance to nil removes it from the + // state altogether if there are not also deposed instances. + ms.SetResourceInstanceCurrent(instAddr, nil, rs.ProviderConfig) + } + + for dk, obj := range is.Deposed { + // Deposed objects should never be "planned", but we'll + // do this anyway for the sake of completeness. + if obj.Status == ObjectPlanned { + ms.ForgetResourceInstanceDeposed(instAddr, dk) + } + } + } + } + + // We may have deleted some objects, which means that we may have + // left a module empty, and so we must prune to preserve the invariant + // that only the root module is allowed to be empty. + s.maybePruneModule(moduleAddr) + } +} + // Lock acquires an explicit lock on the state, allowing direct read and write // access to the returned state object. The caller must call Unlock once // access is no longer needed, and then immediately discard the state pointer diff --git a/terraform/context.go b/terraform/context.go index 5f9b3164d..8f2be6311 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -544,6 +544,15 @@ func (c *Context) Refresh() (*states.State, tfdiags.Diagnostics) { // Copy our own state c.state = c.state.DeepCopy() + // Refresh builds a partial changeset as part of its work because it must + // create placeholder stubs for any resource instances that'll be created + // in subsequent plan so that provider configurations and data resources + // can interpolate from them. This plan is always thrown away after + // the operation completes, restoring any existing changeset. + oldChanges := c.changes + defer func() { c.changes = oldChanges }() + c.changes = plans.NewChanges() + // Build the graph. graph, diags := c.Graph(GraphTypeRefresh, nil) if diags.HasErrors() { @@ -557,6 +566,17 @@ func (c *Context) Refresh() (*states.State, tfdiags.Diagnostics) { return nil, diags } + // During our walk we will have created planned object placeholders in + // state for resource instances that are in configuration but not yet + // created. These were created only to allow expression evaluation to + // work properly in provider and data blocks during the walk and must + // now be discarded, since a subsequent plan walk is responsible for + // creating these "for real". + // TODO: Consolidate refresh and plan into a single walk, so that the + // refresh walk doesn't need to emulate various aspects of the plan + // walk in order to properly evaluate provider and data blocks. + c.state.SyncWrapper().RemovePlannedResourceInstanceObjects() + return c.state, diags } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 639fe47ad..3510c79bc 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -322,7 +322,13 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // Update the state if we care if n.OutputState != nil { *n.OutputState = &states.ResourceInstanceObject{ - Status: states.ObjectReady, + // We use the special "planned" status here to note that this + // object's value is not yet complete. Objects with this status + // cannot be used during expression evaluation, so the caller + // must _also_ record the returned change in the active plan, + // which the expression evaluator will use in preference to this + // incomplete value recorded in the state. + Status: states.ObjectPlanned, Value: plannedNewVal, } } diff --git a/terraform/evaluate.go b/terraform/evaluate.go index 131d08ce0..04d78ed8c 100644 --- a/terraform/evaluate.go +++ b/terraform/evaluate.go @@ -563,6 +563,19 @@ func (d *evaluationStateData) getResourceInstanceSingle(addr addrs.ResourceInsta return val, diags } + if is.Current.Status == states.ObjectPlanned { + // If the object is in planned status then we should not + // get here, since we should've found a pending value + // in the plan above instead. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Missing pending object in plan", + Detail: fmt.Sprintf("Instance %s is marked as having a change pending but that change is not recorded in the plan. This is a bug in Terraform; please report it.", addr), + Subject: &config.DeclRange, + }) + return cty.UnknownVal(ty), diags + } + ios, err := is.Current.Decode(ty) if err != nil { // This shouldn't happen, since by the time we get here @@ -634,6 +647,19 @@ func (d *evaluationStateData) getResourceInstancesAll(addr addrs.Resource, rng t continue } + if is.Current.Status == states.ObjectPlanned { + // If the object is in planned status then we should not + // get here, since we should've found a pending value + // in the plan above instead. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Missing pending object in plan", + Detail: fmt.Sprintf("Instance %s is marked as having a change pending but that change is not recorded in the plan. This is a bug in Terraform; please report it.", instAddr), + Subject: &config.DeclRange, + }) + continue + } + ios, err := is.Current.Decode(ty) if err != nil { // This shouldn't happen, since by the time we get here diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index c0bff0361..472eb853d 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -270,6 +270,20 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResourceNoState( ProviderSchema: &providerSchema, State: &state, }, + + // We must also save the planned change, so that expressions in + // other nodes, such as provider configurations and data resources, + // can work with the planned new value. + // + // This depends on the fact that Context.Refresh creates a + // temporary new empty changeset for the duration of its graph + // walk, and so this recorded change will be discarded immediately + // after the refresh walk completes. + &EvalWriteDiff{ + Addr: addr.Resource, + Change: &change, + ProviderSchema: &providerSchema, + }, }, } }