fixes #8230

[dedamico]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8230

Release note for CHANGELOG:

resource/aws_cloudwatch_log_subscription_filter: prevent change being reported for blank distribution attribute

[bflad]

Hi @dedamico 👋 Would you happen to have a configuration that triggers the behavior so we can add a covering acceptance test? The change to Computed: true is likely okay if the value has different defaults depending on other arguments, although it'd be better if we could use a DiffSuppressFunc based on those other triggering arguments so Terraform can continue to perform drift detection when its not configured. Thanks!

[dedamico]

Hi @bflad! Thanks for the feedback!

I have done the following with the latest commit to my branch:

1. Updated the code to use DiffSuppressFunc rather than Computed.
2. I took a stab at updating the acceptance test to add a Test Step for the slightly different config. The added config's only difference is that the distribution attribute is not set, which is the use case which the bug happened with (AWS API sets default distribution value to ByLogStream if it is not set).

please let me know if anything looks off/wrong and I'll fix it!

[samm-git]

@bflad hi, any chance to get this merged?

[bflad]

Hi @dedamico 👋 Very sorry for the delayed merge process here. When looking at the testing for this resource, we noticed we were not checking Kinesis Data Firehose and Kinesis Stream destinations, to verify any code changes against them as well. I was able to get this testing added with these configurations:

# Kinesis Data Firehose
# Reference: https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/SubscriptionFilters.html#FirehoseExample

data "aws_caller_identity" "current" {}

data "aws_partition" "current" {}

data "aws_region" "current" {}

resource "aws_cloudwatch_log_group" "test" {
  name              = %[1]q
  retention_in_days = 1
}

resource "aws_iam_role" "firehose" {
  name = "%[1]s-firehose"

  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "",
      "Effect": "Allow",
      "Principal": {
        "Service": "firehose.amazonaws.com"
      },
      "Action": "sts:AssumeRole",
      "Condition": {
        "StringEquals": {
          "sts:ExternalId": "${data.aws_caller_identity.current.account_id}"
        }
      }
    }
  ]
}
EOF
}

resource "aws_iam_role_policy" "firehose" {
  role = "${aws_iam_role.firehose.name}"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "",
      "Effect": "Allow",
      "Action": [
        "s3:AbortMultipartUpload",
        "s3:GetBucketLocation",
        "s3:GetObject",
        "s3:ListBucket",
        "s3:ListBucketMultipartUploads",
        "s3:PutObject"
      ],
      "Resource": [
        "${aws_s3_bucket.test.arn}",
        "${aws_s3_bucket.test.arn}/*"
      ]
    }
  ]
}
EOF
}

resource "aws_iam_role" "cloudwatchlogs" {
  name = "%[1]s-cloudwatchlogs"

  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": "sts:AssumeRole",
      "Principal": {
        "Service": "logs.${data.aws_region.current.name}.amazonaws.com"
      },
      "Effect": "Allow",
      "Sid": ""
    }
  ]
}
EOF
}

resource "aws_iam_role_policy" "cloudwatchlogs" {
  role = "${aws_iam_role.cloudwatchlogs.name}"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Action": "firehose:*",
      "Resource": "arn:${data.aws_partition.current.partition}:firehose:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"
    },
    {
      "Effect": "Allow",
      "Action": "iam:PassRole",
      "Resource": "${aws_iam_role.cloudwatchlogs.arn}"
    }
  ]
}
EOF
}

resource "aws_kinesis_firehose_delivery_stream" "test" {
  destination = "extended_s3"
  name        = %[1]q

  extended_s3_configuration {
    bucket_arn = "${aws_s3_bucket.test.arn}"
    role_arn   = "${aws_iam_role.firehose.arn}"
  }
}

resource "aws_s3_bucket" "test" {
  acl           = "private"
  bucket        = %[1]q
  force_destroy = true
}

resource "aws_cloudwatch_log_subscription_filter" "test" {
  destination_arn = "${aws_kinesis_firehose_delivery_stream.test.arn}"
  filter_pattern  = "logtype test"
  log_group_name  = "${aws_cloudwatch_log_group.test.name}"
  name            = %[1]q
  role_arn        = "${aws_iam_role.cloudwatchlogs.arn}"
}

# Kinesis Stream
# Reference: https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/SubscriptionFilters.html#DestinationKinesisExample

data "aws_region" "current" {}

resource "aws_cloudwatch_log_group" "test" {
  name              = %[1]q
  retention_in_days = 1
}

resource "aws_iam_role" "test" {
  name = %[1]q

  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": "sts:AssumeRole",
      "Principal": {
        "Service": "logs.${data.aws_region.current.name}.amazonaws.com"
      },
      "Effect": "Allow",
      "Sid": ""
    }
  ]
}
EOF
}

resource "aws_iam_role_policy" "test" {
  role = "${aws_iam_role.test.name}"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Action": "kinesis:PutRecord",
      "Resource": "${aws_kinesis_stream.test.arn}"
    },
    {
      "Effect": "Allow",
      "Action": "iam:PassRole",
      "Resource": "${aws_iam_role.test.arn}"
    }
  ]
}
EOF
}

resource "aws_kinesis_stream" "test" {
  name        = %[1]q
  shard_count = 1
}

resource "aws_cloudwatch_log_subscription_filter" "test" {
  destination_arn = "${aws_kinesis_stream.test.arn}"
  filter_pattern  = "logtype test"
  log_group_name  = "${aws_cloudwatch_log_group.test.name}"
  name            = %[1]q
  role_arn        = "${aws_iam_role.test.arn}"
}

When checking the debug logs for the value of the Distribution parameter with all three destination types (Lambda was already being tested), it appears they all were using a default value of ByLogStream. When we see this behavior with APIs across all variations, the best solution in Terraform is to use Default with an attribute instead of DiffSuppressFunc (which only would have been necessary if there were differences in the default depending on the destination). Sorry for any confusion here!

Output from acceptance testing after adjusting to Default: cloudwatchlogs.DistributionByLogStream (the test failure is a new bug discovery and will be fixed in a subsequent fix):

--- PASS: TestAccAWSCloudwatchLogSubscriptionFilter_basic (40.61s)
--- PASS: TestAccAWSCloudwatchLogSubscriptionFilter_disappears (41.09s)
--- PASS: TestAccAWSCloudwatchLogSubscriptionFilter_disappears_LogGroup (42.32s)
--- PASS: TestAccAWSCloudwatchLogSubscriptionFilter_Distribution (51.33s)
--- PASS: TestAccAWSCloudwatchLogSubscriptionFilter_DestinationArn_KinesisStream (108.80s)
--- FAIL: TestAccAWSCloudwatchLogSubscriptionFilter_RoleArn (114.10s)
    testing.go:568: Step 2 error: errors during apply:

        Error: Error updating SubscriptionFilter (tf-acc-test-3057491702311743141) for LogGroup (tf-acc-test-3057491702311743141), message: "Could not deliver test message to specified Kinesis stream. Check if the given kinesis stream is in ACTIVE state.", code: "InvalidParameterException"

          on /var/folders/v0/_d108fkx1pbbg4_sh864_7740000gn/T/tf-test472807723/main.tf line 98:
          (source code not available)


--- PASS: TestAccAWSCloudwatchLogSubscriptionFilter_DestinationArn_KinesisDataFirehose (126.02s)

[bflad]

(This was merged in after being rebased - the commits are there despite me closing this 😄 )

[ghost]

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

[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!