Skip to content

Conversation

53ningen
Copy link
Contributor

@53ningen 53ningen commented Oct 26, 2019

Issue #, if available: #289

Description of changes:
Please refer to #289 (comment)

  • Fixed the DynamoDB stream ARN format
  • ListStreams action does not support specific stream.

Description of how you validated changes:

  • make pr

I deployed sam app like this:

Globals:
  Function:
    Timeout: 10
    Runtime: python3.7

Resources:
  DDBTable:
    Type: AWS::DynamoDB::Table
    Properties:
      AttributeDefinitions:
        - AttributeName: id
          AttributeType: S
      KeySchema:
        - AttributeName: id
          KeyType: HASH
      ProvisionedThroughput:
        ReadCapacityUnits: 1
        WriteCapacityUnits: 1
      StreamSpecification:
        StreamViewType: NEW_IMAGE

  ProcessDDBStreamFunction:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: process_ddb_stream/
      Handler: app.lambda_handler
      Policies:
      - DynamoDBStreamReadPolicy:
          TableName: !Select [1, !Split ["/", !GetAtt DDBTable.StreamArn]]
          StreamName: !Select [3, !Split ["/", !GetAtt DDBTable.StreamArn]]

ProcessDDBStreamFunction's Role got the policies as below:

{
  "permissionsBoundary": {},
  "roleName": "aiueo-ProcessDDBStreamFunctionRole-########",
  "policies": [
    {
      "document": {
        "Statement": [
          {
            "Action": [
              "dynamodb:DescribeStream",
              "dynamodb:GetRecords",
              "dynamodb:GetShardIterator"
            ],
            "Resource": "arn:aws:dynamodb:ap-northeast-1:########:table/aiueo-DDBTable-########/stream/2019-10-26T15:18:10.410",
            "Effect": "Allow"
          },
          {
            "Action": [
              "dynamodb:ListStreams"
            ],
            "Resource": "*",
            "Effect": "Allow"
          }
        ]
      },
      "name": "ProcessDDBStreamFunctionRolePolicy0",
      "type": "inline"
    },
    {
      "document": {
        "Version": "2012-10-17",
        "Statement": [
          {
            "Effect": "Allow",
            "Action": [
              "logs:CreateLogGroup",
              "logs:CreateLogStream",
              "logs:PutLogEvents"
            ],
            "Resource": "*"
          }
        ]
      },
      "name": "AWSLambdaBasicExecutionRole",
      "id": "########",
      "type": "managed",
      "arn": "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
    }
  ],
  "trustedEntities": [
    "lambda.amazonaws.com"
  ]
}

I associated the function with the ddb manually, then I created records into the ddb.
I verified that the function was triggered by ddb streams events successfully.

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected
  • Add/update example to examples/2016-10-31

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

@codecov-io
Copy link

codecov-io commented Oct 26, 2019

Codecov Report

Merging #1222 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1222   +/-   ##
========================================
  Coverage    94.43%   94.43%           
========================================
  Files           72       72           
  Lines         3736     3736           
  Branches       739      739           
========================================
  Hits          3528     3528           
  Misses         105      105           
  Partials       103      103

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbc8f79...e6186e3. Read the comment docs.

@mcoup
Copy link

mcoup commented Nov 3, 2019

It is a fix but it still feels a bit like a workaround. Do we maybe have a way to have this added to ddb:

Desired:
"Fn::GetAtt": ["MyTable", "StreamName"]

ddb already supports:
table name: "Ref": "MyTable""
table arn: "Fn::GetAtt": ["MyTable", "Arn"]
stream arn: "Fn::GetAtt": ["MyTable", "StreamArn"]

src

Minor side note,
TableName: !Select [1, !Split ["/", !GetAtt DDBTable.StreamArn]]

Can thus be simplified to:
TableName: !Ref DDBTable

Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

Looks good, just one change and it should be good to go!

"Action": [
"dynamodb:ListStreams"
],
"Resource": "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we tighten the resources that this applies to? This way, they can list all the streams for the table that they specify in the policy template, or all streams if the user specifies a "*" for TableName.

            "Resource": {
              "Fn::Sub": [
                "arn:${AWS::Partition}:dynamodb:${AWS::Region}:${AWS::AccountId}:table/${tableName}/stream/*",
                {
                  "tableName": {
                    "Ref": "TableName"
                  }
                }
              ]
            }

@53ningen 53ningen requested a review from keetonian November 12, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants