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.

Sign up for GitHub

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

Jump to bottom

feat(events-targets): add support for AppSync as an EventBridge rule target #29584

Merged

mergify merged 35 commits into aws:main from onlybakam:appsync-event-target

May 13, 2024

Contributor

onlybakam commented Mar 22, 2024 •

edited

Reason for this change

Introduces support to configure AppSync GraphQLAPI as an EventBridge target.

announcement: https://aws.amazon.com/about-aws/whats-new/2024/01/amazon-eventbridge-appsync-target-buses/
documentation: https://docs.aws.amazon.com/eventbridge/latest/userguide/target-appsync.html

Description of changes

Expose GraphQLEndpointArn attribute in L2 GraphQLAPI construct
Implement events.IRuleTarget for AppSync

rule.addTarget(new targets.AppSync(api, {
  graphQLOperation: 'mutation Publish($message: String!){ publish(message: $message) { message } }',
  variables: events.RuleTargetInput.fromObject({
    message: 'hello world',
  }),
  deadLetterQueue: queue,
}));

Description of how you validated changes

unit test and integration tests

Issue

Solves #29884

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

onlybakam added 15 commits

February 23, 2024 18:04


          --wip-- [skip ci]

35e2940


          --wip-- [skip ci]

5e15a69


          --wip-- [skip ci]

c96645e


          Merge branch 'aws:main' into appsync-event-target

291c856


          Merge branch 'aws:main' into appsync-event-target

48e7451


          add unit tests

22be0d4


          --wip-- [skip ci]

73aa8ac


          --wip-- [skip ci]

0d108bf


          Merge branch 'aws:main' into appsync-event-target

dac9bed


          Merge branch 'aws:main' into appsync-event-target

cf4317f


          update readme

75c66c8


          Merge remote-tracking branch 'origin/appsync-event-target' into appsy…

0ef6506

…nc-event-target


          Merge branch 'aws:main' into appsync-event-target

01bd63a


          update appsync README

3a58232


          Merge remote-tracking branch 'origin/appsync-event-target' into appsy…

3a4b107

…nc-event-target

github-actions bot added p2 beginning-contributor labels

aws-cdk-automation requested a review from a team

March 22, 2024 21:29


          update README

46a4b6c

github-actions bot mentioned this pull request

Weekly PR metrics report #29602

Closed


          fix readme

bbb4c85

aws-cdk-automation added the pr/needs-community-review label

github-actions bot mentioned this pull request

Monthly PR metrics report #29668

Closed

lpizzinidev suggested changes

View reviewed changes

Contributor

lpizzinidev left a comment

Thanks 👍
Left some suggestions for minor adjustments

packages/aws-cdk-lib/aws-events-targets/README.md Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-events-targets/lib/appsync.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-events-targets/lib/appsync.ts

+                  if (this.props.deadLetterQueue) {
+                    addToDeadLetterQueueResourcePolicy(rule, this.props.deadLetterQueue);
+                  }

Contributor

lpizzinidev Apr 1, 2024

From docs:

EventBridge supports AWS AppSync public GraphQL APIs. EventBridge does not currently support AWS AppSync Private APIs.

Shouldn't we validate visibility: 'GLOBAL' as well?

Contributor Author

onlybakam Apr 4, 2024

added check.

packages/aws-cdk-lib/aws-events-targets/lib/index.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-events-targets/lib/appsync.ts

+                  const role = this.props.eventRole || singletonEventRole(this.appsyncApi);
+                  // if a role was not provided, attach a permission
+                  if (!this.props.eventRole) {

Contributor

lpizzinidev Apr 1, 2024

Suggested change

      
                if (!this.props.eventRole) {
          
                if (!this.props.eventRole && this.props.mutationFields.length) {

Should we grant permissions if there are no mutationFields?
Also, is it ok to assume that the provided eventRole will always have the necessary permissions?

Contributor Author

onlybakam Apr 4, 2024

removed mutationFields, developer needs to provide the proper permissions if they specify the role directly. If eventRole is provided by customer, there's no check.

Contributor

lpizzinidev Apr 6, 2024

developer needs to provide the proper permissions if they specify the role directly

Can you please add it to the README with an example?

Contributor Author

onlybakam Apr 25, 2024

done

packages/aws-cdk-lib/aws-events-targets/lib/appsync.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-events-targets/lib/appsync.ts Outdated

+                /**
+                 * The variables that are include in the GraphQL operation.
+                 */
+                readonly variables: events.RuleTargetInput;

Contributor

lpizzinidev Apr 1, 2024

Suggested change

      
              readonly variables: events.RuleTargetInput;
          
              readonly variables?: events.RuleTargetInput;

Can we expect GraphQL queries without variables?

Contributor Author

onlybakam Apr 4, 2024

changed to optional

packages/aws-cdk-lib/aws-events-targets/test/appsync/appsync.test.ts Outdated

Comment on lines 9 to 10

		// GIVEN
		let stack: cdk.Stack;

Contributor

lpizzinidev Apr 1, 2024

Suggested change

      
            // GIVEN
          
            let stack: cdk.Stack;
          
            describe('AppSync GraphQL API target', () => {
          
              let stack: cdk.Stack;
          
              ...

Let's wrap it in a describe to keep it more organized.
Also, can you please remove the commented dead-code in the file?

Contributor Author

onlybakam Apr 4, 2024

updated.

aws-cdk-automation removed the pr/needs-community-review label

onlybakam and others added 5 commits

April 3, 2024 15:41


          Update packages/aws-cdk-lib/aws-events-targets/README.md

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>


          Update packages/aws-cdk-lib/aws-events-targets/lib/appsync.ts

2b77f66

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>


          Update packages/aws-cdk-lib/aws-events-targets/lib/appsync.ts

6cf0ebd

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>


          Add visibility check. Remove

481bdba


          update test

7c3d855

onlybakam added 2 commits

April 24, 2024 15:27


          Rename props to match GraphQLApi type and fix readme comment

80de63a


          update tests

d4667c4

onlybakam had a problem deploying to test-pipeline

April 26, 2024 18:06

— with

GitHub Actions Failure

aws-cdk-automation previously requested changes

View reviewed changes

Collaborator

aws-cdk-automation left a comment

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

onlybakam force-pushed the appsync-event-target branch from 3c1159b to d4667c4 Compare

May 2, 2024 00:40

aws-cdk-automation dismissed their stale review

May 2, 2024 00:43

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

onlybakam added 2 commits

May 1, 2024 17:59


          Merge branch 'main' into appsync-event-target

af01610


          update to eventRole

1621ad9

gracelu0 previously requested changes

View reviewed changes

packages/aws-cdk-lib/aws-appsync/lib/graphqlapi-base.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-appsync/lib/graphqlapi.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-events-targets/test/appsync/appsync.test.ts Outdated

+                        message: events.EventField.fromPath('$.detail'),
+                      }),
+                    }));
+                  }).toThrow('You must have AWS_IAM authorization mode enabled on your API to configure an AppSync target');

