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: Add opt-in config to create order independent log filters #78

Conversation

aburgel
Copy link
Contributor

@aburgel aburgel commented Nov 27, 2024

Which problem is this PR solving?

The cloudwatch-logs module accepts a list of log group names. It uses terraform's count meta argument to create a list of subscription filter resources, which makes the resources dependent on the order of the log group names input.

Adding and removing log group names can cause terraform to create and destroy resources for all log groups after the changed elements in the list. This is troublesome if you dynamically generate the list of log groups (e.g. by using aws_cloudwatch_log_groups data source), because your plan can have quite a lot of changes, even if you're only adding or removing a small number of log groups.

Short description of the changes

Added an opt-in configuration option that uses for_each using the log filter names to create an order independent list of filters. When the list changes and the filter name is used, only differences in the existing and updated list of filters are modified.

How to verify that this has the expected result

I ran plan using this branch on my terraform project, and it resulted in the following:

  # module.honeycomb-cloudwatch-logs.aws_cloudwatch_log_subscription_filter.this[0] will be destroyed
  - resource "aws_cloudwatch_log_subscription_filter" "this" {
      - destination_arn = "arn:aws:firehose:us-east-1:123456:deliverystream/honeycomb-cloudwatch-logs" -> null
      - distribution    = "ByLogStream" -> null
      - id              = "cwlsf-123456" -> null
      - log_group_name  = "/my-log-group" -> null
      - name            = "/my-log-group-logs_subscription_filter" -> null
      - role_arn        = "arn:aws:iam::123456:role/honeycomb-cloudwatch-logs" -> null
        # (1 unchanged attribute hidden)
    }

  # honeycomb-cloudwatch-logs.aws_cloudwatch_log_subscription_filter.this["/my-log-group"] will be created
  + resource "aws_cloudwatch_log_subscription_filter" "this" {
      + destination_arn = "arn:aws:firehose:us-east-1:123456:deliverystream/honeycomb-cloudwatch-logs"
      + distribution    = "ByLogStream"
      + id              = (known after apply)
      + log_group_name  = "/my-log-group"
      + name            = "/my-log-group-logs_subscription_filter"
      + role_arn        = "arn:aws:iam::123456:role/honeycomb-cloudwatch-logs"
        # (1 unchanged attribute hidden)
    }

@aburgel aburgel requested a review from a team as a code owner November 27, 2024 21:42
@MikeGoldsmith
Copy link
Contributor

Thanks for opening this PR @aburgel. We'll take a look and get back to you soon. As you pointed out, we have to be a little careful as they could be considered a breaking change.

@TylerHelmuth TylerHelmuth added the status: oncall Flagged for awareness from Honeycomb Telemetry Oncall label Dec 2, 2024
@MikeGoldsmith
Copy link
Contributor

MikeGoldsmith commented Dec 11, 2024

Hi @aburgel - thanks again for opening this PR.

While we like what you've done, we are a little concerned changing the output behaviour as someone may be using it and changing from index based to a map of filter names could be problematic.

Would you be open to adding an opt-in flag to enable the new, deterministic behaviour? The default behaviour would be to use the index approach.

@aburgel
Copy link
Contributor Author

aburgel commented Dec 15, 2024

@MikeGoldsmith Thanks for the feedback. A flag sounds like a good way forward. I will add that in the next few days.

Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @aburgel. I think we can revert the changes to the old aws_cloudwatch_log_subscription_filter block but otherwise good.

modules/cloudwatch-logs/main.tf Show resolved Hide resolved
@MikeGoldsmith MikeGoldsmith changed the title fix: Use for_each to make log group sub filters order independent fix: Add opt-in config to create order independent log filters Dec 16, 2024
@aburgel
Copy link
Contributor Author

aburgel commented Dec 17, 2024

@MikeGoldsmith thanks for the review and approval! will you hit the merge button and cut a release? with the failing check, merge is disabled for me.

@MikeGoldsmith
Copy link
Contributor

Yep, just needed to do some additional testing. Thanks again @aburgel.

@MikeGoldsmith MikeGoldsmith merged commit c9b7c91 into honeycombio:main Dec 17, 2024
2 of 3 checks passed
@aburgel aburgel deleted the aburgel/log-group-subscription-resource-name branch December 17, 2024 17:27
MikeGoldsmith added a commit that referenced this pull request Dec 17, 2024
## Which problem is this PR solving?

Fix a formatting issue from #78 that wasn't picked up on the PR but
caused the main build to fail.

## Short description of the changes

- Remove white space so the formatting passes validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: oncall Flagged for awareness from Honeycomb Telemetry Oncall
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants