Detect circular references during Validation (#36709)

internal/stacks/stackruntime/internal/stackeval/component_config.go

@@ -258,6 +258,11 @@ func (c *ComponentConfig) ExprReferenceValue(ctx context.Context, phase EvalPhas
 	// We don't expose ComponentConfig in any scope outside of the validation
 	// phase, so this is sufficient for all phases. (See [Component] for how
 	// component results get calculated during the plan and apply phases.)
+
+	// By calling `checkValid` on ourself here, we will cause a cycle error to be exposed if we ended
+	// up within this function while executing c.checkValid initially. This just makes sure that there
+	// are no cycles between components.
+	c.checkValid(ctx, phase)
 	return cty.DynamicVal
 }
 
@@ -382,10 +387,20 @@ func (c *ComponentConfig) checkValid(ctx context.Context, phase EvalPhase) tfdia
 		}))
 		return diags, nil
 	})
-	if err != nil {
-		// this is crazy, we never return an error from the inner function so
-		// this really shouldn't happen.
-		panic(fmt.Sprintf("unexpected error from validate.Do: %s", err))
+	switch err := err.(type) {
+	case promising.ErrSelfDependent:
+		// This is a case where the component is self-dependent, which is
+		// a cycle that we can't resolve. We'll report this as a diagnostic
+		// and then continue on to report any other diagnostics that we found.
+		// The promise reporter is main, so that we can get the names of all promises
+		// involved in the cycle.
+		diags = diags.Append(diagnosticsForPromisingTaskError(err, c.main))
+	default:
+		if err != nil {
+			// this is crazy, we never return an error from the inner function so
+			// this really shouldn't happen.
+			panic(fmt.Sprintf("unexpected error from validate.Do: %s", err))
+		}
 	}
 
 	return diags

internal/stacks/stackruntime/internal/stackeval/local_value_config.go

@@ -76,24 +76,26 @@ func (v *LocalValueConfig) ExprReferenceValue(ctx context.Context, phase EvalPha
 func (v *LocalValueConfig) ValidateValue(ctx context.Context, phase EvalPhase) (cty.Value, tfdiags.Diagnostics) {
 	return withCtyDynamicValPlaceholder(doOnceWithDiags(
 		ctx, v.validatedValue.For(phase), v.main,
-		v.validateValueInner,
+		v.validateValueInner(phase),
 	))
 }
 
 // validateValueInner is the real implementation of ValidateValue, which runs
 // in the background only once per instance of [OutputValueConfig] and then
 // provides the result for all ValidateValue callers simultaneously.
-func (lv *LocalValueConfig) validateValueInner(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
-	var diags tfdiags.Diagnostics
-
-	result, moreDiags := EvalExprAndEvalContext(ctx, lv.config.Value, ValidatePhase, lv.StackConfig(ctx))
-	v := result.Value
-	diags = diags.Append(moreDiags)
-	if moreDiags.HasErrors() {
-		v = cty.UnknownVal(cty.DynamicPseudoType)
+func (lv *LocalValueConfig) validateValueInner(phase EvalPhase) func(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
+	return func(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
+		var diags tfdiags.Diagnostics
+
+		result, moreDiags := EvalExprAndEvalContext(ctx, lv.config.Value, phase, lv.StackConfig(ctx))
+		v := result.Value
+		diags = diags.Append(moreDiags)
+		if moreDiags.HasErrors() {
+			v = cty.UnknownVal(cty.DynamicPseudoType)
+		}
+
+		return v, diags
 	}
-
-	return v, diags
 }
 
 func (v *LocalValueConfig) checkValid(ctx context.Context, phase EvalPhase) tfdiags.Diagnostics {

internal/stacks/stackruntime/internal/stackeval/main.go

@@ -216,6 +216,17 @@ func (m *Main) Inspecting() bool {
 	return m.inspecting != nil
 }
 
+// ValidatingOpts returns the validation options to use during the validate phase,
+// or panics if this [Main] was not instantiated for validation.
+//
+// Do not modify anything reachable through the returned pointer.
+func (m *Main) ValidatingOpts() *ValidateOpts {
+	if !m.Validating() {
+		panic("stacks language runtime is not instantiated for validating")
+	}
+	return &m.validating.opts
+}
+
 // PlanningOpts returns the planning options to use during the planning phase,
 // or panics if this [Main] was not instantiated for planning.
 //
@@ -678,7 +689,7 @@ func (m *Main) PlanTimestamp() time.Time {
 func (m *Main) DependencyLocks(phase EvalPhase) *depsfile.Locks {
 	switch phase {
 	case ValidatePhase:
-		return &m.validating.opts.DependencyLocks
+		return &m.ValidatingOpts().DependencyLocks
 	case PlanPhase:
 		return &m.PlanningOpts().DependencyLocks
 	case ApplyPhase:

internal/stacks/stackruntime/internal/stackeval/output_value_config.go

@@ -88,41 +88,43 @@ func (ov *OutputValueConfig) ValueTypeConstraint(ctx context.Context) cty.Type {
 func (ov *OutputValueConfig) ValidateValue(ctx context.Context, phase EvalPhase) (cty.Value, tfdiags.Diagnostics) {
 	return withCtyDynamicValPlaceholder(doOnceWithDiags(
 		ctx, ov.validatedValue.For(phase), ov.main,
-		ov.validateValueInner,
+		ov.validateValueInner(phase),
 	))
 }
 
 // validateValueInner is the real implementation of ValidateValue, which runs
 // in the background only once per instance of [OutputValueConfig] and then
 // provides the result for all ValidateValue callers simultaneously.
-func (ov *OutputValueConfig) validateValueInner(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
-	var diags tfdiags.Diagnostics
-
-	result, moreDiags := EvalExprAndEvalContext(ctx, ov.config.Value, ValidatePhase, ov.StackConfig(ctx))
-	v := result.Value
-	diags = diags.Append(moreDiags)
-	if moreDiags.HasErrors() {
-		v = ov.markResultValue(cty.UnknownVal(ov.ValueTypeConstraint(ctx)))
+func (ov *OutputValueConfig) validateValueInner(phase EvalPhase) func(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
+	return func(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
+		var diags tfdiags.Diagnostics
+
+		result, moreDiags := EvalExprAndEvalContext(ctx, ov.config.Value, phase, ov.StackConfig(ctx))
+		v := result.Value
+		diags = diags.Append(moreDiags)
+		if moreDiags.HasErrors() {
+			v = ov.markResultValue(cty.UnknownVal(ov.ValueTypeConstraint(ctx)))
+		}
+
+		var err error
+		v, err = convert.Convert(v, ov.config.Type.Constraint)
+		if err != nil {
+			v = cty.UnknownVal(ov.ValueTypeConstraint(ctx))
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid result for output value",
+				Detail: fmt.Sprintf(
+					"The result value does not match the declared type constraint: %s.",
+					tfdiags.FormatError(err),
+				),
+				Subject:     ov.config.Value.Range().Ptr(),
+				Expression:  result.Expression,
+				EvalContext: result.EvalContext,
+			})
+		}
+
+		return ov.markResultValue(v), diags
 	}
-
-	var err error
-	v, err = convert.Convert(v, ov.config.Type.Constraint)
-	if err != nil {
-		v = cty.UnknownVal(ov.ValueTypeConstraint(ctx))
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Invalid result for output value",
-			Detail: fmt.Sprintf(
-				"The result value does not match the declared type constraint: %s.",
-				tfdiags.FormatError(err),
-			),
-			Subject:     ov.config.Value.Range().Ptr(),
-			Expression:  result.Expression,
-			EvalContext: result.EvalContext,
-		})
-	}
-
-	return ov.markResultValue(v), diags
 }
 
 func (ov *OutputValueConfig) markResultValue(v cty.Value) cty.Value {

internal/stacks/stackruntime/internal/stackeval/stack_call_config.go

@@ -105,21 +105,23 @@ func (s *StackCallConfig) ResultType(ctx context.Context) cty.Type {
 func (s *StackCallConfig) ValidateForEachValue(ctx context.Context, phase EvalPhase) (cty.Value, tfdiags.Diagnostics) {
 	return withCtyDynamicValPlaceholder(doOnceWithDiags(
 		ctx, s.forEachValue.For(phase), s.main,
-		s.validateForEachValueInner,
+		s.validateForEachValueInner(phase),
 	))
 }
 
-func (s *StackCallConfig) validateForEachValueInner(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
-	var diags tfdiags.Diagnostics
+func (s *StackCallConfig) validateForEachValueInner(phase EvalPhase) func(context.Context) (cty.Value, tfdiags.Diagnostics) {
+	return func(ctx context.Context) (cty.Value, tfdiags.Diagnostics) {
+		var diags tfdiags.Diagnostics
 
-	if s.config.ForEach == nil {
-		// This stack config isn't even using for_each.
-		return cty.NilVal, diags
-	}
+		if s.config.ForEach == nil {
+			// This stack config isn't even using for_each.
+			return cty.NilVal, diags
+		}
 
-	result, moreDiags := evaluateForEachExpr(ctx, s.config.ForEach, ValidatePhase, s.CallerConfig(ctx), "stack")
-	diags = diags.Append(moreDiags)
-	return result.Value, diags
+		result, moreDiags := evaluateForEachExpr(ctx, s.config.ForEach, phase, s.CallerConfig(ctx), "stack")
+		diags = diags.Append(moreDiags)
+		return result.Value, diags
+	}
 }
 
 // ValidateInputVariableValues evaluates the "inputs" argument inside the
@@ -137,98 +139,101 @@ func (s *StackCallConfig) validateForEachValueInner(ctx context.Context) (cty.Va
 func (s *StackCallConfig) ValidateInputVariableValues(ctx context.Context, phase EvalPhase) (map[stackaddrs.InputVariable]cty.Value, tfdiags.Diagnostics) {
 	return doOnceWithDiags(
 		ctx, s.inputVariableValues.For(phase), s.main,
-		s.validateInputVariableValuesInner,
+		s.validateInputVariableValuesInner(phase),
 	)
 }
 
-func (s *StackCallConfig) validateInputVariableValuesInner(ctx context.Context) (map[stackaddrs.InputVariable]cty.Value, tfdiags.Diagnostics) {
-	var diags tfdiags.Diagnostics
-	callee := s.CalleeConfig(ctx)
-	vars := callee.InputVariables(ctx)
-
-	atys := make(map[string]cty.Type, len(vars))
-	var optional []string
-	defs := make(map[string]cty.Value, len(vars))
-	for addr, v := range vars {
-		aty := v.TypeConstraint()
-
-		atys[addr.Name] = aty
-		if def := v.DefaultValue(ctx); def != cty.NilVal {
-			optional = append(optional, addr.Name)
-			defs[addr.Name] = def
+func (s *StackCallConfig) validateInputVariableValuesInner(phase EvalPhase) func(context.Context) (map[stackaddrs.InputVariable]cty.Value, tfdiags.Diagnostics) {
+	return func(ctx context.Context) (map[stackaddrs.InputVariable]cty.Value, tfdiags.Diagnostics) {
+		var diags tfdiags.Diagnostics
+		callee := s.CalleeConfig(ctx)
+		vars := callee.InputVariables(ctx)
+
+		atys := make(map[string]cty.Type, len(vars))
+		var optional []string
+		defs := make(map[string]cty.Value, len(vars))
+		for addr, v := range vars {
+			aty := v.TypeConstraint()
+
+			atys[addr.Name] = aty
+			if def := v.DefaultValue(ctx); def != cty.NilVal {
+				optional = append(optional, addr.Name)
+				defs[addr.Name] = def
+			}
 		}
-	}
 
-	oty := cty.ObjectWithOptionalAttrs(atys, optional)
+		oty := cty.ObjectWithOptionalAttrs(atys, optional)
 
-	var varsObj cty.Value
-	var hclCtx *hcl.EvalContext // NOTE: remains nil when h.config.Inputs is unset
-	if s.config.Inputs != nil {
-		result, moreDiags := EvalExprAndEvalContext(ctx, s.config.Inputs, ValidatePhase, s)
-		v := result.Value
-		diags = diags.Append(moreDiags)
-		if moreDiags.HasErrors() {
-			v = cty.UnknownVal(oty.WithoutOptionalAttributesDeep())
+		var varsObj cty.Value
+		var hclCtx *hcl.EvalContext // NOTE: remains nil when h.config.Inputs is unset
+		if s.config.Inputs != nil {
+			result, moreDiags := EvalExprAndEvalContext(ctx, s.config.Inputs, phase, s)
+			v := result.Value
+			diags = diags.Append(moreDiags)
+			if moreDiags.HasErrors() {
+				v = cty.UnknownVal(oty.WithoutOptionalAttributesDeep())
+			}
+			varsObj = v
+			hclCtx = result.EvalContext
+		} else {
+			varsObj = cty.EmptyObjectVal
 		}
-		varsObj = v
-		hclCtx = result.EvalContext
-	} else {
-		varsObj = cty.EmptyObjectVal
-	}
-
-	// FIXME: TODO: We need to apply the nested optional attribute defaults
-	// somewhere in here too, but it isn't clear where we should do that since
-	// we're supposed to do that before type conversion but we don't yet have
-	// the isolated variable values to apply the defaults to.
-
-	varsObj, err := convert.Convert(varsObj, oty)
-	if err != nil {
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Invalid input variable definitions",
-			Detail: fmt.Sprintf(
-				"Unsuitable input variable definitions: %s.",
-				tfdiags.FormatError(err),
-			),
-			Subject: s.config.Inputs.Range().Ptr(),
-
-			// NOTE: The following two will be nil if the author didn't
-			// actually define the "inputs" argument, but that's okay
-			// because these fields are both optional anyway.
-			Expression:  s.config.Inputs,
-			EvalContext: hclCtx,
-		})
-		varsObj = cty.UnknownVal(oty.WithoutOptionalAttributesDeep())
-	}
 
-	ret := make(map[stackaddrs.InputVariable]cty.Value, len(vars))
+		// FIXME: TODO: We need to apply the nested optional attribute defaults
+		// somewhere in here too, but it isn't clear where we should do that since
+		// we're supposed to do that before type conversion but we don't yet have
+		// the isolated variable values to apply the defaults to.
+
+		varsObj, err := convert.Convert(varsObj, oty)
+		if err != nil {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid input variable definitions",
+				Detail: fmt.Sprintf(
+					"Unsuitable input variable definitions: %s.",
+					tfdiags.FormatError(err),
+				),
+				Subject: s.config.Inputs.Range().Ptr(),
+
+				// NOTE: The following two will be nil if the author didn't
+				// actually define the "inputs" argument, but that's okay
+				// because these fields are both optional anyway.
+				Expression:  s.config.Inputs,
+				EvalContext: hclCtx,
+			})
+			varsObj = cty.UnknownVal(oty.WithoutOptionalAttributesDeep())
+		}
 
-	for addr := range vars {
-		val := varsObj.GetAttr(addr.Name)
-		if val.IsNull() {
-			if def, ok := defs[addr.Name]; ok {
-				ret[addr] = def
+		ret := make(map[stackaddrs.InputVariable]cty.Value, len(vars))
+
+		for addr := range vars {
+			val := varsObj.GetAttr(addr.Name)
+			if val.IsNull() {
+				if def, ok := defs[addr.Name]; ok {
+					ret[addr] = def
+				} else {
+					diags = diags.Append(&hcl.Diagnostic{
+						Severity: hcl.DiagError,
+						Summary:  "Missing definition for required input variable",
+						Detail:   fmt.Sprintf("The input variable %q is required, so cannot be omitted.", addr.Name),
+						Subject:  s.config.Inputs.Range().Ptr(),
+
+						// NOTE: The following two will be nil if the author didn't
+						// actually define the "inputs" argument, but that's okay
+						// because these fields are both optional anyway.
+						Expression:  s.config.Inputs,
+						EvalContext: hclCtx,
+					})
+					ret[addr] = cty.UnknownVal(atys[addr.Name])
+				}
 			} else {
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Missing definition for required input variable",
-					Detail:   fmt.Sprintf("The input variable %q is required, so cannot be omitted.", addr.Name),
-					Subject:  s.config.Inputs.Range().Ptr(),
-
-					// NOTE: The following two will be nil if the author didn't
-					// actually define the "inputs" argument, but that's okay
-					// because these fields are both optional anyway.
-					Expression:  s.config.Inputs,
-					EvalContext: hclCtx,
-				})
-				ret[addr] = cty.UnknownVal(atys[addr.Name])
+				ret[addr] = val
 			}
-		} else {
-			ret[addr] = val
 		}
+
+		return ret, diags
 	}
 
-	return ret, diags
 }
 
 // InputVariableValues returns the effective input variable values specified

internal/stacks/stackruntime/testdata/mainbundle/test/validate-cyclic-dependency/validate-cyclic-dependency.tfstack.hcl

@@ -0,0 +1,18 @@
+locals {
+  foo = "bar"
+}
+
+component "vault-config" {
+  source = "./"
+  inputs = {
+    ssh_key_private = component.boundary.ssh_key_private
+    bar = local.foo
+  }
+}
+
+component "boundary" {
+  source = "./"
+  inputs = {
+    boundary_vault_token = component.vault-config.boundary_vault_token
+  }
+}

internal/stacks/stackruntime/validate_test.go

@@ -448,6 +448,20 @@ func TestValidate(t *testing.T) {
   - stack.a.output.a value
   - stack.a inputs
 
+Terraform uses references to decide a suitable order for performing operations, so configuration items may not refer to their own results either directly or indirectly.`,
+				))
+			}),
+		},
+		"cyclic-component-dependency": {
+			path: "validate-cyclic-dependency",
+			wantDiags: initDiags(func(diags tfdiags.Diagnostics) tfdiags.Diagnostics {
+				return diags.Append(tfdiags.Sourceless(
+					tfdiags.Error,
+					"Self-dependent items in configuration",
+					`The following items in your configuration form a circular dependency chain through their references:
+  - component.boundary
+  - component.vault-config
+
 Terraform uses references to decide a suitable order for performing operations, so configuration items may not refer to their own results either directly or indirectly.`,
 				))
 			}),