r/aws_budgets_budget: Add support for notifications

[flosell]

#1879 introduced a basic aws_budgets_budget resource.

This PR adds support for budget notifications via E-Mail or SNS.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSBudgetsBudget'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSBudgetsBudget -timeout 120m
=== RUN   TestAccAWSBudgetsBudget_basic
--- PASS: TestAccAWSBudgetsBudget_basic (124.06s)
=== RUN   TestAccAWSBudgetsBudget_prefix
--- PASS: TestAccAWSBudgetsBudget_prefix (108.59s)
=== RUN   TestAccAWSBudgetsBudget_notification
--- PASS: TestAccAWSBudgetsBudget_notification (385.83s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	618.530s

[flosell]

Just realised this PR would change the existing behaviour for people upgrading:
Before this PR any manually configured notifications would be ignored (since the provider doesn't know about them). After the PR, manually configured (i.e. without matching terraform configuration) notifications would be deleted since the provider now manages budgets and notifications.

I'd guess this is not a totally unlikely scenario: Because of the missing notifications support, my current project has a budget-resource and added notifications manually. So if we aren't very careful they will just suddenly disappear.

Is there a smart way to avoid this?

I've tried modelling notifications as a separate resource (here if you want to have a look) but that has its own limitations since the API doesn't give us an ID we could use to find the notification on read - exact match is the only way.

[akerl]

Seems like this PR is stalled?

[flosell]

My current project doesn't rely on budget notifications anymore right now so for me personally this isn't urgent anymore. I'd still be happy to work on any feedback and get this merged though.

Gentle nudge @bflad ?

[hwrdprkns]

@bflad what can we do to get some movement on this PR?

[walterheck]

I would also very much like to see this merged, as currently for instance https://registry.terraform.io/modules/devops-workflow/budgets/aws/0.1.1 has a nasty solution using

locals {
  # Prefix for single budget
  notification_cmd_prefix_single = <<EOF
aws budgets create-notification \
--account-id ${data.aws_caller_identity.current.account_id} \
--budget-name ${var.budget_name_prefix}${var.budget_name}-${title(lower(var.time_unit))} \
--subscribers ${join(" ", formatlist("SubscriptionType=EMAIL,Address=%s", var.emails))} \
--notification \
EOF

  # Prefix for multiple budgets. So only contain stuff that is common to all budgets in var.budgets
  # Could move subscribers here as long as only supporting one
  notification_cmd_prefix = <<EOF
aws budgets create-notification \
--account-id ${data.aws_caller_identity.current.account_id} --budget-name \
EOF
}

[snemetz]

I'll happily update https://registry.terraform.io/modules/devops-workflow/budgets/aws/ if this gets merged and solves the issues. That module is basically using a hack that has some issues. It was a temporary solution.

[walterheck]

@snemetz oh, I'm very happy to have your module, don't get me wrong. Your hack is the (almost) the only way to do this currently and that's horrible on the terraform part, not yours <3

[snemetz]

@walterheck No problem. I believe it could also be done by Terraform calling cloudformation, but I didn't really explore that option.

[rslotte]

Any progess on this PR? @flosell

[flosell]

@rslotte sorry, no news here.

There's one question for the maintainers regarding the upgrade behaviour, other than that this PR would be ready to merge in my opinion.

If you haven't already, add a 👍to the PR and possibly the related issues. Sometimes this helps prioritising things.

[jparten]

Any news? Anything the community can do to get this merged? @flosell

[msvechla]

This would be a great addition to the aws_budgets_budget ressource. What is still open to get this merged?

[JoostSaanen]

Need this one really hard!

[jippi]

This is the only thing in my AWS environment i had to manually configure :)

[cwyl02]

Any update on this PR? We need this feature!

[flosell]

Hey, sorry, no update: As far as I'm concerned, this PR is ready, however the upgrade path for existing codebases isn't ideal (see my comment above). If the community waiting for this has any smart ideas how to improve this, I'd be happy to continue working on this.

Other than that, the PR is only waiting for feedback from the maintainers.
Since it's by now one of the most-upvoted PRs here, maybe it's time for another gentle nudge @bflad ?

[bjoernhaeuser]

After the PR, manually configured (i.e. without matching terraform configuration) notifications would be deleted since the provider now manages budgets and notifications.

They will show up in the diff of terraform, right? Would be "good enough" for me.

[flosell]

@bjoernhaeuser Yes, it would show up in the plan like this:

  ~ aws_budgets_budget.foo
      notification.#:                                                "1" => "0"
      notification.2315001228.comparison_operator:                   "GREATER_THAN" => ""
      notification.2315001228.notification_type:                     "FORECASTED" => ""
      notification.2315001228.subscriber_email_addresses.#:          "1" => "0"
      notification.2315001228.subscriber_email_addresses.3679839588: "foo@example.com" => ""
      notification.2315001228.subscriber_sns_topic_arns.#:           "0" => "0"
      notification.2315001228.threshold:                             "100" => "0"
      notification.2315001228.threshold_type:                        "PERCENTAGE" => ""

So assuming people review terraform plans diligently, it's not too much of an issue.
People running terraform in their CI/CD pipeline without manual review might be affected though - you don't expect terraform to change its behaviour all of a sudden; on the other hand, as far as I know terraform also doesn't make any guarantees to preserve changes made outside of the terraform code.

[bjoernhaeuser]

@flosell If anyone just applies plans there is always the chance of changes like this, therefore I would be fine with it :)

[mbarrien]

Since it appears 1.58.0 is on the horizon, ping? @bflad

[goochi1]

Any update on if this will be merged?

[bflad]

Hi @flosell and everyone waiting. 👋 Thanks for submitting this and apologies for the delayed review here. Please see the below initial feedback and let us know if you have any questions or do not have time to implement the items.

[flosell]

@bflad, thanks for the feedback!

The PR is up to date again (resolved merge conflicts) and I fixed the issues following your suggestions.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSBudgets'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSBudgets -timeout 120m
=== RUN   TestAccAWSBudgetsBudget_basic
=== PAUSE TestAccAWSBudgetsBudget_basic
=== RUN   TestAccAWSBudgetsBudget_prefix
=== PAUSE TestAccAWSBudgetsBudget_prefix
=== RUN   TestAccAWSBudgetsBudget_notification
--- PASS: TestAccAWSBudgetsBudget_notification (269.49s)
=== CONT  TestAccAWSBudgetsBudget_basic
=== CONT  TestAccAWSBudgetsBudget_prefix
--- PASS: TestAccAWSBudgetsBudget_prefix (76.32s)
--- PASS: TestAccAWSBudgetsBudget_basic (84.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	353.837s

[bflad]

Thanks so much for the updates, @flosell 🚀 This will make a bunch of folks happy. 😄

--- PASS: TestAccAWSBudgetsBudget_prefix (14.72s)
--- PASS: TestAccAWSBudgetsBudget_basic (15.95s)
--- PASS: TestAccAWSBudgetsBudget_notification (47.28s)

[bflad]

This has been released in version 2.5.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!