Brightspace / serverless-plugin-for-each

Serverless plugin that adds $forEach syntax to reduce code duplication and allow creating dynamic templates
Apache License 2.0
1 stars 5 forks source link

Multiple uses of $forEach produces wrong output #55

Closed dagrammy closed 1 year ago

dagrammy commented 1 year ago

Hi,

we use the plugin to simplify our deployments and avoid redudancies. Thank you very much for your work! 👍 Now we have noticed a problem.

If you use several $forEach with different iterators within one serverless.yml, then only one iterator is applied for all $forEach`. This is possible for example using imports for different resources.

For example:

serverless.yml

custom:
  queues:
    FIRST: first-queue
    SECOND: second-queue
  topics:
    a: a
    b: b
    c: c

resources:
  - ${file(./queues.yml)}
  - ${file(./topics.yml)}

queues.yml

  $forEach:
    iterator: "${self:custom.queues}"
    template:
        ...do sth...

topics.yml

  $forEach:
    iterator: "${self:custom.topics}"
    template:
        ...do sth...

This should produce something like:

-> first-queue
-> second-queue

-> a
-> b
-> c

Instead the result is:

-> first-queue
-> second-queue

-> first-queue
-> second-queue

Have I missed anything?

I am trying to create a complete example later.

AntonBazhal commented 1 year ago

Hi @dagrammy, I did a quick test and was not able to reproduce the problem. If you could provide a more detailed example, I would be able to look further into this.

dagrammy commented 1 year ago

Hi @AntonBazhal ,

thanks for looking! I created a small reproducer example, you can find it here: https://github.com/dagrammy/serverless-plugin-for-each-test

The serverless.yml looks like this and should create 5 EventBridge scheduled rules (2 rate based and 3 cron based).

service: serverless-plugin-for-each-test
frameworkVersion: '3'

provider:
  name: aws
  region: eu-central-1

custom:
  rate:
    - RATE_A
    - RATE_B
  cron:
    - CRON_Z
    - CRON_Y
    - CRON_X

resources:
  - ${file(./schedule/rate.yml)}
  - ${file(./schedule/cron.yml)}

plugins:
  - serverless-plugin-for-each

and the imported files are:

Resources:
  $forEach:
    iterator: "${self:custom.cron}"
    template:
      CronSchedule$forEach.value:
        Type: AWS::Events::Rule
        Properties:
          ScheduleExpression: cron(0 8 * * ? *)
          Targets:
            - Input: '{"scheduler":"$forEach.value"}'
              Arn: arn:aws:lambda:eu-central-1:123456789:function:callIt
              Id: callItId
Resources:
  $forEach:
    iterator: "${self:custom.rate}"
    template:
      RateSchedule$forEach.value:
        Type: AWS::Events::Rule
        Properties:
          ScheduleExpression: rate(5 minutes)
          Targets:
            - Input: '{"scheduler":"$forEach.value"}'
              Arn: arn:aws:lambda:eu-central-1:123456789:function:callIt
              Id: callItId

The expected result would be, that 5 resources are created.

- RateScheduleRATE_A
- RateScheduleRATE_B
- CronScheduleCRON_Z
- CronScheduleCRON_Y
- CronScheduleCRON_X

But the result shows 6 resources.

- RateScheduleCRON_Z
- RateScheduleCRON_Y
- RateScheduleCRON_X
- CronScheduleCRON_Z
- CronScheduleCRON_Y
- CronScheduleCRON_X

You can find more details in the readme: https://github.com/dagrammy/serverless-plugin-for-each-test/blob/main/README.md

Thank you very much and kind regards!

AntonBazhal commented 1 year ago

Oh, that's an amazing example. Thanks for doing that.

I see what's going on now. The problem is specific to the resources section and how serverless handles resources split into multiple files. Essentially, it merges Resource objects defined in each file, so $forEach sections get merged too. This happens before serverless-plugin-for-each can do its work, so the config the plugin is getting looks like this:

{
  "Resources": {
    "$forEach": {
      "iterator": [
        "CRON_Z",
        "CRON_Y",
        "CRON_X"
      ],
      "template": {
        "RateSchedule$forEach.value": {
          "Type": "AWS::Events::Rule",
          "Properties": {
            "ScheduleExpression": "rate(5 minutes)",
            "Targets": [
              {
                "Input": "{\"scheduler\":\"$forEach.value\"}",
                "Arn": "arn:aws:lambda:eu-central-1:123456789:function:callIt",
                "Id": "callItId"
              }
            ]
          }
        },
        "CronSchedule$forEach.value": {
          "Type": "AWS::Events::Rule",
          "Properties": {
            "ScheduleExpression": "cron(0 8 * * ? *)",
            "Targets": [
              {
                "Input": "{\"scheduler\":\"$forEach.value\"}",
                "Arn": "arn:aws:lambda:eu-central-1:123456789:function:callIt",
                "Id": "callItId"
              }
            ]
          }
        }
      }
    }
  }
}

So the plugin is doing what it's supposed to do and I don't see a way to achieve what you need without modifying the plugin. The only way to prevent serverless from merging configs is to resolve name collision, like

Resources:
  cron:
    $forEach:
      iterator: "${self:custom.cron}"
      template:
        CronSchedule$forEach.value:
          Type: AWS::Events::Rule
          Properties:
            ScheduleExpression: cron(0 8 * * ? *)
            Targets:
              - Input: '{"scheduler":"$forEach.value"}'
                Arn: arn:aws:lambda:eu-central-1:123456789:function:callIt
                Id: callItId

and

Resources:
  rate:
    $forEach:
      iterator: "${self:custom.rate}"
      template:
        RateSchedule$forEach.value:
          Type: AWS::Events::Rule
          Properties:
            ScheduleExpression: rate(5 minutes)
            Targets:
              - Input: '{"scheduler":"$forEach.value"}'
                Arn: arn:aws:lambda:eu-central-1:123456789:function:callIt
                Id: callItId

Unfortunately, that would result in resources being nested under those extra keys instead of being a flat map. The plugin can be changed to have an override property to make it override the parent key (cron or rate in this case), so that the config would look like this:

Resources:
  rate:
    $forEach:
      iterator: "${self:custom.rate}"
      override: true
      template:
        RateSchedule$forEach.value:
          Type: AWS::Events::Rule
          Properties:
            ScheduleExpression: rate(5 minutes)
            Targets:
              - Input: '{"scheduler":"$forEach.value"}'
                Arn: arn:aws:lambda:eu-central-1:123456789:function:callIt
                Id: callItId

I've been thinking of adding this feature for a while, but I've never got to do this.

dagrammy commented 1 year ago

Oh, that's an amazing example. Thanks for doing that.

You are welcome! :)

I see what's going on now. The problem is specific to the resources section and how serverless handles resources split into multiple files. Essentially, it merges Resource objects defined in each file, so $forEach sections get merged too. This happens before serverless-plugin-for-each can do its work, so the config the plugin is getting looks like this:

Thanks for your feedback and great explanation what is going on.

So the plugin is doing what it's supposed to do and I don't see a way to achieve what you need without modifying the plugin. The only way to prevent serverless from merging configs is to resolve name collision, like Unfortunately, that would result in resources being nested under those extra keys instead of being a flat map. The plugin can be changed to have an override property to make it override the parent key (cron or rate in this case), so that the config would look like this: I've been thinking of adding this feature for a while, but I've never got to do this.

The idea with the override property sounds promising. 👍

Another approach that I can think of....

Would it be possible to add an index or suffix to the $forEach:? Something like that:

Resources:
  $forEach_1:
    iterator: "${self:custom.rate}"

Resources:
  $forEach_2:
    iterator: "${self:custom.cron}"

or

Resources:
  $forEach_rate:
    iterator: "${self:custom.rate}"

Resources:
  $forEach_cron:
    iterator: "${self:custom.cron}"

That way it shouldn't be merged and it might be possible to search and then process all elements that start with $forEach:.

But I have no idea if this could work.

AntonBazhal commented 1 year ago

I think the suffix idea is simpler and more intuitive. I'll try implementing it this week as it seems straightforward.

dagrammy commented 1 year ago

That would be great, thank you very much! :slightly_smiling_face:

ghost commented 1 year ago

:tada: This issue has been resolved in version 2.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: