diff --git a/internal/stacks/stackaddrs/component.go b/internal/stacks/stackaddrs/component.go index 2003820169..ebeeff79e7 100644 --- a/internal/stacks/stackaddrs/component.go +++ b/internal/stacks/stackaddrs/component.go @@ -30,6 +30,13 @@ type ComponentInstance struct { func (ComponentInstance) inStackConfigSigil() {} func (ComponentInstance) inStackInstanceSigil() {} +func (c ComponentInstance) String() string { + if c.Key == nil { + return c.Component.String() + } + return c.Component.String() + c.Key.String() +} + // ConfigComponentInstance places a [ComponentInstance] in the context of a // particular [Stack]. type ConfigComponentInstance = InStackConfig[ComponentInstance] diff --git a/internal/stacks/stackaddrs/in_stack.go b/internal/stacks/stackaddrs/in_stack.go index 2966413ffd..d86d47c7a2 100644 --- a/internal/stacks/stackaddrs/in_stack.go +++ b/internal/stacks/stackaddrs/in_stack.go @@ -4,12 +4,14 @@ package stackaddrs // sense to consider as belonging statically to a [Stack]. type StackItemConfig interface { inStackConfigSigil() + String() string } // StackItemDynamic is a type set containing all of the address types that make // sense to consider as belonging dynamically to a [StackInstance]. type StackItemDynamic interface { inStackInstanceSigil() + String() string } // InStackConfig is the generic form of addresses representing configuration @@ -27,6 +29,13 @@ func Config[T StackItemConfig](stackAddr Stack, relAddr T) InStackConfig[T] { } } +func (ist InStackConfig[T]) String() string { + if ist.Stack.IsRoot() { + return ist.Item.String() + } + return ist.Stack.String() + "." + ist.Item.String() +} + // InStackInstance is the generic form of addresses representing dynamic // instances of objects that exist within an instance of a stack. type InStackInstance[T StackItemDynamic] struct { @@ -41,6 +50,13 @@ func Absolute[T StackItemDynamic](stackAddr StackInstance, relAddr T) InStackIns } } +func (ist InStackInstance[T]) String() string { + if ist.Stack.IsRoot() { + return ist.Item.String() + } + return ist.Stack.String() + "." + ist.Item.String() +} + // ConfigForAbs returns the "in stack config" equivalent of the given // "in stack instance" (absolute) address by just discarding any // instance keys from the stack instance steps. diff --git a/internal/stacks/stackaddrs/provider_config.go b/internal/stacks/stackaddrs/provider_config.go index c2c86b55b1..59c312e25f 100644 --- a/internal/stacks/stackaddrs/provider_config.go +++ b/internal/stacks/stackaddrs/provider_config.go @@ -1,6 +1,8 @@ package stackaddrs import ( + "fmt" + "github.com/hashicorp/terraform/internal/addrs" ) @@ -31,6 +33,10 @@ type ProviderConfig struct { func (ProviderConfig) inStackConfigSigil() {} func (ProviderConfig) inStackInstanceSigil() {} +func (c ProviderConfig) String() string { + return fmt.Sprintf("provider[%q].%s", c.Provider, c.Name) +} + // ConfigProviderConfig places a [ProviderConfig] in the context of a particular [Stack]. type ConfigProviderConfig = InStackConfig[ProviderConfig] @@ -48,6 +54,13 @@ type ProviderConfigInstance struct { func (ProviderConfigInstance) inStackConfigSigil() {} func (ProviderConfigInstance) inStackInstanceSigil() {} +func (c ProviderConfigInstance) String() string { + if c.Key == nil { + return c.ProviderConfig.String() + } + return c.ProviderConfig.String() + c.Key.String() +} + // ConfigProviderConfigInstance places a [ProviderConfigInstance] in the context of a particular [Stack]. type ConfigProviderConfigInstance = InStackConfig[ProviderConfigInstance] diff --git a/internal/stacks/stackaddrs/stack.go b/internal/stacks/stackaddrs/stack.go index 22ad175465..5409ac5462 100644 --- a/internal/stacks/stackaddrs/stack.go +++ b/internal/stacks/stackaddrs/stack.go @@ -1,6 +1,8 @@ package stackaddrs import ( + "strings" + "github.com/hashicorp/terraform/internal/addrs" ) @@ -40,6 +42,25 @@ func (s Stack) Child(name string) Stack { return append(ret, StackStep{name}) } +func (s Stack) String() string { + if s.IsRoot() { + // Callers should typically not ask for the string representation of + // the main root stack, but we'll return a reasonable placeholder + // for situations like e.g. internal logs where we just fmt %s in an + // arbitrary stack address that is sometimes the main stack. + return "
" + } + var buf strings.Builder + for i, step := range s { + if i != 0 { + buf.WriteByte('.') + } + buf.WriteString("stack.") + buf.WriteString(step.Name) + } + return buf.String() +} + // StackInstance represents the address of an instance of a stack within // the tree of stacks. // @@ -107,3 +128,25 @@ func (s StackInstance) ConfigAddr() Stack { } return ret } + +func (s StackInstance) String() string { + if s.IsRoot() { + // Callers should typically not ask for the string representation of + // the main root stack, but we'll return a reasonable placeholder + // for situations like e.g. internal logs where we just fmt %s in an + // arbitrary stack address that is sometimes the main stack. + return "
" + } + var buf strings.Builder + for i, step := range s { + if i != 0 { + buf.WriteByte('.') + } + buf.WriteString("stack.") + buf.WriteString(step.Name) + if step.Key != nil { + buf.WriteString(step.Key.String()) + } + } + return buf.String() +} diff --git a/internal/stacks/stackruntime/internal/stackeval/diagnostics.go b/internal/stacks/stackruntime/internal/stackeval/diagnostics.go index ea1de82ecd..c34c8a58a6 100644 --- a/internal/stacks/stackruntime/internal/stackeval/diagnostics.go +++ b/internal/stacks/stackruntime/internal/stackeval/diagnostics.go @@ -1,8 +1,141 @@ package stackeval -import "github.com/hashicorp/terraform/internal/tfdiags" +import ( + "fmt" + "strings" + + "github.com/hashicorp/terraform/internal/promising" + "github.com/hashicorp/terraform/internal/tfdiags" +) type withDiagnostics[T any] struct { Result T Diagnostics tfdiags.Diagnostics } + +// taskSelfDependencyDiagnostics transforms a [promising.ErrSelfDependent] +// error into one or more error diagnostics suitable for returning to an +// end user, after first trying to discover user-friendly names for each +// of the promises involved using the . +func taskSelfDependencyDiagnostics(err promising.ErrSelfDependent, root namedPromiseReporter) tfdiags.Diagnostics { + + promiseNames := collectPromiseNames(root) + distinctPromises := make(map[promising.PromiseID]struct{}) + for _, id := range err { + distinctPromises[id] = struct{}{} + } + + var diags tfdiags.Diagnostics + switch len(distinctPromises) { + case 0: + // Should not get here; there can't be a promise cycle without any + // promises involved in it. + panic("promising.ErrSelfDependent without any promises") + case 1: + const diagSummary = "Object depends on itself" + var promiseID promising.PromiseID + for id := range distinctPromises { + promiseID = id + } + name, ok := promiseNames[promiseID] + if ok { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + diagSummary, + fmt.Sprintf("The object %s depends on its own results, so there is no correct order of operations.", name), + )) + } else { + // This is the worst case to report, since something depended on + // itself but we don't actually know its name. We can't really say + // anything useful here, so we'll treat this as a bug and then + // we can add whatever promise name was missing in order to fix + // that bug. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + diagSummary, + "One of the objects in your configuration refers to its own results, but Terraform was not able to detect which one. The fact that Terraform cannot name the object is a bug; please report it!", + )) + } + default: + // If we have more than one promise involved then it's non-deterministic + // which one we'll detect, since it depends on how the tasks get + // scheduled by the Go runtime. To return a deterministic-ish result + // anyway we'll arbitrarily descide to report whichever promise has + // the lexically-least name as defined by Go's own less than operator + // when applied to strings. + selectedIdx := 0 + selectedName := promiseNames[err[0]] + for i, id := range err { + if selectedName == "" { + // If we don't have a name yet then we'll take whatever we get + selectedIdx = i + selectedName = promiseNames[id] + continue + } + candidateName := promiseNames[id] + if candidateName != "" && candidateName < selectedName { + selectedIdx = i + selectedName = candidateName + } + } + // Now we'll rotate the list of promise IDs so that the one we selected + // appears first. + ids := make([]promising.PromiseID, 0, len(err)) + ids = append(ids, err[selectedIdx:]...) + ids = append(ids, err[:selectedIdx]...) + var nameList strings.Builder + for _, id := range ids { + name := promiseNames[id] + if name == "" { + // We should minimize the number of unnamed promises so that + // we can typically say at least something useful about what + // objects are involved. + name = "(...)" + } + fmt.Fprintf(&nameList, "\n - %s", name) + } + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Objects depend on themselves", + fmt.Sprintf( + "The following objects in your configuration form a circular dependency chain through their references:%s\n\nTerraform uses references to decide a suitable order for visiting objects, so objects may not refer to their own results either directly or indirectly.", + nameList.String(), + ), + )) + + } + return diags +} + +// namedPromiseReporter is an interface implemented by the types in this +// package that perform asynchronous work using the promises model implemented +// by package promising, allowing discovery of user-friendly names for promises +// involved in a particular operation. +// +// We handle this as an out-of-band action so we can avoid the overhead of +// maintaining this metadata in the common case, and instead deal with it +// retroactively only in the rare case that there's a self-dependency problem +// that exhibits as a promise resolution error. +type namedPromiseReporter interface { + // reportNamedPromises calls the given callback for each promise that + // the caller is responsible for, giving a user-friendly name for + // whatever data or action that promise was responsible for. + // + // reportNamedPromises should also delegate to the same method on any + // directly-nested objects that might themselves have promises, so that + // collectPromiseNames can walk the whole tree. This should be done only + // in situations where the original reciever's implementation is itself + // acting as the physical container for the child objects, and not just + // when an object is _logically_ nested within another object. + reportNamedPromises(func(id promising.PromiseID, name string)) +} + +func collectPromiseNames(r namedPromiseReporter) map[promising.PromiseID]string { + ret := make(map[promising.PromiseID]string) + r.reportNamedPromises(func(id promising.PromiseID, name string) { + if id != promising.NoPromise { + ret[id] = name + } + }) + return ret +} diff --git a/internal/stacks/stackruntime/internal/stackeval/input_variable_config.go b/internal/stacks/stackruntime/internal/stackeval/input_variable_config.go index 37b38ebc30..62b040b480 100644 --- a/internal/stacks/stackruntime/internal/stackeval/input_variable_config.go +++ b/internal/stacks/stackruntime/internal/stackeval/input_variable_config.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/ext/typeexpr" + "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/tfdiags" @@ -23,6 +24,7 @@ type InputVariableConfig struct { var _ Validatable = (*InputVariableConfig)(nil) var _ Referenceable = (*InputVariableConfig)(nil) +var _ namedPromiseReporter = (*InputVariableConfig)(nil) func newInputVariableConfig(main *Main, addr stackaddrs.ConfigInputVariable, config *stackconfig.InputVariable) *InputVariableConfig { return &InputVariableConfig{ @@ -133,3 +135,8 @@ func (v *InputVariableConfig) Validate(ctx context.Context) tfdiags.Diagnostics ) return diags } + +// reportNamedPromises implements namedPromiseReporter. +func (s *InputVariableConfig) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + // Nothing to report yet +} diff --git a/internal/stacks/stackruntime/internal/stackeval/main.go b/internal/stacks/stackruntime/internal/stackeval/main.go index d67683690d..788b96373d 100644 --- a/internal/stacks/stackruntime/internal/stackeval/main.go +++ b/internal/stacks/stackruntime/internal/stackeval/main.go @@ -5,6 +5,7 @@ import ( "fmt" "sync" + "github.com/hashicorp/terraform/internal/promising" "github.com/hashicorp/terraform/internal/stacks/stackaddrs" "github.com/hashicorp/terraform/internal/stacks/stackconfig" ) @@ -29,6 +30,8 @@ type Main struct { mainStackConfig *StackConfig } +var _ namedPromiseReporter = (*Main)(nil) + type mainPlanning struct { opts PlanOpts } @@ -84,3 +87,12 @@ func (m *Main) StackCallConfig(ctx context.Context, addr stackaddrs.ConfigStackC } return caller.StackCall(ctx, addr.Item) } + +// reportNamedPromises implements namedPromiseReporter. +func (m *Main) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + m.mu.Lock() + defer m.mu.Unlock() + if m.mainStackConfig != nil { + m.mainStackConfig.reportNamedPromises(cb) + } +} diff --git a/internal/stacks/stackruntime/internal/stackeval/stack_call_config.go b/internal/stacks/stackruntime/internal/stackeval/stack_call_config.go index 8cf2682a04..5a07eda2fb 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack_call_config.go +++ b/internal/stacks/stackruntime/internal/stackeval/stack_call_config.go @@ -25,8 +25,9 @@ type StackCallConfig struct { inputVariableValues promising.Once[withDiagnostics[map[stackaddrs.InputVariable]cty.Value]] } -var _ Validatable = (*InputVariableConfig)(nil) +var _ Validatable = (*StackCallConfig)(nil) var _ ExpressionScope = (*StackCallConfig)(nil) +var _ namedPromiseReporter = (*StackCallConfig)(nil) func newStackCallConfig(main *Main, addr stackaddrs.ConfigStackCall, config *stackconfig.EmbeddedStack) *StackCallConfig { return &StackCallConfig{ @@ -199,3 +200,8 @@ func (s *StackCallConfig) Validate(ctx context.Context) tfdiags.Diagnostics { ) return diags } + +// reportNamedPromises implements namedPromiseReporter. +func (s *StackCallConfig) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + cb(s.inputVariableValues.PromiseID(), s.Addr().String()+" inputs") +} diff --git a/internal/stacks/stackruntime/internal/stackeval/stack_config.go b/internal/stacks/stackruntime/internal/stackeval/stack_config.go index 52e7f2f663..3f9b9594a3 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack_config.go +++ b/internal/stacks/stackruntime/internal/stackeval/stack_config.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/hcl/v2" "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/stackconfig" "github.com/hashicorp/terraform/internal/tfdiags" @@ -34,6 +35,7 @@ type StackConfig struct { } var _ ExpressionScope = (*StackConfig)(nil) +var _ namedPromiseReporter = (*StackConfig)(nil) func newStackConfig(main *Main, addr stackaddrs.Stack, config *stackconfig.ConfigNode) *StackConfig { return &StackConfig{ @@ -179,3 +181,19 @@ func (s *StackConfig) resolveExpressionReference(ctx context.Context, ref stacka return nil, diags } } + +// reportNamedPromises implements namedPromiseReporter. +func (s *StackConfig) reportNamedPromises(cb func(id promising.PromiseID, name string)) { + s.mu.Lock() + defer s.mu.Unlock() + + for _, child := range s.children { + child.reportNamedPromises(cb) + } + for _, child := range s.inputVariables { + child.reportNamedPromises(cb) + } + for _, child := range s.stackCalls { + child.reportNamedPromises(cb) + } +}