Refactor Custom Resources to allow configuring timeouts and secrets #1795

flostadler · 2024-11-04T11:29:04Z

Previously it was assumed that the custom resources do not concern themselves with secretness or handle timeouts.
But this is necessary to support CloudFormation based Custom Resources.
Those can contain secret outputs and support configuring timeouts.

This change modifies the CustomResource interface to accommodate those requirements.
In detail this means that the Create/Update/Read lifecycle methods now returns a PropertyMap instead of a generic map[string]interface{} and take a timeout
parameter.

For reviewing this change, I'd first recommend having a look at the changes to the CustomResource interface in provider/pkg/resources/custom.go and then double check the refactoring changes resulting in that.

This change also introduces tests for the provider's CRUD lifecycle. As part of doing that, I added mocks using uber/gomock.

Relates to pulumi/pulumi-cdk#109

Previously it was assumed that the custom resources do not concern themselves with secretness or handle timeouts. But this is necessary to support CloudFormation based Custom Resources. Those can contain secret outputs and support configuring timeouts. This change modifies the `CustomResource` interface to accomomdate those requirements. In detail this means that the Create/Update/Read lifecycle methods now returns a `PropertyMap` instead of a generic `map[string]interface{}` and take a timeout parameter.

github-actions · 2024-11-04T11:30:08Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

codecov · 2024-11-04T11:33:19Z

Codecov Report

Attention: Patch coverage is 82.31293% with 26 lines in your changes missing coverage. Please review.

Project coverage is 45.36%. Comparing base (0166a63) to head (30e4e3b).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
provider/pkg/resources/extension_resource.go	0.00%	10 Missing ⚠️
provider/pkg/resources/mock_custom_resource.go	81.48%	10 Missing ⚠️
provider/pkg/provider/provider.go	80.64%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1795      +/-   ##
==========================================
+ Coverage   41.17%   45.36%   +4.18%     
==========================================
  Files          35       38       +3     
  Lines        5775     5879     +104     
==========================================
+ Hits         2378     2667     +289     
+ Misses       3187     2989     -198     
- Partials      210      223      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flostadler · 2024-11-04T11:37:36Z

The test coverage of the touched files is mostly through integration tests. I'll try to add some more tests to cover parts of this with units as well

corymhall

Does this change have any user facing impact, or is it purely internal?

flostadler · 2024-11-04T13:30:25Z

Does this change have any user facing impact, or is it purely internal?

This is purely internal

t0yv0 · 2024-11-04T14:04:04Z

provider/pkg/provider/provider.go

 	var createErr error
-	var payload map[string]interface{}
+	timeout := time.Duration(req.GetTimeout()) * time.Second


Unlike bridged providers, guessing there is no need to merge with schema-derived timeouts here?

Also, double-checking, this will be 0 if req.GetTimeout() is unset?

No, there's no notion of passing timeouts to the CloudControl based resources (yet).

At the current time no resource is using timeouts yet. This is just prep for the next changes related to CFN CustomResources

And yes, this will be 0 if no timeouts were specified

t0yv0 · 2024-11-04T14:05:09Z

provider/pkg/provider/provider.go

-// checkpointObject puts inputs in the `__inputs` field of the state.
-func checkpointObject(inputs resource.PropertyMap, outputs map[string]interface{}) resource.PropertyMap {
-	object := resource.NewPropertyMapFromMap(outputs)
-	object["__inputs"] = resource.MakeSecret(resource.NewObjectProperty(inputs))


This was a fairly interesting quirk.

t0yv0 · 2024-11-04T15:21:14Z

provider/pkg/resources/checkpoint.go

+	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
+)
+
+// CheckpointObject puts inputs in the `__inputs` field of the state.


Out of scope here but I am curious now do we understand why it's needed? Throwing a sentence on why we're doing this could be interesting.

This was done to make diff/read more robust (#17) and solve this issue: #16.

In detail, having access to the old inputs during Read.
I'll put this in the docs in a separate PR

t0yv0 · 2024-11-04T15:21:43Z

provider/pkg/resources/custom.go

 	// Read returns the outputs and the updated inputs of the resource.
-	Read(ctx context.Context, urn resource.URN, id string, oldInputs, oldOutputs resource.PropertyMap) (outputs map[string]any, inputs resource.PropertyMap, exists bool, err error)
+	Read(ctx context.Context, urn resource.URN, id string, oldInputs, oldOutputs resource.PropertyMap) (outputs resource.PropertyMap, inputs resource.PropertyMap, exists bool, err error)


Love a little more type-safety here.

t0yv0 · 2024-11-04T15:24:50Z

provider/pkg/resources/custom.go

@@ -12,11 +13,11 @@ type CustomResource interface {
 	// Check validates and transforms the inputs of the resource.
 	Check(ctx context.Context, urn resource.URN, randomSeed []byte, inputs, state resource.PropertyMap, defaultTags map[string]string) (resource.PropertyMap, []ValidationFailure, error)
 	// Create creates a new resource in the cloud provider and returns its unique identifier and outputs.
-	Create(ctx context.Context, urn resource.URN, inputs resource.PropertyMap) (identifier *string, outputs map[string]any, err error)
+	Create(ctx context.Context, urn resource.URN, inputs resource.PropertyMap, timeout time.Duration) (identifier *string, outputs resource.PropertyMap, err error)


This seems like a breaking change but we are expecting this interface to be internal? .. We're on v1.5.0. Nothing consumes it in other codebases? Should we work harder here as in introducing a side-by-side interface?

Yeah, this interface is internal. There's no way somebody could use this interface and plug it into aws-native unless they fork aws-native. It's used internally to wrap arbitrary custom resources:

pulumi-aws-native/provider/pkg/provider/provider.go

Lines 509 to 511 in 0166a63

p.customResources = map[string]resources.CustomResource{

metadata.ExtensionResourceToken: resources.NewExtensionResource(p.ccc),

}

flostadler · 2024-11-04T17:44:12Z

The test coverage of the touched files is mostly through integration tests. I'll try to add some more tests to cover parts of this with units as well

I added unit tests for the provider's CRUD operations. Coverage wise we should be good now and I managed to uncover a bug: #1796

Merge branch 'master' into flostadler/refactor-custom-resources

2105c89

flostadler requested review from t0yv0 and corymhall November 4, 2024 11:30

flostadler self-assigned this Nov 4, 2024

corymhall reviewed Nov 4, 2024

View reviewed changes

t0yv0 reviewed Nov 4, 2024

View reviewed changes

t0yv0 approved these changes Nov 4, 2024

View reviewed changes

flostadler added 2 commits November 4, 2024 18:27

Add mocks for the CloudControl Client and CustomResource interface

460c008

Add unit tests for provider CRUD

30e4e3b

flostadler requested a review from t0yv0 November 4, 2024 18:01

corymhall approved these changes Nov 4, 2024

View reviewed changes

t0yv0 approved these changes Nov 4, 2024

View reviewed changes

flostadler merged commit 2962615 into master Nov 5, 2024
18 checks passed

flostadler deleted the flostadler/refactor-custom-resources branch November 5, 2024 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Custom Resources to allow configuring timeouts and secrets #1795

Refactor Custom Resources to allow configuring timeouts and secrets #1795

flostadler commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

flostadler commented Nov 4, 2024

corymhall left a comment

flostadler commented Nov 4, 2024

t0yv0 Nov 4, 2024

flostadler Nov 4, 2024

flostadler Nov 4, 2024

t0yv0 Nov 4, 2024

t0yv0 Nov 4, 2024

flostadler Nov 4, 2024

t0yv0 Nov 4, 2024

t0yv0 Nov 4, 2024

flostadler Nov 4, 2024

flostadler commented Nov 4, 2024

	p.customResources = map[string]resources.CustomResource{
	metadata.ExtensionResourceToken: resources.NewExtensionResource(p.ccc),
	}

Refactor Custom Resources to allow configuring timeouts and secrets #1795

Refactor Custom Resources to allow configuring timeouts and secrets #1795

Conversation

flostadler commented Nov 4, 2024 • edited Loading

github-actions bot commented Nov 4, 2024

Does the PR have any schema changes?

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

flostadler commented Nov 4, 2024

corymhall left a comment

Choose a reason for hiding this comment

flostadler commented Nov 4, 2024

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

flostadler commented Nov 4, 2024

flostadler commented Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024 •

edited

Loading