Skip to content
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.

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

Seclude CLI params and service config resolution from the core #8364

Closed
20 tasks done
medikoo opened this issue Oct 8, 2020 · 20 comments · Fixed by #9200
Closed
20 tasks done

Seclude CLI params and service config resolution from the core #8364

medikoo opened this issue Oct 8, 2020 · 20 comments · Fixed by #9200

Comments

@medikoo
Copy link
Contributor

medikoo commented Oct 8, 2020

Use case description

Ideally it should be straightforward to use Serverless Framework programmatically. Additionally clean separation of concerns will help significantly the maintenance and will allow to integrate Components engine into the repository.

Improvements towards separation of Packaging and Deployment phases has been moved to #8499

Proposed solution

Step by step let's seclude CLI params and service config resolution logic from a core and layout a new processing flow as follows:

  1. If CLI command is -v, output version info and abort
  2. If in service context:
    1. Resolve service configuration (with support for -c, --config CLI params), into plain not normalized in any way JSON structure). (If there's parsing error and it's not a CLI help request, crash with meaningful error)
    2. Resolve file, self and strToBool variable sources (but only those not depending on other resolvers)
    3. Ensure that provider and eventual provider.stage properties are fully resolved. (If there's a validation error and it's not a CLI help request, crash with meaningful error on provider being not resolved. Show deprecation and warning notice on provider.stage not being resolved)
    4. Load eventual extra env variables from .env files (If there's parsing error and it's not a CLI help request, crash with meaningful error)
    5. Resolve env and remaining file, self and strToBool variable sources (but only those not depending on other resolvers)
    6. Ensure eventual plugins property is fully resolved.
    7. Initialize plugins (at this stage plugins should have no access to service config or any meta):
      (If there's initialization error and it's not a CLI help request, crash with meaningful error)
      1. Register plugin config schema extensions
      2. Register plugin variable extensions
      3. Register commands and hooks
  3. If CLI help request. Display help and abort
  4. Parse CLI arguments (with respect to map of supported commands and options)
  5. If in service context:
    1. Resolve variables for all variable sources which do not depend on config properties
    2. Resolve all remaining variables in service config
  6. Run lifecycle events for CLI command

Additionally to have Framework CLI totally programatic, #1720 has to be addressed.

Implementation spec

Preliminary notes:

0.1 Unify process error handling
  1. Wrap all process logic with try/catch clause
  2. Configure handler which generally mirrors one at Error class (but presents simplified logic due to more generic approach). Place handler logic in lib/cli/handle-error.js (and ensure some tests for it)
  3. Ensure that uncaughtExceptions are handled with same error handler
  4. Remove lib/classes/Error.js logError utility
0.2 Generalize process execution span promise handling (as currently served by serverless.onExitPromise)
  1. Reflect execution span of introduced above try/catch clause in promise accessible at lib/cli/execution-span.js (module should export an unresolved promise and method (to be used internally) for resolving it)
  2. Assign lib/cli/execution-span.js promise to serverless.executionSpan (as it can be valuable for a plugins). Remove it's so far counterpart serverless.onExitPromise and in places it was used, refer to either promise exported by lib/cli/execution-span.js or serverless.executionSpan
  3. Move analytics.sendPending() to top of the try/catch clause
1.0 Seclude -v, --version CLI params handling
  1. At begin of the flow implement following:
    • If CLI command is sls -v [...] or sls --version [...] Generate simple version output which follows one implemented internally in CLI class. Place handling logic in lib/cli/eventually-output-version-info.js (and ensure some tests for it)
    • Abort the process (ensure no further process steps are pursued (but without hard exiting the process))
  2. Remove -v, --version option handling and recognition from CLI.js class
2.0 Resolve eventual service config file path (service context)
  1. Follow up with service context detection logic. It should be implemented in lib/cli/resolve-service-config-path.js and resemble logic we have now here:
    const getConfigFilePath = async (servicePath, options = {}) => {
    if (options.config) {
    const customPath = path.join(servicePath, options.config);
    return fileExists(customPath).then(exists => {
    return exists ? customPath : null;
    });
    }
    const jsonPath = path.join(servicePath, 'serverless.json');
    const ymlPath = path.join(servicePath, 'serverless.yml');
    const yamlPath = path.join(servicePath, 'serverless.yaml');
    const jsPath = path.join(servicePath, 'serverless.js');
    const tsPath = path.join(servicePath, 'serverless.ts');
    const [jsonExists, ymlExists, yamlExists, jsExists, tsExists] = await Promise.all([
    fileExists(jsonPath),
    fileExists(ymlPath),
    fileExists(yamlPath),
    fileExists(jsPath),
    fileExists(tsPath),
    ]);
    if (yamlExists) {
    return yamlPath;
    } else if (ymlExists) {
    return ymlPath;
    } else if (jsonExists) {
    return jsonPath;
    } else if (jsExists) {
    return jsPath;
    } else if (tsExists) {
    return tsPath;
    }
    return null;
    };
  2. Remove from internals any config file path resolution logic and ensure that above is handled as single point of truth:
2.1 Parse service config file source
  1. Follow up with service config content resolution (bare resolution with no normalization or vars resolution at this point). It should be implemented in lib/service-config/read-source.js, and resemble logic we have here:
    getServerlessConfigFilePath(serverless).then(configFilePath => {
    if (!configFilePath) return null;
    const fileExtension = path.extname(configFilePath);
    const isJSOrTsConfigFile = fileExtension === '.js' || fileExtension === '.ts';
    return (isJSOrTsConfigFile
    ? handleJsOrTsConfigFile(configFilePath)
    : readFile(configFilePath)
    ).then(config => {
    if (_.isPlainObject(config)) return config;
    throw new ServerlessError(
    `${path.basename(configFilePath)} must export plain object`,
    'INVALID_CONFIG_OBJECT_TYPE'
    );
    });
    })
    . if readServiceConfigSource crashes expose the error only if it's not CLI help request, otherwise behave as we're not in service context.
  2. Remove from internals any config source resolution:
2.2 Initial (partial) variables resolution

For that we would need to Implement new variables resolver with following modules:

lib/variables/resolve-variables-map.js

Function that takes serviceConfig as an input. Traverses it's properties and returns map of all properties which use variable syntax. Result map should expose all information needed for complete variables resolution without a need of repeated property parsing.

After we will fully override variable resolution that's currently in a framework (point 5.2), function should be configured to also override all serviceConfig properties which depend on variables with null values (technically we move all information to variables map, and remove it from serviceConfig. It's to ensure that eventual further serviceConfig processing in case of variable resolution errors is not affected by unresolved properties content)

Expected format of result map

const sep = "\0";

const exampleResultMap = {
  [`custom${sep}creds`]: {
    raw:
      '${file(../config.${opt:stage, self:provider.stage, "dev"}.json):CREDS}_${self:custom.foo}',
    meta: [
      // Start from last to first
      // If vars are part of a string, provide start and end locations
      // and unconditionally coerce result to string
      { start: 71, end: 89, sources: [{ source: 'self', address: { raw: 'custom.foo' } }] },
      {
        start: 0,
        end: 70,
        sources: [
          {
            source: 'file',
            param: {
              raw: '../config.${opt:stage, self:provider.stage, "dev"}.json',
              meta: [
                {
                  start: 10,
                  end: 50,
                  sources: [
                    { source: 'opt', address: { raw: 'stage' } },
                    { source: 'self', address: { raw: 'provider.stage' } },
                    { raw: 'dev' },
                  ],
                },
              ],
            },
            address: { raw: 'CREDS' },
          },
        ],
      },
    ],
  },
  [`layers${sep}hello${sep}path`]: {
    raw: '${self:custom.layerPath}',
    // If property value is entirely constructed with var
    // No start/end points need to be configured
    // In such case we also support any result type (no string coercion)
    variables: [{ sources: [{ source: 'self', address: { raw: 'custom.layerPath' } }] }],
  },
};

Note: In case of resolution from external files, new added content will need to have eventual variables resolved through same util

lib/variables/resolve-variables.js

Function that takes serviceConfig, variablesMap and variablesResolvers as an input.

