-
Notifications
You must be signed in to change notification settings - Fork 1.8k
add google_kms_crypto_key_version_latest
#11381
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 google_kms_crypto_key_version_latest
#11381
Conversation
Tests analyticsTotal tests: 38 Click here to see the affected service packages
View the build log |
data_google_kms_crypto_key_version_latest
google_kms_crypto_key_version_latest
Tests analyticsTotal tests: 39 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Tests analyticsTotal tests: 39 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Tests analyticsTotal tests: 39 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Tests analyticsTotal tests: 39 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
/gcbrun |
Tests analyticsTotal tests: 39 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
looks like in an attempt to add a new |
@slevenick, I'm out most of this week. Feel free to pass this back to me on Monday if it's not resolved by then. I want to make sure I don't hold this up with my schedule. |
What's the use case for this resource? It looks like most resources that take a version can use "latest" as a special key to get the latest version |
Tests analyticsTotal tests: 39 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
@slevenick This specific data source takes in a number for the version field. Their currently is no way to get the latest cryptoKeyVersion without using a separate curl resource. (base) ┌─(~/Dev/Scratch/latest_version_kms_crypto_version)───────────────────────────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s116)─┐
└─(10:37:48)──> envchain GCLOUD terraform apply ──(Wed,Aug07)─┘
╷
│ Error: Incorrect attribute value type
│
│ on main.tf line 23, in data "google_kms_crypto_key_version" "latest":
│ 23: version = "latest"
│
│ Inappropriate value for attribute "version": a number is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test is failing
} | ||
|
||
func flattenKmsCryptoKeyVersionLatest(versionsList []interface{}, d *schema.ResourceData, config *transport_tpg.Config, cryptoKeyId string) (interface{}, error) { | ||
latestVersion := versionsList[len(versionsList)-1].(map[string]interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this always work? If the last key has been disabled or deleted does it show up here, or what would be considered "latest"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And can you add a test for such a case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When marking versions as Disabled
they are still present in the list but are not available through the API, the following error appears which is also the result of the failing vcr test (will be destroyed is treated the same as a disabled
state):
tf output
:
Error: googleapi: Error 400: projects/hc-terraform-testing/locations/us-central1/keyRings/mau-latest-test/cryptoKeys/mau-crypto-key-test/cryptoKeyVersions/4 is not enabled, current state is: DESTROY_SCHEDULED.
The version list in gcloud console:
So although it's marked as disabled
it will still show up as the latest version. I'll look into adding a filter in order to get versions that have the state as enabled
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon working on this more, I'm starting to believe that having a data source where it's only purpose is to grab the latest version seems like not the best option especially when we consider that their is currently no existing cryptoKeyVersions.list
data source.
This realization comes from the fact that I would also need to implement pageTokens
to cover the case of a cryptoKey containing many versions where it requires pages.
It would be best to actually just focus on a plural cryptoKeyVersions
data source where a boolean field can be added that grabs you the latest version.
example:
data "google_kms_crypto_key_versions" "plural" {
crypto_key = "mau-crypto-latest-test"
latest_version = true
}
@slevenick let me know your thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, google_kms_crypto_key_versions implies that it returns a collection of versions, and latest_version making that only return 1 seems unusual. Maybe the google_kms_crypto_key_versions could have a latest_version
field that contains the latest version though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after a conversation with @SarahFrench through slack, we agreed that the best approach would be to first add support for a plural crypto_key_versions
data source.
From their support for a singular crypo_key_version_latest
data source can be then worked on where it uses the same logic from the plural data source for getting the latest version from a paginated list.
I'll be closing this and opening a PR for the plural one first. Thanks for your initial feedback on this! @slevenick
return fmt.Errorf("Error setting CryptoKeyVersion public key: %s", err) | ||
} | ||
} | ||
d.SetId(fmt.Sprintf("//cloudkms.googleapis.com/v1/%s/cryptoKeyVersions/%d", d.Get("crypto_key"), d.Get("version"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use this format for the ID? A more common format would not be prefixed with //cloudkms...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was based off of how it's setup in data_source_google_kms_crypto_key_version.go
:
magic-modules/mmv1/third_party/terraform/services/kms/data_source_google_kms_crypto_key_version.go
Lines 148 to 154 in fdf0c08
if err := d.Set("public_key", flattenKmsCryptoKeyVersionPublicKey(res, d)); err != nil { | |
return fmt.Errorf("Error setting CryptoKeyVersion public key: %s", err) | |
} | |
} | |
d.SetId(fmt.Sprintf("//cloudkms.googleapis.com/v1/%s/cryptoKeyVersions/%d", d.Get("crypto_key"), d.Get("version"))) | |
return nil |
Tests analyticsTotal tests: 39 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
Currently their is no way for users to grab the latest crypto key version without needing to use a separate resource that can perform a get of
cryptoKeyVersions
in order to grab the latest version from a latest.This PR addes a
crypto_key_version
data source that takes in acrypto_key
id in order to output the latest cryptoVersion for the specified crypto_keysimple tfconfig
:The
GET
request of cryptoVersions:The output from the tfconfig confirms the latest version by outputting version 4 which can be found in the GET request above:
Release Note Template for Downstream PRs (will be copied)