iframe-proxy

preshetin · 2020-02-13T13:50:00Z

What did you implement

This PR implements the validation of the essential parts of serverless.yml config backed by AJV.

This validation architecture consists of one schema that is dynamically extended by plugins (both internal and external). After the schema is extended by plugins, it is used to perform the validation of the user's config.

This PR implements validation engine with relaxed schema. Fine-tuning schema parts is expected to be covered by other PR's.

Closes #6562

How can we verify it

This serverless.yml file contains errors which this PR should log to the console.

# try to use invalid schema name: '1first-digit-is-not-allowed'
service: some-service

# plugins:
#   - newEventPlugin

provider:
  name: aws
  stage: dev

functions:
  someFunc:
    handler: handler.main
    events:
      # 'httpApii' method is invalid so it logs an error
      - httpApii: get foo

Todos

Useful Scripts

npm run test:ci --> Run all validation checks on proposed changes
npm run lint:updated --> Lint all the updated files
npm run lint:fix --> Automatically fix lint problems (if possible)
npm run prettier-check:updated --> Check if updated files adhere to Prettier config
npm run prettify:updated --> Prettify all the updated files

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

Fixes serverless#6562

See serverless#7203

preshetin · 2020-02-13T15:03:25Z

medikoo

@preshetin Thanks for taking a start on that.

As it's a sensitive and significant feature, I think before jumping into an actual implementation, it'll be good to agree on some basic implementation spec.

Let me outline, what I think of it:

Where validation logic should be attached?

I believe that validation should not be configured as a plugin but be a part of a core (so be incorporated into engine which is also responsible for load of config, resolving variables, intiializing plugins). It should be considered as a core feature.

When validation should happen?

Ideally validation should happen after all plugins were initialized and variables resolved but before life-cycle engine jump in.

Technically it's when current service.validate() happens:

serverless/lib/classes/Service.js

Lines 174 to 202 in 7abb23e

    
           validate() { 
        
             _.forEach(this.functions, (functionObj, functionName) => { 
        
               if (!_.isArray(functionObj.events)) { 
        
                 throw new ServerlessError( 
        
                   `Events for "${functionName}" must be an array, not an ${typeof functionObj.events}` 
        
                 ); 
        
               } 
        
             }); 
        
             const provider = this.serverless.getProvider('aws'); 
        
             if (provider) { 
        
               const stage = provider.getStage(); 
        
               this.getAllFunctions().forEach(funcName => { 
        
                 _.forEach(this.getAllEventsInFunction(funcName), event => { 
        
                   if (_.has(event, 'http') && !validAPIGatewayStageNamePattern.test(stage)) { 
        
                     throw new this.serverless.classes.Error( 
        
                       [ 
        
                         `Invalid stage name ${stage}:`, 
        
                         'it should contains only [-_a-zA-Z0-9] for AWS provider if http event are present', 
        
                         'according to API Gateway limitation.', 
        
                       ].join(' ') 
        
                     ); 
        
                   } 
        
                 }); 
        
               }); 
        
             } 
        
             return this; 
        
           }

Which already tries to do some validation which should we back with schema.

I would replace this logic with new schema validation, and ensure upfront we cover the validation it's covered now.

What objects should validation cover?

Some problem is that current config handling is not clean. We do not have one object with which we work, but we span config parts on our service instance here:

serverless/lib/classes/Service.js

