diff --git a/internal/stacks/stackruntime/internal/stackeval/component.go b/internal/stacks/stackruntime/internal/stackeval/component.go index 7906ba71e8..c24f1face0 100644 --- a/internal/stacks/stackruntime/internal/stackeval/component.go +++ b/internal/stacks/stackruntime/internal/stackeval/component.go @@ -347,3 +347,24 @@ func (c *Component) ApplySuccessful(ctx context.Context) bool { func (c *Component) tracingName() string { return c.Addr().String() } + +// reportNamedPromises implements namedPromiseReporter. +func (c *Component) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + name := c.Addr().String() + instsName := name + " instances" + forEachName := name + " for_each" + c.instances.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[map[addrs.InstanceKey]*ComponentInstance]]) { + cb(o.PromiseID(), instsName) + }) + // FIXME: We should call reportNamedPromises on the individual + // ComponentInstance objects too, but promising.Once doesn't allow us + // to peek to see if the Once was already resolved without blocking on + // it, and we don't want to block on any promises in here. + // Without this, any promises belonging to the individual instances will + // not be named in a self-dependency error report, but since references + // to component instances are always indirect through the component this + // shouldn't be a big deal in most cases. + c.forEachValue.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[cty.Value]]) { + cb(o.PromiseID(), forEachName) + }) +} diff --git a/internal/stacks/stackruntime/internal/stackeval/component_config.go b/internal/stacks/stackruntime/internal/stackeval/component_config.go index d717a38c4b..b4ddf8f46e 100644 --- a/internal/stacks/stackruntime/internal/stackeval/component_config.go +++ b/internal/stacks/stackruntime/internal/stackeval/component_config.go @@ -531,6 +531,12 @@ func (c *ComponentConfig) tracingName() string { return c.Addr().String() } +// reportNamedPromises implements namedPromiseReporter. +func (c *ComponentConfig) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + cb(c.validate.PromiseID(), c.Addr().String()) + cb(c.moduleTree.PromiseID(), c.Addr().String()+" modules") +} + // sourceBundleModuleWalker is an implementation of [configs.ModuleWalker] // that loads all modules from a single source bundle. type sourceBundleModuleWalker struct { diff --git a/internal/stacks/stackruntime/internal/stackeval/component_instance.go b/internal/stacks/stackruntime/internal/stackeval/component_instance.go index b4eba207ba..b513129497 100644 --- a/internal/stacks/stackruntime/internal/stackeval/component_instance.go +++ b/internal/stacks/stackruntime/internal/stackeval/component_instance.go @@ -1488,3 +1488,8 @@ func (c *ComponentInstance) resourceTypeSchema(ctx context.Context, providerType func (c *ComponentInstance) tracingName() string { return c.Addr().String() } + +// reportNamedPromises implements namedPromiseReporter. +func (c *ComponentInstance) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + cb(c.moduleTreePlan.PromiseID(), c.Addr().String()+" plan") +} diff --git a/internal/stacks/stackruntime/internal/stackeval/diagnostics_test.go b/internal/stacks/stackruntime/internal/stackeval/diagnostics_test.go new file mode 100644 index 0000000000..d9bc3b9778 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/diagnostics_test.go @@ -0,0 +1,179 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package stackeval + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/collections" + "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/plans" + "github.com/hashicorp/terraform/internal/promising" + "github.com/hashicorp/terraform/internal/providers" + providertest "github.com/hashicorp/terraform/internal/providers/testing" + "github.com/hashicorp/terraform/internal/stacks/stackaddrs" + "github.com/hashicorp/terraform/internal/stacks/stackstate" + "github.com/hashicorp/terraform/internal/tfdiags" +) + +func TestNamedPromisesPlan(t *testing.T) { + // The goal of this test is to make sure we retain namedPromiseReporter + // coverage over various important object types, so that we don't + // accidentally regress the quality of self-reference ("dependency cycle") + // errors under future maintenence. + // + // It isn't totally comprehensive over all implementations of + // namedPromiseReporter, but we do aim to cover the main cases that a + // typical stack configuration might hit. + // + // This is intentionally a test of the namedPromiseReporter implementations + // directly, rather than of the dependency-message-building logic built + // in terms of it, because the goal is for namedPromiseReporter to return + // everything and then the diagnostic reporter to cherry-pick only the + // subset of names it needs, and because this way we can get more test + // coverage without needing fixtures for every possible combination of + // self-references. + + cfg := testStackConfig(t, "planning", "named_promises") + + main := NewForPlanning(cfg, stackstate.NewState(), PlanOpts{ + PlanningMode: plans.NormalMode, + InputVariableValues: map[stackaddrs.InputVariable]ExternalInputValue{ + {Name: "in"}: ExternalInputValue{ + Value: cty.StringVal("hello"), + }, + }, + ProviderFactories: ProviderFactories{ + addrs.MustParseProviderSourceString("example.com/test/happycloud"): providers.FactoryFixed( + &providertest.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + Provider: providers.Schema{ + Block: &configschema.Block{}, + }, + ResourceTypes: map[string]providers.Schema{ + "happycloud_thingy": providers.Schema{ + Block: &configschema.Block{}, + }, + }, + }, + }, + ), + }, + }) + + // We don't actually really care about the plan here. We just want the + // side-effect of getting a bunch of promises created inside "main", which + // we'll then ask about below. + _, diags := testPlan(t, main) + assertNoDiagnostics(t, diags) + + wantNames := collections.NewSetCmp[string]( + // Component-related + `component.foo`, + `component.foo modules`, + `component.foo for_each`, + `component.foo instances`, + + // Nested-stack-related + `stack.child collected outputs`, + `stack.child inputs`, + `stack.child for_each`, + `stack.child instances`, + + // Provider-related + `example.com/test/happycloud schema`, + `provider["example.com/test/happycloud"].main`, + `provider["example.com/test/happycloud"].main for_each`, + `provider["example.com/test/happycloud"].main instances`, + + // Output-value-related + `output.out value`, + `stack.child.output.out value`, + `output.out`, + `stack.child.output.out`, + + // Input-variable-related + `var.in`, + `stack.child.var.in`, + ) + gotNames := collections.NewSetCmp[string]() + ids := map[string]promising.PromiseID{} + main.reportNamedPromises(func(id promising.PromiseID, name string) { + gotNames.Add(name) + // We'll also remember the id associated with each name so that + // we can test the diagnostic message rendering below. + ids[name] = id + // NOTE: Some of the names get reused across both a config object + // and its associated dynamic object when there are no dynamic + // instance keys involved, and for those it's unspecified which + // promise ID will "win", but that's fine for our purposes here + // because we're only testing that some specific names get + // included into the error messages and so it doesn't matter which + // of the promise ids we use to achieve that. + }) + + if diff := cmp.Diff(wantNames, gotNames, collections.CmpOptions); diff != "" { + // If you're here because you've seen a failure where some of the + // wanted names seem to have vanished, and you weren't intentionally + // trying to remove them, check to make sure that the type that was + // supposed to report that name is still reachable indirectly from the + // Main.reportNamedPromises implementation. + t.Errorf("wrong promise names\n%s", diff) + } + + // Since we're now holding all of the information required, let's also + // test that we can render some self-dependency diagnostic messages. + t.Run("diagnostics", func(t *testing.T) { + // For this we need to choose some specific promise ids to report. + // It doesn't matter which ones we use but we can only proceed if + // they were ones detected by the reportNamedPromises call earlier. + providerSchemaPromise := ids[`example.com/test/happycloud schema`] + stackCallInstancesPromise := ids[`stack.child instances`] + if providerSchemaPromise == promising.NoPromise || stackCallInstancesPromise == promising.NoPromise { + t.Fatalf("don't have the promise ids required to test diagnostic rendering") + } + + t.Run("just one", func(t *testing.T) { + err := promising.ErrSelfDependent{stackCallInstancesPromise} + diag := taskSelfDependencyDiagnostic{ + err: err, + root: main, + } + got := diag.Description() + want := tfdiags.Description{ + Summary: `Self-dependent item in configuration`, + Detail: `The item "stack.child instances" depends on its own results, so there is no correct order of operations.`, + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("wrong diagnostic description\n%s", diff) + } + }) + t.Run("multiple", func(t *testing.T) { + err := promising.ErrSelfDependent{ + providerSchemaPromise, + stackCallInstancesPromise, + } + diag := taskSelfDependencyDiagnostic{ + err: err, + root: main, + } + got := diag.Description() + want := tfdiags.Description{ + Summary: `Self-dependent items in configuration`, + Detail: `The following items in your configuration form a circular dependency chain through their references: + - example.com/test/happycloud schema + - stack.child instances + +Terraform uses references to decide a suitable order for performing operations, so configuration items may not refer to their own results either directly or indirectly.`, + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("wrong diagnostic description\n%s", diff) + } + }) + }) +} diff --git a/internal/stacks/stackruntime/internal/stackeval/input_variable.go b/internal/stacks/stackruntime/internal/stackeval/input_variable.go index da9c6a4596..7c66cf5648 100644 --- a/internal/stacks/stackruntime/internal/stackeval/input_variable.go +++ b/internal/stacks/stackruntime/internal/stackeval/input_variable.go @@ -259,6 +259,14 @@ func (v *InputVariable) tracingName() string { return v.Addr().String() } +// reportNamedPromises implements namedPromiseReporter. +func (v *InputVariable) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + name := v.Addr().String() + v.value.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[cty.Value]]) { + cb(o.PromiseID(), name) + }) +} + // ExternalInputValue represents the value of an input variable provided // from outside the stack configuration. type ExternalInputValue struct { diff --git a/internal/stacks/stackruntime/internal/stackeval/main.go b/internal/stacks/stackruntime/internal/stackeval/main.go index bd10ae6ff3..4aab455a81 100644 --- a/internal/stacks/stackruntime/internal/stackeval/main.go +++ b/internal/stacks/stackruntime/internal/stackeval/main.go @@ -596,6 +596,12 @@ func (m *Main) reportNamedPromises(cb func(id promising.PromiseID, name string)) if m.mainStackConfig != nil { m.mainStackConfig.reportNamedPromises(cb) } + if m.mainStack != nil { + m.mainStack.reportNamedPromises(cb) + } + for _, pty := range m.providerTypes { + pty.reportNamedPromises(cb) + } } // availableProvisioners returns the table of provisioner factories that should diff --git a/internal/stacks/stackruntime/internal/stackeval/output_value.go b/internal/stacks/stackruntime/internal/stackeval/output_value.go index fd3978d499..d7ff6a6819 100644 --- a/internal/stacks/stackruntime/internal/stackeval/output_value.go +++ b/internal/stacks/stackruntime/internal/stackeval/output_value.go @@ -190,3 +190,11 @@ func (v *OutputValue) CheckApply(ctx context.Context) ([]stackstate.AppliedChang func (v *OutputValue) tracingName() string { return v.Addr().String() } + +// reportNamedPromises implements namedPromiseReporter. +func (v *OutputValue) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + name := v.Addr().String() + v.resultValue.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[cty.Value]]) { + cb(o.PromiseID(), name) + }) +} diff --git a/internal/stacks/stackruntime/internal/stackeval/provider.go b/internal/stacks/stackruntime/internal/stackeval/provider.go index c5c9990829..59a29ea468 100644 --- a/internal/stacks/stackruntime/internal/stackeval/provider.go +++ b/internal/stacks/stackruntime/internal/stackeval/provider.go @@ -273,3 +273,24 @@ func (p *Provider) CheckApply(ctx context.Context) ([]stackstate.AppliedChange, func (p *Provider) tracingName() string { return p.Addr().String() } + +// reportNamedPromises implements namedPromiseReporter. +func (p *Provider) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + name := p.Addr().String() + forEachName := name + " for_each" + instsName := name + " instances" + p.forEachValue.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[cty.Value]]) { + cb(o.PromiseID(), forEachName) + }) + p.instances.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[map[addrs.InstanceKey]*ProviderInstance]]) { + cb(o.PromiseID(), instsName) + }) + // FIXME: We should call reportNamedPromises on the individual + // ProviderInstance objects too, but promising.Once doesn't allow us + // to peek to see if the Once was already resolved without blocking on + // it, and we don't want to block on any promises in here. + // Without this, any promises belonging to the individual instances will + // not be named in a self-dependency error report, but since references + // to provider instances are always indirect through the provider this + // shouldn't be a big deal in most cases. +} diff --git a/internal/stacks/stackruntime/internal/stackeval/provider_config.go b/internal/stacks/stackruntime/internal/stackeval/provider_config.go index 611d98e2d1..2b4edaf843 100644 --- a/internal/stacks/stackruntime/internal/stackeval/provider_config.go +++ b/internal/stacks/stackruntime/internal/stackeval/provider_config.go @@ -224,3 +224,8 @@ func (p *ProviderConfig) PlanChanges(ctx context.Context) ([]stackplan.PlannedCh func (p *ProviderConfig) tracingName() string { return p.Addr().String() } + +// reportNamedPromises implements namedPromiseReporter. +func (p *ProviderConfig) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + cb(p.providerArgs.PromiseID(), p.Addr().String()) +} diff --git a/internal/stacks/stackruntime/internal/stackeval/provider_instance.go b/internal/stacks/stackruntime/internal/stackeval/provider_instance.go index 6e1ddcf3b5..acd2508644 100644 --- a/internal/stacks/stackruntime/internal/stackeval/provider_instance.go +++ b/internal/stacks/stackruntime/internal/stackeval/provider_instance.go @@ -308,6 +308,18 @@ func (p *ProviderInstance) tracingName() string { return p.Addr().String() } +// reportNamedPromises implements namedPromiseReporter. +func (p *ProviderInstance) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + name := p.Addr().String() + clientName := name + " plugin client" + p.providerArgs.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[cty.Value]]) { + cb(o.PromiseID(), name) + }) + p.client.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[providers.Interface]]) { + cb(o.PromiseID(), clientName) + }) +} + // stubConfiguredProvider is a placeholder provider used when ConfigureProvider // on a real provider fails, so that callers can still receieve a usable client // that will just produce placeholder values from its operations. diff --git a/internal/stacks/stackruntime/internal/stackeval/provider_type.go b/internal/stacks/stackruntime/internal/stackeval/provider_type.go index a3f83c2561..4f4c15ee20 100644 --- a/internal/stacks/stackruntime/internal/stackeval/provider_type.go +++ b/internal/stacks/stackruntime/internal/stackeval/provider_type.go @@ -70,3 +70,8 @@ func (pt *ProviderType) Schema(ctx context.Context) (providers.GetProviderSchema return ret, nil }) } + +// reportNamedPromises implements namedPromiseReporter. +func (pt *ProviderType) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + cb(pt.schema.PromiseID(), pt.Addr().String()+" schema") +} diff --git a/internal/stacks/stackruntime/internal/stackeval/stack.go b/internal/stacks/stackruntime/internal/stackeval/stack.go index abedfa897d..5cacadff21 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack.go +++ b/internal/stacks/stackruntime/internal/stackeval/stack.go @@ -16,6 +16,7 @@ import ( "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/plans" + "github.com/hashicorp/terraform/internal/promising" "github.com/hashicorp/terraform/internal/stacks/stackaddrs" "github.com/hashicorp/terraform/internal/stacks/stackconfig" "github.com/hashicorp/terraform/internal/stacks/stackconfig/typeexpr" @@ -640,3 +641,28 @@ func (s *Stack) tracingName() string { } return addr.String() } + +// reportNamedPromises implements namedPromiseReporter. +func (s *Stack) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + s.mu.Lock() + defer s.mu.Unlock() + + for _, child := range s.childStacks { + child.reportNamedPromises(cb) + } + for _, child := range s.inputVariables { + child.reportNamedPromises(cb) + } + for _, child := range s.outputValues { + child.reportNamedPromises(cb) + } + for _, child := range s.stackCalls { + child.reportNamedPromises(cb) + } + for _, child := range s.components { + child.reportNamedPromises(cb) + } + for _, child := range s.providers { + child.reportNamedPromises(cb) + } +} diff --git a/internal/stacks/stackruntime/internal/stackeval/stack_call.go b/internal/stacks/stackruntime/internal/stackeval/stack_call.go index e2eb7bd6d3..3d914741b1 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack_call.go +++ b/internal/stacks/stackruntime/internal/stackeval/stack_call.go @@ -269,3 +269,24 @@ func (c *StackCall) CheckApply(ctx context.Context) ([]stackstate.AppliedChange, func (c *StackCall) tracingName() string { return c.Addr().String() } + +// reportNamedPromises implements namedPromiseReporter. +func (c *StackCall) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + name := c.Addr().String() + instsName := name + " instances" + forEachName := name + " for_each" + c.instances.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[map[addrs.InstanceKey]*StackCallInstance]]) { + cb(o.PromiseID(), instsName) + }) + // FIXME: We should call reportNamedPromises on the individual + // StackCallInstance objects too, but promising.Once doesn't allow us + // to peek to see if the Once was already resolved without blocking on + // it, and we don't want to block on any promises in here. + // Without this, any promises belonging to the individual instances will + // not be named in a self-dependency error report, but since references + // to stack call instances are always indirect through the stack call this + // shouldn't be a big deal in most cases. + c.forEachValue.Each(func(ep EvalPhase, o *promising.Once[withDiagnostics[cty.Value]]) { + cb(o.PromiseID(), forEachName) + }) +} diff --git a/internal/stacks/stackruntime/internal/stackeval/stack_call_instance.go b/internal/stacks/stackruntime/internal/stackeval/stack_call_instance.go index 9f8d000091..9d56e4e6dc 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack_call_instance.go +++ b/internal/stacks/stackruntime/internal/stackeval/stack_call_instance.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/collections" "github.com/hashicorp/terraform/internal/instances" + "github.com/hashicorp/terraform/internal/promising" "github.com/hashicorp/terraform/internal/stacks/stackaddrs" "github.com/hashicorp/terraform/internal/stacks/stackplan" "github.com/hashicorp/terraform/internal/stacks/stackstate" @@ -204,3 +205,8 @@ func (c *StackCallInstance) CheckApply(ctx context.Context) ([]stackstate.Applie func (c *StackCallInstance) tracingName() string { return fmt.Sprintf("%s call", c.CalledStackAddr()) } + +// reportNamedPromises implements namedPromiseReporter. +func (c *StackCallInstance) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + // StackCallInstance does not currently own any promises +} diff --git a/internal/stacks/stackruntime/internal/stackeval/stack_config.go b/internal/stacks/stackruntime/internal/stackeval/stack_config.go index e170868fb4..3f7324e9d3 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack_config.go +++ b/internal/stacks/stackruntime/internal/stackeval/stack_config.go @@ -528,4 +528,10 @@ func (s *StackConfig) reportNamedPromises(cb func(id promising.PromiseID, name s for _, child := range s.stackCalls { child.reportNamedPromises(cb) } + for _, child := range s.components { + child.reportNamedPromises(cb) + } + for _, child := range s.providers { + child.reportNamedPromises(cb) + } } diff --git a/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/child/named_promises_child.tfstack.hcl b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/child/named_promises_child.tfstack.hcl new file mode 100644 index 0000000000..8da88fe3a2 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/child/named_promises_child.tfstack.hcl @@ -0,0 +1,11 @@ +# This is a very minimal stack configuration just to give us something to +# call as a nested stack in the parent stack configuration. + +variable "in" { + type = string +} + +output "out" { + type = string + value = var.in +} diff --git a/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/named_promises.tf b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/named_promises.tf new file mode 100644 index 0000000000..d54b6cb567 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/named_promises.tf @@ -0,0 +1,13 @@ +# This is an intentionally-minimal module just to give us something to +# point a component block at. + +terraform { + required_providers { + happycloud = { + source = "example.com/test/happycloud" + } + } +} + +resource "happycloud_thingy" "foo" { +} diff --git a/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/named_promises.tfstack.hcl b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/named_promises.tfstack.hcl new file mode 100644 index 0000000000..11cc036709 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/named_promises/named_promises.tfstack.hcl @@ -0,0 +1,35 @@ + +required_providers { + happycloud = { + source = "example.com/test/happycloud" + version = "1.0.0" + } +} + +variable "in" { + type = string +} + +provider "happycloud" "main" { +} + +stack "child" { + source = "./child" + + inputs = { + in = var.in + } +} + +component "foo" { + source = "./" + + providers = { + happycloud = provider.happycloud.main + } +} + +output "out" { + type = string + value = var.in +}