Add Settings system

[T-Dnzt]

Issue/Task Number: #436
closes #436

Overview

This PR adds a settings system to the eWallet. What started simple ended up being a very complex feature to many unexpected problems. In order to manage settings/configuration of the eWallet in a way that users can update it without updating the ENV and restarting, a new sub-application had to be created: ewallet_config.

Changes

• Created ewallet_config
• Added the EWalletConfig.Config that acts as an entry point to the configuration system.
• Turn that config module into a GenServer in order to allow other applications to register themselves as settings users. See implementation details for flow.
• Added tests in ewallet_config
• Added test helpers to allow the Config GenServer to be used in the tests of other sub applications

Implementation Details

Flow

Due to some of our libraries requiring their own Application env (arc, etc.), we needed a way to keep using the famous Application.get_env(). In order to do that, the implemented system works in the following way:

1. Seeds have to be run first, to insert the current set of 19 settings used in the eWallet
2. ewallet_config boots up and starts a GenServer allowing other apps to register themselves
3. Other apps register themselves, specifying which settings they want to use
4. The Application envs of those apps are set using the values from the DB
5. When a setting is updated, the GenServer loops through the registered apps and refresh all their configuration. The current implementation doesn't optimize what gets updated, all apps get all their configuration refreshed.
6. To prevent frequent reloads, it is possible to update multiple settings at once.

Settings

Settings are a module interface to the StoredSetting Ecto schema. This interface was needed to allow any data to be stored inside a data field (or an encrypted one encrypted_data) which is a JSON field.

From outside, it only looks like you're passing value (a property of the Setting struct), but in the background, it gets translated into the format supported in StoredSetting.

A stored setting has the following fields:

• uuid
• id
• key: the human-readable identifier of the setting
• data: the unencrypted data of the setting (stored as {value: "something"})
• encrypted_data: the encrypted data of the setting (stored as {value: "something"})
• type: the type of the data stored in data/encrypted_data. The following types are available: "string", "integer", "map", "boolean", "array"
• description: a description of the setting
• options: a list of potential values for the setting
• parent: the key of the parent setting
• parent_value: the value of the parent setting for this one to be enabled
• secret: if the setting data should be encrypted or not
• position: the position of the setting to maintain order and prevent chaos

Here's an example of a setting:

%{
      key: "email_adapter",
      value: "smtp",
      type: "select",
      options: ["smtp", "local", "test"],
      description:
        "When set to local, a local email adapter will be used. Perfect for testing and development."
}

Usage (example with the ewallet_db sub app)

1. Add the list of settings in config.exs for the sub app

config :ewallet_db,
  ecto_repos: [EWalletDB.Repo],
  env: Mix.env(),
  schemas_to_audit_types: audits,
  audit_types_to_schemas: Enum.into(audits, %{}, fn {key, value} -> {value, key} end),
  settings: [
    :base_url,
    :min_password_length,
    :file_storage_adapter,
    :aws_bucket,
    :aws_region,
    :aws_access_key_id,
    :aws_secret_access_key,
    :gcs_bucket,
    :gcs_credentials
  ]

2. Update application.ex to register the app

settings = Application.get_env(:ewallet, :settings)
Config.register_and_load(:ewallet, settings)

3. Done! Anytime, a setting is updated in the DB, the Application env will be reloaded.

Impact

We need to do a good amount of testing in staging to ensure the settings are properly refreshed in all nodes. Other than that, Application.get_env can still be used to retrieve config in other sub apps.

Seeds need to be run before this feature can be used. The setting seeding is ran as part of the usual mix seed command but can also be ran specifically with mix seed --settings. A prompt will ask the user for the base_url value to set - this will be skipped when specifying the auto-yes option -y (it will use the default value).

Changes coming to this PR

• Removing the select type, instead the options field will define if the value has to match some specific values
• Adding validators for the type and the select values
• Changing the options to an array in the database

To Do in future PRs

• Add the endpoints to update those settings
• Add a test testing that config in all nodes get reloaded properly. I currently have the following test that doesn't work - it seems the nodes aren't connecting to the same database, and checking out the repo for each node doesn't seem to help.

defmodule EWalletConfig.MultiNodeTest do
  use ExUnit.Case, async: false
  alias EWalletConfig.{Config, Repo, ConfigTestHelper}
  alias Ecto.Adapters.SQL.Sandbox

  describe "reload_config/1" do
    test "reloads all settings for all nodes" do
      Sandbox.checkout(Repo)

      ConfigTestHelper.spawn([:test1, :test2, :test3])
      nodes = [Node.self() | Node.list()]

      # Not sure if this is needed, doesn't work anyway
      # Enum.each(nodes, fn node ->
      #  :rpc.block_call(node, Sandbox, :checkout, [Repo])
      # end)

      Sandbox.mode(Repo, {:shared, self()})

      Config.insert(%{key: "my_setting", value: "some_value", type: "string"})

      Enum.each(nodes, fn node ->
        :ok = Config.register_and_load(:my_app, [:my_setting], {Config, node})
        value = :rpc.block_call(node, Application, :get_env, [:my_app, :my_setting])
        assert value == "some_value"
      end)

      Config.update("my_setting", %{value: "new_value"})

      Enum.each(nodes, fn node ->
        value = :rpc.block_call(node, Application, :get_env, [:my_app, :my_setting])
        assert value == "new_value"
      end)
    end
  end

[unnawut]

Sending first batch of comments. Just managed to get half way through. - -"

[unnawut]

How do parent, parent_value and position work?

[T-Dnzt]

• position: Position is used to have a constant order for settings that we define. It cannot be updated and is only set at creation. We can add more settings in the seeds later and fix their positions.
• parent and parent_value: those are used to link settings together in a very simple way. No logic is actually implemented for those, it's mostly intended to be used by clients (like the admin panel) to show settings in a logical way. So if someone selects gcs for file_storage, you can show all settings that have file_storage as a parent were parent_value=gcs.

[unnawut]

Could you add this as a code comment? I think this will be easily lost

[unnawut]

Should schema validators be part of EWalletDB and not EWalletConfig? :S

[T-Dnzt]

If it's part of EWalletDB, then we can't use those validation functions in EWalletConfig. EWalletConfig has no dependencies on any other app, but all other apps depend on it directly or indirectly. I could move back some of the functions to EWalletDB tho, since they are not used in the validations in EWalletConfig anymore.

[unnawut]

Moving back would be nice I think. I feel EWalletDB having to depend on validators from EWalletConfig is a bit counterintuitive.

Also, if the EWalletConfig's validators are the same as EWalletDB's, having duplicate code in this is case fine?

[unnawut]

One final note is I'm not sure a race condition will be a problem here. Would be nice to have @sirn have a look at this PR as well.

[T-Dnzt]

@unnawut That's a good point about race conditions. I'm gonna fix it.

[mederic-p]

You should update https://github.com/omisego/ewallet/blob/master/docs/design/components.md

[sirn]

Hmm, I have a feeling that :settings shouldn't be in config (since it's not exactly configurable). Maybe as a @settings [...] in here (i.e. application.ex) instead?

[sirn]

Or if we do it might make sense to use something among the line of:

config :admin_url,
  sender_email: {:ewallet_config, "default"},
  something_else: {:ewallet_config, nil}

…and iterate over the keys. In this case we can simplify EWalletConfig.Config.register_and_load to just:

EWalletConfig.Config.register_and_load(:admin_api)

[T-Dnzt]

Hmm, but from my point of view, it is configurable - you can set which settings you need in the app for it to work, we can add more in the future or remove them later. I can't really see how it's different from other config like generators or the mime_types.

About listing the settings, we could do that, but I don't really like the idea of having default in there. I see the settings as a global system, with one default value for all the apps, don't really want to let each app set their own default. At least, that's how I see it with the current implementation, it might evolve and requires apps to each have different default values 🤷‍♂️

[sirn]

Maybe we can use {:ewallet_config, "key_to_retrieve"} or something (where nil defaults to the key)?

My issue is mainly with having :settings key in config, which doesn't feel exactly right to me. As in, we're defining the values to be populated into app_env in a different ways than how these settings are normally populated (as in DeferredConfig, or other MFA-style configurations).

Also, if we use {:ewallet_config, ...} we could stub it in tests without involving ewallet_config at all.

[T-Dnzt]

@sirn I'm not sure why we need that {:ewallet_config, ... part for? Also, one of the reason it's in the settings in that format is that I'm actually using the whole list of settings in the shared test helper function there https://github.com/omisego/ewallet/blob/436-settings-system/apps/ewallet_config/test/support/config_test_helper.ex#L15.

[sirn]

I think this should be part of the default seeds na (just add it to seed_spec([])'s list rather than a new function to handle it). seed_spec([]) should be the "final" function to get called.

[T-Dnzt]

What I tried to do here is give the option to only run the settings seeds for all our systems that are already deployed (thought it'd be nice :D) but also have the settings seed being run with the default ones. Totally fine to change it, but not sure I follow what you mean, can you give me some code?

[sirn]

Do we still need the default https://example.com/? 🤔

[T-Dnzt]

I guess not, removing it!

[sirn]

Thibault <notifications@github.com> writes: Aha. Got it. It's usually preferrable to have the final function for these type of recursive functions to no longer call itself because it's usually simpler to follow. For now I think it's OK to leave it like this. Although I think we should find a way to handle this case (and case like `--test`) in the future, since we will need more seed types in the future for sure.

…

T-Dnzt commented on this pull request. > @@ -49,7 +49,11 @@ defmodule Mix.Tasks.Omg.Seed do # defp seed_spec([]) do - [{:ewallet_db, :seeds}] + [{:ewallet_db, :seeds}] ++ seed_spec(["--settings"]) What I tried to do here is give the option to only run the settings seeds for all our systems that are already deployed (thought it'd be nice :D). Totally fine to change it, but not sure I follow what you mean, can you give me some code? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #493 (comment)

[sirn]

Well, we already have `{:system, ...}` and `{:apply, ...}` syntax a a result of using DeferredConfig. I think it make sense if we follow the same syntax rather than introducing a new one. Besides it's nicer when we want to make the configuration key use other configuration scheme (e.g. hard-coded or using DeferredConfig), such as in tests, or in development. Thibault <notifications@github.com> writes:

…

T-Dnzt commented on this pull request. > @@ -9,6 +9,10 @@ defmodule AdminAPI.Application do def start(_type, _args) do import Supervisor.Spec DeferredConfig.populate(:admin_api) + + settings = Application.get_env(:admin_api, :settings) @sirn I'm not sure why we need that `{:ewallet_config, ...` part for? Also, one of the reason it's in the settings in that format is that I'm actually using the whole list of settings in the shared test helper function there https://github.com/omisego/ewallet/blob/436-settings-system/apps/ewallet_config/test/support/config_test_helper.ex#L15. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #493 (comment)

[T-Dnzt]

Merging for now, I've given a try to the {:ewallet_config, nil} approach but it would require too many changes, let's do it in another PR.

[sirn]

👌 Thibault <notifications@github.com> writes:

…

Merging for now, I've given a try to the `{:ewallet_config, nil}` approach but it would require too many changes, let's do it in another PR. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #493 (comment)