feat(appsync): add authenticationConfig to HttpDataSource, fixes #9934 #9971

haruharuharuby · 2020-08-25T21:27:32Z

Description

Adding authorization config to HttpDatasource in aws-appsync module.
Users will be able to specify the awsIamConfig in httpConfig as parameter of HttpDataSource.

Pull Request Checklist

Testing
[x] 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
[x-] Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
[x] Title: use lower-case and doesn't end with a period
[-] Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
[x] 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)

Fixes: #9934

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

…9934

BryanPan342

Thanks for the contribution 🥳 This is a great first pass!

It would be great if you could write up a section in the README talking about HTTP data source. Similar to this section!

See this PR in review right now on RDS! Just to talk about the IamConfig :)

BryanPan342 · 2020-08-25T22:33:57Z

packages/@aws-cdk/aws-appsync/lib/data-source.ts

+export interface HttpDataSourceAuthorizationConfig {
+  /**
+   * The authorization type required by the HTTP endpoint
+   */
+  readonly authorizationType: 'AWS_IAM';
+  /**
+   * The IAM configuration required by the HTTP endpoint
+   */
+  readonly awsIamConfig: HttpDataSourceIamConfig;
+}
+
+/**
+ * The IAM configuration required by the HTTP endpoint
+ */
+export interface HttpDataSourceIamConfig {
+  /**
+   * The signing region for AWS IAM authorization
+   */
+  readonly signingRegion: string;
+
+  /**
+   * The signing service name for AWS IAM authorization
+   */
+  readonly signingServiceName: string;
+}


If the authorization type is always going to AWS_IAM as seen by these docs, I think it might be best to just expose awsIamConfig and remove that additional level of abstraction!

@BryanPan342 I have a question.

JSII does not allow nested objects directly. I got a linting error when I tried to implement like below.

Only string-indexed map types are supported

export interface AwsIamConfig { /** * The authorization type required by the HTTP endpoint */ readonly authorizationType: 'AWS_IAM'; /** * The IAM configuration required by the HTTP endpoint */ readonly awsIamConfig: { /** * The signing region for AWS IAM authorization */ readonly signingRegion: string; /** * The signing service name for AWS IAM authorization */ readonly signingServiceName: string; } }

Do you know the better way to integrate it?

I still code separately. But I tried to along with Cfn definition.
4500ce8

packages/@aws-cdk/aws-appsync/lib/data-source.ts

Co-authored-by: Bryan Pan <[email protected]>

BryanPan342

Im still not convinced that we need the nested interface of

export interface AwsIamConfig {
  authorizationType: 'AWS_IAM',
  awsIamConfig: {
    signingRegion: 'us-east-1',
    signingServiceName: 'states,
  }
}

why can't we just expose the signing configuration?

export interface AwsIamConfig {
    signingRegion: string,
    signingServiceName: string,

and have CDK add the authorization type upon construction?

packages/@aws-cdk/aws-appsync/test/appsync-http.test.ts

BryanPan342 · 2020-08-27T16:51:09Z

packages/@aws-cdk/aws-appsync/README.md

+});
+```
+
+## Imports


i think this might have been moved on merge by accident

should be on line 213

packages/@aws-cdk/aws-appsync/README.md

Co-authored-by: Bryan Pan <[email protected]>

Pull request has been modified.

BryanPan342

Apologies I misread the unit test :( just a little nit picks and this LGTM 🥳

packages/@aws-cdk/aws-appsync/README.md

BryanPan342 · 2020-08-28T03:23:20Z

packages/@aws-cdk/aws-appsync/README.md

+});
+```
+
+## Imports


should be on line 213

packages/@aws-cdk/aws-appsync/test/appsync-http.test.ts

Co-authored-by: Bryan Pan <[email protected]>

BryanPan342 · 2020-08-28T03:55:01Z

i believe the build is failing for this reason

error TS0: @aws-cdk/aws-appsync.GraphqlApiBase#addHttpDataSource changes type of argument options when implementing @aws-cdk/aws-appsync.IGraphqlApi (expected @aws-cdk/aws-appsync.DataSourceOptions, found @aws-cdk/aws-appsync.HttpDataSourceOptions

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

