Require environment variables to be defined at startup #17175

aantix · 2014-10-04T07:08:27Z

Added the config option required_env_vars. This option allows the developer to specify a series of environment variables that must be defined at started, otherwise an exception is thrown.

  # e.g.
  config.required_env_vars = ['rollbar_api_key', 'github_api_key']

If either the ENV['rollbar_api_key'] or ENV['github_api_key'] environmental variables aren't defined, an exception is thrown and the server fails to startup.

We've had several instances where our ops team forgot to export an API key on the server and it wasn't apparent until later.

Especially useful for systems like Heroku where reliance on environment variables is prevalent for Rails apps.

eval · 2014-10-04T09:42:27Z

@aantix nice! I had this itch too and wrote https://github.com/eval/envied#envied-

aantix · 2014-10-16T16:22:47Z

@schneems Finding a way to ensure that third party environment variables are set before deployment has caused us issues in the past with Heroku. Any comments on this PR?

seuros · 2014-10-16T16:34:55Z

I think it should raise after checking all variables

Must define the environment variable(s) API_KEY, GITHUB_SECRET, TWITTER_SECRET

aantix · 2014-10-16T17:08:16Z

@seuros Agree. I made the appropriate changes.

seuros · 2014-10-16T17:10:57Z

railties/lib/rails/application.rb

+
+    def validate_environment_vars!
+      if config.required_env_vars.present?
+        vars = config.required_env_vars.each_with_object([]) do |key, required_vars|


i think each_with object is slower that if you do

required_vars = [] config.required_env_vars.each do |key|

missing_env_vars = [] config.required_env_vars.each { |key| missing_env_vars << key.upcase if ENV[key].blank? }

aantix · 2014-10-16T17:26:45Z

Made the suggested changes.

seuros · 2014-10-16T17:36:12Z

railties/lib/rails/application.rb

+      if config.required_env_vars.present?
+        missing_env_vars = []
+        config.required_env_vars.each do |key|
+          missing_env_vars << key if ENV[key].blank?


ENV vars are upcased.

schneems · 2014-10-16T17:50:05Z

Generally I use ENV.fetch("FOO") when I really want an environment variable to be present. What I really like about this approach is that I might get one error that AWS_SECRET isn't set, and then the next time I try to boot, i'll get a different error. In this PR all the critical vars are listed up front which is a really good experience.

aantix · 2014-10-16T18:04:50Z

railties/lib/rails/application.rb

+      if config.required_env_vars.present?
+        missing_env_vars = []
+        config.required_env_vars.each do |key|
+          key.upcase!


@seuros Now upcasing the env var name. The tests now have upcased env variables to reflect this convention.

seuros · 2014-10-16T20:10:57Z

I think this need a Changelog entry.

aantix · 2014-10-16T20:52:38Z

Added Changelog entry.

seuros · 2014-10-16T21:06:59Z

railties/lib/rails/application.rb

@@ -511,5 +512,17 @@ def validate_secret_key_config! #:nodoc:
        raise "Missing `secret_key_base` for '#{Rails.env}' environment, set this value in `config/secrets.yml`"
      end
    end
+
+    def validate_environment_vars!
+      if config.required_env_vars.present?


What do you think about

if Array(config.required_env_vars).any?

just in case someone screw it and put a string or nil

Good idea. Made the change and wrote a test covering the env as a string case.

sgrif · 2014-10-17T03:06:25Z

I agree that this is redundant with Ruby. Use #fetch for required env variables.

sgrif · 2014-10-17T03:06:58Z

That said this certainly gives a nicer error message.

eval · 2014-10-17T10:55:45Z

railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt

@@ -48,6 +48,9 @@ Rails.application.configure do
  # Decrease the log volume.
  # config.log_level = :info

+  # Enforces that all of the following environment variables are defined before server start
+  # config.required_env_vars = ['api_key', 'other_api_key']


Currently when someone uses symbols (just like with the log_tags-config right below), it will raise (no matter the contents of ENV) but it won't be clear from the error why (e.g. assigning [:A] will err with message "Must define the environment variable(s) A").
To anticipate this, either the lookup could be made more flexible (ENV[key.to_s]) (or the error message more clear ("Must define the environment variable(s) :A")).

Second: as ENV['SECRET_KEY_BASE'] is contained in the default secrets.yml, it might be a good candidate to either include in the example or even uncomment this line and just set it?

Third (nitpick): upcasing ENV-keys seems a bit odd IMO, in that it adds more surprise than value.
Besides that, while not common/recommended, users might actually have lowercase ENV-keys.

Good point on stringifying the key.

@seuros Thoughts on the upcasing of the ENV keys? I'm looking through my production apps and all of my keys are upcased, but I agree that there could be exceptions.

Now making the to_s conversion on the key.

aantix · 2014-10-17T14:47:39Z

@sgrif The fetch is effective but the issue is that the missing env only triggers the exception when fetched. For lazy initializations, the critical path may not be hit until much after system startup. This is an effort to formally declare these envs up front and fail (if missing) quickly.

seuros · 2014-10-17T16:09:08Z

railties/lib/rails/application.rb

+        missing_env_vars = []
+
+        required_vars.each do |key|
+          key = key.to_s


we could do it inline
key = key.to_s.upcase

I'm not so sure we should be forcing the upcase. I agree with @eval, there's definitely mixed case eng variables out there and developers may not have the control to convert them to all upper case. Thoughts?

sounds reasonable.

seuros · 2014-10-17T18:40:18Z

railties/lib/rails/application/configuration.rb

@@ -41,6 +41,7 @@ def initialize(*)
        @railties_order                = [:all]
        @relative_url_root             = ENV["RAILS_RELATIVE_URL_ROOT"]
        @reload_classes_only_on_change = true
+        @required_env_vars             = []


let use a Set here. Multiple gems could require the same vars.
That will avoid us having a messages like

Must define the environment variable(s) FOO_API_KEY FOO_API_KEY FOO_API_KEY FOO_API_KEY

Good call. We want to use a Set as well when aggregating the missing env vars.

Now using a Set and updated the tests to account for duplicate environment var declarations.

andyjeffries · 2014-11-13T10:17:03Z

I agree with @aantix, having this fail early is much better than only triggering it on some edge case/rare path in your app. I like the patch/idea.

seuros · 2015-01-21T10:19:29Z

@aantix , could you rebase ?

…eloper to specify a series of environment variables that must be defined at started (otherwise an exception is thrown). # e.g. config.required_env_vars = ['api_key', 'other_third_party_api_key']

aantix · 2015-01-21T15:40:54Z

Rebased.

rafaelfranca · 2015-01-21T18:14:00Z

railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt

@@ -47,6 +47,9 @@ Rails.application.configure do
  # when problems arise.
  config.log_level = :debug

+  # Enforces that all of the following environment variables are defined before server start
+  # config.required_env_vars = ['API_KEY', 'OTHER_API_KEY']


@dhh what do you think about this being included directly on production.rb? I can see how this feature can be useful but I don't like the idea of promoting it by default. In my projects we usually don't use any ENV key.

dhh · 2015-01-23T02:56:23Z

Not a fan of promoting free-floating ENVs like this. My recommendation would be to let this flow through config/secrets.yml like so:

development:
  secret_key_base: x
  stuff: <%= ENV.fetch 'stuff' %>

That achieves the same thing, and it bundles all your secrets together to be accessed via Rails.application.secrets.

dhh · 2015-01-23T02:58:08Z

We could even explain this pattern in config/secrets.yml as a comment at the top.

andyjeffries · 2015-01-23T08:25:54Z

The only problem with the much simpler mechanism of putting it in secrets.yml is that it doesn't enforce the presence of those settings. That's what this ticket is about, catching deployments to new environments/systems where you haven't configured the environment properly. So if the comment looked more like the one below I'd be in favour:

development:
  secret_key_base: x
  # To use ENVironment variables, add code like this:
  # your_value: <%= ENV.fetch 'YOUR_VALUE' rescue KeyError raise ActiveSupport::MissingEnvVariable.new("YOUR_VALUE") %>

But this then borders on just educating people on a good way of doing it (which could be done in a blog post/Rails docs) rather than having the Framework help people do something that is a common pattern.

(edited: I forgot fetch raises an error, but I'd still prefer this to be a specific non-obvious error rather than just KeyError)

dhh · 2015-01-23T17:59:08Z

Fetch raises automatically and secrets.yml is evaluated at boot. So if you ENV.fetch something that isn't there, it's not going to boot. Education for this usage can go at the top of secrets.yml.

On Jan 23, 2015, at 03:26, Andy Jeffries notifications@github.com wrote:

The only problem with the much simpler mechanism of putting it in secrets.yml is that it doesn't enforce the presence of those settings. That's what this ticket is about, catching deployments to new environments/systems where you haven't configured the environment properly. So if the comment looked more like the one below I'd be in favour:

development:
secret_key_base: x

To use ENVironment variables, add code like this:

your_value: <%= ENV.fetch 'YOUR_VALUE' || raise ActiveSupport::MissingEnvVariable.new("YOUR_VALUE") %>

But this then borders on just educating people on a good way of doing it (which could be done in a blog post/Rails docs) rather than having the Framework help people do something that is a common pattern.

—
Reply to this email directly or view it on GitHub.

aantix · 2015-01-23T19:31:20Z

The secrets.yml approach feels a bit strange just because you're specifying the key name twice (once for the secrets.yml entry and once for the ENV fetch).

My hunch is that developer would keep the secret key name consistent with the ENV var name. What about an (optional) array in the secrets.yml that will automatically map the corresponding ENV value to a secret entry?

E.g.

  required_env_vars:
    - TWITTER_API_KEY
    - ROLLBAR_API_KEY

Using the logic in the original PR, the above required_env_vars would be checked for their existence upon startup. It's one nice consolidated list of external ENVs.

The developer would then access the keys via the conventional

    Rails.application.secrets[:twitter_api_key]

dhh · 2015-01-23T20:29:37Z

The repetition really doesn't bother me because you are pulling something in from the outside world. To me, that's similar to person.name = params[:person][:name] or whatever.

Further more, this is not an area that needs excessive comfort. You're not going to have neither dozens of ENVs nor adding/removing them constantly.

On Jan 23, 2015, at 14:32, Jim Jones notifications@github.com wrote:

The secrets.yml approach feels a bit strange just because you're specifying the key name twice (once for the secrets.yml entry and once for the ENV fetch).

My hunch is that developer would keep the secret key name consistent with the ENV var name. What about an (optional) array in the secrets.yml that will automatically map the corresponding ENV value to a secret entry?

E.g.

required_env_vars:
- TWITTER_API_KEY
- ROLLBAR_API_KEY
Using the logic in the original PR, the above required_env_vars would be checked for their existence upon startup. It's one nice consolidated list of external ENVs.

The developer would then access the keys via the conventional
Rails.application.secrets[:twitter_api_key]
—
Reply to this email directly or view it on GitHub.

pixeltrix · 2015-03-23T19:40:47Z

Turns out that you can't use fetch in secrets.yml as it would blow up in development when secrets.yml is loaded. Also secrets.yml is lazily loaded so it may appear as though the app has booted after a cap deploy.

Probably the simplest way to achieve the desired effect is to add something like the following to config/environment.rb:

# Load the Rails application.
require File.expand_path('../application', __FILE__)

if Rails.env.production?
  %w[SECRET_KEY_BASE API_KEY].each do |var|
    raise RuntimeError, "Missing environment variable: #{var}" unless ENV.key?(var)
  end
end

# Initialize the Rails application.
Rails.application.initialize!

Some people would argue that it's duplicating the list of vars since they'd be defined in a .env file but that's somewhat application dependent - not everyone uses .env files.

rails-bot · 2019-12-18T18:08:55Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

aantix force-pushed the required_environment_variables branch from 2a0b07d to 695ae88 Compare October 16, 2014 17:06

seuros reviewed Oct 16, 2014
View reviewed changes

rafaelfranca added the railties label Oct 16, 2014

seuros added this to the 5.0.0 milestone Oct 16, 2014

aantix force-pushed the required_environment_variables branch from 695ae88 to f618005 Compare October 16, 2014 17:25

seuros reviewed Oct 16, 2014
View reviewed changes

aantix force-pushed the required_environment_variables branch from f618005 to 9ef293a Compare October 16, 2014 18:02

aantix reviewed Oct 16, 2014
View reviewed changes

aantix force-pushed the required_environment_variables branch from 9ef293a to f8b8128 Compare October 16, 2014 20:03

aantix force-pushed the required_environment_variables branch from f8b8128 to 69416bd Compare October 16, 2014 20:52

seuros reviewed Oct 16, 2014
View reviewed changes

aantix force-pushed the required_environment_variables branch from 69416bd to a237474 Compare October 17, 2014 01:17

eval reviewed Oct 17, 2014
View reviewed changes

aantix force-pushed the required_environment_variables branch 2 times, most recently from bbd09df to 7952732 Compare October 17, 2014 16:03

seuros reviewed Oct 17, 2014
View reviewed changes

aantix force-pushed the required_environment_variables branch from 7952732 to 2053139 Compare October 17, 2014 19:09

Added the config option required_env_vars. This option allows the dev…

dea3ed8

…eloper to specify a series of environment variables that must be defined at started (otherwise an exception is thrown). # e.g. config.required_env_vars = ['api_key', 'other_third_party_api_key']

aantix force-pushed the required_environment_variables branch from 2053139 to dea3ed8 Compare January 21, 2015 15:40

rafaelfranca reviewed Jan 21, 2015
View reviewed changes

tijmenb mentioned this pull request Sep 15, 2015

Take secret key base from environment alphagov/static#649

Merged

rails-bot assigned rafaelfranca Oct 20, 2015

rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016

rails-bot bot added the stale label Dec 18, 2019

rails-bot bot closed this Dec 25, 2019

Require environment variables to be defined at startup #17175

Require environment variables to be defined at startup #17175

Conversation

aantix commented Oct 4, 2014

eval commented Oct 4, 2014

aantix commented Oct 16, 2014

seuros commented Oct 16, 2014

aantix commented Oct 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantix commented Oct 16, 2014

Choose a reason for hiding this comment

schneems commented Oct 16, 2014

Choose a reason for hiding this comment

seuros commented Oct 16, 2014

aantix commented Oct 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif commented Oct 17, 2014

sgrif commented Oct 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantix commented Oct 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyjeffries commented Nov 13, 2014

seuros commented Jan 21, 2015

aantix commented Jan 21, 2015

Choose a reason for hiding this comment

dhh commented Jan 23, 2015

dhh commented Jan 23, 2015

andyjeffries commented Jan 23, 2015

dhh commented Jan 23, 2015

To use ENVironment variables, add code like this:

your_value: <%= ENV.fetch 'YOUR_VALUE' || raise ActiveSupport::MissingEnvVariable.new("YOUR_VALUE") %>

aantix commented Jan 23, 2015

dhh commented Jan 23, 2015

pixeltrix commented Mar 23, 2015

rails-bot bot commented Dec 18, 2019