From 2084154458b2427704ec1ff2b67ea006dfe2cd48 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Tue, 16 Sep 2025 14:07:13 -0400 Subject: [PATCH 1/2] actions: return an error if config is null but the schema has required attributes There are various methods in terraform that will let you know if you are missing required attributes - none of which work with hcl.EmptyBody or nil inputs. This adds some helper methods so terraform can check if an action schema has required arguments and returns an error if the config is null but arguments are required. I did not bother with an exhaustive list of missing args, but I'm open to making that change. I'd like to get eyes on this section before I move on to modifying what we send to the provider. --- internal/terraform/context_validate_test.go | 172 +++++++++++++------- internal/terraform/node_action_abstract.go | 5 +- internal/terraform/node_action_validate.go | 20 ++- 3 files changed, 130 insertions(+), 67 deletions(-) diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index 67beadb889fc..9344bbc0bfd8 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 2af00048d56c..3ca9a55b0d69 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 4ad63a7254d1..6a158244023c 100644 --- a/internal/terraform/node_action_validate.go +++ b/internal/terraform/node_action_validate.go @@ -82,16 +82,18 @@ 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() { - return diags - } + // If the action has no "config" attribute at all, we'll send the provider + // an empty body - config = {} and omitted config entirely are the same, internally. + var config hcl.Body + if n.Config.Config == nil { + config = hcl.EmptyBody() } else { - configVal = cty.NullVal(n.Schema.ConfigSchema.ImpliedType()) + config = n.Config.Config + } + configVal, _, valDiags := ctx.EvaluateBlock(config, schema.ConfigSchema, nil, keyData) + diags = diags.Append(valDiags) + if valDiags.HasErrors() { + return diags } // Use unmarked value for validate request From 8a889aaa0ec5aba984f7ac8226d1a8eee7c3b4db Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Thu, 18 Sep 2025 12:39:02 -0400 Subject: [PATCH 2/2] generate nicer errors --- internal/terraform/node_action_validate.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/internal/terraform/node_action_validate.go b/internal/terraform/node_action_validate.go index 6a158244023c..8c36de6309ec 100644 --- a/internal/terraform/node_action_validate.go +++ b/internal/terraform/node_action_validate.go @@ -82,18 +82,24 @@ func (n *NodeValidatableAction) Execute(ctx EvalContext, _ walkOperation) tfdiag return diags } - // If the action has no "config" attribute at all, we'll send the provider - // an empty body - config = {} and omitted config entirely are the same, internally. - var config hcl.Body + config := n.Config.Config if n.Config.Config == nil { config = hcl.EmptyBody() - } else { - config = n.Config.Config } + configVal, _, valDiags := ctx.EvaluateBlock(config, schema.ConfigSchema, nil, keyData) - diags = diags.Append(valDiags) if valDiags.HasErrors() { - return diags + // 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 + } } // Use unmarked value for validate request