From 334c6f1c2c43f79d1bf3a6aa9cb8f1121e91bd00 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 20 Sep 2018 12:30:52 -0700 Subject: [PATCH] core: Be more explicit in how we handle create_before_destroy Previously our handling of create_before_destroy -- and of deposed objects in particular -- was rather "implicit" and spread over various different subsystems. We'd quietly just destroy every deposed object during a destroy operation, without any user-visible plan to do so. Here we make things more explicit by tracking each deposed object individually by its pseudorandomly-allocated key. There are two different mechanisms at play here, building on the same concepts: - During a replace operation with create_before_destroy, we *pre-allocate* a DeposedKey to use for the prior object in the "apply" node and then pass that exact id to the destroy node, ensuring that we only destroy the single object we planned to destroy. In the happy path here the user never actually sees the allocated deposed key because we use it and then immediately destroy it within the same operation. However, that destroy may fail, which brings us to the second mechanism: - If any deposed objects are already present in state during _plan_, we insert a destroy change for them into the plan so that it's explicit to the user that we are going to destroy these additional objects, and then create an individual graph node for each one in DiffTransformer. The main motivation here is to be more careful in how we handle these destroys so that from a user's standpoint we never destroy something without the user knowing about it ahead of time. However, this new organization also hopefully makes the code itself a little easier to follow because the connection between the create and destroy steps of a Replace is reprseented in a single place (in DiffTransformer) and deposed instances each have their own explicit graph node rather than being secretly handled as part of the main instance-level graph node. --- states/module.go | 4 +- states/resource.go | 22 +- states/state.go | 6 + states/sync.go | 23 +- terraform/context_apply_test.go | 21 +- terraform/eval_apply.go | 4 + terraform/eval_state.go | 14 +- terraform/graph_builder_apply.go | 2 + terraform/graph_builder_apply_test.go | 33 +- terraform/graph_builder_destroy_plan.go | 11 +- terraform/graph_builder_plan.go | 16 + terraform/graph_builder_refresh.go | 17 + terraform/node_resource_apply_instance.go | 3 + terraform/node_resource_destroy.go | 41 +-- terraform/node_resource_destroy_deposed.go | 297 ++++++++++++++++++ terraform/node_resource_destroy_test.go | 68 ---- .../apply-depends-create-before/main.tf | 10 +- terraform/transform_deposed.go | 190 ----------- terraform/transform_diff.go | 108 ++++++- terraform/transform_provisioner_test.go | 4 +- terraform/transform_state.go | 44 ++- 21 files changed, 590 insertions(+), 348 deletions(-) create mode 100644 terraform/node_resource_destroy_deposed.go delete mode 100644 terraform/node_resource_destroy_test.go delete mode 100644 terraform/transform_deposed.go diff --git a/states/module.go b/states/module.go index b6727b75d..ea5093754 100644 --- a/states/module.go +++ b/states/module.go @@ -173,12 +173,12 @@ func (ms *Module) ForgetResourceInstanceDeposed(addr addrs.ResourceInstance, key // deposeResourceInstanceObject is the real implementation of // SyncState.DeposeResourceInstanceObject. -func (ms *Module) deposeResourceInstanceObject(addr addrs.ResourceInstance) DeposedKey { +func (ms *Module) deposeResourceInstanceObject(addr addrs.ResourceInstance, forceKey DeposedKey) DeposedKey { is := ms.ResourceInstance(addr) if is == nil { return NotDeposed } - return is.deposeCurrentObject() + return is.deposeCurrentObject(forceKey) } // SetOutputValue writes an output value into the state, overwriting any diff --git a/states/resource.go b/states/resource.go index 2f0f48ac1..e2a2b8588 100644 --- a/states/resource.go +++ b/states/resource.go @@ -104,12 +104,19 @@ func (i *ResourceInstance) HasObjects() bool { // SyncState.DeposeResourceInstanceObject. The exported method uses a lock // to ensure that we can safely allocate an unused deposed key without // collision. -func (i *ResourceInstance) deposeCurrentObject() DeposedKey { +func (i *ResourceInstance) deposeCurrentObject(forceKey DeposedKey) DeposedKey { if !i.HasCurrent() { return NotDeposed } - key := i.findUnusedDeposedKey() + key := forceKey + if key == NotDeposed { + key = i.findUnusedDeposedKey() + } else { + if _, exists := i.Deposed[key]; exists { + panic(fmt.Sprintf("forced key %s is already in use", forceKey)) + } + } i.Deposed[key] = i.Current i.Current = nil return key @@ -134,6 +141,17 @@ func (i *ResourceInstance) GetGeneration(gen Generation) *ResourceInstanceObject panic(fmt.Sprintf("get invalid Generation %#v", gen)) } +// FindUnusedDeposedKey generates a unique DeposedKey that is guaranteed not to +// already be in use for this instance at the time of the call. +// +// Note that the validity of this result may change if new deposed keys are +// allocated before it is used. To avoid this risk, instead use the +// DeposeResourceInstanceObject method on the SyncState wrapper type, which +// allocates a key and uses it atomically. +func (i *ResourceInstance) FindUnusedDeposedKey() DeposedKey { + return i.findUnusedDeposedKey() +} + // findUnusedDeposedKey generates a unique DeposedKey that is guaranteed not to // already be in use for this instance. func (i *ResourceInstance) findUnusedDeposedKey() DeposedKey { diff --git a/states/state.go b/states/state.go index 7a8d72987..67dfe891d 100644 --- a/states/state.go +++ b/states/state.go @@ -61,6 +61,9 @@ func (s *State) Empty() bool { // Module returns the state for the module with the given address, or nil if // the requested module is not tracked in the state. func (s *State) Module(addr addrs.ModuleInstance) *Module { + if s == nil { + panic("State.Module on nil *State") + } return s.Modules[addr.String()] } @@ -127,6 +130,9 @@ func (s *State) Resource(addr addrs.AbsResource) *Resource { // ResourceInstance returns the state for the resource instance with the given // address, or nil if no such resource is tracked in the state. func (s *State) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInstance { + if s == nil { + panic("State.ResourceInstance on nil *State") + } ms := s.Module(addr.Module) if ms == nil { return nil diff --git a/states/sync.go b/states/sync.go index 9f36c42c3..00f2fd317 100644 --- a/states/sync.go +++ b/states/sync.go @@ -323,7 +323,28 @@ func (s *SyncState) DeposeResourceInstanceObject(addr addrs.AbsResourceInstance) return NotDeposed } - return ms.deposeResourceInstanceObject(addr.Resource) + return ms.deposeResourceInstanceObject(addr.Resource, NotDeposed) +} + +// DeposeResourceInstanceObjectForceKey is like DeposeResourceInstanceObject +// but uses a pre-allocated key. It's the caller's responsibility to ensure +// that there aren't any races to use a particular key; this method will panic +// if the given key is already in use. +func (s *SyncState) DeposeResourceInstanceObjectForceKey(addr addrs.AbsResourceInstance, forcedKey DeposedKey) { + s.lock.Lock() + defer s.lock.Unlock() + + if forcedKey == NotDeposed { + // Usage error: should use DeposeResourceInstanceObject in this case + panic("DeposeResourceInstanceObjectForceKey called without forced key") + } + + ms := s.state.Module(addr.Module) + if ms == nil { + return // Nothing to do, since there can't be any current object either. + } + + ms.deposeResourceInstanceObject(addr.Resource, forcedKey) } // ForgetResourceInstanceDeposed removes the record of the deposed object with diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 8bf1e8570..dae07dc68 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8375,32 +8375,35 @@ func TestContext2Apply_createBefore_depends(t *testing.T) { }) if p, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("diags: %s", diags.Err()) + logDiagnostics(t, diags) + t.Fatal("plan failed") } else { - t.Logf("plan: %s", legacyDiffComparisonString(p.Changes)) + t.Logf("plan:\n%s", legacyDiffComparisonString(p.Changes)) } h.Active = true state, diags := ctx.Apply() if diags.HasErrors() { - t.Fatalf("diags: %s", diags.Err()) + logDiagnostics(t, diags) + t.Fatal("apply failed") } mod := state.RootModule() if len(mod.Resources) < 2 { - t.Fatalf("bad: %#v", mod.Resources) + t.Logf("state after apply:\n%s", state.String()) + t.Fatalf("only %d resources in root module; want at least 2", len(mod.Resources)) } - actual := strings.TrimSpace(state.String()) - expected := strings.TrimSpace(testTerraformApplyDependsCreateBeforeStr) - if actual != expected { - t.Fatalf("bad: \n%s\n\n%s", actual, expected) + got := strings.TrimSpace(state.String()) + want := strings.TrimSpace(testTerraformApplyDependsCreateBeforeStr) + if got != want { + t.Fatalf("wrong final state\ngot:\n%s\n\nwant:\n%s", got, want) } // Test that things were managed _in the right order_ order := h.States diffs := h.Diffs - if order[0].GetAttr("id").AsString() != "" || diffs[0].Action == plans.Delete { + if !order[0].IsNull() || diffs[0].Action == plans.Delete { t.Fatalf("should create new instance first: %#v", order) } diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 756e0e5da..4be930bf8 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -224,6 +224,10 @@ func (n *EvalApplyPre) Eval(ctx EvalContext) (interface{}, error) { change := *n.Change absAddr := n.Addr.Absolute(ctx.Path()) + if change == nil { + panic(fmt.Sprintf("EvalApplyPre for %s called with nil Change", absAddr)) + } + if resourceHasUserVisibleApply(n.Addr) { priorState := change.Before plannedNewState := change.After diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 4d2f70cfd..1573f3d38 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -310,6 +310,12 @@ func (n *EvalWriteStateDeposed) Eval(ctx EvalContext) (interface{}, error) { type EvalDeposeState struct { Addr addrs.ResourceInstance + // ForceKey, if a value other than states.NotDeposed, will be used as the + // key for the newly-created deposed object that results from this action. + // If set to states.NotDeposed (the zero value), a new unique key will be + // allocated. + ForceKey states.DeposedKey + // OutputKey, if non-nil, will be written with the deposed object key that // was generated for the object. This can then be passed to // EvalUndeposeState.Key so it knows which deposed instance to forget. @@ -321,7 +327,13 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) { absAddr := n.Addr.Absolute(ctx.Path()) state := ctx.State() - key := state.DeposeResourceInstanceObject(absAddr) + var key states.DeposedKey + if n.ForceKey == states.NotDeposed { + key = state.DeposeResourceInstanceObject(absAddr) + } else { + key = n.ForceKey + state.DeposeResourceInstanceObjectForceKey(absAddr, key) + } log.Printf("[TRACE] EvalDeposeState: prior object for %s now deposed with key %s", absAddr, key) if n.OutputKey != nil { diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index ede1ec3e2..0db65fe62 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -95,6 +95,8 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // ConfigTransformer above. &DiffTransformer{ Concrete: concreteResourceInstance, + Config: b.Config, + State: b.State, Changes: b.Changes, }, diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 78f914bb9..413be7213 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -1,12 +1,13 @@ package terraform import ( + "fmt" "strings" "testing" - "github.com/hashicorp/terraform/plans" - "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/states" ) func TestApplyGraphBuilder_impl(t *testing.T) { @@ -105,20 +106,42 @@ func TestApplyGraphBuilder_depCbd(t *testing.T) { t.Fatalf("wrong path %q", g.Path.String()) } + // We're going to go hunting for our deposed instance node here, so we + // can find out its key to use in the assertions below. + var dk states.DeposedKey + for _, v := range g.Vertices() { + tv, ok := v.(*NodeDestroyDeposedResourceInstanceObject) + if !ok { + continue + } + if dk != states.NotDeposed { + t.Fatalf("more than one deposed instance node in the graph; want only one") + } + dk = tv.DeposedKey + } + if dk == states.NotDeposed { + t.Fatalf("no deposed instance node in the graph; want one") + } + + destroyName := fmt.Sprintf("test_object.A (deposed %s)", dk) + // Create A, Modify B, Destroy A testGraphHappensBefore( t, g, "test_object.A", - "test_object.A (destroy)") + destroyName, + ) testGraphHappensBefore( t, g, "test_object.A", - "test_object.B") + "test_object.B", + ) testGraphHappensBefore( t, g, "test_object.B", - "test_object.A (destroy)") + destroyName, + ) } // This tests the ordering of two resources that are both CBD that diff --git a/terraform/graph_builder_destroy_plan.go b/terraform/graph_builder_destroy_plan.go index 464e4d2fe..e86af161a 100644 --- a/terraform/graph_builder_destroy_plan.go +++ b/terraform/graph_builder_destroy_plan.go @@ -51,6 +51,12 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer { NodeAbstractResourceInstance: a, } } + concreteResourceInstanceDeposed := func(a *NodeAbstractResourceInstance, key states.DeposedKey) dag.Vertex { + return &NodePlanDeposedResourceInstanceObject{ + NodeAbstractResourceInstance: a, + DeposedKey: key, + } + } concreteProvider := func(a *NodeAbstractProvider) dag.Vertex { return &NodeApplyableProvider{ @@ -61,8 +67,9 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer { steps := []GraphTransformer{ // Creates nodes for the resource instances tracked in the state. &StateTransformer{ - Concrete: concreteResourceInstance, - State: b.State, + ConcreteCurrent: concreteResourceInstance, + ConcreteDeposed: concreteResourceInstanceDeposed, + State: b.State, }, // Attach the configuration to any resources diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index 881fa5677..7b9cda1c4 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -69,6 +69,13 @@ func (b *PlanGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Dia func (b *PlanGraphBuilder) Steps() []GraphTransformer { b.once.Do(b.init) + concreteResourceInstanceDeposed := func(a *NodeAbstractResourceInstance, key states.DeposedKey) dag.Vertex { + return &NodePlanDeposedResourceInstanceObject{ + NodeAbstractResourceInstance: a, + DeposedKey: key, + } + } + steps := []GraphTransformer{ // Creates all the resources represented in the config &ConfigTransformer{ @@ -89,6 +96,15 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { Config: b.Config, }, + // We also need nodes for any deposed instance objects present in the + // state, so we can plan to destroy them. (This intentionally + // skips creating nodes for _current_ objects, since ConfigTransformer + // created nodes that will do that during DynamicExpand.) + &StateTransformer{ + ConcreteDeposed: concreteResourceInstanceDeposed, + State: b.State, + }, + // Create orphan output nodes &OrphanOutputTransformer{ Config: b.Config, diff --git a/terraform/graph_builder_refresh.go b/terraform/graph_builder_refresh.go index 84975314f..a3b3de4f8 100644 --- a/terraform/graph_builder_refresh.go +++ b/terraform/graph_builder_refresh.go @@ -78,6 +78,14 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { } } + concreteResourceInstanceDeposed := func(a *NodeAbstractResourceInstance, key states.DeposedKey) dag.Vertex { + // The "Plan" node type also handles refreshing behavior. + return &NodePlanDeposedResourceInstanceObject{ + NodeAbstractResourceInstance: a, + DeposedKey: key, + } + } + concreteDataResource := func(a *NodeAbstractResource) dag.Vertex { return &NodeRefreshableDataResource{ NodeAbstractResource: a, @@ -121,6 +129,15 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { Config: b.Config, }, + // We also need nodes for any deposed instance objects present in the + // state, so we can check if they still exist. (This intentionally + // skips creating nodes for _current_ objects, since ConfigTransformer + // created nodes that will do that during DynamicExpand.) + &StateTransformer{ + ConcreteDeposed: concreteResourceInstanceDeposed, + State: b.State, + }, + // Attach the state &AttachStateTransformer{State: b.State}, diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 402594fb6..7b4123c23 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -23,6 +23,7 @@ type NodeApplyableResourceInstance struct { *NodeAbstractResourceInstance destroyNode GraphNodeDestroyerCBD + graphNodeDeposer // implementation of GraphNodeDeposer } var ( @@ -30,6 +31,7 @@ var ( _ GraphNodeResourceInstance = (*NodeApplyableResourceInstance)(nil) _ GraphNodeCreator = (*NodeApplyableResourceInstance)(nil) _ GraphNodeReferencer = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeDeposer = (*NodeApplyableResourceInstance)(nil) _ GraphNodeEvalable = (*NodeApplyableResourceInstance)(nil) ) @@ -248,6 +250,7 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe }, Then: &EvalDeposeState{ Addr: addr.Resource, + ForceKey: n.PreallocatedDeposedKey, OutputKey: &deposedKey, }, }, diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index fec49da44..ede2ba217 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -37,6 +37,9 @@ var ( ) func (n *NodeDestroyResourceInstance) Name() string { + if n.DeposedKey != states.NotDeposed { + return fmt.Sprintf("%s (destroy deposed %s)", n.ResourceInstanceAddr(), n.DeposedKey) + } return n.ResourceInstanceAddr().String() + " (destroy)" } @@ -115,44 +118,6 @@ func (n *NodeDestroyResourceInstance) References() []*addrs.Reference { return nil } -// GraphNodeDynamicExpandable -func (n *NodeDestroyResourceInstance) DynamicExpand(ctx EvalContext) (*Graph, error) { - if n.DeposedKey != states.NotDeposed { - return nil, fmt.Errorf("NodeDestroyResourceInstance not yet updated to deal with explicit DeposedKey") - } - - // Our graph transformers require direct access to read the entire state - // structure, so we'll lock the whole state for the duration of this work. - state := ctx.State().Lock() - defer ctx.State().Unlock() - - // Start creating the steps - steps := make([]GraphTransformer, 0, 5) - - // We want deposed resources in the state to be destroyed - steps = append(steps, &DeposedTransformer{ - State: state, - InstanceAddr: n.ResourceInstanceAddr(), - ResolvedProvider: n.ResolvedProvider, - }) - - // Target - steps = append(steps, &TargetsTransformer{ - Targets: n.Targets, - }) - - // Always end with the root being added - steps = append(steps, &RootTransformer{}) - - // Build the graph - b := &BasicGraphBuilder{ - Steps: steps, - Name: "NodeResourceDestroy", - } - g, diags := b.Build(ctx.Path()) - return g, diags.ErrWithWarnings() -} - // GraphNodeEvalable func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { addr := n.ResourceInstanceAddr() diff --git a/terraform/node_resource_destroy_deposed.go b/terraform/node_resource_destroy_deposed.go new file mode 100644 index 000000000..c85657975 --- /dev/null +++ b/terraform/node_resource_destroy_deposed.go @@ -0,0 +1,297 @@ +package terraform + +import ( + "fmt" + + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/dag" + "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/providers" + "github.com/hashicorp/terraform/states" +) + +// ConcreteResourceInstanceDeposedNodeFunc is a callback type used to convert +// an abstract resource instance to a concrete one of some type that has +// an associated deposed object key. +type ConcreteResourceInstanceDeposedNodeFunc func(*NodeAbstractResourceInstance, states.DeposedKey) dag.Vertex + +// NodePlanDeposedResourceInstanceObject represents deposed resource +// instance objects during plan. These are distinct from the primary object +// for each resource instance since the only valid operation to do with them +// is to destroy them. +// +// This node type is also used during the refresh walk to ensure that the +// record of a deposed object is up-to-date before we plan to destroy it. +type NodePlanDeposedResourceInstanceObject struct { + *NodeAbstractResourceInstance + DeposedKey states.DeposedKey +} + +var ( + _ GraphNodeResource = (*NodePlanDeposedResourceInstanceObject)(nil) + _ GraphNodeResourceInstance = (*NodePlanDeposedResourceInstanceObject)(nil) + _ GraphNodeReferenceable = (*NodePlanDeposedResourceInstanceObject)(nil) + _ GraphNodeReferencer = (*NodePlanDeposedResourceInstanceObject)(nil) + _ GraphNodeEvalable = (*NodePlanDeposedResourceInstanceObject)(nil) + _ GraphNodeProviderConsumer = (*NodePlanDeposedResourceInstanceObject)(nil) + _ GraphNodeProvisionerConsumer = (*NodePlanDeposedResourceInstanceObject)(nil) +) + +func (n *NodePlanDeposedResourceInstanceObject) Name() string { + return fmt.Sprintf("%s (deposed %s)", n.Addr.String(), n.DeposedKey) +} + +// GraphNodeReferenceable implementation, overriding the one from NodeAbstractResourceInstance +func (n *NodePlanDeposedResourceInstanceObject) ReferenceableAddrs() []addrs.Referenceable { + // Deposed objects don't participate in references. + return nil +} + +// GraphNodeReferencer implementation, overriding the one from NodeAbstractResourceInstance +func (n *NodePlanDeposedResourceInstanceObject) References() []*addrs.Reference { + // We don't evaluate configuration for deposed objects, so they effectively + // make no references. + return nil +} + +// GraphNodeEvalable impl. +func (n *NodePlanDeposedResourceInstanceObject) EvalTree() EvalNode { + addr := n.ResourceInstanceAddr() + + var provider providers.Interface + var providerSchema *ProviderSchema + var state *states.ResourceInstanceObject + + seq := &EvalSequence{Nodes: make([]EvalNode, 0, 5)} + + // During the refresh walk we will ensure that our record of the deposed + // object is up-to-date. If it was already deleted outside of Terraform + // then this will remove it from state and thus avoid us planning a + // destroy for it during the subsequent plan walk. + seq.Nodes = append(seq.Nodes, &EvalOpFilter{ + Ops: []walkOperation{walkRefresh}, + Node: &EvalSequence{ + Nodes: []EvalNode{ + &EvalGetProvider{ + Addr: n.ResolvedProvider, + Output: &provider, + Schema: &providerSchema, + }, + &EvalReadStateDeposed{ + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Key: n.DeposedKey, + Output: &state, + }, + &EvalRefresh{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + Provider: &provider, + ProviderSchema: &providerSchema, + State: &state, + Output: &state, + }, + &EvalWriteStateDeposed{ + Addr: addr.Resource, + Key: n.DeposedKey, + ProviderAddr: n.ResolvedProvider, + ProviderSchema: &providerSchema, + State: &state, + }, + }, + }, + }) + + // During the plan walk we always produce a planned destroy change, because + // destroying is the only supported action for deposed objects. + var change *plans.ResourceInstanceChange + seq.Nodes = append(seq.Nodes, &EvalOpFilter{ + Ops: []walkOperation{walkPlan, walkPlanDestroy}, + Node: &EvalSequence{ + Nodes: []EvalNode{ + &EvalGetProvider{ + Addr: n.ResolvedProvider, + Output: &provider, + Schema: &providerSchema, + }, + &EvalReadStateDeposed{ + Addr: addr.Resource, + Output: &state, + Key: n.DeposedKey, + Provider: &provider, + ProviderSchema: &providerSchema, + }, + &EvalDiffDestroy{ + Addr: addr.Resource, + DeposedKey: n.DeposedKey, + State: &state, + Output: &change, + }, + &EvalWriteDiff{ + Addr: addr.Resource, + DeposedKey: n.DeposedKey, + ProviderSchema: &providerSchema, + Change: &change, + }, + // Since deposed objects cannot be referenced by expressions + // elsewhere, we don't need to also record the planned new + // state in this case. + }, + }, + }) + + return seq +} + + +// NodeDestroyDeposedResourceInstanceObject represents deposed resource +// instance objects during apply. Nodes of this type are inserted by +// DiffTransformer when the planned changeset contains "delete" changes for +// deposed instance objects, and its only supported operation is to destroy +// and then forget the associated object. +type NodeDestroyDeposedResourceInstanceObject struct { + *NodeAbstractResourceInstance + DeposedKey states.DeposedKey +} + +var ( + _ GraphNodeResource = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeResourceInstance = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeDestroyer = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeDestroyerCBD = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeReferenceable = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeReferencer = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeEvalable = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeProviderConsumer = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeProvisionerConsumer = (*NodeDestroyDeposedResourceInstanceObject)(nil) +) + +func (n *NodeDestroyDeposedResourceInstanceObject) Name() string { + return fmt.Sprintf("%s (destroy deposed %s)", n.Addr.String(), n.DeposedKey) +} + +// GraphNodeReferenceable implementation, overriding the one from NodeAbstractResourceInstance +func (n *NodeDestroyDeposedResourceInstanceObject) ReferenceableAddrs() []addrs.Referenceable { + // Deposed objects don't participate in references. + return nil +} + +// GraphNodeReferencer implementation, overriding the one from NodeAbstractResourceInstance +func (n *NodeDestroyDeposedResourceInstanceObject) References() []*addrs.Reference { + // We don't evaluate configuration for deposed objects, so they effectively + // make no references. + return nil +} + +// GraphNodeDestroyer +func (n *NodeDestroyDeposedResourceInstanceObject) DestroyAddr() *addrs.AbsResourceInstance { + addr := n.ResourceInstanceAddr() + return &addr +} + +// GraphNodeDestroyerCBD +func (n *NodeDestroyDeposedResourceInstanceObject) CreateBeforeDestroy() bool { + // A deposed instance is always CreateBeforeDestroy by definition, since + // we use deposed only to handle create-before-destroy. + return true +} + +// GraphNodeDestroyerCBD +func (n *NodeDestroyDeposedResourceInstanceObject) ModifyCreateBeforeDestroy(v bool) error { + if !v { + // Should never happen: deposed instances are _always_ create_before_destroy. + return fmt.Errorf("can't deactivate create_before_destroy for a deposed instance") + } + return nil +} + +// GraphNodeEvalable impl. +func (n *NodeDestroyDeposedResourceInstanceObject) EvalTree() EvalNode { + addr := n.ResourceInstanceAddr() + + var provider providers.Interface + var providerSchema *ProviderSchema + var state *states.ResourceInstanceObject + var change *plans.ResourceInstanceChange + var err error + + return &EvalSequence{ + Nodes: []EvalNode{ + &EvalGetProvider{ + Addr: n.ResolvedProvider, + Output: &provider, + Schema: &providerSchema, + }, + &EvalReadStateDeposed{ + Addr: addr.Resource, + Output: &state, + Key: n.DeposedKey, + Provider: &provider, + ProviderSchema: &providerSchema, + }, + &EvalDiffDestroy{ + Addr: addr.Resource, + State: &state, + Output: &change, + }, + // Call pre-apply hook + &EvalApplyPre{ + Addr: addr.Resource, + State: &state, + Change: &change, + }, + &EvalApply{ + Addr: addr.Resource, + Config: nil, // No configuration because we are destroying + State: &state, + Change: &change, + Provider: &provider, + ProviderAddr: n.ResolvedProvider, + ProviderSchema: &providerSchema, + Output: &state, + Error: &err, + }, + // Always write the resource back to the state deposed... if it + // was successfully destroyed it will be pruned. If it was not, it will + // be caught on the next run. + &EvalWriteStateDeposed{ + Addr: addr.Resource, + Key: n.DeposedKey, + ProviderAddr: n.ResolvedProvider, + ProviderSchema: &providerSchema, + State: &state, + }, + &EvalApplyPost{ + Addr: addr.Resource, + State: &state, + Error: &err, + }, + &EvalReturnError{ + Error: &err, + }, + &EvalUpdateStateHook{}, + }, + } +} + +// GraphNodeDeposer is an optional interface implemented by graph nodes that +// might create a single new deposed object for a specific associated resource +// instance, allowing a caller to optionally pre-allocate a DeposedKey for +// it. +type GraphNodeDeposer interface { + // SetPreallocatedDeposedKey will be called during graph construction + // if a particular node must use a pre-allocated deposed key if/when it + // "deposes" the current object of its associated resource instance. + SetPreallocatedDeposedKey(key states.DeposedKey) +} + +// graphNodeDeposer is an embeddable implementation of GraphNodeDeposer. +// Embed it in a node type to get automatic support for it, and then access +// the field PreallocatedDeposedKey to access any pre-allocated key. +type graphNodeDeposer struct { + PreallocatedDeposedKey states.DeposedKey +} + +func (n *graphNodeDeposer) SetPreallocatedDeposedKey(key states.DeposedKey) { + n.PreallocatedDeposedKey = key +} diff --git a/terraform/node_resource_destroy_test.go b/terraform/node_resource_destroy_test.go deleted file mode 100644 index 25b363d62..000000000 --- a/terraform/node_resource_destroy_test.go +++ /dev/null @@ -1,68 +0,0 @@ -package terraform - -import ( - "strings" - "testing" - - "github.com/hashicorp/terraform/addrs" -) - -func TestNodeDestroyResourceDynamicExpand_deposedCount(t *testing.T) { - state := mustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.bar.0": &ResourceState{ - Type: "aws_instance", - Deposed: []*InstanceState{ - &InstanceState{ - ID: "foo", - }, - }, - Provider: "provider.aws", - }, - "aws_instance.bar.1": &ResourceState{ - Type: "aws_instance", - Deposed: []*InstanceState{ - &InstanceState{ - ID: "bar", - }, - }, - Provider: "provider.aws", - }, - }, - }, - }, - }) - - m := testModule(t, "apply-cbd-count") - n := &NodeDestroyResourceInstance{ - NodeAbstractResourceInstance: &NodeAbstractResourceInstance{ - NodeAbstractResource: NodeAbstractResource{ - Addr: addrs.RootModuleInstance.Resource( - addrs.ManagedResourceMode, "aws_instance", "bar", - ), - Config: m.Module.ManagedResources["aws_instance.bar"], - }, - InstanceKey: addrs.IntKey(0), - ResourceState: state.Modules[""].Resources["aws_instance.bar[0]"], - }, - } - - g, err := n.DynamicExpand(&MockEvalContext{ - PathPath: addrs.RootModuleInstance, - StateState: state.SyncWrapper(), - }) - if err != nil { - t.Fatalf("err: %s", err) - } - - got := strings.TrimSpace(g.String()) - want := strings.TrimSpace(` -aws_instance.bar[0] (deposed 00000001) -`) - if got != want { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", got, want) - } -} diff --git a/terraform/test-fixtures/apply-depends-create-before/main.tf b/terraform/test-fixtures/apply-depends-create-before/main.tf index 89436a000..63478d893 100644 --- a/terraform/test-fixtures/apply-depends-create-before/main.tf +++ b/terraform/test-fixtures/apply-depends-create-before/main.tf @@ -1,10 +1,10 @@ resource "aws_instance" "web" { - require_new = "ami-new" - lifecycle { - create_before_destroy = true - } + require_new = "ami-new" + lifecycle { + create_before_destroy = true + } } resource "aws_instance" "lb" { - instance = "${aws_instance.web.id}" + instance = aws_instance.web.id } diff --git a/terraform/transform_deposed.go b/terraform/transform_deposed.go deleted file mode 100644 index 681e4d8eb..000000000 --- a/terraform/transform_deposed.go +++ /dev/null @@ -1,190 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/plans" - "github.com/hashicorp/terraform/providers" - "github.com/hashicorp/terraform/states" -) - -// DeposedTransformer is a GraphTransformer that adds nodes to the graph for -// the deposed objects associated with a given resource instance. -type DeposedTransformer struct { - // State is the global state, from which we'll retrieve the state for - // the instance given in InstanceAddr. - State *states.State - - // InstanceAddr is the address of the instance whose deposed objects will - // have graph nodes created. - InstanceAddr addrs.AbsResourceInstance - - // The provider used by the resourced which were deposed - ResolvedProvider addrs.AbsProviderConfig -} - -func (t *DeposedTransformer) Transform(g *Graph) error { - rs := t.State.Resource(t.InstanceAddr.ContainingResource()) - if rs == nil { - // If the resource has no state then there can't be deposed objects. - return nil - } - is := rs.Instances[t.InstanceAddr.Resource.Key] - if is == nil { - // If the instance has no state then there can't be deposed objects. - return nil - } - - providerAddr := rs.ProviderConfig - - for k := range is.Deposed { - g.Add(&graphNodeDeposedResource{ - Addr: t.InstanceAddr, - DeposedKey: k, - RecordedProvider: providerAddr, - ResolvedProvider: t.ResolvedProvider, - }) - } - - return nil -} - -// graphNodeDeposedResource is the graph vertex representing a deposed resource. -type graphNodeDeposedResource struct { - Addr addrs.AbsResourceInstance - DeposedKey states.DeposedKey - RecordedProvider addrs.AbsProviderConfig - ResolvedProvider addrs.AbsProviderConfig -} - -var ( - _ GraphNodeProviderConsumer = (*graphNodeDeposedResource)(nil) - _ GraphNodeEvalable = (*graphNodeDeposedResource)(nil) -) - -func (n *graphNodeDeposedResource) Name() string { - return fmt.Sprintf("%s (deposed %s)", n.Addr.String(), n.DeposedKey) -} - -func (n *graphNodeDeposedResource) ProvidedBy() (addrs.AbsProviderConfig, bool) { - return n.RecordedProvider, true -} - -func (n *graphNodeDeposedResource) SetProvider(addr addrs.AbsProviderConfig) { - // Because our ProvidedBy returns exact=true, this is actually rather - // pointless and should always just be the address we asked for. - n.RecordedProvider = addr -} - -// GraphNodeEvalable impl. -func (n *graphNodeDeposedResource) EvalTree() EvalNode { - addr := n.Addr - - var provider providers.Interface - var providerSchema *ProviderSchema - var state *states.ResourceInstanceObject - - seq := &EvalSequence{Nodes: make([]EvalNode, 0, 5)} - - // Refresh the resource - seq.Nodes = append(seq.Nodes, &EvalOpFilter{ - Ops: []walkOperation{walkRefresh}, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalGetProvider{ - Addr: n.ResolvedProvider, - Output: &provider, - Schema: &providerSchema, - }, - &EvalReadStateDeposed{ - Addr: addr.Resource, - ProviderSchema: &providerSchema, - Key: n.DeposedKey, - Output: &state, - }, - &EvalRefresh{ - Addr: addr.Resource, - ProviderAddr: n.ResolvedProvider, - Provider: &provider, - ProviderSchema: &providerSchema, - State: &state, - Output: &state, - }, - &EvalWriteStateDeposed{ - Addr: addr.Resource, - Key: n.DeposedKey, - ProviderAddr: n.ResolvedProvider, - ProviderSchema: &providerSchema, - State: &state, - }, - }, - }, - }) - - // Apply - var change *plans.ResourceInstanceChange - var err error - seq.Nodes = append(seq.Nodes, &EvalOpFilter{ - Ops: []walkOperation{walkApply, walkDestroy}, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalGetProvider{ - Addr: n.ResolvedProvider, - Output: &provider, - Schema: &providerSchema, - }, - &EvalReadStateDeposed{ - Addr: addr.Resource, - Output: &state, - Key: n.DeposedKey, - Provider: &provider, - ProviderSchema: &providerSchema, - }, - &EvalDiffDestroy{ - Addr: addr.Resource, - State: &state, - Output: &change, - }, - // Call pre-apply hook - &EvalApplyPre{ - Addr: addr.Resource, - State: &state, - Change: &change, - }, - &EvalApply{ - Addr: addr.Resource, - Config: nil, // No configuration because we are destroying - State: &state, - Change: &change, - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderSchema: &providerSchema, - Output: &state, - Error: &err, - }, - // Always write the resource back to the state deposed... if it - // was successfully destroyed it will be pruned. If it was not, it will - // be caught on the next run. - &EvalWriteStateDeposed{ - Addr: addr.Resource, - Key: n.DeposedKey, - ProviderAddr: n.ResolvedProvider, - ProviderSchema: &providerSchema, - State: &state, - }, - &EvalApplyPost{ - Addr: addr.Resource, - State: &state, - Error: &err, - }, - &EvalReturnError{ - Error: &err, - }, - &EvalUpdateStateHook{}, - }, - }, - }) - - return seq -} diff --git a/terraform/transform_diff.go b/terraform/transform_diff.go index 4daca54cf..1d991770a 100644 --- a/terraform/transform_diff.go +++ b/terraform/transform_diff.go @@ -4,15 +4,19 @@ import ( "fmt" "log" + "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/states" + "github.com/hashicorp/terraform/tfdiags" ) // DiffTransformer is a GraphTransformer that adds graph nodes representing // each of the resource changes described in the given Changes object. type DiffTransformer struct { Concrete ConcreteResourceInstanceNodeFunc + Config *configs.Config + State *states.State Changes *plans.Changes } @@ -25,6 +29,11 @@ func (t *DiffTransformer) Transform(g *Graph) error { // Go through all the modules in the diff. log.Printf("[TRACE] DiffTransformer starting") + var diags tfdiags.Diagnostics + config := t.Config + state := t.State + changes := t.Changes + // DiffTransformer creates resource _instance_ nodes. If there are any // whole-resource nodes already in the graph, we must ensure that they // get evaluated before any of the corresponding instances by creating @@ -47,14 +56,22 @@ func (t *DiffTransformer) Transform(g *Graph) error { resourceNodes[addr] = append(resourceNodes[addr], rn) } - for _, rc := range t.Changes.Resources { + for _, rc := range changes.Resources { addr := rc.Addr dk := rc.DeposedKey + var rCfg *configs.Resource + + log.Printf("[TRACE] DiffTransformer: found %s change for %s %s", rc.Action, addr, dk) + + modCfg := config.DescendentForInstance(addr.Module) + if modCfg != nil { + rCfg = modCfg.Module.ResourceByAddr(addr.Resource.Resource) + } // Depending on the action we'll need some different combinations of // nodes, because destroying uses a special node type separate from // other actions. - var update, delete bool + var update, delete, createBeforeDestroy bool switch rc.Action { case plans.NoOp: continue @@ -67,6 +84,56 @@ func (t *DiffTransformer) Transform(g *Graph) error { update = true } + if dk != states.NotDeposed && update { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid planned change for deposed object", + fmt.Sprintf("The plan contains a non-delete change for %s deposed object %s. The only valid action for a deposed object is to destroy it, so this is a bug in Terraform.", addr, dk), + )) + continue + } + + if rCfg != nil && rCfg.Managed != nil && rCfg.Managed.CreateBeforeDestroy { + createBeforeDestroy = true + } + + // If we're going to do a create_before_destroy Replace operation then + // we need to allocate a DeposedKey to use to retain the + // not-yet-destroyed prior object, so that the delete node can destroy + // _that_ rather than the newly-created node, which will be current + // by the time the delete node is visited. + if update && delete && createBeforeDestroy { + // In this case, variable dk will be the _pre-assigned_ DeposedKey + // that must be used if the update graph node deposes the current + // instance, which will then align with the same key we pass + // into the destroy node to ensure we destroy exactly the deposed + // object we expect. + if state != nil { + ris := state.ResourceInstance(addr) + if ris == nil { + // Should never happen, since we don't plan to replace an + // instance that doesn't exist yet. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid planned change", + fmt.Sprintf("The plan contains a replace change for %s, which doesn't exist yet. This is a bug in Terraform.", addr), + )) + continue + } + + // Allocating a deposed key separately from using it can be racy + // in general, but we assume here that nothing except the apply + // node we instantiate below will actually make new deposed objects + // in practice, and so the set of already-used keys will not change + // between now and then. + dk = ris.FindUnusedDeposedKey() + } else { + // If we have no state at all yet then we can use _any_ + // DeposedKey. + dk = states.NewDeposedKey() + } + } + if update { // All actions except destroying the node type chosen by t.Concrete abstract := NewNodeAbstractResourceInstance(addr) @@ -75,14 +142,19 @@ func (t *DiffTransformer) Transform(g *Graph) error { node = f(abstract) } - if dk != states.NotDeposed { - // The only valid action for deposed objects is to destroy them. - // Entering this branch suggests a bug in the plan phase that - // proposed this change. - return fmt.Errorf("invalid %s action for deposed object on %s: only Delete is allowed", rc.Action, addr) + if createBeforeDestroy { + // We'll attach our pre-allocated DeposedKey to the node if + // it supports that. NodeApplyableResourceInstance is the + // specific concrete node type we are looking for here really, + // since that's the only node type that might depose objects. + if dn, ok := node.(GraphNodeDeposer); ok { + dn.SetPreallocatedDeposedKey(dk) + } + log.Printf("[TRACE] DiffTransformer: %s will be represented by %s, deposing prior object to %s", addr, dag.VertexName(node), dk) + } else { + log.Printf("[TRACE] DiffTransformer: %s will be represented by %s", addr, dag.VertexName(node)) } - log.Printf("[TRACE] DiffTransformer: %s will be represented by %s", addr, dag.VertexName(node)) g.Add(node) rsrcAddr := addr.ContainingResource().String() for _, rsrcNode := range resourceNodes[rsrcAddr] { @@ -91,11 +163,21 @@ func (t *DiffTransformer) Transform(g *Graph) error { } if delete { - // Destroying always uses this destroy-specific node type. + // Destroying always uses a destroy-specific node type, though + // which one depends on whether we're destroying a current object + // or a deposed object. + var node GraphNodeResourceInstance abstract := NewNodeAbstractResourceInstance(addr) - node := &NodeDestroyResourceInstance{ - NodeAbstractResourceInstance: abstract, - DeposedKey: dk, + if dk == states.NotDeposed { + node = &NodeDestroyResourceInstance{ + NodeAbstractResourceInstance: abstract, + DeposedKey: dk, + } + } else { + node = &NodeDestroyDeposedResourceInstanceObject{ + NodeAbstractResourceInstance: abstract, + DeposedKey: dk, + } } if dk == states.NotDeposed { log.Printf("[TRACE] DiffTransformer: %s will be represented for destruction by %s", addr, dag.VertexName(node)) @@ -117,5 +199,5 @@ func (t *DiffTransformer) Transform(g *Graph) error { log.Printf("[TRACE] DiffTransformer complete") - return nil + return diags.Err() } diff --git a/terraform/transform_provisioner_test.go b/terraform/transform_provisioner_test.go index 1c2c4f772..dd2714f33 100644 --- a/terraform/transform_provisioner_test.go +++ b/terraform/transform_provisioner_test.go @@ -93,8 +93,8 @@ func TestMissingProvisionerTransformer_module(t *testing.T) { }) tf := &StateTransformer{ - Concrete: concreteResource, - State: state, + ConcreteCurrent: concreteResource, + State: state, } if err := tf.Transform(&g); err != nil { t.Fatalf("err: %s", err) diff --git a/terraform/transform_state.go b/terraform/transform_state.go index bd6f0d163..0b52347df 100644 --- a/terraform/transform_state.go +++ b/terraform/transform_state.go @@ -3,7 +3,6 @@ package terraform import ( "log" - "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/states" ) @@ -13,7 +12,15 @@ import ( // This transform is used for example by the DestroyPlanGraphBuilder to ensure // that only resources that are in the state are represented in the graph. type StateTransformer struct { - Concrete ConcreteResourceInstanceNodeFunc + // ConcreteCurrent and ConcreteDeposed are used to specialize the abstract + // resource instance nodes that this transformer will create. + // + // If either of these is nil, the objects of that type will be skipped and + // not added to the graph at all. It doesn't make sense to use this + // transformer without setting at least one of these, since that would + // skip everything and thus be a no-op. + ConcreteCurrent ConcreteResourceInstanceNodeFunc + ConcreteDeposed ConcreteResourceInstanceDeposedNodeFunc State *states.State } @@ -24,24 +31,41 @@ func (t *StateTransformer) Transform(g *Graph) error { return nil } - log.Printf("[TRACE] StateTransformer: starting") + switch { + case t.ConcreteCurrent != nil && t.ConcreteDeposed != nil: + log.Printf("[TRACE] StateTransformer: creating nodes for both current and deposed instance objects") + case t.ConcreteCurrent != nil: + log.Printf("[TRACE] StateTransformer: creating nodes for current instance objects only") + case t.ConcreteDeposed != nil: + log.Printf("[TRACE] StateTransformer: creating nodes for deposed instance objects only") + default: + log.Printf("[TRACE] StateTransformer: pointless no-op call, creating no nodes at all") + } + for _, ms := range t.State.Modules { moduleAddr := ms.Addr for _, rs := range ms.Resources { resourceAddr := rs.Addr.Absolute(moduleAddr) - for key := range rs.Instances { + for key, is := range rs.Instances { addr := resourceAddr.Instance(key) - abstract := NewNodeAbstractResourceInstance(addr) - var node dag.Vertex = abstract - if f := t.Concrete; f != nil { - node = f(abstract) + if obj := is.Current; obj != nil && t.ConcreteCurrent != nil { + abstract := NewNodeAbstractResourceInstance(addr) + node := t.ConcreteCurrent(abstract) + g.Add(node) + log.Printf("[TRACE] StateTransformer: added %T for %s current object", node, addr) } - g.Add(node) - log.Printf("[TRACE] StateTransformer: added %T for %s", node, addr) + if t.ConcreteDeposed != nil { + for dk := range is.Deposed { + abstract := NewNodeAbstractResourceInstance(addr) + node := t.ConcreteDeposed(abstract, dk) + g.Add(node) + log.Printf("[TRACE] StateTransformer: added %T for %s deposed object %s", node, addr, dk) + } + } } } }