Contributor

gracelu0 May 7, 2024

is this the correct error to be checking for this test case? should be it expecting "throw new Error('You must have a valid graphQLEndpointArn set')" instead?

Contributor Author

onlybakam May 8, 2024

right. fixing setup and check.

packages/aws-cdk-lib/aws-events-targets/test/appsync/appsync.test.ts

+                  const sec_api = new appsync.GraphqlApi(stack, 'sec_api', {
+                    name: 'no_iam_api',
+                    definition: appsync.Definition.fromFile(path.join(__dirname, 'appsync.test.graphql')),
+                    authorizationConfig: { additionalAuthorizationModes: [{ authorizationType: appsync.AuthorizationType.IAM }] },

Contributor

gracelu0 May 7, 2024

what happens if the secondary auth type is not IAM ?

Contributor Author

onlybakam May 8, 2024

IAM needs to be one of the supported modes. either primary or secondary. if none are configured as IAM the target definition should fail.

packages/aws-cdk-lib/aws-events-targets/README.md Outdated

+              ```ts
+              import * as appsync from 'aws-cdk-lib/aws-appsync';
+              declare const api: appsync.GraphqlApi;

Contributor

gracelu0 May 7, 2024

I think we should include the instantiation of GraphqlApi to make the example more complete, especially if there are properties that need to be configured to a certain value, e.g. AWS_IAM for authorization


          Update packages/aws-cdk-lib/aws-appsync/lib/graphqlapi-base.ts

6ed6eee

Co-authored-by: Grace Luo <54298030+gracelu0@users.noreply.github.com>

mergify bot dismissed gracelu0’s stale review

May 8, 2024 00:25

Pull request has been modified.

onlybakam and others added 3 commits

May 7, 2024 17:25


          Update packages/aws-cdk-lib/aws-appsync/lib/graphqlapi.ts

b75afa5

Co-authored-by: Grace Luo <54298030+gracelu0@users.noreply.github.com>


          Fix readme and update appsync.test.ts

e18567e


          fix readme

16fc9a7

gracelu0 previously approved these changes

View reviewed changes

Contributor

gracelu0 left a comment

LGTM, thank you for contributing! :)

Contributor

mergify bot commented May 11, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Contributor

gracelu0 commented May 11, 2024

@Mergifyio update

Contributor

mergify bot commented May 11, 2024

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission


          Merge branch 'main' into appsync-event-target

cbaf203

mergify bot dismissed gracelu0’s stale review

May 11, 2024 00:08

Pull request has been modified.

gracelu0 approved these changes

View reviewed changes

Contributor

mergify bot commented May 12, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).


          Merge branch 'main' into appsync-event-target

87f427d

Collaborator

aws-cdk-automation commented May 12, 2024

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 87f427d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Contributor

mergify bot commented May 12, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Contributor

gracelu0 commented May 13, 2024

@Mergifyio queue

Contributor

mergify bot commented May 13, 2024

queue

🛑 The pull request has been removed from the queue `default`

Pull request #29584 has been dequeued. GitHub can't merge the pull request for now.. GitHub can't merge the pull request for an unknown reason. You should retry later..

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

Contributor

gracelu0 commented May 13, 2024

@Mergifyio refresh

Contributor

mergify bot commented May 13, 2024

refresh

✅ Pull request refreshed

mergify bot merged commit 5be88a3 into aws:main

9 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment