Implement direct controller for KMSCryptoKey#9712
Conversation
This patch: 1. Implements the direct controller for KMSCryptoKey under `pkg/controller/direct/kms/`. 2. Resolves and normalizes KeyRingRef and other spec fields. 3. Syncs KRM metadata labels to/from GCP labels. 4. Correctly manages KMS Delete semantics by listing/destroying all key versions and disabling automatic rotation. 5. Updates mockgcp's KMS server to support rotation period updates. 6. Records golden E2E test results for KMSCryptoKey. Fixes GoogleCloudPlatform#9711
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| desired := KMSCryptoKeySpec_ToProto(mapCtx, &a.desired.Spec) | ||
| if mapCtx.Err() != nil { | ||
| return mapCtx.Err() | ||
| } | ||
| desired.Labels = label.NewGCPLabelsFromK8sLabels(a.desired.GetObjectMeta().GetLabels()) |
There was a problem hiding this comment.
We should only call KMSCryptoKeySpec_ToProto (or equivalent) in AdapterForObject, and we should store desired as a proto - the same foramt as actual. Please update the skill to emphasize this.
| } | ||
| desired.Labels = label.NewGCPLabelsFromK8sLabels(a.desired.GetObjectMeta().GetLabels()) | ||
|
|
||
| skipInitialVersionCreation := false |
There was a problem hiding this comment.
I'm assuming this is why we deviated from calling KMSCryptoKeySpec_ToProto in AdapterForObject. For fields like this that are not represent in the API, let's store those separately in kmsCryptoKeyAdapter, maybe in this case desiredSkipInitialVersionCreation would be clearest
| parent := &krm.KMSKeyRingIdentity{ | ||
| Project: a.id.Project, | ||
| Location: a.id.Location, | ||
| Keyring: a.id.KeyRing, | ||
| } |
There was a problem hiding this comment.
Better to create a ParentString() method on the identity type (KMSCryptoKeyIdentity). Please update the skill if that's not clear.
| it := a.gcpClient.ListCryptoKeyVersions(ctx, &kmspb.ListCryptoKeyVersionsRequest{Parent: a.id.String()}) | ||
| for { | ||
| version, err := it.Next() | ||
| if err == iterator.Done { |
There was a problem hiding this comment.
Linter has flagged this: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
| > { | ||
| > "error": { | ||
| > "code": 500, | ||
| > "message": "field \"destroyScheduledDuration\" is not yet handled in mock" |
There was a problem hiding this comment.
Looks like we need to update the mock to cover this
2 participants
Implement the direct controller and record/verify E2E fixtures for KMSCryptoKey.
Changes:
pkg/controller/direct/kms/cryptokey_controller.go.mockgcpKMS mock to handlerotation_periodupdate requests inUpdateCryptoKey.SupportedControllerslist ofstatic_config.go.Fixes #9711