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

Selected config settings can be defined as env vars #2434

Merged
merged 5 commits into from May 19, 2015

Conversation

brew
Copy link
Member

@brew brew commented May 19, 2015

To allow better support for cloud deployments and remote configuration (#2429), some config settings can be separated from the code by being set as environmental variables. Currently these settings are defined in environment.py.

# A list of config settings that can be overridden by environmental variable.
ENV_VAR_WHITELIST = {
    'sqlalchemy.url': 'CKAN_SQLALCHEMY_URL',
    'ckan.datastore.write_url': 'CKAN_DATASTORE_WRITE_URL',
    'ckan.datastore.read_url': 'CKAN_DATASTORE_READ_URL',
    'solr_url': 'CKAN_SOLR_URL',
    'ckan.site_id': 'CKAN_SITE_ID',
    'smtp.server': 'CKAN_SMTP_SERVER',
    'smtp.starttls': 'CKAN_SMTP_STARTTLS',
    'smtp.user': 'CKAN_SMTP_USER',
    'smtp.password': 'CKAN_SMTP_PASSWORD',
    'smtp.mail_from': 'CKAN_SMTP_MAIL_FROM'
}

The list may be expanded in the future.

brew added 4 commits May 15, 2015 14:47
A whitelist of config options that can be set from env vars, currently:

    'sqlalchemy.url': 'CKAN_SQLALCHEMY_URL'
    'ckan.datastore.write_url': 'CKAN_DATASTORE_WRITE_URL'
    'ckan.datastore.read_url': 'CKAN_DATASTORE_READ_URL'
    'solr_url': 'CKAN_SOLR_URL'
    'ckan.site_id': 'CKAN_SITE_ID'

These are set after the config is read from the .ini file and after
plugins have had an opportunity to update the config, but before
app_globals are set from the database.
@amercader amercader self-assigned this May 19, 2015
@davidread
Copy link
Contributor

ENV_VAR_WHITELIST doesn't say what it is for - how about CONFIG_FROM_ENV_VARS or something?

@amercader
Copy link
Member

+1

@amercader amercader merged commit 2db129a into master May 19, 2015
@davidread
Copy link
Contributor

This list of env variables has no reference (even in this Issue) saying how you selected those that are in the list. Yet I know there's been a lot of discussion about the purpose, precedent and security (not that I've followed this closely). Perhaps you could add a note in the code or refer to a doc with the thinking? Otherwise someone may come along and add others willy nilly and the thinking will count for nought.

@brew
Copy link
Member Author

brew commented May 20, 2015

The related issue links to the spec in the ideas and roadmap repository.

@wardi
Copy link
Contributor

wardi commented May 20, 2015

If we're planning on expanding this to support arbitrary options I don't see the pattern.

What about

ENV_VAR_WHITELIST = {
    'sqlalchemy.url': 'CKAN_INI_SQLALCHEMY_URL',
    'ckan.datastore.write_url': 'CKAN_INI_CKAN_DATASTORE_WRITE_URL',
    'ckan.datastore.read_url': 'CKAN_INI_CKAN_DATASTORE_READ_URL',
    'solr_url': 'CKAN_INI_SOLR_URL',
    'ckan.site_id': 'CKAN_INI_CKAN_SITE_ID',
    'smtp.server': 'CKAN_INI_SMTP_SERVER',
    'smtp.starttls': 'CKAN_INI_SMTP_STARTTLS',
    'smtp.user': 'CKAN_INI_SMTP_USER',
    'smtp.password': 'CKAN_INI_SMTP_PASSWORD',
    'smtp.mail_from': 'CKAN_INI_SMTP_MAIL_FROM'
}

@wardi
Copy link
Contributor

wardi commented May 20, 2015

Hmm. still no pattern with the varying _'s and .'s. Also we can't support other INI sections

@amercader
Copy link
Member

This list of env variables has no reference (even in this Issue) saying how you selected those that are in the list. Yet I know there's been a lot of discussion about the purpose, precedent and security (not that I've followed this closely). Perhaps you could add a note in the code or refer to a doc with the thinking? Otherwise someone may come along and add others willy nilly and the thinking will count for nought.

@davidread as part of ongoing work with #2429 I'm writing some docs on the config options section about env vars in config. I'll add the reasoning for choosing them there (basically these are critical settings needed when setting up the application, and env vars make easy to automate and orchestrate deployments)

@amercader
Copy link
Member

@wardi I don't follow regarding the pattern. These are the same keys as in the config object, uppercased, with CKAN_ prepended and points replaced by underscores.

I'm not sure we should support other sections other than [app.main] at the application level.

@wardi
Copy link
Contributor

wardi commented May 20, 2015

@amercader What I mean is there's no way to automatically determine that an environment variable is supposed to be a ckan config setting, and what that setting should be, in the event that we want that sort of thing.

CKAN_ is prepended, but not if the target already starts with "ckan." e.g. CKAN_SITE_ID => ckan.site_id but CKAN_SOLR_URL => solr_url

@amercader
Copy link
Member

I see.
I can't think of a pattern that would allow to automatically match an env var to an existing config option that would take into account non-existant ckan. prefixes and points and underscores (points in env vars seems like a bad idea).

A more sensible approach seems to be allowing extensions to modify this mapping of allowed env vars, similarly to how we are allowing to define which config options can be edited.

@wardi
Copy link
Contributor

wardi commented May 20, 2015

So add a new method that could define environment variables and which config options they map to? That would be flexible, but it would be really nice to be able to guess based on the configuration option what the corresponding environment variable would be called.

The scheme in this PR "uppercase; replace dots with underscores; add a CKAN_ to the front if there isn't one already" isn't too bad. I realise it doesn't need to be reversible if we're declaring the settings we want as environment vars as you suggest.

@smotornyuk smotornyuk deleted the 2429-config-env-var branch December 19, 2018 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants