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.

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,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