Lines 98 to 131 in 7abb23e

    
           if (_.isObject(serverlessFile.service)) { 
        
             that.serviceObject = serverlessFile.service; 
        
             that.service = serverlessFile.service.name; 
        
           } else { 
        
             that.serviceObject = { name: serverlessFile.service }; 
        
             that.service = serverlessFile.service; 
        
           } 
        
           that.app = serverlessFile.app; 
        
           that.tenant = serverlessFile.tenant; 
        
           that.org = serverlessFile.org; 
        
           that.custom = serverlessFile.custom; 
        
           that.plugins = serverlessFile.plugins; 
        
           that.resources = serverlessFile.resources; 
        
           that.functions = serverlessFile.functions || {}; 
        
           // merge so that the default settings are still in place and 
        
           // won't be overwritten 
        
           that.provider = _.merge(that.provider, serverlessFile.provider); 
        
           if (serverlessFile.package) { 
        
             that.package.individually = serverlessFile.package.individually; 
        
             that.package.path = serverlessFile.package.path; 
        
             that.package.artifact = serverlessFile.package.artifact; 
        
             that.package.exclude = serverlessFile.package.exclude; 
        
             that.package.include = serverlessFile.package.include; 
        
             that.package.excludeDevDependencies = serverlessFile.package.excludeDevDependencies; 
        
           } 
        
           if (that.provider.name === 'aws') { 
        
             that.layers = serverlessFile.layers || {}; 
        
           } 
        
           that.outputs = serverlessFile.outputs;

and then work with that. Variables resolution is made on service instance.

Ideally after Variables resolution is made. I think we should prepare a plain object that derives from service as follows:

const configToValidate = {
  service: service.serviceObject,
  app: service.app,
  org: service.org,
  custom: service.custom,
  plugins: service.plugins,
  resources: service.resources,
  functions: service.functions.
  provider: service.provider,
  package: service.package,
  layers: service.layers,
  outputs: service.outputs
}

And validate that.

@pmuens what are your thoughts on that?

medikoo · 2020-02-14T00:22:26Z

The logs indicate that the latest version of @hapi/joi uses the object spread operator which is not supported by Node 6.

For now just follow with Node v6. Hopefully we'd be able to drop support for it shortly

Undo serverless#7203

preshetin · 2020-02-17T13:14:12Z

@medikoo thank you for this guidance. Next time I'll try to start by gathering feedback first before submitting a PR.

I agree with all your points. It would require slight adjustments to this PR (like moving validation logic from package plugin to core Service class) which I am ready to implement.

Also, do you agree with the proposed multi-schema approach? Using many small schemas would allow plugins to easily extend any piece of the schema. Otherwise, I don't think that extending a branch in a big tree-like schema is possible in @hapi/joi lib.

Would you like me to implement changes, or it is better to wait for @pmuens feedback?

Also, I assume we keep sticking with Joi lib, although it does not support Node v6. Hopefully, Node v6 support will be dropped by the time this PR is merged.

medikoo · 2020-02-18T01:51:22Z

I assume we keep sticking with Joi lib, although it does not support Node v6. Hopefully, Node v6 support will be dropped by the time this PR is merged.

I think we can safely assume it's a v2 feature (in that case we may even crash on errors). We plan to rollout v2 in less than a month I think. So it shouldn't be blocked for too long.

Also, do you agree with the proposed multi-schema approach?

I think we need a validation tool that allows to easily in generic way extend base schema (e.g. define a new property on some object which is a deep nested property in global schema).

So we don't have to upfront define those eventual extension points, as otherwise we can easily run into maintenance issues in a future (it's likely that we will need more and more customization options on the way).

I've outlined my concerns at the bottom of description here #6562

Let's continue discussion there (I've also pinned that issue, which I cannot do with PR, so hopefully we get additional feedback from community). After we agree over there on implementation aspects, let's just continue work on implementation over here

pmuens

Great idea! I was always looking for a formal spec for serverless.yml. Ideally we'd put version: 1 or something like that in serverless.yml so that we can have multiple validation schemas (and therefore version serverless.yml).

Only question I have is whether this breaks custom plugins. The main problem is that custom plugins can extend the serverless object in whatever way they want. The "official" documentation says that only the custom property should be used by plugin authors but AFAIK this is not widely known.

medikoo · 2020-07-31T15:53:34Z

@preshetin I've updated the implementation with new version of normalizeAjvErrors.js, I hope you're not upset I've rewritten it from scratch, it was easier for me to approach it without any baggage, especially that our points of view were slightly different.

If you have opportunity please take a look into it, I've tried my best in documenting logic. I'd like to be sure I didn't remove any valuable functionality which was covered in your version.

I've also changed slightly approach in unspecified provider validation - in all (but "off") cases it's just dedicated warning reported.

I've also already created github issues to cover schemas for all config parts which we left not specified fully, and provided links to corresponding issues in code.

I've tested it, and it seems to work really well, would love to publish it on monday! :)

preshetin · 2020-08-01T17:43:28Z

@medikoo I'm not upset at all. In your place, I'd do the same as it is often easier to redo from scratch than understanding someone's logic.

I tested it and it looks pretty good. The new way of showing errors under one list feels more uniform.

One small regression I've noticed. In case of enum error type, the list of allowed values is not shown. I remember you wanted to have it from the very start. To verify it, try setting configValidationMode to warnn. The array of allowed values can be found inside the initial AJV error at params.allowedValues.

When I have time for open source, I'd be happy to jump into one of those numerous issues on fine-tuning other schema parts.

I don't want to be too cheerful about the release because many times before it was postponed. In case this happens, I hope it will go smoothly.

medikoo · 2020-08-03T13:16:56Z

preshetin added 15 commits January 4, 2019 05:02

docs: Ensure to have updated docs with recent changes

bbf856a

feat: Explicitely specify provider name when defininig event schema

b9f1623

feat: Configuration validation backed by schema

f64652c

Fixes serverless#6562

test: Add missing tests for validation against schema

bd1b5c8

fix: Remove code related to 1st validation phase (misunderstood)

d4dbd2d

fix: Ensure to throw error when using variables in plugins

2ee1518

See serverless#7203

refactor: Clean up comments accidentally added

b2e7c92

docs: Extending validation schemas by plugins

5a99d37

fix: Proper validation for plugins param in service

318578e

feat: Validation for service name

41ee24a

fix: More relaxed validation while schema is incomplete

dae1770

fix: Remove potential breaking changes

f92276b

style: Prettier on .md file

0753ba0

fix: Ensure to validate against awsService schema

15477e9

test: Fix failing test

b5b990f

preshetin requested a review from medikoo February 13, 2020 15:03

medikoo reviewed Feb 13, 2020

View reviewed changes

medikoo assigned preshetin Feb 13, 2020

medikoo added the pr/in-progress label Feb 13, 2020

refactor: Remove validation before variables resolution

5997cb4

Undo serverless#7203

preshetin requested a review from medikoo February 17, 2020 13:15

medikoo added help wanted needs feedback and removed help wanted needs feedback labels Feb 18, 2020

pmuens reviewed Feb 18, 2020

View reviewed changes

medikoo added 5 commits July 31, 2020 14:39

Fix configSchema tests

6749525

Improve not specified provider notification

c72311c

Fix tests setup

6b6801b

Fix tests up to recent update

079833d

Unify tests convention

e0aa821

medikoo mentioned this pull request Jul 31, 2020

Cleanup for schema validation #8013

Merged

Merge branch 'master' into config-schema-validation

dd19eea

medikoo added cat/config-schema cat/dx labels Jul 31, 2020

medikoo added 2 commits July 31, 2020 17:25

Document continuity PR's

1c4bdf5

Fix test so it doesn't collide with "app" as defined by the plugin

29c1743

Improve TODO comment

a35ca36

medikoo added 8 commits August 3, 2020 14:13

Define "awsArn" type

2354e17

Fix dataPath groups resolution

99d9ca2

Fix anyOf type error check

fa35da0

Cover with tests deep anyOf case

1d61ee0

Increase tests coverage

441a7ed

Ensure to list possible values for enum type

44a6883

Improve enum listing

e496520

Improve configuration error notes

615a584

medikoo approved these changes Aug 3, 2020

View reviewed changes

medikoo merged commit 268f714 into serverless:master Aug 3, 2020

This was referenced Aug 5, 2020

Config schema: Define AWS "provider" properties (top level only) #8016

Closed

fix(Config Schema): validate resourcePolicy is array #8051

Merged

jiangzhuo mentioned this pull request Aug 28, 2020

