-
Notifications
You must be signed in to change notification settings - Fork 166
feat: Add EventBridge bus logging configuration #181
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
base: master
Are you sure you want to change the base?
Conversation
@svenlito and @magreenbaum can I have a review from you on this one? Thanks in advance |
@tiagovmvieira Good job, how about specifying log group and place to store? |
b139e2c
to
116ada4
Compare
@magreenbaum Thanks for your review ;) I have applied your suggestions. May I get your approval? |
Hi @alavec . Thanks for raising such an important pointing that was missing in my initial implementation. So, I've updated my PR to consider the supported destinations for the EventBridge Bus logs (CloudWatch Log Group, S3 Bucket, Firehose). I've followed the example described in here. Actually, I've adapted it a bit to fit in this EventBridge module, specifically I haven't considered the CloudWatch Log Group, S3 Bucket and Firehose stream resource management in the module, but their required metadata is provided as an input instead. I've also included a new example in the A review from you is much appreaciated ;) |
main.tf
Outdated
var.create_bus && | ||
var.bus_log_config != null && | ||
var.bus_log_config.firehose != null && | ||
var.bus_log_config.firehose.enabled ? 1 : 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you forgot to remove "? 1 : 0".
╷
│ Error: Incorrect condition type
│
│ on .terraform/modules/eventbus.eventbridge/main.tf line 168, in resource "aws_cloudwatch_log_delivery_destination" "firehose":
│ 168: count = (
│ 169: var.create &&
│ 170: var.create_bus &&
│ 171: var.bus_log_config != null &&
│ 172: var.bus_log_config.firehose != null &&
│ 173: var.bus_log_config.firehose.enabled ? 1 : 0
│ 174: ) ? 1 : 0
│ ├────────────────
│ │ var.bus_log_config is object with 5 attributes
│ │ var.bus_log_config.firehose is object with 2 attributes
│ │ var.bus_log_config.firehose.enabled is false
│ │ var.create is true
│ │ var.create_bus is true
│
│ The condition expression must be of type bool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I have applied it accordingly
@antonbabenko could you please review this one? It's a big one, however these functionality is in demand I believe :) |
name = "EventsDeliveryDestination-${var.bus_name}-CWLogs" | ||
|
||
delivery_destination_configuration { | ||
destination_resource_arn = var.bus_log_config.cloudwatch.log_group_arn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the count condition checks var.bus_log_config.cloudwatch != null
, there's a potential null reference issue with accessing var.bus_log_config.cloudwatch.log_group_arn
. Terraform's evaluation order might still attempt to access this property even when the resource isn't created.
Consider using a more defensive approach:
destination_resource_arn = try(var.bus_log_config.cloudwatch.log_group_arn, null)
Or explicitly check for null values:
destination_resource_arn = var.bus_log_config.cloudwatch != null ? var.bus_log_config.cloudwatch.log_group_arn : null
This pattern should be applied to similar references in the S3 and Firehose configurations as well.
destination_resource_arn = var.bus_log_config.cloudwatch.log_group_arn | |
destination_resource_arn = try(var.bus_log_config.cloudwatch.log_group_arn, null) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Description
Adds support for EventBridge bus logging configuration through the new
bus_log_config
variable. This functionality allows users to configure the detail level and logging level for EventBridge buses.The
bus_log_config
variable is an optional object with two fields:include_detail
: Controls the level of detail in logs (accepts "NONE" or "FULL")level
: Controls the logging level (accepts "OFF", "ERROR", "INFO", or "TRACE")Motivation and Context
#182
This change adds essential functionality for monitoring and debugging EventBridge buses. The logging configuration enables users to:
This functionality was missing from the module and is a native AWS EventBridge capability that should be available through the Terraform module.
Breaking Changes
No breaking changes. The new
bus_log_config
variable has an empty default value {} and all fields are optional, maintaining compatibility with existing implementations.How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request