digital-asset / daml

The Daml smart contract language
https://www.digitalasset.com/developers
Other
803 stars 203 forks source link

Review and refactor configuration for runtime components #4610

Open leo-da opened 4 years ago

leo-da commented 4 years ago

Inconsistencies

Sandbox, JSON API, Extractor and Navigator share some common configuration settings (but use different names for these settings), e.g:

Sandbox currently does not require username/password, JSON API does, don't know about Extractor. I strongly believe username/password should be required.

We currently specify PostgreSQL username/password via a command line option, which is not secure, anyone who has access to the machine can do ps -aef and get this username/password.

There is #3147 issue, which talks about adding support for ledger host/port configuration via daml.yaml.

Proposal

We should:

  1. remove command line configuration options and rely on daml.yaml only
  2. standardize common ledger configuration settings and share the ledger config across all DAML assistant components.

We can standardize the command line options too, but it would simplify the main method logic if we only support daml.yaml configuration with an additional option that allows overriding the path to the default configuration, something like: --config-file=<path-to-file-to-override-default-daml-yaml-config>

CC @S11001001 @lima-da @hurryabit @gerolf-da

cocreature commented 4 years ago

cc @bame-da who had a proposal for providing a consistent UX that allows every option to be specified both as a command line option and via daml.yaml (I don’t have the link handy). I don’t think only supporting daml.yaml makes sense. daml.yaml is a per-project configuration file. If you think of something like DABL where one ledger hosts lots of projects you don’t want to configure that via daml.yaml.

hurryabit commented 4 years ago

To emphasize what @cocreature said, daml.yaml is an SDK concern and not be present on platform deployments. Maybe doing it the other way around and only supporting command-line arguments for the platform components (Sandbox and JSON API) and teach daml-helper how to forward them makes more sense. However, to make this pleasant we still need to standardize the command line flags. IMO, Navigator is an SDK tool and should not end up on deployed platforms. I'm not sure about the future plans for extractor.

leo-da commented 4 years ago

If you think of something like DABL where one ledger hosts lots of projects you don’t want to configure that via daml.yaml.

you also do not want to specify any username/password as a a command line parameter. We would not pass any security audit.

BTW it does not have to be daml.yaml, but if we do what I proposed we will have to add support for --config-file=<path-to-file-to-override-default-config>, so different components could start with different config files and connect to different ledgers.

Or maybe completely remove the concept of a default configuration file and require that all components started explicitly with --config-file.

leo-da commented 4 years ago

Maybe doing it the other way around and only supporting command-line arguments for the platform components (Sandbox and JSON API) and teach daml-helper how to forward them makes more sense.

I would actually prefer this approach, and I was pushing for this initially: https://github.com/digital-asset/daml/issues/3147#issuecomment-542764564

leo-da commented 4 years ago

bernhard 4 minutes ago As long as there are no clashes, I'd suggest you give all components new consistent flags that work both via config file and CLI, and keep the old names as aliases only on CLI

bernhard 4 minutes ago That way you have full backward compatibility, but have a nicer uniform interface going forward

leo-da commented 4 years ago

what about ENV variable based config? @da-tanabe asked for it. Are we doing it?

garyverhaegen-da commented 4 years ago

My personal preference would be for no CLI args and no config file, and just provide everything through environment variables.

Reasons:

  1. It's easy to emulate CLI args, you just pass them first:
ARG1=... ARG2=... ARG3=... daml-command
  1. It's easy to emulate config files for setups that need one:
#!/usr/bin/env bash
export ARG1=...
export ARG2=...
export ARG3=...
daml-command
  1. We only have one way of setting args to support. We are not inventing yet another new format that people will have to support, but instead work within well-understood, well-tooled parameters. Every sysadmin knows how to deal with env vars, every infrastructure management tool has ways to set them.
  2. Security-wise, while env vars are not great, they are pretty much the default everyone uses anyway.

I do agree we should keep backwards compatibility where possible, so I am not suggesting removing existing CLI flags (yet), but moving forward I think env-var only for new flags is a better way forward.

da-tanabe commented 4 years ago

I definitely prefer an env-var based approach because in a Docker/Kubernetes context, it's quite easy to manage configuration that way and slinging around config files can be much more difficult.

Some care needs to be taken to account for arguments that could potentially change over the lifecycle of an application (particularly auth tokens, as they should be refreshed on a regular basis); in the Kubernetes ecosystem, it seems to be commonplace to have a token file (as the SDK already supports) and the token value is read from there. So for some things, this could mean DAML_AUTH_TOKEN_FILE instead of DAML_AUTH_TOKEN (or possibly even supporting both).

bame-da commented 4 years ago

I strongly oppose changing any existing configuration mechanisms at this point. If you want to consolidate how different tools are configured, please do it purely additively:

  1. Add new command line flags that are consistent across the tools and keep the old ones as aliases
  2. Add the ability to configure via a config file
  3. Add the ability to set flags via environment variables

We get some idea of flag usage through telemetry so we can see whether people move off the aliases over time so we can deprecate them.

stefanobaghino-da commented 2 years ago

At this point, aligning the configuration with Canton's is also important.