diff --git a/internal/addrs/module_call.go b/internal/addrs/module_call.go index 65474d8132da..162b05a6f6be 100644 --- a/internal/addrs/module_call.go +++ b/internal/addrs/module_call.go @@ -59,6 +59,23 @@ func (c AbsModuleCall) absMoveableSigil() { // AbsModuleCall is "moveable". } +// StaticModule returns the static module path for the receiver. +// +// In other words, it effectively discards all of the dynamic instance keys +// along the path to this call, while retaining the static module names. +// +// Given a representation of module.a["foo"].module.b, this would return +// the [Module]-based representation of module.a.module.b, discarding the +// first step's dynamic instance key "foo". +func (c AbsModuleCall) StaticModule() Module { + ret := make(Module, len(c.Module), len(c.Module)+1) + for i, step := range c.Module { + ret[i] = step.Name + } + ret = append(ret, c.Call.Name) + return ret +} + func (c AbsModuleCall) String() string { if len(c.Module) == 0 { return "module." + c.Call.Name diff --git a/internal/addrs/module_instance.go b/internal/addrs/module_instance.go index 690e14fd2456..a0e06b839eb7 100644 --- a/internal/addrs/module_instance.go +++ b/internal/addrs/module_instance.go @@ -391,6 +391,17 @@ func (m ModuleInstance) Call() (ModuleInstance, ModuleCall) { } } +// AbsCall returns the same information as [ModuleInstance.Call], but returns +// it as a single [AbsModuleCall] value rather than the containing module +// and the local call address separately. +func (m ModuleInstance) AbsCall() AbsModuleCall { + container, call := m.Call() + return AbsModuleCall{ + Module: container, + Call: call, + } +} + // CallInstance returns the module call instance address that corresponds to // the given module instance, along with the address of the module instance // that contains it. diff --git a/internal/instances/expander.go b/internal/instances/expander.go index 8dcd378f884d..497daae2f62a 100644 --- a/internal/instances/expander.go +++ b/internal/instances/expander.go @@ -135,6 +135,45 @@ func (e *Expander) ExpandModule(addr addrs.Module) []addrs.ModuleInstance { return e.expandModule(addr, false) } +// ExpandAbsModuleCall is similar to [Expander.ExpandModule] except that it +// filters the result to include only the instances that belong to the +// given module call instance, and therefore returns just instance keys +// since the rest of the module address is implied by the given argument. +// +// For example, passing an address representing module.a["foo"].module.b +// would include only instances under module.a["foo"], and disregard instances +// under other dynamic paths like module.a["bar"]. +// +// If the requested module call has an unknown expansion (e.g. because it +// had an unknown value for count or for_each) then the second result is +// false and the other results are meaningless. If the second return value is +// true, then the set of module instances is complete, and all of the instances +// have instance keys matching the returned keytype. +// +// The instances are returned in the typical sort order for the returned +// key type: integer keys are sorted numerically, and string keys are sorted +// lexically. +func (e *Expander) ExpandAbsModuleCall(addr addrs.AbsModuleCall) (keyType addrs.InstanceKeyType, insts []addrs.InstanceKey, known bool) { + expParent, ok := e.findModule(addr.Module) + if !ok { + // This module call lives under an unknown-expansion prefix, so we + // cannot answer this question. + return addrs.NoKeyType, nil, false + } + + expCall, ok := expParent.moduleCalls[addr.Call] + if !ok { + // This indicates a bug, since we should've calculated the expansions + // (even if unknown) before any caller asks for the results. + panic(fmt.Sprintf("no expansion has been registered for %s", addr.String())) + } + keyType, instKeys, deferred := expCall.instanceKeys() + if deferred { + return addrs.NoKeyType, nil, false + } + return keyType, instKeys, true +} + // expandModule allows skipping unexpanded module addresses by setting skipUnregistered to true. // This is used by instances.Set, which is only concerned with the expanded // instances, and should not panic when looking up unknown addresses. diff --git a/internal/instances/expander_test.go b/internal/instances/expander_test.go index 74cae227ed9e..a06a4da13208 100644 --- a/internal/instances/expander_test.go +++ b/internal/instances/expander_test.go @@ -297,6 +297,24 @@ func TestExpander(t *testing.T) { t.Errorf("wrong result\n%s", diff) } }) + t.Run("module count2[0] module count2 instances", func(t *testing.T) { + instAddr := mustModuleInstanceAddr(`module.count2[0].module.count2[0]`) + callAddr := instAddr.AbsCall() // discards the final [0] instance key from the above + keyType, got, known := ex.ExpandAbsModuleCall(callAddr) + if !known { + t.Fatal("expansion unknown; want known") + } + if keyType != addrs.IntKeyType { + t.Fatalf("wrong key type %#v; want %#v", keyType, addrs.IntKeyType) + } + want := []addrs.InstanceKey{ + addrs.IntKey(0), + addrs.IntKey(1), + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("wrong result\n%s", diff) + } + }) t.Run("module count2 module count2 GetDeepestExistingModuleInstance", func(t *testing.T) { t.Run("first step invalid", func(t *testing.T) { got := ex.GetDeepestExistingModuleInstance(mustModuleInstanceAddr(`module.count2["nope"].module.count2[0]`)) @@ -596,6 +614,15 @@ func TestExpanderWithUnknowns(t *testing.T) { ex.SetResourceCount(module1Inst1Module2Inst0, resourceAddrKnownExp, 2) ex.SetResourceCountUnknown(module1Inst1Module2Inst0, resourceAddrUnknownExp) + module2Call := addrs.AbsModuleCall{ + Module: module1Inst0, + Call: moduleCallAddr2, + } + _, _, instsKnown := ex.ExpandAbsModuleCall(module2Call) + if instsKnown { + t.Fatalf("instances of %s are known; should be unknown", module2Call.String()) + } + gotKnown := ex.ExpandModule(module2) wantKnown := []addrs.ModuleInstance{ module1Inst1.Child("bar", addrs.IntKey(0)), diff --git a/internal/namedvals/state.go b/internal/namedvals/state.go index 0b4a2ea5aa7c..a96f10c6e3b1 100644 --- a/internal/namedvals/state.go +++ b/internal/namedvals/state.go @@ -107,54 +107,6 @@ func (s *State) GetOutputValue(addr addrs.AbsOutputValue) cty.Value { return s.outputs.GetExactResult(addr) } -func (s *State) GetOutputValuesForModuleCall(parentAddr addrs.ModuleInstance, callAddr addrs.ModuleCall) addrs.Map[addrs.AbsOutputValue, cty.Value] { - s.mu.Lock() - defer s.mu.Unlock() - - // HACK: The "values" data structure isn't really designed to support - // this operation, since it tries to be general over all different named - // value address types but that makes it unable to generically handle - // the problem of finding the module instance for a particular absolute - // address. We'd need a ModuleInstance equivalent of - // addrs.InPartialExpandedModule to achieve that, but our "Abs" address - // types are all hand-written and predate Go having support for generic - // types. - // - // This operation is just a stop-gap until we make the evaluator work - // in a different way to handle placeholder values, so we'll accept it - // being clunky and slow just as a checkpoint to make everything still - // work similarly to how it used to, and then delete this function again - // later once we can implement what we need using just - // [State.GetOutputValue] by having the caller determine which output - // values it should be asking for using the configuration. - - ret := addrs.MakeMap[addrs.AbsOutputValue, cty.Value]() - all := s.outputs.GetExactResults() - - for _, elem := range all.Elems { - outputMod := elem.Key.Module - if outputMod.IsRoot() { - // We cannot enumerate the root module output values with this - // function, because the root module has no "call". - continue - } - callingMod, call := outputMod.Call() - if call != callAddr { - continue - } - if !callingMod.Equal(parentAddr) { - continue - } - - // If we get here then the output value we're holding belongs to - // one of the instances of the call indicated in this function's - // arguments. - ret.PutElement(elem) - } - - return ret -} - func (s *State) HasOutputValue(addr addrs.AbsOutputValue) bool { s.mu.Lock() defer s.mu.Unlock() diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index bbda671e8e91..5a10dc7ee9b5 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -39,6 +39,14 @@ type Evaluator struct { // Config is the root node in the configuration tree. Config *configs.Config + // Instances tracks the dynamic instances that are associated with each + // module call or resource. The graph walk gradually registers the + // set of instances for each object within the graph nodes for those + // objects, and so as long as the graph has been built correctly the + // set of instances for an object should always be available by the time + // we're evaluating expressions that refer to it. + Instances *instances.Expander + // NamedValues is where we keep the values of already-evaluated input // variables, local values, and output values. NamedValues *namedvals.State @@ -314,6 +322,7 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc // Output results live in the module that declares them, which is one of // the child module instances of our current module path. moduleAddr := d.ModulePath.Module().Child(addr.Name) + absAddr := addr.Absolute(d.ModulePath) parentCfg := d.Evaluator.Config.DescendentForInstance(d.ModulePath) callConfig, ok := parentCfg.Module.ModuleCalls[addr.Name] @@ -338,193 +347,126 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc } outputConfigs := moduleConfig.Module.Outputs - // Collect all the relevant outputs that current exist in the state. - // We know the instance path up to this point, and the child module name, - // so we only need to store these by instance key. - stateMap := map[addrs.InstanceKey]map[string]cty.Value{} - for _, elem := range d.Evaluator.NamedValues.GetOutputValuesForModuleCall(d.ModulePath, addr).Elems { - outputAddr := elem.Key - val := elem.Value - - _, callInstance := outputAddr.Module.CallInstance() - instance, ok := stateMap[callInstance.Key] - if !ok { - instance = map[string]cty.Value{} - stateMap[callInstance.Key] = instance - } - - instance[outputAddr.OutputValue.Name] = val - } - - // Get all changes that reside for this module call within our path. - // The change contains the full addr, so we can key these with strings. - changesMap := map[addrs.InstanceKey]map[string]*plans.OutputChangeSrc{} - for _, change := range d.Evaluator.Changes.GetOutputChanges(d.ModulePath, addr) { - _, callInstance := change.Addr.Module.CallInstance() - instance, ok := changesMap[callInstance.Key] - if !ok { - instance = map[string]*plans.OutputChangeSrc{} - changesMap[callInstance.Key] = instance + // We don't do instance expansion during validation, and so we need to + // return an unknown value. Technically we should always return + // cty.DynamicVal here because the final value during plan will always + // be an object or tuple type with unpredictable attributes/elements, + // but because we never actually carry values forward from validation to + // planning we lie a little here and return unknown list and map types, + // just to give us more opportunities to catch author mistakes during + // validation. + // + // This means that in practice any expression that refers to a module + // call must be written to be valid for either a collection type or + // structural type of similar kind, so that it can be considered as + // valid during both the validate and plan walks. + if d.Operation == walkValidate { + atys := make(map[string]cty.Type, len(outputConfigs)) + for name := range outputConfigs { + atys[name] = cty.DynamicPseudoType // output values are dynamically-typed } + instTy := cty.Object(atys) - instance[change.Addr.OutputValue.Name] = change + switch { + case callConfig.Count != nil: + return cty.UnknownVal(cty.List(instTy)), diags + case callConfig.ForEach != nil: + return cty.UnknownVal(cty.Map(instTy)), diags + default: + return cty.UnknownVal(instTy), diags + } + } + + // For all other walk types, we proceed to dynamic evaluation of individual + // instances, using the global instance expander. An earlier graph node + // should always have registered the expansion of this module call before + // we get here, unless there's a bug in the graph builders. + allInstances := d.Evaluator.Instances + instKeyType, instKeys, known := allInstances.ExpandAbsModuleCall(absAddr) + if !known { + // If we don't know which instances exist then we can't really predict + // anything at all. We can't even predict the return type based on + // instKeyType because output values are dynamically-typed and so + // our final result will always be an object or tuple type whose + // attribute/element count we cannot predict. + return cty.DynamicVal, diags } - // Build up all the module objects, creating a map of values for each - // module instance. - moduleInstances := map[addrs.InstanceKey]map[string]cty.Value{} - - // create a dummy object type for validation below - unknownMap := map[string]cty.Type{} - - // the structure is based on the configuration, so iterate through all the - // defined outputs, and add any instance state or changes we find. - for _, cfg := range outputConfigs { - // record the output names for validation - unknownMap[cfg.Name] = cty.DynamicPseudoType - - // get all instance output for this path from the state - for key, states := range stateMap { - outputState, ok := states[cfg.Name] - if !ok { + instanceObjVal := func(instKey addrs.InstanceKey) (cty.Value, tfdiags.Diagnostics) { + // This function must always return a valid value, even if it's + // just a cty.DynamicVal placeholder accompanying error diagnostics. + var diags tfdiags.Diagnostics + + namedVals := d.Evaluator.NamedValues + moduleInstAddr := absAddr.Instance(instKey) + attrs := make(map[string]cty.Value, len(outputConfigs)) + for name := range outputConfigs { + outputAddr := moduleInstAddr.OutputValue(name) + + // Although we do typically expect the graph dependencies to + // ensure that values get registered before they are needed, + // we track depedencies with specific output values where + // possible, instead of with entire module calls, and so + // in this specific case it's valid for some of this call's + // output values to not be known yet, with the graph builder + // being responsible for making sure that no expression + // in the configuration can actually observe that. + if !namedVals.HasOutputValue(outputAddr) { + attrs[name] = cty.DynamicVal continue } - - instance, ok := moduleInstances[key] - if !ok { - instance = map[string]cty.Value{} - moduleInstances[key] = instance - } - - instance[cfg.Name] = outputState + outputVal := namedVals.GetOutputValue(outputAddr) + attrs[name] = outputVal } - // any pending changes override the state state values - for key, changes := range changesMap { - changeSrc, ok := changes[cfg.Name] - if !ok { - continue - } - - instance, ok := moduleInstances[key] - if !ok { - instance = map[string]cty.Value{} - moduleInstances[key] = instance - } - - change, err := changeSrc.Decode() - if err != nil { - // This should happen only if someone has tampered with a plan - // file, so we won't bother with a pretty error for it. - diags = diags.Append(fmt.Errorf("planned change for %s could not be decoded: %s", addr, err)) - instance[cfg.Name] = cty.DynamicVal - continue - } - - instance[cfg.Name] = change.After - - if change.Sensitive { - instance[cfg.Name] = change.After.Mark(marks.Sensitive) - } - } + return cty.ObjectVal(attrs), diags } - var ret cty.Value + switch instKeyType { - // compile the outputs into the correct value type for the each mode - switch { - case callConfig.Count != nil: - // figure out what the last index we have is - length := -1 - for key := range moduleInstances { - intKey, ok := key.(addrs.IntKey) - if !ok { - // old key from state which is being dropped - continue - } - if int(intKey) >= length { - length = int(intKey) + 1 - } + case addrs.NoKeyType: + // In this case we should always have exactly one instance that + // is addrs.NoKey. If not then there's a bug in the [instances.Expander] + // implementation. + if len(instKeys) != 1 { + panic(fmt.Sprintf("module call has no instance key type but has %d instances (should be 1)", len(instKeys))) } + ret, moreDiags := instanceObjVal(instKeys[0]) + diags = diags.Append(moreDiags) + return ret, diags - if length > 0 { - vals := make([]cty.Value, length) - for key, instance := range moduleInstances { - intKey, ok := key.(addrs.IntKey) - if !ok { - // old key from state which is being dropped - continue - } - - vals[int(intKey)] = cty.ObjectVal(instance) - } - - // Insert unknown values where there are any missing instances - for i, v := range vals { - if v.IsNull() { - vals[i] = cty.DynamicVal - continue - } - } - ret = cty.TupleVal(vals) - } else { - ret = cty.EmptyTupleVal + case addrs.IntKeyType: + // We can assume that the instance keys are in ascending numerical order + // and are consecutive, per the contract of allInstances.ExpandModuleCall. + elems := make([]cty.Value, 0, len(instKeys)) + for _, instKey := range instKeys { + instVal, moreDiags := instanceObjVal(instKey) + elems = append(elems, instVal) + diags = diags.Append(moreDiags) } + return cty.TupleVal(elems), diags - case callConfig.ForEach != nil: - vals := make(map[string]cty.Value) - for key, instance := range moduleInstances { - strKey, ok := key.(addrs.StringKey) - if !ok { - continue - } - - vals[string(strKey)] = cty.ObjectVal(instance) - } - - if len(vals) > 0 { - ret = cty.ObjectVal(vals) - } else { - ret = cty.EmptyObjectVal + case addrs.StringKeyType: + attrs := make(map[string]cty.Value, len(instKeys)) + for _, instKey := range instKeys { + instVal, moreDiags := instanceObjVal(instKey) + attrs[string(instKey.(addrs.StringKey))] = instVal + diags = diags.Append(moreDiags) } + return cty.ObjectVal(attrs), diags default: - val, ok := moduleInstances[addrs.NoKey] - if !ok { - // create the object if there wasn't one known - val = map[string]cty.Value{} - for k := range outputConfigs { - val[k] = cty.DynamicVal - } - } - - ret = cty.ObjectVal(val) - } - - // The module won't be expanded during validation, so we need to return an - // unknown value. This will ensure the types looks correct, since we built - // the objects based on the configuration. - if d.Operation == walkValidate { - // While we know the type here and it would be nice to validate whether - // indexes are valid or not, because tuples and objects have fixed - // numbers of elements we can't simply return an unknown value of the - // same type since we have not expanded any instances during - // validation. - // - // In order to validate the expression a little precisely, we'll create - // an unknown map or list here to get more type information. - ty := cty.Object(unknownMap) - switch { - case callConfig.Count != nil: - ret = cty.UnknownVal(cty.List(ty)) - case callConfig.ForEach != nil: - ret = cty.UnknownVal(cty.Map(ty)) - default: - ret = cty.UnknownVal(ty) - } + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Unsupported instance key type`, + Detail: fmt.Sprintf( + `Module call %s has instance key type %#v, which is not supported by the expression evaluator. This is a bug in Terraform.`, + absAddr, instKeyType, + ), + Subject: rng.ToHCL().Ptr(), + }) + return cty.DynamicVal, diags } - - return ret, diags } func (d *evaluationStateData) GetPathAttr(addr addrs.PathAttr, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) { diff --git a/internal/terraform/evaluate_test.go b/internal/terraform/evaluate_test.go index b1e028352335..cfd518f4b331 100644 --- a/internal/terraform/evaluate_test.go +++ b/internal/terraform/evaluate_test.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/lang" "github.com/hashicorp/terraform/internal/lang/marks" "github.com/hashicorp/terraform/internal/namedvals" @@ -538,15 +539,8 @@ func TestEvaluatorGetResource_changes(t *testing.T) { } func TestEvaluatorGetModule(t *testing.T) { - // Create a new evaluator with an existing state - stateSync := states.BuildState(func(ss *states.SyncState) { - ss.SetOutputValue( - addrs.OutputValue{Name: "out"}.Absolute(addrs.ModuleInstance{addrs.ModuleInstanceStep{Name: "mod"}}), - cty.StringVal("bar"), - true, - ) - }).SyncWrapper() - evaluator := evaluatorForModule(stateSync, plans.NewChanges().SyncWrapper()) + evaluator := evaluatorForModule(states.NewState().SyncWrapper(), plans.NewChanges().SyncWrapper()) + evaluator.Instances.SetModuleSingle(addrs.RootModuleInstance, addrs.ModuleCall{Name: "mod"}) evaluator.NamedValues.SetOutputValue( addrs.OutputValue{Name: "out"}.Absolute(addrs.ModuleInstance{addrs.ModuleInstanceStep{Name: "mod"}}), cty.StringVal("bar").Mark(marks.Sensitive), @@ -566,52 +560,6 @@ func TestEvaluatorGetModule(t *testing.T) { if !got.RawEquals(want) { t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, want) } - - // Changes should override the state value - changesSync := plans.NewChanges().SyncWrapper() - change := &plans.OutputChange{ - Addr: addrs.OutputValue{Name: "out"}.Absolute(addrs.ModuleInstance{addrs.ModuleInstanceStep{Name: "mod"}}), - Sensitive: true, - Change: plans.Change{ - After: cty.StringVal("baz"), - }, - } - cs, _ := change.Encode() - changesSync.AppendOutputChange(cs) - evaluator = evaluatorForModule(stateSync, changesSync) - data = &evaluationStateData{ - Evaluator: evaluator, - } - scope = evaluator.Scope(data, nil, nil, lang.ExternalFuncs{}) - want = cty.ObjectVal(map[string]cty.Value{"out": cty.StringVal("baz").Mark(marks.Sensitive)}) - got, diags = scope.Data.GetModule(addrs.ModuleCall{ - Name: "mod", - }, tfdiags.SourceRange{}) - - if len(diags) != 0 { - t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) - } - if !got.RawEquals(want) { - t.Errorf("wrong result %#v; want %#v", got, want) - } - - // Test changes with empty state - evaluator = evaluatorForModule(states.NewState().SyncWrapper(), changesSync) - data = &evaluationStateData{ - Evaluator: evaluator, - } - scope = evaluator.Scope(data, nil, nil, lang.ExternalFuncs{}) - want = cty.ObjectVal(map[string]cty.Value{"out": cty.StringVal("baz").Mark(marks.Sensitive)}) - got, diags = scope.Data.GetModule(addrs.ModuleCall{ - Name: "mod", - }, tfdiags.SourceRange{}) - - if len(diags) != 0 { - t.Errorf("unexpected diagnostics %s", spew.Sdump(diags)) - } - if !got.RawEquals(want) { - t.Errorf("wrong result %#v; want %#v", got, want) - } } func evaluatorForModule(stateSync *states.SyncState, changesSync *plans.ChangesSync) *Evaluator { @@ -643,6 +591,7 @@ func evaluatorForModule(stateSync *states.SyncState, changesSync *plans.ChangesS }, State: stateSync, Changes: changesSync, + Instances: instances.NewExpander(), NamedValues: namedvals.NewState(), } } diff --git a/internal/terraform/graph_walk_context.go b/internal/terraform/graph_walk_context.go index 769e71f10074..d55ad4d99d7f 100644 --- a/internal/terraform/graph_walk_context.go +++ b/internal/terraform/graph_walk_context.go @@ -91,6 +91,7 @@ func (w *ContextGraphWalker) EvalContext() EvalContext { State: w.State, Changes: w.Changes, Plugins: w.Context.plugins, + Instances: w.InstanceExpander, NamedValues: w.NamedValues, PlanTimestamp: w.PlanTimestamp, }