Standardisation of connection parameters resolution

[jaclarke]

Currently how the connection parameters (host, port, database, user, password and tls cert) are resolved from the various cli options/client connection config options and env vars differs between the cli and the client bindings. (Between the client bindings the behaviour is largely the same, except edgedb-go doesn't implement deprecated features like admin connections).

Since the current behaviour of the client bindings has bugs where the configuration resolved from a project linked instance cannot be overridden (geldata/gel-js#163, this bug also exists for other params and across all bindings), one proposal is that we standardise on the behaviour of the cli, with a few changes.

Current CLI behaviour: (in questionable pseudo code)

if (let dsn = --dsn or EDGEDB_INSTANCE if starts with "edgedb://") {
  let {host, port, path, username, password} = parseUrl(dsn)
  return {
    address: {
      host: host ?? "127.0.0.1"
      port: port ?? 5656
    }
    database: strip prefix "/" from path ?? "edgedb"
    username: username ?? "edgedb"
    password: --password ?? EDGEDB_PASSWORD ?? password
  }
}
if (
  let instance = (
    EDGEDB_INSTANCE // containing instance name
    or --instance
    or (
      if not (--host or --port or EDGEDB_HOST or EDGEDB_PORT) getInstanceFromProject() 
    )
  )
) {
  let {host, port, database, username, password, tlsCertData} = readCredentialsFile(instance)
  return {
    address: {
      host: host ?? "localhost"
      port: port
    }
    database: --database ?? EDGEDB_DATABASE ?? database ?? "edgedb"
    username: --user ?? EDGEDB_USER ?? username
    password: --password ?? EDGEDB_PASSWORD ?? password
    tlsCertData: tlsCertData
  }
}

let host = --host ?? EDGEDB_HOST ?? extractHostFromEDGEDB_PORT() ?? "localhost"
let port = --port ?? EDGEDB_PORT ?? 5656
let database = --database ?? EDGEDB_DATABASE ?? "edgedb"
let username = --user ?? EDGEDB_USER ?? "edgedb"
let password = --password ?? EDGEDB_PASSWORD

if (--admin or host contains "/") {
  let addr = host contains "/" ? host : "/var/run/edgedb"
  if (addr not contains ".s.EDGEDB") addr = `${addr}/.s.EDGEDB.${--admin ? "admin." : ""}${port}`

  return { addr, database, username, password }
}

return {
  address: {host, port}
  database,
  username,
  password,
  tlsCertData: if (--tls-ca-file) readTlsCaFile()
}

or diagram form: https://excalidraw.com/#json=5434124462456832,IlWtTuYO7ArrpRlsPjBljw

Proposed changes to CLI behaviour:

• Check the instance name from --instance before EDGEDB_INSTANCE. Currently the env var can override the instance name explicitly given on the command line
• The client bindings currently support parsing database (and dbname), user, password, tls_cert_file and tls_verify_hostname from the query params of a dsn, and also support multiple addresses as the host and port (dsn, config option, and env vars) accepting comma seperated lists of values. Update the CLI to handle these.

There would be one small difference between CLI and client bindings: we would drop support for admin and the edgedbadmin:// protocol in the client bindings, but keep the --admin option in the CLI.

However there are cases in the CLI behaviour where it may not be inituitive to users why the connection params were resolved as they were. For example if you explicitly provide a dsn with --dsn, the password in the dsn can be invisibly overridden by the EDGEDB_PASSWORD env var, but none of the other params can. So an alternative proposal is to create a simpler set of rules that would be easier for the user to reason about, and update both the cli and client bindings to those rules.

My proposal for these rules would be to have a strict overriding hierarchy: explicit param options would always override env vars, which would override the implicit project linked instance credentials, and within an override level, more specific params (like host, port, database, password, etc..) would override the params within broader params (like dsn and instance).

[tailhook]

multiple addresses as the host and port

I'm currently very skeptical about multiple addresses. There are a bunch of problems with defining how that should work:

1. If connect_timeout*hosts > wait_until_available, should we try every conn once or cap at wait_until_available? (both behaviors can be unexpected).
2. We should probably connect in parallel, but this has another bunch of issues: should we drop other connections when the first succeeds? is "success" defined as authenticated connection or TCP established? Will it created double load on the database? Also I don't want to complicate code with parallel code path for now. But changing it later will lead to surprises.
3. Should pool spread connections across addresses uniformly, or should it connect to a single address? Or does it matter at all? (there are use cases for both).

[elprans]

Multiple addresses are absolutely needed for HA. We need to be able to handle multiple A/AAAA records returned from a DNS lookup as well as handle a list of addresses passed directly in case service catalog lookup happens externally. How to handle the address list is another question and it's something we will likely ask you to configure explicitly (i.e. try addresses in a round robin or use happy eyeballs).

[tailhook]

@elprans, you kinda both contradict and prove my point. So if you need HA with multiple IPs, rather than passing EDGEDB_HOST=xxx,yyy you should configure them explicitly using configuration builder that we want to introduce for a long time. Something along the lines of:

Config().set_round_robin_hosts(['xxx', 'yyy'])
Config().set_load_balanced_hosts(['xxx', 'yyy'])
Config().set_ha_hosts(['xxx', 'yyy'])

(and I'm not sure about command-line at all, although, we probably don't need round-robin ?)

So I still think we have to postpone multiple hosts for now.

[elprans]

But this goes against the idea that a bare connect() should work for you in all environments and that connection parameters can be fully described externally. What you are suggesting would require all apps to parse the environment and construct a Config() manually.

[elprans]

To clarify: I'm not arguing that we should keep or implement support for multiple host addresses now. My point was the answer to your question about why we need them in principle.

[tailhook]

But this goes against the idea that a bare connect() should work for you in all environments

I don't think we need to make every parameter configured by environment. Just most common case. I would say that most common case of HA should use single DNS name and SRV records. Or use better name resolution, e.g. using consul (like stolon does?). But that's another discussion. It seems we agree that it doesn't have to be implemented now.

[tailhook]

My proposal for these rules would be to have a strict overriding hierarchy: explicit param options would always override env vars, which would override the implicit project linked instance credentials, and within an override level, more specific params (like host, port, database, password, etc..) would override the params within broader params (like dsn and instance).

I have few issues with that:

1. dsn should override all parameters of the previous level, except password if that was not specified in DSN (we currently always override password by environment, which is probably bad). So this works:

   EDGEDB_PASSWORD=passwd edgedb --dsn=edgedb://user@host/db
   

   Use case is mostly for config files: edgedb.connect(dsn=config.edgedb_dsn) but avoid keeping secrets in config file.

2. I'm not sure about specificity of project file vs environ. On one hand, it's nice if you can override EDGEDB_DATABASE=test ./tests.py and that would use project's instance with a different database. But on the other hand, if you have environment in your $HOME/.profile for some reason, this has to be overridden by the project config.

[tailhook]

Client TLS cert also?

TLS cert is public. It can be comitted to the same place where config file is.

?password_env=EDGEDB_PASSWORD

Yes, that's also an option, although it's weird and hard to discover one.

Why would you have EdgeDB env vars in .profile on your dev machine? We aren't recommending that anywhere and our edgedb instance/project machinery obviates any need to do this. I think having a clear state machine without weird exceptions is more important for usability than trying to cater for obviously wrong setups.

Well, both options are motivated by easier debugging. People googling for "how to set environment variable" will quickly find export XXX or any equivalent of profile. I'm not sure there is XXX=yyy ./cmd equivalent for windows. And then people will complain that project don't work.

"We aren't recommending that" can't be an ultimate argument. We have already seen that people use edgedb in unexpected ways.

Anyway, I'm not sure either one is clearly superior to other 🤷

[tailhook]

One good way of how we could do overriding environment by project settings more useful is putting warnings:

Warning: EDGEB_HOST is overriden by project config

If we do reverse (override projects settings by the environment) we must assume that those environment variables are put intentionally.

[elprans]

TLS cert is public.

But the key required to sign data with it isn't!

Yes, that's also an option, although it's weird and hard to discover one.

Well, to be honest I don't quite understand the use case of splitting the configuration between env and command-line like this. Why wouldn't you specify the entire DSN in the environment? Per this proposal you can do this:

env EDGEDB_DSN="edgedb://..." EDGEDB_PASSWORD="foo" edgedb

if you're so keen on keeping the password separate.

[tailhook]

@elprans, like I've already said the use case is mostly for config files: edgedb.connect(dsn=config.edgedb_dsn) but avoid keeping secrets in config file.

[tailhook]

TLS cert is public.

But the key required to sign data with it isn't!

Ah, sorry I didn't notice that you're talking about client certificates. In this case yes, client cert private key should be taken from either the environment or from file. And yes this kinda sounds like an argument towards password_env/pasword_file (and similar client cert variables).

[tailhook]

Another question is what priority of EDGEDB_INSTANCE vs EDGEDB_CREDENTIALS_FILE (latter we agreed to add I think). And they have the same specificity.

[elprans]

This is one thing that worries me about this proposal: sub-priorities within a configuration level. Alas, I don't see any other way other than erroring out, which isn't great also.

[tailhook]

Oh, and another question: should we fail client bindings which have neither EDGEDB_HOST/EDGEDB_PORT nor project initialized? (like we currently do for CLI)

[elprans]

Not sure about this. For CLI the motivation was clear: it's easy to run it from the wrong directory, but for apps it's not that clear. On the other hand, the default (localhost, 5656) isn't terribly useful either, except for when you want to test against a dev server, but then TLS is still an issue, so you have to have an instance anyway. So, I'm leaning toward "yes" here. What do you think?

[tailhook]

I would say "Yes" too. I think there are virtually no cases when users will have edgedb on localhost:5656, so its always a mistake. For dev server you either need a credentials file as you said, or set EDGEDB_INSECURE_DEV_MODE which means you can have EDGEDB_PORT=5656 in the same environment config too.

[elprans]

So, upon further reflection on the issue we decided that an explicit and documented set of sub-priorities is the least confusing and most flexible way forward.

I propose the following order, where each subsequent entry overrides the previous within the same configuration layer:

EDGEDB_DSN,
EDGEDB_INSTANCE,
EDGEDB_CREDENTIALS_FILE
EDGEDB_HOST, EDGEDB_PORT, etc

Also, there are two kinds of connection parameters: aggregate (dsn, instance, credentials file), and granular (host, port, etc.). Granular parameters override only their part, whereas aggregate parameters override all granular parameters. For example, an explicit --dsn on the command line overrides EDGEDB_HOST, EDGEDB_PORT, etc. On the other hand, an explicit --port on the command line overrides only the port part of EDGEDB_DSN, if the latter is set:

env EDGEDB_DSN=edgedb://foo:1234/bar edgedb --port 4321 --database my
# --> addr: (foo, 4321) db: my
env EDGEDB_HOST=foo EDGEDB_PORT=1234 EDGEDB_DATABASE=my edgedb --dsn edgedb://
# --> addr: (localhost, 5646) db: edgedb

The advantage of this approach is that it's consistent and well-defined.

The downside is that there is a potential confusion when one has a DSN environment variable set, and does connect(host="my_host") expecting the other options to be set to defaults instead of being taken from the environment. This, however, seems acceptable, as we don't recommend hardcoding connection options anyway (you should almost always use a bare connect() and configure the connection in the environment), and we'll be including the source of each connection option in the connection error message if one occurs.

[elprans]

the default port (5656) would override EDGEDB_PORT?

Yes.

[jaclarke]

When do we look for a project instance? If aggregate parameters provide all the granular parts, then there is no need to look for the project instance if an aggregate parameter is given, but then what do we do if there are only granular parameters? Is there a set of granular parameters that is enough to prevent looking for a project instance (current behaviour is host or port is enough) and the gaps are filled by defaults, or do we always look for a project instance if there are no aggregate parameters?

[elprans]

Good question. Generally, it makes no sense to override the address parameters of an instance, but it does make sense to override other options such as the database name and possibly some other connection options. Perhaps we need to retain the special handling of the EDGEDB_HOST/EDGEDB_PORT pair and treat them as "aggregate"? Basically make it so that EDGEDB_HOST === edgedb://EDGEDB_HOST:5656/

[tailhook]

I think there is a use-case where all credentials are shared between instances but different host is used to access the same instance in different data centers. That means using EDGEDB_CREDENTIALS_FILE with EDGEDB_HOST.

So it would be fine if EDGEDB_HOST/EDGEDB_PORT override project, but use case above makes it moot.

[tailhook]

Also with respect to #2922 (comment)
Should EDGEDB_PORT=tcp://host:1235 override EDGEDB_HOST or vice versa?

[tailhook]

And a DOCKER_HOST=tcp://host:5656 should probably be standardized, or removed (geldata/gel-cli#389). It's currently only supported in CLI which is probably bad.

[elprans]

To be honest, I think we should remove it entirely. --link is deprecated and not recommended anymore.

[tailhook]

I think there might be valid use cases for that (at least in legacy project?). I don't think crashing is a good way to handle this issue. We may ignore and warn, but I'm not sure this is a great idea too.

Edit: Well, since --link is deprecated "ignore and warn" might actually be good enough.

[1st1]

What we decided on the zoom call:

1. Compound connection options: EDGEDB_DSN, EDGEDB_INSTANCE, EDGEDB_CREDENTIALS_FILE, EDGEDB_HOST, EDGEDB_PORT. (They all have equivalents command line parameters)
2. Specifying more than one compound connection option in the command line or in environment causes the server to error out (except HOST and PORT are allowed simultaneously).
3. We do not validate environment variables until we use them. When we do use an environment variable we validate it and throw if it contains garbage. Specifically for EDGEDB_PORT, if it contains tcp://host:port stuff - we ignore it issuing a warning.
4. All connection options may be specified in the query part of the DSN URI either directly (password=foo&...), as a reference to a file containing the value (password_file=%2Ffoo&...), or as a reference to an environment variable containing the value (password_env=SOME_VAR). It is an error to specify multiple aspects of the same variable (e.g password_file&password_env). Likewise it is an error to specify a variable in multiple places in the DSN (such as the password after : and in the query part).

[tailhook]

Am I understand this right?

1. EDGEDB_DSN=edgedb://user1@xxx:1234 edgedb --host yyy connects to edgedb@yyy:5656
2. EDGEDB_DSN=edgedb://user1@xxx:1234 edgedb --port 7777 connects to user1@xxx:7777
   And this use case is not covered: Standardisation of connection parameters resolution #2922 (reply in thread)

[elprans]

And this use case is not covered:

We can introduce explicit EDGEDB_OVERRIDE_HOST to cover that use case if it turns out to be important.

[1st1]

And EDGEDB_OVERRIDE_PORT

[elprans]

Instead of OVERRIDE_x we'd simply allow you to do x_env=SOME_ENV_VAR in the DSN.