getlift / lift

Expanding Serverless Framework beyond functions using the AWS CDK
MIT License
912 stars 111 forks source link

Allow existing SNS topic for DLQ SQS alarms #286

Closed chrishanline closed 1 year ago

chrishanline commented 1 year ago

References issue #277

We have existing alerting looking for Alarm and OK message posted to a specific SNS topic.

This change allows you to specify an existing SNS topic ARN for alarm on the DLQ SQS queue alarm construct. When an ARN that begins with arn:aws:sns: is present, that topic is used as an AlarmAction and OKAction instead of creating a new topic and email subscription.

No changes are made if an email address is specified for alarm.

Additionally adds checking for a bare-minimum of an ARN prefix or @ in alarm and throwing an exception if neither is present.

chrishanline commented 1 year ago

@mnapoli or @fredericbarthelet any feedback or thoughts on merging this in?

fredericbarthelet commented 1 year ago

Hi @chrishanline, thanks for proposing a PR for this feature !

We try to provide as simple configurations as possible in Lift, to avoid multiplication of configuration options, such as what you can find in the Serverless Framework current state.

With this in mind, and aware that everyone will have at some point specific requirements for its stack, we developed 2 features :

Lift extensions

With extensions, you can manually modify generated Cloudformation to match your use case. In this particular instance, the following code snippet will produce the expected behavior

constructs:
    my-queue:
        # ...
        alarm: dummy@dummy.com
        extensions:
            alarm:
                Properties:
                    AlarmActions: ["arn:aws:sns:us-east-1:123456789012:alarm-alerts"]

You can find out more about available extensions for the queue construct here.

The downside is you'll still have a SNS topic and subscription being provisioned by Lift without actually being used.

Eject

You can eject from Lift at anytime to regain control over the behavior of a single construct when your use case diverges too much from Lift original intent. You can find out more about ejecting in the corresponding documentation here

chrishanline commented 1 year ago

hi @fredericbarthelet, thanks for the feedback.

when trying the extension, I was unsuccessful. I think I am running into this issue: https://github.com/getlift/lift/issues/265

when I do an eject and look at the cloudformation generated, the AlarmActions ends up on the Queue properties, not the alarm.

serverless.yml

constructs:
  print:
    type: queue
    fifo: true
    maxRetries: 2
    alarm: dummy@alarm.com
    extensions:
      alarm:
        Properties:
          AlarmActions: ["arn:aws:sns:us-east-1:123456789012:alarm-alerts"]

cloudformation from eject

  printDlq09F2B389:
    Type: 'AWS::SQS::Queue'
    Properties:
      FifoQueue: true
      MessageRetentionPeriod: 1209600
      QueueName: paperwork-printing-dev-print-dlq.fifo
      AlarmActions:
        - 'arn:aws:sns:us-east-1:123456789012:alarm-alerts'
    UpdateReplacePolicy: Delete
    DeletionPolicy: Delete
fredericbarthelet commented 1 year ago

Indeed, would you be up to do a PR for #265 to solve this issue :) ? It should be a matter of replacing the duplicate typo by a reference to the provisioned alarm

fredericbarthelet commented 1 year ago

Hey @chrishanline, #265 is now fixed. I included an exemple of what you're trying to achieve in the form of a unit test. https://github.com/getlift/lift/blob/5ff580efe8f126eb8a6fa57082b8229f2467c0cb/test/unit/queues.test.ts#L624-L646 This should allow you to specify your own SNS topic as destination for the alarm.

I'm closing this PR in the meantime, please feel free to continue the conversation on #277 if you're still needing additional configuration options for this feature.

Thanks again for your time improving Lift :)