feat: add settable retention period and encryption to Stream - #89
michaelhtm wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelhtm The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
878be2b to
8440de9
Compare
Retention period and KMS encryption are not part of CreateStream, so they are exposed as settable Spec inputs (desiredRetentionPeriodHours, desiredEncryptionType, encryptionKeyARN) applied via Increase/Decrease StreamRetentionPeriod and Start/StopStreamEncryption in the update path. The observed values remain in Status (retentionPeriodHours, encryptionType, keyID). A KMS key ARN is required so the observed key matches the desired value and the stream is not re-encrypted on every reconcile. Resolves aws-controllers-k8s/community#2420
8440de9 to
5016638
Compare
|
/label release/minor |
ack-prow
Bot
added
the
release/minor
| # DesiredEncryptionType is KMS. A key *ARN* is required (aliases are | ||
| # rejected in the update hook) so the observed Status.KeyID matches the | ||
| # desired value and the controller does not re-encrypt on every reconcile. | ||
| EncryptionKeyARN: |
There was a problem hiding this comment.
Should we label this as KMS key reference?
| return ackerr.NewTerminalError(fmt.Errorf( | ||
| "encryptionKeyARN is required when desiredEncryptionType is KMS")) | ||
| } | ||
| if !strings.HasPrefix(*desired.ko.Spec.EncryptionKeyARN, "arn:") { |
There was a problem hiding this comment.
Q: What's the reason for performing this check in the controller instead of letting the AWS API perform the validation?
There was a problem hiding this comment.
Also if EncryptionKeyARN should in fact not be an ARN it seems like we should change the name to EncryptionKeyID.
There was a problem hiding this comment.
EncryptionKeyARN needs to be in ARN format. Kinesis accepts EncryptionKeyID/Alias, but describe returns the ARN. This will cause a continuous drift. As a fix, we instead just validate the user will pass the kmsARN at all times
| *desired.ko.Spec.DesiredEncryptionType == string(svcsdktypes.EncryptionTypeKms) { | ||
| if desired.ko.Spec.EncryptionKeyARN == nil { | ||
| return ackerr.NewTerminalError(fmt.Errorf( | ||
| "encryptionKeyARN is required when desiredEncryptionType is KMS")) |
There was a problem hiding this comment.
Q: Does Kinesis setup a default KMS key that the encryption type could apply to or is the KMS key always set by the user?
|
|
||
|
|
||
| @dataclass | ||
| class KMS(Bootstrappable): |
There was a problem hiding this comment.
Any reason we aren't using the KMS Key Bootstrappable resource already in ack-test?
|
|
||
| stream.wait_until_deleted(stream_name) | ||
|
|
||
| def test_retention_and_encryption(self): |
There was a problem hiding this comment.
In addition to checking that update reaches AWS it would be good to validate that the AWS resource returns to a synced state after each spec change.
|
Would be pretty great to get this in the next release if possible. Kinesis in ACK is very barebones at the moment. |
|
We are also waiting on the |
|
Any chance this could get reviewed and merged? |
|
Any chance that we could get this one and #91 looked at? The Kinesis controller is pretty much unusable since it is missing almost every configuration propery of a Kinesis stream. |
5 participants
Description of changes:
Retention period and KMS encryption are not part of CreateStream, so they
are exposed as settable Spec inputs (desiredRetentionPeriodHours,
desiredEncryptionType, encryptionKeyARN) applied via Increase/Decrease
StreamRetentionPeriod and Start/StopStreamEncryption in the update path. The
observed values remain in Status (retentionPeriodHours, encryptionType,
keyID). A KMS key ARN is required so the observed key matches the desired
value and the stream is not re-encrypted on every reconcile.
Resolves aws-controllers-k8s/community#2420
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.