From 8b7e7ad27d64973ded4b030636d9806eebf74259 Mon Sep 17 00:00:00 2001 From: Samsondeen <40821565+dsa0x@users.noreply.github.com> Date: Wed, 19 Mar 2025 10:39:23 +0100 Subject: [PATCH] Detect circular references during Validation (#36709) --- .../internal/stackeval/component_config.go | 23 ++- .../internal/stackeval/local_value_config.go | 24 +-- .../stackruntime/internal/stackeval/main.go | 13 +- .../internal/stackeval/output_value_config.go | 58 +++--- .../internal/stackeval/stack_call_config.go | 181 +++++++++--------- .../test/validate-cyclic-dependency/main.tf | 0 .../validate-cyclic-dependency.tfstack.hcl | 18 ++ internal/stacks/stackruntime/validate_test.go | 14 ++ 8 files changed, 199 insertions(+), 132 deletions(-) create mode 100644 internal/stacks/stackruntime/testdata/mainbundle/test/validate-cyclic-dependency/main.tf create mode 100644 internal/stacks/stackruntime/testdata/mainbundle/test/validate-cyclic-dependency/validate-cyclic-dependency.tfstack.hcl diff --git a/internal/stacks/stackruntime/internal/stackeval/component_config.go b/internal/stacks/stackruntime/internal/stackeval/component_config.go index 56212d52da..66f4e05b44 100644 --- a/internal/stacks/stackruntime/internal/stackeval/component_config.go +++ b/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 diff --git a/internal/stacks/stackruntime/internal/stackeval/local_value_config.go b/internal/stacks/stackruntime/internal/stackeval/local_value_config.go index 59f3bfb5be..d220b24a05 100644 --- a/internal/stacks/stackruntime/internal/stackeval/local_value_config.go +++ b/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 { diff --git a/internal/stacks/stackruntime/internal/stackeval/main.go b/internal/stacks/stackruntime/internal/stackeval/main.go index f661e59b4b..878011436a 100644 --- a/internal/stacks/stackruntime/internal/stackeval/main.go +++ b/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: diff --git a/internal/stacks/stackruntime/internal/stackeval/output_value_config.go b/internal/stacks/stackruntime/internal/stackeval/output_value_config.go index 15b305ac34..ce69a17d43 100644 --- a/internal/stacks/stackruntime/internal/stackeval/output_value_config.go +++ b/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 { diff --git a/internal/stacks/stackruntime/internal/stackeval/stack_call_config.go b/internal/stacks/stackruntime/internal/stackeval/stack_call_config.go index c9f6cd5e31..1ceefb9142 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack_call_config.go +++ b/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 diff --git a/internal/stacks/stackruntime/testdata/mainbundle/test/validate-cyclic-dependency/main.tf b/internal/stacks/stackruntime/testdata/mainbundle/test/validate-cyclic-dependency/main.tf new file mode 100644 index 0000000000..e69de29bb2 diff --git a/internal/stacks/stackruntime/testdata/mainbundle/test/validate-cyclic-dependency/validate-cyclic-dependency.tfstack.hcl b/internal/stacks/stackruntime/testdata/mainbundle/test/validate-cyclic-dependency/validate-cyclic-dependency.tfstack.hcl new file mode 100644 index 0000000000..e7e747a23e --- /dev/null +++ b/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 + } +} diff --git a/internal/stacks/stackruntime/validate_test.go b/internal/stacks/stackruntime/validate_test.go index 39cce9af4f..d8d2209652 100644 --- a/internal/stacks/stackruntime/validate_test.go +++ b/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.`, )) }),