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

Fix ResourceLimitExceeded for cloudwatchLog event #5554

Merged
merged 2 commits into from Jan 7, 2019
File filter...
Filter file types
Jump to file or symbol
Failed to load files and symbols.
+273 −12
Diff settings

Always

Just for now

@@ -40,16 +40,6 @@ functions:
filter: '{$.userIdentity.type = Root}' filter: '{$.userIdentity.type = Root}'
``` ```


## Current gotchas

There's currently one gotcha you might face if you use this event definition.

The deployment will fail with an error that a resource limit exceeded if you replace the `logGroup` name of one function with the `logGroup` name of another function in your `serverless.yml` file and run `serverless deploy` (see below for an in-depth example).

This is caused by the fact that CloudFormation tries to attach the new subscription filter before detaching the old one. CloudWatch Logs only support one subscription filter per log group as you can read in the documentation about [CloudWatch Logs Limits](http://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/cloudwatch_limits_cwl.html).

Please keep this gotcha in mind when using this event. We will fix it in an upcoming release.

### Example ### Example


Update your `serverless.yml` file as follows and run `serverless deploy`. Update your `serverless.yml` file as follows and run `serverless deploy`.
@@ -13,13 +13,21 @@ module.exports = {
this.serverless.service.provider.shouldNotDeploy = false; this.serverless.service.provider.shouldNotDeploy = false;


if (this.options.force) { if (this.options.force) {
return BbPromise.resolve(); return this.checkLogGroupSubscriptionFilterResourceLimitExceeded();
} }


return BbPromise.bind(this) return BbPromise.bind(this)
.then(this.getMostRecentObjects) .then(this.getMostRecentObjects)
.then(this.getObjectMetadata) .then(this.getObjectMetadata)
.then(this.checkIfDeploymentIsNecessary); .then(this.checkIfDeploymentIsNecessary)
.then(() => {
if (this.serverless.service.provider.shouldNotDeploy) {
return BbPromise.resolve();
}

// perform the subscription filter checking only if a deployment is required
return this.checkLogGroupSubscriptionFilterResourceLimitExceeded();
});
}, },


