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

Remove runtime configurability of core system components #1602

Merged
merged 15 commits into from Nov 9, 2018

Conversation

Projects
None yet
6 participants
@janl
Member

janl commented Sep 14, 2018

Overview

In the 1.x era of CouchDB, many parts of the core systems were managed via the config system. This is mostly due to in the early days, no good standard patterns for what Erlang apps looked like were obvious. This has changed now.

In addition, being able to change core parts of the database, including what code modules to load when and where, and which OS binaries to run when and where, opened us up to a set of security vulnerabilities, that we want to close once and for all with this PR by no longer allowing runtime configuration of core system parts.

Specifically:

  • daemons
  • [httpd] default_handler
    -httpd_global_handlers
    -httpd_db_handlers
    -httpd_design_handlers
  • vhost_global_handlers
  • redirect_vhost_handler
  • os_daemons
  • query_servers
  • native_query_servers

This patch retains the ability to configure an existing CouchDB installation to, say, add a third party query server, but it’ll require console access to the server and restarting CouchDB from said console.


Details

CouchDB ships with two default query_servers (javascript and coffeescript)
as well as one default native_query_server (query aka mango). These used
to be configured in default.ini in these sections:

[query_servers]
javascript = {{prefix}}/bin/couchjs {{prefix}}/share/server/main.js
coffeescript = {{prefix}}/bin/couchjs {{prefix}}/share/server/main-coffee.js

; enable mango query engine
[native_query_servers]
query = {mango_native_proc, start_link, []}
; erlang query server
; erlang = {couch_native_process, start_link, []}

This allowed end-users post-install and even runtime-changes to which
query servers are enabled and where their binaries live.

This patch changes things, so only a post-install, but not at-runtime
changes are possible from now on.

This still allows people to configure their CouchDB to run a third-
party query server like the somewhat popular Python query server,
but it changes the way the setup is done.

Query Servers

The javascript and coffeescript query servers continue to be enabled
by default. Setup differences have been moved from default.ini to
the couchdb and couchdb.cmd start scripts respectively.

Additional query servers can now be configured using environment
variables:

export COUCHDB_QUERY_SERVER_PYTHON="/path/to/python/query/server.py with args"
couchdb

Where the last segment in the environment variable matches the usual
lowercase(!) query language in the design doc language field.

Multiple query servers can be configured by using more environment
variables.

Native Query Servers

The mango query server continues to be enabled by default. The erlang
query server continues to be disabled by default. This patch adds
a [native_query_servers] enable_erlang_query_server = BOOL setting
(defaults to "false") to enable the erlang query server.

If the legacy configuration for enabling the query server is detected,
that is counted as a true setting as well, so existing configurations
continue to work just fine.

Windows

Since the setting of the ./configure time PREFIX happens during
make release, I had to adapt the couchdb and couchdb.cmd scripts
to have the correct env vars set and the PREFIX replaced there.

I did this to the best of my abilities and research, but this needs
review from the Windows team (Hi Joan! :).

OS Daemons

OS Daemons have been deleted completely as they were deprecated in 2.2.0

Testing recommendations

make check as well as following the instructions above for configuring third party query servers and/or os_daemons as well as enabling the erlang query server.

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

@janl janl referenced this pull request Sep 14, 2018

Merged

fix tests for #1602 #20

janl added a commit to apache/couchdb-documentation that referenced this pull request Sep 14, 2018

@janl

This comment has been minimized.

Member

janl commented Sep 14, 2018

janl added a commit to apache/couchdb-documentation that referenced this pull request Sep 14, 2018

janl added a commit to apache/couchdb-documentation that referenced this pull request Sep 14, 2018

@janl

This comment has been minimized.

Member

janl commented Sep 14, 2018

I’m working through the test fails, but this is worth reviewing already.

@wohali

This comment has been minimized.

Member

wohali commented Sep 14, 2018

So far, the couchdb.cmd file looks good, but I haven't run a full test yet. Waiting until the test suite is cleaned up.

@janl

This comment has been minimized.

Member

janl commented Sep 15, 2018

Latest push also removes couch_httpd_proxy and os_daemons completely

@wohali

This comment has been minimized.

Member

wohali commented Sep 15, 2018

Question: on the COUCHDB_QUERY_SERVER_lang syntax, what happens when lang is erlang? Does this conflict with [native_query_servers] enable_erlang_query_server = true ?

Or query for Mango - same question?

@janl janl force-pushed the feat/conf branch 2 times, most recently from 01e979d to c1cf9f2 Sep 16, 2018

@janl

This comment has been minimized.

Member

janl commented Sep 16, 2018

@wohali good catch, if someone were to do that, it’d override the internal ones. I think that’s still okay, if someone wanted to write a mango engine in, say, Python. We could forbid the configuring of those two specifically, but it’d be introducing quite fiddly code for not much benefit IMHO.

@janl

This comment has been minimized.

Member

janl commented Sep 16, 2018

This is good to review now

@rnewson

This comment has been minimized.

Member

rnewson commented Sep 17, 2018

I'm +1 on removing runtime configurability but would prefer that the configuration is expressed in the .app files of the respective erlang applications. This will allow package maintainers (etc) to more easily tweak at build time if necessary without introducing the runtime malleability that concerns us.

@janl

This comment has been minimized.

Member

janl commented Sep 17, 2018

@rnewson I’m happy to add query server defaults to the .app file for your use-case. I also want to retain the possibility for folks who run binary builds to add their third party query servers via env vars.

@rnewson

This comment has been minimized.

Member

rnewson commented Sep 17, 2018

{application,appname, [
 {description, "blah"},
 {vsn, git},
 {registered,[]},
 {applications,[kernel,stdlib,config,couch_stats]},
 {mod,{appname_app,[]}},
 {env, [
     {httpd_global_handlers, [
         {_all_dbs, couch_httpd_misc_handlers, handle_all_dbs_req},
         ...},
     ...
   ]}
]}. 
@rnewson

This comment has been minimized.

Member

rnewson commented Sep 17, 2018

also noting that changing couchjs stack size is an important ability.

@janl

This comment has been minimized.

Member

janl commented Sep 17, 2018

also noting that changing couchjs stack size is an important ability.

def agree! we could introduce [query_server_config] js_stack_size (and other options) param, and have it apply to new instances spawned after a config change.

@@ -394,25 +390,23 @@ handle_config_terminate(_Server, _Reason, _State) ->
load_conf() ->
%% get vhost globals
VHostGlobals = re:split(config:get("httpd",
"vhost_global_handlers",""), "\\s*,\\s*",[{return, list}]),
VHostGlobals = re:split("_utils, _uuids, _session, _users", "\\s*,\\s*",

This comment has been minimized.

@iilyak

iilyak Sep 17, 2018

Contributor

Did you consider hardcoding it?:

VHostGlobals = ["_utils","_uuids","_session","_users"]

This comment has been minimized.

@janl

janl Sep 17, 2018

Member

good call, might have to keep it dynamic to allow couch.app configurability as per
#1602 (comment) but good call

@wohali

This comment has been minimized.

Member

wohali commented Sep 17, 2018

+1 on the env var approach vs. app files - this makes modification in e.g. Docker containers very straightforward, as well as systemd unit files / init files. "Why not both" is totally fine for me :)

@janl janl added this to the 2.3.0 milestone Sep 19, 2018

@davisp

davisp approved these changes Sep 19, 2018

+1 to everything in general. I don't have a strong opinion either way on the environment variable vs. .app config approaches.

@@ -26,6 +26,16 @@ init([]) ->
worker,
dynamic}
],
Daemons = [
{"index_server", "{couch_index_server, start_link, []}"},

This comment has been minimized.

@davisp

davisp Sep 19, 2018

Member

It seems a bit silly to keep these as strings just to convert all of the terms in the following list comprehension. I'd at least like to remove that bit of silly for this supervisor. I'd also suggest that we just write out the child specs directly in the SecondarySupervisors list above given that's the more traditional way of doing things (or at least using a helper that gets mapped over the name/mfa pairs.

This comment has been minimized.

@janl

janl Nov 9, 2018

Member

Latest push removes the redundant string parsing. I’d accept commits that turn the list comprehension into a helper, or one that writes it all out.

@wohali

This comment has been minimized.

Member

wohali commented Oct 1, 2018

@janl How is httpsd enabled in this new world? Previously that would have been:

[daemons]
httpsd = {chttpd, start_link, [https]}

@janl janl force-pushed the feat/conf branch from c1cf9f2 to ae917e7 Nov 9, 2018

@janl janl force-pushed the feat/conf branch from ae917e7 to 44d04d4 Nov 9, 2018

@janl

This comment has been minimized.

Member

janl commented Nov 9, 2018

I'm +1 on removing runtime configurability but would prefer that the configuration is expressed in the .app files of the respective erlang applications. This will allow package maintainers (etc) to more easily tweak at build time if necessary without introducing the runtime malleability that concerns us.

@rnewson latest push defines all http handlers in couch.app.

@janl

This comment has been minimized.

Member

janl commented Nov 9, 2018

also noting that changing couchjs stack size is an important ability.

Since query servers can still be setup using env vars, the solution to this already exists:

export COUCHDB_QUERY_SERVER_JAVASCRIPT="/path/to/couchjs /path/to/main.js -S STACK SIZE"
couchdb

janl added some commits Nov 9, 2018

feat: change enaabling of chttpsd to `[ssl] enable = true`
The previous configuration `[daemons] httpsd = {chttpd, start_link, [https]}`
is also sill supported for backwards cmpatibility reasons.
@janl

This comment has been minimized.

Member

janl commented Nov 9, 2018

@janl How is httpsd enabled in this new world? Previously that would have been:

@wohali good one, I’ve made this analogous to the native query server:

  1. if the legacy config is found, it is treated as option 2. set to true
  2. now users can do [ssl] enable = BOOL
@janl

This comment has been minimized.

Member

janl commented Nov 9, 2018

Pending any test issues and/or review notes, this is good to go now. Thanks everyone for the input!

@rnewson rnewson self-requested a review Nov 9, 2018

@rnewson

rnewson approved these changes Nov 9, 2018

very neat, and long overdue.

@janl janl merged commit c850ffa into master Nov 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@janl janl deleted the feat/conf branch Nov 9, 2018

janl added a commit that referenced this pull request Nov 9, 2018

janl added a commit that referenced this pull request Nov 9, 2018

janl added a commit that referenced this pull request Nov 9, 2018

@jiangphcn jiangphcn referenced this pull request Nov 20, 2018

Merged

Remove unnecessary setting on chttpd_view_test #1750

2 of 3 tasks complete

wohali added a commit to apache/couchdb-documentation that referenced this pull request Nov 21, 2018

wohali added a commit to apache/couchdb-documentation that referenced this pull request Nov 21, 2018

wohali added a commit to apache/couchdb-documentation that referenced this pull request Nov 21, 2018

wohali added a commit to apache/couchdb-documentation that referenced this pull request Nov 21, 2018

wohali added a commit to apache/couchdb-documentation that referenced this pull request Nov 21, 2018

wohali added a commit to apache/couchdb-documentation that referenced this pull request Nov 21, 2018

@iilyak iilyak referenced this pull request Nov 29, 2018

Merged

Cache query servers in ets table #1778

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment