Ensure module changes are detected in Diff(...) (#400)

* Ensure module changes are detected in Diff(...)

* lint

* lint more

* try plan -no-refresh in Diff

* relax alb example assertions and ues a smaller database in rds example

* try pulumi preview --refresh on the RDS example

* preview --refresh on the RDS module example in e2e TypeScript tests

* pulumi preview --refresh when expecting no changes

* skip expect no changes for aws-lambda because of problematic drift

Author: Zaid-Ajaj

examples/aws-rds-example/index.ts

@@ -52,7 +52,7 @@ new rdsmod.Module("test-rds", {
   publicly_accessible: false,
   allocated_storage: 20,
   max_allocated_storage: 100,
-  instance_class: "db.t4g.large",
+  instance_class: "db.t4g.micro",
   engine_version: "8.0",
   family: "mysql8.0",
   db_name: "completeMysql",

pkg/modprovider/module.go

@@ -71,11 +71,14 @@ func (h *moduleHandler) Check(
 }
 
 func (h *moduleHandler) Diff(
-	_ context.Context,
+	ctx context.Context,
 	req *pulumirpc.DiffRequest,
+	moduleSource TFModuleSource,
+	moduleVersion TFModuleVersion,
+	providersConfig map[string]resource.PropertyMap,
+	inferredModule *InferredModuleSchema,
+	executor string,
 ) (*pulumirpc.DiffResponse, error) {
-	changes := pulumirpc.DiffResponse_DIFF_NONE
-
 	oldInputs, err := plugin.UnmarshalProperties(req.GetOldInputs(), h.marshalOpts())
 	if err != nil {
 		return nil, err
@@ -88,10 +91,60 @@ func (h *moduleHandler) Diff(
 
 	// TODO[pulumi/pulumi-terraform-module#332] detect and correct drift
 	if !oldInputs.DeepEquals(newInputs) {
-		changes = pulumirpc.DiffResponse_DIFF_SOME
+		// Inputs have changed, so we need tell the engine that an update is needed.
+		return &pulumirpc.DiffResponse{Changes: pulumirpc.DiffResponse_DIFF_SOME}, nil
+	}
+
+	// Here, inputs have not changes but the underlying module might have changed
+	// perform a plan to see if there were any changes in the module reported by terraform
+	urn := urn.URN(req.GetUrn())
+	oldOutputs, err := plugin.UnmarshalProperties(req.GetOlds(), h.marshalOpts())
+	if err != nil {
+		return nil, fmt.Errorf("failed to unmarshal old outputs: %w", err)
+	}
+
+	tf, err := h.prepSandbox(
+		ctx,
+		urn,
+		oldInputs,
+		oldOutputs,
+		inferredModule,
+		moduleSource,
+		moduleVersion,
+		providersConfig,
+		executor,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("failed preparing sandbox: %w", err)
+	}
+
+	plan, err := tf.PlanNoRefresh(ctx, newResourceLogger(h.hc, urn))
+	if err != nil {
+		return nil, fmt.Errorf("error performing plan during Diff(...) %w", err)
+	}
+
+	resourcesChanged := false
+	plan.VisitResourcePlans(func(resource *tfsandbox.ResourcePlan) {
+		if resource.ChangeKind() != tfsandbox.NoOp {
+			// if there is any resource change that is not a no-op, we need to update.
+			resourcesChanged = true
+		}
+	})
+
+	outputsChanged := false
+	for _, output := range plan.RawPlan().OutputChanges {
+		if !output.Actions.NoOp() {
+			outputsChanged = true
+			break
+		}
+	}
+
+	if resourcesChanged || outputsChanged {
+		return &pulumirpc.DiffResponse{Changes: pulumirpc.DiffResponse_DIFF_SOME}, nil
 	}
 
-	return &pulumirpc.DiffResponse{Changes: changes}, nil
+	// the module has not changed, return DIFF_NONE.
+	return &pulumirpc.DiffResponse{Changes: pulumirpc.DiffResponse_DIFF_NONE}, nil
 }
 
 func (h *moduleHandler) prepSandbox(
@@ -440,7 +493,7 @@ func (h *moduleHandler) Delete(
 		executor,
 	)
 	if err != nil {
-		return nil, fmt.Errorf("failed preparing tofu sandbox: %w", err)
+		return nil, fmt.Errorf("failed preparing sandbox: %w", err)
 	}
 
 	// TODO[pulumi/pulumi-terraform-module#247] once the engine is ready to receive view steps multiple times, the

pkg/modprovider/server.go

@@ -470,7 +470,9 @@ func (s *server) Diff(
 ) (*pulumirpc.DiffResponse, error) {
 	switch {
 	case req.GetType() == string(moduleTypeToken(s.packageName)):
-		return s.moduleHandler.Diff(ctx, req)
+		providersConfig := cleanProvidersConfig(s.providerConfig)
+		return s.moduleHandler.Diff(ctx, req, s.params.TFModuleSource, s.params.TFModuleVersion, providersConfig,
+			s.inferredModuleSchema, s.moduleExecutor)
 	default:
 		return nil, fmt.Errorf("[Diff]: type %q is not supported yet", req.GetType())
 	}

pkg/tfsandbox/plan.go

@@ -50,20 +50,42 @@ func (t *ModuleRuntime) PlanRefreshOnly(ctx context.Context, logger Logger) (*Pl
 	return p, nil
 }
 
+func (t *ModuleRuntime) PlanNoRefresh(ctx context.Context, logger Logger) (*Plan, error) {
+	plan, err := t.planNoRefresh(ctx, logger)
+	if err != nil {
+		return nil, err
+	}
+
+	p, err := NewPlan(plan)
+	if err != nil {
+		return nil, err
+	}
+	return p, nil
+}
+
 func (t *ModuleRuntime) plan(ctx context.Context, logger Logger) (*tfjson.Plan, error) {
-	return t.planWithOptions(ctx, logger, false /*refreshOnly*/)
+	return t.planWithOptions(ctx, logger, t.planOptions())
 }
 
 func (t *ModuleRuntime) planRefreshOnly(ctx context.Context, logger Logger) (*tfjson.Plan, error) {
-	return t.planWithOptions(ctx, logger, true /*refreshOnly*/)
+	return t.planWithOptions(ctx, logger, t.planOptions(tfexec.RefreshOnly(true)))
 }
 
-func (t *ModuleRuntime) planWithOptions(ctx context.Context, logger Logger, refreshOnly bool) (*tfjson.Plan, error) {
+func (t *ModuleRuntime) planNoRefresh(ctx context.Context, logger Logger) (*tfjson.Plan, error) {
+	return t.planWithOptions(ctx, logger, t.planOptions(tfexec.Refresh(false)))
+}
+
+func (t *ModuleRuntime) planWithOptions(
+	ctx context.Context,
+	logger Logger,
+	options []tfexec.PlanOption,
+) (*tfjson.Plan, error) {
 	planFile := path.Join(t.WorkingDir(), defaultPlanFile)
 	logWriter := newJSONLogPipe(ctx, logger)
 	defer logWriter.Close()
-	_ /*hasChanges*/, err := t.tf.PlanJSON(ctx, logWriter,
-		t.planOptions(tfexec.Out(planFile), tfexec.RefreshOnly(refreshOnly))...)
+
+	planOptions := append(t.planOptions(tfexec.Out(planFile)), options...)
+	_ /*hasChanges*/, err := t.tf.PlanJSON(ctx, logWriter, planOptions...)
 	if err != nil {
 		return nil, fmt.Errorf("error running plan: %w", err)
 	}

tests/acc_test.go

@@ -358,6 +358,107 @@ func Test_TwoInstances_TypeScript(t *testing.T) {
 	})
 }
 
+// Test that changing the source code of a local module causes the module to be updated
+// without any changes to the program code.
+// In this specific example, the change in the source code is from changing outputs
+func TestChangingLocalModuleSourceCodeOutputsCausesUpdate(t *testing.T) {
+	localProviderBinPath := ensureCompiledProvider(t)
+	modulePath := filepath.Join("testdata", "modules", "changing_module_source_outputs")
+	v1, err := filepath.Abs(filepath.Join(modulePath, "mod_v1"))
+	assert.NoError(t, err, "failed to get absolute path for v1 module")
+	v2, err := filepath.Abs(filepath.Join(modulePath, "mod_v2"))
+	assert.NoError(t, err, "failed to get absolute path for v2 module")
+
+	// YAML program that uses the module.
+	// We don't change anything here
+	program := filepath.Join("testdata", "programs", "yaml", "changing_module_source_outputs")
+
+	integrationTest := pulumitest.NewPulumiTest(t, program,
+		opttest.SkipInstall(),
+		opttest.LocalProviderPath(provider, filepath.Dir(localProviderBinPath)))
+
+	pulumiPackageAdd(t, integrationTest, localProviderBinPath, v1, "greeting")
+
+	resultV1 := integrationTest.Up(t)
+	assert.Len(t, resultV1.Outputs, 1, "expected one output")
+	greeting, ok := resultV1.Outputs["greeting"]
+	require.True(t, ok, "expected output greeting")
+	require.Equal(t, "Hello, John!", greeting.Value)
+
+	// Now change the source code of the module from v1 to v2
+	v1SourceCode, err := os.ReadFile(filepath.Join(v1, "main.tf"))
+	require.NoError(t, err, "failed to read v1 source code")
+	v2SourceCode, err := os.ReadFile(filepath.Join(v2, "main.tf"))
+	require.NoError(t, err, "failed to read v2 source code")
+
+	err = os.WriteFile(filepath.Join(v1, "main.tf"), v2SourceCode, 0600)
+	require.NoError(t, err, "failed to write v2 source code to v1 module")
+
+	t.Cleanup(func() {
+		// Restore the original source code after the test
+		err := os.WriteFile(filepath.Join(v1, "main.tf"), v1SourceCode, 0600)
+		require.NoError(t, err, "failed to restore v1 source code")
+	})
+
+	t.Logf("Changed module source code to v2")
+	resultV2 := integrationTest.Up(t)
+	assert.Len(t, resultV2.Outputs, 1, "expected one output")
+	greeting, ok = resultV2.Outputs["greeting"]
+	require.True(t, ok, "expected output greeting")
+	require.Equal(t, "Goodbye, John!", greeting.Value)
+}
+
+// Test that changing the source code of a local module causes the module to be updated
+// In this specific example, the change in the source code is from changing resources
+func TestChangingLocalModuleSourceCodeResourcesCausesUpdate(t *testing.T) {
+	localProviderBinPath := ensureCompiledProvider(t)
+	modulePath := filepath.Join("testdata", "modules", "changing_module_source_resources")
+	v1, err := filepath.Abs(filepath.Join(modulePath, "mod_v1"))
+	assert.NoError(t, err, "failed to get absolute path for v1 module")
+	v2, err := filepath.Abs(filepath.Join(modulePath, "mod_v2"))
+	assert.NoError(t, err, "failed to get absolute path for v2 module")
+
+	// YAML program that uses the module.
+	// We don't change anything here
+	program := filepath.Join("testdata", "programs", "yaml", "changing_module_source_resources")
+
+	integrationTest := pulumitest.NewPulumiTest(t, program,
+		opttest.SkipInstall(),
+		opttest.LocalProviderPath(provider, filepath.Dir(localProviderBinPath)))
+
+	pulumiPackageAdd(t, integrationTest, localProviderBinPath, v1, "rand")
+
+	integrationTest.Up(t)
+	// assert that the module was created with the expected resource
+	mustFindDeploymentResourceByType(t, integrationTest, "rand:tf:random_integer")
+
+	// Now change the source code of the module from v1 to v2
+	v1SourceCode, err := os.ReadFile(filepath.Join(v1, "main.tf"))
+	require.NoError(t, err, "failed to read v1 source code")
+	v2SourceCode, err := os.ReadFile(filepath.Join(v2, "main.tf"))
+	require.NoError(t, err, "failed to read v2 source code")
+
+	err = os.WriteFile(filepath.Join(v1, "main.tf"), v2SourceCode, 0600)
+	require.NoError(t, err, "failed to write v2 source code to v1 module")
+
+	t.Cleanup(func() {
+		// Restore the original source code after the test
+		err := os.WriteFile(filepath.Join(v1, "main.tf"), v1SourceCode, 0600)
+		require.NoError(t, err, "failed to restore v1 source code")
+	})
+
+	preview := integrationTest.Preview(t)
+	assert.Equal(t, 1, preview.ChangeSummary[apitype.OpType("delete")],
+		"expected one resource to be deleted")
+	assert.Equal(t, 1, preview.ChangeSummary[apitype.OpType("create")],
+		"expected one resource to be created")
+
+	integrationTest.Up(t)
+	// assert that the resource within the module has changed
+	// from random_integer into random_pet
+	mustFindDeploymentResourceByType(t, integrationTest, "rand:tf:random_pet")
+}
+
 func TestGenerateTerraformAwsModulesSDKs(t *testing.T) {
 	t.Parallel()
 
@@ -651,6 +752,8 @@ func TestE2eTs(t *testing.T) {
 		upExpect            map[string]int
 		deleteExpect        map[string]int
 		diffNoChangesExpect map[apitype.OpType]int
+		previewRefresh      bool // Whether to run preview with refresh enabled
+		skipNoChangesExpect bool // Whether to skip the no changes expectation
 	}
 
 	testcases := []testCase{
@@ -677,6 +780,8 @@ func TestE2eTs(t *testing.T) {
 			moduleName:      "terraform-aws-modules/lambda/aws",
 			moduleVersion:   "7.20.1",
 			moduleNamespace: "lambda",
+			// aws lambda module creates drift even in terraform
+			skipNoChangesExpect: true,
 			previewExpect: map[apitype.OpType]int{
 				apitype.OpType("create"): 8,
 			},
@@ -702,6 +807,9 @@ func TestE2eTs(t *testing.T) {
 			moduleName:      "terraform-aws-modules/rds/aws",
 			moduleVersion:   "6.10.0",
 			moduleNamespace: "rds",
+			// RDS module has immediate drift after creation,
+			// so we need to refresh to get the correct preview
+			previewRefresh: true,
 			previewExpect: map[apitype.OpType]int{
 				apitype.OpType("create"): 10,
 			},
@@ -736,7 +844,11 @@ func TestE2eTs(t *testing.T) {
 			pulumiPackageAdd(t, integrationTest, localProviderBinPath, tc.moduleName, tc.moduleVersion, tc.moduleNamespace)
 
 			// Preview
-			previewResult := integrationTest.Preview(t, optpreview.Diff())
+			previewOptions := []optpreview.Option{optpreview.Diff()}
+			if tc.previewRefresh {
+				previewOptions = append(previewOptions, optpreview.Refresh())
+			}
+			previewResult := integrationTest.Preview(t, previewOptions...)
 			t.Logf("pulumi preview:\n%s", previewResult.StdOut+previewResult.StdErr)
 			autogold.Expect(tc.previewExpect).Equal(t, previewResult.ChangeSummary)
 
@@ -746,9 +858,11 @@ func TestE2eTs(t *testing.T) {
 			autogold.Expect(&tc.upExpect).Equal(t, upResult.Summary.ResourceChanges)
 
 			// Preview expect no changes
-			previewResult = integrationTest.Preview(t, optpreview.Diff())
+			previewResult = integrationTest.Preview(t, previewOptions...)
 			t.Logf("pulumi preview\n%s", previewResult.StdOut+previewResult.StdErr)
-			autogold.Expect(tc.diffNoChangesExpect).Equal(t, previewResult.ChangeSummary)
+			if !tc.skipNoChangesExpect {
+				autogold.Expect(tc.diffNoChangesExpect).Equal(t, previewResult.ChangeSummary)
+			}
 
 			// Delete
 			destroyResult := integrationTest.Destroy(t)