From a6cdfad590d5d2bafe928b7096241bef5f527537 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 9 Jan 2020 14:00:45 -0500 Subject: [PATCH 1/2] NodeDestroyResource needs to be referencable The change in #23696 removed the NodeAbstractResource methods from the NodeDestroyResource type, in order to prevent other resource behaviors, like requesting a provider. While this node type is not directly referenced, it was implicitly ordered against the module cleanup by virtue of being a resource node. Since there's no good entry point to test this ordering at the moment, --- terraform/node_resource_destroy.go | 33 +++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index c5042436dd..8dd432f211 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -281,13 +281,34 @@ type NodeDestroyResource struct { } var ( - _ GraphNodeEvalable = (*NodeDestroyResource)(nil) + _ GraphNodeResource = (*NodeDestroyResource)(nil) + _ GraphNodeReferenceable = (*NodeDestroyResource)(nil) + _ GraphNodeReferencer = (*NodeDestroyResource)(nil) + _ GraphNodeEvalable = (*NodeDestroyResource)(nil) ) func (n *NodeDestroyResource) Name() string { return n.NodeAbstractResource.ResourceAddr().String() + " (clean up state)" } +// GraphNodeReferenceable, overriding NodeAbstractResource +func (n *NodeDestroyResource) ReferenceableAddrs() []addrs.Referenceable { + // NodeDestroyResource doesn't participate in references: the graph + // builder that created it should ensure directly that it already depends + // on every other node related to its resource, without relying on + // references. + return nil +} + +// GraphNodeReferencer, overriding NodeAbstractResource +func (n *NodeDestroyResource) References() []*addrs.Reference { + // NodeDestroyResource doesn't participate in references: the graph + // builder that created it should ensure directly that it already depends + // on every other node related to its resource, without relying on + // references. + return nil +} + // GraphNodeEvalable func (n *NodeDestroyResource) EvalTree() EvalNode { // This EvalNode will produce an error if the resource isn't already @@ -298,3 +319,13 @@ func (n *NodeDestroyResource) EvalTree() EvalNode { Addr: n.NodeAbstractResource.ResourceAddr().Resource, } } + +// GraphNodeResource +func (n *NodeDestroyResource) ResourceAddr() addrs.AbsResource { + return n.NodeAbstractResource.ResourceAddr() +} + +// GraphNodeSubpath +func (n *NodeDestroyResource) Path() addrs.ModuleInstance { + return n.NodeAbstractResource.Path() +} From 4aa8a1cece1031c4f0ec3f70a2d4702ab8790c6c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 10 Jan 2020 16:22:40 -0500 Subject: [PATCH 2/2] Add GraphNodeNoProvider to skip adding a providers While the NodeDestroyResource type should not be a GraphNodeProviderConsumer, we're going to avoid uncovering more hidden behavior by explicitly skipping provider creation and connections in the provider transformers. This should be removed when more in-depth testing can be done during a major release cycle. --- terraform/node_resource_destroy.go | 23 ++++++++++++++++++++--- terraform/transform_provider.go | 14 ++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 8dd432f211..0374d83ddb 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -277,7 +277,7 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { // leaving skeleton resource objects in state after their instances have // all been destroyed. type NodeDestroyResource struct { - NodeAbstractResource *NodeAbstractResource + *NodeAbstractResource } var ( @@ -285,10 +285,17 @@ var ( _ GraphNodeReferenceable = (*NodeDestroyResource)(nil) _ GraphNodeReferencer = (*NodeDestroyResource)(nil) _ GraphNodeEvalable = (*NodeDestroyResource)(nil) + + // FIXME: this is here to document that this node is both + // GraphNodeProviderConsumer by virtue of the embedded + // NodeAbstractResource, but that behavior is not desired and we skip it by + // checking for GraphNodeNoProvider. + _ GraphNodeProviderConsumer = (*NodeDestroyResource)(nil) + _ GraphNodeNoProvider = (*NodeDestroyResource)(nil) ) func (n *NodeDestroyResource) Name() string { - return n.NodeAbstractResource.ResourceAddr().String() + " (clean up state)" + return n.ResourceAddr().String() + " (clean up state)" } // GraphNodeReferenceable, overriding NodeAbstractResource @@ -316,7 +323,7 @@ func (n *NodeDestroyResource) EvalTree() EvalNode { // leftover husk of a resource in state after all of the child instances // and their objects were destroyed. return &EvalForgetResourceState{ - Addr: n.NodeAbstractResource.ResourceAddr().Resource, + Addr: n.ResourceAddr().Resource, } } @@ -329,3 +336,13 @@ func (n *NodeDestroyResource) ResourceAddr() addrs.AbsResource { func (n *NodeDestroyResource) Path() addrs.ModuleInstance { return n.NodeAbstractResource.Path() } + +// GraphNodeNoProvider +// FIXME: this should be removed once the node can be separated from the +// Internal NodeAbstractResource behavior. +func (n *NodeDestroyResource) NoProvider() { +} + +type GraphNodeNoProvider interface { + NoProvider() +} diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 8b8dff86fa..8cfb0b5cb5 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -99,6 +99,13 @@ func (t *ProviderTransformer) Transform(g *Graph) error { needConfigured := map[string]addrs.AbsProviderConfig{} for _, v := range g.Vertices() { + // FIXME: fix the type that implements this, so it's not a + // GraphNodeProviderConsumer. + // check if we want to skip connecting this to a provider + if _, ok := v.(GraphNodeNoProvider); ok { + continue + } + // Does the vertex _directly_ use a provider? if pv, ok := v.(GraphNodeProviderConsumer); ok { requested[v] = make(map[string]ProviderRequest) @@ -275,6 +282,13 @@ func (t *MissingProviderTransformer) Transform(g *Graph) error { var err error m := providerVertexMap(g) for _, v := range g.Vertices() { + // FIXME: fix the type that implements this, so it's not a + // GraphNodeProviderConsumer. + // check if we want to skip connecting this to a provider + if _, ok := v.(GraphNodeNoProvider); ok { + continue + } + pv, ok := v.(GraphNodeProviderConsumer) if !ok { continue