diff --git a/internal/lang/eval/config_prepare.go b/internal/lang/eval/config_prepare.go index 3c96c10ef1..ea76383cb2 100644 --- a/internal/lang/eval/config_prepare.go +++ b/internal/lang/eval/config_prepare.go @@ -8,11 +8,13 @@ package eval import ( "context" + "github.com/zclconf/go-cty/cty" + "github.com/opentofu/opentofu/internal/addrs" "github.com/opentofu/opentofu/internal/lang/eval/internal/configgraph" + "github.com/opentofu/opentofu/internal/lang/grapheval" "github.com/opentofu/opentofu/internal/plans/objchange" "github.com/opentofu/opentofu/internal/tfdiags" - "github.com/zclconf/go-cty/cty" ) // PrepareToPlan implements an extra preparation phase we perform before @@ -37,13 +39,31 @@ import ( // evaluate using real resource planning results. The planning phase will // then be able to produce its own tighter version of this information to // use when building the execution graph for the apply phase. -// -// This must be called with a [context.Context] that's associated with a -// [grapheval.Worker]. func (c *ConfigInstance) PrepareToPlan(ctx context.Context) (*ResourceRelationships, tfdiags.Diagnostics) { + // All of our work will be associated with a workgraph worker that serves + // as the initial worker node in the work graph. + ctx = grapheval.ContextWithNewWorker(ctx) + rootModuleInstance, diags := c.precheckedModuleInstance(ctx) - ret := &ResourceRelationships{} - _ = rootModuleInstance // TODO: Actually analyze this to find the information to return + if diags.HasErrors() { + return nil, diags + } + ret := &ResourceRelationships{ + EphemeralResourceUsers: addrs.MakeMap[addrs.AbsResourceInstance, addrs.Set[addrs.AbsResourceInstance]](), + } + for depender := range rootModuleInstance.ResourceInstancesDeep(ctx) { + dependerAddr := depender.Addr + for dependee := range depender.ResourceInstanceDependencies(ctx) { + dependeeAddr := dependee.Addr + if dependeeAddr.Resource.Resource.Mode == addrs.EphemeralResourceMode { + if !ret.EphemeralResourceUsers.Has(dependeeAddr) { + ret.EphemeralResourceUsers.Put(dependeeAddr, addrs.MakeSet[addrs.AbsResourceInstance]()) + } + set := ret.EphemeralResourceUsers.Get(dependeeAddr) + set.Add(dependerAddr) + } + } + } return ret, diags } diff --git a/internal/lang/eval/config_prepare_test.go b/internal/lang/eval/config_prepare_test.go new file mode 100644 index 0000000000..85aeb55bb0 --- /dev/null +++ b/internal/lang/eval/config_prepare_test.go @@ -0,0 +1,186 @@ +// Copyright (c) The OpenTofu Authors +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2023 HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package eval_test + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" + + "github.com/opentofu/opentofu/internal/addrs" + "github.com/opentofu/opentofu/internal/configs" + "github.com/opentofu/opentofu/internal/configs/configschema" + "github.com/opentofu/opentofu/internal/lang/eval" + "github.com/opentofu/opentofu/internal/providers" +) + +func TestPrepare_ephemeralResourceUsers(t *testing.T) { + configInst, diags := eval.NewConfigInstance(t.Context(), &eval.ConfigCall{ + EvalContext: &eval.EvalContext{ + Modules: eval.ModulesForTesting(map[addrs.ModuleSourceLocal]*configs.Module{ + addrs.ModuleSourceLocal("."): configs.ModuleFromStringForTesting(t, ` + terraform { + required_providers { + foo = { + source = "test/foo" + } + } + } + ephemeral "foo" "a" { + count = 2 + + name = "a ${count.index}" + } + ephemeral "foo" "b" { + count = 2 + + name = ephemeral.foo.a[count.index].id + } + locals { + # This is intentionally a more complex expression + # to analyze, to prove that we can still chase the + # instance-specific references through it. + # This produces a tuple of two-element tuples with the + # corresponding ids of ephemeral.foo.a and + # ephemeral.foo.b respectively. + together = [ + for i, a in ephemeral.foo.a : + [a.id, ephemeral.foo.b[i].id] + ] + } + resource "foo" "c" { + count = 2 + + # Even indirectly through this projection of values + # from the two ephemeral resources we should correctly + # detect that foo.c instances are correlated with + # ephemeral.foo.a and ephemeral.foo.b instances of + # the same index. + something = local.together[count.index] + + # The above is really just an overly-complicated way of + # writing this: + # + # something = [ + # ephemeral.foo.a[count.index], + # ephemeral.foo.b[count.index], + # ] + } + `), + }), + Providers: eval.ProvidersForTesting(map[addrs.Provider]*providers.GetProviderSchemaResponse{ + addrs.MustParseProviderSourceString("test/foo"): { + Provider: providers.Schema{ + Block: &configschema.Block{}, + }, + EphemeralResources: map[string]providers.Schema{ + "foo": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Required: true, + }, + "id": { + Type: cty.String, + Computed: true, + }, + }, + }, + }, + }, + ResourceTypes: map[string]providers.Schema{ + "foo": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "something": { + Type: cty.List(cty.String), + Optional: true, + WriteOnly: true, + }, + }, + }, + }, + }, + }, + }), + }, + RootModuleSource: addrs.ModuleSourceLocal("."), + InputValues: eval.InputValuesForTesting(map[string]cty.Value{}), + }) + if diags.HasErrors() { + t.Fatalf("unexpected errors: %s", diags.Err()) + } + + got, diags := configInst.PrepareToPlan(t.Context()) + if diags.HasErrors() { + t.Fatalf("unexpected errors: %s", diags.Err()) + } + + fooA := addrs.Resource{ + Mode: addrs.EphemeralResourceMode, + Type: "foo", + Name: "a", + }.Absolute(addrs.RootModuleInstance) + fooB := addrs.Resource{ + Mode: addrs.EphemeralResourceMode, + Type: "foo", + Name: "b", + }.Absolute(addrs.RootModuleInstance) + fooC := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "foo", + Name: "c", + }.Absolute(addrs.RootModuleInstance) + inst0 := addrs.IntKey(0) + inst1 := addrs.IntKey(1) + + // The analysis should detect that: + // - ephemeral.foo.a[0] is used by ephemeral.foo.b[0] and foo.c[0] + // - ephemeral.foo.a[1] is used by ephemeral.foo.b[1] and foo.c[1] + // - ephemeral.foo.b[0] is used by only foo.c[0] + // - ephemeral.foo.b[1] is used by only foo.c[1] + // In particular, the evaluator should be able to notice that + // only the correlated instance keys have any relationship between + // them, and so e.g. ephemeral.foo.a[0] is NOT used by ephemeral.foo.b[1]. + // + // This level of precision was not possible in the traditional + // "package tofu" language runtime, because it calculated dependencies + // based only on static analysis, but this new evaluator uses dynamic + // analysis. Refer to [configgraph.ContributingResourceInstances] + // to learn more about how that's meant to work, if you're trying to + // debug a regression here that made the analysis less precise. + want := &eval.ResourceRelationships{ + // Note that this field captures _inverse_ dependencies: the values + // are instances that depend on the keys. + // + // The best way to understand this is that the ephemeral resource + // instance identified in an element's key must remain "open" until all + // of the instances identified in the element's value have finished + // planning. + EphemeralResourceUsers: addrs.MakeMap( + addrs.MakeMapElem(fooA.Instance(inst0), addrs.MakeSet( + fooB.Instance(inst0), + fooC.Instance(inst0), + )), + addrs.MakeMapElem(fooA.Instance(inst1), addrs.MakeSet( + fooB.Instance(inst1), + fooC.Instance(inst1), + )), + addrs.MakeMapElem(fooB.Instance(inst0), addrs.MakeSet( + fooC.Instance(inst0), + )), + addrs.MakeMapElem(fooB.Instance(inst1), addrs.MakeSet( + fooC.Instance(inst1), + )), + ), + } + + if diff := cmp.Diff(want, got); diff != "" { + t.Error("wrong result\n" + diff) + } +} diff --git a/internal/lang/eval/internal/configgraph/instances.go b/internal/lang/eval/internal/configgraph/instances.go index 02823308e1..b8eac13976 100644 --- a/internal/lang/eval/internal/configgraph/instances.go +++ b/internal/lang/eval/internal/configgraph/instances.go @@ -106,6 +106,22 @@ func compileInstances[T any]( ValueMarks: valueMarks, } for key, repData := range insts { + // We transfer valueMarks to the each.key, each.value and count.index + // values because valueMarks are the marks on whatever value we used to + // decide which instances exist, and so any reference to those + // should carry along mark information. + // In particular this propagates resource instance dependency + // information in case the count or for_each expression was derived + // from resource instance attributes. + if repData.EachKey != cty.NilVal { + repData.EachKey = repData.EachKey.WithMarks(valueMarks) + } + if repData.EachValue != cty.NilVal { + repData.EachValue = repData.EachValue.WithMarks(valueMarks) + } + if repData.CountIndex != cty.NilVal { + repData.CountIndex = repData.CountIndex.WithMarks(valueMarks) + } obj := compileInstance(ctx, key, repData) ret.Instances[key] = obj } @@ -127,10 +143,10 @@ func compilePlaceholderInstance[T any]( repData := instances.RepetitionData{} switch keyType { case addrs.StringKeyType: - repData.EachKey = cty.UnknownVal(cty.String) - repData.EachValue = cty.DynamicVal + repData.EachKey = cty.UnknownVal(cty.String).WithMarks(valueMarks) + repData.EachValue = cty.DynamicVal.WithMarks(valueMarks) case addrs.IntKeyType: - repData.CountIndex = cty.UnknownVal(cty.Number) + repData.CountIndex = cty.UnknownVal(cty.Number).WithMarks(valueMarks) } key := addrs.WildcardKey{keyType} placeholder := compileInstance(ctx, key, repData) diff --git a/internal/lang/eval/internal/configgraph/module_instance.go b/internal/lang/eval/internal/configgraph/module_instance.go index c62e6115ab..dcd326d956 100644 --- a/internal/lang/eval/internal/configgraph/module_instance.go +++ b/internal/lang/eval/internal/configgraph/module_instance.go @@ -8,6 +8,7 @@ package configgraph import ( "context" "fmt" + "iter" "github.com/apparentlymart/go-workgraph/workgraph" "github.com/hashicorp/hcl/v2" @@ -118,6 +119,31 @@ func (m *ModuleInstance) ValueSourceRange() *tfdiags.SourceRange { return m.CallDeclRange } +// ResourceInstancesDeep returns a sequence of all of the resource instances +// declared both in this module instance and across all child resource +// instances. +// +// The result is trustworthy only if [ModuleInstance.CheckAll] returns without +// errors. When errors are present the result is best-effort and likely to +// be incomplete. +func (m *ModuleInstance) ResourceInstancesDeep(ctx context.Context) iter.Seq[*ResourceInstance] { + return func(yield func(*ResourceInstance) bool) { + for _, r := range m.ResourceNodes { + // NOTE: r.Instances will block if the resource's [InstanceSelector] + // depends on other parts of the configuration that aren't yet + // ready to produce their value. + for _, inst := range r.Instances(ctx) { + if !yield(inst) { + return + } + } + } + + // TODO: Once we actually support child module calls, ask for the + // instances of each one and then collect its resource instances too. + } +} + // CheckAll visits this module and everything it contains to drive evaluation // of all of the expressions in the configuration and collect any diagnostics // they return. diff --git a/internal/lang/eval/internal/configgraph/resource_instance.go b/internal/lang/eval/internal/configgraph/resource_instance.go index e35f7a6c70..6ad76f8f2c 100644 --- a/internal/lang/eval/internal/configgraph/resource_instance.go +++ b/internal/lang/eval/internal/configgraph/resource_instance.go @@ -8,6 +8,7 @@ package configgraph import ( "context" "fmt" + "iter" "github.com/apparentlymart/go-workgraph/workgraph" "github.com/hashicorp/hcl/v2" @@ -78,6 +79,15 @@ func (ri *ResourceInstance) StaticCheckTraversal(traversal hcl.Traversal) tfdiag // Value implements exprs.Valuer. func (ri *ResourceInstance) Value(ctx context.Context) (cty.Value, tfdiags.Diagnostics) { + // TODO: Preconditions? Or should that be handled in the parent [Resource] + // before we even attempt instance expansion? (Need to check the current + // behavior in the existing system, to see whether preconditions guard + // instance expansion.) + // If we take preconditions into account here then we must transfer + // [ResourceInstanceMark] marks from the check rule expressions into + // configVal because config evaluation indirectly depends on those + // references. + // We use the configuration value here only for its marks, since that // allows us to propagate any configVal, diags := ri.ConfigValuer.Value(ctx) @@ -97,11 +107,55 @@ func (ri *ResourceInstance) Value(ctx context.Context) (cty.Value, tfdiags.Diagn // function to do. resultVal, diags := ri.GetResultValue(ctx, configVal) + // TODO: Postconditions, and transfer [ResourceInstanceMark] marks from + // the check rule expressions onto resultVal because the presence of + // a valid result value indirectly depends on those references. + // The result needs some additional preparation to make sure it's // marked correctly for ongoing use in other expressions. return prepareResourceInstanceResult(resultVal, ri, configVal), diags } +// ResourceInstanceDependencies returns a sequence of any other resource +// instances whose results this resource instance depends on. +// +// The result of this is trustworthy only if [ResourceInstance.CheckAll] +// returns without diagnostics. If errors are present then the result is +// best-effort but likely to be incomplete. +func (ri *ResourceInstance) ResourceInstanceDependencies(ctx context.Context) iter.Seq[*ResourceInstance] { + // FIXME: This should also take into account: + // - indirect references through the configuration of the provider instance + // this resource instance uses (which should arrive as marks on the + // [ProviderInstanceRefType] value that represents the provider instance), + // once we've actually got a Valuer to return the provider instance + // reference value. + // - explicit dependencies in the depends_on argument + // - ....anything else? + // + // We should NOT need to take into account dependencies of the parent + // resource's InstanceSelector because substitutions of + // count.index/each.key/each.value will transfer those in automatically by + // the RepetitionData values being marked. + + // We ignore diagnostics here because callers should always perform a + // CheckAll tree walk, including a visit to this resource instance object, + // before trusting anything else that any configgraph nodes report. + resultVal := diagsHandledElsewhere(ri.Value(ctx)) + + // Our Value method always marks its result as depending on this + // resource instance so that any expressions that refer to it will + // be treated as depending on this resource instance, but we want + // to filter that out here because otherwise we'd be reporting that + // this resource depends on itself, which is impossible and confusing. + return func(yield func(*ResourceInstance) bool) { + for depInst := range ContributingResourceInstances(resultVal) { + if depInst != ri { + yield(depInst) + } + } + } +} + // ValueSourceRange implements exprs.Valuer. func (ri *ResourceInstance) ValueSourceRange() *tfdiags.SourceRange { return ri.ConfigValuer.ValueSourceRange() diff --git a/internal/lang/exprs/attribute.go b/internal/lang/exprs/attribute.go index 64eb993a8d..92c391c936 100644 --- a/internal/lang/exprs/attribute.go +++ b/internal/lang/exprs/attribute.go @@ -32,6 +32,42 @@ func ValueOf(v Valuer) Attribute { return valueOf{v} } +// ### Regarding the possibility of instances of one resource being able to ### +// ### refer to each other without that being treated as a "self-reference" ### +// ### error... ### +// +// TODO: We could potentially have two more implementations of [Attribute] here +// which represent object and tuple values (respectively) that can potentially +// be partially-constructed when all of the references include a known +// index step underneath the attribute, but behave like a normal +// [ValueOf] if there's at least one reference that doesn't include a known +// index step. In principle that could be used for multi-instance objects +// like resources to allow instances to refer to each other without it being +// treated as a self-reference. +// +// If the hcl.EvalContext builder has known index steps then it can build +// an object or tuple where any indices not accessed are either not populated +// at all (for an object) or set to [cty.DynamicVal] (for a tuple, where we +// need to populate any "gaps" between the indices being used). +// +// However, there's various other groundwork we'd need to do before we could +// make that work, including but probably not limited to: +// - Have some alternative to hcl.Traversal that can support index steps whose +// keys are [Valuer] instead of static [cty.Value], so that a reference +// like aws_instance.example[each.key] can have that each.key evaluated +// as part of preparing the hcl.EvalContext and we can dynamically decide +// which individual index to populate to satisfy that reference. +// - Some way to make sure that any marks that would normally be placed on +// naked aws_instance.example still get applied to the result even when +// we skip calling the [Valuer] for aws_instance.example as a whole. +// +// As long as we're using [hcl.Traversal] in its current form we would only +// be able to do this partial-building trick when the index key is a constant, +// like in aws_instance.example["foo"], but that's such an unusual situation +// to be not worth the complexity of handling it. It would only be worth it +// if we could support dynamic index keys, so that e.g. a single resource can +// represent a tree by having child nodes refer to the ids of their parents. + // NestedSymbolTableFromAttribute returns the symbol table from an attribute // that was returned from [NestedSymbolTable], or nil for any other kind of // attribute.