Allow component resources to inherit `providers` from component resources #10933

iwahbe · 2022-10-05T18:18:02Z

Description

Fixes #10640

Checklist

I have added tests that prove my fix is effective or that my feature works

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

pulumi-bot · 2022-10-05T18:21:34Z

Changelog

[uncommitted] (2022-10-20)

Bug Fixes

[cli/engine] Component Resources inherit thier parents providers map
#10933

Frassle

Can't this be fixed in the engine rather than the SDKs?

iwahbe · 2022-10-05T23:44:14Z

@Frassle This can be fixed in the engine. It's a cleaner fix but requires bifurcating providers logic. It needs to occur before remote component resources are constructed, which means it needs to live in the resmon.

justinvp · 2022-10-06T21:01:14Z

Any tests?

iwahbe · 2022-10-06T23:35:18Z

Any tests?

I was trying to come up with a way to test this without writing a full integration test. I'll add an integration test shortly.

Frassle · 2022-10-07T07:29:30Z

You could test this in pulumi_tests.go, there's a lot of other engine tests there.

Frassle · 2022-10-07T08:42:21Z

sdk/python/lib/pulumi/runtime/resource.py

@@ -154,7 +154,7 @@ async def prepare_resource(
    # For remote resources, merge any provider opts into a single dict, and then create a new dict with all of the
    # resolved provider refs.
    provider_refs: Dict[str, Optional[str]] = {}
-    if remote and opts is not None:
+    if (remote or not custom) and opts is not None:


This is just "not custom" now isn't it? Resources are either LocalComponent, RemoteComponent, or Custom (remote=true, custom=true should be inexpressible). Also is a similar change not needed in the dotnet code?

I couldn't find the equivalent logic. It is just a branch on custom:

pulumi/sdk/dotnet/Pulumi/Resources/Resource.cs

Lines 221 to 259 in 85cfd79

if (custom)

{

var provider = customOpts?.Provider;

if (provider == null)

{

if (options.Parent != null)

{

// If no provider was given, but we have a parent, then inherit the

// provider from our parent.

options.Provider = options.Parent.GetProvider(type);

}

}

else

{

// If a provider was specified, add it to the providers map under this type's package so that

// any children of this resource inherit its provider.

var typeComponents = type.Split(":");

if (typeComponents.Length == 3)

{

var pkg = typeComponents[0];

this._providers = this._providers.SetItem(pkg, provider);

}

}

}

else

{

// Note: we checked above that at most one of options.provider or options.providers

// is set.

// If options.provider is set, treat that as if we were given a array of provider

// with that single value in it. Otherwise, take the array of providers, convert it

// to a map and combine with any providers we've already set from our parent.

var providerList = options.Provider != null

? new List<ProviderResource> { options.Provider }

: componentOpts?.Providers;

this._providers = this._providers.AddRange(ConvertToProvidersMap(providerList));

}

.NET should work as is.

iwahbe · 2022-10-07T17:15:45Z

Writing the necessary test is taking longer than I thought. This is what I'm thinking:
We need a test that looks like this:

Create provider p of package pkgA.
Create language specific component c1, passing providers=[p].
c1 creates MLC component c2 of package pkgA as a child, passing parent=c1 on creation.
c2 creates custom resource r of package pkgB.
We assert that r was created with provider p.

iwahbe · 2022-10-07T18:45:46Z

You could test this in pulumi_tests.go, there's a lot of other engine tests there.

It doesn't look like pulumi_test.go puts resmon under test (which is where the change in engine logic is located).

Frassle · 2022-10-07T20:18:39Z

It doesn't look like pulumi_test.go puts resmon under test (which is where the change in engine logic is located).

"pkg/engine/lifecycletest/pulumi_test.go"? That tests resmon because it tests via sending grpc messages to a resource monitor (either fully over grpc, or just method calls).

iwahbe · 2022-10-10T19:20:12Z

It doesn't look like pulumi_test.go puts resmon under test (which is where the change in engine logic is located).

"pkg/engine/lifecycletest/pulumi_test.go"? That tests resmon because it tests via sending grpc messages to a resource monitor (either fully over grpc, or just method calls).

Having though about it longer, I think it's still better to have an integration test here. We ensure that the SDK and the engine are behaving appropriately together.

Frassle · 2022-10-18T21:31:53Z

tests/integration/construct_component/testcomponent-go/main.go

@@ -41,7 +41,7 @@ func NewResource(ctx *pulumi.Context, name string, echo pulumi.Input,
 	opts ...pulumi.ResourceOption) (*Resource, error) {
 	args := &ResourceArgs{Echo: echo}
 	var resource Resource
-	err := ctx.RegisterResource("testcomponent:index:Resource", name, args, &resource, opts...)
+	err := ctx.RegisterResource(providerName+":index:Resource", name, args, &resource, opts...)


I don't see providerName declared in here?

Its declared on line 104.

pulumi/tests/integration/construct_component/testcomponent-go/main.go

Line 104 in 12c75e9

const providerName = "testcomponent"

Frassle · 2022-10-18T21:33:05Z

pkg/resource/deploy/source_eval.go

@@ -1115,6 +1131,13 @@ func (rm *resmon) RegisterResource(ctx context.Context,

 	// If this is a remote component, fetch its provider and issue the construct call. Otherwise, register the resource.
 	var result *RegisterResult
+	if !custom {
+		defer func() {


Why is this defered? Can't we always just store the provider list as it comes in?

We know the value when we declare the deferred function (L1135), but we don't know the key (URN) to store it at. Note that the defer only finishes the store if we successfully stored an URN.

Should we just move this code down to L1197 when we have the result, so we don't have to have the defer here?

If there's a reason it needs to live here with the defer, it needs a comment explaining why.

justinvp

LGTM. It took me a while to parse what the test was doing (maybe my brain is just foggy from being sick).

Isn't this only testing the Go SDK? Any reason we're not testing the other SDKs?

Does Java have the same problem? Should we be sending the providers to the engine there as well?

pkg/resource/deploy/source_eval.go

justinvp · 2022-10-20T03:29:20Z

pkg/resource/deploy/source_eval.go

@@ -1115,6 +1131,13 @@ func (rm *resmon) RegisterResource(ctx context.Context,

 	// If this is a remote component, fetch its provider and issue the construct call. Otherwise, register the resource.
 	var result *RegisterResult
+	if !custom {
+		defer func() {


Should we just move this code down to L1197 when we have the result, so we don't have to have the defer here?

If there's a reason it needs to live here with the defer, it needs a comment explaining why.

tests/integration/component_setup.sh

tests/integration/construct_component/testcomponent2-go/main.go

tests/integration/construct_nested_component/go/Pulumi.yaml

tests/integration/construct_nested_component/go/main.go

This is approach needs to occur in `resmon` since `*resmon` dispatches `Construct` calls directly. TS: Send providers map for all components Ensure that `providers` is passed in all SDKs The .NET SDK already does the right thing Add golang integration test Add local dependency Fix test Use new test Get test working Fix nits

iwahbe · 2022-10-20T22:45:57Z

bors r+

10933: Allow component resources to inherit `providers` from component resources r=iwahbe a=iwahbe  # Description  Fixes #10640 ## Checklist  - [x] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Ian Wahbe <ian@wahbe.com>

iwahbe · 2022-10-20T22:46:49Z

Java has the same semantics as .NET, which already did the correct thing. I added a typescript test here. It was only Go because one SDK was sufficient to test that the engine did there right thing.

bors · 2022-10-20T23:12:01Z

Canceled.

iwahbe · 2022-10-20T23:12:36Z

bors r+

bors · 2022-10-20T23:47:44Z

Build succeeded:

bors-ok

Frassle reviewed Oct 5, 2022

View reviewed changes

iwahbe force-pushed the iwahbe/10640/inherit-through-component-resources branch from a932ea4 to 9f88b05 Compare October 5, 2022 21:06

iwahbe requested review from Frassle and justinvp October 5, 2022 23:55

iwahbe self-assigned this Oct 5, 2022

Frassle reviewed Oct 7, 2022

View reviewed changes

iwahbe requested a review from Frassle October 10, 2022 19:19

iwahbe force-pushed the iwahbe/10640/inherit-through-component-resources branch 3 times, most recently from 48b929e to 12c75e9 Compare October 14, 2022 01:09

Frassle reviewed Oct 18, 2022

View reviewed changes

iwahbe requested a review from Frassle October 18, 2022 22:32

justinvp approved these changes Oct 20, 2022

View reviewed changes

iwahbe mentioned this pull request Oct 20, 2022

Component children of component resources don't inherit their parents providers #10640

Closed

iwahbe force-pushed the iwahbe/10640/inherit-through-component-resources branch from 1c220f3 to e387ba8 Compare October 20, 2022 22:36

iwahbe force-pushed the iwahbe/10640/inherit-through-component-resources branch from 701fb7a to e387ba8 Compare October 20, 2022 23:11

bors bot merged commit a5cac1d into master Oct 20, 2022

bors bot deleted the iwahbe/10640/inherit-through-component-resources branch October 20, 2022 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow component resources to inherit `providers` from component resources #10933

Allow component resources to inherit `providers` from component resources #10933

iwahbe commented Oct 5, 2022 •

edited

pulumi-bot commented Oct 5, 2022 •

edited

Frassle left a comment

iwahbe commented Oct 5, 2022

justinvp commented Oct 6, 2022

iwahbe commented Oct 6, 2022

Frassle commented Oct 7, 2022

Frassle Oct 7, 2022

iwahbe Oct 7, 2022 •

edited

iwahbe commented Oct 7, 2022

iwahbe commented Oct 7, 2022

Frassle commented Oct 7, 2022

iwahbe commented Oct 10, 2022

Frassle Oct 18, 2022

iwahbe Oct 18, 2022

Frassle Oct 18, 2022

iwahbe Oct 18, 2022

justinvp Oct 20, 2022

justinvp left a comment

justinvp Oct 20, 2022

iwahbe commented Oct 20, 2022

iwahbe commented Oct 20, 2022

bors bot commented Oct 20, 2022

iwahbe commented Oct 20, 2022

bors bot commented Oct 20, 2022

	if (custom)
	{
	var provider = customOpts?.Provider;
	if (provider == null)
	{
	if (options.Parent != null)
	{
	// If no provider was given, but we have a parent, then inherit the
	// provider from our parent.

	options.Provider = options.Parent.GetProvider(type);
	}
	}
	else
	{
	// If a provider was specified, add it to the providers map under this type's package so that
	// any children of this resource inherit its provider.
	var typeComponents = type.Split(":");
	if (typeComponents.Length == 3)
	{
	var pkg = typeComponents[0];
	this._providers = this._providers.SetItem(pkg, provider);
	}
	}
	}
	else
	{
	// Note: we checked above that at most one of options.provider or options.providers
	// is set.

	// If options.provider is set, treat that as if we were given a array of provider
	// with that single value in it. Otherwise, take the array of providers, convert it
	// to a map and combine with any providers we've already set from our parent.
	var providerList = options.Provider != null
	? new List<ProviderResource> { options.Provider }
	: componentOpts?.Providers;

	this._providers = this._providers.AddRange(ConvertToProvidersMap(providerList));
	}

Allow component resources to inherit providers from component resources #10933

Allow component resources to inherit providers from component resources #10933

Conversation

iwahbe commented Oct 5, 2022 • edited

Description

Checklist

pulumi-bot commented Oct 5, 2022 • edited

Changelog

[uncommitted] (2022-10-20)

Bug Fixes

Frassle left a comment

Choose a reason for hiding this comment

iwahbe commented Oct 5, 2022

justinvp commented Oct 6, 2022

iwahbe commented Oct 6, 2022

Frassle commented Oct 7, 2022

Frassle Oct 7, 2022

Choose a reason for hiding this comment

iwahbe Oct 7, 2022 • edited

Choose a reason for hiding this comment

iwahbe commented Oct 7, 2022

iwahbe commented Oct 7, 2022

Frassle commented Oct 7, 2022

iwahbe commented Oct 10, 2022

Frassle Oct 18, 2022

Choose a reason for hiding this comment

iwahbe Oct 18, 2022

Choose a reason for hiding this comment

Frassle Oct 18, 2022

Choose a reason for hiding this comment

iwahbe Oct 18, 2022

Choose a reason for hiding this comment

justinvp Oct 20, 2022

Choose a reason for hiding this comment

justinvp left a comment

Choose a reason for hiding this comment

justinvp Oct 20, 2022

Choose a reason for hiding this comment

iwahbe commented Oct 20, 2022

iwahbe commented Oct 20, 2022

bors bot commented Oct 20, 2022

iwahbe commented Oct 20, 2022

bors bot commented Oct 20, 2022

Allow component resources to inherit `providers` from component resources #10933

Allow component resources to inherit `providers` from component resources #10933

iwahbe commented Oct 5, 2022 •

edited

pulumi-bot commented Oct 5, 2022 •

edited

iwahbe Oct 7, 2022 •

edited