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): Move appsync permission grants to base class #27818

Closed
wants to merge 8 commits into from

Conversation

TimQuelch
Copy link

This allows the grant functions to be called for an API imported with GraphqlApi.fromGraphqlApiAttributes

The grant methods as they currently exist don't use any properties that are unique to the full GraphqlApi construct. They are able to be moved to GraphqlApiBase and IGraphqlApi without any behavior change.

Closes #23031 .


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

This allows the grant functions to be called for an imported graphql API
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Nov 2, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team November 2, 2023 23:56
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.

@TimQuelch
Copy link
Author

I don't think anything necessarily needs to be added to the current README, as this change should just make a imported API act more like a constructed one. Although I'm happy to modify it if anyone disagrees

Exemption Request

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Nov 3, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, the implementation looks good.

However, a couple of additions are needed (as suggested by the linter):

  1. An integration test (here) to verify that granting permissions on an imported API works for better coverage
  2. Some documentation about the fact that permissions are grantable on imported APIs as well (the Permissions section is a good place for it)

@vinayak-kukreja vinayak-kukreja removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 7, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 7, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review November 8, 2023 04:33

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 8, 2023
This reverts commit 9e74b05.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks for the implementation 👍
Some re-organization is needed in my opinion.

@@ -108,4 +109,29 @@ new Function(stack, 'testFail', {
environment: { APPSYNC_ENDPOINT: api.graphqlUrl },
});

const otherStack = new Stack(app, 'aws-appsync-import-integ');
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation looks good.
Can you please move it to a separate integ.graphql-import-iam.ts file in the same directory?

the expected `arn` for the imported api, given the `apiId`. For creating data
sources and resolvers, an `apiId` is sufficient.
the expected `arn` for the imported api, given the `apiId`. sources, creating
resolvers, and granting IAM permissions, an `apiId` is sufficient.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's instead add a section here:

### Permissions on imported APIs

You can use the `grant` functions on imported GraphQL APIs as well:

```ts
declare const api: appsync.GraphqlApi;
declare const role: iam.Role;

const importedApi = appsync.GraphqlApi.fromGraphqlApiAttributes(this, 'IApi', {
  graphqlApiId: api.apiId,
  graphqlApiArn: api.arn,
});

// For generic types
importedApi.grantMutation(role, 'updateImportedExample');

// For custom types and granular design
importedApi.grant(role, appsync.IamResource.ofType('Mutation', 'updateImportedExample'), 'appsync:GraphQL');

Feel free to improve it further.

@kaizencc
Copy link
Contributor

@TimQuelch are you still available to work on this? Looks like there are some outstanding action items from the latest review.

@paulhcsun
Copy link
Contributor

Closing for staleness, @TimQuelch feel free to open a new PR if you'd like to continue working on this.

@paulhcsun paulhcsun closed this Jan 30, 2024
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/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

appsync-alpha: Add grant methods to IGraphqlApi
6 participants