Skip to content

Do not store production information in .yml files #13463

Merged
merged 5 commits into from Dec 23, 2013
@josevalim
Ruby on Rails member

Instead, read information from environment variables.

@josevalim josevalim Do not store production information in .yml files
Instead, read information from environment variables.
d22a359
@josevalim
Ruby on Rails member

This approach is much better than the .gitignore one, which requires extra work on setting up the machine for development, and also works nicely with most deploy platforms.

@vipulnsward
Ruby on Rails member

:100:
Should work for everyone from #13388

@rafaelfranca
Ruby on Rails member

@josevalim what do you think about using a namespace for the environment variables. Something like RAILS_DATABASE_URL. Since environment variables are not Rails specific in my opinion would be better if we add the namespace.

@carlosantoniodasilva
Ruby on Rails member

@rafaelfranca @josevalim or even prefix using the app's name so that it works per-app.

@josevalim
Ruby on Rails member

@rafaelfranca that sounds like a good idea. I just want to point out that Active Record already checks for DATABASE_URL in a couple places. I know @hone did some work during RailsConf to extract that out but I can't remember if it was merged. @hone?

@guilleiguaran
Ruby on Rails member

We are using DATABASE_URL, SECRET_KEY_BASE in some places of code, adding a prefix now might break some existing apps :grin:

/cc @hone @schneems wdyt about this?

@laserlemon

I love the move toward using ENV and I hope the conversation continues in this direction. I think we could go further by populating and consuming ENV across the board, in development and test as well.

The config/secrets.yml file could populate ENV and Rails.application.secrets could then read from ENV. I think this would also avoid surprise from production secrets needing to be strings while development/test secrets would not.

@fxn
Ruby on Rails member
fxn commented Dec 23, 2013

I don't mind switching to this default, but would not be so affirmative in the comment. For once, the mere default suggests the convention treats production differently for a reason, and second I have seen several client projects where the production stuff is in the repo without any problem.

Whether production config in the repo is good or bad depends on the project.

@josevalim
Ruby on Rails member

@fxn Good point. Although I would say those companies are probably very comfortable with what they are doing and they probably know when to abandon rails golden path. :)

@eval
eval commented Dec 23, 2013

+1 From the perspective of failing fast, ENV.fetch might be better. Also: a useful stacktrace.

@zmoazeni

-1 to using ENV.fetch. That breaks other environments that doesn't need those ENV variables set (development, test) when the yaml file gets evaluated.

@eval
eval commented Dec 23, 2013

@zmoazeni ah, yes, that won't work

@schneems
Ruby on Rails member

Amazing :+1:

@rafaelfranca internally rails is still relying on the DATABASE_URL in a few places for example: 971d510#diff-c31f5df16f211a1d5d0060bb67b0f81eR86

Setting a convention for environment variables used by Rails to start with RAILS_ would help avoid accidentally setting an env var the user didn't know about or from accidentally clashing with other libraries. As long as we can know the correct value through convention or by putting it in a machine read-able place we can handle setting a sane default.

josevalim added some commits Dec 23, 2013
@josevalim josevalim Guarantee the connection resolver handles string values
This commit also cleans up the rake tasks that were checking
for DATABASE_URL in different places.

In fact, it would be nice to deprecate DATABASE_URL usage in the long
term, considering the direction we are moving of allowing those in .yml
files.
c390e60
@josevalim josevalim Add examples and namespace ENV options with "RAILS_" facf2b4
@josevalim josevalim Use the new Resolver API in dbconsole f6e3633
@josevalim josevalim Update CHANGELOG [ci skip] 2d4cfb2
@josevalim
Ruby on Rails member

I've added the RAILS_ prefix, added examples to the database.yml files and improved some code in AR. I think this is good to go.

@josevalim josevalim merged commit 33cb2f3 into rails:master Dec 23, 2013
@josevalim josevalim deleted the josevalim:jv-env branch Dec 23, 2013
@carlosantoniodasilva
Ruby on Rails member

Looks like travis failed railties tests after this, mind taking a look bro? (sorry I won't be able =/)

@schneems schneems added a commit to schneems/rails that referenced this pull request Dec 25, 2013
@schneems schneems Partial fix of database url tests
Prior to #13463 when `DATABASE_URL` was set, Rails automagically used that value instead of the database.yml. There are tests in dbs_test that expect this to still be true. After that PR, `RAILS_DATABASE_URL` is expected to be read into the YAML file via ERB, this PR fixes that behavior.

Note: this does not entirely fix the tests. It seems that `ActiveRecord::Tasks::DatabaseTasks.current_config` does not process the url string correctly (convert it into a hash), and ` ActiveRecord::Tasks::DatabaseTasks.structure_load(current_config, filename)` as well as other methods in `DatabaseTasks` expect a hash.

It seems like we should involve the resolver somewhere in this process to correctly convert the database url, I do not know the best place for that /cc @josevalim
eb3ec6b
@schneems schneems added a commit to schneems/rails that referenced this pull request Dec 25, 2013
@schneems schneems Partial fix of database url tests
Prior to #13463 when `DATABASE_URL` was set, Rails automagically used that value instead of the database.yml. There are tests in dbs_test that expect this to still be true. After that PR, `RAILS_DATABASE_URL` is expected to be read into the YAML file via ERB, this PR fixes that behavior.

Note: this does not entirely fix the tests. It seems that `ActiveRecord::Tasks::DatabaseTasks.current_config` does not process the url string correctly (convert it into a hash), and ` ActiveRecord::Tasks::DatabaseTasks.structure_load(current_config, filename)` as well as other methods in `DatabaseTasks` expect a hash.

It seems like we should involve the resolver somewhere in this process to correctly convert the database url, I do not know the best place for that /cc @josevalim
ed41f52
@schneems schneems added a commit to schneems/rails that referenced this pull request Dec 25, 2013
@schneems schneems Partial fix of database url tests
Prior to #13463 when `DATABASE_URL` was set, Rails automagically used that value instead of the database.yml. There are tests in dbs_test that expect this to still be true. After that PR, `RAILS_DATABASE_URL` is expected to be read into the YAML file via ERB, this PR fixes that behavior.

Note: this does not entirely fix the tests. It seems that `ActiveRecord::Tasks::DatabaseTasks.current_config` does not process the url string correctly (convert it into a hash), and ` ActiveRecord::Tasks::DatabaseTasks.structure_load(current_config, filename)` as well as other methods in `DatabaseTasks` expect a hash.

It seems like we should involve the resolver somewhere in this process to correctly convert the database url, I do not know the best place for that /cc @josevalim
ba88293
@fdietz fdietz added a commit to fdietz/team_dashboard that referenced this pull request Dec 30, 2013
@fdietz fdietz change config keys to more easily map to ENV vars
Current conversations about this topic, including wether to add secrets.ymlto source control by default:

* rails/rails#13388
* rails/rails#13463

I expect to support something more closely to the [figaro](https://github.com/laserlemon/figaro)
gem in the future. For now I wait until Rails 4.1 is released.
0db7a56
@schneems
Ruby on Rails member
schneems commented Jan 2, 2014

I'm still looking into behavior of Active Record concerning environment variables. I would like to address consistency of behavior across multiple ways AR can be used, and re-visit the bike shed that is naming environment variables.

tl;dr I'm working on making behavior of Active Record consistent across all scenarios with regard to DATABASE_URL being present. I'm also recommending we switch back to using DATABASE_URL in the database.yml file. For more info read my super long explanations below.

Consistency of Environments

Here are all the ways that AR initiates a connection today:

  • Stand Alone (without rails)

    • rake db:<tasks>
    • ActiveRecord.establish_connection
  • With Rails

    • rake db:<tasks>
    • rails <server> | <console>
    • rails dbconsole

We should make all of these behave exactly the same way, which if you dig into AR is non-trivial. I'm working to see if I can put all of this logic in one place for consistency, but as I mentioned, it's a non-trivial task.

Currently AR can be configured via the environment variable DATABASE_URL or by manually injecting a hash of values which is what Rails does, reading in database.yml and setting AR appropriately. AR expects to be able to use DATABASE_URL without the use of Rails, and we cannot rip out this functionality without deprecating IMHO. This presents a problem though when both config is set, and a DATABASE_URL is present. Currently the DATABASE_URL should "win" and none of the values in database.yml are used. This is somewhat unexpected to me if I were to set values such as pool in the production: group of database.yml they are ignored. Here is my prosed matrix of how this behavior should work:

No database.yml
No DATABASE_URL
=> Error
database.yml present
No DATABASE_URL
=> Use database.yml configuration
No database.yml
DATABASE_URL present
=> use DATABASE_URL configuration
database.yml present
DATABASE_URL present
=> Merged into `url` sub key. If both specify `url` sub key, the `database.yml` `url` 
   sub key "wins". If other paramaters `adapter` or `database` are specified in YAML, 
   they are discarded as the `url` sub key "wins".

Implementation

I'm currently working on getting the above to work and be consistent with all environments, the last barrier I have to overcome is the Rake tasks. I expect this last bit to take 80% of the effort. This also brings me to my second topic: env var naming.

Env Var Naming

As I mentioned above, AR already has built in support for DATABASE_URL and as of today if DATABASE_URL is present, the database.yml values won't be used at all. If I am able to implement and get accepted my above proposal, this won't be much of an issue but right now it is.

I was originally :+1: on name-spacing the environment variables with RAILS_ though after digging in AR for a few days and thinking about it more, I think we should use non-namespaced urls, so DATABASE_URL instead of RAILS_DATABASE_URL. Why?

DATABASE_URL isn't just a Rails concept, it's actually used in other frameworks, most notably: DATABASE_URL is used in django. And there's no reason it cannot be used in other frameworks.

One of the initial reasons for namespacing was to prevent conflicts between different languages running on the same box. I think this is the minority case, a more likely use case would be multiple Rails applications on the same box. Using the RAILS_ namespacing would do nothing to help there. If you wanted truly multi-tennant database url connections they would need to be something like RAILS_<appname>_<uuid>_DATABASE_URL which is unweildy to work with and likely causes more problems than it solves.

Deployment seems to be leaning towards using containers (through LXC, docker, or similar) which allows you to set a clean ENV per each app. If you need to deploy multiple apps to one box, you can use .env files, or your own custom database.yml setup. Any of these solutions eliminate the problem of cross app ENV var contamination.

By keeping this value standard, you could have your container generation code create a DATABASE_URL env var by default, and not have to worry about it. The alternative isn't horrible, you just alias export RAILS_DATABASE_URL=$DATABASE_URL but as I mentioned before, unless I (or someone else) fixes the above behavior this will cause unexpected behavior in your app, as database.yml gets ignored :grimmace:

I'm out to lunch on namespacing of SECRET_KEY_BASE, it's not already a pseudo-standard the way that DATABASE_URL has become. I also see no real benefit of keeping the RAILS_ namespace.

I'm interested in talking about this point more, right now I would recommend switching back from RAILS_DATABASE_URL => DATABASE_URL. I'm happy to do all the conversion work here.

@rafaelfranca
Ruby on Rails member

Merged into url sub key. If both specify url sub key, the database.yml url sub key "wins".

Should not the the DATABASE_URL the winner in this case too?

I'm interested in talking about this point more, right now I would recommend switching back from RAILS_DATABASE_URL => DATABASE_URL.

When I proposed the namespacing I was more concerned with the secret key. Since the DATABASE_URL is already being used and it is a pseudo-standard we can change it back.

@schneems
Ruby on Rails member
schneems commented Jan 2, 2014

@rafaelfranca I'm okay with that behavior as well. I like the ability to change this value if needed.

If we're saying that DATABASE_URL is guaranteed to be on a system deployed with docker, or Heroku (which it will be) then it is impossible to use a different database. Here are a few different use cases. First, by default it won't matter which wins, as they'll be the same:

$ echo $DATABASE_URL
postgresql://localhost/something

$ cat config/database.yml
production:
  url: <%= ENV['DATABASE_URL'] %>

It would be confusing to me, if I modified the database.yml and it was not used:

$ echo $DATABASE_URL
postgresql://localhost/something

$ cat config/database.yml
production:
  url: <%= ENV['SOME_OTHER_URL'] %>

However if a user has DATABASE_URL set, and no url specified in production: the values get discarded anyway, so maybe this is okay:

$ echo $DATABASE_URL
postgresql://localhost/something

$ cat config/database.yml
production:
  database: foo
  adapter: mysql

I will submit a PR to change the default env var values to DATABASE_URL.

@rafaelfranca
Ruby on Rails member

Agree the second case is confusing. Supposing that a user for some reason want to use a different database in production that the one configured in the DATABASE_URL, they will not have how to do. So I think it is fine if the user specify the url key in the production configuration this key wins.

Lets go with your proposed behavior

@schneems schneems added a commit to schneems/rails that referenced this pull request Jan 2, 2014
@schneems schneems Use DATABASE_URL by default
See rails#13463 (comment) for full conversation.
f642b18
@roderickvd roderickvd added a commit to roderickvd/rails that referenced this pull request Jan 3, 2014
@schneems schneems Use DATABASE_URL by default
See rails#13463 (comment) for full conversation.
b583485
@schneems
Ruby on Rails member
schneems commented Jan 3, 2014

Here's my somewhat ambitious proposal actually implemented: #13582

If we want to change any of the merge behavior it is all isolated to one place, so it should be way easier now than before.

@hone hone added a commit to heroku/heroku-buildpack-ruby that referenced this pull request Feb 18, 2014
@hone hone don't write database.yml in Rails 4.1 cd74fb9
@hone hone referenced this pull request in heroku/heroku-buildpack-ruby Feb 18, 2014
Merged

Rails 4.1.0 Support #227

@hone hone added a commit to heroku/heroku-buildpack-ruby that referenced this pull request Feb 18, 2014
@hone hone Rails 4.1 support
* don't write `config/database.yml` in Rails 4.1, See rails/rails#13463 (comment).
* setup env var SECRET_KEY_BASE for Rails 4.1's `config/secrets.yml`
d44241d
@hone hone added a commit to heroku/heroku-buildpack-ruby that referenced this pull request Feb 19, 2014
@hone hone Rails 4.1 support
* don't write `config/database.yml` in Rails 4.1, See rails/rails#13463 (comment).
* setup env var SECRET_KEY_BASE for Rails 4.1's `config/secrets.yml`
41dd02b
@squeedee squeedee added a commit to cloudfoundry/ruby-buildpack that referenced this pull request Apr 10, 2014
@hone hone Rails 4.1 support
* don't write `config/database.yml` in Rails 4.1, See rails/rails#13463 (comment).
* setup env var SECRET_KEY_BASE for Rails 4.1's `config/secrets.yml`
432b557
@reidmorrison

If the purpose of removing database configuration information is purely for security and/or compliance reasons then encrypting the passwords and secrets in config files is sufficient. We encrypted all production passwords and secrets.yml for PCI compliance using the SymmetricEncryption gem

@stevenharman stevenharman added a commit to stevenharman/cellar that referenced this pull request Jun 12, 2014
@stevenharman stevenharman Fetch current DB config from ActiveRecord
Rails has changed how DB connection configs are loaded and merged
together -- DATABASE_URL Env Vars are now merged with database.yml
settings (see:
rails/rails#13463 (comment)). We now
fetch the current config from `ActiveRecord::Base.configurations`, which
will ensure the settings have been properly loaded.
e1300a9
@grekko grekko added a commit to betterplace/team_dashboard that referenced this pull request Sep 28, 2014
@fdietz fdietz change config keys to more easily map to ENV vars
Current conversations about this topic, including wether to add secrets.ymlto source control by default:

* rails/rails#13388
* rails/rails#13463

I expect to support something more closely to the [figaro](https://github.com/laserlemon/figaro)
gem in the future. For now I wait until Rails 4.1 is released.
46f7ec5
@calebthompson calebthompson referenced this pull request in thoughtbot/suspenders Jan 27, 2015
Merged

Replace Unicorn with Puma #501

@MattFenelon MattFenelon added a commit to MattFenelon/standalone-migrations that referenced this pull request Apr 14, 2015
@MattFenelon MattFenelon Properly support DATABASE_URL
Switched out the code for loading configuration for Rails's version of
loading database configuration.

Rails has implemented support for DATABASE_URL in rails/rails@13463 and
has dealt with the possible combinations of having DATABASE_URL set and
a YAML config file, see rails/rails#13463 (comment).

There is a breaking change in the way the hash is loaded; Rails does not
convert the keys of the hash to symbols, instead they are left as
strings; any code reliant on the hash keys being symbols will break.
012e608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.