Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CMEK support in Firebase CLI for Firestore databases #7479

Merged
merged 11 commits into from
Jul 29, 2024

Conversation

jinyangtang
Copy link
Contributor

@jinyangtang jinyangtang commented Jul 22, 2024

Description

This lets Firebase CLI create a CMEK (Customer-managed encryption keys) Firestore database and view the key associated with a CMEK database.

  • Create can take a --kms-key-name option to create a CMEK database. (No change for Update because we currently don't support update a CMEK database's key)
  • Get displays Kms Key Name and Active Key Versions for CMEK databases.

Scenarios Tested

  • Creating a new database with no options, non-CMEK database is created by default
  • Creating a new database with kms-key-name provided, a CMEK database is created
  • Fail to create a database because --kms-key-name is misspelled
  • Fail to create a database because --kms-key-name is provided with an empty value
  • Fail to create a database because an invalid key name is provided in --kms-key-name option
  • Fail to create a database because user didn't grant correct key permission to Firestore P4SA
  • Get a CMEK database, it shows correct key and in-use key versions in Kms Key Name and Active Key Version rows
  • Get a CMEK database, it shows only Kms Key Name when Active Key Version is not available from backend yet right after database creation (it needs 10min to populate)
  • Get a non-CMEK database, it shows no Kms Key Name and Active Key Version rows

Sample Commands

  • firebase firetore:databases:create <database> # as before, to create a non-CMEK database
  • firebase firetore:databases:create <database> --kms-key-name <key>

Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

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

LGTM with some small suggestions

@@ -25,6 +25,10 @@ export const command = new Command("firestore:databases:create <database>")
"--point-in-time-recovery <enablement>",
"Whether to enable the PITR feature on this database, for example 'ENABLED' or 'DISABLED'. Default is 'DISABLED'",
)
.option(
"--kms-key-name <kmsKeyName>",
"The resource ID of a Cloud KMS key. If set, the database created will be a Customer-managed Encryption Key (CMEK) database encrypted with this key. This feature is allowlist only in initial launch.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a TODO with a buganizer link to clean this message up once its open to all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, have added.

const databaseResp: types.DatabaseResp = await api.createDatabase(
options.project,
database,
options.location,
type,
deleteProtectionState,
pointInTimeRecoveryEnablement,
cmekConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: Create database has 7 args - consider switching this to a parameter object (ie https://go/tott/550)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have switched to a param object, thanks

@joehan
Copy link
Contributor

joehan commented Jul 26, 2024

Could you add a CHANGELOG entry for this?

@jinyangtang jinyangtang merged commit dd32d0c into master Jul 29, 2024
41 checks passed
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.

2 participants