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

Custom Resources + Additional Permissions #9

Closed
mindstorms6 opened this issue May 31, 2018 · 9 comments
Closed

Custom Resources + Additional Permissions #9

mindstorms6 opened this issue May 31, 2018 · 9 comments

Comments

@mindstorms6
Copy link
Contributor

mindstorms6 commented May 31, 2018

Howdy!

Sometimes, when you're making a custom resource - you want it to do amazing things.

For example, say I want a custom resource that can do DNS Cross Account Delegation. Thus, my lambda role needs to be able to sts:AssumeRole into whatever target account, as well as route53:Get* in my account. This is of course super frequent (custom resources doing things that require additional AWS Permissions)

We currently have a custom resource provider that's amazing and beautiful - but it doesn't offer the ability to add additional policies to the created lambda, which makes me sad.

This issue is about fixing that, and getting feedback on how to do so.

Options (that I know of):

  1. In the LambdaBackedCustomResourceProps, add a thing like additionalPermissions, and add them to the Lambda during ensureLambda. (or extend the LambdaProps class of LambdaBackedCustomResourceProps to be something like CustomResourceLambdaProps and add a new field additionalPermissions). Functionally, these are the same, it's just where that property lives.

This isn't super elegant - the Lambda is a singleton, but the custom resource can be added over and over, backed by the same lambda. (This is generally how it's meant to be used). It's unlikely each one would have different perms (or lambda props), but could be confusing to customers. Right now the uuid is what determines if a new Lambda is created. (The confusing bit being - I created 2 custom resources with different props, but the same UUID, but that means the first one will be the one the props are taken for)

  1. Refactor this class to just take in a Lambda (optionally of course) that we would just always use if present.

This option lets the customer go crazy on customizing the Lambda - and we just use it without too much hassle.

Whatcha think?

@rix0rrr
Copy link
Contributor

rix0rrr commented May 31, 2018

See, this is why people who don't write applications shouldn't write frameworks. Never would have occurred to me that the Lambdas would need additional permissions.

Can we turn this around in some way? Can we just instantiate the CustomResource multiple times, add permissions to it. Probably the permissions assignment happens internally to the CustomResource class, based on props, so that

new CrossAccountDNSThingy(this, 'DNSThingy', {
    targetAccount: '123456789012'
});

Would internally add the appropriate permissions to the Lambda?

And then if multiple CrossAccountDNSThingies get instantiated, we can maybe check in some way if the Lambdas are ACTUALLY the same and if so deduplicate them, otherwise just add a different lambda for every instantiation of the custom resource?

If that's too hard, we can also optimize for the assumption that custom resources are usually one-offs, and always simply do "one lambda for one custom resource instantiation", and bet on the fact that nobody's ever going to use 1 lambda to create 100 CrossAccountDNSThingies.

Solution 2 as presented might be in the goldilocks zone after all. I'd prefer the lambdas to be deduped transparently, but that might turn out to be hard.

@eladb
Copy link
Contributor

eladb commented May 31, 2018

To be honest, I don't think the current API is solid. It's confusing (even before introducing the permissions). What happens if I create another instance of the same resource with different LambdaProps.

I think CustomResource should just accept a LambdaRef in it's props and the user will have to define it somewhere and pass it. In most cases, there will be a wrapping construct for the CustomResource, and users can use an idiom to define the Lambda as a singleton:

const lambda = Stack.find(this).tryFindChild(uuid) as Lambda;
if (!lambda) {
  lambda = new Lambda(this, 'Bla', { /* props */ });
}

@rix0rrr
Copy link
Contributor

rix0rrr commented May 31, 2018

But that's exactly the same, except you're now making every CR implementor do the same work.

@mindstorms6
Copy link
Contributor Author

What if we did some insane thing like

const customResourceFactory = new CustomResourceFactory(this,"blah",{_some set of lambda props, perms etc}
customResourceFactory.resource({targetAccount:1234})
customResourceFactory.resource({targetAccount:5678})

It's the call to .resource that actually adds the new custom resource to the stack - the first call is just to make the lambda attach to the parent

(I'm really bad at computers - so just ignore me if this is dumb)

This kind of falls in line with how you'd do it in CFN

MyLambda:
  Type: "AWS::Lambda::Function"
  Properties:
     ... stuff ...
MyDoItResource:
  Type: Custom::stuff
   Properties:
      ServiceToken: [ GetArn ]
      TargetAccount: 1235
MyDoItResource2:
  Type: Custom::stuff
   Properties:
      ServiceToken: [ GetArn ]
      TargetAccount: 1235

@mindstorms6
Copy link
Contributor Author

I might build a prototype of this - I need this for some fo the other constructs I'm building.

Thoughts welcome before I get too far in <3

mindstorms6 added a commit that referenced this issue Jun 3, 2018
This change makes it such that each custom resource is split into 2 parts

The "resource provider" and instances of the custom resource, that refer to a given provider.

This also for the Lambda backed provider adds the ability to add custom permissions :)
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 4, 2018

What I would prefer is that custom resources look exactly like regular resources though.

That is, I don't want the user to be able to tell where the resource comes from, either implemented "server side" by CFN or "client side" by Lambda.

@eladb
Copy link
Contributor

eladb commented Jun 4, 2018

👍 that's exactly what I wrote in #9

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 5, 2018

Something else that's not really cool... we now landed the change where we can add permissions to the Lambda role, but we still cannot do the reverse: get the role ARN so that we can add permissions on resources so they can be touched by the Lambda.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 5, 2018

I'm implementing the SingletonLambda solution.

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

No branches or pull requests

3 participants