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.

Sign up for GitHub

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

Jump to bottom

Setup APIGW CloudWatch role via custom resource #6531

Merged
merged 14 commits into from Aug 14, 2019
Merged

Setup APIGW CloudWatch role via custom resource #6531

merged 14 commits into from Aug 14, 2019

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Aug 13, 2019 •
edited

APIGW CloudWatch role needs to be assigned. so logs for APIGW can succesfully be wrirtten.

Still APIGW Cloudwatch role is global per account & region combination, which makes it error-prone if handled via CloudFormation stacks. Situation is as follows:

  • For every stack new IAM role is created (not breaking unless we're within AWS limits)
  • On creation of every stack, eventual previous APIGW CloudWatch role assignement is overriden with new one (not breaking)
  • On removal of stack, the role is removed, which makes CloudWatch role assignment broken for remaining stacks (breaking).

Proposed solution derives Cloudwatch role assignment to custom resource, which:

  • Creates and assigns new role, if there's no CloudWatch role assigned yet for given region
  • Ensures that dedicated role, is accompanied with policy that allows to write CloudWatch logs

Tested by deploying new services, and by deploying services deployed previously with older version of SLS.

Updated integration tests, so it confirms that logs are written

Is it a breaking change?: NO

@medikoo medikoo added the bug label Aug 13, 2019
@medikoo medikoo added this to the 1.50.0 milestone Aug 13, 2019
@medikoo medikoo self-assigned this Aug 13, 2019
@medikoo medikoo force-pushed the apigw-logs-setup-fix branch 4 times, most recently from 3cde6dc to b332850 Compare August 13, 2019 15:09
@medikoo medikoo force-pushed the apigw-logs-setup-fix branch 2 times, most recently from becd7c6 to ea8633e Compare August 13, 2019 15:27
alexdebrie
alexdebrie suggested changes Aug 13, 2019
View reviewed changes
Copy link
Contributor

@alexdebrie alexdebrie left a comment

Choose a reason for hiding this comment

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

Looks pretty good with a few small notes

lib/plugins/aws/customResources/index.js
} else {
return BbPromise.reject(`No implementation found for Custom Resource "${resourceName}"`);
}
if (FunctionName.length > 64) {
return BbPromise.reject(
new Error(`Resolved custom resource function name '${FunctionName}' is too long`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this happen if the service + stage name combo is too long? Is there anything we could do to help the user, or are they stuck?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this happen if the service + stage name combo is too long?

I believe so.

This change is purely about exposing that error earlier than it happens now (during CF deployment).

I plan to refactor a custom resources handling, and with that I'll ensure we're more reliable here, but I don't want to focus on that in context of that PR

lib/plugins/aws/customResources/resources/apiGatewayCloudWatchRole/handler.js Show resolved Hide resolved
lib/plugins/aws/lib/naming.js Outdated
@@ -504,4 +498,18 @@ module.exports = {
getCustomResourceEventBridgeResourceLogicalId(functionName, idx) {
return `${this.getNormalizedFunctionName(functionName)}CustomEventBridge${idx}`;
},
// API Gateway Account Logs Write Role
getCustomResourceApiGatewayAccountCloudWatchRoleHandlerFunctionName() {
return 'custom-resource-apigw-account-cw-role';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this shorter to give more room for service + stage name?

Do you know what our current effective limit is for service + stage combo, given other resources we create? We should try to not shorter than any further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stripped the account to make it safer

lib/plugins/aws/lib/naming.test.js
@@ -468,18 +468,6 @@ describe('#naming()', () => {
});
});

describe('#getApiGatewayLogsRoleLogicalId()', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want tests for the new naming functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

alexdebrie
alexdebrie previously approved these changes Aug 13, 2019
View reviewed changes
Copy link
Contributor

@alexdebrie alexdebrie left a comment

Choose a reason for hiding this comment

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

Tested and it worked for me!

@medikoo medikoo merged commit 6a84748 into master Aug 14, 2019
@medikoo medikoo deleted the apigw-logs-setup-fix branch August 14, 2019 08:05
@medikoo medikoo mentioned this pull request Aug 14, 2019
Closed
8 tasks
@medikoo medikoo mentioned this pull request Aug 26, 2019
Merged
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
allow specific ApiGateway logs role
2da4848 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now possible to use a pre-existing role in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::619871182895:role/services/slacknowledge
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
713530a 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now possible to use a pre-existing role in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::619871182895:role/services/slacknowledge
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
3abc4f0 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now simpler to allow hardcoding a role ARN in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::619871182895:role/services/slacknowledge
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
e0e5c0c 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now simpler to allow hardcoding a role ARN in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::123456789:role/a-service-with-all-necessary-permissions
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
8e83f63 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now simpler to allow hardcoding a role ARN in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::123456789:role/a-service-with-all-necessary-permissions
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
237cfbd 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now simpler to allow hardcoding a role ARN in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::123456789:role/a-service-with-all-necessary-permissions
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
6b40e70 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now simpler to allow hardcoding a role ARN in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::123456789:role/a-service-with-all-necessary-permissions
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 25, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
54074ab 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now simpler to allow hardcoding a role ARN in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::123456789:role/a-service-with-all-necessary-permissions
@JackDanger JackDanger mentioned this pull request Sep 25, 2019
Merged
8 tasks
JackDanger added a commit to JackDanger/serverless that referenced this pull request Sep 26, 2019
@JackDanger
Allow specifying ApiGateway logs role ARN
4ad1ded 
Since serverless#6531 improved how we manage API Gateway's CloudWatch log IAM role
it's now simpler to allow hardcoding a role ARN in the YAML config.

This addresses [this
comment](serverless#6591 (comment))
and unblocks anyone who needs to keep the IAM permissions for an app
scoped to just that app.

Example usage:

    provider:
      name: aws
      logs:
        restApi:
          role:
            arn:aws:iam::123456789:role/a-service-with-all-necessary-permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexdebrie alexdebrie alexdebrie left review comments

Assignees

@medikoo medikoo

Labels
bug
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@medikoo @alexdebrie