Merge pull request #6471 from sameer-s/master

Allow AWS Subscription Filters to be reordered

lib/plugins/aws/deploy/lib/checkForChanges.js

@@ -160,6 +160,7 @@ module.exports = {
             return BbPromise.resolve();
           }
 
+          let logSubscriptionSerialNumber = 0;
           const promises = functionObj.events.map(event => {
             if (!event.cloudwatchLog) {
               return BbPromise.resolve();
@@ -180,6 +181,8 @@ module.exports = {
             const accountId = account.accountId;
             const partition = account.partition;
 
+            logSubscriptionSerialNumber++;
+
             /*
               return a new promise that will check the resource limit exceeded and will fix it if
               the option is enabled
@@ -188,9 +191,11 @@ module.exports = {
               cloudWatchLogsSdk,
               accountId,
               logGroupName,
+              functionName,
               functionObj,
               region,
               partition,
+              logSubscriptionSerialNumber,
             });
           });
 
@@ -204,9 +209,11 @@ module.exports = {
     const cloudWatchLogsSdk = params.cloudWatchLogsSdk;
     const accountId = params.accountId;
     const logGroupName = params.logGroupName;
+    const functionName = params.functionName;
     const functionObj = params.functionObj;
     const region = params.region;
     const partition = params.partition;
+    const logSubscriptionSerialNumber = params.logSubscriptionSerialNumber;
 
     return (
       cloudWatchLogsSdk
@@ -220,19 +227,26 @@ module.exports = {
             return false;
           }
 
-          const oldDestinationArn = subscriptionFilter.destinationArn;
           const filterName = subscriptionFilter.filterName;
+
+          const oldDestinationArn = subscriptionFilter.destinationArn;
           const newDestinationArn = `arn:${partition}:lambda:${region}:${accountId}:function:${functionObj.name}`;
 
+          const oldLogicalId = this.getLogicalIdFromFilterName(filterName);
+          const newLogicalId = this.provider.naming.getCloudWatchLogLogicalId(
+            functionName,
+            logSubscriptionSerialNumber
+          );
+
           // everything is fine, just return
-          if (oldDestinationArn === newDestinationArn) {
+          if (oldDestinationArn === newDestinationArn && oldLogicalId === newLogicalId) {
             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
-        */
+            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();
         })
         /*
@@ -242,4 +256,12 @@ module.exports = {
         .catch(() => undefined)
     );
   },
+
+  getLogicalIdFromFilterName(filterName) {
+    // Filter name format:
+    // {stack name}-{logical id}-{random alphanumeric characters}
+    // Note that the stack name can include hyphens
+    const split = filterName.split('-');
+    return split[split.length - 2];
+  },
 };

lib/plugins/aws/deploy/lib/checkForChanges.test.js

@@ -593,7 +593,7 @@ describe('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', () => {
+      it('should not call delete if there is a subFilter and the ARNs/logical IDs are the same', () => {
         awsDeploy.serverless.service.functions = {
           first: {
             events: [{ cloudwatchLog: '/aws/lambda/hello1' }],
@@ -606,7 +606,7 @@ describe('checkForChanges', () => {
           subscriptionFilters: [
             {
               destinationArn: `arn:aws:lambda:${region}:${accountId}:function:${serviceName}-dev-first`,
-              filterName: 'dummy-filter',
+              filterName: 'stack-name-FirstLogsSubscriptionFilterCloudWatchLog1-1KAK9SAG7Y9YN',
             },
           ],
         };
@@ -629,7 +629,7 @@ describe('checkForChanges', () => {
           subscriptionFilters: [
             {
               destinationArn: `arn:aws:lambda:${region}:${accountId}:function:${serviceName}-dev-not-first`,
-              filterName: 'dummy-filter',
+              filterName: 'stack-name-FirstLogsSubscriptionFilterCloudWatchLog1-1KAK9SAG7Y9YN',
             },
           ],
         };
@@ -639,7 +639,30 @@ describe('checkForChanges', () => {
           .then(() => expect(deleteSubscriptionFilterStub).to.have.been.called);
       });
 
-      it('should not call delete if there is a subFilter and the ARNs are the same with custom function name', () => {
+      it('should call delete if there is a subFilter but the logical IDs are not the same', () => {
+        awsDeploy.serverless.service.functions = {
+          first: {
+            events: [{ cloudwatchLog: '/aws/lambda/hello1' }],
+          },
+        };
+
+        awsDeploy.serverless.service.setFunctionNames();
+
+        describeSubscriptionFiltersResponse = {
+          subscriptionFilters: [
+            {
+              destinationArn: `arn:aws:lambda:${region}:${accountId}:function:${serviceName}-dev-first`,
+              filterName: 'stack-name-FirstLogsSubscriptionFilterCloudWatchLog2-1KAK9SAG7Y9YN',
+            },
+          ],
+        };
+
+        return awsDeploy
+          .checkForChanges()
+          .then(() => expect(deleteSubscriptionFilterStub).to.have.been.called);
+      });
+
+      it('should not call delete if there is a subFilter and the ARNs/logical IDs are the same with custom function name', () => {
         awsDeploy.serverless.service.functions = {
           first: {
             name: 'my-test-function',
@@ -653,7 +676,7 @@ describe('checkForChanges', () => {
           subscriptionFilters: [
             {
               destinationArn: `arn:aws:lambda:${region}:${accountId}:function:my-test-function`,
-              filterName: 'dummy-filter',
+              filterName: 'stack-name-FirstLogsSubscriptionFilterCloudWatchLog1-1KAK9SAG7Y9YN',
             },
           ],
         };
@@ -663,7 +686,7 @@ describe('checkForChanges', () => {
           .then(() => expect(deleteSubscriptionFilterStub).to.not.have.been.called);
       });
 
-      it('should not call delete when ARN is the same accounting for non-standard partitions', () => {
+      it('should not call delete when ARN/logical IDs are the same accounting for non-standard partitions', () => {
         provider.getAccountInfo.restore();
         sandbox.stub(provider, 'getAccountInfo').returns(
           BbPromise.resolve({
@@ -684,7 +707,7 @@ describe('checkForChanges', () => {
           subscriptionFilters: [
             {
               destinationArn: `arn:aws-us-gov:lambda:${region}:${accountId}:function:my-test-function`,
-              filterName: 'dummy-filter',
+              filterName: 'stack-name-FirstLogsSubscriptionFilterCloudWatchLog1-1KAK9SAG7Y9YN',
             },
           ],
         };
@@ -708,7 +731,7 @@ describe('checkForChanges', () => {
           subscriptionFilters: [
             {
               destinationArn: `arn:aws:lambda:${region}:${accountId}:function:my-other-test-function`,
-              filterName: 'dummy-filter',
+              filterName: 'stack-name-FirstLogsSubscriptionFilterCloudWatchLog1-1KAK9SAG7Y9YN',
             },
           ],
         };