Merge pull request #24904 from hashicorp/jbardin/plan-data-sources

Evaluate data sources in plan when necessary

Author: jbardin

plans/changes.go

@@ -52,6 +52,22 @@ func (c *Changes) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInst
 	}
 
 	return nil
+
+}
+
+// InstancesForConfigResource returns the planned change for the current objects
+// of the resource instances of the given address, if any. Returns nil if no
+// changes are planned.
+func (c *Changes) InstancesForConfigResource(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc {
+	var changes []*ResourceInstanceChangeSrc
+	for _, rc := range c.Resources {
+		resAddr := rc.Addr.ContainingResource().Config()
+		if resAddr.Equal(addr) && rc.DeposedKey == states.NotDeposed {
+			changes = append(changes, rc)
+		}
+	}
+
+	return changes
 }
 
 // ResourceInstanceDeposed returns the plan change of a deposed object of

plans/changes_sync.go

@@ -62,6 +62,29 @@ func (cs *ChangesSync) GetResourceInstanceChange(addr addrs.AbsResourceInstance,
 	panic(fmt.Sprintf("unsupported generation value %#v", gen))
 }
 
+// GetChangesForConfigResource searched the set of resource instance
+// changes and returns all changes related to a given configuration address.
+// This is be used to find possible changes related to a configuration
+// reference.
+//
+// If no such changes exist, nil is returned.
+//
+// The returned objects are a deep copy of the change recorded in the plan, so
+// callers may mutate them although it's generally better (less confusing) to
+// treat planned changes as immutable after they've been initially constructed.
+func (cs *ChangesSync) GetChangesForConfigResource(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc {
+	if cs == nil {
+		panic("GetChangesForConfigResource on nil ChangesSync")
+	}
+	cs.lock.Lock()
+	defer cs.lock.Unlock()
+	var changes []*ResourceInstanceChangeSrc
+	for _, c := range cs.changes.InstancesForConfigResource(addr) {
+		changes = append(changes, c.DeepCopy())
+	}
+	return changes
+}
+
 // RemoveResourceInstanceChange searches the set of resource instance changes
 // for one matching the given address and generation, and removes it from the
 // set if it exists.

terraform/context_apply_test.go

@@ -8724,7 +8724,20 @@ func TestContext2Apply_destroyNestedModuleWithAttrsReferencingResource(t *testin
 // that resource to be applied first.
 func TestContext2Apply_dataDependsOn(t *testing.T) {
 	p := testProvider("null")
-	m := testModule(t, "apply-data-depends-on")
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "null_instance" "write" {
+  foo = "attribute"
+}
+
+data "null_data_source" "read" {
+  depends_on = ["null_instance.write"]
+}
+
+resource "null_instance" "depends" {
+  foo = data.null_data_source.read.foo
+}
+`})
 
 	ctx := testContext2(t, &ContextOpts{
 		Config: m,
@@ -8782,6 +8795,63 @@ func TestContext2Apply_dataDependsOn(t *testing.T) {
 	if actual != expected {
 		t.Fatalf("bad:\n%s", strings.TrimSpace(state.String()))
 	}
+
+	// run another plan to make sure the data source doesn't show as a change
+	plan, diags := ctx.Plan()
+	assertNoErrors(t, diags)
+
+	for _, c := range plan.Changes.Resources {
+		if c.Action != plans.NoOp {
+			t.Fatalf("unexpected change for %s", c.Addr)
+		}
+	}
+
+	// now we cause a change in the first resource, which should trigger a plan
+	// in the data source, and the resource that depends on the data source
+	// must plan a change as well.
+	m = testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "null_instance" "write" {
+  foo = "new"
+}
+
+data "null_data_source" "read" {
+  depends_on = ["null_instance.write"]
+}
+
+resource "null_instance" "depends" {
+  foo = data.null_data_source.read.foo
+}
+`})
+
+	p.ApplyFn = func(info *InstanceInfo, s *InstanceState, d *InstanceDiff) (*InstanceState, error) {
+		// the side effect of the resource being applied
+		provisionerOutput = "APPLIED_AGAIN"
+		return testApplyFn(info, s, d)
+	}
+
+	ctx = testContext2(t, &ContextOpts{
+		Config: m,
+		State:  state,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("null"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags = ctx.Plan()
+	assertNoErrors(t, diags)
+
+	expectedChanges := map[string]plans.Action{
+		"null_instance.write":        plans.Update,
+		"data.null_data_source.read": plans.Read,
+		"null_instance.depends":      plans.Update,
+	}
+
+	for _, c := range plan.Changes.Resources {
+		if c.Action != expectedChanges[c.Addr.String()] {
+			t.Errorf("unexpected %s for %s", c.Action, c.Addr)
+		}
+	}
 }
 
 func TestContext2Apply_terraformWorkspace(t *testing.T) {

terraform/context_plan_test.go

@@ -1893,9 +1893,9 @@ func TestContext2Plan_computedInFunction(t *testing.T) {
 	assertNoErrors(t, diags)
 
 	if p.ReadDataSourceCalled {
-		t.Fatalf("ReadDataSource was called on provider during plan; should not have been called")
+		// there was no config change to read during plan
+		t.Fatalf("ReadDataSource should not have been called")
 	}
-
 }
 
 func TestContext2Plan_computedDataCountResource(t *testing.T) {
@@ -1993,6 +1993,7 @@ func TestContext2Plan_dataResourceBecomesComputed(t *testing.T) {
 		DataSources: map[string]*configschema.Block{
 			"aws_data_source": {
 				Attributes: map[string]*configschema.Attribute{
+					"id":  {Type: cty.String, Computed: true},
 					"foo": {Type: cty.String, Optional: true},
 				},
 			},
@@ -4992,8 +4993,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 		}
 	}
 	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
+		cfg := req.Config.AsValueMap()
+		cfg["id"] = cty.StringVal("data_id")
 		return providers.ReadDataSourceResponse{
-			Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("ReadDataSource called, but should not have been")),
+			State: cty.ObjectVal(cfg),
 		}
 	}
 
@@ -5010,9 +5013,6 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 	// thus the plan call below is forced to produce a deferred read action.
 
 	plan, diags := ctx.Plan()
-	if p.ReadDataSourceCalled {
-		t.Errorf("ReadDataSource was called on the provider, but should not have been because we didn't refresh")
-	}
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
@@ -5042,38 +5042,30 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
 					"num":      cty.StringVal("2"),
-					"computed": cty.UnknownVal(cty.String),
+					"computed": cty.StringVal("data_id"),
 				}), ric.After)
 			case "aws_instance.foo[1]":
 				if res.Action != plans.Create {
 					t.Fatalf("resource %s should be created, got %s", ric.Addr, ric.Action)
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
 					"num":      cty.StringVal("2"),
-					"computed": cty.UnknownVal(cty.String),
+					"computed": cty.StringVal("data_id"),
 				}), ric.After)
 			case "data.aws_vpc.bar[0]":
 				if res.Action != plans.Read {
 					t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action)
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
-					// In a normal flow we would've read an exact value in
-					// ReadDataSource, but because this test doesn't run
-					// cty.Refresh we have no opportunity to do that lookup
-					// and a deferred read is forced.
-					"id":  cty.UnknownVal(cty.String),
+					"id":  cty.StringVal("data_id"),
 					"foo": cty.StringVal("0"),
 				}), ric.After)
 			case "data.aws_vpc.bar[1]":
 				if res.Action != plans.Read {
 					t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action)
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
-					// In a normal flow we would've read an exact value in
-					// ReadDataSource, but because this test doesn't run
-					// cty.Refresh we have no opportunity to do that lookup
-					// and a deferred read is forced.
-					"id":  cty.UnknownVal(cty.String),
+					"id":  cty.StringVal("data_id"),
 					"foo": cty.StringVal("1"),
 				}), ric.After)
 			default:
