Skip to content

Commit

Permalink
stacks: handle unknown values in component for_each
Browse files Browse the repository at this point in the history
  • Loading branch information
DanielMSchmidt committed Apr 19, 2024
1 parent c1d608e commit c2663f8
Show file tree
Hide file tree
Showing 13 changed files with 611 additions and 103 deletions.
30 changes: 15 additions & 15 deletions internal/stacks/stackruntime/internal/stackeval/component.go
Expand Up @@ -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:
Expand Down Expand Up @@ -174,7 +163,7 @@ func (c *Component) CheckInstances(ctx context.Context, phase EvalPhase) (map[ad

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 {
Expand Down Expand Up @@ -209,6 +198,12 @@ func (c *Component) ResultValue(ctx context.Context, phase EvalPhase) cty.Value
return cty.DynamicVal
}

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 cty.DynamicVal
}

// We expect that the instances all have string keys, which will
// become the keys of a map that we're returning.
elems := make(map[string]cty.Value, len(insts))
Expand Down Expand Up @@ -257,6 +252,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 {
Expand All @@ -266,9 +267,8 @@ func (c *Component) PlanIsComplete(ctx context.Context) bool {
// get returned by a different return path.
return false
}
if !plan.Complete {
return false
}

return plan.Complete
}
// If we get here without returning false then we can say that
// all of the instance plans are complete.
Expand Down
Expand Up @@ -576,6 +576,11 @@ func (c *ComponentInstance) CheckModuleTreePlan(ctx context.Context) (*plans.Pla
}
}

// The instance is also upstream deferred if the for_each value for this instance is unknown.
if c.key == addrs.WildcardKey {
upstreamDeferred = true
}

// NOTE: This ComponentInstance type only deals with component
// instances currently declared in the configuration. See
// [ComponentInstanceRemoved] for the model of a component instance
Expand Down
43 changes: 22 additions & 21 deletions internal/stacks/stackruntime/internal/stackeval/component_test.go
Expand Up @@ -190,14 +190,12 @@ 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 a single instance with dynamic values in the repetition data.
// We don't distinguish between invalid and unknown for_each values.
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)
}
})
subtestInPromisingTask(t, "unknown", func(ctx context.Context, t *testing.T) {
Expand All @@ -208,30 +206,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")
}
})
})
Expand Down
23 changes: 18 additions & 5 deletions internal/stacks/stackruntime/internal/stackeval/for_each.go
Expand Up @@ -145,7 +145,7 @@ 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, supportsUnknownForEach bool) map[addrs.InstanceKey]T {
switch {

case maybeForEachVal == cty.NilVal:
Expand All @@ -154,10 +154,12 @@ func instancesMap[T any](maybeForEachVal cty.Value, makeInst func(addrs.Instance
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 supportsUnknownForEach {
return unknownForEachInstancesMap(makeInst)
} else {
return nil
}

default:
// Otherwise we should be able to assume the value is valid per the
Expand Down Expand Up @@ -239,6 +241,17 @@ func noForEachInstancesMap[T any](makeInst func(addrs.InstanceKey, instances.Rep
}
}

func unknownForEachInstancesMap[T any](makeInst func(addrs.InstanceKey, instances.RepetitionData) T) map[addrs.InstanceKey]T {
return map[addrs.InstanceKey]T{
addrs.WildcardKey: makeInst(addrs.WildcardKey, instances.RepetitionData{
// As we don't know the for_each value, we can only provide dynamic values
// for the repetition symbols.
EachKey: cty.DynamicVal,
EachValue: cty.DynamicVal,
}),
}
}

// markSafeLengthInt allows calling LengthInt on marked values safely
func markSafeLengthInt(val cty.Value) int {
v, _ := val.UnmarkDeep()
Expand Down

0 comments on commit c2663f8

Please sign in to comment.