feat: add support to use a different KMS key for backup encryption #511

Aashiq-J · 2024-11-06T09:50:47Z

Description

Issue : #509

Release required?

No release
Patch release (x.x.X)
Minor release (x.X.x)
Major release (X.x.x)

Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

If relevant, a test for the change is included or updated with this PR.
If relevant, documentation for the change is included or updated with this PR.

For mergers

Use a conventional commit message to set the release level. Follow the guidelines.
Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
Use the Squash and merge option.

Aashiq-J · 2024-11-06T09:51:05Z

/run pipeline

Aashiq-J · 2024-11-06T12:58:07Z

/run pipeline

ocofaigh · 2024-11-06T14:49:46Z

Need to wait for ~~#513~~ #514 to be merged to unblock pipeline

ocofaigh · 2024-11-06T14:51:00Z

solutions/standard/variables.tf

    tcp_keepalives_count       = 6
    archive_timeout            = 1800
-    wal_level                  = "replica"
+    wal_level                  = "logical"


no don't do this - there is a backend bug. We need to remove wal_level all together from the configuration so it defaults to replica. I think @shemau is going to make the change in #513

actually its coming in #514

ocofaigh

@Aashiq-J Too many changes here - check out my comments

solutions/standard/variables.tf

ocofaigh · 2024-11-06T17:13:09Z

solutions/standard/variables.tf

+}
+
+variable "existing_backup_kms_instance_crn" {
+  description = "The CRN of a Hyper Protect Crypto Services or Key Protect instance in the same account as the PostgreSQL instance. This value is used to create an authorization policy if `skip_iam_authorization_policy` is false. If not specified, a root key is created."


why are you saying same account? We support KMS being in any account since we have the ibmcloud_kms_api_key input. I think its safe to assume all KMS will be in same account

If not specified, a root key is created.

^ This doesn't make sense here? The instance CRN is required to create the key and the auth policy. If not specified we should mention that for backup encryption, it will use the same instance specified in existing_kms_instance_crn

ocofaigh · 2024-11-06T17:13:47Z

solutions/standard/variables.tf

+
+variable "existing_backup_kms_key_crn" {
+  type        = string
+  description = "The CRN of a Hyper Protect Crypto Services or Key Protect root key to use for disk encryption. If not specified, a root key is created in the KMS instance."


description needs to be updated to say its used for backup encryption

solutions/standard/variables.tf

solutions/standard/provider.tf

…ules/terraform-ibm-icd-postgresql into backup-kms

Aashiq-J · 2024-11-07T08:45:41Z

/run pipeline

Aashiq-J · 2024-11-07T09:02:57Z

/run pipeline

Aashiq-J · 2024-11-07T09:06:04Z

/run pipeline

Aashiq-J · 2024-11-07T09:16:53Z

/run pipeline

Aashiq-J · 2024-11-07T09:56:37Z

/run pipeline

ocofaigh · 2024-11-07T09:57:50Z

@Aashiq-J Hold off running pipeline - I'm reviewing..

ocofaigh

see suggestions

ocofaigh · 2024-11-07T09:59:17Z

solutions/standard/variables.tf

+}
+
+variable "existing_backup_kms_instance_crn" {
+  description = "Optional. The CRN of a Hyper Protect Crypto Services or Key Protect that is used to create keys for encrypting the PostgreSQL instance backup. If no value is set for `existing_backup_kms_instance_crn` and `existing_backup_kms_key_crn`, it will use the same instance specified in `existing_kms_instance_crn` or the same key CRN specified in `existing_kms_key_crn`. BYOK for backups is available only in US regions `us-south` and `us-east`, and `eu-de`. [Learn more](https://cloud.ibm.com/docs/cloud-databases?topic=cloud-databases-key-protect&interface=ui#key-byok)"


Its not a "PostgreSQL instance backup" - its a database backup.
How about this for wording:

The CRN of an Hyper Protect Crypto Services or Key Protect instance that you want to use to encrypt database backups. If no value is passed, the value of the existing_kms_instance_crn input will be used, however backup encryption is only supported in certain regions so you need to ensure the KMS for backup is coming from one of the supported regions. Learn more.

ocofaigh · 2024-11-07T10:01:43Z

solutions/standard/variables.tf

+##############################################################
+variable "existing_backup_kms_key_crn" {
+  type        = string
+  description = "Optional. The CRN of a Hyper Protect Crypto Services or Key Protect root key to use for backup encryption. If no value is set for `existing_backup_kms_instance_crn` and `existing_backup_kms_key_crn`, it will use the same instance specified in `existing_kms_instance_crn` or the same key CRN specified in `existing_kms_key_crn`. BYOK for backups is available only in US regions `us-south` and `us-east`, and `eu-de`. [Learn more](https://cloud.ibm.com/docs/cloud-databases?topic=cloud-databases-key-protect&interface=ui#key-byok)"


The CRN of an Hyper Protect Crypto Services or Key Protect encryption key that you want to use to encrypt database backups. If no value is passed, the value of existing_kms_key_crn is used. If no is passed for that, a new key will be created in the provided KMS instance and used for both disk encryption, and backup encryption.

ocofaigh · 2024-11-07T10:05:53Z

solutions/standard/variables.tf


 variable "existing_kms_instance_crn" {
-  description = "The CRN of a Hyper Protect Crypto Services or Key Protect instance in the same account as the PostgreSQL instance. This value is used to create an authorization policy if `skip_iam_authorization_policy` is false. If not specified, a root key is created."
+  description = "The CRN of a Hyper Protect Crypto Services or Key Protect that is used to create keys for encrypting the PostgreSQL instance disks. If you are not using an existing KMS root key, you must specify this CRN. If you are using an existing KMS root key and auth policy is not set for PostgreSQL to KMS, you must specify this CRN."


The CRN of an Hyper Protect Crypto Services or Key Protect instance that you want to use for both disk and backup encryption. Backup encryption is only supported is some regions (learn more), so if you need to use a different instance for backup encryption from a supported region, use the existing_backup_kms_instance_crn input.

ocofaigh · 2024-11-07T10:08:48Z

solutions/standard/variables.tf

 variable "existing_kms_key_crn" {
  type        = string
-  description = "The CRN of a Hyper Protect Crypto Services or Key Protect root key to use for disk encryption. If not specified, a root key is created in the KMS instance."
+  description = "The CRN of a Hyper Protect Crypto Services or Key Protect root key to use for disk encryption. To create a key ring and key, pass a value for the `existing_kms_instance_crn` input variable."


The CRN of an Hyper Protect Crypto Services or Key Protect encryption key that you want to use to use for both disk and backup encryption. If no value is passed, a new key ring and key will be created in the instance provided in the existing_kms_instance_crn input. Backup encryption is only supported is some regions (learn more), so if you need to use a key from a different region for backup encryption, use the existing_backup_kms_key_crn input.

…form-ibm-icd-postgresql into backup-kms

ocofaigh · 2024-11-07T12:45:19Z

/run pipeline

Aashiq-J · 2024-11-07T14:18:11Z

/run pipeline

terraform-ibm-modules-ops · 2024-11-07T15:08:57Z

🎉 This PR is included in version 3.18.0 🎉

The release is available on:

GitHub release
v3.18.0

Your semantic-release bot 📦🚀

Aashiq-J added 2 commits November 6, 2024 09:40

feat: add support for backup-kms

5554535

udpate

e1f09e1

Aashiq-J requested review from Ak-sky and rajatagarwal-ibm as code owners November 6, 2024 09:50

Merge branch 'main' into backup-kms

8f6e9b6

change wal_level type

275c209

Aashiq-J requested review from Ak-sky, ocofaigh, rajatagarwal-ibm and shemau and removed request for Ak-sky and rajatagarwal-ibm November 6, 2024 13:35

fix: remove wal_level default values and validation

a475c71

ocofaigh reviewed Nov 6, 2024

View reviewed changes

Aashiq-J and others added 2 commits November 6, 2024 20:50

revert

51dad9c

SKIP UPGRADE TEST

0007363

ocofaigh requested changes Nov 6, 2024

View reviewed changes

Merge branch 'skip-wal-level' of https://github.com/terraform-ibm-mod…

f37bd49

…ules/terraform-ibm-icd-postgresql into backup-kms

Aashiq-J mentioned this pull request Nov 7, 2024

Expose the ability to provision from a backup CRN in the postgresql DA #510

Closed

review changes

9920a2f

Merge branch 'main' into backup-kms

5af863b

update catalog

6e3550d

update description

5e74750

Aashiq-J requested a review from ocofaigh November 7, 2024 09:24

update description

4bb7756

ocofaigh requested changes Nov 7, 2024

View reviewed changes

Aashiq-J added 2 commits November 7, 2024 16:40

Merge branch 'main' of https://github.com/terraform-ibm-modules/terra…

f426504

…form-ibm-icd-postgresql into backup-kms

update description

f2db8e6

ocofaigh approved these changes Nov 7, 2024

View reviewed changes

Merge branch 'main' into backup-kms

a987965

ocofaigh merged commit 68cd6c7 into main Nov 7, 2024
2 checks passed

ocofaigh deleted the backup-kms branch November 7, 2024 15:08

terraform-ibm-modules-ops added the released label Nov 7, 2024

feat: add support to use a different KMS key for backup encryption #511

feat: add support to use a different KMS key for backup encryption #511

Conversation

Aashiq-J commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Uh oh!

Aashiq-J commented Nov 6, 2024

Uh oh!

Aashiq-J commented Nov 6, 2024

Uh oh!

ocofaigh commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aashiq-J commented Nov 7, 2024

Uh oh!

Aashiq-J commented Nov 7, 2024

Uh oh!

Aashiq-J commented Nov 7, 2024

Uh oh!

Aashiq-J commented Nov 7, 2024

Uh oh!

Aashiq-J commented Nov 7, 2024

Uh oh!

ocofaigh commented Nov 7, 2024

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocofaigh commented Nov 7, 2024

Uh oh!

Aashiq-J commented Nov 7, 2024

Uh oh!

Uh oh!

terraform-ibm-modules-ops commented Nov 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Aashiq-J commented Nov 6, 2024 •

edited

Loading

ocofaigh commented Nov 6, 2024 •

edited

Loading