@@ -5513,11 +5505,18 @@ func TestContext2Plan_invalidOutput(t *testing.T) {
 data "aws_data_source" "name" {}
 
 output "out" {
-  value = "${data.aws_data_source.name.missing}"
+  value = data.aws_data_source.name.missing
 }`,
 	})
 
 	p := testProvider("aws")
+	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
+		State: cty.ObjectVal(map[string]cty.Value{
+			"id":  cty.StringVal("data_id"),
+			"foo": cty.StringVal("foo"),
+		}),
+	}
+
 	ctx := testContext2(t, &ContextOpts{
 		Config: m,
 		Providers: map[addrs.Provider]providers.Factory{
@@ -5558,6 +5557,13 @@ resource "aws_instance" "foo" {
 	})
 
 	p := testProvider("aws")
+	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
+		State: cty.ObjectVal(map[string]cty.Value{
+			"id":  cty.StringVal("data_id"),
+			"foo": cty.StringVal("foo"),
+		}),
+	}
+
 	ctx := testContext2(t, &ContextOpts{
 		Config: m,
 		Providers: map[addrs.Provider]providers.Factory{

terraform/context_refresh_test.go

@@ -949,32 +949,11 @@ func TestContext2Refresh_dataState(t *testing.T) {
 
 	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
 		m := req.Config.AsValueMap()
-		m["inputs"] = cty.MapVal(map[string]cty.Value{"test": cty.StringVal("yes")})
 		readStateVal = cty.ObjectVal(m)
 
 		return providers.ReadDataSourceResponse{
 			State: readStateVal,
 		}
-
-		// FIXME: should the "outputs" value here be added to the reutnred state?
-		// Attributes: map[string]*ResourceAttrDiff{
-		// 	"inputs.#": {
-		// 		Old:  "0",
-		// 		New:  "1",
-		// 		Type: DiffAttrInput,
-		// 	},
-		// 	"inputs.test": {
-		// 		Old:  "",
-		// 		New:  "yes",
-		// 		Type: DiffAttrInput,
-		// 	},
-		// 	"outputs.#": {
-		// 		Old:         "",
-		// 		New:         "",
-		// 		NewComputed: true,
-		// 		Type:        DiffAttrOutput,
-		// 	},
-		// },
 	}
 
 	s, diags := ctx.Refresh()
@@ -986,14 +965,6 @@ func TestContext2Refresh_dataState(t *testing.T) {
 		t.Fatal("ReadDataSource should have been called")
 	}
 
-	// mod := s.RootModule()
-	// if got := mod.Resources["data.null_data_source.testing"].Primary.ID; got != "-" {
-	// 	t.Fatalf("resource id is %q; want %s", got, "-")
-	// }
-	// if !reflect.DeepEqual(mod.Resources["data.null_data_source.testing"].Primary, p.ReadDataApplyReturn) {
-	// 	t.Fatalf("bad: %#v", mod.Resources)
-	// }
-
 	mod := s.RootModule()
 
 	newState, err := mod.Resources["data.null_data_source.testing"].Instances[addrs.NoKey].Current.Decode(schema.ImpliedType())
@@ -1612,6 +1583,11 @@ func TestContext2Refresh_dataResourceDependsOn(t *testing.T) {
 		},
 	}
 	p.DiffFn = testDiffFn
+	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
+		State: cty.ObjectVal(map[string]cty.Value{
+			"compute": cty.StringVal("value"),
+		}),
+	}
 
 	state := states.NewState()
 	root := state.EnsureModule(addrs.RootModuleInstance)

terraform/context_test.go

@@ -693,11 +693,12 @@ func testProviderSchema(name string) *ProviderSchema {
 				Attributes: map[string]*configschema.Attribute{
 					"id": {
 						Type:     cty.String,
-						Optional: true,
+						Computed: true,
 					},
 					"foo": {
 						Type:     cty.String,
 						Optional: true,
+						Computed: true,
 					},
 				},
 			},