feat: Support resource extensions

[glb]

What did you implement

The merge logic for extending resources with the existing framework using resources.Resources has some surprising behaviours.

This introduces a more formalized definition for extending resources, where Metadata and Properties will be shallowly merged (the values for existing keys will be replaced instead of merged), the DependsOn list will be added to, and the values for CreationPolicy, DeletionPolicy, UpdatePolicy, and UpdateReplacePolicy will be replaced if they exist in the extension.

Extension of other resource attributes is not supported at this time.

Related to but does not completely address the requirements in #6575, as fully complying with those requirements would be a breaking change.

Related to #7338.

How can we verify it

1. Copy serverless.yml below to disk.

2. Run serverless package.

3. Examine .serverless/cloudformation-template-update-stack.json. Validate that the SampleLogGroup resource has RetentionInDays set to "30".

See also the automated tests that validate the full behaviour.

serverless.yml

service: test

provider:
  name: aws
  runtime: go1.x
  stage: dev

functions:
  sample:
    handler: bin/sample
    events:
      - http:
          method: post
          path: /sample

resources:
  extensions:
    SampleLogGroup:
      Properties:
        RetentionInDays: '30'

package:
  exclude:
    - ./**
  include:
    - ./bin/**

Todos

• Write and run all tests
• Write documentation
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

[codecov-io]

Codecov Report

Merging #7352 into master will increase coverage by 0.02%.
The diff coverage is 90.41%.

@@            Coverage Diff             @@
##           master    #7352      +/-   ##
==========================================
+ Coverage   87.99%   88.02%   +0.02%     
==========================================
  Files         240      240              
  Lines        8997     9007      +10     
==========================================
+ Hits         7917     7928      +11     
+ Misses       1080     1079       -1

Impacted Files	Coverage Δ
lib/utils/tracking.js	78.49% <ø> (ø)	⬆️
...Resources/resources/eventBridge/lib/eventBridge.js	0% <0%> (ø)	⬆️
lib/plugins/aws/lib/naming.js	97.35% <100%> (ø)	⬆️
...customResources/resources/eventBridge/lib/utils.js	70% <100%> (+70%)	⬆️
...ns/aws/package/compile/events/eventBridge/index.js	100% <100%> (ø)	⬆️
...mpile/events/lib/ensureApiGatewayCloudWatchRole.js	100% <100%> (ø)	⬆️
lib/plugins/aws/invokeLocal/index.js	72.01% <100%> (ø)	⬆️
...ib/plugins/aws/package/compile/events/sns/index.js	93.4% <82.6%> (ø)	⬆️
...lugins/aws/package/compile/events/httpApi/index.js	88.88% <92.3%> (ø)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5570c7...ab7ee19. Read the comment docs.

[medikoo]

@glb looks great! I've just posted a suggestion to use more natural constructs for iteration.

@pmuens please take a look, I think it's an imporant addition (with v2 we probably should drop support for resources.Resources entirely, and propose resources.addition instead and validate they do not override anything generated by the framework).

I'd also improve the documentation (I'll do it prior merging PR)

[pmuens]

Neat idea 👍

Do we also plan to support other cloud providers as well? I'm asking since resources was the central place where every provider plugin could expect custom provider related resources. How would an extension work if we implement this for another provider (e.g. Azure)? Is an extension provider independent or should it only be used for AWS?

[glb]

@medikoo I've updated to use for..of as requested; I did it as a separate commit so it would be easier to review.

@pmuens I'm going to assume your question was for @medikoo :)

[medikoo]

Do we also plan to support other cloud providers as well?

I think it's a CloudFormation specific thing. Not sure if other providers have that, and if, whether it's still named resources (?)

Technically currently resources is top-level, although in my understanding it was designed just for AWS (is that correct?)

If we agree that all top level should be assumed as provider-agnostic, then I would probably move it to provider.resources or even provider.cloudFormationResources, to indicate clearly we're handling here CloudFormation template

@pmuens What are your thoughts?

[glb]

@medikoo / @pmuens I'm not super-comfortable with the direction this conversation is taking ... this PR came about because @medikoo suggested that provider.apiGateway.deployment.dependsOn was the wrong direction:

(I wouldn't propagate it to provider namespace, as we don't keep there CF logic)

If you're talking about a future change to remove resources as part of the final resolution of #6575, then could I suggest that maybe that's separate from whether this PR is a valid intermediate step?

Personally I liked the direction that this was going in, even though it was different from what I'd originally proposed, but ultimately my (selfish) goal is to be able to reliably add something to the AWS::ApiGateway::Deployment's DependsOn attribute, ideally soonish. Currently I can't do that, and actually I'm reminded that unless jacob-meacham/serverless-plugin-bind-deployment-id#26 gets merged I still won't be able to, even if this change goes in.

I suppose another option would be to write a plugin that does the specific thing I need?

[medikoo]

@glb This PR is intended to be purely about introduction of resources.extensions, we're not expecting any changes to resources here.

What I'm discussing with @pmuens is wheter it's not better to provide extensions at provider.cloudFormationResources.extensions instead of placing them at resources.extensions (that's all)

Simply to coin right config schema from a start.

We will do changes to resources in other PR, and that'll be a breaking change to be published with v2

[glb]

Thanks @medikoo ... I think you had a very good point when you suggested that extensions should be placed with the existing resources, because in a given service they are going to be closely related.

Personally I put provider near the top of my serverless.yml and resources near the end; perhaps I'm alone in this but I don't think so. Putting the extensions under provider would make these definitions pretty much as far apart as possible in the file.

[medikoo]

As I inspected history of resources config. It was first proposed as resources.aws and then aws was stripped here: 2c20687

I assume that intention was that resources will be a provider agnostic option, which will naturally resort to CloudFormation resources in case of AWS, and eventually some other concept in case of other providers.

It's probably ok to not change that

How would an extension work if we implement this for another provider (e.g. Azure)? Is an extension provider independent or should it only be used for AWS?

@pmuens I think idea should be same as in case of AWS, that resources.extensions should be about extending resources created by the framework.

General idea is to replace resources with something as resources.additions and resources.extensions, to ensure bug free handling. History shown that extending resources as created by Framework demands extra care to avoid issues (we cannot just deeply merge it as it's the case now, as sometimes deep property merge produces invalid structures, and sometimes user is not be aware that framework genereated resource will be overriden with his setup).

This distinction will allow to fix such issues.

In v1 we can add resources.extensions and with v2 we would be able to add resources.addtions and do not support any setup directly on resources

[pmuens]

Technically currently resources is top-level, although in my understanding it was designed just for AWS (is that correct?)

The resources section was introduced to be a way for all providers to bring in additional resource one needs to spin up a whole service.

If we agree that all top level should be assumed as provider-agnostic, then I would probably move it to provider.resources or even provider.cloudFormationResources, to indicate clearly we're handling here CloudFormation template

That sounds reasonable, however I agree with @glb that the resources are usually defined at the bottom and the provider config is defined at the top of the serverless.yml file.

IMHO it's not a big deal to introduce the extensions config at the resources level. I just wanted to ensure that we'll implement this in a way that it's provider agnostic (maybe we need to re-use this in other provider plugins as well).

[medikoo]

@glb @pmuens I believe we agree it's fine to stick to resources.extensions. Therefore this implementation looks good to take.

[medikoo]

@pmuens can you give it a second look?