Getting error in deployment while using serverless-lambda-edge-pre-existing-cloudfront plugin serverless-operations/serverless-lambda-edge-pre-existing-cloudfront#9

Closed

medikoo mentioned this pull request Oct 16, 2020

"Cannot read property 'LogicalResourceId' of null" (export in use) #3589

Closed

sthuber90 mentioned this pull request Feb 12, 2021

Give warning on invalid event definition #5916

Closed

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Config schema validation#7335

Config schema validation#7335
medikoo merged 130 commits into
serverless:masterfrom
preshetin:config-schema-validation

preshetin commented Feb 13, 2020 •

edited

Loading

Uh oh!

preshetin commented Feb 13, 2020

Uh oh!

medikoo left a comment •

edited

Loading

Uh oh!

medikoo commented Feb 14, 2020

Uh oh!

preshetin commented Feb 17, 2020 •

edited

Loading

Uh oh!

medikoo commented Feb 18, 2020 •

edited

Loading

Uh oh!

pmuens left a comment •

edited

Loading

Uh oh!

medikoo commented Jul 31, 2020

Uh oh!

preshetin commented Aug 1, 2020

Uh oh!

medikoo commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	validate() {
	_.forEach(this.functions, (functionObj, functionName) => {
	if (!_.isArray(functionObj.events)) {
	throw new ServerlessError(
	`Events for "${functionName}" must be an array, not an ${typeof functionObj.events}`
	);
	}
	});

	const provider = this.serverless.getProvider('aws');
	if (provider) {
	const stage = provider.getStage();
	this.getAllFunctions().forEach(funcName => {
	_.forEach(this.getAllEventsInFunction(funcName), event => {
	if (_.has(event, 'http') && !validAPIGatewayStageNamePattern.test(stage)) {
	throw new this.serverless.classes.Error(
	[
	`Invalid stage name ${stage}:`,
	'it should contains only [-_a-zA-Z0-9] for AWS provider if http event are present',
	'according to API Gateway limitation.',
	].join(' ')
	);
	}
	});
	});
	}

	return this;
	}

	if (_.isObject(serverlessFile.service)) {
	that.serviceObject = serverlessFile.service;
	that.service = serverlessFile.service.name;
	} else {
	that.serviceObject = { name: serverlessFile.service };
	that.service = serverlessFile.service;
	}

	that.app = serverlessFile.app;
	that.tenant = serverlessFile.tenant;
	that.org = serverlessFile.org;
	that.custom = serverlessFile.custom;
	that.plugins = serverlessFile.plugins;
	that.resources = serverlessFile.resources;
	that.functions = serverlessFile.functions \|\| {};

	// merge so that the default settings are still in place and
	// won't be overwritten
	that.provider = _.merge(that.provider, serverlessFile.provider);

	if (serverlessFile.package) {
	that.package.individually = serverlessFile.package.individually;
	that.package.path = serverlessFile.package.path;
	that.package.artifact = serverlessFile.package.artifact;
	that.package.exclude = serverlessFile.package.exclude;
	that.package.include = serverlessFile.package.include;
	that.package.excludeDevDependencies = serverlessFile.package.excludeDevDependencies;
	}

	if (that.provider.name === 'aws') {
	that.layers = serverlessFile.layers \|\| {};
	}

	that.outputs = serverlessFile.outputs;

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

preshetin commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What did you implement

How can we verify it

Todos

Uh oh!

preshetin commented Feb 13, 2020

Uh oh!

medikoo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Where validation logic should be attached?

When validation should happen?

What objects should validation cover?

Uh oh!

medikoo commented Feb 14, 2020

Uh oh!

preshetin commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medikoo commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmuens left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo commented Jul 31, 2020

Uh oh!

preshetin commented Aug 1, 2020

Uh oh!

medikoo commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

preshetin commented Feb 13, 2020 •

edited

Loading

medikoo left a comment •

edited

Loading

preshetin commented Feb 17, 2020 •

edited

Loading

medikoo commented Feb 18, 2020 •

edited

Loading

pmuens left a comment •

edited

Loading