radar / elastic

A thin veneer over HTTPotion that talks to Elastic Search
https://hex.pm/packages/elastic
MIT License
65 stars 22 forks source link

12 factor concerns #3

Closed bglusman closed 7 years ago

bglusman commented 7 years ago

Hey @radar,

Working on tests and docs for #2, but wanted to discuss one other concern... Currently, with the API access and secret needed in config, they're required at compile time, which also means they're included in the compiled code. I'm wondering if this is the best way, and if this doesn't present security concerns, as well as the need to recompile the app if the only thing changing is credentials/endpoint. It also makes it a little harder to dockerize my app, as I need these values at container build time instead of runtime, and I can't reuse a staging container in production without recompiling. What do you think? Do you have same issues/dealing with them some other way?

bglusman commented 7 years ago

Actually, just realized I can do this myself with no changes by just moving the config out of config.exs and into Application.start via Application.put_env but might still want to document this as an option/reccomendation to avoid people compiling secrets etc into their executables?

radar commented 7 years ago

I believe it is wrong that they are required at compiled time. The config values are evaluated when the application starts. Here's what I've done to verify this today:

  1. Created config/prod.exs with this content:
use Mix.Config

config :elastic,
  index_prefix: System.get_env("INDEX_PREFIX")
  1. Compiled the app using MIX_ENV=prod mix compile
  2. Ran: INDEX_PREFIX=one MIX_ENV=prod iex -S mix
  3. Verified that Elastic.Index.name("foo") shows "one_foo"
  4. Ran: INDEX_PREFIX=two MIX_ENV=prod iex -S mix
  5. Verified that Elastic.Index.name("foo") shows "two_foo"

Keep in mind that this would work only if you're passing in the environment variables when running the container, rather than when building it.

I don't think there's any changes necessary to Elastic here. You should be passing in env variables when running the container and that'll allow you to run the same container on staging + prod.

bglusman commented 7 years ago

Hey @radar FYI, and for anyone else finding this in the future, don't fully understand the test you ran (I suspect it just recompiled implicitly from the mix command?), but this seems to document the issue I was talking about: http://engineering.avvo.com/articles/using-env-with-elixir-and-docker.html

My actual solution at the time (and still) was to just move a bunch of the config into Application.start like so:

defmodule DataReturn do
  use Application
  # See http://elixir-lang.org/docs/stable/elixir/Application.html
  # for more information on OTP Applications
  def start(_type, _args) do
    import Supervisor.Spec

    Application.put_env(:data_return, :secret_encryption_key, System.get_env("SECRET_ENCRYPTION_KEY"))
    Application.put_env(:elastic, :base_url, System.get_env("ELASTICSEARCH_URL"))
    Application.put_env(:elastic, :aws, %{
        enabled: true,
        access_key_id: System.get_env("ELASTICSEARCH_ACCESS"),
        secret_access_key: System.get_env("ELASTICSEARCH_SECRET"),
        region: System.get_env("AWS_REGION")
      }
    )

Unsure if I should switch to using ExRM with RELX_REPLACE_OS_VARS as that article suggests or leave as is for one less dependency, but it's not really this libraries issue in any case, seems to just be how Elixir does config, which is... frustrating there's not a cleaner switch, but maybe reasonable, I dunno...

radar commented 7 years ago

Thanks for the info @bglusman.

We're running an application which uses Elastic inside of a Docker container and we have config for this in prod.exs:

config :elastic,
  use_mix_env: true,
  index_prefix: "app_name",
  base_url: "https://" <> System.get_env("ES_ENDPOINT"),
  aws: %{
    enabled: true,
    access_key_id: (System.get_env("AWS_ID") || raise "AWS_ID not set"),
    secret_access_key: (System.get_env("AWS_KEY") || raise "AWS_KEY not set"),

I don't understand why that's not possible in your situation, sorry.