Merge pull request #35016 from hashicorp/TF-13963

stacks: handle unknown values in component for_each

internal/stacks/stackruntime/internal/stackeval/component.go

@@ -116,17 +116,6 @@ func (c *Component) CheckForEachValue(ctx context.Context, phase EvalPhase) (cty
 					return cty.DynamicVal, diags
 				}
 
-				if !result.Value.IsKnown() {
-					// FIXME: We should somehow allow this and emit a
-					// "deferred change" representing all of the as-yet-unknown
-					// instances of this call and everything beneath it.
-					diags = diags.Append(result.Diagnostic(
-						tfdiags.Error,
-						"Invalid for_each value",
-						"The for_each value must not be derived from values that will be determined only during the apply phase.",
-					))
-				}
-
 				return result.Value, diags
 
 			default:
@@ -170,11 +159,16 @@ func (c *Component) CheckInstances(ctx context.Context, phase EvalPhase) (map[ad
 		ctx, c.instances.For(phase), c.main,
 		func(ctx context.Context) (map[addrs.InstanceKey]*ComponentInstance, tfdiags.Diagnostics) {
 			var diags tfdiags.Diagnostics
-			forEachVal := c.ForEachValue(ctx, phase)
+			forEachVal, forEachValueDiags := c.CheckForEachValue(ctx, phase)
+
+			diags = diags.Append(forEachValueDiags)
+			if diags.HasErrors() {
+				return nil, diags
+			}
 
 			ret := instancesMap(forEachVal, func(ik addrs.InstanceKey, rd instances.RepetitionData) *ComponentInstance {
 				return newComponentInstance(c, ik, rd)
-			})
+			}, true)
 
 			addrs := make([]stackaddrs.AbsComponentInstance, 0, len(ret))
 			for _, ci := range ret {
@@ -257,6 +251,12 @@ func (c *Component) PlanIsComplete(ctx context.Context) bool {
 		return false
 	}
 
+	if insts[addrs.WildcardKey] != nil {
+		// If the wildcard key is used the instance originates from an unknown
+		// for_each value, which means the result is unknown.
+		return false
+	}
+
 	for _, inst := range insts {
 		plan := inst.ModuleTreePlan(ctx)
 		if plan == nil {
@@ -266,6 +266,7 @@ func (c *Component) PlanIsComplete(ctx context.Context) bool {
 			// get returned by a different return path.
 			return false
 		}
+
 		if !plan.Complete {
 			return false
 		}
@@ -283,9 +284,7 @@ func (c *Component) ExprReferenceValue(ctx context.Context, phase EvalPhase) cty
 func (c *Component) checkValid(ctx context.Context, phase EvalPhase) tfdiags.Diagnostics {
 	var diags tfdiags.Diagnostics
 
-	_, moreDiags := c.CheckForEachValue(ctx, phase)
-	diags = diags.Append(moreDiags)
-	_, moreDiags = c.CheckInstances(ctx, phase)
+	_, moreDiags := c.CheckInstances(ctx, phase)
 	diags = diags.Append(moreDiags)
 
 	return diags

internal/stacks/stackruntime/internal/stackeval/component_instance.go

@@ -558,24 +558,29 @@ func (c *ComponentInstance) CheckModuleTreePlan(ctx context.Context) (*plans.Pla
 			// If any of our upstream components have incomplete plans then
 			// we need to force treating everything in this component as
 			// deferred so we can preserve the correct dependency ordering.
-			upstreamDeferred := false
+			deferred := false
 			for _, depAddr := range c.call.RequiredComponents(ctx).Elems() {
 				depStack := c.main.Stack(ctx, depAddr.Stack, PlanPhase)
 				if depStack == nil {
-					upstreamDeferred = true // to be conservative
+					deferred = true // to be conservative
 					break
 				}
 				depComponent := depStack.Component(ctx, depAddr.Item)
 				if depComponent == nil {
-					upstreamDeferred = true // to be conservative
+					deferred = true // to be conservative
 					break
 				}
 				if !depComponent.PlanIsComplete(ctx) {
-					upstreamDeferred = true
+					deferred = true
 					break
 				}
 			}
 
+			// The instance is also upstream deferred if the for_each value for this instance is unknown.
+			if c.key == addrs.WildcardKey {
+				deferred = true
+			}
+
 			// NOTE: This ComponentInstance type only deals with component
 			// instances currently declared in the configuration. See
 			// [ComponentInstanceRemoved] for the model of a component instance
@@ -586,7 +591,7 @@ func (c *ComponentInstance) CheckModuleTreePlan(ctx context.Context) (*plans.Pla
 				SetVariables:               inputValues,
 				ExternalProviders:          providerClients,
 				DeferralAllowed:            stackPlanOpts.DeferralAllowed,
-				ExternalDependencyDeferred: upstreamDeferred,
+				ExternalDependencyDeferred: deferred,
 
 				// This is set by some tests but should not be used in main code.
 				// (nil means to use the real time when tfCtx.Plan was called.)

internal/stacks/stackruntime/internal/stackeval/component_test.go

@@ -190,15 +190,17 @@ func TestComponentCheckInstances(t *testing.T) {
 			}
 
 			// When the for_each expression is invalid, CheckInstances should
-			// return nil to represent that we don't know enough to predict
-			// how many instances there are. This is a different result than
-			// when we know there are zero instances, which would be a non-nil
-			// empty map.
+			// return nil and diagnostics.
 			gotInsts, diags := component.CheckInstances(ctx, InspectPhase)
-			assertNoDiags(t, diags)
+
 			if gotInsts != nil {
-				t.Errorf("wrong instances; want nil\n%#v", gotInsts)
+				t.Fatalf("unexpected instances\ngot:  %#v\nwant: nil", gotInsts)
 			}
+
+			assertMatchingDiag(t, diags, func(diag tfdiags.Diagnostic) bool {
+				return (diag.Severity() == tfdiags.Error &&
+					diag.Description().Detail == "The for_each expression must produce either a map of any type or a set of strings. The keys of the map or the set elements will serve as unique identifiers for multiple instances of this component.")
+			})
 		})
 		subtestInPromisingTask(t, "unknown", func(ctx context.Context, t *testing.T) {
 			main := testEvaluator(t, testEvaluatorOpts{
@@ -208,30 +210,33 @@ func TestComponentCheckInstances(t *testing.T) {
 				},
 			})
 
-			// For now it's invalid to use an unknown value in for_each.
-			// Later we're expecting to make this succeed but announce that
-			// planning everything beneath this component must be deferred to a
-			// future plan after everything else has been applied first.
 			component := getComponent(ctx, main)
 			gotVal, diags := component.CheckForEachValue(ctx, InspectPhase)
-			assertMatchingDiag(t, diags, func(diag tfdiags.Diagnostic) bool {
-				return (diag.Severity() == tfdiags.Error &&
-					diag.Description().Detail == "The for_each value must not be derived from values that will be determined only during the apply phase.")
-			})
+			assertNoDiags(t, diags)
+
 			wantVal := cty.UnknownVal(cty.Map(cty.EmptyObject))
 			if !wantVal.RawEquals(gotVal) {
 				t.Errorf("wrong result\ngot:  %#v\nwant: %#v", gotVal, wantVal)
 			}
 
-			// When the for_each expression is invalid, CheckInstances should
-			// return nil to represent that we don't know enough to predict
-			// how many instances there are. This is a different result than
-			// when we know there are zero instances, which would be a non-nil
-			// empty map.
+			// When the for_each expression is unknown, CheckInstances should
+			// return a single instance with dynamic values in the repetition data.
 			gotInsts, diags := component.CheckInstances(ctx, InspectPhase)
 			assertNoDiags(t, diags)
-			if gotInsts != nil {
-				t.Errorf("wrong instances; want nil\n%#v", gotInsts)
+			if got, want := len(gotInsts), 1; got != want {
+				t.Fatalf("wrong number of instances %d; want %d\n%#v", got, want, gotInsts)
+			}
+
+			if gotInsts[addrs.WildcardKey] == nil {
+				t.Fatalf("missing expected addrs.WildcardKey instance\n%#v", gotInsts)
+			}
+
+			if gotInsts[addrs.WildcardKey].repetition.EachKey.IsKnown() {
+				t.Errorf("EachKey should be unknown, but is known")
+			}
+
+			if gotInsts[addrs.WildcardKey].repetition.EachValue.IsKnown() {
+				t.Errorf("EachValue should be unknown, but is known")
 			}
 		})
 	})
@@ -388,16 +393,13 @@ func TestComponentResultValue(t *testing.T) {
 			component := getComponent(ctx, t, main)
 			got := component.ResultValue(ctx, InspectPhase)
 			// When the for_each expression is unknown, the result value
-			// is unknown too so we can use it as a placeholder for partial
-			// downstream checking.
-			want := cty.DynamicVal
-			// FIXME: the cmp transformer ctydebug.CmpOptions seems to find
-			// this particular pair of values troubling, causing it to get
-			// into an infinite recursion. For now we'll just use RawEquals,
-			// at the expense of a less helpful failure message. This seems
-			// to be a bug in upstream ctydebug.
-			if !want.RawEquals(got) {
-				t.Fatalf("wrong result\ngot:  %#v\nwant: %#v", got, want)
+			// is an instance map with the wildcard key and an empty object
+			want := cty.ObjectVal(map[string]cty.Value{
+				"*": cty.EmptyObjectVal,
+			})
+
+			if diff := cmp.Diff(want, got, ctydebug.CmpOptions); diff != "" {
+				t.Fatalf("wrong result\n%s", diff)
 			}
 		})
 	})

internal/stacks/stackruntime/internal/stackeval/for_each.go

@@ -69,22 +69,18 @@ func evaluateForEachExpr(ctx context.Context, expr hcl.Expression, phase EvalPha
 	case ty.IsObjectType() || ty.IsMapType():
 		// okay
 
-	case !result.Value.IsKnown():
-		// we can't validate further without knowing the value
-		return result, diags
-
 	case ty.IsSetType():
-		if markSafeLengthInt(result.Value) == 0 {
-			// we are okay with an empty set
-			return result, diags
-		}
-
 		// since we can't use a set values that are unknown, we treat the
 		// entire set as unknown
 		if !result.Value.IsWhollyKnown() {
 			return result, diags
 		}
 
+		if markSafeLengthInt(result.Value) == 0 {
+			// we are okay with an empty set
+			return result, diags
+		}
+
 		if !ty.ElementType().Equals(cty.String) {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity:    hcl.DiagError,
@@ -111,6 +107,12 @@ func evaluateForEachExpr(ctx context.Context, expr hcl.Expression, phase EvalPha
 			}
 		}
 
+	case !result.Value.IsWhollyKnown() && ty.HasDynamicTypes():
+		// If the value is unknown and has dynamic types, we can't
+		// determine if it's a valid for_each value, so we'll just
+		// return the unknown value.
+		return result, diags
+
 	default:
 		diags = diags.Append(&hcl.Diagnostic{
 			Severity:    hcl.DiagError,
@@ -145,19 +147,20 @@ func evaluateForEachExpr(ctx context.Context, expr hcl.Expression, phase EvalPha
 // If maybeForEach value is non-nil but not a valid value produced by
 // [evaluateForEachExpr] then the behavior is unpredictable, including the
 // possibility of a panic.
-func instancesMap[T any](maybeForEachVal cty.Value, makeInst func(addrs.InstanceKey, instances.RepetitionData) T) map[addrs.InstanceKey]T {
+func instancesMap[T any](maybeForEachVal cty.Value, makeInst func(addrs.InstanceKey, instances.RepetitionData) T, allowsUnknown bool) map[addrs.InstanceKey]T {
 	switch {
-
 	case maybeForEachVal == cty.NilVal:
 		// No for_each expression at all, then. We have exactly one instance
 		// without an instance key and with no repetition data.
 		return noForEachInstancesMap(makeInst)
 
 	case !maybeForEachVal.IsKnown():
-		// The for_each expression is too invalid for us to be able to
-		// know which instances exist. A totally nil map (as opposed to a
-		// non-nil map of length zero) signals that situation.
-		return nil
+		// This is temporary to gradually rollout support for unknown for_each values
+		if allowsUnknown {
+			return unknownForEachInstancesMap(maybeForEachVal.Type(), makeInst)
+		} else {
+			return nil
+		}
 
 	default:
 		// Otherwise we should be able to assume the value is valid per the
@@ -239,6 +242,12 @@ func noForEachInstancesMap[T any](makeInst func(addrs.InstanceKey, instances.Rep
 	}
 }
 
+func unknownForEachInstancesMap[T any](ty cty.Type, makeInst func(addrs.InstanceKey, instances.RepetitionData) T) map[addrs.InstanceKey]T {
+	return map[addrs.InstanceKey]T{
+		addrs.WildcardKey: makeInst(addrs.WildcardKey, instances.UnknownForEachRepetitionData(ty)),
+	}
+}
+
 // markSafeLengthInt allows calling LengthInt on marked values safely
 func markSafeLengthInt(val cty.Value) int {
 	v, _ := val.UnmarkDeep()