getMostRecentObjects() { getMostRecentObjects() {
@@ -112,4 +120,107 @@ module.exports = {


return BbPromise.resolve(); return BbPromise.resolve();
}, },

/**
* @description Cloudwatch imposes a hard limit of 1 subscription filter per log group.
* If we change a cloudwatchLog event entry to add a subscription filter to a log group
* that already had one before, it will throw an error because CloudFormation firstly
* tries to create and replace the new subscription filter (therefore hitting the limit)
* before deleting the old one. This precompile process aims to delete existent
* subscription filters of functions that a new filter was provided, by checking the
* current ARN with the new one that will be generated.
* See: https://git.io/fpKCM
*/
checkLogGroupSubscriptionFilterResourceLimitExceeded() {
const region = this.provider.getRegion();
const serviceName = this.serverless.service.getServiceName();
const stage = this.provider.getStage();
const cloudWatchLogsSdk = new this.provider.sdk.CloudWatchLogs({ region });

return this.provider.getAccountId()
.then(accountId =>
Promise.all(this.serverless.service.getAllFunctions().map((functionName) => {
const functionObj = this.serverless.service.getFunction(functionName);

if (!functionObj.events) {
return BbPromise.resolve();
}

const promises = functionObj.events.map((event) => {
if (!event.cloudwatchLog) {
return BbPromise.resolve();
}

let logGroupName;

/*
it isn't necessary to run sanity checks here as they already happened during the
compile step
*/
if (typeof event.cloudwatchLog === 'object') {
logGroupName = event.cloudwatchLog.logGroup.replace(/\r?\n/g, '');
} else {
logGroupName = event.cloudwatchLog.replace(/\r?\n/g, '');
}

/*
return a new promise that will check the resource limit exceeded and will fix it if
the option is enabled
*/
return this.fixLogGroupSubscriptionFilters({
cloudWatchLogsSdk,
accountId,
logGroupName,
functionName,
region,
serviceName,
stage,
});
});

return Promise.all(promises);
})));
},

fixLogGroupSubscriptionFilters(params) {
const cloudWatchLogsSdk = params.cloudWatchLogsSdk;
const accountId = params.accountId;
const logGroupName = params.logGroupName;
const functionName = params.functionName;
const region = params.region;
const serviceName = params.serviceName;
const stage = params.stage;

return cloudWatchLogsSdk.describeSubscriptionFilters({ logGroupName }).promise()
.then((response) => {
const subscriptionFilter = response.subscriptionFilters[0];

// log group doesn't have any subscription filters currently
if (!subscriptionFilter) {
return false;
}

const oldDestinationArn = subscriptionFilter.destinationArn;
const filterName = subscriptionFilter.filterName;
const newDestinationArn =
`arn:aws:lambda:${region}:${accountId}:function:${serviceName}-${stage}-${functionName}`;

// everything is fine, just return
if (oldDestinationArn === newDestinationArn) {
return false;
}

/*
If the destinations functions' ARNs doesn't match, we need to delete the current
subscription filter to prevent the resource limit exceeded error to happen
*/
return cloudWatchLogsSdk
.deleteSubscriptionFilter({ logGroupName, filterName }).promise();
})
/*
it will throw when trying to get subscription filters of a log group that was just added
to the serverless.yml (therefore not created in AWS yet), we can safely ignore this error
*/
.catch(() => undefined);

This comment has been minimized.

@dschep

dschep Dec 21, 2018

Member

Oh, nevermind, this addresses my 'one last thought', right?

This comment has been minimized.

@rdsedmundo

rdsedmundo Dec 21, 2018

Contributor

Yes, that's right.

},
}; };
@@ -7,6 +7,7 @@ const path = require('path');
const globby = require('globby'); const globby = require('globby');
const sinon = require('sinon'); const sinon = require('sinon');
const chai = require('chai'); const chai = require('chai');
const BbPromise = require('bluebird');
const proxyquire = require('proxyquire'); const proxyquire = require('proxyquire');
const normalizeFiles = require('../../lib/normalizeFiles'); const normalizeFiles = require('../../lib/normalizeFiles');
const AwsProvider = require('../../provider/awsProvider'); const AwsProvider = require('../../provider/awsProvider');
@@ -60,6 +61,7 @@ describe('checkForChanges', () => {
let getMostRecentObjectsStub; let getMostRecentObjectsStub;
let getObjectMetadataStub; let getObjectMetadataStub;
let checkIfDeploymentIsNecessaryStub; let checkIfDeploymentIsNecessaryStub;
let checkLogGroupSubscriptionFilterResourceLimitExceededStub;


beforeEach(() => { beforeEach(() => {
getMostRecentObjectsStub = sinon getMostRecentObjectsStub = sinon
@@ -68,19 +70,24 @@ describe('checkForChanges', () => {
.stub(awsDeploy, 'getObjectMetadata').resolves(); .stub(awsDeploy, 'getObjectMetadata').resolves();
checkIfDeploymentIsNecessaryStub = sinon checkIfDeploymentIsNecessaryStub = sinon
.stub(awsDeploy, 'checkIfDeploymentIsNecessary').resolves(); .stub(awsDeploy, 'checkIfDeploymentIsNecessary').resolves();
checkLogGroupSubscriptionFilterResourceLimitExceededStub = sinon
.stub(awsDeploy, 'checkLogGroupSubscriptionFilterResourceLimitExceeded').resolves();
}); });


afterEach(() => { afterEach(() => {
awsDeploy.getMostRecentObjects.restore(); awsDeploy.getMostRecentObjects.restore();
awsDeploy.getObjectMetadata.restore(); awsDeploy.getObjectMetadata.restore();
awsDeploy.checkIfDeploymentIsNecessary.restore(); awsDeploy.checkIfDeploymentIsNecessary.restore();
awsDeploy.checkLogGroupSubscriptionFilterResourceLimitExceeded.restore();
}); });


it('should run promise chain in order', () => expect(awsDeploy.checkForChanges()) it('should run promise chain in order', () => expect(awsDeploy.checkForChanges())
.to.be.fulfilled.then(() => { .to.be.fulfilled.then(() => {
expect(getMostRecentObjectsStub).to.have.been.calledOnce; expect(getMostRecentObjectsStub).to.have.been.calledOnce;
expect(getObjectMetadataStub).to.have.been.calledAfter(getMostRecentObjectsStub); expect(getObjectMetadataStub).to.have.been.calledAfter(getMostRecentObjectsStub);
expect(checkIfDeploymentIsNecessaryStub).to.have.been.calledAfter(getObjectMetadataStub); expect(checkIfDeploymentIsNecessaryStub).to.have.been.calledAfter(getObjectMetadataStub);
expect(checkLogGroupSubscriptionFilterResourceLimitExceededStub).to.have.been
.calledAfter(checkIfDeploymentIsNecessaryStub);


expect(awsDeploy.serverless.service.provider.shouldNotDeploy).to.equal(false); expect(awsDeploy.serverless.service.provider.shouldNotDeploy).to.equal(false);
}) })
@@ -476,4 +483,157 @@ describe('checkForChanges', () => {
}); });
}); });
}); });

