From 2be524d6ac3f09cee05d394971b79569be1c45f9 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Fri, 14 Dec 2018 17:14:17 -0800
Subject: [PATCH] core: Validate depends_on and ignore_changes traversals

Both depends_on and ignore_changes contain references to objects that we
can validate.

Historically Terraform has not validated these, instead just ignoring
references to non-existent objects. Since there is no reason to refer to
something that doesn't exist, we'll now verify this and return errors so
that users get explicit feedback on any typos they may have made, rather
than just wondering why what they added seems to have no effect.

This is particularly important for ignore_changes because users have
historically used strange values here to try to exploit the fact that
Terraform was resolving ignore_changes against a flatmap. This will give
them explicit feedback for any odd constructs that the configuration
upgrade tool doesn't know how to detect and fix.
---
 lang/eval.go                       |  4 ++
 terraform/context_validate_test.go | 67 ++++++++++++++++++++++++++++++
 terraform/eval_validate.go         | 18 ++++++++
 3 files changed, 89 insertions(+)

diff --git a/lang/eval.go b/lang/eval.go
index e3f470d81..b5aaa8588 100644
--- a/lang/eval.go
+++ b/lang/eval.go
@@ -154,6 +154,10 @@ func (s *Scope) EvalContext(refs []*addrs.Reference) (*hcl.EvalContext, tfdiags.
 }
 
 func (s *Scope) evalContext(refs []*addrs.Reference, selfAddr addrs.Referenceable) (*hcl.EvalContext, tfdiags.Diagnostics) {
+	if s == nil {
+		panic("attempt to construct EvalContext for nil Scope")
+	}
+
 	var diags tfdiags.Diagnostics
 	vals := make(map[string]cty.Value)
 	funcs := s.Functions()
diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go
index 4801c7e0f..e1518ee1c 100644
--- a/terraform/context_validate_test.go
+++ b/terraform/context_validate_test.go
@@ -1315,3 +1315,70 @@ output "out" {
 		t.Fatalf("wrong error:\ngot:  %s\nwant: message containing %q", got, want)
 	}
 }
+
+func TestContext2Validate_invalidDependsOnResourceRef(t *testing.T) {
+	// This test is verifying that we raise an error if depends_on
+	// refers to something that doesn't exist in configuration.
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_instance" "bar" {
+  depends_on = [test_resource.nonexistant]
+}
+`,
+	})
+
+	p := testProvider("test")
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	diags := ctx.Validate()
+	if !diags.HasErrors() {
+		t.Fatal("succeeded; want errors")
+	}
+	// Should get this error:
+	// Reference to undeclared module: No module call named "foo" is declared in the root module.
+	if got, want := diags.Err().Error(), "Reference to undeclared resource:"; strings.Index(got, want) == -1 {
+		t.Fatalf("wrong error:\ngot:  %s\nwant: message containing %q", got, want)
+	}
+}
+
+func TestContext2Validate_invalidResourceIgnoreChanges(t *testing.T) {
+	// This test is verifying that we raise an error if ignore_changes
+	// refers to something that can be statically detected as not conforming
+	// to the resource type schema.
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_instance" "bar" {
+  lifecycle {
+    ignore_changes = [does_not_exist_in_schema]
+  }
+}
+`,
+	})
+
+	p := testProvider("test")
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	diags := ctx.Validate()
+	if !diags.HasErrors() {
+		t.Fatal("succeeded; want errors")
+	}
+	// Should get this error:
+	// Reference to undeclared module: No module call named "foo" is declared in the root module.
+	if got, want := diags.Err().Error(), `no argument, nested block, or exported attribute named "does_not_exist_in_schema"`; strings.Index(got, want) == -1 {
+		t.Fatalf("wrong error:\ngot:  %s\nwant: message containing %q", got, want)
+	}
+}
diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go
index 3d79f7989..06db8da0c 100644
--- a/terraform/eval_validate.go
+++ b/terraform/eval_validate.go
@@ -366,6 +366,17 @@ func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) {
 				Subject:  ref.Remaining.SourceRange().Ptr(),
 			})
 		}
+
+		// The ref must also refer to something that exists. To test that,
+		// we'll just eval it and count on the fact that our evaluator will
+		// detect references to non-existent objects.
+		if !diags.HasErrors() {
+			scope := ctx.EvaluationScope(nil, EvalDataForNoInstanceKey)
+			if scope != nil { // sometimes nil in tests, due to incomplete mocks
+				_, refDiags = scope.EvalReference(ref, cty.DynamicPseudoType)
+				diags = diags.Append(refDiags)
+			}
+		}
 	}
 
 	// Provider entry point varies depending on resource mode, because
@@ -390,6 +401,13 @@ func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) {
 			return nil, diags.Err()
 		}
 
+		if cfg.Managed != nil { // can be nil only in tests with poorly-configured mocks
+			for _, traversal := range cfg.Managed.IgnoreChanges {
+				moreDiags := schema.StaticValidateTraversal(traversal)
+				diags = diags.Append(moreDiags)
+			}
+		}
+
 		req := providers.ValidateResourceTypeConfigRequest{
 			TypeName: cfg.Type,
 			Config:   configVal,