Add option to encrypt Firehose Streams with customer managed keys

[StevenKGER]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #11721

Release note for CHANGELOG:

resource_aws_kinesis_firehose_delivery_stream: add customer managed key option to server-side encryption

Output from acceptance testing:

=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSE
=== PAUSE TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSE
=== CONT  TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSE
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSE (359.61s)

[suryatejaswi89]

Is this feature now available to use ?

[StevenKGER]

Hi @suryatejaswi89,
well, the pull request is still open and I'm waiting for feedback/approval as well. However, bflad changed some labels and I hope, there will be any progress regarding this PR as soon as possible.

Given the fact, that there are other 631 PRs as well, it can unfortunately take a long time for another action to happen.

[bflad]

Hi @StevenKGER 👋 Thank you for submitting this. Please see the initial feedback below and let us know if you have questions or do not have time to implement the items.

[bflad]

For a few reasons, we would prefer to not create a function like this:

• Different terminology used across the Terraform Provider ecosystem (we would potentially refer to this as "getting the children attributes of the first configuration block")
• Prevalence of the opposite pattern of this function, where the length/nil checking is done per configuration block handling function (changing this should be discussed in a proposal issue)
• Higher potential for nil dereference panics since this function can return nil and even in the refactored usage above this is not checked

[StevenKGER]

I understand your point, but I'm not quite sure, how to resolve this problem. The easiest thing would be to undo the changes and copy the code to the functions, if required. But this is of course not very beautiful to read and handle, since it would be repeating code then.
Since the issue is just for adding CMK support and not changing the structure of the file to save some code, I wouldn't recommend to make this change even more difficult to understand (like already refactoring this is a change, which is not really understandable when reading the issue's title).

Do you have any idea how to change this or should this be part of another issue?

[bflad]

Can you please explain the changes below? The additional isKinesisFirehoseDeliveryStreamOptionDisabled(o) and !(oldOptions["kms_key_type"] == firehose.KeyTypeAwsOwnedCmk && newOptions["kms_key_type"] == firehose.KeyTypeAwsOwnedCmk) checks seem extraneous. We should instead return the API errors or add ForceNew: true to a schema attribute if necessary.

[StevenKGER]

Well - the change was created some months ago and I also don't see the reason why to use it for now as well. I tested it manually as well as automated and it works with reverted changes. Feel free, to resolve this conversation, if anything is fine.

[bflad]

Hi @StevenKGER 👋 Thank you for those updates -- I have merged this in with only some minor changes to ensure that existing configurations only having enabled = true would continue to work as expected. 👍

Output from acceptance testing (failure unrelated and present on main branch):

--- FAIL: TestAccAWSKinesisFirehoseDeliveryStream_basic (137.23s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_disappears (115.02s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchConfigEndpointUpdates (762.27s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchConfigUpdates (774.08s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchWithVpcConfigUpdates (1484.58s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Deserializer_Update (123.54s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Enabled (158.39s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_HiveJsonSerDe_Empty (102.83s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OpenXJsonSerDe_Empty (101.94s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OrcSerDe_Empty (129.27s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_ParquetSerDe_Empty (119.56s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Serializer_Update (129.94s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_ErrorOutputPrefix (112.55s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_ExternalUpdate (124.46s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_KinesisStreamSource (93.87s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_ProcessingConfiguration_Empty (100.64s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3basic (118.87s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3KmsKeyArn (123.20s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3Updates (157.94s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_missingProcessingConfiguration (99.87s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_RedshiftConfigUpdates (419.08s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basic (102.16s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSE (242.21s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSEAndKeyArn (228.19s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithSSEAndKeyType (238.52s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basicWithTags (132.87s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3ConfigUpdates (186.91s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3KinesisStreamSource (95.29s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3WithCloudwatchLogging (102.04s)
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_SplunkConfigUpdates (151.72s)

[ghost]

This has been released in version 3.7.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!