describe('#checkLogGroupSubscriptionFilterResourceLimitExceeded', () => {
let CloudWatchLogsStub;
let deleteSubscriptionFilterStub;
const accountId = '123456789';
const serviceName = 'my-service';
const region = 'us-east-1';
let describeSubscriptionFiltersResponse = {};
let sandbox;

beforeEach(() => {
sandbox = sinon.sandbox.create();

CloudWatchLogsStub = class {
constructor() {
this.deleteSubscriptionFilter = deleteSubscriptionFilterStub = sinon.spy(() => ({
promise: () => BbPromise.resolve(),
}));

this.describeSubscriptionFilters = sinon.spy(() => ({
promise: () => BbPromise.resolve(describeSubscriptionFiltersResponse),
}));
}
};

provider.sdk.CloudWatchLogs = CloudWatchLogsStub;

sandbox.stub(provider, 'getAccountId')
.returns(BbPromise.resolve(accountId));

sandbox.stub(awsDeploy.serverless.service, 'getServiceName')
.returns(serviceName);

sandbox.stub(awsDeploy, 'getMostRecentObjects').resolves();
sandbox.stub(awsDeploy, 'getObjectMetadata').resolves();
sandbox.stub(awsDeploy, 'checkIfDeploymentIsNecessary').resolves();
});

afterEach(() => {
sandbox.restore();
});

it('should not call checkLogGroup if deployment is not required', () => {
awsDeploy.checkIfDeploymentIsNecessary.restore();

sandbox.stub(awsDeploy, 'checkIfDeploymentIsNecessary', () => new Promise((resolve) => {
awsDeploy.serverless.service.provider.shouldNotDeploy = true;
resolve();
}));

const spy = sinon.spy(awsDeploy, 'checkLogGroupSubscriptionFilterResourceLimitExceeded');

return awsDeploy.checkForChanges().then(() => {
expect(spy).to.not.have.been.called;
});
});

it('should work normally when there are functions without events', () => {
awsDeploy.serverless.service.functions = {
first: {},
};

expect(awsDeploy.checkForChanges()).to.be.fulfilled;
});

it('should work normally when there are functions events that are not cloudWwatchLog', () => {
awsDeploy.serverless.service.functions = {
first: {
events: [
{ dummyEvent: 'test' },
],
},
};

expect(awsDeploy.checkForChanges()).to.be.fulfilled;
});

describe('option to force update is set', () => {
beforeEach(() => {
awsDeploy.serverless.service.provider.cloudWatchLogs = {};
});

afterEach(() => {
awsDeploy.serverless.service.provider.cloudWatchLogs = undefined;
});

it('should not call delete if there are no subscriptionFilters', () => {
awsDeploy.serverless.service.functions = {
first: {
events: [
{ cloudwatchLog: '/aws/lambda/hello1' },
],
},
};

describeSubscriptionFiltersResponse = {
subscriptionFilters: [],
};

return awsDeploy.checkForChanges().then(() => {
expect(deleteSubscriptionFilterStub).to.not.have.been.called;
});
});

it('should not call delete if there is a subFilter and the ARNs are the same', () => {
awsDeploy.serverless.service.functions = {
first: {
events: [
{ cloudwatchLog: '/aws/lambda/hello1' },
],
},
};

describeSubscriptionFiltersResponse = {
subscriptionFilters: [
{
destinationArn:
`arn:aws:lambda:${region}:${accountId}:function:${serviceName}-dev-first`,
filterName: 'dummy-filter',
},
],
};

return awsDeploy.checkForChanges().then(() => {
expect(deleteSubscriptionFilterStub).to.not.have.been.called;
});
});

it('should call delete if there is a subFilter but the ARNs are not the same', () => {
awsDeploy.serverless.service.functions = {
first: {
events: [
{ cloudwatchLog: '/aws/lambda/hello1' },
],
},
};

describeSubscriptionFiltersResponse = {
subscriptionFilters: [
{
destinationArn:
`arn:aws:lambda:${region}:${accountId}:function:${serviceName}-dev-not-first`,
filterName: 'dummy-filter',
},
],
};

return awsDeploy.checkForChanges().then(() => {
expect(deleteSubscriptionFilterStub).to.have.been.called;
});
});
});
});
}); });
ProTip! Use n and p to navigate between commits in a pull request.