fabric-testbed / fabrictestbed-extensions

Extensions for the fabric APUI/CLI.
MIT License
7 stars 12 forks source link

Standardize configuration, maybe? #127

Closed sajith closed 9 months ago

sajith commented 1 year ago

FABlib's configuration file is something of an odd duck for three reasons: (1) because of its location, and (2) because of its contents, and (3) because of order of precedence in which configuration file overrides environment variables (usually it is the other way around).

First, the location: the configuration file is expected to be found in ~/work/fabric_config/fabric_rc:

https://github.com/fabric-testbed/fabrictestbed-extensions/blob/3f8c55be24afb04adb07a54b8ee1db88aae9acd1/fabrictestbed_extensions/fablib/fablib.py#L532

This is different from (for example) ~/.config/fabric-testbed.net/ on Linux distros, and ~/Library/Application Support/fabric-testbed.net on macOS, and whatever similar thing they might have on Windows. The library platformdirs can help us with this, and platformdirs is already an indirect dependency for us because jupyter-core depends on it.

Second, format of the configuration file:

export FABRIC_CREDMGR_HOST=cm.fabric-testbed.net
export FABRIC_ORCHESTRATOR_HOST=orchestrator.fabric-testbed.net

export FABRIC_PROJECT_ID=df3efd91-bd30-47f2-aaed-244fee6f1da5
export FABRIC_TOKEN_LOCATION=/home/fabric/.tokens.json

export FABRIC_BASTION_HOST=bastion.fabric-testbed.net
...

Good thing about this format is that we can source it from bash, although maybe not from other shells. Bad thing is that it is a bit of an odd format for a configuration file, and we use hand-rolled code to parse it:

https://github.com/fabric-testbed/fabrictestbed-extensions/blob/3f8c55be24afb04adb07a54b8ee1db88aae9acd1/fabrictestbed_extensions/fablib/fablib.py#L541-L552

Perhaps we can use Python's configparser to get it to a less bash-like shape, and avoid having a custom parser? And then the config file would look like:

[DEFAULT]
cm_host = cm.fabric-testbed.net
orchestrator_host = orchestrator.fabric-testbed.net
project_id = df3efd91-bd30-47f2-aaed-244fee6f1da5
token_path = /path/to/.tokens.json
bastion_host = bastion.fabric-testbed.net
...

The current configuration style works well enough, and it is simple and understandable, and we have an existing workflow when onboarding new FABRIC users. I am wondering if we should look at making it a little more "standard-ish", and if it is worth the trouble.

The third thing is that environment variables are usually used to quickly override settings from configuration files, and not the other way round.

Now, none of the above-stated three things are deal-breakers, and there's more urgent work to be done in fablib. There's also that changing the existing behavior has costs.

If we try to change the existing configuration method, I imagine that the fallout will involve: (1) updating the configure.ipynb notebook accordingly, (2) communicating the change to FABRIC users early, (3) having some way to migrate the existing configurations, both in FABRIC-hosted JupyterHub instance and elsewhere, and (4) we will also need to decide if we're going to break backward compatibility or not, and if we do want to break it, how to go about it.

I just wanted to write down my thoughts (mostly because I noticed that we are doing things our special way, and I'm not sure if it is a good thing or a bad thing), do some bikeshedding collect some thoughts, and, in the end, if we do end up making any changes, ensure that we make them in a considered manner.

sajith commented 1 year ago

Also there are several manual (and therefore error-prone) steps involved in configuring FABlib for the first time. Perhaps we can have a small cli for these tasks, such as fablib configure and maybe fablib update-tokens and the such?

To give credit where it is due: this idea from Microsoft Planetary Computer's Python SDK, which offers a planetarycomputer configure command.

https://pypi.org/project/planetary-computer/

sajith commented 1 year ago

Perhaps we could use python-dotenv instead of configparser.

kthare10 commented 10 months ago

Do we still want fabric_rc to bash environment file?

paul-ruth commented 10 months ago

I think it would be good to keep it as an option for backward compatibility. Hopefully, there is a bash/env reader library we can use. Then it could default to a newer config style (.yml ?) but fall back to fabric_rcif that is all that is found.

sajith commented 9 months ago

Perhaps we can add a command like fablib-cli env (or pull fabric-cli into fablib repository and add an env subcommand to it) that will emit exports the way ssh-agent does:

$ ssh-agent -s # for bash
SSH_AUTH_SOCK=/tmp/ssh-XXXXXXGrSGCL/agent.1103238; export SSH_AUTH_SOCK;
SSH_AGENT_PID=1103239; export SSH_AGENT_PID;
echo Agent pid 1103239;

or, in the case of csh like shells:

$ ssh-agent -c
setenv SSH_AUTH_SOCK /tmp/ssh-XXXXXXfgyMqr/agent.1103652;
setenv SSH_AGENT_PID 1103653;
echo Agent pid 1103653;

That way eval $(fablib-cli env) should add the environment variables to the current shell. We might be breaking only some compatibility if we remove fabric_rc, but not all.