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

Environment variables support #37

Open
misuzu opened this issue Oct 5, 2024 · 10 comments
Open

Environment variables support #37

misuzu opened this issue Oct 5, 2024 · 10 comments
Assignees

Comments

@misuzu
Copy link

misuzu commented Oct 5, 2024

Is your feature request related to a problem? Please describe.
When dealing with a lot of servers it's impossible to use automatic deployment tools (e.g. docker-compose) because it's required to edit configuration files manually.

Describe the solution you'd like
The ability to override values in config using environment variables.
Something like this: https://luckperms.net/wiki/Configuration#environment-variables

Describe alternatives you've considered
Copying the entire configuration files between servers, but it quickly becomes unmaintainable if I need to use different values for different servers. The ability to only set options that needs changing is much easier and convenient.
I've tried generating config file with the only options I've changed but e.g. HuskHomes/HuskChat/Velocitab just overwrites some of the settings (the nested ones).
#26 should probably fix this, but it would be better to not deal with files at all.

Additional context

@Exlll
Copy link
Owner

Exlll commented Oct 5, 2024

Hey, @misuzu! Thanks for your suggestion, sounds like an useful addition to the library 🙂

Do you have an idea of how this feature should be used? Two possibilities come quickly to mind: either use a special class or an annotation.

The special class version could look like this:

private Env host = new Env("MY_HOST_ENVVAR", /* the default value if env var not set */ "127.0.0.1");
private Env port = new Env("MY_PORT_ENVVAR", 25565);

String h = host.getValue(String.class)
int p = port.getValue(int.class)

... and the annotation version like this:

@Env("MY_HOST_ENVVAR")
private String host = "127.0.0.1";
@Env("MY_PORT_ENVVAR")
private int port = 25565;

Perhaps you got other ideas?

@misuzu
Copy link
Author

misuzu commented Oct 5, 2024

Perhaps you got other ideas?

Maybe there's a way to automatically generate environment variables keys and if that keys have values use that values instead of the values in the config file?
Ideally plugins writes should not have to do anything (besides updating the ConfigLib and maybe enabling the feature), or it will be pretty much pointless.
Here's how LuckPerms does this: https://github.com/LuckPerms/LuckPerms/blob/9d80de5d5cbc15c75e01e4d1faf143fb9906ab42/common/src/main/java/me/lucko/luckperms/common/config/generic/adapter/EnvironmentVariableConfigAdapter.java#L44

@Exlll
Copy link
Owner

Exlll commented Oct 5, 2024

Automatically resolving the names of auto-generated environment variables would currently (i.e. without some major rewrite) only be possible for the simple case in which configurations are not nested. For example, if you have a configuration like this

package a.b.c;
record MyConfig(int k) {}

then an environment variable CONFIGLIB_a.b.c.MyConfig.k could be used to set the value of i during deserialization. Here, the package has to be included to guarantee uniqueness between different MyConfig class that could live in different packages (e.g. ones that come from different plugins).

For the following example, however, no auto-generated environment variables can be used.

package a.b.c;
record RootConfig(SubConfig s) {}

package d.e.f;
record SubConfig(int k) {}

The reason for that is that when the library is deserializing an object it doesn't know about all the parents of this object. In the example above, when SubConfig is deserialized, the library doesn't know that its parent is RootConfig. (Besides, an auto-generated varaible name like CONFIGLIB_a.b.c.RootConfig.s_d.e.f.SubConfig.k looks pretty bad in my opinion; the name becomes even worse when dealing with nested classes.)

Another problem I see is that the name implicitly comes from the structure of your code, most likely without any documentation: What happens when the plugin author moves RootConfig or SubConfig to a different package? Then the field won't be set appropriately and the user will need to change the name of their environment variables.

Also, from a security perspective, I don't like the idea of having some magic way of updating configuration values and no indication that this is wanted.

And as a last point: Without special class/annotation support you cannot access the values of other environment variables like, for example, $USER or $PATH and any other variables your system might set.


When going for annotation based support for environment variables, I can see a solution where support for setting values via annotation can be enabled on a type level like in this example:

package a.b.c;

@Env(prefix = "MY_PLUGIN")
record MyConfig(int a, int b){}

This would allow setting the values for a and b via MY_PLUGIN_a and MY_PLUGIN_b, respectively. This way, since the prefix is deliberately chosen by plugin authors, the CONFIGLIB_ prefix and the package can both be omitted.

@misuzu
Copy link
Author

misuzu commented Oct 6, 2024

Another problem I see is that the name implicitly comes from the structure of your code, most likely without any documentation: What happens when the plugin author moves RootConfig or SubConfig to a different package? Then the field won't be set appropriately and the user will need to change the name of their environment variables.

The environment variables keys should be generated from the structure of config files, not code. Using the code structure to do this wouldn't make sense.

Also, from a security perspective, I don't like the idea of having some magic way of updating configuration values and no indication that this is wanted.

The LuckPerms just prints used environment variables to stdout on startup, it makes it pretty obvious where the configuration came from:

<..........>
[18:13:19] [Server thread/INFO]: [LuckPerms] Loading configuration...
[18:13:19] [Server thread/INFO]: [LuckPerms] Resolved configuration value from environment variable: LUCKPERMS_DATA_ADDRESS = localhost
[18:13:19] [Server thread/INFO]: [LuckPerms] Resolved configuration value from environment variable: LUCKPERMS_DATA_DATABASE = minecraft
[18:13:19] [Server thread/INFO]: [LuckPerms] Resolved configuration value from environment variable: LUCKPERMS_DATA_USERNAME = minecraft
[18:13:19] [Server thread/INFO]: [LuckPerms] Resolved configuration value from environment variable: LUCKPERMS_STORAGE_METHOD = postgresql
[18:13:20] [Server thread/INFO]: [LuckPerms] Loading storage provider... [POSTGRESQL]
<..........>

And as a last point: Without special class/annotation support you cannot access the values of other environment variables like, for example, $USER or $PATH and any other variables your system might set.

Why would a plugin author do this? The System.getenv for such cases is a way to go, using config files to read such keys would be really weird.

The user interface should look something like this:
Suppose we have plugins which have the following configuration files (all of them are using ConfigLib):
https://william278.net/docs/huskchat/config-files
https://william278.net/docs/huskhomes/config-files
https://william278.net/docs/husksync/config-file

If the user needs to override the value of replacers.EMOJI.enabled in filters.yml for HuskChat, adding the HUSKCHAT_FILTERS_REPLACERS_EMOJI_ENABLED="false" environment variable should override that value in that file.
By the same logic, the value of message_command.group_messages.enabled in config.yml should be overridable by setting HUSKCHAT_CONFIG_MESSAGE_COMMAND_GROUP_MESSAGES_ENABLED="false".

So a <plugin-name>-<config-file>-<nested-config-key> key structure is pretty much a requirement.

A more real life example, if I need to configure database backend and enable/disable some features I just include the following environment variables to all servers that needs such configuration:

HUSKHOMES_CONFIG_DATABASE_TYPE="POSTGRESQL"
HUSKHOMES_CONFIG_DATABASE_CREDENTIALS_HOST="localhost"
HUSKHOMES_CONFIG_DATABASE_CREDENTIALS_PORT="5432"
HUSKHOMES_CONFIG_DATABASE_CREDENTIALS_DATABASE="minecraft"
HUSKHOMES_CONFIG_DATABASE_CREDENTIALS_USERNAME="minecraft"
HUSKHOMES_CONFIG_DATABASE_CREDENTIALS_PASSWORD=""
HUSKHOMES_CONFIG_CROSS_SERVER_ENABLED="true"

HUSKSYNC_CONFIG_DATABASE_TYPE="POSTGRES"
HUSKSYNC_CONFIG_DATABASE_CREDENTIALS_HOST="localhost"
HUSKSYNC_CONFIG_DATABASE_CREDENTIALS_PORT="5432"
HUSKSYNC_CONFIG_DATABASE_CREDENTIALS_DATABASE="minecraft"
HUSKSYNC_CONFIG_DATABASE_CREDENTIALS_USERNAME="minecraft"
HUSKSYNC_CONFIG_DATABASE_CREDENTIALS_PASSWORD=""
HUSKSYNC_CONFIG_SYNCHRONIZATION_FEATURES_GAME_MODE="false"
HUSKSYNC_CONFIG_SYNCHRONIZATION_FEATURES_FLIGHT_STATUS="false"

