trek10inc / serverless-secrets

An opinionated tool for safely managing and deploying Serverless projects and their secrets.
Other
165 stars 27 forks source link

Using with Serverless Webpack #26

Open RishitKedia opened 6 years ago

RishitKedia commented 6 years ago

Hey there! 👋

This is great! Thank you for writing this plugin. 🙏

Well, I'm not able to get past something when using Serverless Secrets with Serverless Webpack.


I get the following warning from Webpack when bundling my code: 6:16-77 Critical dependency: the request of a dependency is an expression

It's due to the way the .serverless-secrets.json config file is required in this line: https://github.com/trek10inc/serverless-secrets/blob/6cf4af4f915a53a94c90d1d882d03fb5650cb5d7/client/index.js#L6


And, I get the following error when I run my function: Error: Cannot find module "."

It's because this is what my handler.js contains for the previous line after bundling:

const secrets = !(function webpackMissingModule() {
  var e = new Error("Cannot find module \".\"");
  e.code = 'MODULE_NOT_FOUND';
  throw e;
}())

Any thoughts on how this can be fixed? 😰

Thanks for your time! 😃

/cc @HyperBrain [Sorry to bother you, but maybe you can help us with this] 😛

HyperBrain commented 6 years ago

@RishitKedia No problem 😄

I think you can use the webpack file-loader to achieve the integration of the .serverless-secrets.json. Just handle that file with the file-loader and use a plain const secrets = require('./<whatever path>/serverless-secrets.json'); - maybe with the relative path you declared in the file-loader.

Internally, the compile will put the json file (named with a hash) into the build output and will automatically rewrite the require to fetch the file. So you do not need to even think about how to reference it, and you do not need to have any error handling - as the require will be resolved at compile time.

If you have to use the dynamic require by serverless-secrets, this approach may not work. Maybe then the secrets plugin should offer a method to use a loaded secrets json instead of requiring it by itself -> so you would use the file-loader, load the json and initialize the secrets plugin with it?

HyperBrain commented 6 years ago

A different approach might be to use webpack-plugin-copy to copy serverless-secrets.json to build output directory. Then the dynamic require from the plugin might be resolved correctly.

RishitKedia commented 6 years ago

@HyperBrain I tried both the file-loader and copy-webpack-plugin approaches with no luck.

From what I've read, it seems that Webpack does not like dynamic require()s, and it's better to use static paths. But, since the .serverless-secrets.json file is stored in my Serverless service directory (next to webpack.config.js and serverless.yml), the Secrets plugin has to dynamically require() the file using process.cwd() to get the directory of my service. I also tried to play with the context (using ContextReplacementPlugin) with no success so far.

I like your idea of the Secrets plugin offering a method to initialize the client with the file. So, I think we'll have to wait for the guys behind the plugin to tell us what they think about this. 🙂

Thanks for all your help, Frank. 🙏

HyperBrain commented 6 years ago

@RishitKedia Can you try with the PR I just opened? The trick is, to add the configuration file with the file-loader now and call client.init(require(.... the config file)) before using load() or loadByName() once in the handler file.

If that works, the maintainers here should have a look at it, as it enables the secrets plugin for all serverless-webpack users.

RishitKedia commented 6 years ago

@HyperBrain Thank you for taking out the time to work on this. 👍

I'm still getting the warning for the dynamic require(): Critical dependency: the request of a dependency is an expression

Also, if I remove the dynamic require(), I don't get the warning, but when I run my function with your fix, I get the following error: TypeError: Cannot read property '$global' of undefined

It points to this line, even though my .serverless-secrets.json file contains environments.$global.

HyperBrain commented 6 years ago

@RishitKedia I will have a look at that with a local test project later.

The undefined error is most likely a small confifuration issue with the JSON and the file loader.

For the expression problem, I have to check if the client file gets packed/bundled or not. If it gets bundled, webpack has the beforementioned problem because it cannot determine the dependencies (and thus emits the error). Maybe a solution would be to use the file-loader for the client index file too, to prevent webpack trying to extract any dependencies and evaluate the file.

azurelogic commented 6 years ago

Hi @HyperBrain and @RishitKedia,

Sorry for the long delay. Been super busy with other projects and finally got back to some maintenance.

@RishitKedia Have you managed to get @HyperBrain's solution working correctly? If so, is everything necessary documented in his PR or was there more that you needed to do? I'm working on getting this package out of beta ASAP and I'd like to integrate this if possible before that release. If timing doesn't work out, we can always bring it in under 3.1.0.

Thanks to both of you for your assistance!

RishitKedia commented 6 years ago

Hey, @azurelogic! 👋

I tried a few things but wasn't able to get @HyperBrain's PR working correctly since I'm not very experienced with Webpack.

Waiting for @HyperBrain to work his magic on his PR once he gets time to test it locally. 😉

Thanks for considering this! 🙏

HyperBrain commented 6 years ago

I'm testing right now locally. As soon as I succeed setting up a working configuration I'll write it here so that anyone can test if it really works in a real-world scenario.

HyperBrain commented 6 years ago

Hi there. I got a setup working. In general, I think this is the right way to go, but it may need some additional tweaks and/or small changes (e.g. add the hints to the README with this PR, etc.). The solution is straight forward without the need of any workarounds or complex configurations.

Introduction

I created a small test project that uses a stored SSM parameter to verify that it works. The parameter is named 'testsecret' and has the value 'myvalue' for testing purposes.

Upfront, this is the output of the test lambda as reference, and you see that it fetched the variable correctly from SSM and printed the value 'myvalue'.

START RequestId: f9332ad4-cdee-11e7-a9fc-05ee68a7f188 Version: $LATEST
2017-11-20T12:33:13.467Z    f9332ad4-cdee-11e7-a9fc-05ee68a7f188    myvalue
END RequestId: f9332ad4-cdee-11e7-a9fc-05ee68a7f188
REPORT RequestId: f9332ad4-cdee-11e7-a9fc-05ee68a7f188  Duration: 613.66 ms Billed Duration: 700 ms     Memory Size: 1024 MB    Max Memory Used: 38 MB

I documented all steps that have to be made below, including the service, the webpack config and a sample handler.

The setup (how to configure the handler and service)

The service

There are only a few requirements for the service definition. See the sample yaml snippet below to see how things have to be set.

You have to add the webpack and the secrets plugin to your serverless.yml, so that the secrets plugin precedes the webpack plugin. This is important because webpack relies on the files generated by the secrets plugin.

Define the environmentSecrets section as stated in the README.

Currently aws-sdk is mentioned as production dependency in the secrets plugin. With webpack that would lead to a packaging of the SDK. We do not want that as it only bloats the package and AWS Lambda already has a working SDK installed. So we tell the webpack plugin to exclude the aws-sdk.

It is important to use serverless-webpack@^4.0.0 as the forced exclude is only available there.

# serverless.yml
...
plugins:
  - serverless-secrets
  - serverless-webpack
...
provider:
  ...
  environmentSecrets:
    API_KEY: 'testsecret'
...
custom:
  webpackIncludeModules:
    forceExclude:
      - aws-sdk

The Webpack configuration

Because serverless-secrets is a development dependency and the actual client, that is used in our production handlers is contained in there, the easiest way to get (only) the client in, is to have it just bundled. This can be achieved by telling node-externals to include it, instead of marking it as external module. Nothing more has to be done to the webpack configuration file.

module.exports = {
  entry: slsw.lib.entries,
  target: 'node',
  externals: [
    nodeExternals({
      whitelist: [
        "serverless-secrets/client"
      ]
    })
  ],
  ...
}

The Lambda handler

With the changes in the PR we now can initialize the secrets plugin client with a preloaded .serverless-secrets.json. Because we bundle the client, we can just require the temporary config file in our handler. Then webpack will just bundle the client AND the config, so that the compiled handler now contains everything needed and does not have any external dependencies anymore.

I commented the new behavior in the sample handler below.

// Given: a secret named 'testsecret' is stored in SSM with value 'myvalue'
// Given: an environmentSecret named 'API_KEY' exists with a value of 'testsecret'

// This require will be removed by webpack, because it bundles the client now.
const secrets = require('serverless-secrets/client');
// We use the new (PR) init function here, to initialize the secrets plugin client with the 
// preloaded pregenerated JSON. With this require, webpack will automatically bundle the JSON
// and removes the needed dynamic require that would fail.
// This has to be a static path. The file is located in the root of the service. So you have to
// specify the proper backwards relative path from your handler directory.
secrets.init(require('./.serverless-secrets.json'));

module.exports.handler = function (event, context, callback) {
  const secretsPromise = secrets.load();
  secretsPromise.then(() => {
    // process.env.API_KEY now contains 'myvalue'
    console.log(process.env.API_KEY);
    callback(null, { statusCode: 200 });
  }).catch(err => {
    // handle errors here
    callback(null, { statusCode: 500, body: err.message });
  });
};

The output from the very top is exactly from this handler.

The package / deployment

You can check with serverless package that it works. However there is a warning but it can be ignored (see below).

$ node node_modules/serverless/bin/serverless deploy
Serverless: Generating Serverless Secrets Config
Serverless: Serverless Secrets beginning packaging process
Serverless: Writing .serverless-secrets.json
Serverless: Validating secrets
Serverless: Secrets validated
Serverless: Adding environment variable placeholders for Serverless Secrets
Serverless: Bundling with Webpack...
Time: 660ms
   Asset     Size  Chunks             Chunk Names
first.js  8.41 kB       0  [emitted]  first
   [0] C:/Projects/serverless/serverless-secrets/client/index.js 2.11 kB {0} [bu
ilt]
   [1] ./first.js 725 bytes {0} [built]
   [2] external "path" 42 bytes {0} [not cacheable]
   [3] external "lodash" 42 bytes {0} [not cacheable]
   [4] C:/Projects/serverless/serverless-secrets/lib/constants.js 68 bytes {0} [
built]
   [5] C:/Projects/serverless/serverless-secrets/lib/providers/aws.js 1.85 kB {0
} [built]
   [6] external "aws-sdk" 42 bytes {0} [not cacheable]
   [7] C:/Projects/serverless/serverless-secrets/client 160 bytes {0} [built]
   [8] ./.serverless-secrets.json 226 bytes {0} [built]

WARNING in C:/Projects/serverless/serverless-secrets/client/index.js
20:24-85 Critical dependency: the request of a dependency is an expression

The critical webpack warning emitted can be ignored, because with the PR the init function will use the argument given to init to initialize the configuration. Here is the code generated by webpack, where you see, that it works and is safe.

function init(config) {
  if (!secrets) {
    secrets = config || !(function webpackMissingModule() { var e = new Error("Cannot find module \".\""); e.code = 'MODULE_NOT_FOUND'; throw e; }());
  }
}

The function is compiled in a way that keeps the OR and thus never runs into the previously failing code part because the JSON is bundled and init is called from the handler at the very start. Maybe there is a way to get this (the warning and second part of the OR) removed completely, by using some webpack plugin (like the define plugin). This would be more a cosmetic change though. However, I will do some tests and maybe change the PR a bit, so that I can manage to remove the warning.

Please let me know, if it works for you, of if there are any additional issues that have to be solved. I will try to use it in one of our bigger projects too, to have some more reliable test results.

HyperBrain commented 6 years ago

@azurelogic While implementing and testing my PR I found a few things that might be improved. One of them is e.g. that the plugin introduces aws-sdk as dependency (which is by nature and due to the client which of course needs the SDK to access the SSM parameter store). But having it as production dependency of the plugin leads to the fact, that it will be a production dependency of the service (the user code) too. This is imo not very good, because every AWS service will try to exclude the SDK because it is already available on AWS Lambda. In my opinion it would be better if it was a peer dependency in the plugin, so that the Serverless project has to add it (but would just add it to its development dependencies to satisfy the peer dependency constraint). Otherwise the user has to take care of the exclusion by himself (either with Serverless or with the webpack plugin). What do you think about that? I can create a new issue (improvement) here - the "fix" should be trivial.

RishitKedia commented 6 years ago

@HyperBrain Hey, Frank, thanks for such a detailed explanation. 💯

But I'm still not able to make it work. 😭


  1. I'm still getting the error I had reported earlier when I run the function:

TypeError: Cannot read property '$global' of undefined

It points to this line, even though my .serverless-secrets.json file contains environments.$global.

You mentioned that it could be a small configuration issue with the JSON and the file-loader. Have you made any changes there? I'm loading the JSON using:

{
  test: /\.serverless-secrets.json$/,
  use: [{
    loader: 'file-loader',
    options: {
      name: '[name].[ext]'
    }
  }]
}

  1. I'm also having issues using serverless-webpack with Yarn Workspaces.

The bundled file is very huge (4 MB when used with Yarn Workspaces) compared to the same project when not used with Yarn Workspaces (25 kB).

It bundles all the dependencies even when using webpack-node-externals and webpackIncludeModules.


I've also reduced my project to the bare minimal, but I'm still getting the issues. 🤔

HyperBrain commented 6 years ago

I did not use the file-loader at all. Only the things I mentioned in the description. So the the json is just a plain require - nothing else. Remove the whole file-loader rule from the config as it is not needed at all. The only important thing is, that the require must contain the relative path to your service root, e.g.: require('../../../.serverless-secrets.json') in case the handlers are nested somewhere.

Regarding YARN: serverless-webpack currently completely relies on npm and uses that. I'm sure that's the reason why it does not package the minimum dependencies. I plan to add yarn support to serverless-webpack, so that you just can switch by setting some configuration variable in serverless.yml. But it will still take some time until I've that ready.

Did you set the forceExclude "aws-sdk" and switch to sls-webpack 4.0.0? The 4MB reminds me somehow of the size of the AWS SDK.

HyperBrain commented 6 years ago

This is the complete webpack config I used for testing:

const path = require('path');
const slsw = require('serverless-webpack');
const nodeExternals = require('webpack-node-externals');

module.exports = {
  entry: slsw.lib.entries,
  target: 'node',
  externals: [
    nodeExternals({
      whitelist: [
        "serverless-secrets/client"
      ]
    })
  ],
  module: {
    rules: [
      {
        test: /\.js$/,
        exclude: /node_modules/,
        use: [
          {
            loader: 'babel-loader'
          }
        ],
      }
    ]
  },
  output: {
    libraryTarget: 'commonjs',
    path: path.join(__dirname, '.webpack'),
    filename: '[name].js'
  }
};
RishitKedia commented 6 years ago

@HyperBrain

  1. Ah, I thought we still had to load the JSON with the file-loader. 😛

The error goes away, and a similar one crops up pointing to the same line: TypeError: Cannot read property 'split' of undefined


  1. Ah, okay, no problem, I'll wait until you get the Yarn thing done. :bowtie:

Do you want me to create a new issue for this on serverless-webpack?

And, yup, I did forceExclude aws-sdk, and I'm using v4.0.0 of serverless-webpack.

HyperBrain commented 6 years ago

Ok. That means for some reason your process.env._HANDLER is not set. But that should be "only" another configuration issue in one of the files. Can you check that the order of the plugins in serverless.yml is correct (like mentioned in the above description) and that the environmentSecrets object is set properly? The order is important as it defines the execution sequence in which the plugins are executed. secrets must be executed before webpack.

HyperBrain commented 6 years ago

... and yes. Feel free to create a feature request that demands the optional use of yarn instead of npm for the packaging.

RishitKedia commented 6 years ago

Yup, this is the order of the plugins:

plugins:
  - serverless-secrets
  - serverless-webpack
  - serverless-offline

And, environmentSecrets has also been set properly.

HyperBrain commented 6 years ago

That's strange. The _HANDLER environment variable is not set by the secrets plugin, nor is it set by the webpack plugin. It is just evaluated in the client (but that is completely unrelated to my fix). image It looks like someone sets this, or it is set in the AWS lambda environment. Did you try with offline, or did you try to run a deployed version? If offline, please try to deploy and check online if it works. This would state that the _HANDLER environment variable is only present on AWS lambda. Then it would be a bug in offline - the lambda environment faked there would be incomplete. I did my tests online, not with offline.

HyperBrain commented 6 years ago

I googled a bit and found out that this variable is set online only (in the AWS lambda environment). So I assume that serverless-secrets does not really work with serverless-offline yet, as offline does not set this variable.

HyperBrain commented 6 years ago

@azurelogic Can you confirm that?

RishitKedia commented 6 years ago

Ah, damn, I've been testing both online and offline, but this time I only tested offline. Yes, it works when deployed to AWS, finally! 😜 So, it's a bug in serverless-offline.

Thanks for your time, Frank. I really appreciate it. 🙏

HyperBrain commented 6 years ago

😄 Thanks for testing. So I think the PR is finally working, but I have to adapt the README accordingly to have it finished. Maybe I'll just commit my comment from above as section there.

azurelogic commented 6 years ago

@HyperBrain I made some assumptions about _HANDLER that are proving problematic. I discovered that env var while working on the Lambda Debugger project (I dumped some of the Lambda environment which showed me how they grab the handler). I didn't realize that other solutions wouldn't have filled that in. Fortunately, #30 might save the day, just need to be sure that it works in more complex scenarios. Thanks again for your contributions!

azurelogic commented 6 years ago

@HyperBrain This issue started because of the dynamic require which really was a hack to not have to do fs.readFileSync followed by a JSON.parse. I've switched to that route now to avoid similar issues in the future. I don't know if that simplifies or complicates integrating with serverless-webpack. Thoughts?

As for the _HANDLER problem, I'm looking into that next. I know that AWS sets it. Just need to find a solid solution to that problem.

HyperBrain commented 6 years ago

The PR with the init() solved the problem for serverless-webpack. The problem with webpack is, that any dedicated read from the client will require that the file is present. We just should check how that behaves. I can do that as soon as you have something ready. Removing the dynamic read is a good thing though, because it will remove the webpack warning (even if it is then done in an unused code branch). But then the messaging during the build is clean.

franciscocpg commented 6 years ago

This issue started because of the dynamic require which really was a hack to not have to do fs.readFileSync followed by a JSON.parse. I've switched to that route now to avoid similar issues in the future. I don't know if that simplifies or complicates integrating with serverless-webpack.

After this change I've updated @HyperBrain https://github.com/trek10inc/serverless-secrets/issues/26#issuecomment-345689648 a little bit.

The Webpack configuration

I've added WebpackPluginCopy to copy .serverless-secrets.json to build folder.

plugins: [
    new WebpackPluginCopy([
      { from: '.serverless-secrets.json' }
    ])
  ]

The Lambda handler

It's safe to remove this line now

secrets.init(require('./.serverless-secrets.json'));

The package / deployment

No more warnings here :pray:

============================

That's it. After this change we don't need to deal anymore with dynamic requires (through relative paths, :stuck_out_tongue_closed_eyes:) and with undesired package / deployment warnings when using serverless-webpack.