Fix dependency handling #179

t0yv0 · 2025-03-05T22:39:59Z

This change fixes dependency handling. The fix is coarse-grained but important for correctly ordering deletes. The implementation is currently lacking capabilities to track dependencies precisely through the TF black box.

What this does:

if dependencies are passed as inputs to a module instance, ModuleState Custom Resource now depends on all these dependencies. This means, in particular, that tofu destroy will now have to complete before the dependencies start getting destroyed themselves. This fixes issues such as Dependency information lost for RDS module depending on AWS resources #234
if Pulumi resources depend on any of the module's outputs, they now also will depend on all the dependencies of the module's inputs, they should order their destroy operations accordingly. They also depend on the Component resource itself, and I believe depending on a Component is equivalent in Pulumi to depending on all its children, which should mean they transitively depend on ModuleState as well.

How this works: unfortunately plumbing is not very friendly at the moment for preserving first-class Output values and they tend to get lost. The fixes remedy that just enough to grab these Output values in key locations. Then the code harvests all dependencies from first-class Output values and re-applies them to every output returned from Construct to broadcast them. A test is added that asserts on Pulumi statefile shapes to verify these dependencies.

Fixes #151
Fixes #234

Questions:

Does the code need to process ConstructRequest.{Dependencies, PropertyDependencies} or it is obsoleted by first-class Output values?
Do we need to copy up the URN of ModuleState resource and add it to the broadcast of module outputs, so that resources that consume the outputs of the Component resource not only depend on the Component and indirectly on ModuleState because it is the child of the Component, but also directly depend on the ModuleState?
Do dependencies work through first-class providers? That might need further work around Configure; Test: dependencies through explicit provider configuration #187 is tracking

t0yv0 · 2025-03-05T22:40:32Z

pkg/property/unmarshal_test.go

@@ -62,6 +66,11 @@ func TestUnmarhsalPropertiesThreadThroughRegisterResource(t *testing.T) {
 					Element: resource.NewStringProperty(""),
 				}),
 			},
+			inputsReceived: resource.PropertyMap{
+				"foo": resource.NewOutputProperty(resource.Output{


Unknowns get normalized to Outputs. That's interesting.

t0yv0 · 2025-03-05T22:40:43Z

pkg/property/unmarshal_test.go

@@ -70,15 +79,20 @@ func TestUnmarhsalPropertiesThreadThroughRegisterResource(t *testing.T) {
 					Element: resource.NewStringProperty("SECRET"),
 				}),
 			},
+			inputsReceived: resource.PropertyMap{


Secrets get normalized to outputs as well.

t0yv0 · 2025-03-05T22:40:59Z

pkg/property/unmarshal_test.go

 		},
 		{
 			name: "array",
 			inputs: resource.PropertyMap{
 				"foo": resource.NewArrayProperty([]resource.PropertyValue{
 					resource.NewStringProperty("foo"),
-					resource.NewSecretProperty(&resource.Secret{


Just simplify this since secrets get normalized to outputs, avoid them.

t0yv0 · 2025-03-05T22:41:04Z

pkg/property/unmarshal_test.go

@@ -87,9 +101,74 @@ func TestUnmarhsalPropertiesThreadThroughRegisterResource(t *testing.T) {
 			inputs: resource.PropertyMap{
 				"foo": resource.NewObjectProperty(resource.PropertyMap{
 					"p1": resource.NewStringProperty("foo"),
-					"p2": resource.NewSecretProperty(&resource.Secret{
-						Element: resource.NewStringProperty("SECRET"),


Fixes #151

t0yv0 · 2025-03-20T21:03:32Z

This change is part of the following stack:

test: establish a baseline for testing dep propagation #236
- Fix dependency handling #179 ◀

_{Change managed by git-spice.}

t0yv0 · 2025-03-20T21:43:48Z

@justinvp somehow can't tag you as reviewer, strange GH features.

tests/examples_test.go

tests/acc_test.go

guineveresaenger

I've verified that this indeed fixes #234.

Do we have confidence that turning Unknowns and Secrets into OutputProperty is safe?

pkg/property/unmarshal_test.go

pkg/modprovider/server.go

t0yv0 · 2025-03-20T23:36:00Z

Do we have confidence that turning Unknowns and Secrets into OutputProperty is safe?

Those are semantically equivalent. Also, secretes + unknowns should be under test in this repo AFAIK, though maybe something somewhere is lacking in rigor.

guineveresaenger

Really appreciate the comments here, thank you for letting me nit pick them.

justinvp

LGTM. Seems worth merging as-is.

To answer some of your questions...

Does the code need to process ConstructRequest.{Dependencies, PropertyDependencies} or it is obsoleted by first-class Output values?

Just a note that "PropertyDependencies" is actually called inputDependencies on ConstructRequest, which I assume is what you mean. Yes, it is obsoleted by first-class Output values. Since you're handling that now, you can ignore ConstructRequest.InputDependencies.

Dependencies represents the ResourceOptions.DependsOn. You will want to handle it. You have this code:

pulumi-terraform-module/pkg/modprovider/server.go

Lines 361 to 365 in ab31764

    
           return pulumiprovider.Construct(ctx, req, s.hostClient.EngineConn(), func( 
        
           	ctx *pulumi.Context, typ, name string, 
        
           	_ pulumiprovider.ConstructInputs, 
        
           	_ pulumi.ResourceOption, 
        
           ) (*pulumiprovider.ConstructResult, error) {

The ResourceOption being passed into the callback (currently ignored) will have the DependsOn filled in for you based on the ConstructRequest.Dependencies, via https://github.com/pulumi/pulumi/blob/6bb0e44f1bb9119435d9ec02217f5ebba9c4b9f3/sdk/go/pulumi/provider.go#L117-L119

You're going to want to think about passing that ResourceOption to your NewModuleComponentResource inside Construct, so the actual component gets the resource options that the user specified. I think it's a matter of passing it as the last arg of NewModuleComponentResource.

if Pulumi resources depend on any of the module's outputs, they now also will depend on all the dependencies of the module's inputs

Do we need to copy up the URN of ModuleState resource and add it to the broadcast of module outputs, so that resources that consume the outputs of the Component resource not only depend on the Component and indirectly on ModuleState because it is the child of the Component, but also directly depend on the ModuleState?

It may be sufficient to only have the module's outputs depend on ModuleState, if ModuleState depends on all the inputs. Then you might not have to propagate all the input dependencies to the module outputs.

t0yv0 · 2025-03-21T02:02:08Z

Test non-deterministic re-orders the deps sigh. I'll fix up.

      	resource.PropertyKey("max"): {
        		urn.URN("urn:pulumi:test::ts-dep-tester::randmod:index:Module::myrandmod"),
        		urn.URN("urn:pulumi:test::ts-dep-tester::random:index/randomInteger:RandomInteger::seed"),
        		urn.URN("urn:pulumi:test::ts-dep-tester::randmod:index:Module::myrandmod"),
        	},

t0yv0 commented Mar 5, 2025

View reviewed changes

t0yv0 force-pushed the t0yv0/fix-151 branch from 64aa93f to d8ab048 Compare March 20, 2025 17:53

guineveresaenger mentioned this pull request Mar 20, 2025

Dependency information lost for RDS module depending on AWS resources #234

Open

t0yv0 added 4 commits March 20, 2025 17:01

Fix first-class output turnaround

5139a16

Fixes #151

Simplify

36515de

Flow first-class outputs to module state inputs

ae541ce

Demonstrate new dependencies being tracked

26877e4

t0yv0 changed the base branch from main to t0yv0/dependencies-baseline March 20, 2025 21:03

t0yv0 force-pushed the t0yv0/fix-151 branch from d8ab048 to 26877e4 Compare March 20, 2025 21:03

t0yv0 mentioned this pull request Mar 20, 2025

test: establish a baseline for testing dep propagation #236

Open

t0yv0 requested a review from guineveresaenger March 20, 2025 21:04

t0yv0 added 5 commits March 20, 2025 17:05

See if RDS destroy is fixed

186c77f

Remove one more TODO

45a0225

Test transitivity

5ff5360

More suspect locations for FC-outputs

891385e

Lint

42aee05

t0yv0 requested a review from corymhall March 20, 2025 21:43

Explain why KeepOutputs: false in CheckConfig/Configure

5408e41

t0yv0 commented Mar 20, 2025

View reviewed changes

tests/examples_test.go Outdated Show resolved Hide resolved

t0yv0 commented Mar 20, 2025

View reviewed changes

tests/acc_test.go Outdated Show resolved Hide resolved

Fix transitive dependencies

fa46269

t0yv0 changed the title ~~Fix first-class output turnaround~~ Fix dependency handling Mar 20, 2025

t0yv0 marked this pull request as ready for review March 20, 2025 22:47

t0yv0 requested a review from justinvp March 20, 2025 22:48

guineveresaenger reviewed Mar 20, 2025

View reviewed changes

pkg/property/unmarshal_test.go Outdated Show resolved Hide resolved

pkg/modprovider/server.go Outdated Show resolved Hide resolved

t0yv0 requested a review from guineveresaenger March 20, 2025 23:24

t0yv0 added 2 commits March 20, 2025 19:28

Grammar

5f020b8

More grammar

60590d9

Remove commented out lines

bc79019

guineveresaenger approved these changes Mar 20, 2025

View reviewed changes

guineveresaenger mentioned this pull request Mar 21, 2025

Add basic e2e test for RDS module #203

Open

justinvp approved these changes Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dependency handling #179

Fix dependency handling #179

t0yv0 commented Mar 5, 2025 •

edited

Loading

t0yv0 Mar 5, 2025

t0yv0 Mar 5, 2025

t0yv0 Mar 5, 2025

t0yv0 Mar 5, 2025

t0yv0 commented Mar 20, 2025

t0yv0 commented Mar 20, 2025

guineveresaenger left a comment

t0yv0 commented Mar 20, 2025

guineveresaenger left a comment

justinvp left a comment

t0yv0 commented Mar 21, 2025

	return pulumiprovider.Construct(ctx, req, s.hostClient.EngineConn(), func(
	ctx *pulumi.Context, typ, name string,
	_ pulumiprovider.ConstructInputs,
	_ pulumi.ResourceOption,
	) (*pulumiprovider.ConstructResult, error) {

Fix dependency handling #179

Are you sure you want to change the base?

Fix dependency handling #179

Conversation

t0yv0 commented Mar 5, 2025 • edited Loading

t0yv0 Mar 5, 2025

Choose a reason for hiding this comment

t0yv0 Mar 5, 2025

Choose a reason for hiding this comment

t0yv0 Mar 5, 2025

Choose a reason for hiding this comment

t0yv0 Mar 5, 2025

Choose a reason for hiding this comment

t0yv0 commented Mar 20, 2025

t0yv0 commented Mar 20, 2025

guineveresaenger left a comment

Choose a reason for hiding this comment

t0yv0 commented Mar 20, 2025

guineveresaenger left a comment

Choose a reason for hiding this comment

justinvp left a comment

Choose a reason for hiding this comment

t0yv0 commented Mar 21, 2025

t0yv0 commented Mar 5, 2025 •

edited

Loading