From dcd762e81d718014c4fc8a349c3ce232ca4c22cb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 22 Nov 2022 09:11:10 -0500 Subject: [PATCH] evaluate outputs from state Outputs were being evaluated from changes, even during apply. Make sure we update the state correctly, and remove the existing change. This requires adding more Planning fields to the output nodes to differentiate whether the output is being planned or applied because the same type handles both cases. We can evaluate separately whether new types should be introduced to deal with both cases. The module node cleanup was also prematurely removing module outputs from the state before evaluation. This was not noticed before because the evaluation was always falling back to changes. Have the root module node do the final cleanup for all its children. It turns out sensitive was also being handled incorrectly, and only sensitive from configuration was being considered. Make sure to mark the output as sensitive when storing sensitive values into state, and OR sensitive marks with the state config when evaluating the output values. --- internal/terraform/evaluate.go | 11 ++--- internal/terraform/graph_builder_plan.go | 6 ++- internal/terraform/node_module_expand.go | 14 +++---- internal/terraform/node_module_expand_test.go | 12 ++++++ internal/terraform/node_output.go | 40 +++++++++++++++---- internal/terraform/transform_orphan_output.go | 8 ++-- 6 files changed, 66 insertions(+), 25 deletions(-) diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index d521443472..7eeff534cd 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -375,6 +375,11 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc // so we only need to store these by instance key. stateMap := map[addrs.InstanceKey]map[string]cty.Value{} for _, output := range d.Evaluator.State.ModuleOutputs(d.ModulePath, addr) { + val := output.Value + if output.Sensitive { + val = val.Mark(marks.Sensitive) + } + _, callInstance := output.Addr.Module.CallInstance() instance, ok := stateMap[callInstance.Key] if !ok { @@ -382,7 +387,7 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc stateMap[callInstance.Key] = instance } - instance[output.Addr.OutputValue.Name] = output.Value + instance[output.Addr.OutputValue.Name] = val } // Get all changes that reside for this module call within our path. @@ -426,10 +431,6 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc } instance[cfg.Name] = outputState - - if cfg.Sensitive { - instance[cfg.Name] = outputState.Mark(marks.Sensitive) - } } // any pending changes override the state state values diff --git a/internal/terraform/graph_builder_plan.go b/internal/terraform/graph_builder_plan.go index 2c706647b4..f085e9aa2d 100644 --- a/internal/terraform/graph_builder_plan.go +++ b/internal/terraform/graph_builder_plan.go @@ -150,7 +150,11 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { &AttachStateTransformer{State: b.State}, // Create orphan output nodes - &OrphanOutputTransformer{Config: b.Config, State: b.State}, + &OrphanOutputTransformer{ + Config: b.Config, + State: b.State, + Planning: true, + }, // Attach the configuration to any resources &AttachResourceConfigTransformer{Config: b.Config}, diff --git a/internal/terraform/node_module_expand.go b/internal/terraform/node_module_expand.go index 49389ac654..28ec2b4e36 100644 --- a/internal/terraform/node_module_expand.go +++ b/internal/terraform/node_module_expand.go @@ -176,22 +176,20 @@ func (n *nodeCloseModule) Name() string { } func (n *nodeCloseModule) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { - if n.Addr.IsRoot() { - // If this is the root module, we are cleaning up the walk, so close - // any running provisioners - diags = diags.Append(ctx.CloseProvisioners()) + if !n.Addr.IsRoot() { + return } + // If this is the root module, we are cleaning up the walk, so close + // any running provisioners + diags = diags.Append(ctx.CloseProvisioners()) + switch op { case walkApply, walkDestroy: state := ctx.State().Lock() defer ctx.State().Unlock() for modKey, mod := range state.Modules { - if !n.Addr.Equal(mod.Addr.Module()) { - continue - } - // clean out any empty resources for resKey, res := range mod.Resources { if len(res.Instances) == 0 { diff --git a/internal/terraform/node_module_expand_test.go b/internal/terraform/node_module_expand_test.go index 42eb91a6dc..146f754f37 100644 --- a/internal/terraform/node_module_expand_test.go +++ b/internal/terraform/node_module_expand_test.go @@ -47,6 +47,18 @@ func TestNodeCloseModuleExecute(t *testing.T) { t.Fatalf("unexpected error: %s", diags.Err()) } + // Since module.child has no resources, it should be removed + if _, ok := state.Modules["module.child"]; !ok { + t.Fatal("module.child should not be removed from state yet") + } + + // the root module should do all the module cleanup + node = nodeCloseModule{addrs.RootModule} + diags = node.Execute(ctx, walkApply) + if diags.HasErrors() { + t.Fatalf("unexpected error: %s", diags.Err()) + } + // Since module.child has no resources, it should be removed if _, ok := state.Modules["module.child"]; ok { t.Fatal("module.child was not removed from state") diff --git a/internal/terraform/node_output.go b/internal/terraform/node_output.go index eccf780cb2..b7c45734ef 100644 --- a/internal/terraform/node_output.go +++ b/internal/terraform/node_output.go @@ -102,7 +102,8 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { switch { case module.IsRoot() && (n.PlanDestroy || n.ApplyDestroy): node = &NodeDestroyableOutput{ - Addr: absAddr, + Addr: absAddr, + Planning: n.Planning, } case n.PlanDestroy: @@ -116,6 +117,7 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { Change: change, RefreshOnly: n.RefreshOnly, DestroyApply: n.ApplyDestroy, + Planning: n.Planning, } } @@ -203,6 +205,8 @@ type NodeApplyableOutput struct { // DestroyApply indicates that we are applying a destroy plan, and do not // need to account for conditional blocks. DestroyApply bool + + Planning bool } var ( @@ -421,7 +425,8 @@ func (n *NodeApplyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.DotNo // NodeDestroyableOutput represents an output that is "destroyable": // its application will remove the output from the state. type NodeDestroyableOutput struct { - Addr addrs.AbsOutputValue + Addr addrs.AbsOutputValue + Planning bool } var ( @@ -468,7 +473,7 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) tfdia } changes := ctx.Changes() - if changes != nil { + if changes != nil && n.Planning { change := &plans.OutputChange{ Addr: n.Addr, Sensitive: sensitiveBefore, @@ -485,6 +490,7 @@ func (n *NodeDestroyableOutput) Execute(ctx EvalContext, op walkOperation) tfdia panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err)) } log.Printf("[TRACE] NodeDestroyableOutput: Saving %s change for %s in changeset", change.Action, n.Addr) + changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present changes.AppendOutputChange(cs) // add the new planned change } @@ -509,7 +515,7 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C // there and lookup the prior value in the state. This is used in // preference to the state where present, since it *is* able to represent // unknowns, while the state cannot. - if changes != nil { + if changes != nil && n.Planning { // if this is a root module, try to get a before value from the state for // the diff sensitiveBefore := false @@ -575,8 +581,13 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C panic(fmt.Sprintf("planned change for %s could not be encoded: %s", n.Addr, err)) } log.Printf("[TRACE] setValue: Saving %s change for %s in changeset", change.Action, n.Addr) - changes.RemoveOutputChange(n.Addr) // remove any existing planned change, if present - changes.AppendOutputChange(cs) // add the new planned change + changes.AppendOutputChange(cs) // add the new planned change + } + + if changes != nil && !n.Planning { + // During apply there is no longer any change to track, so we must + // ensure the state is updated and not overridden by a change. + changes.RemoveOutputChange(n.Addr) } if val.IsKnown() && !val.IsNull() { @@ -584,9 +595,22 @@ func (n *NodeApplyableOutput) setValue(state *states.SyncState, changes *plans.C // out here and then we'll save the real unknown value in the planned // changeset below, if we have one on this graph walk. log.Printf("[TRACE] setValue: Saving value for %s in state", n.Addr) - unmarkedVal, _ := val.UnmarkDeep() + + sensitive := n.Config.Sensitive + unmarkedVal, valueMarks := val.UnmarkDeep() + + // If the evaluate value contains sensitive marks, the output has no + // choice but to declare itself as "sensitive". + for mark := range valueMarks { + if mark == marks.Sensitive { + sensitive = true + break + } + } + stateVal := cty.UnknownAsNull(unmarkedVal) - state.SetOutputValue(n.Addr, stateVal, n.Config.Sensitive) + state.SetOutputValue(n.Addr, stateVal, sensitive) + } else { log.Printf("[TRACE] setValue: Removing %s from state (it is now null)", n.Addr) state.RemoveOutputValue(n.Addr) diff --git a/internal/terraform/transform_orphan_output.go b/internal/terraform/transform_orphan_output.go index 320c91fbe1..fe3e9ce419 100644 --- a/internal/terraform/transform_orphan_output.go +++ b/internal/terraform/transform_orphan_output.go @@ -12,8 +12,9 @@ import ( // in the given config that are in the state and adds them to the graph // for deletion. type OrphanOutputTransformer struct { - Config *configs.Config // Root of config tree - State *states.State // State is the root state + Config *configs.Config // Root of config tree + State *states.State // State is the root state + Planning bool } func (t *OrphanOutputTransformer) Transform(g *Graph) error { @@ -52,7 +53,8 @@ func (t *OrphanOutputTransformer) transform(g *Graph, ms *states.Module) error { } g.Add(&NodeDestroyableOutput{ - Addr: addrs.OutputValue{Name: name}.Absolute(moduleAddr), + Addr: addrs.OutputValue{Name: name}.Absolute(moduleAddr), + Planning: t.Planning, }) }