Initial support for resource methods (authoring from Node.js, calling from Python) #7363

justinvp · 2021-06-24T17:40:27Z

This PR adds initial support for resource methods (via a new Call gRPC method similar to Invoke), support for implementing methods in Node.js, and support for calling methods in Python.

The PR is broken up into separate commits for easier review.

I've included a basic integration test, but plan on adding more test coverage in follow-up PRs (i.e. creating resources from methods, returning errors, and passing plain values).

I'll be opening separate PRs on the codegen and docgen changes. And follow-up PRs with support for calling and authoring methods in the other languages.

Part of #7072

justinvp · 2021-06-24T21:15:56Z

sdk/proto/provider.proto

+    // Call dynamically executes a method in the provider associated with a component resource.
+    rpc Call(CallRequest) returns (CallResponse) {}


@pgavlin, I'm curious to hear your thoughts on Call. The original design was for methods to be implemented with Invoke (and we'd augment Invoke with the additional information needed to be able to create resources). However, @lukehoban and I were wondering if it'd be better to have a separate RPC, since we'd be adapting the raw RPC values into the Pulumi SDK model in each language, just like Construct. Which then leaves Invoke for leaf functions that just provide raw compute / data sources.

I've added Call in this PR, and think it's probably best to have it, but part of me still feels a little unsure. On the one hand, I do think it simplifies the implementation in the providers: it's more clear when implementing Call that the context and values of the call are that of the Pulumi SDK model. On the other hand, we're now bifurcating functions listed in the schema where some are implemented with Invoke (leaf functions that don't accept inputs and don't create resources, i.e. raw compute / data sources) and others are implemented with Call (methods & leaf functions that do accept inputs and create resources).

If we're going to keep Call one question I have is what do we do about mocking. Our mocking APIs for Invoke are actually named Call (e.g. Node.js, Python, Go, .NET), so I don't think we'd be able to distinguish these unless we came up with a different name for Call (or used a different name for the mock API). I was thinking we may be able to get by having both Invoke and Call pass through to the mock's Call (similar to how both registerResource and readReasource are passed through to the mock newResource).

Briefly answered earlier.

I do think that we should keep Call: there is additional info that we need in this method that we really don't need for PoD functions (e.g. input dependencies, monitor info), and I agree that this simplifies the implementation.

I was thinking we may be able to get by having both Invoke and Call pass through to the mock's Call (similar to how both registerResource and readReasource are passed through to the mock newResource).

This would be my preference for now. It might be the case that in the future we need to separate remote component resource registration and Call out into their own mocks, as the shapes of the inputs are a bit different (i.e. the user's mocks might actually want to see Inputs rather than a bunch of data + dependency info).

justinvp · 2021-06-24T21:16:04Z

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

+            # Construct a provider reference from the given provider, if one is available on the resource.
+            provider_ref, version = None, ""
+            if res is not None:
+                if res._provider is not None:
+                    provider_urn = await res._provider.urn.future()
+                    provider_id = (await res._provider.id.future()) or rpc.UNKNOWN
+                    provider_ref = f"{provider_urn}::{provider_id}"
+                    log.debug(f"Call using provider {provider_ref}")
+                version = res._version or ""


Regular invokes accept an InvokeOptions which allows specifying the provider and version.

Having a generated method accept InvokeOptions as an argument felt unnatural to me since, if you wanted to specify a custom provider, you would have done so when creating the resource itself.

So I've made it so that the provider / version is picked up from the resource, which is passed into this function as part of the generated SDK code, e.g.:

def get_kubeconfig(__self__, profile_name: Optional[str] = None, role_arn: Optional[str] = None) -> pulumi.Output['Cluster.GetKubeconfigResult']: __args__ = dict() __args__['__self__'] = __self__ __args__['profileName'] = profile_name __args__['roleArn'] = role_arn return pulumi.runtime.call('eks:index:Cluster/getKubeconfig', __args__, res=__self__, typ=Cluster.GetKubeconfigResult)

There are two cases that I need to handle in subsequent changes:

If we are going to use Call for leave functions that accept inputs / create resources, we will want to support InvokeOptions for that, so I'll need to update this function to be able to pass either a resource or InvokeOptions to specify the custom provider.

If the instance of the resource was rehydrated from a resource reference, I don't think the res._provider or res._version will be available to us. For that case, we'll need to make sure we can fill in that information from the built-in pulumi:pulumi:getResource.

If we are going to use Call for leave functions that accept inputs / create resources, we will want to support InvokeOptions for that, so I'll need to update this function to be able to pass either a resource or InvokeOptions to specify the custom provider.

I believe that we will use Call for this as well, yes. I think that we can probably get away with eliding options from methods, but providing them for module-level functions.

justinvp · 2021-06-24T21:17:31Z

tests/integration/construct_component_methods/testcomponent/index.ts

+        if (opts?.urn) {
+            args = <any>{
+                first: undefined,
+                second: undefined,
+            };
+        }


This and the if on line 26 are annoying. This is to be able to support recreating an instance of Component that comes back as __self__ resource reference. I wish there was a cleaner way to enable this for components, so authors won't have to handle this.

Hm, yeah, these are bothersome. I'll give this some thought.

Again, I think this is something that can be improved by importing the generated SDK for a component. While the component instance will be different from the provider component class, both should fulfill the same public interface (which is a requirement for methods I believe). Then we can add some framework magic to do something like ComponentFunction.prototype.apply( this=SDKInstance, args...)

justinvp · 2021-06-24T21:19:36Z

tests/integration/construct_component_methods/testcomponent/index.ts

+        // Register any resources that can come back as resource references that need to be rehydrated.
+        pulumi.runtime.registerResourceModule("testcomponent", "index", {
+            version: this.version,
+            construct: (name, type, urn) => {
+                switch (type) {
+                    case "testcomponent:index:Component":
+                        return new Component(name, <any>undefined, { urn });
+                    default:
+                        throw new Error(`unknown resource type ${type}`);
+                }
+            },
+        });


Similarly, it's annoying that component authors would have to write this in order to provide support for methods (for __self__ to come back as a resource reference.

I'm actually wondering whether we could/should implicitly register any resource the first time it is created (in the Resource base class), so component authors don't have to even think about or write this.

I'm actually wondering whether we could/should implicitly register any resource the first time it is created (in the Resource base class), so component authors don't have to even think about or write this.

Ah, and this is a problem b/c component authors need these to be registered and don't have the benefit of the schema-generated registrations. I think that in the longer term, we can probably address this with code generation as well, but what you suggest would probably work for now. Given that the registrations would be taking place in the implementing component, I don't think that it would be likely (or perhaps possible) to see a reference to a resource prior to some instance of that resource being created.

Similarly, it's annoying that component authors would have to write this in order to provide support for methods (for self to come back as a resource reference.

Another option is to just require that call authors import their own generated SDKs. I have done this in working with astro.

tests/integration/construct_component_methods/testcomponent/index.ts

pgavlin · 2021-06-25T19:38:01Z

pkg/resource/deploy/source_eval.go

+// Call dynamically executes a method in the provider associated with a component resource.
+func (rm *resmon) Call(ctx context.Context, req *pulumirpc.CallRequest) (*pulumirpc.CallResponse, error) {


This looks good. I'd love to see some inner-loop tests in pkg/engine/lifecycletest.

pgavlin · 2021-06-25T19:43:11Z

sdk/proto/provider.proto

+    // Call dynamically executes a method in the provider associated with a component resource.
+    rpc Call(CallRequest) returns (CallResponse) {}


Briefly answered earlier.

I do think that we should keep Call: there is additional info that we need in this method that we really don't need for PoD functions (e.g. input dependencies, monitor info), and I agree that this simplifies the implementation.

I was thinking we may be able to get by having both Invoke and Call pass through to the mock's Call (similar to how both registerResource and readReasource are passed through to the mock newResource).

This would be my preference for now. It might be the case that in the future we need to separate remote component resource registration and Call out into their own mocks, as the shapes of the inputs are a bit different (i.e. the user's mocks might actually want to see Inputs rather than a bunch of data + dependency info).

pgavlin · 2021-06-25T19:48:31Z

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

+            # Construct a provider reference from the given provider, if one is available on the resource.
+            provider_ref, version = None, ""
+            if res is not None:
+                if res._provider is not None:
+                    provider_urn = await res._provider.urn.future()
+                    provider_id = (await res._provider.id.future()) or rpc.UNKNOWN
+                    provider_ref = f"{provider_urn}::{provider_id}"
+                    log.debug(f"Call using provider {provider_ref}")
+                version = res._version or ""


If we are going to use Call for leave functions that accept inputs / create resources, we will want to support InvokeOptions for that, so I'll need to update this function to be able to pass either a resource or InvokeOptions to specify the custom provider.

I believe that we will use Call for this as well, yes. I think that we can probably get away with eliding options from methods, but providing them for module-level functions.

pgavlin · 2021-06-25T19:49:30Z

tests/integration/construct_component_methods/testcomponent/index.ts

+        if (opts?.urn) {
+            args = <any>{
+                first: undefined,
+                second: undefined,
+            };
+        }


Hm, yeah, these are bothersome. I'll give this some thought.

pgavlin · 2021-06-25T19:53:31Z

tests/integration/construct_component_methods/testcomponent/index.ts

+        // Register any resources that can come back as resource references that need to be rehydrated.
+        pulumi.runtime.registerResourceModule("testcomponent", "index", {
+            version: this.version,
+            construct: (name, type, urn) => {
+                switch (type) {
+                    case "testcomponent:index:Component":
+                        return new Component(name, <any>undefined, { urn });
+                    default:
+                        throw new Error(`unknown resource type ${type}`);
+                }
+            },
+        });


I'm actually wondering whether we could/should implicitly register any resource the first time it is created (in the Resource base class), so component authors don't have to even think about or write this.

Ah, and this is a problem b/c component authors need these to be registered and don't have the benefit of the schema-generated registrations. I think that in the longer term, we can probably address this with code generation as well, but what you suggest would probably work for now. Given that the registrations would be taking place in the implementing component, I don't think that it would be likely (or perhaps possible) to see a reference to a resource prior to some instance of that resource being created.

EvanBoyle · 2021-06-28T20:49:37Z

sdk/proto/provider.proto

+    string tok = 1;                                        // the function token to invoke.
+    google.protobuf.Struct args = 2;                       // the arguments for the function invocation.
+    map<string, ArgumentDependencies> argDependencies = 3; // a map from argument keys to the dependencies of the argument.
+    string provider = 4;                                   // an optional reference to the provider to use for this invoke.


Does this provider refer to the component provider?

How does this interact with the provider map that is supplied on construct? How does a call impl retrieve that?

pulumi/sdk/proto/provider.proto

Line 269 in 634e97c

map<string, string> providers = 13; // the map of providers to use for this resource's children.

Along the same lines, do resources created within call auto-parent to the component? Should this be configurable?

Do we need to provide ways to expose all resource properties to call?

Also ~~config, and~~ config secret keys.

Does this provider refer to the component provider?

No, it's the provider where the Call is implemented. This is the same as with Invoke. When making an Invoke or Call, you call it on the monitor, and you can include the provider/version to use:

pulumi/sdk/proto/resource.proto

Line 26 in 398fb2f

rpc Invoke(InvokeRequest) returns (InvokeResponse) {}

And the engine will lookup the specified provider (otherwise using the default provider) and forward the call:

pulumi/pkg/resource/deploy/source_eval.go

Lines 559 to 561 in 398fb2f

prov, err := getProviderFromSource(rm.providers, rm.defaultProviders, providerReq, req.GetProvider())

if err != nil {

return nil, err

pulumi/sdk/proto/provider.proto

Line 37 in 398fb2f

rpc Invoke(InvokeRequest) returns (InvokeResponse) {}

How does this interact with the provider map that is supplied on construct? How does a call impl retrieve that?

It's unrelated. There's not currently a way to fetch a component's map of providers outside of when the component is being registered. If there are particular providers that you want to use when creating resources from methods, you'll either have to have the component save them in the state, or pass them in as arguments.

Along the same lines, do resources created within call auto-parent to the component? Should this be configurable?

No, there's no auto-parenting.

Do we need to provide ways to expose all resource properties to call?

I'm not sure what you mean. For methods, there is a __self__ argument that is a resource reference to the component, so when it is deserialized, it will have all of the component's state available for use.

config secret keys.

Yep, I added this while rebasing.

From a practical perspective:

const x = new Cluster("foo", { providers: { aws: explicitAwsProvider }}); const y = x.AddSubnet();

I would find it very surprising as a user that resources created in x.AddSubnet() used the default AWS provider. The interaction of resources options provided to a component and subsequent usage of Call is something we should make an explicit design decision on.

Personally, this seems like it will be common enough that telling users to transfer resource options into state in order for Call to have access is way too painful.

Personally, this seems like it will be common enough that telling users to transfer resource options into state in order for Call to have access is way too painful.

I agree. We should match the existing behavior to the greatest extent possible, which may mean exploring additional options for persistence. This might force the issue a bit on using a resource reference -> object table to ensure that any resource that has been marshaller in or out of a process is always represented by the same object (which would of course simplify other aspects of authoring as well).

This might force the issue a bit on using a resource reference -> object table to ensure that any resource that has been marshaller in or out of a process is always represented by the same object

Yes, this would definitely make things easier for implementers. Let me explore this a bit in the next day or two. If we're going to make a change along these lines, it'll be in a subsequent PR(s).

sdk/nodejs/provider/server.ts

EvanBoyle · 2021-06-28T22:21:58Z

sdk/nodejs/resource.ts

-        this.__data = remote ? Promise.resolve(<TData>{}) : this.initializeAndRegisterOutputs(args);
+        super(type, name, /*custom:*/ false, /*props:*/ remote || opts?.urn ? args : {}, opts, remote);
+        this.__registered = remote || !!opts?.urn;
+        this.__data = remote || opts?.urn ? Promise.resolve(<TData>{}) : this.initializeAndRegisterOutputs(args);


These changes shouldn't be necessary if the caller specifies remote: true. This would be taken care of automatically if provider implementers import their own SDKs.

EvanBoyle

Overall the approach appears to make sense. A couple of comments to consider, and some tests/cases that should be added. Fine by me to merge as is, but we should open issues to track follow ups first. There are two changes that should be merged in from recent node runtime work.

Summary of main concerns:

Tests for methods that create resources, with artificial latency injected. (concerns about resources created within apply).
Plan for supporting resource options for call (resource options inherited from parent somehow? explicitly provided?).

Call is similar to Invoke, but includes additional information to allow adapting the RPC values into the Pulumi SDK model (for each language).

This tests basic functionality of a method implementation on a component in Node.js and calling from Python.

justinvp · 2021-06-30T14:47:33Z

Follow-up items are being tracked in #7072

justinvp mentioned this pull request Jun 24, 2021

Support methods for multi-language components #7072

Closed

12 tasks

justinvp commented Jun 24, 2021

View reviewed changes

tests/integration/construct_component_methods/testcomponent/index.ts Outdated Show resolved Hide resolved

justinvp requested review from pgavlin and EvanBoyle June 25, 2021 18:06

pgavlin approved these changes Jun 25, 2021

View reviewed changes

EvanBoyle reviewed Jun 28, 2021

View reviewed changes

sdk/nodejs/provider/server.ts Show resolved Hide resolved

EvanBoyle reviewed Jun 28, 2021

View reviewed changes

sdk/nodejs/provider/server.ts Outdated Show resolved Hide resolved

EvanBoyle reviewed Jun 28, 2021

View reviewed changes

EvanBoyle approved these changes Jun 28, 2021

View reviewed changes

This was referenced Jun 28, 2021

[sdk/nodejs] Support for calling methods #7377

Merged

[sdk/go] Support for implementing methods in provider #7379

Merged

justinvp force-pushed the justin/methods branch 2 times, most recently from 70e3b6e to 7851a52 Compare June 29, 2021 19:26

justinvp added 6 commits June 30, 2021 06:25

Add Call gRPC method

764e1ef

Call is similar to Invoke, but includes additional information to allow adapting the RPC values into the Pulumi SDK model (for each language).

[sdk/nodejs] Support for implementing methods in provider

6562695

[sdk/python] Support for calling methods

615e6ea

Integration test

9158f01

This tests basic functionality of a method implementation on a component in Node.js and calling from Python.

Update CHANGELOG_PENDING.md

b76be96

Keep resource refs plain

be1f99a

justinvp force-pushed the justin/methods branch from 7851a52 to be1f99a Compare June 30, 2021 13:25

justinvp merged commit 84b574f into master Jun 30, 2021

pulumi-bot deleted the justin/methods branch June 30, 2021 14:48

This was referenced Jul 1, 2021

[codegen/python] Emit resource methods #7364

Merged

[codegen/go] Emit resource methods #7438

Merged

emiliza added this to the 0.58 milestone Jul 7, 2021

justinvp mentioned this pull request Jul 26, 2021

Methods: Make __self__ resource options available for implicit use by children from methods #7650

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for resource methods (authoring from Node.js, calling from Python) #7363

Initial support for resource methods (authoring from Node.js, calling from Python) #7363

justinvp commented Jun 24, 2021

justinvp Jun 24, 2021 •

edited

Loading

pgavlin Jun 25, 2021

justinvp Jun 24, 2021

pgavlin Jun 25, 2021

justinvp Jun 24, 2021

pgavlin Jun 25, 2021

EvanBoyle Jun 28, 2021

justinvp Jun 24, 2021

pgavlin Jun 25, 2021

EvanBoyle Jun 28, 2021

pgavlin Jun 25, 2021

pgavlin Jun 25, 2021

pgavlin Jun 25, 2021

pgavlin Jun 25, 2021

pgavlin Jun 25, 2021

EvanBoyle Jun 28, 2021

EvanBoyle Jun 28, 2021

EvanBoyle Jun 28, 2021 •

edited

Loading

justinvp Jun 29, 2021

EvanBoyle Jun 29, 2021

pgavlin Jun 29, 2021

justinvp Jun 30, 2021

EvanBoyle Jun 28, 2021

EvanBoyle left a comment

justinvp commented Jun 30, 2021

		// Call dynamically executes a method in the provider associated with a component resource.
		rpc Call(CallRequest) returns (CallResponse) {}

		// Call dynamically executes a method in the provider associated with a component resource.
		func (rm resmon) Call(ctx context.Context, req pulumirpc.CallRequest) (*pulumirpc.CallResponse, error) {

	prov, err := getProviderFromSource(rm.providers, rm.defaultProviders, providerReq, req.GetProvider())
	if err != nil {
	return nil, err

Initial support for resource methods (authoring from Node.js, calling from Python) #7363

Initial support for resource methods (authoring from Node.js, calling from Python) #7363

Conversation

justinvp commented Jun 24, 2021

justinvp Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanBoyle Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanBoyle left a comment

Choose a reason for hiding this comment

justinvp commented Jun 30, 2021

justinvp Jun 24, 2021 •

edited

Loading

EvanBoyle Jun 28, 2021 •

edited

Loading