-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(cloudformation): aws custom resource #1850
Conversation
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 like this idea and agree this should probably go into a separate module (maybe
@aws-cdk/custom-resource
could be the "home" for stuff like that and we can add other types of custom resources there. @rix0rrr what do you think?) - Missing unit/integration tests
await awsService[action](parameters).promise(); | ||
} | ||
|
||
await respond('SUCCESS', 'OK'); |
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.
The physical resource ID is actually a pretty important thing to get right when defining custom resources. I am wondering if we can do better than logStreamName
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.
Indeed, if we want to avoid calling the onDelete
during a UPDATE_COMPLETE_CLEANUP_IN_PROGRESS
when a parameter of the onUpdate
changes we need something like a constant here, no? can we use the LogicalResourceId
?
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 is a clever idea. To be useful, this resource is going to be an exercise in metaprogramming.
Logical ID wouldn't be good enough. I think a template string of sorts will need to come from the parameter. Also wonder if we might want to call multiple functions.
At least we'd want to explode the API result into a set of output attributes, 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.
+1, I think we will need to let users specify the path to where the physical ID can be found in the response JSON (at least), and this mechanism can also be used to produced a bunch of "GetAtt" attributes.
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.
In the response only or also in the request? If you look at the example with the putTargets
call what would you specify as physical ID?
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.
In the response only or also in the request? If you look at the example with the
putTargets
call what would you specify as physical ID?
The physical resource ID parameters needs to be able to be JSONPath member. It means it's not an ID by itself, but it's instructions for how to get the ID out of the API call response. So it should be able to be something like:
$.CreateThingResponse.ThingArn
And the custom resource should get the value out of the response object. Probably should be able to be different for both Create and Update calls.
Now, whether it can be EITHER a literal or a JSONPath I'm not sure on. I'm sure there are cases in which a literal would make sense, so I'm not against disallowing it, but the JSONPath-style query needs to be possible.
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.
P.S: When I say JSONPath, I'm cool with faking it by splitting on .
and leaving it there, I don't mean literally importing a jsonpath library, we don't have a good enough story around bundled lambda dependencies to make that work.
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 love to see this move forward. I think we should rename this to AwsCustomResource
(the fact that under the hood it uses the JS SDK is almost completely encapsulated (besides the names of the methods).
I think this looks great! |
How would you format the data returned by the custom resource in this case? There's always the option of chaining API calls by defining multiple |
* The IAM policy statements to allow the different calls. Use only if | ||
* resource restriction is needed. | ||
* | ||
* @default Allow onCreate, onUpdate and onDelete calls on all resources ('*') |
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 believe the default is to extract the permissions from the calls
Updated to support physical resource id based on a path in the API response. |
This custom resource could be used to support https://docs.aws.amazon.com/AmazonECS/latest/developerguide/specifying-sensitive-data.html (gap with CF) |
@jogold congrats on getting this merged, it was a beast! Delighted to see it the next release! |
Indeed. Great job @jogold One thing that I was thinking about is what happens if the default SDK on Lambda is too old and doesn't support some features. I heard there was a way to somehow monkey-patch the SDK to be able to issue requests regardless of whether they have generated shapes and clients in the SDK or not. Maybe we should use this mechanism (basically use the lowest layers of the SDK) so that users are truly never blocked. |
Interesting, I will have a look at this. In the meantime, we should at least switch to the Node.js 10.x runtime which offers a more recent version of the SDK. Will open a PR for that. |
This PR adds a CF custom resource to make calls on the AWS API using AWS SDK JS v2. There are lots of use cases when the CF coverage is not sufficient and adding a simple API call can solve the problem. It could be also used internally to create better L2 constructs.
Does this fit in the scope of the cdk?
If accepted, I think that ideally it should live in its own lerna package.
API:
Fargate scheduled task example (could be used in
@aws-cdk/aws-ecs
to implement the missingFargateEventRuleTarget
):Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.