Aws http event schema #8301

thewizarodofoz · 2020-09-29T16:17:31Z

Addresses: #8018

This PR is getting pretty big, so I think it would be wise to limit it to the http event schema (and do the provider schema in another PR). @medikoo WDYT?

codecov-commenter · 2020-09-29T16:19:36Z

Codecov Report

Merging #8301 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8301      +/-   ##
==========================================
- Coverage   88.01%   87.95%   -0.07%     
==========================================
  Files         249      249              
  Lines        9307     9255      -52     
==========================================
- Hits         8192     8140      -52     
  Misses       1115     1115

Impacted Files	Coverage Δ
...ins/aws/package/compile/events/apiGateway/index.js	`100.00% <100.00%> (ø)`
.../aws/package/compile/events/apiGateway/lib/cors.js	`100.00% <100.00%> (ø)`
.../package/compile/events/apiGateway/lib/validate.js	`96.23% <100.00%> (-0.88%)`	⬇️

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 d2fb696...71104ac. Read the comment docs.

medikoo · 2020-09-30T11:35:54Z

This PR is getting pretty big, so I think it would be wise to limit it to the http event schema (and do the provider schema in another PR). @medikoo WDYT?

I think it's a good call, unless there are some properties which could be configured in both contexts, then I would also add them to provider section.

thewizarodofoz · 2020-10-01T15:11:06Z

Questions:

authorizer.claims - should we list the standard claims?
request.parameters[location][key].required - seems to me that if the value of a parameter is an object, 'required' is required because of this logic
should we share schema utils functions like arrayOrSingle, mapOfType - if so, where?
not sure about all of the if/else parts, might be better to have a base schema and extend that per use-case like we did in other schemas?

medikoo · 2020-10-01T16:08:48Z

@thewizarodofoz thanks for questions. I know it's a challenging one.

authorizer.claims - should we list the standard claims?

I think we may skip on that, and just accept whatever string. Trying to be strict on such detached things (as I see this is setn as into some string formartted config to AWS), may result with unwanted maintenance cost,

request.parameters[location][key].required - seems to me that if the value of a parameter is an object, 'required' is required because of this logic

I see what you mean. That if it's an object with no required property, we will assign an object where boolean is expected.
I see it as an error in our logic (which we can fix in other PR) - we can see in PR that introduced that, that there was no intention to make required required: #7897. I would definitely not imply required as required property

medikoo · 2020-10-01T16:11:32Z

should we share schema utils functions like arrayOrSingle, mapOfType

After we have #8319 in, there's no need for arrayOrSingle. We should just define array type (and if singular value was passed it'll be coerced to array, by schema validation engine)

I wouldn't generalize mapOfType, there's no duplication here (as we had with arrayOrSingle), and gain is minimal (while it makes schema less apparent)

medikoo · 2020-10-01T16:13:03Z

not sure about all of the if/else parts, might be better to have a base schema and extend that per use-case like we did in other schemas?

Can you elaborate a bit more? Anyway I would stay away from if/else, as it'll make reported errors obscure and not well informative. I think it's ok to implement more complex validation rules inline and let errors with meaningful messages surface

thewizarodofoz · 2020-10-01T22:18:56Z

@medikoo ready for review

medikoo

Thank you @thewizarodofoz ! Please see my comments, I think schema for object version has one important flaw (I decided to pause my review until we clarify that)

medikoo · 2020-10-02T11:08:35Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

-      // TODO: Complete schema, see https://github.com/serverless/serverless/issues/8018
-      anyOf: [{ type: 'string' }, { type: 'object' }],
+      oneOf: [
+        { type: 'string', pattern: `^(${httpMethods.join('|')}) \\S+` },


I think we need to use regex to be case insensitive (same as we did in HTTP API:

serverless/lib/plugins/aws/package/compile/events/httpApi/index.js

Line 57 in 5851bca

{ type: 'string', regexp: methodPathPattern.toString() },

)

Note that supporting just all lower case and all upper case will probably be not good enoug, as I take Get or Post also works

Do you think we should have the same case-insensitive treatment (using regex) for similar cases like authorizer.type and integration?

I think whenever we need to use regexp pattern and expect case insensitive matching, we should use regex instead of pattern

So if it's the case for authorizer.type an integration then, yes

medikoo · 2020-10-02T11:11:06Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+    method: {
+      enum: extendWithUppercase(httpMethods),
+    },
+    path: { type: 'string' },


It'll be nice to use same regex as in httpApi event

medikoo · 2020-10-02T11:14:20Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+        { type: 'string', pattern: `^(${httpMethods.join('|')}) \\S+` },
+        baseIntegrationSchema,
+        awsIntegrationSchema,
+        cognitoIntegrationSchema,
+        httpIntegrationSchema,
+        httpVpcLinkIntegrationSchema,
+        lambdaIntegrationSchema,
+        lambdaProxyIntegrationSchema,
+        mockIntegrationSchema,


I believe we expect method and path unconditionally, while above configuration, seem to expect different set of properties exclusively (and most of them do not require method and path), that doesn't look right.

In my understanding, schema for http is either string (in form of method path), or single version of object where method and path are required.

In similar manner as in httpApi event here:

serverless/lib/plugins/aws/package/compile/events/httpApi/index.js

Lines 57 to 79 in 5851bca

{ type: 'string', regexp: methodPathPattern.toString() },

{

type: 'object',

properties: {

authorizer: {

oneOf: [

{ type: 'string' },

{

type: 'object',

properties: {

id: {

anyOf: [{ type: 'string' }, { $ref: '#/definitions/awsCfFunction' }],

},

name: { type: 'string' },

scopes: { type: 'array', items: { type: 'string' } },

},

oneOf: [{ required: ['id'] }, { required: ['name'] }],

additionalProperties: false,

},

],

},

method: { type: 'string', regexp: methodPattern.toString() },

path: { type: 'string', regexp: /^(?:\*|\/\S*)$/.toString() },

but ofc we will have more properties in case of this event

I declared path as required in baseIntegrationSchema, so all other integration schemas inherit that.
I did fail to declare method as required as well.
The reason I used multiple object schemas instead of one is there many conditions regarding integration types. That is what I referred to when I wrote about if/else - if I go with one object for all integration types, then I would have to use many if/else clauses to declare all constraints.

Indeed, sorry I failed to see, that other objects, share all properties from baseIntegrationSchema

medikoo

Thank you @thewizarodofoz! It's a very challenging one, and what you've propsed looks very promising.

Please see my comments

medikoo · 2020-10-05T13:47:22Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+  oneOf: [
+    { type: 'string' },
+    caseInsensitive('aws_iam'),
+    { $ref: '#/definitions/awsArn' },


I think it should be awsArnString

medikoo · 2020-10-05T13:58:09Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+          oneOf: ['token', 'cognito_user_pools', 'request', 'aws_iam'].map(caseInsensitive),
+        },
+        authorizerId: {
+          oneOf: [{ type: 'string' }, { $ref: '#/definitions/awsCfRef' }],


Why not simply awsCfInstruction? Are we sure that relying on any other, aside of Ref, intrinsic functions is not supported here?

medikoo · 2020-10-05T14:00:18Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+          oneOf: [{ type: 'string' }, { $ref: '#/definitions/awsCfRef' }],
+        },
+        arn: { $ref: '#/definitions/awsArn' },
+        resultTtlInSeconds: { type: 'integer', maximum: 3600 },


Let's also set minimum

I think minimum should be 0 because:

If you specify a value greater than 0, API Gateway caches the authorizer responses

Meaning, setting 0 will disable cache. I have no experience with this, but I guess it's a valid option.

Sure, my point was, to not welcome negative values, we should set minimum: 0 I believe

medikoo · 2020-10-05T14:01:30Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+        identityValidationExpression: { type: 'string' },
+        managedExternally: { type: 'boolean' },
+        name: { type: 'string' },
+      },


Let's keep alphabetical order

lib/plugins/aws/package/compile/events/apiGateway/index.js

medikoo · 2020-10-05T14:13:02Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+      properties: {
+        allowCredentials: { type: 'boolean' },
+        headers: { type: 'array', items: { type: 'string' } },
+        maxAge: { type: 'integer', minimum: 0 },


I think it should be minimum 1

medikoo · 2020-10-05T14:22:17Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+        baseIntegrationSchema,
+        awsIntegrationSchema,
+        cognitoIntegrationSchema,
+        httpIntegrationSchema,
+        httpVpcLinkIntegrationSchema,
+        lambdaIntegrationSchema,
+        lambdaProxyIntegrationSchema,
+        mockIntegrationSchema,


One problem with this kind of configuration is that eventual errors will not be informative.

afaik if I declare method: FOO in otherwise perfectly valid configuration, the result error will be: "Unsupported configuration format", with no hint that error is at method property.

It goes from fact, that with such schema it's challenging to resolve which exactly oneOf option was chosen by user. AJV will expose errors resolved for each option, showing all of them will produce unbearable noise (we intentionally convert it then, to one "Unsupported configuration format")

We already had similar cases before, and we decided to configure properties for all options in one level, and leave (or introduce) inline validation that's meaningful for user as e.g. follows:

if (integrationType !== X && authorizer.claims) { throw new ServelressError("authorizer.claims is not supported for given integration type"); }

I would also stick here to that.

I believe we should not try to validate any complex interchangeable rules with JSON schema, as it then it becomes a maintenance burden we can't show a meaningful error message to user.

I understand.
I will refactor this PR to have a single schema, and leave the validations needed for the edge cases.

…hema

medikoo

Thank you @thewizarodofoz, that looks very good! I've found few last potentials issues. Otherwise it seems ready to go

lib/plugins/aws/package/compile/events/apiGateway/index.js

medikoo · 2020-10-06T15:21:25Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+            async: { type: 'boolean' },
+            authorizer: authorizerSchema,
+            connectionId: { type: 'string' },
+            connectionType: { enum: ['vpc-link', 'VPC_LINK'] },


We need to support both forms in case insensitive way

lib/plugins/aws/package/compile/events/apiGateway/index.js

medikoo · 2020-10-06T15:32:55Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+    passThrough: { enum: ['NEVER', 'WHEN_NO_MATCH', 'WHEN_NO_TEMPLATES'] },
+    schema: {
+      type: 'object',
+      additionalProperties: { type: ['object', 'string'] },


On AWS side it's type Json, does it mean that both forms are accepted, object and string ?

I got confused by the example:

functions: create: handler: posts.create events: - http: path: posts/create method: post request: schema: application/json: ${file(create_request.json)}

I understand now that ${file(create_request.json)} resolves to json (as object) so the string option is not relevant

medikoo · 2020-10-06T15:46:04Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+      oneOf: [
+        { properties: { origin: { type: 'string' } } },
+        {
+          properties: {
+            origins: {
+              type: 'array',
+              items: { type: 'string' },
+            },
+          },
+        },
+      ],


As I test locally, in this form schema will in most cases fail, we've used oneOf, and no property listed is required (so in most cases both variants match, and that's invalid by schema).

I guess it's not exposed by tests, only because there are no tests that involve this event written with runServerless

Normally we configure such things, by defining involved properties normally in properties section , and use oneOf to configure two different required collections

codecov-io · 2020-10-07T08:36:40Z

Codecov Report

Merging #8301 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8301      +/-   ##
==========================================
+ Coverage   88.01%   88.03%   +0.01%     
==========================================
  Files         249      249              
  Lines        9307     9267      -40     
==========================================
- Hits         8192     8158      -34     
+ Misses       1115     1109       -6

Impacted Files	Coverage Δ
...ins/aws/package/compile/events/apiGateway/index.js	`100.00% <100.00%> (ø)`
.../aws/package/compile/events/apiGateway/lib/cors.js	`100.00% <100.00%> (ø)`
.../package/compile/events/apiGateway/lib/validate.js	`96.23% <100.00%> (-0.88%)`	⬇️
lib/plugins/aws/deployFunction/index.js	`89.56% <0.00%> (-0.71%)`	⬇️
...ins/aws/customResources/resources/s3/lib/bucket.js	`0.00% <0.00%> (ø)`
...ws/customResources/resources/s3/lib/permissions.js	`0.00% <0.00%> (ø)`
...Resources/resources/eventBridge/lib/eventBridge.js	`0.00% <0.00%> (ø)`
...Resources/resources/eventBridge/lib/permissions.js	`0.00% <0.00%> (ø)`
...esources/resources/cognitoUserPool/lib/userPool.js	`0.00% <0.00%> (ø)`
...urces/resources/cognitoUserPool/lib/permissions.js	`0.00% <0.00%> (ø)`
... 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 d2fb696...912ab8c. Read the comment docs.

medikoo

Thank you @thewizarodofoz! We're closer! :)

medikoo · 2020-10-07T10:00:08Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+            async: { type: 'boolean' },
+            authorizer: authorizerSchema,
+            connectionId: { type: 'string' },
+            connectionType: { enum: ['vpc-link', 'VPC_LINK'].map(caseInsensitive) },


I think this produces as schema, where we expect an objects with regexp properties.

I believe this should be converted to anyOf list

I used oneOf to be consistent with other places that have .map(caseInsensitive). The array will have distinct patterns, so a value can never match more than one option.

medikoo · 2020-10-07T10:01:01Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+      type: 'object',
+      propertyNames: {
+        type: 'string',
+        pattern: '\\d{3}',


I think we need to wrap it with ^ and $

medikoo · 2020-10-07T10:03:07Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+    },
+    template: {
+      type: 'object',
+      additionalProperties: { type: ['string', 'null'] },


Let's not distinguish null in schema.

We actually support null at every property (it's treated to be equivalent to undefined or not set at all)

Adding extra recognition is specific schema is not effective in any way

medikoo · 2020-10-07T11:09:59Z

lib/plugins/aws/package/compile/events/apiGateway/index.js

+          items: { type: 'string' },
+        },
+      },
+      oneOf: [{ required: ['origin'] }, { required: ['origins'] }],


I think we need to also introduce required: [], otherwise we make passing origin required (either via origin or origins option)

It seems to me from the code that indeed one of them is required. We also had this manual validation that I removed which supports my theory.

Indeed, it appears that if someone uses an object form, then not providing origin will fail.

Still that looks as apparent design bug, as by supporting cors: true we indicate that all properties have defaults. Intuitively object form should fall into origin: '*' if one was not set (it's e.g. the case in HTTP API, which has similar cors setting)

Anyway this is side issue (not a concern of this PR) so yes let's leave it as required.

medikoo

Thank you @thewizarodofoz it's great to have that in! Thanks for all the patience, that was tricky, as we learned this event properties set is large as has a lot quirks

ozwi added 4 commits September 29, 2020 16:03

first go at http event schema

684c3ea

remove inline validations

b843b02

remove validation tests

b7236f2

remove validation tests

af31807

thewizarodofoz marked this pull request as draft September 29, 2020 16:17

medikoo mentioned this pull request Sep 30, 2020

feat(AJV): Add missing provider level schema properties #8297

Merged

final schema touches

0ea0dc8

thewizarodofoz marked this pull request as ready for review October 1, 2020 15:11

ozwi added 2 commits October 1, 2020 20:48

no need for arrayOrSingle due to type coercion

276f688

refactor to remove if/else

b8e46ad

medikoo suggested changes Oct 2, 2020

View reviewed changes

medikoo added cat/aws-event-http-api cat/config-schema enhancement labels Oct 2, 2020

medikoo assigned thewizarodofoz Oct 2, 2020

ozwi added 2 commits October 2, 2020 14:37

use case-insensitive for path and method

50fb4c8

use case-insensitive for authorizer.type and integration

e76e4d5

thewizarodofoz requested a review from medikoo October 3, 2020 11:25

medikoo suggested changes Oct 5, 2020

View reviewed changes

ozwi added 4 commits October 6, 2020 15:31

schema

f6c4fd9

remove inline validations which are covered by schema

f2c1a87

remove schema tests

a04fb5d

lint fixes

b9820ad

ozwi added 3 commits October 6, 2020 15:31

tests fixes

d8d59cf

Merge remote-tracking branch 'upstream/master' into aws-http-event-sc…

7011b4c

…hema

allow path without leading slash

71104ac

thewizarodofoz requested a review from medikoo October 6, 2020 13:58

medikoo suggested changes Oct 6, 2020

View reviewed changes

ozwi added 4 commits October 7, 2020 11:05

support case insensitive connectionType

7bd1098

add statusCodes.headers

34c1fe7

fix response.schema

0cfae06

fix origin/origins required

c32a320

thewizarodofoz requested a review from medikoo October 7, 2020 09:33

medikoo suggested changes Oct 7, 2020

View reviewed changes

small schema fixes

912ab8c

thewizarodofoz requested a review from medikoo October 8, 2020 10:15

medikoo approved these changes Oct 8, 2020

View reviewed changes

medikoo merged commit f235041 into serverless:master Oct 8, 2020

medikoo mentioned this pull request Oct 8, 2020

Config schema: Define AWS "http" function event properties #8018

Closed

8 tasks

thewizarodofoz deleted the aws-http-event-schema branch October 8, 2020 11:09

This was referenced Oct 21, 2020

Unrecognized property ‘documentation’ deliveryhero/serverless-aws-documentation#166

Open

Unrecognized property ‘documentation’ #8438

Closed

medikoo mentioned this pull request Dec 3, 2020

http integration schema validation #8573

Closed

	{ type: 'string', regexp: methodPathPattern.toString() },
	{
	type: 'object',
	properties: {
	authorizer: {
	oneOf: [
	{ type: 'string' },
	{
	type: 'object',
	properties: {
	id: {
	anyOf: [{ type: 'string' }, { $ref: '#/definitions/awsCfFunction' }],
	},
	name: { type: 'string' },
	scopes: { type: 'array', items: { type: 'string' } },
	},
	oneOf: [{ required: ['id'] }, { required: ['name'] }],
	additionalProperties: false,
	},
	],
	},
	method: { type: 'string', regexp: methodPattern.toString() },
	path: { type: 'string', regexp: /^(?:\\|\/\S)$/.toString() },

Aws http event schema #8301

Aws http event schema #8301

Conversation

thewizarodofoz commented Sep 29, 2020 • edited by medikoo

codecov-commenter commented Sep 29, 2020 • edited

Codecov Report

medikoo commented Sep 30, 2020

thewizarodofoz commented Oct 1, 2020 • edited by medikoo

medikoo commented Oct 1, 2020

medikoo commented Oct 1, 2020

medikoo commented Oct 1, 2020

thewizarodofoz commented Oct 1, 2020

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 7, 2020 • edited

Codecov Report

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

thewizarodofoz commented Sep 29, 2020 •

edited by medikoo

codecov-commenter commented Sep 29, 2020 •

edited

thewizarodofoz commented Oct 1, 2020 •

edited by medikoo

medikoo Oct 5, 2020 •

edited

codecov-io commented Oct 7, 2020 •

edited