Backport of actions: return an error if config is omitted but the schema has required attrs into v1.14

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