variablesResolvers is expected to be a simple map with source type as a key (e.g. self, fileetc.) and function that takesserviceConfig` and eeventual param configured for resolver as arguments. Function may return result sync way or async via returned promise

There should be attempt to resolve every property.

  • If resolution succeed, resolved value should be assigned on serviceConfig object and variable reference removed from variablesMap.
  • if resolution for any would fail it should be reported by an error which trough code resembles why it failed (A. not supported source, B. invalid configuration, C. missing necessary input, D. external service error). Failed resolution attempt should be stored in variablesMap (in future processing, resolution should be reattempted only if fail was caused by A error, in other cases there should be no retry.

If there's any fail. Function crashes, and on it's error it should expose errorneusVariableKeys property with keys to each variable resolutions that failed.


Having above:

  1. Generate variables map
  2. Attempt to resolve file, self and strToBool variable sources (but only those not depending on other resolvers). If it fails ignore any not supported source errors. If there are other errors and it's not a CLI help request, in initial stage, ignore them, but after addressing 5.2 signal them with warning message and show a deprecation that with next major we will fail.
2.3 Ensure provider and provider.stage properties are resolved.
  1. Inspect variables map:
    • if provider property still depends on variable resolution, crash with meaningful error, that we cannot accept given form of configuration
    • if provider.stage property still depends on variable resolution. Show warning and deprecation, stating that it's not recommend to use variables at this property and that we will fail on that with next major
2.4 Ensure to load env variables from .env files

Follow up with resolution of environment variables from .env files (currently being implemented at #8413)

2.5 Further (partial) variables resolution

As in 2.1 step, attempt to resolve file, self, strToBool and env variable sources (but only those not depending on other resolvers). If it fails ignore any not supported source errors. If there are other errors and it's not a CLI help request in initial stage, ignore them, but after addressing 5.2 signal them with warning message and show a deprecation that with next major we will fail.

2.6 Ensure eventual plugins property is fully resolved.

Inspect variables map, if plugins property still depends on variable resolution, crash with meaningful error, that we cannot accept given form of configuration

2.7.0 Recognize help command
  1. Implement is help CLI command logic in lib/cli/is-help-command.js (it should follow cli.isHelpRequest logic but also recognize --help-components) and adapt it in internals:
2.7.1 Recognize commands which are independent of external plugins

Handling of those commands ideally should be totally secluded from Framework engine, still to not impose too timetaking refactor at this step let's simply mark them, to make further processing possible (having that addressed, let's open an issue calling for their seclusion)

  1. If CLI command is either plugin, login, logout or dashboard pass to Serverless constructor a shouldMutePluginInitializationErrors: true option, and internally assign t to _shouldMutePluginInitializationErrors property
  2. In pluginManager.resolveServicePlugins() Rely on serverles._shouldMutePluginInitializationErrors and remove pluginManager.pluginIndependentCommands property.
2.7.2 Initialize Serverless instance

(this will most likely lay out naturally and should not require any code changes)

Follow up with construction of Serverless instance and invoke of serverless.init()

3.0 If CLI help command show help and abort
  1. Implement display option help logic (to be used by various help variants) in lib/cli/help/options.js. It should take our common command configuration object, and resemble logic we have at cli.displayCommandOptions()
  2. Implement interactive CLI help logic in lib/cli/help/interactive.js. It should be a function that accepts an interactiveCLI command configuration and resembles logic we have at `cli.generateInteactiveCliHelp()
  3. Implement main CLI help logic in lib/cli/help/framework.js. It should be a function that accepts a loadedPlugins and resembles logic we have at cli.generateMainHelp() (note we should have CLI: Remove help --verbose option and improve general help output #8497 addressed at this point)
  4. Implement specific command help logic in lib/cli/help/command.js. It should be a function that accepts commandName and command arguments, and:
  5. if interactive CLI help request. Find InteractiveCli plugin, run lib/cli/help/interactive.js with its comand and abort
  6. If general (not command specific) help request, run lib/cli/help/framework.js with serverless.cli.loadedCommands
  7. If command help request, find given command in serverless.cli.loadedCommands
    1. If specified command is not found show warning and output general help
    2. Otherwise run lib/cli/help/command.js with resolved comman
  8. Remove following code:
4.0 Parse CLI arguments
  1. Having a map of all supported commands and options follow up with resolution of CLI arguments:
    • Parse CLI args with logic as in resolveCliInput
    • Ensure that all CLI args validation steps (aside of validateServerlessConfigDependency and assignDefaultOptions) as pursued in pluginManager.invoke are taken care of. Ideally if it's generalized, so can be also used to validate Components CLI input
    • Let's put it into lib/cli/parse-params.js
  2. Pass resolved commands and options to serverless.run() method. In context serverless.run() assign those properties onprocessedInput property
    • To not break things for external plugins we cannot just remove resolution of processedInput, that happens in serverless.init(). Still let's override there processedInput with getter that exposes a deprecation message if property is accessed at initialization phase (having that we will remove it next major)
    • In each internal plugin remove handling of second constructor option (CLI options) - (it'll also automatically address other important concern -> Ensure options as passed to plugins is not modified #2582). If for some reason reliance on CLI options seems crucial at implementation phase, then move its handling to initialize lifecycle hook (it's first lifecycle event propagated unconditionally). Access CLI options from serverless.processedInput (and treat it as read only)
    • Refactor pluginManager.validateOptions so it's eventual errors do not refer to CLI params (this method will now be effective only for programmatic usage)
    • Refactor pluginManager.validateServerlessConfigDependency so it's eventual errors do not refer to CLI usage (e.g. we should refer to service context and not to service directory)
    • Remove pluginManger.convertShortcutsIntoOptions as Framework will already be populated with resolved shortcuts
5.1 Resolve variables for all variable sources which do not depend on config properties

As in 2.1 step, attempt to resolve all variable sources which do not depend on config properties.

If it fails ignore any not supported source errors. If there are other errors in initial stage, ignore them, but after addressing 5.2 signal them with warning message and show a deprecation that with next major we will fail.

5.2 Resolve all remaining variables in service config

As in 2.1 step, attempt to resolve all remaining variables.

If it fails signal them with warning message and show a deprecation that with next major we will fail. Additionally:

  • Ensure that after resolving variables map (point 2.2) all service config properties configure through variables are preset to null
  • In any variable resolution step, convert errors ignoring to warnings with and deprecation that with next major we will fail.

Remove all variable resolution logic from Framework core

6.0 Run lifecycle events for CLI command

(this will most likely lay out naturally and should not require any code changes)

Follow up with serverless.run()


Progress summary:

  • 0.0.1 - Tests refactor: Move all unit tests to "test/unit" folder #8478 - Move all unit tests to test/unit folder
  • 0.0.2 - CLI: Remove help --verbose option and improve general help output #8497 - Remove CLI help --verbose option and improve general help output
  • 0.1.0 - Unify process error handling
  • 0.2.0 - Generalize process execution span promise handling
  • 1.0.0 - Seclude -v, --version CLI params handling
  • 2.0.0 - Resolve eventual service config file path (service context)
  • 2.1.0 - Parse service config file source
  • 2.2.0 - Initial (partial) variables resolution
  • 2.3.0 - Ensure provider and provider.stage properties are resolved.
  • 2.4.0 - Ensure to load env variables from .env files
  • 2.5.0 - Further (partial) variables resolution
  • 2.6.0 - Ensure eventual plugins property is fully resolved
  • 2.7.0 - Recognize help command
  • 2.7.1 - Recognize commands which are independent of external plugins
  • 2.7.2 - Initialize Serverless instance
  • 3.0.0 - If CLI help command show help and abort
  • 4.0.0 - Parse CLI arguments
  • 5.1.0 - Resolve variables for all variable sources which do not depend on config properties
  • 5.2.0 - Resolve all remaining variables in service config
  • 6.0.0 - Run lifecycle events for CLI command
@pgrzesik
Copy link
Contributor

pgrzesik commented Dec 7, 2020

Hello @medikoo 👋 I've started thinking about this refactoring, starting with the very first step: 0.1 Unify process error handling and I'd like to ensure that my thinking is correct and to clarify a few things.

  1. By Wrap all process logic with try/catch clause, do you have in mind wrapping the whole highlighted logic with the following approach?
(async () => {
  try {
    ...
  } catch {
  ...
  }
})()
  1. What do you exactly mean by simplified logic/generic approach to that handler? What should be simplified - that is not clear to me (yet)?
  2. How do you imagine still supporting enterpriseErrorHandler in that solution? By nesting try/catch clauses?

Thanks in advance 🙇

@medikoo
Copy link
Contributor Author

medikoo commented Dec 8, 2020

@pgrzesik great thanks for asking. This issue was not specified in deep detail, as we planned to handled that internally (intentionally I didn't put "help wanted" label at ths point), as some parts are challenging, and it needs to be done carefuly with other aspects in mind (like future components adaptation)

I've also actually also started work on my side on that (but didn't place PR yet) :)

