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

feat: Allow setting id parameter in notification object #236

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

shbedev
Copy link
Contributor

@shbedev shbedev commented Jun 6, 2023

Description

Allow setting the id parameter inside the notification block.

Motivation and Context

When configuring bucket notifications using map type inside another map, like in the example here, there is no control on the order of the notifications.

This can cause terraform to change the resource and replace the notifications order after importing bucket notification, because the state contains list of sorted notifications and the map order can be different.

sns_notifications = {
  ios = {
    topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
    events        = ["s3:ObjectCreated:*"]
    filter_prefix = "ios/"
  },
  android = {
    topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
    events        = ["s3:ObjectCreated:*"]
    filter_prefix = "android/"
  }
}
 # aws_s3_bucket_notification.this[0] will be updated in-place
  ~ resource "aws_s3_bucket_notification" "this" {
        id          = "example-bucket-id"
        # (2 unchanged attributes hidden)

      ~ topic {
          ~ filter_prefix = "ios/" -> "android/"
          ~ id            = "ios" -> "android"
          ~ topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload" -> "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
            # (1 unchanged attribute hidden)
        }
      ~ topic {
          ~ filter_prefix = "android/" -> "ios/"
          ~ id            = "android" -> "ios"
          ~ topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload" -> "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
            # (1 unchanged attribute hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

This made me configure the notifications in a list rather then map type, but then I realized that the id parameter is not configurable, and will be the key (index) of the object in the list.

sns_notifications = [
    {
      topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
      events        = ["s3:ObjectCreated:*"]
      filter_prefix = "ios/"
      id            = "ios"
    },
    {
      topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
      events        = ["s3:ObjectCreated:*"]
      filter_prefix = "android/"
      id            = "android"
    }
  ]
Terraform will perform the following actions:

  # aws_s3_bucket_notification.this[0] will be updated in-place
  ~ resource "aws_s3_bucket_notification" "this" {
        id          = "example-bucket-id"
        # (2 unchanged attributes hidden)

      ~ topic {
          ~ id            = "ios" -> "0"
            # (3 unchanged attributes hidden)
        }
      ~ topic {
          ~ id            = "android" -> "1"
            # (3 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

The PR will make it possible to configure the id inside each notification block and will allow use of list type over map when you need to control the order of the notifications.

Breaking Changes

No breaking changes.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@shbedev shbedev changed the title Allow setting id parameter in notification object feat: Allow setting id parameter in notification object Jun 6, 2023
@antonbabenko
Copy link
Member

This example is correct because it is a map with keys:

sns_notifications = {
  ios = {
    topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
    events        = ["s3:ObjectCreated:*"]
    filter_prefix = "ios/"
  },
  android = {
    topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
    events        = ["s3:ObjectCreated:*"]
    filter_prefix = "android/"
  }
}

topic is used as a block inside aws_s3_bucket_notification, so the order should not matter.

The change you are proposing is correct but it should not be necessary if you provide input as map(map(any)) like in the example.

@shbedev
Copy link
Contributor Author

shbedev commented Jun 7, 2023

@antonbabenko maybe I am missing something, but when using pure terraform and setting the topic blocks, I have control of the order of the notifications:

resource "aws_s3_bucket_notification" "bucket_notification" {
  bucket = "example-bucket"

  topic {
    topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
    events        = ["s3:ObjectCreated:*"]
    filter_prefix = "ios/"
    id            = "ios"
  }

  topic {
    topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
    events        = ["s3:ObjectCreated:*"]
    filter_prefix = "android/"
    id            = "android"
  }
}

In the plan, you can see that the id ios is the first, and android second:

# aws_s3_bucket_notification.bucket_notification will be created
  + resource "aws_s3_bucket_notification" "bucket_notification" {
      + bucket      = "example-bucket"
      + eventbridge = false
      + id          = (known after apply)

      + topic {
          + events        = [
              + "s3:ObjectCreated:*",
            ]
          + filter_prefix = "ios/"
          + id            = "ios"
          + topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
        }
      + topic {
          + events        = [
              + "s3:ObjectCreated:*",
            ]
          + filter_prefix = "android/"
          + id            = "android"
          + topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

When configuring the same order using the module, the order of the notifications is changing, and ios is in the second place:

module "notification" {
  source = "github.com/terraform-aws-modules/terraform-aws-s3-bucket//modules/notification?ref=v3.11.0"

  eventbridge       = true
  create_sqs_policy = false
  create_sns_policy = false

  sns_notifications = {
    ios = {
      topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
      events        = ["s3:ObjectCreated:*"]
      filter_prefix = "ios/"
    },
    android = {
      topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
      events        = ["s3:ObjectCreated:*"]
      filter_prefix = "android"
    }
  }
}
Terraform will perform the following actions:

  # module.notification.aws_s3_bucket_notification.this[0] will be created
  + resource "aws_s3_bucket_notification" "this" {
      + eventbridge = true
      + id          = (known after apply)

      + topic {
          + events        = [
              + "s3:ObjectCreated:*",
            ]
          + filter_prefix = "android"
          + id            = "android"
          + topic_arn     = "arn:aws:sns:eu-west-1:123456789012:android-log-upload"
        }
      + topic {
          + events        = [
              + "s3:ObjectCreated:*",
            ]
          + filter_prefix = "ios/"
          + id            = "ios"
          + topic_arn     = "arn:aws:sns:eu-west-1:123456789012:ios-log-upload"
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

So when using the module with map type, the order is not preserved and the module passes the map keys down to the resource in alphabetical order and not in the order they were configured:

module "notification" {
  source = "github.com/terraform-aws-modules/terraform-aws-s3-bucket//modules/notification?ref=v3.11.0"

  eventbridge       = true
  create_sqs_policy = false
  create_sns_policy = false

  sns_notifications = {
    c = {
      topic_arn     = "arn:aws:sns:eu-west-1:123456789012:c"
      events        = ["s3:ObjectCreated:*"]
      filter_prefix = "c"
    }
    a = {
      topic_arn     = "arn:aws:sns:eu-west-1:123456789012:a"
      events        = ["s3:ObjectCreated:*"]
      filter_prefix = "a"
    }
    b = {
      topic_arn     = "arn:aws:sns:eu-west-1:123456789012:b"
      events        = ["s3:ObjectCreated:*"]
      filter_prefix = "b"
    }
  }
}
Terraform will perform the following actions:

  # module.notification.aws_s3_bucket_notification.this[0] will be created
  + resource "aws_s3_bucket_notification" "this" {
      + eventbridge = true
      + id          = (known after apply)

      + topic {
          + events        = [
              + "s3:ObjectCreated:*",
            ]
          + filter_prefix = "a"
          + id            = "a"
          + topic_arn     = "arn:aws:sns:eu-west-1:123456789012:a"
        }
      + topic {
          + events        = [
              + "s3:ObjectCreated:*",
            ]
          + filter_prefix = "b"
          + id            = "b"
          + topic_arn     = "arn:aws:sns:eu-west-1:123456789012:b"
        }
      + topic {
          + events        = [
              + "s3:ObjectCreated:*",
            ]
          + filter_prefix = "c"
          + id            = "c"
          + topic_arn     = "arn:aws:sns:eu-west-1:123456789012:c"
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

This is makes sense when creating resources, but when importing notifications that are not in alphabetical order, terraform forces a change of the resource.

@antonbabenko
Copy link
Member

Thank you very much for the explanation.

This seems like a bug in Terraform AWS provider (please open an issue there), but I will also merge your PR since it fixes the problem you're experiencing!

@antonbabenko antonbabenko merged commit f9067dc into terraform-aws-modules:master Jun 8, 2023
antonbabenko pushed a commit that referenced this pull request Jun 8, 2023
## [3.12.0](v3.11.0...v3.12.0) (2023-06-08)

### Features

* Allow setting id parameter in notification object ([#236](#236)) ([f9067dc](f9067dc))
@antonbabenko
Copy link
Member

This PR is included in version 3.12.0 🎉

@github-actions
Copy link

github-actions bot commented Jul 9, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants