feat(@aws-cdk/aws-events-triggers): introducing sqs target support in event rule

[made2591]

Extend the Target for Event Rule to SQS Queues.

This may be needed in situations where users want to create an Event Rule and have as a target an SQS Queue.

I extended the package @aws-cdk/aws-events-targets by introducing a new Typescript file https://github.com/made2591/aws-cdk/blob/feature/aws-events-targets-sqs/packages/%40aws-cdk/aws-events-targets/lib/sqs.ts and by following the same approach used for SNS Topic

I also wrote the respective tests in the test folder. The SqsParameters are still not available but it's a skeleton to synthesize a Simple Queue as a Target.

closes #1786

---

Pull Request Checklist

• Testing
  • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
  • CLI change?: coordinate update of integration tests with team
  • cdk-init template change?: coordinated update of integration tests with team
• Docs
  • jsdocs: All public APIs documented
  • README: README and/or documentation topic updated
  • Design: For significant features, design document added to design folder
• Title and Description
  • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
  • Title: use lower-case and doesn't end with a period
  • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
  • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
• Sensitive Modules (requires 2 PR approvers)
  • IAM Policy Document (in @aws-cdk/aws-iam)
  • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
  • Grant APIs (only if not based on official documentation with a reference)

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

[made2591]

@rix0rrr thank you for your feedback. I'm sorry I didn't provide to you all the info about the error: the error is thrown when the template is not compliant - the test fail - I don't know if it is the expected way to exit but I guess so

    81 |   public assertOrThrow(inspector: StackInspector) {
    82 |     if (!this.assertUsing(inspector)) {
  > 83 |       throw new Error(this.generateErrorMessage());
       |             ^
    84 |     }
    85 |   }
    86 |
    at HaveResourceAssertion.assertOrThrow (../assert/lib/assertions/have-resource.ts:83:13)
    at StackInspector._to (../assert/lib/inspector.ts:25:15)
    at StackInspector.to (../assert/lib/inspector.ts:15:25)
    at Object.<anonymous>.test (test/sqs/sqs.test.ts:78:17)

I fixed the code and the error is not there anymore: I was able to add the condition as you said, and the messageGroupId parameter as well. I remember I tried to put the condition inside service before without success - I think there was some other mistake. Now I have a design problem and I think is due to the introduction of condition...🤔 Attached the template, I'm having a look into

Resources:
  MyRuleA44AB831:
    Type: AWS::Events::Rule
    Properties:
      ScheduleExpression: rate(1 minute)
      State: ENABLED
      Targets:
        - Arn:
            Fn::GetAtt:
              - MyQueueE6CA6235
              - Arn
          Id: MyQueue
  MyQueueE6CA6235:
    Type: AWS::SQS::Queue
  MyQueuePolicy6BBEDDAC:
    Type: AWS::SQS::QueuePolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action:
              - sqs:SendMessage
              - sqs:SendMessageBatch
              - sqs:GetQueueAttributes
              - sqs:GetQueueUrl
            Condition:
              ArnEqual:
                aws:SourceArn:
                  Fn::GetAtt:
                    - MyRuleA44AB831
                    - Arn
            Effect: Allow
            Principal:
              Service:
                Fn::Join:
                  - ""
                  - - events.
                    - Ref: AWS::URLSuffix
            Resource:
              Fn::GetAtt:
                - MyQueueE6CA6235
                - Arn
        Version: "2012-10-17"
      Queues:
        - Ref: MyQueueE6CA6235

This is the output provided by Cloudformation:

Invalid value for the parameter Policy. (Service: AmazonSQS; Status Code: 400; Error Code: InvalidAttributeValue; Request ID: d05e3b85-7cb8-5a28-b8ae-6eee5581d891)

[made2591]

I brought the changes of 0.33.0 in my branch. Package.json changes in peerDependencies needed for sqs integration

[made2591]

@rix0rrr @eladb is there anything else I can do it? If so, please feel free to ask :) sorry for the mistakes, I'm still learning 😅

Changes addressed

[Visorgood]

Hello @made2591 , hello @rix0rrr !
First of all, thank you very much for making this feature true!
I just started testing it, and I got a weird result. I have an app with 2 stacks. One contains the SQS queue, the other one contains the SF. Stack with the state machine depends on the stack with the queue. I wanted to put the Rule into the stack with the state machine, so that a message would be sent to the queue on the completion of the state machine completeness. Unfortunately I'm getting this error:

Error: 'stack-with-sf' depends on 'stack-with-queue' (dependency added using stack.addDependency()). Adding this dependency (stack-with-queue/MyQueue/Policy/Resource -> stack-with-sf/MySFExecutionSucceededEvent/Resource.Arn) would create a cyclic reference.

Before that, we were doing myQueue.grantSendMessages(mySF.stateMachine.role), and sent a message to the queue manually from the SF - it worked well. Do you have any idea why I am getting this error?

This is the code I am using:

new Rule(this, 'MySFExecutionSucceededEvent', {
                ruleName: `succeeded-rule`,
                enabled: true,
                eventPattern: {
                    source: ["aws.states"],
                    detailType: ["Step Functions Execution Status Change"],
                    detail: {
                      "status": ["SUCCEEDED"],
                      "stateMachineArn": [mySF.stateMachine.stateMachineArn]
                    }
                  },
                  targets: [new SqsQueue(myQueue)]
            })

[Visorgood]

Even simpler:

this works

myQueue.grantSendMessages(mySF.stateMachine.role)

this throws the error about cyclic dependency

myQueue.grantSendMessages(new ArnPrincipal(mySF.stateMachine.stateMachineArn))

[made2591]

@Visorgood I'm not 100%, but I think the error you are getting is due to the CDK lifecycle (see docs).

When you define the targets of your rule, the rule doesn't exist yet. The permissions are granted in the bind method (source): this requires the arn of the rule, to respect the least privilege principle - something we decided to introduce to build more safe rules. Before this change, there was no control over the permission given to events, so maybe this is the reason you didn't have to do it in two steps.

Try to add the target manually after the creation of the rule:

...
rule.addTarget(new SqsQueue(myQueue));

and see what happens. If it solved, imagine this behaviour is not so far from the one you get with SAM when you have IAM permissions that need to be constrained to the Function itself: you have to split the role from the definition of the function, to have references available in two different steps in another point of the template.

Hope to be helpful

[Visorgood]

Doing addTarget after creating the rule throws same error unfortunately.

[made2591]

Yes, I found what is wrong: it's due to the condition we put around the grantSendMessages (source) as suggested by @eladb in this comment. I fixed the bug by removing this condition, and I was able to have the two separated stacks and no circular dependencies. But...I believe that this permission condition should be there even if it introduces dependencies if there are two stacks so... I don't know how if for the moment we should just remove the boundaries because it's logically wrong to define them inside the binding operation, or change how addDependencies method (source) works... waiting from feedback from someone else

[made2591]

Btw, if someone wants to jump in and help in debugging, this is the stack I used to test:

import cdk = require('@aws-cdk/cdk');
import stepfunctions = require('@aws-cdk/aws-stepfunctions');
import events = require('@aws-cdk/aws-events');
import targets = require('@aws-cdk/aws-events-targets');
import sqs = require('@aws-cdk/aws-sqs');

interface RuleStackProps extends cdk.StackProps {
  queue: sqs.Queue
}

export class RuleStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props: RuleStackProps) {
    super(scope, id, props);

    let mySF = new stepfunctions.StateMachine(this, "MySF", {
      definition: new stepfunctions.Wait(this, 'Wait X Seconds', {
        duration: stepfunctions.WaitDuration.secondsPath('$.wait_time'),
      })
    })

    new events.Rule(this, 'MySFExecutionSucceededEvent', {
      ruleName: `succeeded-rule`,
      enabled: true,
      eventPattern: {
        source: ["aws.states"],
        detailType: ["Step Functions Execution Status Change"],
        detail: {
          "status": ["SUCCEEDED"],
          "stateMachineArn": [mySF.stateMachineArn]
        }
      },
      targets: [new targets.SqsQueue(props.queue)]
    })

  }
}

export interface TestSqsEventTargetsStack extends cdk.Stack {
  queue: sqs.Queue
}

export class TestSqsEventTargetsStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    this.queue = new sqs.Queue(this, "MyQueue")

  }
}

and

import 'source-map-support/register';
import cdk = require('@aws-cdk/cdk');
import { TestSqsEventTargetsStack, RuleStack } from '../lib/test-sqs-event-targets-stack';

const app = new cdk.App();
const queueStack = new TestSqsEventTargetsStack(app, 'TestSqsEventTargetsStack');
new RuleStack(app, 'RuleStack', { queue: queueStack.queue });
app.run()

This throws the error mentioned above: without boundaries conditions it doesn't. This is also the reason why with SNS Topic it works yet...

[Visorgood]

Great news! Thanks a lot for the effort!

[Visorgood]

Hello @made2591 ! I assume CDK 0.35 doesn't contain this fix yet?

[made2591]

No, it doesn't: to be honest as I said to you I don't know if it's ok to remove that permission constraint - actually, there was interest in propagating it to other targets as well 😊 so, I'm still waiting for a feedback for someone else from AWS, just to validate a PR in case it has to be fixed in that way

[Visorgood]

But then it means that this functionality although available in AWS will not be available via CDK? I mean the ability to integrate different stacks with CloudWatch Rules. Is there any other way to make this work, except putting everything into one stack?

[Visorgood]

I've managed to work it around by creating a Lambda in the same stack with the Rule, and trigger that Lambda by the rule, instead of sending an SQS message. The Lambda in turn sends a message to the queue that is in the different stack - and this works and worked before. Of course this required me to create small code for that Lambda, but this is still not so bad.