Line 80 in beb0269

    
           addHttpDataSource(id: string, endpoint: string, options?: DataSourceOptions): HttpDataSource;

this needs to be changed to HttpDataSourceOptions instead of DataSourceOptions

…/github.com/hugtechio/aws-cdk into feature/authorization_within_httpdatasource

…ource

haruharuharuby · 2020-08-28T04:51:14Z

@BryanPan342 I've just pushed the code that removing nested construction. Thank you for reviewing.
e6250b2

haruharuharuby · 2020-08-28T06:12:29Z

@BryanPan342 I want to solve the CodeBuild error. Can you show me how to look for the Error in the CodeBuild log?

BryanPan342 · 2020-08-28T16:48:22Z

i believe the build is failing for this reason
error TS0: @aws-cdk/aws-appsync.GraphqlApiBase#addHttpDataSource changes type of argument options when implementing @aws-cdk/aws-appsync.IGraphqlApi (expected @aws-cdk/aws-appsync.DataSourceOptions, found @aws-cdk/aws-appsync.HttpDataSourceOptions
aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts

Line 80 in beb0269

addHttpDataSource(id: string, endpoint: string, options?: DataSourceOptions): HttpDataSource;

this needs to be changed to HttpDataSourceOptions instead of DataSourceOptions

@haruharuharuby this is the error + fix

if you hit the build logs link in the AWS CodeBuild CI Report then you will get directed to the output

then Ctrl-F for the !!!!! or scroll all the way to the bottom of the log.

haruharuharuby · 2020-08-29T04:18:25Z

#9934

BryanPan342

@haruharuharuby
I had an idea on how we could clean up the if/else statement.. thoughts?

BryanPan342 · 2020-08-29T06:27:09Z

packages/@aws-cdk/aws-appsync/test/appsync-http.test.ts

+      name: 'custom',
+      description: 'custom description',
+      authorizationConfig: {
+        authorizationType: 'AWS_IAM',


why is this line necessary? the authorizationType is fixed to AWS_IAM no?

Yes, It true. AWS_IAM takes only a single value. I remove it.

BryanPan342 · 2020-08-29T06:28:27Z

packages/@aws-cdk/aws-appsync/lib/data-source.ts

+  /**
+   * The authorization type required by the HTTP endpoint
+   */
+  readonly authorizationType: 'AWS_IAM';


is this field necessary if it's fixed?

could we just make the interface

export interface AwsIamConfig { readonly signingRegion: string; readonly signingServiceName: string; }

So the authorizationType removes from the caller. Building it Constructor in DataSource.

packages/@aws-cdk/aws-appsync/lib/data-source.ts

thx! Co-authored-by: Bryan Pan <[email protected]>

… addHttpDataSource

haruharuharuby · 2020-08-30T09:58:45Z

@BryanPan342 I've just passed CI process. (fixed some linting and Unit test)

BryanPan342

Woooo LGTM 🎉

Thanks for the contribution this looks great!

I'll let @MrArnoldPalmer give his blessing before its shipped 😊

haruharuharuby · 2020-09-01T09:59:21Z

@BryanPan342 Thank you for a lot of reviews. :)

mergify · 2020-09-02T16:51:23Z

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

BryanPan342 · 2020-09-02T17:17:51Z

@haruharuharuby I think you need to allow maintainers to edit your PR in order for Mergify to work

haruharuharuby · 2020-09-03T04:57:37Z

@BryanPan342 Sorry I made this PR from my org account. So suppose from this doc, I can't edit permission.
So I could not find that menu.

https://github.community/t/how-can-we-enable-allow-edits-from-maintainers-by-default/2847

is it necessary to remake PR on my regular account (haruharuharuby)

Pull request has been modified.

aws-cdk-automation · 2020-09-03T07:31:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7f608ba
Result: SUCCEEDED
Build Logs (available for 30 days)

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

BryanPan342 · 2020-09-03T16:22:51Z

@BryanPan342 Sorry I made this PR from my org account. So suppose from this doc, I can't edit permission.
So I could not find that menu.

https://github.community/t/how-can-we-enable-allow-edits-from-maintainers-by-default/2847

is it necessary to remake PR on my regular account (haruharuharuby)

@haruharuharuby im guessing that's the right move? if you upload it with the same edits i can approve of it right away

haruharuharuby · 2020-09-04T07:13:48Z

@BryanPan342 Thank you for following up. I made another PR by @haruharuharuby the same as this PR.
Please check it.

#10171

haruharuharuby · 2020-09-04T07:17:47Z

@haruharuharuby im guessing that's the right move? if you upload it with the same edits i can approve of it right away

Yes, I think the only way to mearging is to make another PR. looking over in this PR(coming from Org account), I could not find any way that displaying the "allow changes by maintainers".

@BryanPan342

BryanPan342 · 2020-09-04T15:33:03Z

Closing in favor of #10171

This PR supersedes #9971 ### Description Adding authorization config to HttpDatasource in aws-appsync module. Users will be able to specify the awsIamConfig in httpConfig as parameter of HttpDataSource. Fixes: #9934 ----

feat(appsync): add authenticationConfig to HttpDataSource, fixes aws#…

1467be2

…9934

haruharuharuby mentioned this pull request Aug 25, 2020

[aws-appsync] Adding AuthorizationConfig within construction HttpDataSource of the appsync module. #9934

Closed

2 tasks

BryanPan342 added the @aws-cdk/aws-appsync Related to AWS AppSync label Aug 25, 2020

BryanPan342 added this to the AppSync to 'developer preview' milestone Aug 25, 2020

BryanPan342 requested review from BryanPan342 and MrArnoldPalmer August 25, 2020 22:31

BryanPan342 reviewed Aug 25, 2020

View reviewed changes

haruharuharuby and others added 6 commits August 26, 2020 09:01

Update packages/@aws-cdk/aws-appsync/lib/data-source.ts

eab93cf

Co-authored-by: Bryan Pan <[email protected]>

Update packages/@aws-cdk/aws-appsync/lib/data-source.ts

a594a35

Co-authored-by: Bryan Pan <[email protected]>

feat(appsync): add example of http data source to the readme

dbbe4bf

feat(appsync): AuthorizationConfig along with Cfn definition

4500ce8

Merge branch 'master' into feature/authorization_within_httpdatasource

ad51794

Merge branch 'master' into feature/authorization_within_httpdatasource

b24c6f3

BryanPan342 previously requested changes Aug 27, 2020

View reviewed changes

Update packages/@aws-cdk/aws-appsync/README.md

442828b

Co-authored-by: Bryan Pan <[email protected]>

BryanPan342 reviewed Aug 28, 2020

View reviewed changes

haruharuharuby and others added 3 commits August 28, 2020 05:43

Update packages/@aws-cdk/aws-appsync/README.md

8c68f3c

Co-authored-by: Bryan Pan <[email protected]>

Update packages/@aws-cdk/aws-appsync/README.md

beb0269

Co-authored-by: Bryan Pan <[email protected]>

feat(appsync): Update readme (position of the Import header)

b289be5

haruharuharuby and others added 3 commits August 28, 2020 05:56

Merge branch 'master' into feature/authorization_within_httpdatasource

f8879e6

Merge branch 'feature/authorization_within_httpdatasource' of https:/…

26fe550

…/github.com/hugtechio/aws-cdk into feature/authorization_within_httpdatasource

feat(appsync) remove nested construction of awsIamConfig in HttpDataS…

e6250b2

…ource

Merge branch 'master' into feature/authorization_within_httpdatasource

5b574c6

haruharuharuby and others added 2 commits August 28, 2020 22:53

Merge branch 'master' into feature/authorization_within_httpdatasource

97ab65a

feat(appsync) fix options bug addHttpDataSource, addLambdaDataSource

08b42d9

Merge branch 'master' into feature/authorization_within_httpdatasource

48ee5a9

BryanPan342 reviewed Aug 29, 2020

View reviewed changes

haruharuharuby and others added 6 commits August 29, 2020 08:46

Update packages/@aws-cdk/aws-appsync/lib/data-source.ts

9be1c7c

thx! Co-authored-by: Bryan Pan <[email protected]>

feat(appsync) fix it is not necessary to specify authorizationType in…

f1a1145

… addHttpDataSource

feat(appsync) fix lint error

5e7d3af

feat(appsync) fix lint error

3268ff4

feat(appsync) fix lint error

e33d77c

feat(appsync) fix lint unit test

392bf12

SomayaB assigned MrArnoldPalmer Aug 31, 2020

BryanPan342 added the pr/do-not-merge This PR should not be merged at this time. label Aug 31, 2020

BryanPan342 previously approved these changes Aug 31, 2020

View reviewed changes

MrArnoldPalmer previously approved these changes Sep 2, 2020

View reviewed changes

MrArnoldPalmer removed the pr/do-not-merge This PR should not be merged at this time. label Sep 2, 2020

Merge branch 'master' into feature/authorization_within_httpdatasource

7f608ba

haruharuharuby mentioned this pull request Sep 4, 2020

feat(appsync): add authorization config to the HttpDataSource #10171

Merged

BryanPan342 closed this Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appsync): add authenticationConfig to HttpDataSource, fixes #9934 #9971

feat(appsync): add authenticationConfig to HttpDataSource, fixes #9934 #9971

haruharuharuby commented Aug 25, 2020 •

edited by BryanPan342

Loading

BryanPan342 left a comment

BryanPan342 Aug 25, 2020

haruharuharuby Aug 26, 2020 •

edited

Loading

BryanPan342 left a comment

BryanPan342 Aug 27, 2020

BryanPan342 Aug 28, 2020

BryanPan342 left a comment

BryanPan342 Aug 28, 2020

BryanPan342 commented Aug 28, 2020

haruharuharuby commented Aug 28, 2020

haruharuharuby commented Aug 28, 2020

BryanPan342 commented Aug 28, 2020

haruharuharuby commented Aug 29, 2020

BryanPan342 left a comment

BryanPan342 Aug 29, 2020

haruharuharuby Aug 30, 2020

BryanPan342 Aug 29, 2020

haruharuharuby Aug 30, 2020

haruharuharuby commented Aug 30, 2020

BryanPan342 left a comment

haruharuharuby commented Sep 1, 2020

mergify bot commented Sep 2, 2020

BryanPan342 commented Sep 2, 2020 •

edited

Loading

haruharuharuby commented Sep 3, 2020 •

edited

Loading

aws-cdk-automation commented Sep 3, 2020

BryanPan342 commented Sep 3, 2020

haruharuharuby commented Sep 4, 2020

haruharuharuby commented Sep 4, 2020 •

edited

Loading

BryanPan342 commented Sep 4, 2020

feat(appsync): add authenticationConfig to HttpDataSource, fixes #9934 #9971

feat(appsync): add authenticationConfig to HttpDataSource, fixes #9934 #9971

Conversation

haruharuharuby commented Aug 25, 2020 • edited by BryanPan342 Loading

Description

Pull Request Checklist

BryanPan342 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haruharuharuby Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

BryanPan342 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanPan342 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanPan342 commented Aug 28, 2020

haruharuharuby commented Aug 28, 2020

haruharuharuby commented Aug 28, 2020

BryanPan342 commented Aug 28, 2020

haruharuharuby commented Aug 29, 2020

BryanPan342 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haruharuharuby commented Aug 30, 2020

BryanPan342 left a comment

Choose a reason for hiding this comment

haruharuharuby commented Sep 1, 2020

mergify bot commented Sep 2, 2020

BryanPan342 commented Sep 2, 2020 • edited Loading

haruharuharuby commented Sep 3, 2020 • edited Loading

aws-cdk-automation commented Sep 3, 2020

AWS CodeBuild CI Report

BryanPan342 commented Sep 3, 2020

haruharuharuby commented Sep 4, 2020

haruharuharuby commented Sep 4, 2020 • edited Loading

BryanPan342 commented Sep 4, 2020

haruharuharuby commented Aug 25, 2020 •

edited by BryanPan342

Loading

haruharuharuby Aug 26, 2020 •

edited

Loading

BryanPan342 commented Sep 2, 2020 •

edited

Loading

haruharuharuby commented Sep 3, 2020 •

edited

Loading

haruharuharuby commented Sep 4, 2020 •

edited

Loading