External client configuration proposal #95

cretz · 2024-09-05T19:51:49Z

Summary

This is the proposal for external client configuration (i.e. config files and profiles)
See it rendered
This is the second round of review (see External client configuration proposal #94 for the first)
Like other proposals, all feedback welcome. If the PR discussion gets too large or the review round comes to an end w/ more review needed, it will be closed and a new one will be created referencing this one.

Notable changes since first round:

Added more usage examples including making configuration files more prominent
Removed the concept of profile-specific environment variables
Changed the default config filename to make it clear it's not for all config that may ever happen, just for clients
Switched file format to TOML
Changed SDKs to use "extension" approach since they'll need a new TOML dependency (there are some outstanding questions here regarding whether to use Rust Core)
Added env var support for gRPC headers

all-sdk/external-client-configuration.md

…adata env vars

cretz · 2024-10-23T14:56:41Z

New updates based on team discussion (will also update PR description):

Switched file format to TOML
Changed SDKs to use "extension" approach since they'll need a new TOML dependency (there are some outstanding questions here regarding whether to use Rust Core)
Added env var support for gRPC headers

mjameswh · 2024-10-23T16:20:20Z

all-sdk/external-client-configuration.md

+
+And then still use CLI like
+
+    temporal workflow list --namespace my-specific-namespace


Uh? Why is this implicitly using the temporal-cloud profile?

Did you mean the profile above to be named default instead? Or add a --profile temporal-cloud argument to this command and the one below, as well as the python snippet?

Also, would this work on Cloud today? or only once we get global endpoints?

If not, I think we should use a non-cloud example, or clearly indicate that this is pending future Cloud changes.

Did you mean the profile above to be named default instead?

Yes, sorry, when translating to TOML in b8cf8fe I accidentally change the profile name, needs to be default, will fix

Also, would this work on Cloud today? or only once we get global endpoints?

Yes for data plane. Clients are just addresses and namespaces and auth. What shared endpoints will provide is a way for cloud ops API to use this as well.

mjameswh · 2024-10-23T16:30:25Z

all-sdk/external-client-configuration.md

+
+* The format is a TOML object with the following fields:
+  * `profile` - Object with keys as profile names. The default profile should be named `default`.
+    * `<name>` - Profile object with configuration values. Profile names are case-insensitive. 💭 Why? They are needed


No longer true:
~~They are needed for environment variables which are case insensitive~~

Still, I think case-insensitive profile names make sense. We could want to reopen the env variable avenue later, and that would be a likely source of error for users anyway.

Are we ok accepting any valid TOML key as profile name? even quoted keys?

I don't think we should.

For the same reasons that we choose to make profile name case-insensitive, I think it would be safer to restrict valid profile names to a subset of Bare Key, e.g. [a-z][a-z0-9-]* or [a-z][a-z0-9]*(?:-[a-z0-9]+)*.

Still, I think case-insensitive profile names make sense

I'm not really looking to protect from typos here. I don't think case sensitivity of profile name is any more painful than case sensitivity of config keys (we require all lowercase) or env var names (we require all upper) or CLI flag names or whatever. If we need to relax case sensitivity later we can, but I think it's normal for people to expect it.

Are we ok accepting any valid TOML key as profile name? even quoted keys?

I am ok with this for the most part, or at least I can't see any obvious reason why not. But I admittedly did not cover advanced validation of things in this proposal. I tend to want to accept loosely and have the usage fail if needed as opposed to have lots of matching validation in lots of different languages.

mjameswh · 2024-10-23T16:53:04Z

all-sdk/external-client-configuration.md

+    * `<name>` - Profile object with configuration values. Profile names are case-insensitive. 💭 Why? They are needed
+      for environment variables which are case insensitive. It is a validation error to have a config file with two
+      separately-cased profile names that are equal case-insensitively.
+      * `address` - Client address, aka gRPC "host:port". This cannot be a URL, it must be `host:port`.


Let's be specific about IPv6 address, e.g.: [ipv6address]:port

I am thinking less specific actually, heh. I am thinking "whatever gRPC wants". For some users that do not care about the cross-language compatibility, they may set this to myresolver:///ignoredvalue for their custom gRPC resolver in Go or something. I only put this "cannot be a URL" comment to clarify because I know Core and others in the past had confusion on whether something was a URL. Looking back, I will probably back off validation here and just say "address passed to the underlying client, we encourage host:port".

Looking back, I will probably back off validation here and just say "address passed to the underlying client, we encourage host:port".

I disagree. temporal config set should validate for what is expected to be the correct format. Not doing so will make this a common source of errors for new users for a benefit that only matters for a very tiny percentage of "expert users", who could anyway still provide "out-of-norm" values by editing the config file by hand.

So what do you believe the validation for address should be if I want to have Go allow a type of value that, say, Python doesn't support, such as a custom resolver? To validate this value properly means to assume how an SDK uses the value or assume that all of our SDKs share a common validation of this value which is not accurate.

We can just tell people that want to use more advanced language-specific addresses that they can't use config. It's a bit unfair, but doable.

Validation in CLI should be based on a least-common-denominator format that we guarantee to be supported in every SDKs to support.

IMO, that should be:

ipv4:port hotname:port [ipv6]:port

Anything more than that should not be accepted as input on temporal config set. That way, we promote using things that will work in all SDKs.

However, I don't think we need the SDK side to validate that. So if a user edit his config file and manually puts a value of another form, the SDK will just pass that to the grpc library, and that may work or that may not. So we're not preventing these advanced users from using config, we just ask them to dismiss the child locks that CLI has in the interest of the vast majority.

mjameswh · 2024-10-23T17:04:58Z

all-sdk/external-client-configuration.md

+      * `tls` - A boolean (true is same as empty object) _or_ an object. Note the default for this value is dependent
+        upon other settings here. 💭 Why? We regret not making TLS the default in the past, so we will make it default
+        if `api_key` is present. If TLS is an object, it can have the following possible fields:
+        * `client_cert_path` - File path to mTLS client cert. Mutually exclusive with `client_cert_data`.


For all path options:

If relative, evaluates relative to the config file?

Is there anything particular that we should specify about paths on Windows, to avoid cross-languages incoherencies? Should we be worried about WSL and similar?

Don't necessarily need to write this just here, but please put it somewhere so that it survives the review.

If relative, evaluates relative to the config file?

Good question. I generally like relative to mean current working directory, but I understand that's confusing for external configs. I will clarify that this should be relative to config file.

Is there anything particular that we should specify about paths on Windows, to avoid cross-languages incoherencies? Should we be worried about WSL and similar?

Nope, not that I can think of

There are three ways this path setting can be set: config file, env var, and programmatically. After some thought, it is confusing to have different ways of specifying the same value have different relative bases, so I think it makes the most sense to be all relative to the current working directory. I will add a point clarifying this.

There are three ways this path setting can be set: config file, env var, and programmatically.

Good point.

I think it makes the most sense to be all relative to the current working directory

I don't see how being relative to the current working directory can be appropriate for config file. That means a profile would only be usable when running CLI or your app from a very specific directory.

I think the only reasonable options are:

In config, support absolute paths only; or

Use config file location for paths specified in config file, and CWD for paths specified through env variables or programmatically; that is, paths being read out of config file would be normalized very early, inside the load function itself and before merging with env variables. Then code later down the road can resolve remaining paths (that would be either from ENV or programmatic) relative to CWD.

Treating these config keys different based on where they are set is confusing IMO. We have a want in our SDKs to be able to load the environment config as an object to be viewed. We want that object to have the exact values the user set, not some resolved form (just in case they want to see what was there or want to save or whatever). How are you going to have different rules for the same config key from different places and still give a full config object with overrides? We will definitely encourage people to use absolute file paths in config, but it is easier for a user to understand IMO that putting it in the file or putting it on the CLI arg behaves the same way.

I should be able to say "You can set --tls-cert-path as arg or TEMPORAL_TLS_CLIENT_CERT_PATH env var or you can set tls.client_cert_path config item, they are the same, your choice".

I am proposing that, in the config, we require absolute paths. I am not proposing any other changes (certainly no changes to the behavior of the existing env variables or flags).

I am proposing that, in the config, we require absolute paths

Ah, ok I misread "As a compromise, I propose requiring absolute paths and rejecting relative paths as invalid config" as all config, but you mean config file. I think we can mostly do this.

So, concerning the last part of my comment above, can you clarify what you expect the behavior of temporal config --key tls.client-cert-path --value my/relative/path.cert to be that writes to config file (compared to the deprecated form of temporal env --key tls-cert-path --value my/relative/path.cert)? Before, our env config supported relative paths, but now our new one does not, which I assume is an intentional regression?

Yep, I think we would have to expand the path to its absolute form before writing to the config file, unless we want to simply fail saying the path is invalid/must be absolute.

Ok, I will update this doc clarifying that config file loading should fail if the path is not absolute (for now) and maybe config file writing from CLI can resolve the path.

Added part saying that file paths inside of config files must be absolute

mjameswh · 2024-10-23T18:03:01Z

all-sdk/external-client-configuration.md

+
+* All samples will be updated to load from config and default to local dev server w/ default namespace.
+  * In samples we try to avoid shared code. In .NET and Python, there is no default target host, and in Ruby there will
+    not even be a default namespace. So every sample in these languages is going to have to not only have the


So every sample in these languages is going to have to not only have the load-from-config code, but also conditionally apply the target host as localhost:7233 if not set in external config.

🤔 That kind of defeats on of the purpose of the load-from-config utility function, and would have little demonstrative value in samples as most "sample-level" users would, in practice, either fully adopt the config file approach, or fully specify their client config programmatically.

I think we should revisit the question on either load-from-config should fill in a default address and namespace. I totally understand why that's tricky in these languages, but I'm not sure the current proposal is the most reasonable compromise.

I think we should revisit the question on either load-from-config should fill in a default address and namespace.

There is no default address and namespace in some SDKs is the point. We have removed this default as we've become more mature knowing that the older defaults (which we cannot change) are not as useful in deployed code. But for those SDKs w/out a default address/namespace there is a default address/namespace in samples, so they must set it. But that's only for samples, we shouldn't affect non-sample code w/ sample-only defaults.

Why not just make it required for the config to specify these things (even for langs where the in-code options allow a default)? It sure would make the config file more readable by default, and can still be overridden with env var.

Because some people just want to configure, say, only the remote codec with a config file or env var. Or there is nothing in config and only address is set by env var and namespace is expected to be set by user. Or config file only sets address and namespace is set via env var. All config keys are optional (as is even the presence of the config file). It's only when the client options are used does whether a value being present matter.

It'd be normal to have:

options, err := envconfig.LoadClientOptionsFromConfig("default") if err != nil { return err } options.Namespace = "some-code-defined-value" cl, err := client.Dial(options)

mjameswh · 2024-10-23T22:13:54Z

all-sdk/external-client-configuration.md

+  it returns a `ConnectionOptions & ClientOptions`.
+  * Could also have it return `{ connectionOptions: ConnectionOptions, clientOptions: ClientOptions }` if that makes
+    more sense than a union.
+* TypeScript SDK _does not_ support codec settings and will error if seen in config.


Because a user with a profile configured with a codec should expect that codec to be used, not silently ignored.

I agree with your argument, but I'm not totally comfortable with the consequence.

To error in such cases implies that a user who has codecs AND is using an SDK that doesn't have remote codec support would have to create two distinct profiles, one for use with CLI with remote codec configured, and the other for use with his SDK, without remote codec configured. That's very inconvenient.

Not saying that's wrong, but to be coherent with this decision, we will need to move forward with implementing remote codecs in all SDKs. Not difficult, but IIRC, there were doubts about either that's a good idea.

Otherwise, we may find ourselves needing to add an extra flag to LoadClientOptions in all SDKs that don't have support for remote codecs, to intentionally ignore remote codec settings from config file/environment (i.e. just turn off that specific error). That's ugly. I'd definitely rather not get down to this.

👍 I agree we'd need to support remote codecs in each SDK to get the benefit of this setting in every SDK. In the meantime, erroring is the clearest approach I think.

cretz · 2024-10-28T14:18:49Z

Added updates for:

Config file being temporal.toml knowing future non-client config can be in a subkey
Core-based SDKs will use Rust Core if they can (TS is still a bit up in the air)

mjameswh · 2024-10-28T20:32:46Z

all-sdk/external-client-configuration.md

@@ -337,7 +344,7 @@ is SDK specific).
        * This is the new way and we should not try to support both at the same time, it can get confusing.
  * The default is `default`, and this can also be set via `TEMPORAL_PROFILE` env var.
 * CLI will accept `--config-file`.
-  * Default is `~/.config/temporalio/temporal-client.toml`, also can be set via `TEMPORAL_CONFIG_FILE` env var.
+  * Default is `~/.config/temporalio/temporal.toml`, also can be set via `TEMPORAL_CONFIG_FILE` env var.


Given that this line is prescriptive, it should also specify the correct path for Windows.

This is specified above at:

The default config file location is <app-config-dir>/temporalio/temporal.toml for all platforms. [...] This is as defined by https://pkg.go.dev/os#UserConfigDir which, as of this writing, is the logic defined in docs and in code at https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/os/file.go;l=528.

And there's a hint before in one of the flows as sample instructions to users:

macOS - $HOME/Library/Application Support/temporalio/temporal.toml

Windows - %AppData%/temporalio/temporal.toml (often C:\Users\<myuser>\AppData\Roaming\temporalio\temporal.toml)

Linux - $HOME/.config/temporalio/temporal.toml

But I can take that part and make it clearer here if needed.

Yeah, it's clearly indicated elsewhere, and I would normally not be picky on this kind of small incoherencies.

But this specific line sounds like it is authoritative, so being incomplete in this case is more confusing.

TBH, I'd just remove this line, or formulate it as an example rather than a prescriptive statement.

Removed the part saying what the default is here under the CLI section hoping people will look at the general specification section

all-sdk/external-client-configuration.md

josh-berry · 2024-11-05T01:41:36Z

all-sdk/external-client-configuration.md

+  * 💭 Why TOML?
+    * Original document was JSON, but team discussion decided that the benefit to humans of TOML outweighed the concerns
+      of making SDKs have extensions with TOML support.
+* For settings that accept files above, they are relative to the current working directory.


Need to clarify that the config file must hold absolute paths and the CLI will always write absolute paths.

Ah, I clarified this elsewhere but not here. Will do.

josh-berry · 2024-11-05T01:43:12Z

all-sdk/external-client-configuration.md

+    * After discussion, it was decided future non-client use of this config (which may never occur) can be sub keys of
+      this file instead of having a separate file or separate config for every separate use. And client is the common
+      enough initial (and maybe only) use case to get the top-level without burdening users.
+* Unlike all other ways to provide settings, all file paths inside configuration files must be absolute or loading


josh-berry · 2024-11-05T01:46:03Z

all-sdk/external-client-configuration.md

+* Try to load profile from configuration file
+  * Only if file loading is enabled (which it is by default)
+  * If user provides specific config file, use that, otherwise use the default location
+  * If user specifies the profile, use that, otherwise use the default


Do we want to add a step in here somewhere with the config-file-specific absolute path validation?

I admittedly do not have validation details in this proposal, but I can add this.

cretz · 2024-11-18T14:24:06Z

No outstanding issues, merging

cretz added 4 commits August 15, 2024 14:30

External client configuration proposal

d0c5d0f

Requested updates

d84db96

More updates

a8a13c4

Address some feedback after meetings

9d040b7

cretz mentioned this pull request Sep 5, 2024

External client configuration proposal #94

Closed

cretz commented Sep 5, 2024

View reviewed changes

all-sdk/external-client-configuration.md Outdated Show resolved Hide resolved

cretz commented Sep 5, 2024

View reviewed changes

all-sdk/external-client-configuration.md Outdated Show resolved Hide resolved

Switched to TOML, add SDK parts as "extensions", and support gRPC met…

b8cf8fe

…adata env vars

mjameswh reviewed Oct 23, 2024

View reviewed changes

This was referenced Oct 24, 2024

[Feature Request] Support TEMPORAL_GRPC_META_<whatever> environment variable temporalio/cli#699

Open

[Feature Request] Common external configuration of client connection options temporalio/features#441

Open

Confir file and core-based SDK updates

72e3502

mjameswh reviewed Oct 28, 2024

View reviewed changes

all-sdk/external-client-configuration.md Show resolved Hide resolved

Require paths inside config file to be absolute

a398d4e

josh-berry approved these changes Nov 5, 2024

View reviewed changes

cretz merged commit 904b7ed into temporalio:master Nov 18, 2024
2 checks passed

cretz deleted the external-client-configuration branch November 18, 2024 14:24


		And then still use CLI like

		temporal workflow list --namespace my-specific-namespace

External client configuration proposal #95

External client configuration proposal #95

Conversation

cretz commented Sep 5, 2024 • edited Loading

Summary

cretz commented Oct 23, 2024

mjameswh Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjameswh Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

mjameswh Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz commented Oct 28, 2024

Choose a reason for hiding this comment

cretz Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz commented Nov 18, 2024

cretz commented Sep 5, 2024 •

edited

Loading

mjameswh Oct 23, 2024 •

edited

Loading

mjameswh Oct 23, 2024 •

edited

Loading

cretz Oct 23, 2024 •

edited

Loading

mjameswh Oct 28, 2024 •

edited

Loading

cretz Oct 28, 2024 •

edited

Loading

cretz Oct 23, 2024 •

edited

Loading

cretz Oct 28, 2024 •

edited

Loading