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(appsync): add L2 constructs for AWS AppSync Events #32505

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

kwwendt
Copy link

@kwwendt kwwendt commented Dec 13, 2024

Issue # (if applicable)

Closes #32004

Reason for this change

This is in support of AWS AppSync Events.

Description of changes

  • New constructs for EventApi and ChannelNamespace to support AWS AppSync Events.
  • Create common file for authorization config across EventApi and GraphqlApi constructs.
  • Create common file for common resources across EventApi and GraphqlApi constructs.

Description of how you validated changes

Added both unit and integration tests for AWS AppSync Event API changes.

Contributors

@mazyu36 @onlybakam @kwwendt

Checklist


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

@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Dec 13, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team December 13, 2024 03:10
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Dec 13, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kwwendt kwwendt changed the title feat(appsync): Add L2 constructs for AWS AppSync Events feat(appsync): add L2 constructs for AWS AppSync Events Dec 13, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 13, 2024 03:21

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

@kwwendt kwwendt requested a review from godwingrs22 December 20, 2024 17:39
@mergify mergify bot dismissed godwingrs22’s stale review December 28, 2024 00:53

Pull request has been modified.

@gracelu0 gracelu0 removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 30, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 30, 2024
@godwingrs22 godwingrs22 added the needs-security-review Related to feature or issues that needs security review label Jan 2, 2025
Copy link
Member

@godwingrs22 godwingrs22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kwwendt for addressing the comments. Your effort on this L2 construct is greatly appreciated, and I'm looking forward to seeing it in action. I've left a few additional comments related to the grant methods, particularly regarding their implementation and testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the unit tests we have for the grantConnect(), grantPublish(), and grantSubscribe() methods on the channel namespace level. However, to ensure these methods work as expected in a real-world scenario, it would be beneficial to update this integration tests.

Specifically, I recommend updating this integ test that tests the whole event api. The test could include:

  1. Using api.grantConnect() to allow connection to the API
  2. Using namespace.grantPublish() to permit publishing events to specific channel namespace
  3. Using namespace.grantSubscribe() to allow subscribing to events in a specific channel namespace
  4. Using namespace.grantPublishAndSubscribe() to allow publishing and subscribing to events

After granting these permissions, the test could attempt to connect to the API, publish test events, and subscribe to receive events. This would provide a more comprehensive validation of the constructs in a practical context.

Let me know if it doesn't makes sense.

const defaultNamespace = api.addChannelNamespace('default');
const namespace1 = api.addChannelNamespace('namespace1');

defaultNamespace.grantPublish(func);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does grantPublish and grantSubscribe applicable to both channel namespace and event api? I noticed that the test cases only demonstrate granting publish and subscribe access at the channel namespace level, and I couldn't find test cases for granting these permissions to the whole event API.

Additionally, the IAM console descriptions state as follows

EventPublish - Grants permission to publish events to a channel namespace
EventSubscribe - Grants permission to subscribe to a channel namespace

This makes me wonder about the necessity and functionality of the following methods in eventapi.ts, given that we have similar methods in channel-namespace.ts

public grantPublish(grantee: IGrantable)
public grantSubscribe(grantee: IGrantable)
public grantPublishAndSubscribe(grantee: IGrantable)

If these grant methods are indeed applicable to the event API as a whole, it would be beneficial to have unit tests and integration tests that cover these methods, similar to how we test the grant methods for channel namespaces.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods for defined in eventapi.ts would grant either publish, subscribe, or both for all channel namespaces associated with the API. This is called out in the doc strings for each method. Example below for grantPublish:

  /**
   * Adds an IAM policy statement for EventPublish access to this EventApi to an IAM
   * principal's policy. This grants publish permission for all channels within the API.
   *
   * @param grantee The principal
   */

That's a good catch on missing unit tests there. I will add those in. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. Thanks for the clarification.

Comment on lines +147 to +148
defaultNamespace.grantSubscribe(func);
namespace1.grantSubscribe(func);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have unit test and integ test similar that tests namespace1.grantPublishAndSubscribe() grant method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in this new unit test: Appsync Event API channel namespace - grant publish and subscribe from channel in appsync-channel-namespace.test.ts.

@mergify mergify bot dismissed godwingrs22’s stale review January 11, 2025 00:37

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 91172b4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-security-review Related to feature or issues that needs security review p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

appsync: L2 for AppSync Events
6 participants