Skip to content

Lambda/Role changes#113

Merged
drboyer merged 8 commits intomasterfrom
role-changes
Oct 22, 2020
Merged

Lambda/Role changes#113
drboyer merged 8 commits intomasterfrom
role-changes

Conversation

@drboyer
Copy link
Copy Markdown
Contributor

@drboyer drboyer commented Oct 12, 2020

What's this PR?

What's left?

I left a question in #103 about some advanced functionality @rclark originally suggested. We should probably come to a conclusion on that before merging this.

About this review

  • Are my new/updated comments logical?
  • Sorry for all the fixture updates - this change reordered templates which meant lots of fixture updates (and meant I made use of the UPDATE environment variable)

@drboyer drboyer requested a review from a team October 12, 2020 20:40
Copy link
Copy Markdown
Contributor

@rclark rclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that when RoleARN is set

  • Statement should be disallowed to prevent confusion
  • We should add an AWS::IAM::Policy to the specified RoleARN to be absolutely sure that it has permission to access the log group.

Comment thread lib/shortcuts/api.md Outdated
@drboyer
Copy link
Copy Markdown
Contributor Author

drboyer commented Oct 21, 2020

I've updated this PR to attach a new LogPolicy to all Lambdas, per discussion. Based on that I had to re-jigger some configuration in the Stream and Queue lambdas, but hopefully the change makes sense.

I'll give this a try in a PR where I'd like to provide my own role, but in the meantime, putting this back up for review.

@drboyer drboyer requested a review from rclark October 21, 2020 16:34
Comment thread lib/shortcuts/lambda.js Outdated
Condition,
Properties: {
PolicyName: 'lambda-log-access',
Roles: (RoleArn) ? RoleArn : { 'Ref': `${LogicalName}Role` },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nervous here because it looks like this might mismatch role names (Ref value for a role) with role arns (per the variable name).

Do you know whether this CloudFormation property accepts name or arn or either?

Copy link
Copy Markdown
Contributor Author

@drboyer drboyer Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that makes me a little nervous too - I need to test it. The docs note that the name of the role should be used, but I know sometimes you can mix and match them alright. Will report back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it turns out you do need to provide just the role name with these policies - you'll get an error if you provide the full ARN. But the Lambda Function resource requires the full ARN 😭

As a result, I added some logic to handle this. It's now working in my test repository.

Copy link
Copy Markdown
Contributor

@rclark rclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sorting this out!

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.

Role shortcuts should set the policy version in the generated policy Lambda shortcuts should allow you to provide a complete IAM role

2 participants