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

Fix bucket name in policy statements, fixing #81 #82

Merged
merged 11 commits into from
May 3, 2024

Conversation

benjamin-hg
Copy link
Contributor

@benjamin-hg benjamin-hg commented Apr 26, 2024

what

  • Limit generated S3 bucket name to 63 characters

why

  • AWS imposed limit, derived from DNS limits, allowing S3 buckets to be accessed by name via DNS

references

…ce" when used as part of terraform-aws-elastic-beanstalk-environment cloudposse#81
@benjamin-hg benjamin-hg requested review from a team as code owners April 26, 2024 12:19
@mergify mergify bot added the triage Needs triage label Apr 26, 2024
@benjamin-hg
Copy link
Contributor Author

benjamin-hg commented Apr 26, 2024

After rm -rf .terraform/modules and terraform init, my deployment actually fails with this patch applied. Seems like I was not working with the defined modules versions.

I'm using cloudposse/elastic-beanstalk-environment/aws v0.51.2 and it still fails.

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@nitrocode
Copy link
Member

/test-terratest

main.tf Show resolved Hide resolved
@nitrocode
Copy link
Member

/test terratest

@nitrocode
Copy link
Member

/terratest

@nitrocode nitrocode added the patch A minor, backward compatible change label Apr 28, 2024
@nitrocode nitrocode enabled auto-merge (squash) April 28, 2024 19:19
auto-merge was automatically disabled April 29, 2024 08:13

Head branch was pushed to by a user without write access

@benjamin-hg
Copy link
Contributor Author

@nitrocode I guess terratest failed because the test does not set the bucket name and hence the module was disabled. Hope fb07687 fixes the issue

@nitrocode
Copy link
Member

@benjamin-hg please

  1. revert fb07687
  2. copy the same variable definition of bucket_name from the upstream module to
    variable "bucket_name" {
    type = string
    default = null
    description = "Bucket name. If provided, the bucket will be created with this name instead of generating the name from the context"
    }

@benjamin-hg
Copy link
Contributor Author

benjamin-hg commented Apr 30, 2024

@benjamin-hg please

  1. revert fb07687
  2. copy the same variable definition of bucket_name from the upstream module to
    variable "bucket_name" {
    type = string
    default = null
    description = "Bucket name. If provided, the bucket will be created with this name instead of generating the name from the context"
    }

@nitrocode I'm sorry but I didn't get it. 😬

  1. Before fb07687, terratest was failing because the name of the bucket was empty in the resource of the policy statement. Setting the bucket_name module to enabled by default (if bucket_name variable is not set) should solve the issue that the bucket name taken from the bucket_name module is empty because the module is not enabled. Why do you think this can be reverted?

  2. The variable definition is already there:
    https://github.com/benjamin-hg/terraform-aws-lb-s3-bucket/blob/18bed393e11e76b4346893f1f264ebe883abca2c/variables.tf#L7-L11 so copying it over would not cause any change.

@nitrocode
Copy link
Member

Hmm I'm unsure here but it seems like changing the false to true may maintain the bucket name null label (via the enabled flag) if the var.bucket_name = null which seems wrong.

Let me cc others in this PR @kevcube @RoseSecurity

@benjamin-hg you may also want to post in sweetops slack community's #pr-reviews for more visibility.

milldr
milldr previously approved these changes May 3, 2024
@milldr
Copy link
Member

milldr commented May 3, 2024

/terratest

@mergify mergify bot removed the triage Needs triage label May 3, 2024
@Nuru Nuru added bugfix Change that restores intended behavior and removed patch A minor, backward compatible change labels May 3, 2024
@Nuru
Copy link
Contributor

Nuru commented May 3, 2024

/terratest

@Nuru Nuru self-requested a review May 3, 2024 18:34
@Nuru Nuru merged commit a642b87 into cloudposse:main May 3, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Change that restores intended behavior
Projects
None yet
4 participants