Allow AWS Subscription Filters to be reordered

[sameer-s]

What did you implement:

Closes #5817
Closes #6263

How did you implement it:

As suggested in #6263, changed cloudwatch log subscription filters to name their logical IDs using their log group name rather than using an incrementing serial number. This prevents CloudFormation from getting confused (due to new subscription filters having the same name but different log groups than older ones) when a subscription filter is added to or removed from anywhere except the end of a template.

However, this brings up another problem: this would break any projects that already existed using the serial number naming scheme (I tested and confirmed that this problem does exist). Therefore, I also added an extra condition to the code that removes subscription filters if the old and new destination ARNs don't match: they must also have the same logical ID.

One last thing to note: while this second fix could be implemented on its own to solve the problem in #5817, there are some issues with taking that approach. First of all, continuing to implement subscription filters using serial numbers rather than filter names would end up causing many more deletions than are necessary. Additionally, this second fix is pretty much impossible (or at least very difficult) to implement without the first one, as there isn't really a way to calculate the "expected" logical ID from the fixLogGroupSubscriptionFilters method, since there's no good way to get which serial number will correspond to a log group.

How can we verify it:

Create/deploy a project with CloudWatch logs subscriptions, and then re-deploy it with the order of some of the subscriptions swapped. This will throw a Resource limit exceeded on the upstream but should work fine on the PR. For the sake of brevity and time, I'm not going to rewrite it here, but there's a good example listed in the initial bug report (#5817).

I've tested this locally, but you would also probably want to verify that a project deployed using the upstream version (with serial numbers) would redeploy fine on the newer version.

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

• Write tests and confirm existing functionality is not broken.
  Validate via npm test
• Write documentation
• Ensure there are no lint errors.
  Validate via npm run lint-updated
  Note: Some reported issues can be automatically fixed by running npm run lint:fix
• Ensure introduced changes match Prettier formatting.
  Validate via npm run prettier-check-updated
  Note: All reported issues can be automatically fixed by running npm run prettify-updated
• Make sure code coverage hasn't dropped
• Provide verification config / commands / resources
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

[pmuens]

Hey @sameer-s thanks for working on this 👍

I just looked through the code and added some comments. It looks like this change could result in a breaking change because resource logical ids could be too long. Perhaps we can check the length or use a trimmed and hashed version of the log group name we're using instead of the index to mitigate this problem.

Also I'm not sure if people overwrite this resource via the Resources section in their serverless.yml file.

[pmuens]

IMHO we should rename this variable since it's not a numerical index anymore.

[pmuens]

This could result in a breaking change because the logical id can be too long (that was the reason we've used integers in the past). I'm not sure what the limit for the length of the resource logical id is, but it might make sense to check that we're compliant with that.

[sameer-s]

How do you think the best way to approach this problem is, then? It seems like it would be a bad idea to just assume that no one would be overriding these resources - I could definitely imagine a situation where someone was doing that to maybe set a custom RoleArn.

Today I'll try to take a look to see if there's any way to get the sequential serial numbers from the fixLogGroupSubscriptionFilters since that would be an easy way to fix the re-ordering problem without having to change the naming convention for subscription filters.

[sameer-s]

Alright, this latest iteration reverts back to using the serial numbers, but continues checking if the logical ID matches what is expected. Turns out it was not nearly as difficult as I thought - I'd simply gotten confused because when I added a print statement, it appeared as if the loop through the cloudwatchLog events in checkForChanges wasn't being executed in the same order as they were in the file, but that was just because of the asynchronous nature of promises.

Anyway, this solution should resolve the issues of the original bug report without introducing some of the other potential ones you brought up.

[pmuens]

Thanks for updating this PR @sameer-s 👍

I just looked through the code and it seems like a good solution since it doesn't touch that many different parts. Tested it with an example service and everything worked as expected.

LGTM