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

Upgrade to v4 aws provider #131

Merged
merged 24 commits into from
Feb 22, 2022
Merged

Upgrade to v4 aws provider #131

merged 24 commits into from
Feb 22, 2022

Conversation

nitrocode
Copy link
Member

@nitrocode nitrocode commented Feb 10, 2022

what

  • Upgrade to v4 aws provider
  • Deprecated policy in favor of source_policy_documents, closes Fix: Use list(string) for var.policy #124
  • Deprecated grants in favor of acl_grants
  • Deprecated lifecycle_rules in favor of lifecycle_configuration_rules
  • All tests pass
    • Current tests pass
    • Tests do not check website inputs. The website_inputs is unavailable in the tests.
    • Tests do not check cors. The cors_rule_inputs is available but unused.
  • Backwards compatible - Is it possible to make it backwards compatible ?
    • Generate the lifecycle rule ids ?
    • Translate the grant.permissions into separate grant.permission blocks ?
  • Migration document in this repo's Wiki

why

  • Current version incompatible with AWS provider v4.x
│ Error: Unsupported attribute
│ 
│   on .terraform-mdev/modules/s3_bucket/main.tf line 166, in resource "aws_s3_bucket" "default":
│  166:         for_each = local.s3_replication_rules == null ? [] : local.s3_replication_rules
│ 
│ This object does not have an attribute named "s3_replication_rules".
  • One upgraded to version 4.x, module no longer works with AWS provider 3.x
  • Pin to >= 4.2.0 because of relevant bug fixes needed by this module

references

notes

  • grants is now deprecated, but still supported. New input replacing it is acl_grants. The only difference is that grants takes a list of permissions and acl_grants takes a single permission. The only reason we are making grants deprecated is so that our inputs track the AWS resource inputs.
  • lifecycle_rules is now deprecated but still supported, replaced with lifecycle_configuration_rules, which is identical except that lifecycle_configuration_rules has an id for each rule, which is now required by the AWS resource (doc). This module supplies an id for lifecycle_rules.
  • aws_s3_bucket_lifecycle_configuration requires a static rules.filter element due to Bad error (message?) aws_s3_bucket_lifecycle_configuration complains about "Base level prefix" when there is none hashicorp/terraform-provider-aws#23299

@nitrocode nitrocode added the patch A minor, backward compatible change label Feb 10, 2022
@nitrocode nitrocode requested review from a team as code owners February 10, 2022 19:23
korenyoni
korenyoni previously approved these changes Feb 10, 2022
@korenyoni
Copy link
Member

/test all

@nitrocode
Copy link
Member Author

/test all

korenyoni
korenyoni previously approved these changes Feb 10, 2022
@nitrocode nitrocode removed the patch A minor, backward compatible change label Feb 10, 2022
@nitrocode nitrocode marked this pull request as draft February 10, 2022 20:35
@nitrocode nitrocode changed the title Set pin to v3 for now Upgrade to v4 aws provider Feb 10, 2022
main.tf Outdated Show resolved Hide resolved
main.tf Outdated 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 1c615f0 - Attempt to upgrade to aws v4 - 2 new errors were added

Change details

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

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
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 59b69f2 - Auto Format - 1 new error was added and 3 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_NETWORKING_52 Added /main.tf aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target_extra.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket.aws_s3_bucket.default


dynamic "versioning" {
for_each = var.versioning_enabled ? [true] : []
count = local.enabled ? 1 : 0
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 cross-region replication enabled
    Resource: module.s3_bucket_replication_target.aws_s3_bucket.default | ID: BC_AWS_GENERAL_72

How to Fix

resource "aws_s3_bucket" "test" {
  ...
+  replication_configuration {
+    role = aws_iam_role.replication.arn
+    rules {
+      id     = "foobar"
+      prefix = "foo"
+      status = "Enabled"
+
+      destination {
+        bucket        = aws_s3_bucket.destination.arn
+        storage_class = "STANDARD"
+      }
+    }
+  }
}

Description

Cross-region replication enables automatic, asynchronous copying of objects across S3 buckets. By default, replication supports copying new S3 objects after it is enabled. It is also possible to use replication to copy existing objects and clone them to a different bucket, but in order to do so, you must contact AWS Support.

Dependent Resources



Calculating...

🎉   Fixed by commit 492c82b - Make backwards compatible

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 492c82b - Make backwards compatible - 3 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target_extra.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket.aws_s3_bucket.default


dynamic "versioning" {
for_each = var.versioning_enabled ? [true] : []
count = local.enabled ? 1 : 0
Copy link

@bridgecrew bridgecrew bot Feb 21, 2022

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 cross-region replication enabled
    Resource: module.s3_bucket.aws_s3_bucket.default | ID: BC_AWS_GENERAL_72

How to Fix

resource "aws_s3_bucket" "test" {
  ...
+  replication_configuration {
+    role = aws_iam_role.replication.arn
+    rules {
+      id     = "foobar"
+      prefix = "foo"
+      status = "Enabled"
+
+      destination {
+        bucket        = aws_s3_bucket.destination.arn
+        storage_class = "STANDARD"
+      }
+    }
+  }
}

Description

Cross-region replication enables automatic, asynchronous copying of objects across S3 buckets. By default, replication supports copying new S3 objects after it is enabled. It is also possible to use replication to copy existing objects and clone them to a different bucket, but in order to do so, you must contact AWS Support.

🎉   Fixed by commit 550e49b - lifecycle filter must be static, abort... must by dynamic.


dynamic "versioning" {
for_each = var.versioning_enabled ? [true] : []
count = local.enabled ? 1 : 0
Copy link

@bridgecrew bridgecrew bot Feb 21, 2022

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 cross-region replication enabled
    Resource: module.s3_bucket_replication_target_extra.aws_s3_bucket.default | ID: BC_AWS_GENERAL_72

How to Fix

resource "aws_s3_bucket" "test" {
  ...
+  replication_configuration {
+    role = aws_iam_role.replication.arn
+    rules {
+      id     = "foobar"
+      prefix = "foo"
+      status = "Enabled"
+
+      destination {
+        bucket        = aws_s3_bucket.destination.arn
+        storage_class = "STANDARD"
+      }
+    }
+  }
}

Description

Cross-region replication enables automatic, asynchronous copying of objects across S3 buckets. By default, replication supports copying new S3 objects after it is enabled. It is also possible to use replication to copy existing objects and clone them to a different bucket, but in order to do so, you must contact AWS Support.

🎉   Fixed by commit 550e49b - lifecycle filter must be static, abort... must by dynamic.


dynamic "versioning" {
for_each = var.versioning_enabled ? [true] : []
count = local.enabled ? 1 : 0
Copy link

@bridgecrew bridgecrew bot Feb 21, 2022

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 cross-region replication enabled
    Resource: module.s3_bucket_replication_target.aws_s3_bucket.default | ID: BC_AWS_GENERAL_72

How to Fix

resource "aws_s3_bucket" "test" {
  ...
+  replication_configuration {
+    role = aws_iam_role.replication.arn
+    rules {
+      id     = "foobar"
+      prefix = "foo"
+      status = "Enabled"
+
+      destination {
+        bucket        = aws_s3_bucket.destination.arn
+        storage_class = "STANDARD"
+      }
+    }
+  }
}

Description

Cross-region replication enables automatic, asynchronous copying of objects across S3 buckets. By default, replication supports copying new S3 objects after it is enabled. It is also possible to use replication to copy existing objects and clone them to a different bucket, but in order to do so, you must contact AWS Support.

🎉   Fixed by commit 550e49b - lifecycle filter must be static, abort... must by dynamic.

@Nuru Nuru requested a review from korenyoni February 21, 2022 11:16
@Nuru
Copy link
Contributor

Nuru commented Feb 21, 2022

/test all

@@ -1,10 +1,10 @@
terraform {
required_version = ">= 0.13.0"
required_version = ">= 1.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

do we have to move this pin to be greater than 1.0? I believe we only need to worry about the aws provider

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 550e49b - lifecycle filter must be static, abort... must by dynamic. - 3 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target_extra.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket.aws_s3_bucket.default

@Nuru
Copy link
Contributor

Nuru commented Feb 22, 2022

/test all

main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
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 b3babb4 - Do not reference non-existent object member - 4 errors were fixed.

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target_extra.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket_replication_target.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf module.s3_bucket.aws_s3_bucket.default
BC_AWS_GENERAL_72 Fixed /main.tf aws_s3_bucket.default

main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
@Nuru Nuru marked this pull request as ready for review February 22, 2022 01:12
@Nuru Nuru requested a review from Benbentwo February 22, 2022 01:12
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments regarding bridgecrew

@Nuru Nuru added the enhancement New feature or request label Feb 22, 2022
@Nuru Nuru requested a review from aknysh February 22, 2022 03:28
@Nuru Nuru merged commit 03a9738 into master Feb 22, 2022
@Nuru Nuru deleted the aws-v3 branch February 22, 2022 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS 4.x Provider causes issues with this module
7 participants