-
Notifications
You must be signed in to change notification settings - Fork 13
Support for ingesting KSM crypto key rings and crypto keys #31
Conversation
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.
Just a few small notes.
@@ -94,6 +96,7 @@ The following relationships are created/mapped: | |||
| `google_compute_subnetwork` | **HAS** | `google_compute_instance` | | |||
| `google_iam_service_account` | **ASSIGNED** | `google_iam_role` | | |||
| `google_iam_service_account` | **HAS** | `google_iam_service_account_key` | | |||
| `google_kms_key_ring` | **HAS** | `google_kms_crypto_key` | |
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.
Is there any relationship that should be created between a higher-level account or service and google_kms_key_ring
?
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.
There is no relationship between service account and crypto key/ring. Service account keys are different. We have a google_iam_service_account_key
for that. We should consider creating the relationship with the account, but I'll tackle this separately.
@@ -0,0 +1,10 @@ | |||
export const STEP_CLOUD_KMS_KEY_RINGS = 'fetch-kms-key-rings'; |
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.
Same question as I posed in your network resources PR - just wondering if it'd be nice to start moving in the direction of
const Entities = {
KMS_KEY_RING: {
resourceName: 'KMS Key Ring',
_type: 'google_kms_key_ring',
_class: 'Vault',
},
KMS_CRYPTO_KEY: {
resourceName: 'KMS Crypto Key',
_type: 'google_kms_crypto_key',
_class: ['Key', 'CryptoKey'],
},
}
Your call. Obviously this would some inconsistencies with the rest of the services you've already implemented.
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.
Thanks. I plan to do this in bulk separately.
11410e3
to
533394e
Compare
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.
Thanks for responding to my comments. Looks good.
NOTE: This branches off of
initial-networking