Anyway let me answer your questions:

By Wrap all process logic with try/catch clause, do you have in mind wrapping the whole highlighted logic with the following approach?

It's about covering most of Framework CLI logic with async try/catch clause (yes, as you show in your snippet) and handle any error with unified handler.

What do you exactly mean by simplified logic/generic approach to that handler?

I think I envisioned a simpler output, but as I was implementing this, I didn't go any further than refactoring this logic so it works as standalone. I guess in this initial phase, there's not much else to simplify (probably later when will want to cover also Components with same handler, some output will have to be updated)

How do you imagine still supporting enterpriseErrorHandler in that solution?

At this point I would leave it working same way. I would just not mute original error if handler crashes on it's own (I don't think that current logic is right)

@pgrzesik
Copy link
Contributor

pgrzesik commented Dec 8, 2020

Thanks for a comprehensive explanation @medikoo and sorry - I've missed the fact that this ticket didn't have "help wanted" label 🙇

@neverendingqs
Copy link
Contributor

neverendingqs commented Mar 1, 2021

Ah also, if file loads first before env, then technically users can do whatever they want by loading dotenv in a JavaScript config file as long as they import at least one variable from that file. That could be an immediate workaround for anyone on v3.

@medikoo
Copy link
Contributor Author

medikoo commented Mar 1, 2021

if file loads first before env

Technically it's more that all properties are resolved at once, and if property which is behind variable depends on the other behind variable, then the other is ensured to be fully resolved before we attempt to resolve first one.

@neverendingqs
Copy link
Contributor

@neverendingqs note that we're open to offer any help needed to adapt the plugin to new resolver.

Hi @medikoo (cc @pgrzesik). If you have a chance, I would appreciate your feedback on doing something like this to support more complicated dotenv workflows in Serverless, to make sure I am understanding the new variables engine correctly:

I think this is the best way forward into serverless>=3.0.0, instead of continuing support of serverless-dotenv-plugin.

@neverendingqs
Copy link
Contributor

I'm not sure if this should go in its own issue, but thoughts on .env files (2 iv) being resolved before file (2 ii)? That would allow environment variables defined in .env be used in files (e.g.

configs.js

module.exports = async ({ options, resolveConfigurationProperty }) => {
  const region = options.region
    || process.env.REGION
    || 'us-east-1;
  ...
}

.env

REGION=us-east-2

).

@medikoo
Copy link
Contributor Author

medikoo commented Mar 15, 2021

@neverendingqs I believe we fixed issue you were confused by, at #9110

In general there's no order in which variables are resolved. All are resolved at once, and if depedency is detected, the dependent waits for dependency.

In case were you were approaching error, problem was that at initial resolution phase we assume env variables not being fulfilled (due to eventual .env not being loaded yet), and it's the reason JS function resolver crashed.
Bug on our side was that we exposed that crash. Proper behavior is to silently catch it, and re-run resolver after .env is loaded. #9110 puts that on track.

@neverendingqs
Copy link
Contributor

Thanks! I understand what is happening better now, but am still unsure how the example I provided above will resolve with the changes in #9110.

If there are no ${env:REGION} references in serverless.yml, will region resolve to us-east-1 or us-east-2? I suspect it can resolve to us-east-1 today because process.env.REGION won't cause an error.

Having it call dotenv to load all environment variables in .env first is handy because we can then read and/or manipulate environment variables in JavaScript files, either for making decisions on overrides like in the example or to parse the value first (e.g. convert a CSV string into an array).

.env

REGION=us-east-2

configs.js

module.exports = async ({ options, resolveConfigurationProperty }) => {
  const region = options.region  // Not provided in `options`
    || process.env.REGION        // Defined in `.env`
    || 'us-east-1';              // Default if not found in either `options` or `.env`

  // Convert an env var CSV string into an array before returning
  const statements = (process.env.ALLOWED_PRINCIPALS || []).map(...);
  ...
}

@medikoo
Copy link
Contributor Author

medikoo commented Mar 15, 2021

If there are no ${env:REGION} references in serverless.yml, will region resolve to us-east-1 or us-east-2?

AWS Region in a framework is resolved as:

  • If --region option was passed with a CLI command use it
  • If provider.region was set in service configuration use it
  • Use us-east-1

No environment variable influences that resolution.

Still you've pointed one aspect that's currently missed when JS function resolvers are in place.

Indeed when such resolver is invoked first time, .env files are not loaded yet, and currently there are no clear means paved on how to abort such invocation, so it's re-triggered after .env is loaded (technically it can be hacked by throwing an error with MISSING_VARIABLE_DEPENDENCY code, but that's dirty).

I've proposed on how to solve it at #9112 and it's not challenging to do. We will implement that shortly, as definitely it's needed to consider JS resolvers functionality complete.

@medikoo
Copy link
Contributor Author

medikoo commented Mar 18, 2021

@neverendingqs experience with JS function resolvers should be improved with next version, as we've just merged this: #9140

@neverendingqs
Copy link
Contributor

Thanks!

it looks like dotenv is loaded after variable resolution is attempted:

  • resolverConfiguration = {
    servicePath: process.cwd(),
    configuration,
    variablesMeta,
    sources: {
    env: require('../lib/configuration/variables/sources/env'),
    file: require('../lib/configuration/variables/sources/file'),
    opt: require('../lib/configuration/variables/sources/opt'),
    self: require('../lib/configuration/variables/sources/self'),
    strToBool: require('../lib/configuration/variables/sources/str-to-bool'),
    },
    options,
    fulfilledSources: new Set(['file', 'self', 'strToBool']),
    propertyPathsToResolve: new Set(['provider\0stage', 'useDotenv']),
    };
    await resolveVariables(resolverConfiguration);
  • // Load eventual environment variables from .env files
    await require('../lib/cli/conditionally-load-dotenv')(options, configuration);

Is this because we need to resolve useDotenv first before we can determine if we should use dotenv?

Could we avoid a resolution of env variables if we instead loaded environment variables from dotenv first? As a side effect, it would also resolve the use case I described in #8364 (comment) and #8364 (comment).

@medikoo
Copy link
Contributor Author

medikoo commented Mar 18, 2021

Is this because we need to resolve useDotenv first before we can determine if we should use dotenv?

Yes, and we need to resolve stage (which depends on provider.stage) to be able to determine eventual .env.<stage> filename.

Could we avoid a resolution of env variables if we instead loaded environment variables from dotenv first?

Why? What exactly problem will it solve? Note that .env won't, override already set environment variables, so it's safe to resolve those already found in environment.

@neverendingqs
Copy link
Contributor

Why? What exactly problem will it solve? Note that .env won't, override already set environment variables, so it's safe to resolve those already found in environment.

Using the below as an example:

module.exports = async ({ options, resolveConfigurationProperty }) => {
  // Convert an env var CSV string into an array of e.g. IAM statements before returning
  const statements = (process.env.ALLOWED_PRINCIPALS || [])
    .split(',')
    .map(principal => ({ ... } );
  return { statements };
}

If ALLOWED_PRINCIPALS is never set in the system and only defined in dotenv we will always return

{ statements: [] }

today. This means we can't use different .env.{stage} files to set different ALLOWED_PRINCIPALS values.

Note: this is just a use case I would be interested in, but totally understandable if it's not one that will be supported.

@medikoo
Copy link
Contributor Author

medikoo commented Mar 18, 2021

@neverendingqs with next release JS function resolver will be invoked before .env files are loaded, only if it plays part in resolution of either provider.stage or useDotenv properties. Otherwise it'll be invoked after .env files are loaded. Just that I believe will fix your use case

And in case JS function resolver plays part of resolution of provider.stage or useDotenv then obviously it needs to be called before .env files are loaded, but then it also should not internally rely on env vars, as that creates an obvious conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants