diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fb92f02d5c0..ae5e9424e232 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## 1.9.4 (August 7, 2024) + +BUG FIXES: + +* core: Unneeded variable validations were being executed during a destroy plan, which could cause plans starting with incomplete state to fail. ([#35511](https://github.com/hashicorp/terraform/issues/35511)) +* init: Don't crash when discovering invalid syntax in duplicate required_providers blocks. ([#35533](https://github.com/hashicorp/terraform/issues/35533)) + ## 1.9.3 (July 24, 2024) ENHANCEMENTS: diff --git a/internal/checks/state_report.go b/internal/checks/state_report.go index ca02cb47aa1c..afe07740e2c5 100644 --- a/internal/checks/state_report.go +++ b/internal/checks/state_report.go @@ -5,6 +5,7 @@ package checks import ( "fmt" + "log" "github.com/hashicorp/terraform/internal/addrs" ) @@ -50,6 +51,7 @@ func (c *State) ReportCheckableObjects(configAddr addrs.ConfigCheckable, objectA checks[checkType] = make([]Status, count) } + log.Printf("[TRACE] ReportCheckableObjects: %s -> %s", configAddr, objectAddr) st.objects.Put(objectAddr, checks) } } diff --git a/internal/command/init_test.go b/internal/command/init_test.go index 5f51b1bc45b4..71c99b31641a 100644 --- a/internal/command/init_test.go +++ b/internal/command/init_test.go @@ -2945,6 +2945,66 @@ hashicorp/test: no available releases match the given constraints 1.0.1, } } +func TestInit_testsWithOverriddenInvalidRequiredProviders(t *testing.T) { + td := t.TempDir() + testCopyDir(t, testFixturePath("init-with-overrides-and-duplicates"), td) + defer testChdir(t, td)() + + provider := applyFixtureProvider() // We just want the types from this provider. + + providerSource, close := newMockProviderSource(t, map[string][]string{ + "hashicorp/test": {"1.0.0"}, + }) + defer close() + + ui := new(cli.MockUi) + view, done := testView(t) + c := &InitCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(provider), + Ui: ui, + View: view, + ProviderSource: providerSource, + }, + } + + args := []string{} + code := c.Run(args) // just make sure it doesn't crash. + if code != 1 { + t.Fatalf("expected failure but got: \n%s", done(t).All()) + } +} + +func TestInit_testsWithInvalidRequiredProviders(t *testing.T) { + td := t.TempDir() + testCopyDir(t, testFixturePath("init-with-duplicates"), td) + defer testChdir(t, td)() + + provider := applyFixtureProvider() // We just want the types from this provider. + + providerSource, close := newMockProviderSource(t, map[string][]string{ + "hashicorp/test": {"1.0.0"}, + }) + defer close() + + ui := new(cli.MockUi) + view, done := testView(t) + c := &InitCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(provider), + Ui: ui, + View: view, + ProviderSource: providerSource, + }, + } + + args := []string{} + code := c.Run(args) // just make sure it doesn't crash. + if code != 1 { + t.Fatalf("expected failure but got: \n%s", done(t).All()) + } +} + func TestInit_testsWithModule(t *testing.T) { // Create a temporary working directory that is empty td := t.TempDir() diff --git a/internal/command/testdata/init-with-duplicates/primary.tf b/internal/command/testdata/init-with-duplicates/primary.tf new file mode 100644 index 000000000000..fcc6047614c0 --- /dev/null +++ b/internal/command/testdata/init-with-duplicates/primary.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } +} + +resource "test_instance" "foo" { + ami = "bar" +} diff --git a/internal/command/testdata/init-with-duplicates/secondary.tf b/internal/command/testdata/init-with-duplicates/secondary.tf new file mode 100644 index 000000000000..e11209619bcb --- /dev/null +++ b/internal/command/testdata/init-with-duplicates/secondary.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + { // This typo is deliberate, we want to test that the parser can handle it. + } +} + +resource "test_instance" "bar" { + ami = "foo" +} diff --git a/internal/command/testdata/init-with-overrides-and-duplicates/primary.tf b/internal/command/testdata/init-with-overrides-and-duplicates/primary.tf new file mode 100644 index 000000000000..fcc6047614c0 --- /dev/null +++ b/internal/command/testdata/init-with-overrides-and-duplicates/primary.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } +} + +resource "test_instance" "foo" { + ami = "bar" +} diff --git a/internal/command/testdata/init-with-overrides-and-duplicates/secondary.tf b/internal/command/testdata/init-with-overrides-and-duplicates/secondary.tf new file mode 100644 index 000000000000..2ed0d7feeb0e --- /dev/null +++ b/internal/command/testdata/init-with-overrides-and-duplicates/secondary.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } +} + +resource "test_instance" "bar" { + ami = "foo" +} diff --git a/internal/command/testdata/init-with-overrides-and-duplicates/secondary_override.tf b/internal/command/testdata/init-with-overrides-and-duplicates/secondary_override.tf new file mode 100644 index 000000000000..3c269f3732b3 --- /dev/null +++ b/internal/command/testdata/init-with-overrides-and-duplicates/secondary_override.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + { // This typo is deliberate, we want to test that the parser can handle it. + } +} + +resource "test_instance" "bar" { + ami = "override" +} diff --git a/internal/configs/parser_config.go b/internal/configs/parser_config.go index 83148bfe3159..468809e061d8 100644 --- a/internal/configs/parser_config.go +++ b/internal/configs/parser_config.go @@ -120,7 +120,9 @@ func parseConfigFile(body hcl.Body, diags hcl.Diagnostics, override, allowExperi case "required_providers": reqs, reqsDiags := decodeRequiredProvidersBlock(innerBlock) diags = append(diags, reqsDiags...) - file.RequiredProviders = append(file.RequiredProviders, reqs) + if reqs != nil { + file.RequiredProviders = append(file.RequiredProviders, reqs) + } case "provider_meta": providerCfg, cfgDiags := decodeProviderMetaBlock(innerBlock) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 521b3d612fef..f952e328f53a 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -2177,6 +2177,13 @@ variable "input" { } } +# In order for the variable to be validated during destroy, it must be required +# by the destroy plan. This is done by having the test provider require the +# value in order to destroy the test_object instance. +provider "test" { + test_string = var.input +} + resource "test_object" "a" { test_string = var.input } diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index a38e3f3c1475..5a3675ae01ce 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -5658,3 +5658,66 @@ resource "test_object" "a" { } } + +func TestContext2Plan_destroySkipsVariableValidations(t *testing.T) { + // this validation cannot block destroy, because we can't be sure arbitrary + // expressions can be evaluated at all during destroy. + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "input" { + type = string + + validation { + condition = var.input == "foo" + error_message = "bad input" + } +} + +resource "test_object" "a" { + test_string = var.input +} +`, + }) + + p := simpleMockProvider() + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, states.BuildState(func(state *states.SyncState) { + state.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.a"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"test_string":"foo"}`), + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + }), &PlanOpts{ + Mode: plans.DestroyMode, + SetVariables: InputValues{ + "input": { + Value: cty.StringVal("foo"), + SourceType: ValueFromCLIArg, + SourceRange: tfdiags.SourceRange{}, + }, + }, + }) + if diags.HasErrors() { + t.Errorf("expected no errors, but got %s", diags) + } + + planResult := plan.Checks.GetObjectResult(addrs.AbsInputVariableInstance{ + Variable: addrs.InputVariable{ + Name: "input", + }, + Module: addrs.RootModuleInstance, + }) + + if planResult.Status != checks.StatusUnknown { + // checks should not have been evaluated, because the variable is not required for destroy. + t.Errorf("expected checks to be pass but was %s", planResult.Status) + } +} diff --git a/internal/terraform/graph.go b/internal/terraform/graph.go index 98b1e857094a..d641fb57b4d8 100644 --- a/internal/terraform/graph.go +++ b/internal/terraform/graph.go @@ -51,6 +51,14 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics { log.Printf("[TRACE] vertex %q: starting visit (%T)", dag.VertexName(v), v) defer func() { + if r := recover(); r != nil { + // If the walkFn panics, we get confusing logs about how the + // visit was complete. To stop this, we'll catch the panic log + // that the vertex panicked without finishing and re-panic. + log.Printf("[ERROR] vertex %q panicked", dag.VertexName(v)) + panic(r) // re-panic + } + if diags.HasErrors() { for _, diag := range diags { if diag.Severity() == tfdiags.Error { diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index 892da6a6005c..1fc34ad5f9d2 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -68,6 +68,7 @@ func (n *nodeExpandModuleVariable) DynamicExpand(ctx EvalContext) (*Graph, tfdia forEachModuleInstance(expander, n.Module, false, func(module addrs.ModuleInstance) { addr := n.Addr.Absolute(module) if checkableAddrs != nil { + log.Printf("[TRACE] nodeExpandModuleVariable: found checkable object %s", addr) checkableAddrs.Add(addr) } diff --git a/internal/terraform/node_variable_validation.go b/internal/terraform/node_variable_validation.go index 4322e35902aa..6d8cd3876b6b 100644 --- a/internal/terraform/node_variable_validation.go +++ b/internal/terraform/node_variable_validation.go @@ -40,6 +40,7 @@ var _ GraphNodeModulePath = (*nodeVariableValidation)(nil) var _ GraphNodeReferenceable = (*nodeVariableValidation)(nil) var _ GraphNodeReferencer = (*nodeVariableValidation)(nil) var _ GraphNodeExecutable = (*nodeVariableValidation)(nil) +var _ graphNodeTemporaryValue = (*nodeVariableValidation)(nil) func (n *nodeVariableValidation) Name() string { return fmt.Sprintf("%s (validation)", n.configAddr.String()) @@ -57,6 +58,15 @@ func (n *nodeVariableValidation) ReferenceableAddrs() []addrs.Referenceable { return []addrs.Referenceable{n.configAddr.Variable} } +// nodeVariableValidation must act as if it's part of the associated variable +// node, and that means mirroring all that node's graph behavior. Root module +// variable are not temporary however, but because during a destroy we can't +// ensure that all references can be evaluated, we must skip validation unless +// absolutely necessary to avoid blocking the destroy from proceeding. +func (n *nodeVariableValidation) temporaryValue() bool { + return true +} + // References implements [GraphNodeReferencer], announcing anything that // the check rules refer to, other than the variable that's being validated // (which gets its dependency connected by [variableValidationTransformer] diff --git a/version/VERSION b/version/VERSION index 77fee73a8cf9..d615fd0c04ab 100644 --- a/version/VERSION +++ b/version/VERSION @@ -1 +1 @@ -1.9.3 +1.9.4