danny-avila / LibreChat

Enhanced ChatGPT Clone: Features Anthropic, OpenAI, Assistants API, Azure, Groq, GPT-4o, Mistral, OpenRouter, Vertex AI, Gemini, Artifacts, AI model switching, message search, langchain, DALL-E-3, ChatGPT Plugins, OpenAI Functions, Secure Multi-User System, Presets, completely open-source for self-hosting. Actively in public development.
https://librechat.ai/
MIT License
17.2k stars 2.87k forks source link

[Bug]: current env pattern conflicts with containers (solution provided) #801

Closed f1yn closed 1 year ago

f1yn commented 1 year ago

Contact Details

dev@flyn.ca

What happened?

While wrapping up #785, I noticed that the env structure still needs a little refinement. Normally I'd say this would be a feature request, but it does affect reliability and robustness of container based deployments (including via docker-compose).

Short

The LibreChat git repo should make the .env .env.production and .env.development something that needs to be opted into, instead of checked out in the tree AS-IS - for container based usages.

Long

In most container engines, you provide -e="SOME_NAME=SOME_VALUE" which will perform something similar toexport SOME_NAME=SOME_VALUE" into the environment that your container runs in. Even more conveniently, most also provide --env-file which does the same thing, but points directly into a file.

The problem starts when this fact is combined with the use of dotenv, which overrides any environment variables that are exposed through the host. For instance, if the --env-file=./mylocalenv flag is used on a new container, the container environment will have something like:

HOST=value_i_actually_want

Which is ideal because it lives outside of the LibreChat source tree, and will be inherited by the running processes started in the container. But what ends up happening is it will be overridden by dotenvs multiple values:

# LibreChat/.env
HOST=some_other_value

# LibreChat/.env.production
HOST=another_value_I_dont_want

The docker-compose file shows that all three need to passed, which unfortunately isn't a good pattern for containers.

Best solution

The easiest way to solve this would be to add yet another (opt-in) env value that would prevent dotenv from reading the .env .env.production and .env.development files in favor of whatever values are given by the container.

I don't mind adding this to my work, but I'd need your thoughts before I add it to the documentation work. Was hoping to not make any code changes, but I don't mind.

Steps to Reproduce

  1. Use LibreChat with docker compose, or other container-based installation.
  2. Find yourself having multiple sources of truth for a single env.

What browsers are you seeing the problem on?

No response

Relevant log output

No response

Screenshots

No response

Code of Conduct

danny-avila commented 1 year ago

The easiest way to solve this would be to add yet another (opt-in) env value that would prevent dotenv from reading the .env .env.production and .env.development files in favor of whatever values are given by the container.

Maybe I'm wrong, but I was convinced that in the compose file, anything under environment would take precedent over the env_file values, referring to this:

    env_file:
      - .env
    environment:
      - HOST=0.0.0.0

The reason I have to believe this is that these values are indeed used over any existing .env files, otherwise my docker environment would not work as expected

Just did my homework to read the docs:

env_file

... Environment variables declared in the environment section override these values – this holds true even if those values are empty or undefined.

According to the Docker Compose file reference, values added in the environment section of a Docker Compose file will, indeed, take precedence over those from the env_file:

When you set the same environment variable in multiple files, here’s the priority used by Compose to choose which value to use:

  1. Compose file
  2. Shell environment variables
  3. Environment file
  4. Dockerfile
  5. Variable is not defined

source

So with this in mind, HOST=value_i_actually_want is specified under environment and dotenv does not conflict or the app would break in my experience, even with the same variables set in .env file

Maybe this isn't best practice or I'm misunderstanding the situation, let me know! I appreciate you looking into this. I'm certainly open to change here.

f1yn commented 1 year ago

Maybe I explained the problem poorly.

Here's my container configuration (in example):

.myintendedenv (the one I am passing into docker/podman as env-file)

I then have the source in a sub-folder, which is solely used for building the container image.

./LibreChat/.env
./LibreChat/.env.development
./LibreChat/.env.production

The ones above are being consumed by default by dotenv, even if I don't pass them through into the container. That's because the image that gets built includes them and reads them due to the dotenv package.

The above creates a situation where even though I specify a value in .myprefferedenv, because LibreChat uses dotenv the included .env.* files are always loaded, regardless of if I want them to or not.

Normally I'd just override these with empty volumes, but that's very redundant and creates a messy configuration. If I delete the .env files I no longer want, it creates a diff that complicates the auto-update functionally as now I've removed checked out files from the git tree.

If I added a flag that would stop dotenv from overriding the ones I provide from the environment itself, then I wouldn't be experiencing issues. Their documentation also suggests against this, but it's easier just to conditionally prevent loading them if a specific env value is present (like process.env.DISABLE_ENV_FILE).

danny-avila commented 1 year ago

If I added a flag that would stop dotenv from overriding the ones I provide from the environment itself, then I wouldn't be experiencing issues. Their documentation also suggests against this, but it's easier just to conditionally prevent loading them if a specific env value is present (like process.env.DISABLE_ENV_FILE).

I see I think this is fine as long as everything works as before for someone that never opts into DISABLE_ENV_FILE

I wasn't totally involved in creating config/loader.js, which handles:

./LibreChat/.env
./LibreChat/.env.production
./LibreChat/.env.development

Maybe @ClaraLeigh can speak to this better (Australia TZ).

The image only needs env variables during build time for the config/loader.js script and come to think of it, not sure how necessary it is anymore. The frontend (client) doesn't need those variables during build time anymore, and the backend (api) can use .env variables at run time, not when the image is being built, and this maybe was overlooked.

Would it simplify things if this logic is refactored so production/development env variables are not needed during build time? I really don't see why they should be used during build time (though this was maybe necessary early on in the project).

ClaraLeigh commented 1 year ago

Little confused, why do you need the other .env files if you don't want to use them? Isn't the easiest solution here to delete the other .env.x files that you don't need?

f1yn commented 1 year ago

I was worried about .env values existing in the upstream source, but it looks like I may have done this to myself while experimenting.

@ClaraLeigh I'll close this as it does indeed look like a nonissue.