Create config "service"

[ionut-arm]

Given that with increasing complexity we will most likely see a need for accessing configuration values (e.g. for logging) throughout the service, I propose creating a structure that will act as a "config service".

The service will rely on a Config struct wrapped in a global singleton. The config will have a builder which initializes the state and all other methods will be "object-less":

pub static CONFIG: LocalStorage<Config> = LocalStorage::new();

pub struct Config {
  verbose_log: bool,
  ...
}

impl Config {
  pub fn verbose_log() -> bool {
    CONFIG.get().verbose_log
  }
}

The configuration can then be accessed from anywhere in the service as Config::verbose_log(). Whenever we need to add more flags/configuration values, this can be done quite easily, without having to change any signatures/interfaces apart from Config.

[hug-dev]

I definitely see the simplicity of doing something like this. I can make some comments however:

• this seems to be a Read-Only singleton, meaning that it will not handle configuration reload and that is why it can work without synchronization primitives
• to handle configuration reload and have something mutable, a RwLock will be needed around that structure and then to access the data, the locking will be needed.
• what would be put in the Config structure? What do we decide to put in that struct and what do we decide to give to components during initialisation?

I have always thought (but maybe I am wrong!) that limiting the scope of variables and only giving to different components what they need is better/safer idiom. If a value is truely needed everywhere, then it would make sense to have global data.

In the case of verbose_log, as this value is read-only in-between configuration reloads it could fit well inside the the Provider structures as it then won't even need any synchronisation?

Also, what if we have in that Config structure a mix of different values needed by different providers? That would look a bit like our old Configuration structure where we had one structure for all providers and providers were only picking from that structure what they needed.
If we mix those things together in the same read-write global Config structure, then we might have to synchronise two providers wanting to access different values in the same structure.

[ionut-arm]

this seems to be a Read-Only singleton, meaning that it will not handle configuration reload and that is why it can work without synchronization primitives
In the case of verbose_log, as this value is read-only in-between configuration reloads it could fit well inside the the Provider structures as it then won't even need any synchronisation?

We already handle the thread management problem because you can't just destroy providers and other such structures (which are themselves readonly) if you don't stop the threadpool. Here's the code:

 if reload_signal.swap(false, Ordering::Relaxed) {
            let _ = sd_notify::notify(false, &[sd_notify::NotifyState::Reloading]);
            info!("SIGHUP signal received. Reloading the configuration...");

            threadpool.join();
...

so that's covered - when you recreate the Config structure it happens in a single-threaded environment where you don't need any synchronisation.

I have always thought (but maybe I am wrong!) that limiting the scope of variables and only giving to different components what they need is better/safer idiom.

It is, when it only affects those variables. We could just give this data to the providers (though even that would be overkill, having all providers share these flags), but we'd have to also put it in the key info managers, authenticators etc. Logging is a cross-cutting concern and other languages have frameworks that allow you to handle these in a way that affects everything at the same time (I've not checked for Rust, tbh). My idea is that since some configuration values apply equally to everything in the service, they should be available globally. Another configuration value like this might be, for example, a flag for whether deprecated values should be rejected or not.

Also, what if we have in that Config structure a mix of different values needed by different providers?

That's not what I had in mind, I only intend to use this for configuration values that affect similar mechanisms everywhere in the service.

If we mix those things together in the same read-write global Config structure, then we might have to synchronise two providers wanting to access different values in the same structure.

Not needed since it will be read-only as per what I described above.

[hug-dev]

when you recreate the Config structure it happens in a single-threaded environment where you don't need any synchronisation.

How to recreate it if it is a static variable?

[ionut-arm]

when you recreate the Config structure it happens in a single-threaded environment where you don't need any synchronisation.

How to recreate it if it is a static variable?

True, I guess Rust wouldn't normally let you do that, but we can make it let us. Given that we know that the usage pattern is safe, we can do something like

struct ConfigWrapper(RefCell<Config>);

unsafe impl Send for ConfigWrapper {}

unsafe impl Sync for ConfigWrapper {}

static CONFIG: ConfigWrapper = ...

And only allow construction of ConfigWrapper/setting of CONFIG through the builder.

[hug-dev]

Yes we could do it using unsafe as it would be modified in a single-threaded moment. But that would also remove compile-time checks if we ever modify that part of the code to be multi-threaded in any time in the future.

Could we list the config fields that would need, today, this feature? For verbose_logs, is it a security option to hide logs coming from PKCS11 and TSS libraries?

I guess I am worried that we trade safety for ease of use of configuration.

[ionut-arm]

Done in #182 - thread safety was fixed with using atomic structures within the GlobalConfig structure