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.

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

fix(allow ITable for dynamodb stream patterns) #214

Merged
merged 5 commits into from
Jun 9, 2021
Merged

fix(allow ITable for dynamodb stream patterns) #214

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7451a9a
fix(allow ITable for dynamodb stream patterns)
hnishar Jun 8, 2021
172e127
fix(allow ITable for dynamodb stream patterns)
hnishar Jun 9, 2021
8ff9b21
fixing the cfn nag issues
hnishar Jun 9, 2021
774b75c
PR feedback
hnishar Jun 9, 2021
aa9da31
Merge branch 'main' into ddb-stream-itable-fix
hnishar Jun 9, 2021
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
5 changes: 3 additions & 2 deletions ...-solutions-constructs/aws-dynamodb-stream-lambda-elasticsearch-kibana/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ _Parameters_
|existingLambdaObj?|[`lambda.Function`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html)|Existing instance of Lambda Function object, providing both this and `lambdaFunctionProps` will cause an error.|
|lambdaFunctionProps?|[`lambda.FunctionProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.FunctionProps.html)|User provided props to override the default props for the Lambda function.|
|dynamoTableProps?|[`dynamodb.TableProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.TableProps.html)|Optional user provided props to override the default props for DynamoDB Table|
|existingTableObj?|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Existing instance of DynamoDB table object, providing both this and `dynamoTableProps` will cause an error.|
|existingTableInterface?|[`dynamodb.ITable`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.ITable.html)|Existing instance of DynamoDB table object or interface, providing both this and `dynamoTableProps` will cause an error.|
|dynamoEventSourceProps?|[`aws-lambda-event-sources.DynamoEventSourceProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda-event-sources.DynamoEventSourceProps.html)|Optional user provided props to override the default props for DynamoDB Event Source|
|esDomainProps?|[`elasticsearch.CfnDomainProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticsearch.CfnDomainProps.html)|Optional user provided props to override the default props for the Elasticsearch Service|
|domainName|`string`|Domain name for the Cognito and the Elasticsearch Service|
Expand All @@ -73,7 +73,8 @@ _Parameters_

| **Name** | **Type** | **Description** |
|:-------------|:----------------|-----------------|
|dynamoTable|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|
|dynamoTableInterface|[`dynamodb.ITable`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.ITable.html)|Returns an instance of dynamodb.ITable created by the construct|
|dynamoTable?|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add:

Returns an instance of dynamodb.Table created by the construct. IMPORTANT: If existingTableInterface was provided in the props object, this property will be undefined.

|lambdaFunction|[`lambda.Function`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html)|Returns an instance of lambda.Function created by the construct|
|userPool|[`cognito.UserPool`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-cognito.UserPool.html)|Returns an instance of cognito.UserPool created by the construct|
|userPoolClient|[`cognito.UserPoolClient`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-cognito.UserPoolClient.html)|Returns an instance of cognito.UserPoolClient created by the construct|
Expand Down
8 changes: 5 additions & 3 deletions ...ns/@aws-solutions-constructs/aws-dynamodb-stream-lambda-elasticsearch-kibana/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export interface DynamoDBStreamToLambdaToElasticSearchAndKibanaProps {
*
* @default - None
*/
readonly existingTableObj?: dynamodb.Table,
readonly existingTableInterface?: dynamodb.ITable,
/**
* Optional user provided props to override the default props
*
Expand Down Expand Up @@ -101,7 +101,8 @@ export class DynamoDBStreamToLambdaToElasticSearchAndKibana extends Construct {
private dynamoDBStreamToLambda: DynamoDBStreamToLambda;
private lambdaToElasticSearchAndKibana: LambdaToElasticSearchAndKibana;
public readonly lambdaFunction: lambda.Function;
public readonly dynamoTable: dynamodb.Table;
public readonly dynamoTableInterface: dynamodb.ITable;
public readonly dynamoTable?: dynamodb.Table;
public readonly userPool: cognito.UserPool;
public readonly userPoolClient: cognito.UserPoolClient;
public readonly identityPool: cognito.CfnIdentityPool;
Expand All @@ -126,7 +127,7 @@ export class DynamoDBStreamToLambdaToElasticSearchAndKibana extends Construct {
lambdaFunctionProps: props.lambdaFunctionProps,
dynamoEventSourceProps: props.dynamoEventSourceProps,
dynamoTableProps: props.dynamoTableProps,
existingTableObj: props.existingTableObj,
existingTableInterface: props.existingTableInterface,
deploySqsDlqQueue: props.deploySqsDlqQueue,
sqsDlqQueueProps: props.sqsDlqQueueProps
};
Expand All @@ -146,6 +147,7 @@ export class DynamoDBStreamToLambdaToElasticSearchAndKibana extends Construct {
this.lambdaToElasticSearchAndKibana = new LambdaToElasticSearchAndKibana(this, 'LambdaToElasticSearch', _props2);

this.dynamoTable = this.dynamoDBStreamToLambda.dynamoTable;
this.dynamoTableInterface = this.dynamoDBStreamToLambda.dynamoTableInterface;
this.userPool = this.lambdaToElasticSearchAndKibana.userPool;
this.userPoolClient = this.lambdaToElasticSearchAndKibana.userPoolClient;
this.identityPool = this.lambdaToElasticSearchAndKibana.identityPool;
Expand Down
5 changes: 3 additions & 2 deletions source/patterns/@aws-solutions-constructs/aws-dynamodb-stream-lambda/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,15 @@ _Parameters_
|existingLambdaObj?|[`lambda.Function`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html)|Existing instance of Lambda Function object, providing both this and `lambdaFunctionProps` will cause an error.|
|lambdaFunctionProps?|[`lambda.FunctionProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.FunctionProps.html)|User provided props to override the default props for the Lambda function.|
|dynamoTableProps?|[`dynamodb.TableProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.TableProps.html)|Optional user provided props to override the default props for DynamoDB Table|
|existingTableObj?|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Existing instance of DynamoDB table object, providing both this and `dynamoTableProps` will cause an error.|
|existingTableInterface?|[`dynamodb.ITable`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.ITable.html)|Existing instance of DynamoDB table object or interface, providing both this and `dynamoTableProps` will cause an error.|
|dynamoEventSourceProps?|[`aws-lambda-event-sources.DynamoEventSourceProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda-event-sources.DynamoEventSourceProps.html)|Optional user provided props to override the default props for DynamoDB Event Source|

## Pattern Properties

| **Name** | **Type** | **Description** |
|:-------------|:----------------|-----------------|
|dynamoTable|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|
|dynamoTableInterface|[`dynamodb.ITable`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.ITable.html)|Returns an instance of dynamodb.ITable created by the construct|
|dynamoTable?|[`dynamodb.Table`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-dynamodb.Table.html)|Returns an instance of dynamodb.Table created by the construct|
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment

|lambdaFunction|[`lambda.Function`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html)|Returns an instance of lambda.Function created by the construct|

## Lambda Function
Expand Down
18 changes: 12 additions & 6 deletions source/patterns/@aws-solutions-constructs/aws-dynamodb-stream-lambda/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export interface DynamoDBStreamToLambdaProps {
*
* @default - None
*/
readonly existingTableObj?: dynamodb.Table,
readonly existingTableInterface?: dynamodb.ITable,
/**
* Optional user provided props to override the default props
*
Expand All @@ -69,7 +69,8 @@ export interface DynamoDBStreamToLambdaProps {

export class DynamoDBStreamToLambda extends Construct {
public readonly lambdaFunction: lambda.Function;
public readonly dynamoTable: dynamodb.Table;
public readonly dynamoTableInterface: dynamodb.ITable;
public readonly dynamoTable?: dynamodb.Table;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expose both this as an optional property as well as the ITable, which will always exist. Then we need to document well when the Table object would be available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting something like this:

  public readonly dynamoTable?: dynamodb.Table;
  public readonly dynamoTableInterface?: dynamodb.ITable;

where dynamoTable will be available if the user user provides dynamoTableProps or existingTableObj of type Table and dynamoTableInterface will be available if the user provides existingTableObj of type ITable ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking:

  public readonly dynamoTable?: dynamodb.Table;
  public readonly dynamoTableInterface: dynamodb.ITable;

(making as much info as possible available). But I hadn't thought about names, that's unfortunate.

As I recall, if the client supplies existingTableObject the code currently returns ITable regardless of what was passed to the ITable parameter. This is actually pretty sticky - why don't you set up a meeting tomorrow morning for you, Ryan and me to toss around ideas. The cleanest solution would be to go ITable for everything about this construct - the only drawback is that if someone needs the table somewhere else they can't use it. The workaround would be to create the table somewhere else (either independently or as part of another construct).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, cannot go with ITable for everything then the user won't be able to make mutating calls on dynamoTable property e.g. dynamoTable.addGlobalSecondaryIndex(), also it will break the existing customer's code..


/**
* @summary Constructs a new instance of the LambdaToDynamoDB class.
Expand All @@ -88,20 +89,25 @@ export class DynamoDBStreamToLambda extends Construct {
lambdaFunctionProps: props.lambdaFunctionProps
});

this.dynamoTable = defaults.buildDynamoDBTableWithStream(this, {
const table: dynamodb.ITable = defaults.buildDynamoDBTableWithStream(this, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Purposefully pointing to an object as the wrong type seems a little cheesy (and fraught with peril). I like the returning 2 values approach better. It also allows cleaner code in this routine, as this line could be"

[dynamoTableInterface, dynamoTable] = defaults.buildDynamoDBTableWithStream(...

Lines 97-99 are then no longer needed

dynamoTableProps: props.dynamoTableProps,
existingTableObj: props.existingTableObj
existingTableInterface: props.existingTableInterface
});

this.dynamoTableInterface = table;
if (!props.existingTableInterface) {
this.dynamoTable = table as dynamodb.Table;
}

// Grant DynamoDB Stream read perimssion for lambda function
this.dynamoTable.grantStreamRead(this.lambdaFunction.grantPrincipal);
table.grantStreamRead(this.lambdaFunction.grantPrincipal);

// Add the Lambda event source mapping
const eventSourceProps = defaults.DynamoEventSourceProps(this, {
eventSourceProps: props.dynamoEventSourceProps,
deploySqsDlqQueue: props.deploySqsDlqQueue,
sqsDlqQueueProps: props.sqsDlqQueueProps
});
this.lambdaFunction.addEventSource(new DynamoEventSource(this.dynamoTable, eventSourceProps));
this.lambdaFunction.addEventSource(new DynamoEventSource(table, eventSourceProps));
}
}
98 changes: 94 additions & 4 deletions .../@aws-solutions-constructs/aws-dynamodb-stream-lambda/test/dynamodb-stream-lambda.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,13 +185,37 @@ test('check dynamodb table stream override', () => {

});

test('check getter methods', () => {
test('check getter methods without existingTableInterface', () => {
const stack = new cdk.Stack();

const construct: DynamoDBStreamToLambda = deployNewFunc(stack);

expect(construct.lambdaFunction !== null);
expect(construct.dynamoTable !== null);
expect(construct.lambdaFunction).toBeInstanceOf(lambda.Function);
expect(construct.dynamoTableInterface).toHaveProperty('tableName');
expect(construct.dynamoTable).toBeInstanceOf(dynamodb.Table);
expect(construct.dynamoTable).toHaveProperty('addGlobalSecondaryIndex');
});

test('check getter methods with existingTableInterface', () => {
const stack = new cdk.Stack();

const construct: DynamoDBStreamToLambda = new DynamoDBStreamToLambda(stack, 'test', {
existingTableInterface: new dynamodb.Table(stack, 'table', {
partitionKey: {
name: 'id',
type: dynamodb.AttributeType.STRING
},
stream: dynamodb.StreamViewType.NEW_AND_OLD_IMAGES
}),
lambdaFunctionProps: {
code: lambda.Code.fromAsset(`${__dirname}/lambda`),
runtime: lambda.Runtime.NODEJS_12_X,
handler: 'index.handler'
},
});

expect(construct.lambdaFunction).toBeInstanceOf(lambda.Function);
expect(construct.dynamoTable).toBeUndefined();
});

test('check exception for Missing existingObj from props', () => {
Expand All @@ -205,4 +229,70 @@ test('check exception for Missing existingObj from props', () => {
} catch (e) {
expect(e).toBeInstanceOf(Error);
}
});
});

test('check dynamodb table stream override with ITable', () => {
const stack = new cdk.Stack();
const existingTableInterface = dynamodb.Table.fromTableAttributes(stack, 'existingtable', {
tableArn: 'arn:aws:dynamodb:us-east-1:xxxxxxxxxxxxx:table/existing-table',
tableStreamArn: 'arn:aws:dynamodb:us-east-1:xxxxxxxxxxxxx:table/existing-table/stream/2020-06-22T18:34:05.824'
});
const props: DynamoDBStreamToLambdaProps = {
lambdaFunctionProps: {
code: lambda.Code.fromAsset(`${__dirname}/lambda`),
runtime: lambda.Runtime.NODEJS_12_X,
handler: 'index.handler'
},
existingTableInterface
};

new DynamoDBStreamToLambda(stack, 'test-lambda-dynamodb-stack', props);

expect(stack).toHaveResource('AWS::Lambda::EventSourceMapping', {
EventSourceArn: "arn:aws:dynamodb:us-east-1:xxxxxxxxxxxxx:table/existing-table/stream/2020-06-22T18:34:05.824",
});

expect(stack).toHaveResource('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: [
"xray:PutTraceSegments",
"xray:PutTelemetryRecords"
],
Effect: "Allow",
Resource: "*"
},
{
Action: "dynamodb:ListStreams",
Effect: "Allow",
Resource: "*"
},
{
Action: [
"dynamodb:DescribeStream",
"dynamodb:GetRecords",
"dynamodb:GetShardIterator"
],
Effect: "Allow",
Resource: "arn:aws:dynamodb:us-east-1:xxxxxxxxxxxxx:table/existing-table/stream/2020-06-22T18:34:05.824"
},
{
Action: [
"sqs:SendMessage",
"sqs:GetQueueAttributes",
"sqs:GetQueueUrl"
],
Effect: "Allow",
Resource: {
"Fn::GetAtt": [
"testlambdadynamodbstackSqsDlqQueue4CC9868B",
"Arn"
]
}
}
],
Version: "2012-10-17"
}
});
});
Loading