Skip to content
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

Merged
merged 31 commits into from
May 29, 2019
Merged

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Feb 25, 2019

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:

new AwsSdkJsCustomResource(this, 'AwsSdk', {
  onCreate: {  // AWS SDK call when resource is created (defaults to onUpdate)
    service: '...',
    action: '...',
    parameters: { ... }
  }.
  onUpdate: { ... }. // AWS SDK call when resource is updated (defaults to onCreate)
  onDelete: { ... }, // AWS SDK call when resource is deleted
  policyStatements: [...] // Automatically derived from the calls if not specified
});

Fargate scheduled task example (could be used in @aws-cdk/aws-ecs to implement the missing FargateEventRuleTarget):

const vpc = ...;
const cluster = new ecs.Cluster(...);

const taskDefinition = new ecs.FargateTaskDefinition(...);

const rule = new events.EventRule(this, 'Rule', {
  scheduleExpression: 'rate(1 hour)',
});

const ruleRole = new iam.Role(...);

new AwsSdkJsCustomResource(this, 'PutTargets', {
  onCreate: {
    service: 'CloudWatchEvents',
    action: 'putTargets',
    parameters: {
      Rule: rule.ruleName,
      Targets: [
        Arn: cluster.clusterArn,
        Id: ...,
        EcsParameters: {
          taskDefinitionArn: taskDefinition.taskDefinitionArn,
          LaunchType: 'FARGATE',
          NetworkConfiguration: {
            awsvpcConfiguration: {
              AssignPublicIp: 'DISABLED',
              SecurityGroups: [...],
              Subnets: vpc.privateSubnets.map(subnet => subnet.subnetId),
            },
          },
          RoleArn: ruleRole.roleArn
        }
      ]
    }
  }
})

Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@jogold jogold requested a review from a team as a code owner February 25, 2019 09:23
Copy link
Contributor

@eladb eladb left a 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');
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@eladb eladb left a 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).

@dnakov
Copy link
Contributor

dnakov commented Mar 24, 2019

I think this looks great!
Just one suggestion, it may be useful to accept an array in the on* methods. Sometimes, an update needs to Delete + Create, so would be great to be able to specify both

@jogold
Copy link
Contributor Author

jogold commented Mar 24, 2019

Just one suggestion, it may be useful to accept an array in the on* methods. Sometimes, an update needs to Delete + Create, so would be great to be able to specify both

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 AwsCustomResource and using CloudFormation Ref between them.

* 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 ('*')
Copy link
Contributor

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

@jogold
Copy link
Contributor Author

jogold commented Mar 28, 2019

Updated to support physical resource id based on a path in the API response.

@jogold jogold changed the title feat(cloudformation): aws sdk js custom resource feat(cloudformation): aws custom resource Mar 28, 2019
@jogold
Copy link
Contributor Author

jogold commented Mar 29, 2019

This custom resource could be used to support https://docs.aws.amazon.com/AmazonECS/latest/developerguide/specifying-sensitive-data.html (gap with CF)

@jogold jogold requested a review from RomainMuller as a code owner April 2, 2019 08:15
@eladb eladb merged commit 9a48b66 into aws:master May 29, 2019
@BDQ
Copy link
Contributor

BDQ commented May 30, 2019

@jogold congrats on getting this merged, it was a beast! Delighted to see it the next release!

@eladb
Copy link
Contributor

eladb commented May 30, 2019

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.

@jogold
Copy link
Contributor Author

jogold commented May 30, 2019

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.

@jogold jogold deleted the aws-sdk-js-resource branch June 13, 2019 13:21
This was referenced Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants