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 authenticationConfig to HttpDataSource, fixes #9934 #9971

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1467be2
feat(appsync): add authenticationConfig to HttpDataSource, fixes #9934
haruharuharuby Aug 25, 2020
eab93cf
Update packages/@aws-cdk/aws-appsync/lib/data-source.ts
haruharuharuby Aug 26, 2020
a594a35
Update packages/@aws-cdk/aws-appsync/lib/data-source.ts
haruharuharuby Aug 26, 2020
dbbe4bf
feat(appsync): add example of http data source to the readme
haruharuharuby Aug 26, 2020
4500ce8
feat(appsync): AuthorizationConfig along with Cfn definition
haruharuharuby Aug 26, 2020
ad51794
Merge branch 'master' into feature/authorization_within_httpdatasource
haruharuharuby Aug 26, 2020
b24c6f3
Merge branch 'master' into feature/authorization_within_httpdatasource
haruharuharuby Aug 27, 2020
442828b
Update packages/@aws-cdk/aws-appsync/README.md
haruharuharuby Aug 28, 2020
8c68f3c
Update packages/@aws-cdk/aws-appsync/README.md
haruharuharuby Aug 28, 2020
beb0269
Update packages/@aws-cdk/aws-appsync/README.md
haruharuharuby Aug 28, 2020
b289be5
feat(appsync): Update readme (position of the Import header)
haruharuharuby Aug 28, 2020
f8879e6
Merge branch 'master' into feature/authorization_within_httpdatasource
haruharuharuby Aug 28, 2020
26fe550
Merge branch 'feature/authorization_within_httpdatasource' of https:/…
haruharuharuby Aug 28, 2020
e6250b2
feat(appsync) remove nested construction of awsIamConfig in HttpDataS…
haruharuharuby Aug 28, 2020
5b574c6
Merge branch 'master' into feature/authorization_within_httpdatasource
haruharuharuby Aug 28, 2020
97ab65a
Merge branch 'master' into feature/authorization_within_httpdatasource
haruharuharuby Aug 28, 2020
08b42d9
feat(appsync) fix options bug addHttpDataSource, addLambdaDataSource
haruharuharuby Aug 28, 2020
48ee5a9
Merge branch 'master' into feature/authorization_within_httpdatasource
haruharuharuby Aug 29, 2020
9be1c7c
Update packages/@aws-cdk/aws-appsync/lib/data-source.ts
haruharuharuby Aug 29, 2020
f1a1145
feat(appsync) fix it is not necessary to specify authorizationType in…
haruharuharuby Aug 29, 2020
5e7d3af
feat(appsync) fix lint error
haruharuharuby Aug 30, 2020
3268ff4
feat(appsync) fix lint error
haruharuharuby Aug 30, 2020
e33d77c
feat(appsync) fix lint error
haruharuharuby Aug 30, 2020
392bf12
feat(appsync) fix lint unit test
haruharuharuby Aug 30, 2020
7f608ba
Merge branch 'master' into feature/authorization_within_httpdatasource
haruharuharuby Sep 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions packages/@aws-cdk/aws-appsync/lib/data-source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,35 @@ export class DynamoDbDataSource extends BackedDataSource {
}
}

/**
* The authorization config in case the HTTP endpoint requires authorization
*/
export interface HttpDataSourceAuthorizationConfig {
/**
* The authorization type required by the HTTP endpoint
*/
readonly authorizationType: 'AWS_IAM';
Copy link
Contributor

Choose a reason for hiding this comment

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

is this field necessary if it's fixed?

could we just make the interface

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/**
* 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@haruharuharuby haruharuharuby Aug 26, 2020

Choose a reason for hiding this comment

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

@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

haruharuharuby marked this conversation as resolved.
Show resolved Hide resolved

/**
* Properties for an AppSync http datasource
*/
Expand All @@ -211,6 +240,7 @@ export interface HttpDataSourceProps extends BaseDataSourceProps {
* The http endpoint
*/
readonly endpoint: string;
readonly authorizationConfig?: HttpDataSourceAuthorizationConfig;
haruharuharuby marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -221,6 +251,7 @@ export class HttpDataSource extends BaseDataSource {
super(scope, id, props, {
httpConfig: {
endpoint: props.endpoint,
authorizationConfig: props.authorizationConfig
},
type: 'HTTP',
});
Expand Down
15 changes: 13 additions & 2 deletions packages/@aws-cdk/aws-appsync/lib/graphqlapi-base.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ITable } from '@aws-cdk/aws-dynamodb';
import { IFunction } from '@aws-cdk/aws-lambda';
import { CfnResource, IResource, Resource } from '@aws-cdk/core';
import { DynamoDbDataSource, HttpDataSource, LambdaDataSource, NoneDataSource } from './data-source';
import { DynamoDbDataSource, HttpDataSource, LambdaDataSource, NoneDataSource, HttpDataSourceAuthorizationConfig } from './data-source';

/**
* Optional configuration for data sources
Expand All @@ -22,6 +22,16 @@ export interface DataSourceOptions {
readonly description?: string;
}

/**
* Optional configuration for Http data sources
*/
export interface HttpDataSourceOptions extends DataSourceOptions {
/**
* The authorization config in case the HTTP endpoint requires authorization
*/
readonly authorizationConfig?: HttpDataSourceAuthorizationConfig;
}

/**
* Interface for GraphQL
*/
Expand Down Expand Up @@ -140,12 +150,13 @@ export abstract class GraphqlApiBase extends Resource implements IGraphqlApi {
* @param endpoint The http endpoint
* @param options The optional configuration for this data source
*/
public addHttpDataSource(id: string, endpoint: string, options?: DataSourceOptions): HttpDataSource {
public addHttpDataSource(id: string, endpoint: string, options?: HttpDataSourceOptions): HttpDataSource {
return new HttpDataSource(this, id, {
api: this,
endpoint,
name: options?.name,
description: options?.description,
authorizationConfig: options?.authorizationConfig
});
}

Expand Down
29 changes: 29 additions & 0 deletions packages/@aws-cdk/aws-appsync/test/appsync-http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,35 @@ describe('Http Data Source configuration', () => {
});
});

test('appsync configures name, authorizationConfig correctly', () => {
// WHEN
api.addHttpDataSource('ds', endpoint, {
name: 'custom',
description: 'custom description',
authorizationConfig: {
authorizationType: 'AWS_IAM',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

awsIamConfig: {
signingRegion: 'us-east-1',
signingServiceName: 'states'
}
BryanPan342 marked this conversation as resolved.
Show resolved Hide resolved
}
});

// THEN
expect(stack).toHaveResourceLike('AWS::AppSync::DataSource', {
Type: 'HTTP',
Name: 'custom',
Description: 'custom description',
AuthorizationConfig: {
authorizationType: 'AWS_IAM',
awsIamConfig: {
signingRegion: 'us-east-1',
signingServiceName: 'states'
}
}
});
})

test('appsync errors when creating multiple http data sources with no configuration', () => {
// THEN
expect(() => {
Expand Down