RFC 510: DynamoDB Global Table L2

[vinayak-kukreja]

This is a request for comments about DynamoDB GlobalTable L2 construct. See #510 for additional details.

APIs are signed off by @rix0rrr and @madeline-k .

---

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

[rix0rrr]

I like it! There are three big things left in my mind:

• I think a GSI probably shouldn't have a default capacity, we should make the user specify it? At least the default for a replica's GSI capacity should be the base GSI, not the replica table.
• I'd like one final thought to be spent on using enums vs enum-like classes for billing mode (and articulated why you prefer one over the other). I can also see why you might not want to.
• I'd like an example for encryption, it's not clear to me now whether it's a central property or a per-replica property

[rix0rrr]

Is this necessary to "turn on" the GSI for the replica? Or are all GSIs automatically turned on, and this is only necessary to tweak settings?

[rix0rrr]

Was there a conscious decision between a list and a map?

[vinayak-kukreja]

This is not required to turn on the GSIs. They would be enabled for each replica.

This is just to specify some settings for GSIs that are configurable per replica.

[vinayak-kukreja]

Was there a conscious decision between a list and a map?

Yes, I was just trying to keep it similar to how GSIs were defined. We can also have it like,

{
	'UniqueGsiName': {
		read: Capacity.fixed(55),
		contributorInsightsEnabled: false,
	}
}

[vinayak-kukreja]

read and contributorInsightsEnabled are the only two props that can be configured for the GSI per replica.

CFN resource: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-dynamodb-globaltable-replicaglobalsecondaryindexspecification.html

[rix0rrr]

{
	'UniqueGsiName': {
		read: Capacity.fixed(55),
		contributorInsightsEnabled: false,
	}
}

I kind of enjoy the above. Feels like less ceremony to me. No?

[vinayak-kukreja]

Hey, I have just updated the Replica GSI options from list to map. I can see this can also be done for GSI, Replica and LSI. Let me know if that is what you would prefer. :)

For me, it makes sense to have them as list since it conveys intent like, list of global secondary indexes, or list of replicas, but I am fine either way.

[rix0rrr]

This needs an example.

Also above in the RFC you said that kmsKeyArn was a per-replica property, but if the encryption setting is global that might be confusing.

[vinayak-kukreja]

Will add an example.

---

The keyArns only need to be provided if the encryption mode is KEY_ARNS. The encryption mode will remain the same for all replicas but the keys would be specific to the replica.

I can instead add a table level property to add [ { region: keyArn} ] but I believe it make more sense to have this in replica props.

[rix0rrr]

Again this could be an enum-like class: `

encryption: TableEncryption.regionKeyArns({
  'us-east-1': 'arn:kms:...',
  'us-east-2': 'arn:kms:...',
});

[rix0rrr]

Taking into account that the table itself also needs a key, and that should probably be an IKey for single-region consistency:

encryption: TableEncryption.customerManagedKey(tableKey, {
  replicaKeyArns: {
    'us-east-1': 'arn:kms:...',
    'us-east-2': 'arn:kms:...',
  }
});

Perhaps?

[rix0rrr]

Almost there! A couple of final details to think about, but they're very minor. I like this, after you consider the last couple of points I think this is good to merge.

[rix0rrr]

Don't forget the keyARN for the table itself. Which probably shouldn't be an ARN but an IKey.

[rix0rrr]

Taking into account that the table itself also needs a key, and that should probably be an IKey for single-region consistency:

encryption: TableEncryption.customerManagedKey(tableKey, {
  replicaKeyArns: {
    'us-east-1': 'arn:kms:...',
    'us-east-2': 'arn:kms:...',
  }
});

Perhaps?

[rix0rrr]

Encryption is not really relevant for this example, so if you remove it the example will be shorter and easier to read.

[vinayak-kukreja]

Will update this.

Pull request has been modified.