Fix a couple of checkov issues on modernisation-platform-terraform-s3-bucket

[SteveLinden]

There are a couple of issues popping up on the checkov checks. Ewa suggested we correct the issue rather than ignore the checkov check.

"This option is currently not supported in this module. The fix should be to allow to opt in for it by passing the sns topic arn." for Check: CKV2_AWS_62: "Ensure S3 buckets should have event notifications enabled" error

Environment details

modernisation-platform-terraform-s3-bucket
Also in closed PR ministryofjustice/modernisation-platform-terraform-s3-bucket#252

Application Name

Fix the checkov error.

Description of application

This will correct the issue popping up on the checkov overnight checks.

Definition of done

• error corrected
• no issues showing in the checkov checks
• clear error log

[SteveLinden]

Ignored one check that was popping up (300 - set time for failed uploads) and added some code to pick up the arn and put it in the code. Looks like this has corrected the issues but I will keep this open until the overnight run has completed. Works OK with a manual run.

[SteveLinden]

Change didn't work. May need to speak @ewastempel about to understand what she needs for this.

[ewastempel]

The goal of this ticket is to explore if it is possible to address the following checkov issue, rather than suppressing it:
#checkov:skip=CKV2_AWS_62: "Ensure S3 buckets should have event notifications enabled"

Currently the S3 tf module does not allow for s3 bucket notifications. This ticket is to see if it can be enabled by improving the module.

This can be a useful read when implementing this:
https://francescoboffa.com/s3-bucket-notifications/
https://docs.aws.amazon.com/AmazonS3/latest/userguide/EventNotifications.html
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_notification

[SteveLinden]

Various changes made and lots of issues with the multiple rows in the replication section. Many errors on the terratest part so multiple plans sent through

[SteveLinden]

After discussions we are doing something slightly different to this. It will be to stop the run if there are blank values on two variables.

Additionally, one of the failures (300) will be covered in another call #5133

[SteveLinden]

Added a test so will clear out my checks and push this to be checked.

[SteveLinden]

Further adjustments needed to the test process

[SteveLinden]

A number of changes made and pushed up to github. This is now in a ready to review status so I will post it on the channel.

[SteveLinden]

A few final changes were made to make sure the new check ran and was pushed to output. This was pushed through to main. Previous changes had accidentally been applied to main when a dependabot run was undertaken.

Will close on Monday after the checkov run is checked.

[SteveLinden]

PRs
First
and
second

[SteveLinden]

Another checkov issue popped up so this has been added to the skip list and posted to the latest PR

It was under the unit-test part which had not been tested when we worked on it. This has now been done and it is skipped.

[SteveLinden]

Latest release applied. It will be checked again tomorrow after the next run. If OK it will be closed.

[SteveLinden]

It worked!
After discussions the release will be set to latest and users on ask and update will be informed. They do not necessarily need to use it but it will be made available.