When going for annotation based support for environment variables, I can see a solution where support for setting values via annotation can be enabled on a type level like in this example:

If there's a need for plugins authors to make such changes there wouldn't be much value adding this feature to the ConfigLib, plugins authors could already just use System.getenv instead.

@Exlll
Copy link
Owner

Exlll commented Oct 6, 2024

Alright, thanks for the clarifying examples, now I better understand how you would like to use this feature.

The fact that during deserialization objects don't know about their parents (as mentioned in my previous post) still remains. That means that the replacement of environment variables would have to happen earlier in the process. This could possibly happen during a post-processing step of the Map instance that the YAML parser returns.

Since the core parts of this library are plugin agnostic and the YAML content does not necessarily need to be loaded from a file (reading from InputStream is supported), plugin authors have to at least somehow provide the prefix manually, for example, via a YamlConfigurationProperties object:

YamlConfigurationProperties.newBuilder()
        .resolveEnvironmentVariables("HUSKCHAT_FILTERS_")
        // ...or alternatively
        .resolveEnvironmentVariables((String envVar) -> envVar.startsWith("HUSKCHAT_FILTERS_"))
        .build();

The library would then iterate over all environment variables, select the ones with the correct prefix, split each of them at some character (by default _) and finally try to find and update that variable in the (possibly nested) map instance returned from the YAML parser.

Because _ is a valid member for YAML fields, the character that is used for splitting environment variables should be configurable. Because YAML is case-sensitive, the environment variables should also be used in a case-sensitive manner to avoid ambiguities. For example, for this YAML content, there should be two separate environment variables, namely PREFIX_alpha_beta and PREFIX_ALPHA_BETA.

alpha:
  beta: 1
ALPHA:
  BETA: 2 

What do you think?

If you want a solution that requires absolutely no change from plugin authors, then what you want is not possible, because the information which plugin called YamlConfigurations.load/read is not available to the library.

@misuzu
Copy link
Author

misuzu commented Oct 8, 2024

Here's how I'd do it:

  1. generate "default" config
  2. use config from step 1 to generate config from environment variables
  3. load config file from disk
  4. merge config from step 2 into config from step 3

The most difficult part is step 2, here's a POC in python (I don't know java, sorry):

def get_env_by_config(environ: dict, config: dict, prefix: str) -> dict:
    result = {}
    for key, value in config.items():
        env_key = prefix + '_' + key.upper()
        if isinstance(value, dict):
            envs = get_env_by_config(environ, value, env_key)
            if envs:
                result[key] = envs
        elif isinstance(value, list):
            # could be handled by something more sophisticated
            # but at this point it's not that useful
            continue
        else:
            env_value = environ.get(env_key)
            print(env_key, '=', env_value)
            if env_value is not None:
                if isinstance(value, str):
                    result[key] = env_value
                elif isinstance(value, bool):
                    result[key] = env_value == 'true'
                elif isinstance(value, int):
                    result[key] = int(env_value)
                elif isinstance(value, float):
                    result[key] = float(env_value)
    return result


result = get_env_by_config(
    {
        'MYPLUGIN_TEST_DATABASE_TYPE': 'POSTGRES',
        'MYPLUGIN_TEST_DATABASE_CREDENTIALS_HOST': '127.0.0.1',
        'MYPLUGIN_TEST_DATABASE_CREDENTIALS_PORT': '5432',
        'MYPLUGIN_TEST_SYNCHRONIZATION_FEATURES_GAME_MODE': 'false',
        'MYPLUGIN_TEST_SYNCHRONIZATION_FEATURES_FLIGHT_STATUS': 'true',
    },
    {
        'database': {
            'type': 'MYSQL',
            'credentials': {
                'host': 'localhost',
                'port': 3306,
                'username': 'root',
                'password': '',
            },
        },
        'synchronization': {
            'features': {
                'inventory': True,
                'game_mode': True,
                'flight_status': False,
            },
        },
    },
    'MYPLUGIN_TEST',
)
print(result)

The first argument is basically a mock of os.environ, the second one is a "default" config, and a third is a key prefix.

Here's an output of this script:

% python envs.py
MYPLUGIN_TEST_DATABASE_TYPE = POSTGRES
MYPLUGIN_TEST_DATABASE_CREDENTIALS_HOST = 127.0.0.1
MYPLUGIN_TEST_DATABASE_CREDENTIALS_PORT = 5432
MYPLUGIN_TEST_DATABASE_CREDENTIALS_USERNAME = None
MYPLUGIN_TEST_DATABASE_CREDENTIALS_PASSWORD = None
MYPLUGIN_TEST_SYNCHRONIZATION_FEATURES_INVENTORY = None
MYPLUGIN_TEST_SYNCHRONIZATION_FEATURES_GAME_MODE = false
MYPLUGIN_TEST_SYNCHRONIZATION_FEATURES_FLIGHT_STATUS = true
{'database': {'type': 'POSTGRES', 'credentials': {'host': '127.0.0.1', 'port': 5432}}, 'synchronization': {'features': {'game_mode': False, 'flight_status': True}}}

@Exlll
Copy link
Owner

Exlll commented Oct 8, 2024

Again, if you want a solution that requires absolutely no change from plugin authors, then what you want is not possible. Plugins authors need to change their code and manually pass a prefix (MYPLUGIN_TEST in your example) to ConfigLib. ConfigLib cannot automatically deduce a prefix because it does not know to which plugin a configuration belongs or from which plugin a configuration is loaded.

If you are okay with that, I can implement this feature as described in my previous post. If you think that it's pointless if it doesn't work without changes from plugin authors (because they can already use System.getenv), then I think that we cannot really move forward with this issue.

@misuzu
Copy link
Author

misuzu commented Oct 9, 2024

If you think that it's pointless if it doesn't work without changes from plugin authors (because they can already use System.getenv), then I think that we cannot really move forward with this issue.

If they have to declare every single environment variable then yes, it's pointless. If they only need to update the lib and enable the feature by adding a prefix for config file or whatever, then it will be useful.

If you are okay with that, I can implement this feature as described in my previous post.

The library would then iterate over all environment variables, select the ones with the correct prefix, split each of them at some character (by default _) and finally try to find and update that variable in the (possibly nested) map instance returned from the YAML parser.

This should work the other way around: by recursively iterating over map and generating environment variables keys on the way, like in my post. Parsing environment variable key by splitting at _ would be basically guessing with uncertain outcome.

Because _ is a valid member for YAML fields, the character that is used for splitting environment variables should be configurable. Because YAML is case-sensitive, the environment variables should also be used in a case-sensitive manner to avoid ambiguities. For example, for this YAML content, there should be two separate environment variables, namely PREFIX_alpha_beta and PREFIX_ALPHA_BETA.

The convention for environment variables keys is to be all uppercase with _ between words. On Windows the environment variables keys are case-insensitive. If there's the same key with a different casing in a plugins config - that's a really weird choice which I haven't seen so far.

Either way, this is how I see this feature based on my experience of using and writing different software configured using environment variables. You aren't obliged to do anything (obviously) and I still have the option of asking for this feature in every single plugin (by manually calling System.getenv for every option that's needed), but it's better to algorithmically solve this once in one place.

@Exlll Exlll self-assigned this Oct 9, 2024
@Exlll
Copy link
Owner

Exlll commented Oct 10, 2024

Alright, I'll try to implement it over the weekend and will keep you updated.

If there's the same key with a different casing in a plugins config - that's a really weird choice which I haven't seen so far.

Yes, I agree that it's weird, but since it's possible, I'd like to be cover it somehow so that it does not cause some subtle bug - will see.

@Exlll
Copy link
Owner

Exlll commented Oct 20, 2024

Quick update: The feature itself is ready; only the documentation is missing. However, I found another quite annoying bug in the library that I need to fix first which might take a little bit more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants