fix(dynamodb): replicas not created on table replacement #13300
Conversation
|
|
|
Successfully tested the following:
|
|
Sorry @jogold , I was on vacation. I'll look at the PR this week. Thanks for the contribution! |
There was a problem hiding this comment.
Some comments/question, but in general looks great as always @jogold !
Did you go through an entire cycle of 1) creating a global Table with the old code, 2) updating the Table to this new code, to see if nothing breaks, 3) forcing a replacement, and 4) verifying the replacement is handled correctly? If not, could you please? Thanks!
| console.log('Update table: %j', data); | ||
|
|
||
| if (event.RequestType === 'Create' || event.RequestType === 'Update') { | ||
| return { PhysicalResourceId: `${event.ResourceProperties.TableName}-${event.ResourceProperties.Region}` }; |
There was a problem hiding this comment.
This is actually 2 functional changes, correct?
- Previously, we returned
PhysicalResourceIdalso for deletes, now we don't. - We used to return
event.ResourceProperties.Regionas the physical ID, now we return`${event.ResourceProperties.TableName}-${event.ResourceProperties.Region}`.
So, is 1. intentional, or accidental? And what about 2. - what will be the behavior for existing customers of global DB Tables?
There was a problem hiding this comment.
For existing users of global DB tables if they update to the new the code the only thing that will happen is the replacement of the inline policies with managed policies. This is because nothing changes on the properties of the custom resource so it won't receive any event (Create/Update/Delete).
Now if later a user decides to remove one or more replica regions, the custom resource will receive a Delete event. If we don't change how the physical resource id is returned for a Delete event then CF will receive a new physical resource id (table-region instead of region). Changing the physical resource id in a Delete event is not allowed by CF and the Delete will fail (from a CF perspective and this also means that it will not wait for isComplete). So yes 1. is intentional. (this explanation is also valid for a table replacement when the old replica gets deleted).
Returning an empty object on delete ensures that the custom resource provider framework uses the physical resource id from the event (should have been done in the first place).
EDIT: it's the framework, not CF, that doesn't allow to change the physical resource id during a Delete event
| // a replacement. Use the table name in the description to force a managed | ||
| // policy replacement when the table name changes. This way we preserve permissions | ||
| // to delete old replicas in case of a table replacement. | ||
| description: `DynamoDB replication managed policy for table ${sourceTable.tableName}`, |
There was a problem hiding this comment.
This is an awesome trick! I'll have to remember this one to use in other Custom Resources.
I'm just a little worried about the delete order - since this depends on the Table, won't it be deleted first, before the replica?
There was a problem hiding this comment.
This is fine because it depends on the role used by onEventHandler and isCompleteHandler, see also #8224 for more context on this SourceTableAttachedPolicy
See also my test #13300 (comment).
Yes, see #13300 (comment) |
|
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Process `Update` events resulting from table replacements. Include the table name in the physical resource id to receive a `Delete` event when the table is replaced. This allows to clean "old" replicas. Use a managed policy instead of an inline policy for the custom resource. An update of the description property of a managed policy requires a replacement. If we use the table name in the description it forces a managed policy replacement when the table name changes. This way we preserve permissions to delete old replicas in case of a table replacement: a new managed policy with permissions for the new table is created during the update phase and the old managed policy with permissions for the old table is removed only during the update clean up phase. The logical ID of the `SourceTableAttachedPolicy` needs to be updated because CF doesn't allow to change a resource type. Closes aws#12332 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Process
Updateevents resulting from table replacements.Include the table name in the physical resource id to receive a
Deleteevent when the table is replaced. This allows to clean "old" replicas.
Use a managed policy instead of an inline policy for the custom resource.
An update of the description property of a managed policy requires a
replacement. If we use the table name in the description it forces a
managed policy replacement when the table name changes. This way we preserve
permissions to delete old replicas in case of a table replacement: a new
managed policy with permissions for the new table is created during the
update phase and the old managed policy with permissions for the old table is
removed only during the update clean up phase.
The logical ID of the
SourceTableAttachedPolicyneeds to be updated becauseCF doesn't allow to change a resource type.
Closes #12332
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license