Skip to content
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

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

Merged
merged 10 commits into from
Jun 4, 2019
Merged

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

merged 10 commits into from
Jun 4, 2019

Conversation

made2591
Copy link
Contributor

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 made2591 requested a review from a team as a code owner May 30, 2019 12:44
package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-events-targets/package.json Outdated Show resolved Hide resolved
eladb
eladb previously requested changes May 30, 2019
package-lock.json Show resolved Hide resolved
packages/@aws-cdk/aws-events-targets/lib/sqs.ts Outdated Show resolved Hide resolved
@made2591
Copy link
Contributor Author

@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
Copy link
Contributor Author

made2591 commented Jun 1, 2019

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

@made2591 made2591 closed this Jun 1, 2019
@made2591 made2591 reopened this Jun 1, 2019
@made2591
Copy link
Contributor Author

made2591 commented Jun 1, 2019

@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 😅

@rix0rrr rix0rrr self-assigned this Jun 4, 2019
@rix0rrr rix0rrr dismissed eladb’s stale review June 4, 2019 12:12

Changes addressed

@rix0rrr rix0rrr merged commit 078e34a into aws:master Jun 4, 2019
@made2591 made2591 deleted the feature/aws-events-targets-sqs branch June 4, 2019 16:48
@Visorgood
Copy link

Visorgood commented Jun 11, 2019

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
Copy link

Even simpler:

this works

myQueue.grantSendMessages(mySF.stateMachine.role)

this throws the error about cyclic dependency

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

@made2591
Copy link
Contributor Author

made2591 commented Jun 11, 2019

@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
Copy link

Doing addTarget after creating the rule throws same error unfortunately.

@made2591
Copy link
Contributor Author

made2591 commented Jun 18, 2019

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
Copy link
Contributor Author

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
Copy link

Great news! Thanks a lot for the effort!

@Visorgood
Copy link

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

@made2591
Copy link
Contributor Author

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
Copy link

Visorgood commented Jun 20, 2019

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
Copy link

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.

This was referenced Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloudwatch SQS Event Target
4 participants