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.

Author: mildwonkey

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")
+				}
+			}
 		})
 	}
 }

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,

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