-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add implementation for CloudFormation Custom Resource Emulator #1806
Conversation
This change is part of the following stack: Change managed by git-spice. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
baf7ba4
to
1d4b118
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1806 +/- ##
==========================================
+ Coverage 46.81% 48.13% +1.32%
==========================================
Files 42 43 +1
Lines 6167 6554 +387
==========================================
+ Hits 2887 3155 +268
- Misses 3052 3156 +104
- Partials 228 243 +15 ☔ View full report in Codecov by Sentry. |
59357d1
to
94cfa06
Compare
60b5047
to
4cd4ddb
Compare
94cfa06
to
19b72f2
Compare
19b72f2
to
bc9e116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor comments, otherwise this is awesome! Great work!
// - Initiates cleanup of old resource | ||
// - Sends DELETE event for old PhysicalResourceId | ||
// 5. Returns updated properties and new PhysicalResourceId | ||
func (c *cfnCustomResource) Update(ctx context.Context, urn urn.URN, id string, inputs, oldInputs, state resource.PropertyMap, timeout time.Duration) (resource.PropertyMap, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for us to get the resource options here, specifically the retainOnDelete
option? I know there were times that I would set the retainOnDelete
to true
for custom resources because they didn't handle delete well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, not that I'm aware of: https://pulumi-developer-docs.readthedocs.io/latest/docs/_generated/proto/provider.html#pulumirpc-updaterequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only MLCs have access to retainOnDelete it seems:
https://github.com/pulumi/pulumi/blob/master/proto/pulumi/provider.proto#L473
For CustomResources that are traditional I don't think the engine ever cooperates with the provider on this. The engine handles it all engine-side.
}, | ||
"serviceToken": { | ||
Description: "The service token to use for the Custom Resource. The service token is invoked when the resource is created, updated, or deleted.\n" + | ||
"This can be a Lambda Function ARN with optional version or alias identifiers.\n\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence here is a bit confusing. "This can be a Lambda Function ARN", perhaps give a quick example?
Also should this be marked Secret/Sensitive in the scehema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lambda function ARNs (including alias or version) are a very common concept in AWS. I don't think we should go into too much detail about it here.
This is also not a secret, ARNs are just unique IDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused that this is called "serviceToken". This made me think of access tokens or some such. If there's precedent of calling lambda ARNs "tokens" then TIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't like the name... It's what CloudFormation called this property. I think there's precedence to stick to their naming patterns given we're trying to emulate their behavior/API
// Read returns the current inputs and outputs of the custom resource because CFN custom resources do not store state. | ||
// They are just a stateless wrapper around a Lambda function or SNS topic. | ||
func (c *cfnCustomResource) Read(ctx context.Context, urn urn.URN, id string, oldInputs resource.PropertyMap, oldState resource.PropertyMap) (resource.PropertyMap, resource.PropertyMap, bool, error) { | ||
if len(oldState) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment here that len(oldState) is trying to guess if we are in a Read-for-import.
The developer docs are improving but not quite there yet but are being improved (cc @lunaris )
|
||
type Clock interface { | ||
Now() time.Time | ||
Since(time.Time) time.Duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Go likes minimal interfaces so you only really need Now(), as Since can be computed from Now in a func
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that would clutter the implementation. The purpose of this interface is to make the code testable. I'd rather just pass through to the stdlib than add custom code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm, I guess this is a bit of a moot point from my side given that you can subtract times and get the duration that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's really a small nit, feel free to disregard.
// It returns the inputs, validation failures, and an error if the inputs cannot be unmarshalled. | ||
func (c *cfnCustomResource) Check(ctx context.Context, urn urn.URN, randomSeed []byte, inputs resource.PropertyMap, state resource.PropertyMap, defaultTags map[string]string) (resource.PropertyMap, []ValidationFailure, error) { | ||
var typedInputs CfnCustomResourceInputs | ||
_, err := resourcex.Unmarshal(&typedInputs, inputs, resourcex.UnmarshalOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not fully trust this function, and reading the docs quickly is not very reassuring some of the concerns I have.
I think we might need to think about this a bit (probably out of scope of this PR but before being fully confident).
What happens with unknowns, dependencies and secrets?
For secrets it's a design question - custom resources do not support them right? So we need to support secrets through the black-box transformation I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already quite heavily using the resourcex.Decode
and resourcex.Unmarshal
methods in the provider. From what I can tell they work fine.
Additionally we're not using the unmarshalled typedInputs
to construct the outputs, but rather use the inputs
resource.PropertyMap
to construct the outputs.
From that aspect, we're properly handling secrets.
Unknowns are a good point though. I'll try to double check whether this can receive unknowns if SupportsPreview: false
.
// - On success: Returns PhysicalResourceId and properties | ||
// - On failure: Returns error with reason | ||
// - Handles `NoEcho` for sensitive data | ||
func (c *cfnCustomResource) Create(ctx context.Context, urn urn.URN, inputs resource.PropertyMap, timeout time.Duration) (*string, resource.PropertyMap, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if this is ever called in Preview, but looks like the provider is SupportsPreview: false so we're good. It never is! No unknowns then.
|
||
// Check validates the inputs of the resource and applies default values if necessary. | ||
// It returns the inputs, validation failures, and an error if the inputs cannot be unmarshalled. | ||
func (c *cfnCustomResource) Check(ctx context.Context, urn urn.URN, randomSeed []byte, inputs resource.PropertyMap, state resource.PropertyMap, defaultTags map[string]string) (resource.PropertyMap, []ValidationFailure, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might still receive unknowns but I'm not 100% sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double check. It's not a big deal if we stick to using the PropertyMap for Check
}, | ||
}, | ||
{ | ||
name: "SecretResponse", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to see here what is returned to the engine from Create in this test case?
Perhaps with hexops/autogold
if it's too much trouble to write by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're asserting this with the assertion in line 348. In case noEcho
is set to true, we expect the data
prop to be marked as a secret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok!
|
||
expectedError := fmt.Errorf("failed to invoke lambda") | ||
stackID := "stack-id" | ||
serviceToken := "arn:aws:lambda:us-west-2:123456789012:function:my-function" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the serviceToken example, I see. Indeed it looks like a lambda ARN. Interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
cdfdb9b
to
914125c
Compare
This PR adds support for CloudFormation Custom Resource to the aws-native provider. It implements an emulator that enables Pulumi programs to interact with Lambda-backed CloudFormation Custom Resources.
A CloudFormation custom resource is essentially an extension point to run arbitrary code as part of the CloudFormation lifecycle. It is similar in concept to the Pulumi Command Provider, the difference being that CloudFormation CustomResources are executed in the Cloud; either through Lambda or SNS.
For the first implementation we decided to limit the scope to Lambda backed Custom Resources, because the SNS variants are not widely used.
Custom Resource Protocol
The implementation follows the CloudFormation Custom Resource protocol. I derived the necessary parts by combining information from the docs, CDKs CustomResource Framework and trial&error.
Notable aspects of that protocol are:
ResponseURL
that needs to be included in the request payload.Custom Resource Lifecycle
Reviewer Notes
Key areas to review:
Update
lifecycleExposing this resource and schematizing it is part of this PR #1807.
Automatically cleaning up the response objects is not included in this PR in order to keep its size manageable. Implementing this is tracked here: #1813.
Please pay special attention to:
Testing
Related Issues