bigbluebutton/bbb-webrtc-sfu

Extend the configuration scheme to allow an override file

prlanzarin opened this issue · 7 comments

prlanzarin commented

Is your update request related to a problem? Please describe.

We need a way to specify an override configuration file that lives somewhere else other than /<app>/config/*.

While node-config naturally allows overrides based on NODE_ENV, the config directory is hardcoded right now.

Add some way to specify the override file location (/etc/*) and then merge that with the base configuration.

More

Same rationale as:

Target is 2.3, so development/master branches.

prlanzarin commented

@schrd

Doing some digging now.
Turns out there's an easy way to use override files with node-config in its latest version. I've updated the development branch with the latest node-config version (see #84).

The approach is as follows:

  • Set the $NODE_CONFIG_DIR environment variable for the service to point to all directories you wish to use as configuration directories. e.g.: $NODE_CONFIG_DIR="/etc/bigbluebutton/bbb-webrtc-sfu/:/usr/local/bigbluebutton/bbb-webrtc-sfu/config/";
  • Add an override file in /etc/bigbluebutton/bbb-webrtc-sfu/ respecting node-config's file precedence (see https://github.com/lorenwest/node-config/wiki/Configuration-Files#file-load-order). Our current /usr/local/.../config/default.yml is the lowest precedence order, so anything that goes above it should work. I recommend using production.yml as the override file since it is the run environment the package uses AFAIK. So e.g.: /etc/bigbluebutton/bbb-webrtc-sfu/production.yml
  • When those are properly set, config merging is automagical.

Does that work for you?

schrd commented

@prlanzarin

This would work. I'd prefer to have all config files in /etc/bigbluebutton but this is a minor issue if bbb-webrtc-sfu has its own directory. I wonder if the fact that arrays are not merged but replaced might break this override mechanisms for setups with multiple kurentos (see https://github.com/lorenwest/node-config/wiki/Configuration-Files#arrays-are-merged-by-replacement).

The goal is to allow operators to change some configuration values and not having to copy all of the settings, so that developers could add new configuration options and ship defaults which then would be applied automatically. With the way array replacement works, this would IMHO not possible. A workaround would be to change the arrays into objects. One config setting that comes into my mind is the multiplekurentos option.

prlanzarin commented

@schrd

The goal is to allow operators to change some configuration values and not having to copy all of the settings, so that developers could add new configuration options and ship defaults which then would be applied automatically. With the way array replacement works, this would IMHO not possible. A workaround would be to change the arrays into objects. One config setting that comes into my mind is the multiplekurentos option.

My 2cts is that's not really a problem, but a good thing. IMHO the fact that arrays are merged by replacement is safer than arbitrarily merging them according to a logic which might or might not be clear to the operator.

The operator will not need to copy all of the settings. The operator will need to use a named setting in the override as they would with any other setting. The array configs webrtc-sfu has are named at their root (eg modules:, kurento:). Member of the arrays are of the same object type, but unnamed. The arrays are not sorted, so fully merging (instead of replacing) would need to be positionally aware which would need the operator to keep track to any "ordering" changes to upstream configuration arrays, which is weird.

I have the same rationale about the HTML5 settings. Arrays should be replaced, not merged.

The example you give about spinning up multiple KMSs is a good one. A thought experiment:

Scenario 1:

  • Upstream ships with default single KMS: kurento: [ kms-entry1 ]
  • Operator overrides it with three KMSs: kurento: [ kms-entry1, kms-entry2, kms-entry3 ]. This array will be put in place of the default entirely, either with replacement or merging. Both work.

Scenario 2:

  • Upstream turns its default to 3 KMS: kurento: [ kms-entry1, kms-entry2, kms-entry3 ]
  • Operator wants one of the following (which are all possible)
    • Roll back to single Kurento:
      • with replacement, kurento: [ kms-entry1 ] works (and makes sense)
      • with merging, how do we roll back to a single one?
    • Add a 4th Kurento instance to also handle mediaType: main (cameras) and split camera load in 2. This works (and I know for a fact it's a valid use case)
      • with replacement, kurento: [ kms-entry1, kms-entry2, kms-entry3, kms-entry4 ] works (and makes sense)
      • with merging: what does the operator need to do to add a fourth entry? If they put a single instance which is not in the original config (ie, kurento: [ kms-entry4 ], with the URL different than 1-3 and mediaType: main), the merging will not add a new one, but instead merge entry4 with entry1. The operator will end up having to do as they would with replacement.
prlanzarin commented

@schrd ping. Is this good or do you think I need to adjust something/find something else?

schrd commented

@prlanzarin I think this works. You are right regarding the array configuration, replacing is better than merging.

prlanzarin commented

Ok, cool. I'll close this then. Anything else, just ping me.