Skip to content

Derived optional keys in tedge_config interact badly with all_or_nothing #2362

@jarhodes314

Description

@jarhodes314

Describe the bug
With the config (don't read into the specifics too much:

define_tedge_config! {
    mqtt: {
        external: {
            cert_file: Utf8PathBuf,

            key_file: Utf8PathBuf,
        },
    },
    c8y: {
        proxy: {
            #[tedge_config(default(from_optional_key = "mqtt.external.cert_file"))]
            cert_file: Utf8PathBuf,

            #[tedge_config(default(from_optional_key = "mqtt.external.key_file"))]
            key_file: Utf8PathBuf,
        },
    },
}

If the user sets c8y.proxy.cert_file, but not c8y.proxy.key_file, and thin-edge calls all_or_nothing on those two configurations, we fail with the error:

  × The thin-edge configuration is invalid. The settings ["c8y.proxy.cert_file", "mqtt.external.key_file"] must either all be configured, or all unset. Currently ["c8y.proxy.cert_file"] are set, and ["mqtt.external.key_file"] are unset.

The error message should show the key that corresponds to the one that is set (so c8y.proxy.key_file). The list of keys which are configured, however, does not suffer from this issue, it shows mqtt.external.cert_file/c8y.proxy.cert_file as expected depending on which one is configured.

Expected behavior
The error message should mention the appropriate key (in the above case, c8y.proxy.key_file). If the user has set the fallback key (in this case, mqtt.external.cert_file), it should instead show the corresponding key (mqtt.external.key_file) as unset.

Additional context
This is likely a pain to fix. One thing that springs to mind is when we return keys in OptionalConfig, we could return a list of the possible keys, which trace from the key we tried to read through all the possible default keys (assuming we're not using functions to generate defaults, which is almost universally true) and an index to represent the key that was actually read. When asked to find all_or_nothing for two keys, we can (probably) reasonably assume that if the first configuration was set using the first key from the list, the customer meant to set the second configuration from the first key from the list. There is also the question of what happens when one of the settings has a fallback and the other doesn't, but so long as we do something vaguely sensible (e.g. always quote the original key, not the fallback), we're pretty safe here, and realistically this is highly unlikely to be a problem that ever needs solving.

The context around this also highlights a deeper issue with the design, specifically around the interaction between fallback values and all_or_nothing. If someone sets c8y.proxy.cert_file, mqtt.external.cert_file and mqtt.external.key_file, we currently use the c8y.proxy.cert_file with mqtt.external.key_file. So we assume they've wanted to use the MQTT private key with a separately configured C8Y proxy certificate, but do we actually want to do that? I'm fairly sure the answer is "no" in this case. In fixing this, the error message should be careful to clarify what's going on as tedge config get will show values for both c8y.proxy.cert_file and c8y.proxy.key_file (which will just show the value of mqtt.external.key_file). Maybe tedge config get could show some supplemental information about whether the key is the original or a fallback to help clarify this further, but a good error message when users actually encounter this issue is probably more valuable in this case due to the potential for confusion.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingrefactoringDeveloper value

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions