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

AJV Schema should accept useDotenv=false #8566

Closed
jer-sen opened this issue Dec 1, 2020 · 77 comments · Fixed by #10003
Closed

AJV Schema should accept useDotenv=false #8566

jer-sen opened this issue Dec 1, 2020 · 77 comments · Fixed by #10003

Comments

@jer-sen
Copy link

jer-sen commented Dec 1, 2020

In version 2.14 configSchema.jscontains useDotenv: { const: true }, but falseis a valid value cf https://www.serverless.com/framework/docs/providers/aws/guide/serverless.yml/

Also deprecation warning LOAD_VARIABLES_FROM_ENV_FILES should not be triggered when useDotenv=false.

@medikoo
Copy link
Contributor

medikoo commented Dec 1, 2020

@jer-sen Technically we support only true, as we want to remove this setting starting with new major (without ability to turning off .env auto load behavior)

In serverless.yml we list it to document it, but by convention we also show default setting, false is to kind of express the default (still real default is lack of this setting, technically we could express ith with null)

@jer-sen
Copy link
Author

jer-sen commented Dec 2, 2020

Ok I will find an other file extension for my environment files.
Anyway, it's quite dangerous to automatically upload .env files (often containing sensitive data) by default. Maybe you should reconsider the default behaviour change.

@medikoo
Copy link
Contributor

medikoo commented Dec 2, 2020

Ok I will find an other file extension for my environment files.

Why exactly do you have them, what's the purpose?

Anyway, it's quite dangerous to automatically upload .env files

That's a very good point, it was overlooked that they're packaged, we'll provide a patch shortly

@jer-sen
Copy link
Author

jer-sen commented Dec 2, 2020

I use one locally to configure environment variables (loaded with dotenv npm module). But I prefer not to have my production or dangerous (if misconfigured) variables stored and editable on my computer so, since I have only few lambdas, I've manually set my lambda environment variables using AWS console.
I'm open to any idea to better manage all this...

@medikoo
Copy link
Contributor

medikoo commented Dec 2, 2020

I use one locally to configure environment variables (loaded with dotenv npm module)

Serverless Framework loads variables only from .env files in service folder, and they're not exposed to lambdas (not published to AWS), they're purely for Serverless Node.js process.

@jer-sen
Copy link
Author

jer-sen commented Dec 2, 2020

Ok. So there is no security issue 😃
I'll stay with my workflow, just with a new file extension not used by serverless.

@medikoo medikoo assigned medikoo and unassigned jer-sen Dec 2, 2020
@takeda
Copy link
Contributor

takeda commented Dec 5, 2020

@medikoo I use .env with direnv and nix to set up my dev environment.

Direnv takes care of setting up my environment correctly. Since serverless already can use environment variable, this functionality is completely redundant to me. And might possibly cause some weird hard to debug issues. Please reconsider adding option to disable this and possibly not have it enabled by default.

I think anyone who uses .env/.envrc probably already has some kind of tool that sets environment when entering the directory.

@medikoo
Copy link
Contributor

medikoo commented Dec 5, 2020

@takeda thanks for input. What exactly issues do you envision? In my understanding worst case scenario is that Framework will just set some environment variables to same values that Direnv written (so will not introduce any effective change)

@takeda
Copy link
Contributor

takeda commented Dec 5, 2020

@medikoo it's just the magical behavior that I'm afraid of, since it can create some unexpected behavior that might be harder to trubleshoot.

I'm assuming those variables will be accessible through ${env:...}.

So for example I have SOMETHING=1 set in .env, and that's what I normally want to use, but I want to run command with different value so I do:

SOMETHING=2 sls deploy ...

But sls uses the value from .env.

A variation of it is also, that it uses .envrc which is more powerful than .env, since it can contain shell statements. By default direnv ignores .env unless you place dotenv in .envrc. I might purposefully remove dotenv when testing something and then be surprised that those variables are still accessible.

Most people who use .env already use tooling that understand these files. It's good too have programs follow the principle of least astonishment, to avoid strange behavior.

@medikoo
Copy link
Contributor

medikoo commented Dec 7, 2020

So for example I have SOMETHING=1 set in .env, and that's what I normally want to use, but I want to run command with different value so I do:
But sls uses the value from .env.

@takeda Actuallysls won't override any existing environment variables, with values from .env (sorry I've put wrong conclusion in previous comment)

Check https://github.com/motdotla/dotenv#what-happens-to-environment-variables-that-were-already-set. We follow all recommended practices of dotenv package, and do not tweak it's defaults in any way

@mnapoli
Copy link
Contributor

mnapoli commented Dec 18, 2020

To chime in here: I use .env with all sorts of frameworks (e.g. Symfony and Laravel) to configure applications running locally. It's a nicer alternative to setting real environment variables for local development.

But on Lambda, we have environment variables and we can set them up in serverless.yml, I don't see the point of reading .env files. If anything is a bit sensitive, SSM or Secrets Manager is here for that. I'm really afraid of whatever side effects this could have when this magic lands in the next major version (i.e. dev variables being deployed when they shouldn't be).

Maybe this could be an opt-in behavior? E.g. something like:

service: app

provider:
    environmentFile: .env

@medikoo
Copy link
Contributor

medikoo commented Dec 18, 2020

@mnapoli Serverless doesn't deploy env vars from .env, they're only loaded into CLI process (and only if they're not set already). They are not distributed to lambda environments

@mnapoli
Copy link
Contributor

mnapoli commented Dec 19, 2020

@medikoo thanks for the clarification, that sounds much more reasonable then! What's the use case with supporting .env btw?

@medikoo
Copy link
Contributor

medikoo commented Dec 19, 2020

What's the use case with supporting .env btw?

It's handy to decide which env vars should be used per directory. Without it usually you'd prefix command with needed env vars, which is not very convenient

@medikoo
Copy link
Contributor

medikoo commented May 17, 2021

@Nyholm thanks for description. What is "PHP Application" ? Why it shares the folder with Serverless service? Which do you feel is more cardinal in this case? Is this that Serverless service complements the "PHP Application", or is it the other way?
Can you describe briefly what "PHP Application" does and what "Serverless service" does?

@Nyholm
Copy link

Nyholm commented May 17, 2021

What is "PHP Application" ?

Any PHP application. In my specific case it is most likely Symfony. This is my application, my website, my API etc.

Why it shares the folder with Serverless service?

Maybe I misunderstand you here. My website lives in /my/project. Im always referring to the /my/project/.env when I write ".env". I assume that you refer to my application/website as a "Serverless service".

Which do you feel is more cardinal in this case? Is this that Serverless service complements the "PHP Application", or is it the other way?

It is the PHP application/website that is the main thing for me. Serverless is just a tool.

Can you describe briefly what "PHP Application" does and what "Serverless service" does?

I think we are using different terminology. Maybe you can elaborate based on my this message.

@medikoo
Copy link
Contributor

medikoo commented May 17, 2021

Maybe I misunderstand you here. My website lives in /my/project. Im always referring to the /my/project/.env when I write ".env". I assume that you refer to my application/website as a "Serverless service".

Issue you're describing only happens if your application/website is configured in same folder as Serverless service (so application/website root folder and serverless service root folder is same).

Is this the case on your side? Do you keep your Serverless service (so serverless.yml file) in same folder in which you configure your application/website? If so, why?

@Nyholm
Copy link

Nyholm commented May 17, 2021

Yes. It is true that my application lives in /my/project. Here you find .env, index.php and serverless.yml.

If so, why?

That is an excellent question. =)

My application lives there because that is basically how you do it in PHP. One application per git repository and the application root is the same as the git root.

Im not sure why serverless.yaml is in the root too, where else should I put it?

@medikoo
Copy link
Contributor

medikoo commented May 17, 2021

Im not sure why serverless.yaml is in the root too, where else should I put it?

Simply keep PHP project and Serverless service at two different folders, especially if there are no common parts between them.

Mixing two unrelated applications within one folder feels as bad pattern to me. It's hard to upfront assume which file belongs to which, and that makes it way harder to reason about them.

@Nyholm
Copy link

Nyholm commented May 17, 2021

Simply keep PHP project and Serverless service at two different folders, especially if there are no common parts between them.

Interesting. In my PHP focused mind, this does not make any sense for me. This is a good example of the root files on a normal PHP website deployed with serverless. As you see, there is all kinds of config files there that belongs to different tools and package managers.

Screenshot 2021-05-17 at 16 45 40

I know Im not an exception here. =)

Mixing two unrelated applications within one folder feels as bad pattern to me.

That is not the way I see it. I see my serverless.yaml as a configuration file to the serverless tool I use to deploy my application. Im happy to use .serverless/serverless.yml, but I do see that they live in the same git repository (and git root is the same as PHP project root).

... that makes it way harder to reason about them

I think that is the situation we are in. You may convince me that this is a bad idea and I'll maybe change. But there are a lot of other PHP developers that wont. Hence the discussion about Serverless reading .env files.

@mnapoli
Copy link
Contributor

mnapoli commented May 17, 2021

I think @Nyholm is trying to say that serverless.yml lives at the root of the project directory because that's how the project is deployed.

Just like you have a README, or a Makefile, or a docker-compose.yml file at the root of the project.

Mixing two unrelated applications within one folder

In that context, they are not really unrelated: the PHP project is the one being deployed to Lambda. There is only 1 application here.

Does that make more sense?

Let's say the project is an HTTP API (whatever the language). If serverless.yml deploys the API to AWS, where would you put serverless.yml @medikoo?

If I look at official Serverless examples, the serverless.yml file is always at the root of the GitHub project, for example: https://github.com/serverless/examples/tree/master/aws-node-twitter-joke-bot

@medikoo
Copy link
Contributor

medikoo commented May 17, 2021

In that context, they are not really unrelated: the PHP project is the one being deployed to Lambda

Thanks @mnapoli for that clarification. It was totally not obvious to me, I assumed both are not that related.

In that case, the PHP project is part of Serverless Service, and I believe right way to approach it, is to put PHP project to some dedicated folder. Ideally code that's used to deploy the app should not be mixed with code that's run in the cloud (especially it if it implies a risk of conflicts)

If I look at official Serverless examples, the serverless.yml file is always at the root of the GitHub project, for example: https://github.com/serverless/examples/tree/master/aws-node-twitter-joke-bot

Yes, if repository hosts just single Serverless service it's totally natural and right to have serverless.yml at root (it would be weird to set it up differently), but if repository would host multiple Serverless services, or Serveless service and e.g. Ruby on rails application, I would definitely nest each entity in its own folder and not try to mix code that's not common to all of them

@Nyholm
Copy link

Nyholm commented May 17, 2021

In that case, the PHP project is part of Serverless Service, and I believe right way to approach it, is to put PHP project to some dedicated folder.

But that not how people do PHP projects =)

Ideally code that's used to deploy the app should not be mixed with code that's run in the cloud (especially it if it implies a risk of conflicts)

True. But there is no "code to deploy the app". It is just a config file (serverless.yml)

We (PHP people) are aware of the "risk of conflicts". That is why we are desperate to fin a way to disable the new "read env files" feature.


I believe this input shows a bit the situation this new Serverless feature has put us in. I hope there could be a workaround.

Ie Make the name of .env that Serverless reads configurable. Or use /my/project/.serverless/.env or to add an option to disable this feature.

@medikoo
Copy link
Contributor

medikoo commented May 17, 2021

I believe this input shows a bit the situation this new Serverless feature has put us in.

From where I stand, this is just one example of collisions, which may happen when we try to maintain two different projects in same folder.

Technically we can fix this specific collision with some opt-out workaround, but with that approach we won't fix any other collisions which may occur due to this kind of organization. The only solid fix is to nest PHP project in some sub folder

@chekalsky
Copy link

To continue the conversation started — why do you consider serverless as different application and not the part of what's in the git root? I always was thinking about serverless.yml as of package.json or docker-compose.yml which describes how application should be deployed, not as another application itself.

Would it be feasible to provide an opt-out option for this new dotEnv behaviour?

@christian-eriksson
Copy link

christian-eriksson commented Jun 2, 2021

I wrote this in the forum thread asking about adding a useDotenv=false option, but this might be a better place.

One example of a use case where I might want to set useDotenv to false is if I’m using serverless.ts config files. Where I prefer loading my variables in my own modules and importing them into the Serverless config. These variables may be loaded from dotenv files, and may not be loaded the same way or using the same conditions as Serverless. So we (my script and Serverless) may choose different files.

In my config modules I might do some checking and verification on the environment. I can potentially verify both that things are set and that things aren't set. I may also (though I probably wouldn't recommend it) change the environment into something that is "valid" let's say I need to remove an environment variable due to some condition.

Basically this means that my environment, once Serverless actually starts deploying (or my application starts locally), may be different to what it was after I verified it. Which to me seems like a potentially huge problem.

@christian-eriksson
Copy link

christian-eriksson commented Jun 2, 2021

To be clear a CLI flag, like the one @medikoo suggested would solve this, I see no need to necessarily put the dotenv behavior config in the serverless file.

@medikoo
Copy link
Contributor

medikoo commented Jun 2, 2021

@chekalsky we're going into direction where Serverless services could be deployed programmatically, e.g. you would be able to require a Serverless instance and deploy programmatically multiple services at once. In that model handling useDotenv configuration at service level cannot be done reliably, as environment variables affects whole Node.js process, and not single service in such case (imagine one service having useDotenv: false and other useDotenv: true).

So summarizing, technically it's a CLI process specific setting, and not a Serverless service specific setting that's why I believe it doesn't belong to service configuration, and we propose to introduce opt-out via CLI param

Similarly is with package.json, in one process you can handle multiple of those, and none of them should affect global state of a process.

@medikoo medikoo added this to the v3-ready milestone Jun 2, 2021
@siolfyr
Copy link

siolfyr commented Aug 4, 2021

This feels like a mistake that will cause a lot of problems for users. Is the benefit of adding this so great that causing all of those problems is worth it? Why purposely use a name which conflicts with programs previously designed using Serverless?

I don't think it should be opt-out, but rather opt-in.

@takeda
Copy link
Contributor

takeda commented Aug 4, 2021

My feeling exactly, despite all of the concerns it still being pushed. And not only it is enabled by default, there's no option to even disable it. I already have a tool that uses .env file and I'm perfectly happy with how it works.

What I'm trying to avoid is wasting few hours on an issue that could be traced to this behavior. At very least use a different name like .serverlessrc

@pgrzesik
Copy link
Contributor

pgrzesik commented Aug 9, 2021

Hello @siolfyr and @takeda - thanks for voicing your opinion here. We are going to introduce a CLI option that will allow to opt-out from this behavior, as @medikoo mentioned in one of the previous comments. Would that satisfy your needs? What potential issues do you see with this behavior? Maybe there's a case that we're missing that is really problematic

@medikoo
Copy link
Contributor

medikoo commented Sep 17, 2021

We've decided (internally) to revert from deprecation notice, and in v3 we will keep the very same behavior as it's in v2 (.env files will be loaded by the Framework only if useDotenv: true setting is set).

The reasoning is that we're worried to break for users which already use .env files to configure the environment for lambdas, where the same root folder is shared by both service configuration and lambda logic

PR that removes the deprecation in v2 will be introduced in the next days (once it's in, we will close this issue)

@chekalsky
Copy link

❤️ Thank you

@ljwagerfield
Copy link

ljwagerfield commented Sep 22, 2021

Thank you @medikoo. A war story FWIW...

For reasons I'm still unsure of, setting useDotenv=true caused some of our environments to fail due to our EC2 instances listening on the wrong port after deployment: the CI pipeline had (somehow) taken the port number from the .env after this change, instead of from the environment section in serverless.yml.

Whilst I can't seem to recreate the problem locally (the artifacts that I build locally do indeed have the correct port in them), all I can say is: the problem occurs in CI for us when setting useDotenv=true.

@medikoo
Copy link
Contributor

medikoo commented Sep 22, 2021

@ljwagerfield all that useDotenv: true does, is loading (not yet populated) environment variables from .env (or .env<stage>), that's it.

@ljwagerfield
Copy link

loading (not yet populated) environment variables from .env (or .env), that's it.

This would cause an issue if the desired env vars (being set elsewhere, say by another plugin) followed the same logic. For example, if they were only being set when absent, then they will no-longer be set after setting useDotenv=true. I'm not sure if that's the exact issue I'm facing or not, but just raising as it's not as innocent of a behaviour as it may seem.

@ljwagerfield
Copy link

ljwagerfield commented Sep 22, 2021

all useDotenv: true does is load (not yet populated) environment variables from .env (or .env), that's it.

This is the problem useDotenv: true seems to be causing:

This would cause an issue if the desired env vars (being set elsewhere, say by another plugin) followed the same logic.

The specifics are:

  1. Our .env file contains local developer-only config, not intended to be used for deployments.
  2. We use serverless-plugin-scripts to run a post-deployment action from serverless. It calls npm run deploy.
  3. npm run deploy calls serverless-export-env to extract the environment section of serverless.yml into some temporary file (temp.env).
  4. npm run deploy then loads the vars from temp.env using dotenv
  5. npm run deploy then calls webpack which builds a bundle. It uses the EnvironmentPlugin.

When setting useDotenv: true the .env file (containing local vars) is loaded into the serverless process, and when serverless spawns the npm run deploy command via serverless-plugin-scripts it inherits those environment vars from the parent serverless process. As such, when step 4 runs, it doesn't load the intended PROD vars, as the LOCAL vars are already loaded from step 1. The result is we deploy LOCAL env vars to PROD.

@medikoo
Copy link
Contributor

medikoo commented Sep 23, 2021

Our .env file contains local developer-only config, not intended to be used for deployments.

@ljwagerfield then you should not use useDotenv: true setting. This trigger is literally about loading environment variables from .env files for deployment specifically (it's not to populate environment variables into Lambda config).

@ljwagerfield
Copy link

Thank you @medikoo, glad it won't be mandatory in v3 🙏

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.