From 6bbfbab93ec63b0d4f385c2e2d97b60e2ef9288c Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 30 May 2018 12:01:05 -0700 Subject: [PATCH] core: Produce correct references for destroy nodes Prior to the introduction of our "addrs" package, we represented destroy nodes as a special kind of address string ending in ".destroy" or ".destroy-cbd". Using references to resolve these dependencies is a strange idea to begin with, since these are not user-visible addresses, but rather than refactor that now we instead have these weird pseudo-address types ResourcePhase and ResourceInstancePhase that correspond go those weird address suffixes, thus restoring the prior behavior. In future we should rework this so that destroy node edges are not handled as references at all, and instead handled as part of DestroyEdgeTransformer where there's better context for implementing this logic and it can be maintained and tested in a single place. --- addrs/resource_phase.go | 48 ++++++++++++++++++++++++++++++ terraform/node_resource_apply.go | 6 +--- terraform/node_resource_destroy.go | 31 +++++++++++-------- terraform/transform_reference.go | 9 ++++-- 4 files changed, 74 insertions(+), 20 deletions(-) diff --git a/addrs/resource_phase.go b/addrs/resource_phase.go index 1659d62ff..9bdbdc421 100644 --- a/addrs/resource_phase.go +++ b/addrs/resource_phase.go @@ -30,6 +30,12 @@ func (r ResourceInstance) Phase(rpt ResourceInstancePhaseType) ResourceInstanceP } } +// ContainingResource returns an address for the same phase of the resource +// that this instance belongs to. +func (rp ResourceInstancePhase) ContainingResource() ResourcePhase { + return rp.ResourceInstance.Resource.Phase(rp.Phase) +} + func (rp ResourceInstancePhase) String() string { // We use a different separator here than usual to ensure that we'll // never conflict with any non-phased resource instance string. This @@ -55,3 +61,45 @@ const ( func (rpt ResourceInstancePhaseType) String() string { return string(rpt) } + +// ResourcePhase is a special kind of reference used only internally +// during graph building to represent resources that are in a +// non-primary state. +// +// Graph nodes can declare themselves referenceable via a resource phase +// or can declare that they reference a resource phase in order to accomodate +// secondary graph nodes dealing with, for example, destroy actions. +// +// Since resources (as opposed to instances) aren't actually phased, this +// address type is used only as an approximation during initial construction +// of the resource-oriented plan graph, under the assumption that resource +// instances with ResourceInstancePhase addresses will be created in dynamic +// subgraphs during the graph walk. +// +// This special reference type cannot be accessed directly by end-users, and +// should never be shown in the UI. +type ResourcePhase struct { + referenceable + Resource Resource + Phase ResourceInstancePhaseType +} + +var _ Referenceable = ResourcePhase{} + +// Phase returns a special "phase address" for the receving instance. See the +// documentation of ResourceInstancePhase for the limited situations where this +// is intended to be used. +func (r Resource) Phase(rpt ResourceInstancePhaseType) ResourcePhase { + return ResourcePhase{ + Resource: r, + Phase: rpt, + } +} + +func (rp ResourcePhase) String() string { + // We use a different separator here than usual to ensure that we'll + // never conflict with any non-phased resource instance string. This + // is intentionally something that would fail parsing with ParseRef, + // because this special address type should never be exposed in the UI. + return fmt.Sprintf("%s#%s", rp.Resource, rp.Phase) +} diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index b0d71c715..c72865e9f 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -52,12 +52,8 @@ func (n *NodeApplyableResourceInstance) References() []*addrs.Reference { newRef.Remaining = nil // can't access attributes of something being destroyed ret = append(ret, &newRef) case addrs.Resource: - // We'll guess that this is actually a reference to a no-key - // instance here, and generate a reference under that assumption. - // If that's not true then this won't do any harm, since there - // won't actually be a node with this address. newRef := *ref // shallow copy so we can mutate - newRef.Subject = tr.Instance(addrs.NoKey).Phase(addrs.ResourceInstancePhaseDestroy) + newRef.Subject = tr.Phase(addrs.ResourceInstancePhaseDestroy) newRef.Remaining = nil // can't access attributes of something being destroyed ret = append(ret, &newRef) } diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 40864a03a..2c0af9013 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -55,20 +55,27 @@ func (n *NodeDestroyResourceInstance) ModifyCreateBeforeDestroy(v bool) error { } // GraphNodeReferenceable, overriding NodeAbstractResource -func (n *NodeDestroyResourceInstance) ReferenceableName() []addrs.Referenceable { - relAddr := n.ResourceInstanceAddr().Resource - switch { - case n.CreateBeforeDestroy(): - return []addrs.Referenceable{ - relAddr.ContainingResource(), - relAddr.Phase(addrs.ResourceInstancePhaseDestroyCBD), - } - default: - return []addrs.Referenceable{ - relAddr.ContainingResource(), - relAddr.Phase(addrs.ResourceInstancePhaseDestroy), +func (n *NodeDestroyResourceInstance) ReferenceableAddrs() []addrs.Referenceable { + normalAddrs := n.NodeAbstractResourceInstance.ReferenceableAddrs() + destroyAddrs := make([]addrs.Referenceable, len(normalAddrs)) + + phaseType := addrs.ResourceInstancePhaseDestroy + if n.CreateBeforeDestroy() { + phaseType = addrs.ResourceInstancePhaseDestroyCBD + } + + for i, normalAddr := range normalAddrs { + switch ta := normalAddr.(type) { + case addrs.Resource: + destroyAddrs[i] = ta.Phase(phaseType) + case addrs.ResourceInstance: + destroyAddrs[i] = ta.Phase(phaseType) + default: + destroyAddrs[i] = normalAddr } } + + return destroyAddrs } // GraphNodeReferencer, overriding NodeAbstractResource diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index ff4092e6a..4e8cae06d 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -210,10 +210,13 @@ func (m *ReferenceMap) References(v dag.Vertex) ([]dag.Vertex, []addrs.Reference // might be in a resource-oriented graph rather than an // instance-oriented graph, and so we'll see if we have the // resource itself instead. - if ri, ok := subject.(addrs.ResourceInstance); ok { - subject = ri.Resource - key = m.referenceMapKey(v, subject) + switch ri := subject.(type) { + case addrs.ResourceInstance: + subject = ri.ContainingResource() + case addrs.ResourceInstancePhase: + subject = ri.ContainingResource() } + key = m.referenceMapKey(v, subject) } vertices := m.vertices[key]