From e241e1ace1571058e7b571d27c2898dcbde2f962 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Fri, 19 Sep 2025 08:54:30 -0400 Subject: [PATCH] actions: return an error if config is omitted but the schema has required attrs (#37626) * actions: improve handling of null config so we can properly report missing required arguments There are various methods in terraform that will let you know if you are missing required attributes - none of which work with nil inputs. This commit changes the handling, passing in an empty object if the config was null, and adding additional context to the error message when the config block is missing. --- internal/terraform/context_validate_test.go | 172 +++++++++++++------- internal/terraform/node_action_abstract.go | 5 +- internal/terraform/node_action_validate.go | 24 ++- 3 files changed, 135 insertions(+), 66 deletions(-) diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index 67beadb889..9344bbc0bf 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -3575,60 +3575,114 @@ func TestContext2Validate_queryList(t *testing.T) { // Action Validation is largely exercised in context_plan_actions_test.go func TestContext2Validate_action(t *testing.T) { tests := map[string]struct { - config string - wantErr string + config string + wantErr string + expectValidateCalled bool }{ "valid config": { ` -terraform { - required_providers { - test = { - source = "hashicorp/test" - version = "1.0.0" + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } + } + action "test_register" "foo" { + config { + host = "cmdb.snot" + } + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_register.foo] + } + } + } + `, + "", + true, + }, + "validly null config": { // this doesn't seem likely, but let's make sure nothing panics. + ` + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } } - } -} -action "test_register" "foo" { - config { - host = "cmdb.snot" - } -} -resource "test_instance" "foo" { - lifecycle { - action_trigger { - events = [after_create] - actions = [action.test_register.foo] - } - } -} -`, + action "test_other" "foo" { + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_other.foo] + } + } + } + `, "", + true, }, "missing required config": { ` -terraform { - required_providers { - test = { - source = "hashicorp/test" - version = "1.0.0" + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } } - } -} -action "test_register" "foo" { - config {} -} -resource "test_instance" "foo" { - lifecycle { - action_trigger { - events = [after_create] - actions = [action.test_register.foo] - } - } -} -`, - "host is null", + action "test_register" "foo" { + config {} + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_register.foo] + } + } + } + `, + "Missing required argument: The argument \"host\" is required, but no definition was found.", + false, + }, + "invalid required config (provider validation)": { + ` + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } + } + action "test_register" "foo" { + config { + host = "invalid" + } + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_register.foo] + } + } + } + `, + "Invalid value for required argument \"host\" because I said so", + true, }, - "invalid nil config config": { + "invalid nil config": { ` terraform { required_providers { @@ -3649,7 +3703,8 @@ resource "test_instance" "foo" { } } `, - "config is null", + "Missing required argument: The argument \"host\" is required, but was not set.", + false, }, } @@ -3669,6 +3724,14 @@ resource "test_instance" "foo" { }, Actions: map[string]*providers.ActionSchema{ "test_register": { + ConfigSchema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "host": {Type: cty.String, Required: true}, + "output": {Type: cty.String, Computed: true, Optional: true}, + }, + }, + }, + "test_other": { ConfigSchema: &configschema.Block{ Attributes: map[string]*configschema.Attribute{ "host": {Type: cty.String, Optional: true}, @@ -3678,12 +3741,8 @@ resource "test_instance" "foo" { }, }) p.ValidateActionConfigFn = func(r providers.ValidateActionConfigRequest) (resp providers.ValidateActionConfigResponse) { - if r.Config.IsNull() { - resp.Diagnostics = resp.Diagnostics.Append(errors.New("config is null")) - return - } - if r.Config.GetAttr("host").IsNull() { - resp.Diagnostics = resp.Diagnostics.Append(errors.New("host is null")) + if r.Config.GetAttr("host") == cty.StringVal("invalid") { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("Invalid value for required argument \"host\" because I said so")) } return } @@ -3695,10 +3754,6 @@ resource "test_instance" "foo" { }) diags := ctx.Validate(m, nil) - if !p.ValidateActionConfigCalled { - t.Fatal("ValidateAction RPC was not called") - } - if test.wantErr != "" { if !diags.HasErrors() { t.Errorf("unexpected success\nwant errors: %s", test.wantErr) @@ -3710,6 +3765,13 @@ resource "test_instance" "foo" { t.Errorf("unexpected error\ngot: %s", diags.Err().Error()) } } + if p.ValidateActionConfigCalled != test.expectValidateCalled { + if test.expectValidateCalled { + t.Fatal("provider Validate RPC was expected, but not called") + } else { + t.Fatal("unexpected Validate RCP call") + } + } }) } } diff --git a/internal/terraform/node_action_abstract.go b/internal/terraform/node_action_abstract.go index 2af00048d5..3ca9a55b0d 100644 --- a/internal/terraform/node_action_abstract.go +++ b/internal/terraform/node_action_abstract.go @@ -44,9 +44,8 @@ func (n NodeAbstractAction) Name() string { // abstract action to a concrete one of some type. type ConcreteActionNodeFunc func(*NodeAbstractAction) dag.Vertex -// I'm not sure why my ConcreteActionNodeFUnction kept being nil in tests, but -// this is much more robust. If it isn't a validate walk, we need -// nodeExpandActionDeclaration. +// DefaultConcreteActionNodeFunc is the default ConcreteActionNodeFunc used by +// everything except validate. func DefaultConcreteActionNodeFunc(a *NodeAbstractAction) dag.Vertex { return &nodeExpandActionDeclaration{ NodeAbstractAction: a, diff --git a/internal/terraform/node_action_validate.go b/internal/terraform/node_action_validate.go index 4ad63a7254..8c36de6309 100644 --- a/internal/terraform/node_action_validate.go +++ b/internal/terraform/node_action_validate.go @@ -82,16 +82,24 @@ func (n *NodeValidatableAction) Execute(ctx EvalContext, _ walkOperation) tfdiag return diags } - var configVal cty.Value - var valDiags tfdiags.Diagnostics - if n.Config.Config != nil { - configVal, _, valDiags = ctx.EvaluateBlock(n.Config.Config, schema.ConfigSchema, nil, keyData) - diags = diags.Append(valDiags) - if valDiags.HasErrors() { + config := n.Config.Config + if n.Config.Config == nil { + config = hcl.EmptyBody() + } + + configVal, _, valDiags := ctx.EvaluateBlock(config, schema.ConfigSchema, nil, keyData) + if valDiags.HasErrors() { + // If there was no config block at all, we'll add a Context range to the returned diagnostic + if n.Config.Config == nil { + for _, diag := range valDiags.ToHCL() { + diag.Context = &n.Config.DeclRange + diags = diags.Append(diag) + } + return diags + } else { + diags = diags.Append(valDiags) return diags } - } else { - configVal = cty.NullVal(n.Schema.ConfigSchema.ImpliedType()) } // Use unmarked value for validate request