Aws cloudfront event schema #8250

thewizarodofoz · 2020-09-15T15:11:01Z

Closes: #8025

A few things I'm not sure of:

Is the regex for pathPattern necessary? I derived it from this doc
Should I add the full schema for origin? It will be quite verobse...
Should I add the full schema for behavior? It will be quite verobse...
Will the uri format work for origin?

medikoo

Thanks you @thewizarodofoz ! It looks promising. Please see my comments

Is the regex for pathPattern necessary? I derived it from this doc

I think what you proposed looks good. I've just picked on few potential issues

Should I add the full schema for origin? It will be quite verobse

In general we define all config properties, and we did so for all events so far. So in this case I'd also ensure that we define them all.
There's a pretty good reference for it: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-origin.html

Should I add the full schema for behavior? It will be quite verobse..

Yes I'd also follow with it. Here case is more complex as some properties are supported on its own in upper level (as pathPattern). In this case I'd allow to set only properties which are not handled in other scope (so no support for PathPattern at this level)

Will the uri format work for origin?

I think it's a great pick (btw I've just learned about format keyword`)

medikoo · 2020-09-16T08:16:47Z

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

+        isDefaultOrigin: { type: 'boolean' },
+        includeBody: { type: 'boolean' },
+        behavior: { type: 'object' },
+      },


Let's add additionalProperties: false

medikoo · 2020-09-16T08:19:44Z

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

+          type: 'string',
+          enum: ['viewer-request', 'origin-request', 'origin-response', 'viewer-response'],
+        },
+        pathPattern: { type: 'string', pattern: '^([A-Za-z0-9-_.*$/~"\'@:+]|&amp;)+$' },


- char needs to be escaped (otherwise in [] it implies a range between characters.

moved it to the end of the array like we do in other patterns

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

codecov-commenter · 2020-09-16T13:43:38Z

Codecov Report

Merging #8250 into master will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8250      +/-   ##
==========================================
- Coverage   88.11%   88.02%   -0.09%     
==========================================
  Files         248      248              
  Lines        9388     9312      -76     
==========================================
- Hits         8272     8197      -75     
+ Misses       1116     1115       -1

Impacted Files	Coverage Δ
...ins/aws/package/compile/events/cloudFront/index.js	`99.32% <100.00%> (ø)`
.../package/compile/events/websockets/lib/validate.js	`97.22% <0.00%> (-0.51%)`	⬇️
lib/plugins/aws/package/compile/functions/index.js	`97.03% <0.00%> (-0.22%)`	⬇️
lib/configSchema.js	`100.00% <0.00%> (ø)`
lib/utils/getCommandSuggestion.js	`100.00% <0.00%> (ø)`
lib/plugins/aws/invokeLocal/index.js	`69.66% <0.00%> (ø)`
lib/plugins/aws/provider/awsProvider.js	`93.11% <0.00%> (ø)`
lib/plugins/interactiveCli/initializeService.js	`100.00% <0.00%> (ø)`
...ib/plugins/aws/package/compile/events/sns/index.js	`100.00% <0.00%> (ø)`
...ib/plugins/aws/package/compile/events/sqs/index.js	`100.00% <0.00%> (ø)`
... and 5 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 372ce54...f6476e2. Read the comment docs.

medikoo

Thank you @thewizarodofoz that looks outstanding! I've found few last issues, otherwise it looks as what we need

medikoo · 2020-09-17T13:10:26Z

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

+            HTTPPort: { type: 'integer' },
+            HTTPSPort: { type: 'integer' },


Let's maybe set min 0 and max 65535

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

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

+            HTTPSPort: { type: 'integer' },
+            OriginKeepaliveTimeout: { type: 'integer', miminum: 1, maximum: 60 },
+            OriginProtocolPolicy: {
+              type: 'string',


With enum, type is ineffective

medikoo · 2020-09-17T13:11:25Z

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

+            OriginReadTimeout: { type: 'integer', miminum: 1, maximum: 60 },
+            OriginSSLProtocols: {
+              type: 'array',
+              items: { type: 'string', enum: ['SSLv3', 'TLSv1', 'TLSv1.1', 'TLSv1.2'] },


Doesn't seem to be addressed (we can remove type: 'string')

medikoo · 2020-09-17T13:17:24Z

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

+          required: ['OriginProtocolPolicy'],
+        },
+        DomainName: { type: 'string' },
+        Id: { type: 'string' },


I think we should not include Id, as we unconditionally generate it:

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

Line 149 in 6f392b3

Id: naming.getCloudFrontOriginId(originObj),

medikoo · 2020-09-17T13:25:58Z

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

+            { type: 'array', items: [{ const: 'GET' }, { const: 'HEAD' }] },
+            { type: 'array', items: [{ const: 'GET' }, { const: 'HEAD' }, { const: 'OPTIONS' }] },
+            {
+              type: 'array',
+              items: [
+                { const: 'GET' },
+                { const: 'HEAD' },
+                { const: 'OPTIONS' },
+                { const: 'PUT' },
+                { const: 'PATCH' },
+                { const: 'POST' },
+                { const: 'DELETE' },
+              ],
+            },


If understand we allow three exact collections. If it's the case then above doesn't meet that, as AllowedMethods: ["GET"] will pass.

I believe we can achieve needed validation through { enum: [["GET", "HEAD"], [... ], [...]] }

medikoo · 2020-09-17T13:26:17Z

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

+          oneOf: [
+            { type: 'array', items: [{ const: 'GET' }, { const: 'HEAD' }] },
+            { type: 'array', items: [{ const: 'GET' }, { const: 'HEAD' }, { const: 'OPTIONS' }] },
+          ],


medikoo · 2020-09-17T13:31:54Z

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

+        SmoothStreaming: { type: 'boolean' },
+        TrustedSigners: { type: 'array', items: { type: 'string' } },
+        ViewerProtocolPolicy: {
+          type: 'string',


No need for type

medikoo · 2020-09-17T13:33:55Z

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

+        },
+        isDefaultOrigin: { type: 'boolean' },
+        includeBody: { type: 'boolean' },
+        behavior: behaviorObjectSchema,


Let's ensure alphabetical order (when there's larger count of properties, it's then easier to find one)

medikoo · 2020-09-17T13:34:51Z

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

    this.serverless.configSchemaHandler.defineFunctionEvent('aws', 'cloudFront', {
      type: 'object',
+      properties: {
+        eventType: {
+          type: 'string',


Let's remove type here

medikoo

@thewizarodofoz that looks great! There's just one style nit that left addressed, see my comment

medikoo · 2020-09-22T07:49:53Z

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

+            OriginReadTimeout: { type: 'integer', miminum: 1, maximum: 60 },
+            OriginSSLProtocols: {
+              type: 'array',
+              items: { type: 'string', enum: ['SSLv3', 'TLSv1', 'TLSv1.1', 'TLSv1.2'] },


Doesn't seem to be addressed (we can remove type: 'string')

medikoo

Thank you @thewizarodofoz! That's outstanding!

ozwi added 4 commits September 15, 2020 17:42

schema

7b7a2a9

remove schema validation tests

2282c44

prettify

a3d1b8f

prettify

5703ee4

medikoo added cat/aws-event-cloudfront cat/config-schema enhancement labels Sep 16, 2020

medikoo suggested changes Sep 16, 2020

View reviewed changes

medikoo assigned thewizarodofoz Sep 16, 2020

behavior and origin schemas

2f5524c

thewizarodofoz requested a review from medikoo September 16, 2020 13:46

medikoo suggested changes Sep 17, 2020

View reviewed changes

ozwi added 7 commits September 22, 2020 10:29

ports min/max

e8ab079

remove type from enum

f1356df

remove Id

fb32776

fix allowed methods

283fb6c

fix cached methods

0c343c3

alphabetical order

20b58cf

prettify

7502dd4

thewizarodofoz requested a review from medikoo September 22, 2020 07:42

medikoo reviewed Sep 22, 2020

View reviewed changes

remove type from enum

f6476e2

thewizarodofoz requested a review from medikoo September 22, 2020 07:58

medikoo approved these changes Sep 22, 2020

View reviewed changes

medikoo merged commit 8943693 into serverless:master Sep 22, 2020

thewizarodofoz deleted the aws-cloudfront-event-schema branch September 22, 2020 15:16

jede mentioned this pull request Sep 30, 2020

Fix schema validation of CloudFront distribution config #8308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aws cloudfront event schema #8250

Aws cloudfront event schema #8250

thewizarodofoz commented Sep 15, 2020

medikoo left a comment

medikoo Sep 16, 2020

medikoo Sep 16, 2020

thewizarodofoz Sep 16, 2020

codecov-commenter commented Sep 16, 2020 •

edited

medikoo left a comment

medikoo Sep 17, 2020

medikoo Sep 17, 2020

medikoo Sep 17, 2020

medikoo Sep 22, 2020

medikoo Sep 17, 2020

medikoo Sep 17, 2020

medikoo Sep 17, 2020

medikoo Sep 17, 2020

medikoo Sep 17, 2020

medikoo Sep 17, 2020

medikoo left a comment

medikoo Sep 22, 2020

medikoo left a comment

		HTTPPort: { type: 'integer' },
		HTTPSPort: { type: 'integer' },

Aws cloudfront event schema #8250

Aws cloudfront event schema #8250

Conversation

thewizarodofoz commented Sep 15, 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

codecov-commenter commented Sep 16, 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

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

medikoo left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 16, 2020 •

edited