#terraform-aws-modules (2023-05)
Terraform Modules
Discussions related to https://github.com/terraform-aws-modules
Archive: https://archive.sweetops.com/terraform-aws-modules/
2023-05-02
Hello team, the error in the aws-flow-logs is the pending PR open at here https://github.com/cloudposse/terraform-aws-vpc-flow-logs-s3-bucket/pull/52.
Can someone from @cloudposse-team check, rebase and approve that s3-logs version upgrade? Same for the
• https://github.com/cloudposse/terraform-aws-s3-bucket
• https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn Root cause: old s3-logs dependencies to version 0.26.0 in root modules. Upgrade to 1.0.0 minimum is required. Prefered 1.1.0
@JoseF Thank you for this PR.
The recent changes to the S3 resource in the AWS provider have made me question the wisdom of continuing to provide modules such as vpc-flow-logs-s3-bucket. How would you feel if we removed the ability of that module to create and S3 bucket and required you to make the S3 bucket outside of it (e.g. via s3-log-storage) and pass in the bucket ARN?
Great. Honestly I had no idea such s3-logs were created until they start failing. Considering that an terraform-aws-s3-bucket already exists and does a perfect job.
Too much redundancy or hidden dependancies, with ARN is simpler an cleaner. Less modules to maintain. Also gives user control to update their own S3 versions.
And what about the other modules which also have this s3-logs dependency like s3-cdn?
We have limited resources and primarily update our open source modules in response to (paying) customer needs. That said, we consider the S3 failures significant, and will prioritize fixing them.
Our general plan is to remove the creation of S3 buckets from most modules and, where modules currently create S3 buckets, accept an S3 bucket ARN instead. We will probably keep s3-log-storage and s3-website as well as s3-bucket but I don’t know what else.
Totally understandable. I would contribute more but I don’t know how to gain access to PR or so.
I have not found any other way to deploy flow_logs in a iso/hipaa/soc2 compliant environment. So for me, is mandatory to use this type of components.
This is not the first time I ask about how to contribute to the project, in the past with some RDS missing cw alarms. Anyway I managed to handle this s3 issue internally for me so far. Waiting for the official version when it comes.
@JoseF You can contribute to the projects by making a pull request. Step-by-step instructions are here among other places.
Note that we are way behind in reviewing pull requests. It may be weeks or months before we get to review it. However, if it solves an urgent need or is very small, you can ask for an expedited review by posting the PR in this channel and tagging the team, as you did at the start of this thread.
Ohh man, thanks, that’s useful.
Modules terraform-aws-s3-bucket and terraform-aws-s3-log-storage have been updated to work with the new AWS S3 defaults. Other modules dependent on them should be updated soon, but if you find one not automatically updated (either no PR or the PR was not auto-merged) and you are waiting on it, ping @cloudposse-team in this channel with a request to update the module you need.
I’m working on some upgrades to terraform-aws-vpc-flow-logs-s3-bucket so it may take a couple of days before that is updated and released.
2023-05-06
2023-05-07
2023-05-08
terraform-aws-utils v1.2.0 is now available, updated to include the newest regions, and now also providing an all_regions output.
2023-05-12
I definitely appreciate the update of the s3-cdn repo just about 2 minutes ago
Hold on, those release were related to ci updates, I think the s3 patch is still waiting there
2023-05-14
Hi there, I was implementing your EKS cluster in a terragrunt based environment, as you know terragrunt forces you to be a lot more careful about inputs and outputs, and apparently the region variable is not used anywhere in the module, except for the testing/examples, into the provider. Since that means a further output/input in terragrunt (it’s unlikely a big deal with plain terraform as you don’t have to deal with each input), would makes sense to remove it from the module’s [variables.tf](http://variables.tf)? Happy to create a PR if this makes sense to you.
EDIT: On a second thought, since removing it from the variables would require also a change into the module declaration, can I at least set a default?
EDIT 2: Well, at this point I can set a default as a terragrunt input myself . Anyway that region var is unused and should be removed :)
@Jeremy G (Cloud Posse) @Dan Miller (Cloud Posse)
Yes the region variable isnt used. It’s left over from this addition, which has been significantly updated since. Please feel free to create that PR and ping me!
Wow I completely missed your answer, @Dan Miller (Cloud Posse), sorry about that! I see that the variable is still there even if clearly flagged as “OBSOLETE (not needed)”, I will create a quick patch just to keep it clean.
Do you think we should just get rid of region or it should be moved into [variables-deprecated.tf](http://variables-deprecated.tf)?
@nnsense How much of a headache is this for you? My preference would be not to make any changes at this time, and to remove the variable altogether when we have a more compelling reason to make breaking changes and release them all as v5 of the module. To proceed on this path, you can open an issue asking for the variable to be removed, so the request stays visible, and we will take care of it as part of the v5 release, whenever that happens (no current plans).
Would that be OK with you?
Why you think it’s a headache? I just thought to be helpful, if it’s ok I’m fine with it, case closed
I think @Jeremy G (Cloud Posse)’s point is just we try to reduce the number of semver major versions (e.g. breaking changes) and therefore prefer to group together breaking changes. So if it’s not a pain (aka headache) of us to ask, let’s create an issue to track this instead, and slate it for deprecation in the next major release.
Hi Erik, I was under the impression that variable was unused from a long time, so not really a breaking change. I will create an issue as you proposed asap.
However, if you remove a variable from a module, even if it’s unused within the module, it becomes a breaking change. This is because if someone was setting that variable in their Terraform code, it would cause their configuration to break. This violates the semantic versioning (“semver”) contract, which states that minor changes should not introduce breaking changes. We try to minimize major version releases, as they tend to be disruptive.
That makes sense, thanks
Haha, yea, and we haven’t always been so careful. In the past 18mo, we’ve been moving most of our modules from 0.x releases to 1x+ releases and properly honoring semver.
In 0.x, there’s no promise of stability so we could break things
If it can help, I wouldn’t misunderstand if “How much of a headache is this for you?” was at the end of your sentence. That’s likely perfectly clear in English but.. I’m not native Thanks both for your time
2023-05-15
@Dan Miller (Cloud Posse) has joined the channel
2023-05-17
:exclamation: All terraform module repos have their default branch renamed from master to main today. You will need to re-clone or:
git branch -m master main
git fetch origin
git branch -u origin/main main
git remote set-head origin -a
2023-05-26
Hello colleagues , we see when we enable a sns notification as the override variable in terraform , it getting only destroyed and not getting recreated . Any idea why ?
Terraform code :
variable "sns_endpoint" {
description = "Endpoint for SNS Endpoint"
type = string
}
resource "aws_sns_topic_subscription" "user_updates_email_target" {
topic_arn = aws_sns_topic.sap_topic.arn
protocol = "email"
endpoint = "${var.sns_endpoint}"
}
Terraform plan :
Plan: 227 to add, 8 to change, 9999 to destroy.
Changes to Outputs:
~ account_id = "092716918791" -> "144080655895"
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value: ^C