fix(dynamodb): cannot change serverSideEncryption from true to false

[RomainMuller]

When a table was deployed with serverSideEncryption set to true (by
requesting AWS_MANAGED or CUSTOM server side encryption), it was not
possible to switch back to DEFAULT as this could drop the
serverSideEncryption configuration altogether, which CloudFormation
will not allow.

This changes makes Table continue to not set the
serverSideEncryption configuration if nothing was configured (the user
chose the implicit default behavior), but to actually set the value
explicitly to false if the user explicitly requests DEFAULT
encryption.

This makes it possible to flip away from AWS_MANAGED and CUSTOM
encryption to the cheaper alternative that is DEFAULT.

Fixes #8286

---

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

[eladb]

This feels off. What happens if I set encryption to MANAGED and then remove my setting with the expectation that now I'll use the default encryption (as the documentation of this prop indicates). Sounds like this is going to fail.

Why not just always be explicit?

[RomainMuller]

This feels off. What happens if I set encryption to MANAGED and then remove my setting with the expectation that now I'll use the default encryption (as the documentation of this prop indicates). Sounds like this is going to fail.

Why not just always be explicit?

Because turning an argument required is a breaking change to a stable API, and so this is not an option?

[eladb]

This feels off. What happens if I set encryption to MANAGED and then remove my setting with the expectation that now I'll use the default encryption (as the documentation of this prop indicates). Sounds like this is going to fail.

Why not just always be explicit?

Because turning an argument required is a breaking change to a stable API, and so this is not an option?

I am not proposing to make it required. Just that no value will render the same as DEFAULT. Yes it means that existing stacks will now change to explicitly indicate this but I don’t think that’s a problem. Do you?

[RomainMuller]

@eladb - last I checked, changing this value causes "some interruption" according to the CloudFormation docs. I don't feel comfortable causing "some interruption" to all existing stacks...

[eladb]

@eladb - last I checked, changing this value causes "some interruption" according to the CloudFormation docs. I don't feel comfortable causing "some interruption" to all existing stacks...

So I would deprecate this field and introduce a new one with proper semantics.

[RomainMuller]

@eladb - last I checked, changing this value causes "some interruption" according to the CloudFormation docs. I don't feel comfortable causing "some interruption" to all existing stacks...

So I would deprecate this field and introduce a new one with proper semantics.

Won't that mean the new field must be optional, and the behavior when it's undefined must be exactly the current behavior? Wouldn't that basically be replicating the current field?

[eladb]

Won't that mean the new field must be optional, and the behavior when it's undefined must be exactly the current behavior? Wouldn't that basically be replicating the current field?

Can we introduce a new field for which the behavior will be that if it is undefined it will explicitly set encryption to DEFAULT?

[RomainMuller]

Won't that mean the new field must be optional, and the behavior when it's undefined must be exactly the current behavior? Wouldn't that basically be replicating the current field?

Can we introduce a new field for which the behavior will be that if it is undefined it will explicitly set encryption to DEFAULT?

But won't this change the behavior of existing apps that do not set this hypothetical new property (meaning all of them)?

[eladb]

Won't that mean the new field must be optional, and the behavior when it's undefined must be exactly the current behavior? Wouldn't that basically be replicating the current field?

Can we introduce a new field for which the behavior will be that if it is undefined it will explicitly set encryption to DEFAULT?

But won't this change the behavior of existing apps that do not set this hypothetical new property (meaning all of them)?

As long as they won't break or replace resources I think that's a legitimate change. No?

[RomainMuller]

As long as they won't break or replace resources I think that's a legitimate change. No?

We've come full circle to the original premise of the discussion... Which is that changing this value might cause some interruption and I don't think this is acceptable.

[mergify]

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-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 0b61e85
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

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).