Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use object lock enabled #148

Merged
merged 4 commits into from
May 6, 2022
Merged

Use object lock enabled #148

merged 4 commits into from
May 6, 2022

Conversation

nitrocode
Copy link
Member

@nitrocode nitrocode commented Apr 22, 2022

what

  • Use object lock enabled

why

  • Deprecation of dynamic object_lock_configuration for object_lock_enabled
│ Warning: Argument is deprecated
│
│   with module.bucket.aws_s3_bucket.default,
│   on .terraform/modules/bucket/main.tf line 30, in resource "aws_s3_bucket" "default":30: resource "aws_s3_bucket" "default" {
│
│ Use the top-level parameter object_lock_enabled and the aws_s3_bucket_object_lock_configuration resource instead

references

@nitrocode nitrocode added the patch A minor, backward compatible change label Apr 22, 2022
@nitrocode
Copy link
Member Author

/test all

main.tf Show resolved Hide resolved
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️   Due to 04397bf - Auto Format - 1 new error was added

Change details

Error ID Change Path Resource
BC_AWS_NETWORKING_52 Added /main.tf aws_s3_bucket.default

Gowiem
Gowiem previously approved these changes Apr 22, 2022
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I'll leave it open in case you want to get another set of eyes since this module has had some recent flux 👍

Nuru
Nuru previously requested changes Apr 23, 2022
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but before approving it, I want some extra testing, due to this note which says changing the value "Forces new resource". We want to be extra careful that we are not causing a change that could end up having Terraform destroy and replace an existing S3 bucket.

@Benbentwo
Copy link
Member

Benbentwo commented May 6, 2022

Tested with a config where object_lock_configuration enabled via:

  object_lock_configuration = {
    mode  = var.object_lock_mode_archive
    days  = var.object_lock_days_archive
    years = null
  }

This branch fixes perma drift.

Also tested changing

  object_lock_configuration =  null

This did end up planning to delete the bucket. however this is if you change configuration. For those who have it configured this fixes consistency bugs, for those who don't it won't be a problem. It only becomes a problem when attempting to switch. from enabled -> disabled.

@nitrocode nitrocode requested a review from Nuru May 6, 2022 16:57
@nitrocode nitrocode dismissed Nuru’s stale review May 6, 2022 17:08

Requesting a new review

@mergify mergify bot dismissed Gowiem’s stale review May 6, 2022 18:48

This Pull Request has been updated, so we're dismissing all reviews.

@Nuru
Copy link
Contributor

Nuru commented May 6, 2022

/test all

object_lock_enabled = "Enabled"
}
}
object_lock_enabled = local.object_lock_enabled
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW   Ensure S3 Bucket has public access blocks
    Resource: aws_s3_bucket.default | ID: BC_AWS_NETWORKING_52

How to Fix

resource "aws_s3_bucket" "bucket_good_1" {
  bucket = "bucket_good"
}

resource "aws_s3_bucket_public_access_block" "access_good_1" {
  bucket = aws_s3_bucket.bucket_good_1.id

  block_public_acls   = true
  block_public_policy = true
}

Description

When you create an S3 bucket, it is good practice to set the additional resource **aws_s3_bucket_public_access_block** to ensure the bucket is never accidentally public.

We recommend you ensure S3 bucket has public access blocks. If the public access block is not attached it defaults to False.

Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️   Due to 9557b6e - Merge branch 'master' into use-object-lock-enabled - 1 new error was added

Change details

Error ID Change Path Resource
BC_AWS_NETWORKING_52 Added /main.tf aws_s3_bucket.default

object_lock_enabled = "Enabled"
}
}
object_lock_enabled = local.object_lock_enabled
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW   Ensure S3 Bucket has public access blocks
    Resource: aws_s3_bucket.default | ID: BC_AWS_NETWORKING_52

How to Fix

resource "aws_s3_bucket" "bucket_good_1" {
  bucket = "bucket_good"
}

resource "aws_s3_bucket_public_access_block" "access_good_1" {
  bucket = aws_s3_bucket.bucket_good_1.id

  block_public_acls   = true
  block_public_policy = true
}

Description

When you create an S3 bucket, it is good practice to set the additional resource **aws_s3_bucket_public_access_block** to ensure the bucket is never accidentally public.

We recommend you ensure S3 bucket has public access blocks. If the public access block is not attached it defaults to False.

Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️   Due to 021ec90 - Update go-getter for security fix - 1 new error was added

Change details

Error ID Change Path Resource
BC_AWS_NETWORKING_52 Added /main.tf aws_s3_bucket.default

@Nuru
Copy link
Contributor

Nuru commented May 6, 2022

/test all

Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BridgeCrew complaint is a mistake on BridgeCrew's part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants