Skip to content

feat(aws): schema for resources when using AWS provider #8139

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.

Sign up for GitHub

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

Merged
medikoo merged 1 commit into from
Sep 7, 2020
Merged

feat(aws): schema for resources when using AWS provider #8139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions lib/classes/Service.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,16 @@ describe('Service', () => {
resources: [
{
Resources: {
aws: {
resourcesProp: 'value',
resource1: {
Type: 'value',
},
},
},
{
Resources: {
azure: {},
resource2: {
Type: 'value2',
},
},
},
],
Expand All @@ -195,8 +197,8 @@ describe('Service', () => {
)
.then(({ cfTemplate: { Resources } }) => {
expect(Resources).to.be.an('object');
expect(Resources.aws).to.deep.equal({ resourcesProp: 'value' });
expect(Resources.azure).to.deep.equal({});
expect(Resources.resource1).to.deep.equal({ Type: 'value' });
expect(Resources.resource2).to.deep.equal({ Type: 'value2' });
}));

it('should merge functions given as an array', () =>
Expand Down
81 changes: 79 additions & 2 deletions lib/plugins/aws/provider/awsProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,15 @@ class AwsProvider {
required: ['Fn::Sub'],
additionalProperties: false,
},
awsResourceProperties: {
Properties: { type: 'object' },
CreationPolicy: { type: 'object' },
DeletionPolicy: { type: 'string' },
DependsOn: { type: 'array', items: { type: 'string' } },
Metadata: { type: 'object' },
UpdatePolicy: { type: 'object' },
UpdateReplacePolicy: { type: 'string' },
},
},
provider: {
properties: {
Expand Down Expand Up @@ -323,8 +332,76 @@ class AwsProvider {
},
},
},
// TODO: Complete schema, see https://github.com/serverless/serverless/issues/8014
resources: { type: 'object' },
resources: {
properties: {
AWSTemplateFormatVersion: {
type: 'string',
},
Description: {
type: 'string',
},
Metadata: {
type: 'object',
},
Parameters: {
type: 'object',
},
Mappings: {
type: 'object',
},
Conditions: {
type: 'object',
},
Transform: {
type: 'object',
},
// Not replicating the full JSON schema from https://s3.amazonaws.com/cfn-resource-specifications-us-east-1-prod/schemas/2.15.0/all-spec.json
// as that gets into the specifics for each resource type.
//
// The only required attribute is `Type`; `Properties` and other common attributes are optional.
// See also https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html
Resources: {

@medikoo medikoo Aug 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's provide definitions for all possible CF template properties (listed here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/template-anatomy.html ) but ofc without going into deep details, it should serve more as sanity check

@glb glb Aug 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To clarify: you're just looking for a top-level check on AWSTemplateFormatVersion, Description, Metadata, Parameters, Mappings, Conditions, Transform, and Outputs in addition to the existing schema for Resources here?

@medikoo medikoo Aug 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, exactly, as all of them are supported by Framework

type: 'object',
patternProperties: {
'^[a-zA-Z0-9]{1,255}$': {
type: 'object',
properties: {
$ref: '#/definitions/awsResourceProperties',
},
additionalProperties: {
Type: { type: 'string' },

@medikoo medikoo Aug 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It'll probably be good to add definition for Properties (just `type: 'object') and all properties listed here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-product-attribute-reference.html (just sanity checks).

Ideally if we define all those properties (aside of Type) as reusable definition. For that I'd add support for provider to define its own definitions here:

serverless/lib/classes/ConfigSchemaHandler/index.js

Line 172 in 5f85543

So they're added to this object:

serverless/lib/configSchema.js

Line 116 in 5f85543

definitions: {

With this move I would also define awsArn and awsCfImport in scope of defineProvider call (move out from core schema).

Having that I would reuse this definition here, in form:

{
  properties: { $ref: '#/definitions/awsResource' },
  additionalProperties: {
    Type: { type: "string: }
  }

and reuse it also in context of resources.extensions

@glb glb Aug 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've put in a first attempt at a smoke-check definition for Properties, CreationPolicy, DeletionPolicy, DependsOn, Metadata, UpdatePolicy, and UpdateReplacePolicy. I'm not sure I understand why you want these to be reusable definitions?

Note that the supported properties for resources.extensions are different from the supported properties for resources, so we can't re-use the type definition for resources and resources.extensions. Type is explicitly not supported for resources.extensions (see #7352 and specifically this switch statement).

I can look into adding support for extending definitions and moving awsArn and awsCfImport.

@medikoo medikoo Aug 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that the supported properties for resources.extensions are different from the supported properties for resources,

Both resources.Resources and resource.extensions extend CloudFormation resources, why do you assume that supported properties are different?

@glb glb Aug 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@medikoo resources.Resources allows you to add new resources or extend existing resources; resource.extensions explicitly only allows you to extend existing resources. When I wrote #7352 at your request, the implementation intentionally disallowed specifying Type in resources.extensions for this reason.

@medikoo medikoo Aug 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, afaik the only difference between them is that Type is not supported in context of resources.extensions.

That's also the reason why I suggested to exclude Type in generalization. Afaik generalization as I proposed can work well for both, or do I miss something?

},
},
},
additionalProperties: false,
},
extensions: {
type: 'object',
patternProperties: {
// names have the same restrictions as CloudFormation Resources section
'^[a-zA-Z0-9]{1,255}$': {
type: 'object',
// this lists the supported properties, other properties are "Not supported. An error will be thrown
// if you try to extend an unsupported attribute."
// this is different than the above schema for `Resources`, which allows the `Type` attribute.
// extensions are explicitly meant to extend the definition of existing resources.
properties: {
$ref: '#/definitions/awsResourceProperties',
},
additionalProperties: false,
},
},
additionalProperties: false,
},
// According to https://s3.amazonaws.com/cfn-resource-specifications-us-east-1-prod/schemas/2.15.0/all-spec.json
// `Outputs` is just an "object", though it seems like this is under-specifying that section a bit.
// See also https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/outputs-section-structure.html
Outputs: {
type: 'object',
},
},
additionalProperties: false,
},

@medikoo medikoo Aug 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need additionalProperties: false here

});
}
this.requestCache = {};
Expand Down