pulumi / pulumi-google-native

Apache License 2.0
70 stars 18 forks source link

cloudkms.v1.CryptoKey - how to set a rotationPeriod without every deployment updating the key #237

Open ColeSiegelTR opened 2 years ago

ColeSiegelTR commented 2 years ago

When creating a CryptoKey with rotationPeriod set, it is required to specify the nextRotationTime

const bucketKey = new gcp.cloudkms.v1.CryptoKey('bucket_key', {
    keyRingId: config.kms.keyRing,
    location: config.kms.region,
    purpose: "ENCRYPT_DECRYPT",
    project: config.project.id,
    rotationPeriod: config.kms.rotationPeriod,
    nextRotationTime: new Date().toDateString(),
    cryptoKeyId: config.buckets.extract.kms
}, {
    dependsOn: keyRing
});

However, the initial rotation time needs to be dynamically determined based on the Date/rotationPeriod, while subsequent rotations performed by GCP automatically update the nextRotationTime.

The problem is that the Pulumi resource will store the nextRotationTime and attempt to update it on subsequent deployments, it seems like this value shouldn't be tracked by Pulumi? Any suggestions?

Previewing update (a206447_data-lake-idt-content-expedia-dev):
     Type                                    Name                                                                             Plan        Info
     pulumi:pulumi:Stack                     a206447_data-lake-idt-content-expedia-a206447_data-lake-idt-content-expedia-dev              
 +-  └─ google-native:cloudkms/v1:CryptoKey  bucket_key                                                                       replace     [diff: ~nextRotationTime]
ColeSiegelTR commented 2 years ago

https://cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings.cryptoKeys#CryptoKey

mikhailshilkov commented 2 years ago

@ColeSiegelTR Could you please try the following options?

  1. Remove the nextRotationTime after the initial update. Will an undefined value work for subsequent updates?

  2. Include nextRotationTime to the ignoreChanges option. Then, you would also have to refresh the state before each update. Refresh will pull the latest nextRotationTime value and use it in the update payload.

I'm not yet sure how to solve this on the provider level - but if (1) or (2) works that can inform our thinking here.

ColeSiegelTR commented 2 years ago

Hi @mikhailshilkov

Sorry for the delay in my response. Your option #2 adding the nextRotationTime to ignoreChanges worked really well (maybe it should be default behavior for this property?). I didn't realize that was an option, thank you for sharing. I didn't follow what you meant about needing to run refresh, would that be just if I care about referencing the current nextRotationTime? Or will I need to run refresh in order to avoid breaking anything in the next PATCH request even for unrelated properties? I think because PATCH requires update masks it should be safe to just ignore this property entirely once it is managed by GCP.

Also in case it helps, I also tried #1 - after commenting out nextRotationTime an update was still triggered:

colesiegel@TR-C02VD7EGHTD6 repro % pulumi up -v 1
Previewing update (test-bug-repro):
     Type                                    Name                      Plan        Info
     pulumi:pulumi:Stack                     bug-repro-test-bug-repro              
 +-  └─ google-native:cloudkms/v1:CryptoKey  bucket_key                replace     [diff: -nextRotationTime]

Resources:
    +-1 to replace
    1 unchanged

Do you want to perform this update? yes
Updating (test-bug-repro):
     Type                                    Name                      Status                   Info
     pulumi:pulumi:Stack                     bug-repro-test-bug-repro  **failed**               1 error
 +-  └─ google-native:cloudkms/v1:CryptoKey  bucket_key                **replacing failed**     [diff: -nextRotationTime]; 1 error

Diagnostics:
  pulumi:pulumi:Stack (bug-repro-test-bug-repro):
    error: update failed

  google-native:cloudkms/v1:CryptoKey (bucket_key):
    error: error sending request: googleapi: Error 409: CryptoKey projects/projectidxxx/locations/us-east4/keyRings/a206447-us-east4-keyring-test6/cryptoKeys/test-key-id123 already exists.: "https://cloudkms.googleapis.com/v1/projects/projectidxxx/locations/us-east4/keyRings/a206447-us-east4-keyring-test6/cryptoKeys?cryptoKeyId=test-key-id123" map[cryptoKeyId:test-key-id123 keyRingId:a206447-us-east4-keyring-test6 location:us-east4 project:projectidxxx purpose:ENCRYPT_DECRYPT rotationPeriod:86400s]

Resources:
    +-1 replaced
    1 unchanged
lblackstone commented 2 years ago

I wanted to note that the update is fixed now with #386. I agree that the nextRotationTime should be an internal detail that the user doesn't have to worry about, so I'll leave this issue open to track that improvement.

stefanhenseler commented 1 year ago

@lblackstone Running into the same issue.

  const key = new google-native.cloudkms.v1.CryptoKey("database-key", {
            project: args.project,
            location: args.region,
            versionTemplate: {
                algorithm: gcp.cloudkms.v1.CryptoKeyVersionTemplateAlgorithm.GoogleSymmetricEncryption,
                protectionLevel: gcp.cloudkms.v1.CryptoKeyVersionTemplateProtectionLevel.Hsm,
            },
            purpose: gcp.cloudkms.v1.CryptoKeyPurpose.EncryptDecrypt,
            keyRingId: keyRing.keyRingId,
            rotationPeriod: "7890000s", // 3 months
            nextRotationTime: new Date(Date.now() + 86400 * 1000).toISOString(),
        }, { parent: this });

This causes the nextRotationTime to be updated every time I run an apply, and I don't want this because we use the pulumi-operator to apply this program. The current behavior would move the rotation time up during every reconciliation, so I guess it would never rotate :). I don't quite get the reason for this property anyway because It's already implied when the key should be rotated via the rotationPeriod. Looking at the cli example here: https://cloud.google.com/kms/docs/rotating-keys#automatic

NEXT_ROTATION_TIME: the timestamp at which to complete the first rotation, for example "2023-01-01T01:02:03". You can omit --next-rotation-time to schedule the first rotation for 7 days from when you run the command. For more information, see [CryptoKey.nextRotationTime](https://cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings.cryptoKeys#CryptoKey.FIELDS.next_rotation_time). I guess the behavior should be that this property defaults to 7 Days from now and is dropped for every update.