diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index b3592a301..bdcfa1a07 100644 --- a/command/apply_destroy_test.go +++ b/command/apply_destroy_test.go @@ -195,7 +195,7 @@ func TestApply_destroyTargeted(t *testing.T) { Mode: addrs.ManagedResourceMode, Type: "test_instance", Name: "foo", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + }.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance), &states.ResourceInstanceObjectSrc{ AttrsJSON: []byte(`{"id":"i-ab123"}`), Status: states.ObjectReady, @@ -212,8 +212,9 @@ func TestApply_destroyTargeted(t *testing.T) { Name: "foo", }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), &states.ResourceInstanceObjectSrc{ - AttrsJSON: []byte(`{"id":"i-abc123"}`), - Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"i-abc123"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_instance.foo")}, + Status: states.ObjectReady, }, addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("test"), diff --git a/configs/parser_config_test.go b/configs/parser_config_test.go index 13d90fed6..7832914c9 100644 --- a/configs/parser_config_test.go +++ b/configs/parser_config_test.go @@ -224,3 +224,62 @@ func TestParserLoadConfigFileWarning(t *testing.T) { }) } } + +// TestParseLoadConfigFileError is a test that verifies files from +// testdata/warning-files produce particular errors. +// +// This test does not verify that reading these files produces the correct +// file element contents in spite of those errors. More detailed assertions +// may be made on some subset of these configuration files in other tests. +func TestParserLoadConfigFileError(t *testing.T) { + files, err := ioutil.ReadDir("testdata/error-files") + if err != nil { + t.Fatal(err) + } + + for _, info := range files { + name := info.Name() + t.Run(name, func(t *testing.T) { + src, err := ioutil.ReadFile(filepath.Join("testdata/error-files", name)) + if err != nil { + t.Fatal(err) + } + + // First we'll scan the file to see what warnings are expected. + // That's declared inside the files themselves by using the + // string "ERROR: " somewhere on each line that is expected + // to produce a warning, followed by the expected warning summary + // text. A single-line comment (with #) is the main way to do that. + const marker = "ERROR: " + sc := bufio.NewScanner(bytes.NewReader(src)) + wantErrors := make(map[int]string) + lineNum := 1 + for sc.Scan() { + lineText := sc.Text() + if idx := strings.Index(lineText, marker); idx != -1 { + summaryText := lineText[idx+len(marker):] + wantErrors[lineNum] = summaryText + } + lineNum++ + } + + parser := testParser(map[string]string{ + name: string(src), + }) + + _, diags := parser.LoadConfigFile(name) + + gotErrors := make(map[int]string) + for _, diag := range diags { + if diag.Severity != hcl.DiagError || diag.Subject == nil { + continue + } + gotErrors[diag.Subject.Start.Line] = diag.Summary + } + + if diff := cmp.Diff(wantErrors, gotErrors); diff != "" { + t.Errorf("wrong errors\n%s", diff) + } + }) + } +} diff --git a/configs/provisioner.go b/configs/provisioner.go index 6f380860a..769382513 100644 --- a/configs/provisioner.go +++ b/configs/provisioner.go @@ -147,8 +147,8 @@ func onlySelfRefs(body hcl.Body) hcl.Diagnostics { if !valid { diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "External references from destroy provisioners are deprecated", + Severity: hcl.DiagError, + Summary: "Invalid reference from destroy provisioner", Detail: "Destroy-time provisioners and their connection configurations may only " + "reference attributes of the related resource, via 'self', 'count.index', " + "or 'each.key'.\n\nReferences to other resources during the destroy phase " + diff --git a/configs/testdata/warning-files/destroy-provisioners.tf b/configs/testdata/error-files/destroy-provisioners.tf similarity index 69% rename from configs/testdata/warning-files/destroy-provisioners.tf rename to configs/testdata/error-files/destroy-provisioners.tf index c6c28b823..4831b5302 100644 --- a/configs/testdata/warning-files/destroy-provisioners.tf +++ b/configs/testdata/error-files/destroy-provisioners.tf @@ -5,7 +5,7 @@ locals { resource "null_resource" "a" { connection { host = self.hostname - user = local.user # WARNING: External references from destroy provisioners are deprecated + user = local.user # ERROR: Invalid reference from destroy provisioner } provisioner "remote-exec" { @@ -36,9 +36,9 @@ resource "null_resource" "b" { when = destroy connection { host = self.hostname - user = local.user # WARNING: External references from destroy provisioners are deprecated + user = local.user # ERROR: Invalid reference from destroy provisioner } - command = "echo ${local.name}" # WARNING: External references from destroy provisioners are deprecated + command = "echo ${local.name}" # ERROR: Invalid reference from destroy provisioner } } diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 8f316c5fa..af5916fbb 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -290,35 +290,26 @@ func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) { p := testProvider("aws") p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.a": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "parent", - }, - Dependencies: []string{"module.child"}, - Provider: "provider.aws", - }, - }, - }, - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.child": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "child", - }, - Provider: "provider.aws", - }, - }, - }, + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"parent"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.child")}, }, - }) + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.child").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"child"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) { // verify the apply happens in the correct order @@ -1264,30 +1255,26 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { p := testProvider("aws") p.ApplyFn = testApplyFn p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - }, - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - }, - }, - }, + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.bar").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), }, - }) + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.bar")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) // Record the order we see Apply var actual []string @@ -1329,34 +1316,6 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { // Test that destroy ordering is correct with dependencies only // in the state. func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { - legacyState := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - Provider: "provider.aws", - }, - - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - Dependencies: []string{"aws_instance.foo"}, - Provider: "provider.aws", - }, - }, - }, - }, - }) - newState := states.NewState() root := newState.EnsureModule(addrs.RootModuleInstance) root.SetResourceInstanceCurrent( @@ -1404,9 +1363,6 @@ func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { // It is possible for this to be racy, so we loop a number of times // just to check. for i := 0; i < 10; i++ { - t.Run("legacy", func(t *testing.T) { - testContext2Apply_destroyDependsOnStateOnly(t, legacyState) - }) t.Run("new", func(t *testing.T) { testContext2Apply_destroyDependsOnStateOnly(t, newState) }) @@ -1458,34 +1414,6 @@ func testContext2Apply_destroyDependsOnStateOnly(t *testing.T, state *states.Sta // Test that destroy ordering is correct with dependencies only // in the state within a module (GH-11749) func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { - legacyState := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - Provider: "provider.aws", - }, - - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - Dependencies: []string{"aws_instance.foo"}, - Provider: "provider.aws", - }, - }, - }, - }, - }) - newState := states.NewState() child := newState.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) child.SetResourceInstanceCurrent( @@ -1533,9 +1461,6 @@ func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { // It is possible for this to be racy, so we loop a number of times // just to check. for i := 0; i < 10; i++ { - t.Run("legacy", func(t *testing.T) { - testContext2Apply_destroyDependsOnStateOnlyModule(t, legacyState) - }) t.Run("new", func(t *testing.T) { testContext2Apply_destroyDependsOnStateOnlyModule(t, newState) }) @@ -2118,72 +2043,6 @@ aws_instance.foo: `) } -// for_each values cannot be used in the provisioner during destroy. -// There may be a way to handle this, but for now make sure we print an error -// rather than crashing with an invalid config. -func TestContext2Apply_provisionerDestroyForEach(t *testing.T) { - m := testModule(t, "apply-provisioner-each") - p := testProvider("aws") - pr := testProvisioner() - p.DiffFn = testDiffFn - p.ApplyFn = testApplyFn - - s := &states.State{ - Modules: map[string]*states.Module{ - "": &states.Module{ - Resources: map[string]*states.Resource{ - "aws_instance.bar": &states.Resource{ - Addr: addrs.Resource{Mode: 77, Type: "aws_instance", Name: "bar"}, - EachMode: states.EachMap, - Instances: map[addrs.InstanceKey]*states.ResourceInstance{ - addrs.StringKey("a"): &states.ResourceInstance{ - Current: &states.ResourceInstanceObjectSrc{ - AttrsJSON: []byte(`{"foo":"bar","id":"foo"}`), - }, - }, - addrs.StringKey("b"): &states.ResourceInstance{ - Current: &states.ResourceInstanceObjectSrc{ - AttrsJSON: []byte(`{"foo":"bar","id":"foo"}`), - }, - }, - }, - ProviderConfig: addrs.AbsProviderConfig{ - Module: addrs.ModuleInstance(nil), - Provider: addrs.NewLegacyProvider("aws"), - }, - }, - }, - }, - }, - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - State: s, - Destroy: true, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("plan errors: %s", diags.Err()) - } - - _, diags := ctx.Apply() - if diags == nil { - t.Fatal("should error") - } - if !strings.Contains(diags.Err().Error(), "each.value cannot be used in this context") { - t.Fatal("unexpected error:", diags.Err()) - } -} - func TestContext2Apply_cancelProvisioner(t *testing.T) { m := testModule(t, "apply-cancel-provisioner") p := testProvider("aws") @@ -2831,30 +2690,26 @@ func TestContext2Apply_moduleDestroyOrder(t *testing.T) { }, } - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.b": resourceState("aws_instance", "b"), - }, - }, - - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.a": resourceState("aws_instance", "a"), - }, - Outputs: map[string]*OutputState{ - "a_output": &OutputState{ - Type: "string", - Sensitive: false, - Value: "a", - }, - }, - }, + state := states.NewState() + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), }, - }) + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.b").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.aws_instance.a")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) ctx := testContext2(t, &ContextOpts{ Config: m, @@ -5763,196 +5618,6 @@ aws_instance.foo: } } -func TestContext2Apply_provisionerDestroyModule(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-module") - p := testProvider("aws") - pr := testProvisioner() - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { - val, ok := c.Config["command"] - if !ok || val != "value" { - t.Fatalf("bad value for foo: %v %#v", val, c) - } - - return nil - } - - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - }, - }, - }, - }, - }) - - ctx := testContext2(t, &ContextOpts{ - Config: m, - State: state, - Destroy: true, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("plan errors: %s", diags.Err()) - } - - state, diags := ctx.Apply() - if diags.HasErrors() { - t.Fatalf("diags: %s", diags.Err()) - } - - checkStateString(t, state, ``) - - // Verify apply was invoked - if !pr.ProvisionResourceCalled { - t.Fatalf("provisioner not invoked") - } -} - -func TestContext2Apply_provisionerDestroyRef(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-ref") - p := testProvider("aws") - pr := testProvisioner() - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { - val, ok := c.Config["command"] - if !ok || val != "hello" { - return fmt.Errorf("bad value for command: %v %#v", val, c) - } - - return nil - } - - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{ - "value": "hello", - }, - }, - Provider: "provider.aws", - }, - - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - Provider: "provider.aws", - }, - }, - }, - }, - }) - - ctx := testContext2(t, &ContextOpts{ - Config: m, - State: state, - Destroy: true, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatalf("plan errors: %s", diags.Err()) - } - - state, diags := ctx.Apply() - if diags.HasErrors() { - t.Fatalf("diags: %s", diags.Err()) - } - - checkStateString(t, state, ``) - - // Verify apply was invoked - if !pr.ProvisionResourceCalled { - t.Fatalf("provisioner not invoked") - } -} - -// Test that a destroy provisioner referencing an invalid key errors. -func TestContext2Apply_provisionerDestroyRefInvalid(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-ref-invalid") - p := testProvider("aws") - pr := testProvisioner() - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - pr.ApplyFn = func(rs *InstanceState, c *ResourceConfig) error { - return nil - } - - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - }, - - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - }, - }, - }, - }, - }) - - ctx := testContext2(t, &ContextOpts{ - Config: m, - State: state, - Destroy: true, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - }) - - // this was an apply test, but this is now caught in Validation - if diags := ctx.Validate(); !diags.HasErrors() { - t.Fatal("expected error") - } -} - func TestContext2Apply_provisionerResourceRef(t *testing.T) { m := testModule(t, "apply-provisioner-resource-ref") p := testProvider("aws") @@ -8350,259 +8015,32 @@ aws_instance.bar: `) } -func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-locals") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - - pr := testProvisioner() - pr.ApplyFn = func(_ *InstanceState, rc *ResourceConfig) error { - cmd, ok := rc.Get("command") - if !ok || cmd != "local" { - return fmt.Errorf("provisioner got %v:%s", ok, cmd) - } - return nil - } - pr.GetSchemaResponse = provisioners.GetSchemaResponse{ - Provisioner: &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "command": { - Type: cty.String, - Required: true, - }, - "when": { - Type: cty.String, - Optional: true, - }, - }, - }, - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "1234"), - }, - }, - }, - }), - Destroy: true, - // the test works without targeting, but this also tests that the local - // node isn't inadvertently pruned because of the wrong evaluation - // order. - Targets: []addrs.Targetable{ - addrs.RootModuleInstance.Resource( - addrs.ManagedResourceMode, "aws_instance", "foo", - ), - }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if _, diags := ctx.Apply(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if !pr.ProvisionResourceCalled { - t.Fatal("provisioner not called") - } -} - -// this also tests a local value in the config referencing a resource that -// wasn't in the state during destroy. -func TestContext2Apply_destroyProvisionerWithMultipleLocals(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-multiple-locals") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - - pr := testProvisioner() - pr.GetSchemaResponse = provisioners.GetSchemaResponse{ - Provisioner: &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "id": { - Type: cty.String, - Required: true, - }, - "command": { - Type: cty.String, - Required: true, - }, - "when": { - Type: cty.String, - Optional: true, - }, - }, - }, - } - - pr.ApplyFn = func(is *InstanceState, rc *ResourceConfig) error { - cmd, ok := rc.Get("command") - if !ok { - return errors.New("no command in provisioner") - } - id, ok := rc.Get("id") - if !ok { - return errors.New("no id in provisioner") - } - - switch id { - case "1234": - if cmd != "local" { - return fmt.Errorf("provisioner %q got:%q", is.ID, cmd) - } - case "3456": - if cmd != "1234" { - return fmt.Errorf("provisioner %q got:%q", is.ID, cmd) - } - default: - t.Fatal("unknown instance") - } - return nil - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "1234"), - "aws_instance.bar": resourceState("aws_instance", "3456"), - }, - }, - }, - }), - Destroy: true, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if _, diags := ctx.Apply(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - if !pr.ProvisionResourceCalled { - t.Fatal("provisioner not called") - } -} - -func TestContext2Apply_destroyProvisionerWithOutput(t *testing.T) { - m := testModule(t, "apply-provisioner-destroy-outputs") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - - pr := testProvisioner() - pr.ApplyFn = func(is *InstanceState, rc *ResourceConfig) error { - cmd, ok := rc.Get("command") - if !ok || cmd != "3" { - return fmt.Errorf("provisioner for %s got %v:%s", is.ID, ok, cmd) - } - return nil - } - ctx := testContext2(t, &ContextOpts{ - Config: m, - ProviderResolver: providers.ResolverFixed( - map[addrs.Provider]providers.Factory{ - addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), - }, - ), - Provisioners: map[string]ProvisionerFactory{ - "shell": testProvisionerFuncFixed(pr), - }, - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root"}, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "1"), - }, - Outputs: map[string]*OutputState{ - "value": { - Type: "string", - Value: "3", - }, - }, - }, - &ModuleState{ - Path: []string{"root", "mod"}, - Resources: map[string]*ResourceState{ - "aws_instance.baz": resourceState("aws_instance", "3"), - }, - // state needs to be properly initialized - Outputs: map[string]*OutputState{}, - }, - &ModuleState{ - Path: []string{"root", "mod2"}, - Resources: map[string]*ResourceState{ - "aws_instance.bar": resourceState("aws_instance", "2"), - }, - }, - }, - }), - Destroy: true, - - // targeting the source of the value used by all resources should still - // destroy them all. - Targets: []addrs.Targetable{ - addrs.RootModuleInstance.Child("mod", addrs.NoKey).Resource( - addrs.ManagedResourceMode, "aws_instance", "baz", - ), - }, - }) - - if _, diags := ctx.Plan(); diags.HasErrors() { - t.Fatal(diags.Err()) - } - - state, diags := ctx.Apply() - if diags.HasErrors() { - t.Fatal(diags.Err()) - } - if !pr.ProvisionResourceCalled { - t.Fatal("provisioner not called") - } - - // confirm all outputs were removed too - for _, mod := range state.Modules { - if len(mod.OutputValues) > 0 { - t.Fatalf("output left in module state: %#v\n", mod) - } - } -} - func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { m := testModule(t, "apply-destroy-targeted-count") p := testProvider("aws") p.ApplyFn = testApplyFn p.DiffFn = testDiffFn + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"i-bcd345"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.bar").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"i-abc123"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("aws_instance.foo")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/aws"]`), + ) + ctx := testContext2(t, &ContextOpts{ Config: m, ProviderResolver: providers.ResolverFixed( @@ -8610,17 +8048,7 @@ func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) { addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), }, ), - State: MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.foo": resourceState("aws_instance", "i-bcd345"), - "aws_instance.bar": resourceState("aws_instance", "i-abc123"), - }, - }, - }, - }), + State: state, Targets: []addrs.Targetable{ addrs.RootModuleInstance.Resource( addrs.ManagedResourceMode, "aws_instance", "foo", @@ -10236,29 +9664,16 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { p.ApplyFn = testApplyFn p.DiffFn = testDiffFn - s := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - }, - &ModuleState{ - Path: []string{"root", "child"}, - }, - &ModuleState{ - Path: []string{"root", "mod", "removed"}, - Resources: map[string]*ResourceState{ - "aws_instance.child": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - }, - // this provider doesn't exist - Provider: "provider.aws.baz", - }, - }, - }, + state := states.NewState() + removed := state.EnsureModule(addrs.RootModuleInstance.Child("mod", addrs.NoKey).Child("removed", addrs.NoKey)) + removed.SetResourceInstanceCurrent( + mustResourceInstanceAddr("aws_instance.child").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), }, - }) + mustProviderConfig(`provider["registry.terraform.io/-/aws"].baz`), + ) ctx := testContext2(t, &ContextOpts{ Config: m, @@ -10267,7 +9682,7 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { addrs.NewLegacyProvider("aws"): testProviderFuncFixed(p), }, ), - State: s, + State: state, Destroy: true, }) @@ -10277,10 +9692,10 @@ func TestContext2Apply_destroyWithProviders(t *testing.T) { } // correct the state - s.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.AbsProviderConfig{ + state.Modules["module.mod.module.removed"].Resources["aws_instance.child"].ProviderConfig = addrs.AbsProviderConfig{ Provider: addrs.NewLegacyProvider("aws"), - Module: addrs.RootModuleInstance, Alias: "bar", + Module: addrs.RootModuleInstance, } if _, diags := ctx.Plan(); diags.HasErrors() { diff --git a/terraform/edge_destroy.go b/terraform/edge_destroy.go deleted file mode 100644 index bc9d638aa..000000000 --- a/terraform/edge_destroy.go +++ /dev/null @@ -1,17 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/terraform/dag" -) - -// DestroyEdge is an edge that represents a standard "destroy" relationship: -// Target depends on Source because Source is destroying. -type DestroyEdge struct { - S, T dag.Vertex -} - -func (e *DestroyEdge) Hashcode() interface{} { return fmt.Sprintf("%p-%p", e.S, e.T) } -func (e *DestroyEdge) Source() dag.Vertex { return e.S } -func (e *DestroyEdge) Target() dag.Vertex { return e.T } diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 00ddcf5c4..a9aba06bb 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -168,20 +168,10 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Config: b.Config, State: b.State, Schemas: b.Schemas, - Destroy: b.Destroy, }, - // Handle destroy time transformations for output and local values. - // Reverse the edges from outputs and locals, so that - // interpolations don't fail during destroy. // Create a destroy node for outputs to remove them from the state. - GraphTransformIf( - func() bool { return b.Destroy }, - GraphTransformMulti( - &DestroyValueReferenceTransformer{}, - &DestroyOutputTransformer{}, - ), - ), + &DestroyOutputTransformer{Destroy: b.Destroy}, // Prune unreferenced values, which may have interpolations that can't // be resolved. diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 593c6149a..c0827d05b 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -235,12 +235,6 @@ func TestApplyGraphBuilder_doubleCBD(t *testing.T) { "test_object.B", destroyB, ) - - // actual := strings.TrimSpace(g.String()) - // expected := strings.TrimSpace(testApplyGraphBuilderDoubleCBDStr) - // if actual != expected { - // t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - // } } // This tests the ordering of two resources being destroyed that depend @@ -263,33 +257,26 @@ func TestApplyGraphBuilder_destroyStateOnly(t *testing.T) { }, } - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: []string{"root", "child"}, - Resources: map[string]*ResourceState{ - "test_object.A": &ResourceState{ - Type: "test_object", - Primary: &InstanceState{ - ID: "foo", - Attributes: map[string]string{}, - }, - Provider: "provider.test", - }, - - "test_object.B": &ResourceState{ - Type: "test_object", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{}, - }, - Dependencies: []string{"test_object.A"}, - Provider: "provider.test", - }, - }, - }, + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), }, - }) + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.A")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) b := &ApplyGraphBuilder{ Config: testModule(t, "empty"), @@ -304,7 +291,6 @@ func TestApplyGraphBuilder_destroyStateOnly(t *testing.T) { if diags.HasErrors() { t.Fatalf("err: %s", diags.Err()) } - t.Logf("Graph:\n%s", g.String()) if g.Path.String() != addrs.RootModuleInstance.String() { t.Fatalf("wrong path %q", g.Path.String()) @@ -376,11 +362,33 @@ func TestApplyGraphBuilder_moduleDestroy(t *testing.T) { }, } + state := states.NewState() + modA := state.EnsureModule(addrs.RootModuleInstance.Child("A", addrs.NoKey)) + modA.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + modB := state.EnsureModule(addrs.RootModuleInstance.Child("B", addrs.NoKey)) + modB.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.foo").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo","value":"foo"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.A.test_object.foo")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + b := &ApplyGraphBuilder{ Config: testModule(t, "graph-builder-apply-module-destroy"), Changes: changes, Components: simpleMockComponentFactory(), Schemas: simpleTestSchemas(), + State: state, } g, err := b.Build(addrs.RootModuleInstance) diff --git a/terraform/graph_builder_destroy_plan.go b/terraform/graph_builder_destroy_plan.go index a6047a9b4..08ee4e63e 100644 --- a/terraform/graph_builder_destroy_plan.go +++ b/terraform/graph_builder_destroy_plan.go @@ -72,6 +72,9 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer { State: b.State, }, + // Attach the state + &AttachStateTransformer{State: b.State}, + // Attach the configuration to any resources &AttachResourceConfigTransformer{Config: b.Config}, diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 2149f4535..bab23fd5e 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -10,7 +10,6 @@ import ( "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/lang" "github.com/hashicorp/terraform/states" - "github.com/hashicorp/terraform/tfdiags" ) // ConcreteResourceNodeFunc is a callback type used to convert an @@ -241,46 +240,6 @@ func (n *NodeAbstractResourceInstance) References() []*addrs.Reference { return n.NodeAbstractResource.References() } - // FIXME: remove once the deprecated DependsOn values have been removed from state - // The state dependencies are now connected in a separate transformation as - // absolute addresses, but we need to keep this here until we can be sure - // that no state will need to use the old depends_on references. - if rs := n.ResourceState; rs != nil { - if s := rs.Instance(n.InstanceKey); s != nil { - // State is still storing dependencies as old-style strings, so we'll - // need to do a little work here to massage this to the form we now - // want. - var result []*addrs.Reference - - // It is (apparently) possible for s.Current to be nil. This proved - // difficult to reproduce, so we will fix the symptom here and hope - // to find the root cause another time. - // - // https://github.com/hashicorp/terraform/issues/21407 - if s.Current == nil { - log.Printf("[WARN] no current state found for %s", n.Name()) - return nil - } - for _, addr := range s.Current.DependsOn { - if addr == nil { - // Should never happen; indicates a bug in the state loader - panic(fmt.Sprintf("dependencies for current object on %s contains nil address", n.ResourceInstanceAddr())) - } - - // This is a little weird: we need to manufacture an addrs.Reference - // with a fake range here because the state isn't something we can - // make source references into. - result = append(result, &addrs.Reference{ - Subject: addr, - SourceRange: tfdiags.SourceRange{ - Filename: "(state file)", - }, - }) - } - return result - } - } - // If we have neither config nor state then we have no references. return nil } diff --git a/terraform/testdata/apply-provisioner-destroy-locals/main.tf b/terraform/testdata/apply-provisioner-destroy-locals/main.tf deleted file mode 100644 index 5818e7c7d..000000000 --- a/terraform/testdata/apply-provisioner-destroy-locals/main.tf +++ /dev/null @@ -1,14 +0,0 @@ -locals { - value = "local" -} - -resource "aws_instance" "foo" { - provisioner "shell" { - command = "${local.value}" - when = "create" - } - provisioner "shell" { - command = "${local.value}" - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-module/child/main.tf b/terraform/testdata/apply-provisioner-destroy-module/child/main.tf deleted file mode 100644 index a8b8d123c..000000000 --- a/terraform/testdata/apply-provisioner-destroy-module/child/main.tf +++ /dev/null @@ -1,10 +0,0 @@ -variable "key" {} - -resource "aws_instance" "foo" { - foo = "bar" - - provisioner "shell" { - command = "${var.key}" - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-module/main.tf b/terraform/testdata/apply-provisioner-destroy-module/main.tf deleted file mode 100644 index 817ae043d..000000000 --- a/terraform/testdata/apply-provisioner-destroy-module/main.tf +++ /dev/null @@ -1,4 +0,0 @@ -module "child" { - source = "./child" - key = "value" -} diff --git a/terraform/testdata/apply-provisioner-destroy-multiple-locals/main.tf b/terraform/testdata/apply-provisioner-destroy-multiple-locals/main.tf deleted file mode 100644 index 2050b19a1..000000000 --- a/terraform/testdata/apply-provisioner-destroy-multiple-locals/main.tf +++ /dev/null @@ -1,26 +0,0 @@ -locals { - value = "local" - foo_id = aws_instance.foo.id - - // baz is not in the state during destroy, but this is a valid config that - // should not fail. - baz_id = aws_instance.baz.id -} - -resource "aws_instance" "baz" {} - -resource "aws_instance" "foo" { - provisioner "shell" { - id = self.id - command = local.value - when = "destroy" - } -} - -resource "aws_instance" "bar" { - provisioner "shell" { - id = self.id - command = local.foo_id - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-outputs/main.tf b/terraform/testdata/apply-provisioner-destroy-outputs/main.tf deleted file mode 100644 index 9a5843aa3..000000000 --- a/terraform/testdata/apply-provisioner-destroy-outputs/main.tf +++ /dev/null @@ -1,23 +0,0 @@ -module "mod" { - source = "./mod" -} - -locals { - value = "${module.mod.value}" -} - -resource "aws_instance" "foo" { - provisioner "shell" { - command = "${local.value}" - when = "destroy" - } -} - -module "mod2" { - source = "./mod2" - value = "${module.mod.value}" -} - -output "value" { - value = "${local.value}" -} diff --git a/terraform/testdata/apply-provisioner-destroy-outputs/mod/main.tf b/terraform/testdata/apply-provisioner-destroy-outputs/mod/main.tf deleted file mode 100644 index 1f4ad09c5..000000000 --- a/terraform/testdata/apply-provisioner-destroy-outputs/mod/main.tf +++ /dev/null @@ -1,5 +0,0 @@ -output "value" { - value = "${aws_instance.baz.id}" -} - -resource "aws_instance" "baz" {} diff --git a/terraform/testdata/apply-provisioner-destroy-outputs/mod2/main.tf b/terraform/testdata/apply-provisioner-destroy-outputs/mod2/main.tf deleted file mode 100644 index a476bb920..000000000 --- a/terraform/testdata/apply-provisioner-destroy-outputs/mod2/main.tf +++ /dev/null @@ -1,10 +0,0 @@ -variable "value" { -} - -resource "aws_instance" "bar" { - provisioner "shell" { - command = "${var.value}" - when = "destroy" - } -} - diff --git a/terraform/testdata/apply-provisioner-destroy-ref-invalid/main.tf b/terraform/testdata/apply-provisioner-destroy-ref-invalid/main.tf deleted file mode 100644 index fb4a96b10..000000000 --- a/terraform/testdata/apply-provisioner-destroy-ref-invalid/main.tf +++ /dev/null @@ -1,12 +0,0 @@ -resource "aws_instance" "bar" { - value = "hello" -} - -resource "aws_instance" "foo" { - foo = "bar" - - provisioner "shell" { - command = aws_instance.bar.does_not_exist - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-destroy-ref/main.tf b/terraform/testdata/apply-provisioner-destroy-ref/main.tf deleted file mode 100644 index b36916df1..000000000 --- a/terraform/testdata/apply-provisioner-destroy-ref/main.tf +++ /dev/null @@ -1,12 +0,0 @@ -resource "aws_instance" "bar" { - value = "hello" -} - -resource "aws_instance" "foo" { - foo = "bar" - - provisioner "shell" { - command = "${aws_instance.bar.value}" - when = "destroy" - } -} diff --git a/terraform/testdata/apply-provisioner-each/main.tf b/terraform/testdata/apply-provisioner-each/main.tf index 8d29a5c16..29be7206e 100644 --- a/terraform/testdata/apply-provisioner-each/main.tf +++ b/terraform/testdata/apply-provisioner-each/main.tf @@ -2,6 +2,6 @@ resource "aws_instance" "bar" { for_each = toset(["a"]) provisioner "shell" { when = "destroy" - command = "echo ${each.value}" + command = "echo ${each.key}" } } diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 410a709ea..0bfcda96e 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -134,25 +134,16 @@ type CBDEdgeTransformer struct { // obtain schema information from providers and provisioners so we can // properly resolve implicit dependencies. Schemas *Schemas - - // If the operation is a simple destroy, no transformation is done. - Destroy bool } func (t *CBDEdgeTransformer) Transform(g *Graph) error { - if t.Destroy { - return nil - } - // Go through and reverse any destroy edges - destroyMap := make(map[string][]dag.Vertex) for _, v := range g.Vertices() { dn, ok := v.(GraphNodeDestroyerCBD) if !ok { continue } - dern, ok := v.(GraphNodeDestroyer) - if !ok { + if _, ok = v.(GraphNodeDestroyer); !ok { continue } @@ -162,158 +153,17 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { // Find the resource edges for _, e := range g.EdgesTo(v) { - switch de := e.(type) { - case *DestroyEdge: - // we need to invert the destroy edge from the create node - log.Printf("[TRACE] CBDEdgeTransformer: inverting edge: %s => %s", - dag.VertexName(de.Source()), dag.VertexName(de.Target())) + src := e.Source() - // Found it! Invert. - g.RemoveEdge(de) - applyNode := de.Source() - destroyNode := de.Target() - g.Connect(&DestroyEdge{S: destroyNode, T: applyNode}) - default: - // We cannot have any direct dependencies from creators when - // the node is CBD without inducing a cycle. - if _, ok := e.Source().(GraphNodeCreator); ok { - log.Printf("[TRACE] CBDEdgeTransformer: removing non DestroyEdge to CBD destroy node: %s => %s", dag.VertexName(e.Source()), dag.VertexName(e.Target())) - g.RemoveEdge(e) - } + // If source is a create node, invert the edge. + // This covers both the node's own creator, as well as reversing + // any dependants' edges. + if _, ok := src.(GraphNodeCreator); ok { + log.Printf("[TRACE] CBDEdgeTransformer: reversing edge %s -> %s", dag.VertexName(src), dag.VertexName(v)) + g.RemoveEdge(e) + g.Connect(dag.BasicEdge(v, src)) } } - - // If the address has an index, we strip that. Our depMap creation - // graph doesn't expand counts so we don't currently get _exact_ - // dependencies. One day when we limit dependencies more exactly - // this will have to change. We have a test case covering this - // (depNonCBDCountBoth) so it'll be caught. - addr := dern.DestroyAddr() - key := addr.ContainingResource().String() - - // Add this to the list of nodes that we need to fix up - // the edges for (step 2 above in the docs). - destroyMap[key] = append(destroyMap[key], v) } - - // If we have no CBD nodes, then our work here is done - if len(destroyMap) == 0 { - return nil - } - - // We have CBD nodes. We now have to move on to the much more difficult - // task of connecting dependencies of the creation side of the destroy - // to the destruction node. The easiest way to explain this is an example: - // - // Given a pre-destroy dependence of: A => B - // And A has CBD set. - // - // The resulting graph should be: A => B => A_d - // - // They key here is that B happens before A is destroyed. This is to - // facilitate the primary purpose for CBD: making sure that downstreams - // are properly updated to avoid downtime before the resource is destroyed. - depMap, err := t.depMap(g, destroyMap) - if err != nil { - return err - } - - // We now have the mapping of resource addresses to the destroy - // nodes they need to depend on. We now go through our own vertices to - // find any matching these addresses and make the connection. - for _, v := range g.Vertices() { - // We're looking for creators - rn, ok := v.(GraphNodeCreator) - if !ok { - continue - } - - // Get the address - addr := rn.CreateAddr() - - // If the address has an index, we strip that. Our depMap creation - // graph doesn't expand counts so we don't currently get _exact_ - // dependencies. One day when we limit dependencies more exactly - // this will have to change. We have a test case covering this - // (depNonCBDCount) so it'll be caught. - key := addr.ContainingResource().String() - - // If there is nothing this resource should depend on, ignore it - dns, ok := depMap[key] - if !ok { - continue - } - - // We have nodes! Make the connection - for _, dn := range dns { - log.Printf("[TRACE] CBDEdgeTransformer: destroy depends on dependence: %s => %s", - dag.VertexName(dn), dag.VertexName(v)) - g.Connect(dag.BasicEdge(dn, v)) - } - } - return nil } - -func (t *CBDEdgeTransformer) depMap(g *Graph, destroyMap map[string][]dag.Vertex) (map[string][]dag.Vertex, error) { - // Build the list of destroy nodes that each resource address should depend - // on. For example, when we find B, we map the address of B to A_d in the - // "depMap" variable below. - - // Use a nested map to remove duplicate edges. - depMap := make(map[string]map[dag.Vertex]struct{}) - for _, v := range g.Vertices() { - // We're looking for resources. - rn, ok := v.(GraphNodeResource) - if !ok { - continue - } - - // Get the address - addr := rn.ResourceAddr() - key := addr.String() - - // Get the destroy nodes that are destroying this resource. - // If there aren't any, then we don't need to worry about - // any connections. - dns, ok := destroyMap[key] - if !ok { - continue - } - - // Get the nodes that depend on this on. In the example above: - // finding B in A => B. Since dependencies can span modules, walk all - // descendents of the resource. - des, _ := g.Descendents(v) - for _, v := range des.List() { - // We're looking for resources. - rn, ok := v.(GraphNodeResource) - if !ok { - continue - } - - // Keep track of the destroy nodes that this address - // needs to depend on. - key := rn.ResourceAddr().String() - - deps, ok := depMap[key] - if !ok { - deps = make(map[dag.Vertex]struct{}) - } - - for _, d := range dns { - deps[d] = struct{}{} - } - depMap[key] = deps - } - } - - result := map[string][]dag.Vertex{} - for k, m := range depMap { - for v := range m { - result[k] = append(result[k], v) - } - } - - return result, nil -} diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index deaddf96c..0831d7ad9 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -341,12 +341,10 @@ func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) { test_object.A\[0\] test_object.A\[0\] \(destroy deposed \w+\) test_object.A\[0\] - test_object.A\[1\] test_object.B\[0\] test_object.B\[1\] test_object.A\[1\] test_object.A\[1\] \(destroy deposed \w+\) - test_object.A\[0\] test_object.A\[1\] test_object.B\[0\] test_object.B\[1\] diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index f52429229..5092b3abb 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -56,7 +56,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { // Build a map of what is being destroyed (by address string) to // the list of destroyers. destroyers := make(map[string][]GraphNodeDestroyer) - destroyerAddrs := make(map[string]addrs.AbsResourceInstance) // Record the creators, which will need to depend on the destroyers if they // are only being updated. @@ -79,7 +78,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { key := addr.String() log.Printf("[TRACE] DestroyEdgeTransformer: %q (%T) destroys %s", dag.VertexName(n), v, key) destroyers[key] = append(destroyers[key], n) - destroyerAddrs[key] = addr resAddr := addr.Resource.Resource.Absolute(addr.Module).String() destroyersByResource[resAddr] = append(destroyersByResource[resAddr], n) @@ -151,7 +149,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { "[TRACE] DestroyEdgeTransformer: connecting creator %q with destroyer %q", dag.VertexName(a), dag.VertexName(a_d)) - g.Connect(&DestroyEdge{S: a, T: a_d}) + g.Connect(dag.BasicEdge(a, a_d)) // Attach the destroy node to the creator // There really shouldn't be more than one destroyer, but even if @@ -165,159 +163,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { } } - // This is strange but is the easiest way to get the dependencies - // of a node that is being destroyed. We use another graph to make sure - // the resource is in the graph and ask for references. We have to do this - // because the node that is being destroyed may NOT be in the graph. - // - // Example: resource A is force new, then destroy A AND create A are - // in the graph. BUT if resource A is just pure destroy, then only - // destroy A is in the graph, and create A is not. - providerFn := func(a *NodeAbstractProvider) dag.Vertex { - return &NodeApplyableProvider{NodeAbstractProvider: a} - } - steps := []GraphTransformer{ - // Add the local values - &LocalTransformer{Config: t.Config}, - - // Add outputs and metadata - &OutputTransformer{Config: t.Config}, - &AttachResourceConfigTransformer{Config: t.Config}, - &AttachStateTransformer{State: t.State}, - - // Add all the variables. We can depend on resources through - // variables due to module parameters, and we need to properly - // determine that. - &RootVariableTransformer{Config: t.Config}, - &ModuleVariableTransformer{Config: t.Config}, - - TransformProviders(nil, providerFn, t.Config), - - // Must attach schemas before ReferenceTransformer so that we can - // analyze the configuration to find references. - &AttachSchemaTransformer{Schemas: t.Schemas}, - - &ReferenceTransformer{}, - } - - // Go through all the nodes being destroyed and create a graph. - // The resulting graph is only of things being CREATED. For example, - // following our example, the resulting graph would be: - // - // A, B (with no edges) - // - var tempG Graph - var tempDestroyed []dag.Vertex - for d := range destroyers { - // d is the string key for the resource being destroyed. We actually - // want the address value, which we stashed earlier. - addr := destroyerAddrs[d] - - // This part is a little bit weird but is the best way to - // find the dependencies we need to: build a graph and use the - // attach config and state transformers then ask for references. - abstract := NewNodeAbstractResourceInstance(addr) - tempG.Add(abstract) - tempDestroyed = append(tempDestroyed, abstract) - - // We also add the destroy version here since the destroy can - // depend on things that the creation doesn't (destroy provisioners). - destroy := &NodeDestroyResourceInstance{NodeAbstractResourceInstance: abstract} - tempG.Add(destroy) - tempDestroyed = append(tempDestroyed, destroy) - } - - // Run the graph transforms so we have the information we need to - // build references. - log.Printf("[TRACE] DestroyEdgeTransformer: constructing temporary graph for analysis of references, starting from:\n%s", tempG.StringWithNodeTypes()) - for _, s := range steps { - log.Printf("[TRACE] DestroyEdgeTransformer: running %T on temporary graph", s) - if err := s.Transform(&tempG); err != nil { - log.Printf("[TRACE] DestroyEdgeTransformer: %T failed: %s", s, err) - return err - } - } - log.Printf("[TRACE] DestroyEdgeTransformer: temporary reference graph:\n%s", tempG.String()) - - // Go through all the nodes in the graph and determine what they - // depend on. - for _, v := range tempDestroyed { - // Find all ancestors of this to determine the edges we'll depend on - vs, err := tempG.Ancestors(v) - if err != nil { - return err - } - - refs := make([]dag.Vertex, 0, vs.Len()) - for _, raw := range vs.List() { - refs = append(refs, raw.(dag.Vertex)) - } - - refNames := make([]string, len(refs)) - for i, ref := range refs { - refNames[i] = dag.VertexName(ref) - } - log.Printf( - "[TRACE] DestroyEdgeTransformer: creation node %q references %s", - dag.VertexName(v), refNames) - - // If we have no references, then we won't need to do anything - if len(refs) == 0 { - continue - } - - // Get the destroy node for this. In the example of our struct, - // we are currently at B and we're looking for B_d. - rn, ok := v.(GraphNodeResourceInstance) - if !ok { - log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s, since it's not a resource", dag.VertexName(v)) - continue - } - - addr := rn.ResourceInstanceAddr() - dns := destroyers[addr.String()] - - // We have dependencies, check if any are being destroyed - // to build the list of things that we must depend on! - // - // In the example of the struct, if we have: - // - // B_d => A_d => A => B - // - // Then at this point in the algorithm we started with B_d, - // we built B (to get dependencies), and we found A. We're now looking - // to see if A_d exists. - var depDestroyers []dag.Vertex - for _, v := range refs { - rn, ok := v.(GraphNodeResourceInstance) - if !ok { - continue - } - - addr := rn.ResourceInstanceAddr() - key := addr.String() - if ds, ok := destroyers[key]; ok { - for _, d := range ds { - depDestroyers = append(depDestroyers, d.(dag.Vertex)) - log.Printf( - "[TRACE] DestroyEdgeTransformer: destruction of %q depends on %s", - key, dag.VertexName(d)) - } - } - } - - // Go through and make the connections. Use the variable - // names "a_d" and "b_d" to reference our example. - for _, a_d := range dns { - for _, b_d := range depDestroyers { - if b_d != a_d { - log.Printf("[TRACE] DestroyEdgeTransformer: %q depends on %q", dag.VertexName(b_d), dag.VertexName(a_d)) - g.Connect(dag.BasicEdge(b_d, a_d)) - } - } - } - } - return t.pruneResources(g) } diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index 01059db02..19a141b52 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -5,12 +5,37 @@ import ( "testing" "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/states" ) func TestDestroyEdgeTransformer_basic(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.B"}) + g.Add(testDestroyNode("test_object.A")) + g.Add(testDestroyNode("test_object.B")) + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-basic"), Schemas: simpleTestSchemas(), @@ -26,31 +51,48 @@ func TestDestroyEdgeTransformer_basic(t *testing.T) { } } -func TestDestroyEdgeTransformer_create(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.B"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.A"}) - tf := &DestroyEdgeTransformer{ - Config: testModule(t, "transform-destroy-edge-basic"), - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformDestroyEdgeCreatorStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - } -} - func TestDestroyEdgeTransformer_multi(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.B"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.C"}) + g.Add(testDestroyNode("test_object.A")) + g.Add(testDestroyNode("test_object.B")) + g.Add(testDestroyNode("test_object.C")) + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.C").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"C","test_string":"x"}`), + Dependencies: []addrs.AbsResource{ + mustResourceAddr("test_object.A"), + mustResourceAddr("test_object.B"), + }, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-multi"), Schemas: simpleTestSchemas(), @@ -68,7 +110,7 @@ func TestDestroyEdgeTransformer_multi(t *testing.T) { func TestDestroyEdgeTransformer_selfRef(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) + g.Add(testDestroyNode("test_object.A")) tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-self-ref"), Schemas: simpleTestSchemas(), @@ -86,8 +128,33 @@ func TestDestroyEdgeTransformer_selfRef(t *testing.T) { func TestDestroyEdgeTransformer_module(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.b"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.a"}) + g.Add(testDestroyNode("module.child.test_object.b")) + g.Add(testDestroyNode("test_object.a")) + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.b")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.b").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b","test_string":"x"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-module"), Schemas: simpleTestSchemas(), @@ -105,9 +172,46 @@ func TestDestroyEdgeTransformer_module(t *testing.T) { func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) { g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.a"}) - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.b"}) - g.Add(&graphNodeDestroyerTest{AddrString: "module.child.test_object.c"}) + g.Add(testDestroyNode("module.child.test_object.a")) + g.Add(testDestroyNode("module.child.test_object.b")) + g.Add(testDestroyNode("module.child.test_object.c")) + + state := states.NewState() + child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.b").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b","test_string":"x"}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("module.child.test_object.a")}, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + child.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.c").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"c","test_string":"x"}`), + Dependencies: []addrs.AbsResource{ + mustResourceAddr("module.child.test_object.a"), + mustResourceAddr("module.child.test_object.b"), + }, + }, + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), + ) + + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + tf := &DestroyEdgeTransformer{ Config: testModule(t, "transform-destroy-edge-module-only"), Schemas: simpleTestSchemas(), @@ -130,86 +234,17 @@ module.child.test_object.c (destroy) } } -type graphNodeCreatorTest struct { - AddrString string - Refs []string -} +func testDestroyNode(addrString string) GraphNodeDestroyer { + instAddr := mustResourceInstanceAddr(addrString) -var ( - _ GraphNodeCreator = (*graphNodeCreatorTest)(nil) - _ GraphNodeReferencer = (*graphNodeCreatorTest)(nil) -) + abs := NewNodeAbstractResource(instAddr.ContainingResource()) -func (n *graphNodeCreatorTest) Name() string { - return n.CreateAddr().String() -} - -func (n *graphNodeCreatorTest) mustAddr() addrs.AbsResourceInstance { - addr, diags := addrs.ParseAbsResourceInstanceStr(n.AddrString) - if diags.HasErrors() { - panic(diags.Err()) - } - return addr -} - -func (n *graphNodeCreatorTest) Path() addrs.ModuleInstance { - return n.mustAddr().Module -} - -func (n *graphNodeCreatorTest) CreateAddr() *addrs.AbsResourceInstance { - addr := n.mustAddr() - return &addr -} - -func (n *graphNodeCreatorTest) References() []*addrs.Reference { - ret := make([]*addrs.Reference, len(n.Refs)) - for i, str := range n.Refs { - ref, diags := addrs.ParseRefStr(str) - if diags.HasErrors() { - panic(diags.Err()) - } - ret[i] = ref - } - return ret -} - -type graphNodeDestroyerTest struct { - AddrString string - CBD bool - Modified bool -} - -var _ GraphNodeDestroyer = (*graphNodeDestroyerTest)(nil) - -func (n *graphNodeDestroyerTest) Name() string { - result := n.DestroyAddr().String() + " (destroy)" - if n.Modified { - result += " (modified)" + inst := &NodeAbstractResourceInstance{ + NodeAbstractResource: *abs, + InstanceKey: instAddr.Resource.Key, } - return result -} - -func (n *graphNodeDestroyerTest) mustAddr() addrs.AbsResourceInstance { - addr, diags := addrs.ParseAbsResourceInstanceStr(n.AddrString) - if diags.HasErrors() { - panic(diags.Err()) - } - return addr -} - -func (n *graphNodeDestroyerTest) CreateBeforeDestroy() bool { - return n.CBD -} - -func (n *graphNodeDestroyerTest) ModifyCreateBeforeDestroy(v bool) error { - n.Modified = true - return nil -} - -func (n *graphNodeDestroyerTest) DestroyAddr() *addrs.AbsResourceInstance { - addr := n.mustAddr() - return &addr + return &NodeDestroyResourceInstance{NodeAbstractResourceInstance: inst} } const testTransformDestroyEdgeBasicStr = ` diff --git a/terraform/transform_output.go b/terraform/transform_output.go index ed93cdb87..f68228b38 100644 --- a/terraform/transform_output.go +++ b/terraform/transform_output.go @@ -60,9 +60,15 @@ func (t *OutputTransformer) transform(g *Graph, c *configs.Config) error { // outputs during destroy. We need to do this to ensure that no stale outputs // are ever left in the state. type DestroyOutputTransformer struct { + Destroy bool } func (t *DestroyOutputTransformer) Transform(g *Graph) error { + // Only clean root outputs on a full destroy + if !t.Destroy { + return nil + } + for _, v := range g.Vertices() { output, ok := v.(*NodeApplyableOutput) if !ok { diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index df34a3cea..371265bab 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -80,6 +80,12 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { // Find the things that reference things and connect them for _, v := range vs { + if _, ok := v.(GraphNodeDestroyer); ok { + // destroy nodes references are not connected, since they can only + // use their own state. + continue + } + parents, _ := m.References(v) parentsDbg := make([]string, len(parents)) for i, v := range parents { @@ -170,42 +176,6 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error { return nil } -// DestroyReferenceTransformer is a GraphTransformer that reverses the edges -// for locals and outputs that depend on other nodes which will be -// removed during destroy. If a destroy node is evaluated before the local or -// output value, it will be removed from the state, and the later interpolation -// will fail. -type DestroyValueReferenceTransformer struct{} - -func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error { - vs := g.Vertices() - for _, v := range vs { - switch v.(type) { - case *NodeApplyableOutput, *NodeLocal: - // OK - default: - continue - } - - // reverse any outgoing edges so that the value is evaluated first. - for _, e := range g.EdgesFrom(v) { - target := e.Target() - - // only destroy nodes will be evaluated in reverse - if _, ok := target.(GraphNodeDestroyer); !ok { - continue - } - - log.Printf("[TRACE] output dep: %s", dag.VertexName(target)) - - g.RemoveEdge(e) - g.Connect(&DestroyEdge{S: target, T: v}) - } - } - - return nil -} - // PruneUnusedValuesTransformer is a GraphTransformer that removes local, // variable, and output values which are not referenced in the graph. If these // values reference a resource that is no longer in the state the interpolation