-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Allow lambdas to take additional permissions as part of their properties! #16
Conversation
In the case of custom resources (as they stand right now), we need to be able to add permissions to the lambda when creating it. This also fixes a minor partition bug - where we had hard coded `arn:aws:whatever` (we should open an issue about this gloablly, it could also apply to service principals that vary per partition )
* Additional permissions to add to the created Lambda Role. | ||
* You can also call addToRolePolicy to the created lambda. | ||
*/ | ||
additionalPermissions?: PolicyStatement[]; |
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.
Let's just call this policy
(we are refraining from using the term "permission" because it has a "grant" connotation).
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.
Why don't we just add a addToRolePolicy()
function that forwards to the Lambda?
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.
Not sure I follow... this is essentially a decrlarative way to define the initial role policy for the lambda.
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 don't like the inconsistency introduced here. If we do it here, we have to do it for all resources that have associated Roles (CodeBuild, CodePipeline, Fleet, etc.). Why not simply make the user call addToRolePolicy()
with their additional statements?
I think it makes more sense as an property of LambdaBackedCustomResourceProvider
, which can then call addToRolePolicy()
for each statement that needs to be added at Lambda instantiation time.
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 agree about the inconsistency, but I also think it's useful to be able to prime the policy document with initial statements declaratively, so let's just add this to our guidelines and add a policy?: PolicyStatement[]
property to any role-based resource.
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.
initialRolePolicy
?
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.
initialRolePolicies
feels like the right words in my brain
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.
But "policy" is the whole thing which is a collection of "statements", and you're passing statements here.
So I would say, either "policy" (singular) or "statements", but I think I prefer the first one.
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 vote for policy
. It conveys exactly what it is. The type information adds "it's an array of policy statements".
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.
there's only 8 hard problems in computer science, and all of them are naming things
I'm going to make it policy
and post a new rev of this just to get myself unblocked. Feel free to debate. (intialPolicyStatments
?)
@@ -113,8 +119,11 @@ export class Lambda extends LambdaRef { | |||
|
|||
this.role = new Role(this, 'ServiceRole', { | |||
assumedBy: new ServicePrincipal('lambda.amazonaws.com'), | |||
managedPolicyArns: [ 'arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole' ], | |||
managedPolicyArns: [ new FnConcat('arn:', new AwsPartition(), ':iam::aws:policy/service-role/AWSLambdaBasicExecutionRole')], |
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.
Maybe we should just use Arn.fromComponents
here?
}); | ||
if (props.additionalPermissions && props.additionalPermissions.length > 0) { | ||
props.additionalPermissions.forEach(permission => this.role!.addToPolicy(permission)); |
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 that if you use a normal for-loop, you won't need to use !
since the compiler can deduce that role is assigned.
This is how I would write this:
for (const p of props.additionalPermissions || []) {
this.role.addToPolicy(p);
}
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.
Nice!
It's the java in me 😭
The reason I'm leaning towards "initialPolicy" is because this will not be the recommended way to set up policies (addToXxx() and friends will be), and just "policy" sounds so final and definitive to my ears.
… On 4 Jun 2018, at 20:20, Breland Miley ***@***.***> wrote:
@mindstorms6 commented on this pull request.
In packages/aws-cdk-lambda/lib/lambda.ts:
> @@ -70,6 +70,12 @@ export interface LambdaProps {
* @default The default value is 128 MB
*/
memorySize?: number;
+
+ /**
+ * Additional permissions to add to the created Lambda Role.
+ * You can also call addToRolePolicy to the created lambda.
+ */
+ additionalPermissions?: PolicyStatement[];
there's only 8 hard problems in computer science, and all of them are naming things
I'm going to make it policy and post a new rev of this just to get myself unblocked. Feel free to debate. (intialPolicyStatments?)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Updated based on comments here - nothing too surprising |
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.
Please rename to initialPolicy
and merge.
* | ||
* You can call `addToRolePolicy` to the created lambda to add statements post creation. | ||
*/ | ||
initialPolicyStatements?: PolicyStatement[]; |
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.
Sorry but we decided eventually that this should be initialPolicy
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 need this change as well so if that's all, I'll change that final thing and push it myself :)
managedPolicyArns: [ 'arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole' ], | ||
// the arn is in the form of - arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole | ||
managedPolicyArns: [ Arn.fromComponents({ | ||
service: "iam", |
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: use single quotes instead of double quotes
In the case of custom resources (as they stand right now), we need to be able to add permissions to the lambda when creating it.
This also fixes a minor partition bug - where we had hard coded
arn:aws:whatever
(we should open an issue about this globally, it could also apply to service principals that vary per partition )By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.