Add CLI support for applying settings patches

[dlon]

This adds a command mullvad import-settings for applying patches to the settings. It takes a positional argument that should bet set either to a path (for reading from a file) or - (from stdin).

Currently, only changes to relay overrides are recognized. Changes to other settings are unrecognized/rejected by the daemon.

Here is an example of a valid patch:

$ mullvad import-settings -
{
  "relay_overrides": [
    {
      "hostname": "se-got-wg-001",
      "ipv4_addr_in": "1.2.3.4"
    }
  ]
}

Related: #5410

Fixes DES-445

---

This change is 

[MarkusPettersson98]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dlon)

---

mullvad-cli/src/main.rs line 140 at r1 (raw file):

    ImportSettings {
        /// File to read from. If this is "-", read from standard input
        file: String,

It would be great if this pattern of reading either from file or from stdin could be expressed at the type level, much like https://docs.rs/clap-stdin/latest/clap_stdin/struct.FileOrStdin.html. What do you think?

Code quote:

file: String

---

mullvad-cli/src/cmds/import_settings.rs line 47 at r1 (raw file):

/// Read until EOF or until newline when the last pair of braces has been closed
fn read_settings_from_reader(mut reader: impl BufRead) -> Result<String> {

I don't feel too comfortable hand-rolling this JSON-parser in this fashion. Is there no way serde could be used to work with structured data instead? 😊

Code quote:

read_settings_from_reader

---

mullvad-cli/src/cmds/import_settings.rs line 52 at r1 (raw file):

    let mut was_open = false;
    let mut close_after_newline = false;
    let mut brace_count = 0;

Keeping track of braces and making sure that they are balanced is suitably done using a stack (Vec), pushing & popping braces as you go. Checking for balance is just checking if the stack is empty 😊

Code quote:

let mut brace_count = 0;

---

mullvad-daemon/src/lib.rs line 353 at r1 (raw file):

    VerifyPlayPurchase(ResponseTx<(), Error>, PlayPurchase),
    /// Patch the settings using a blob of JSON settings
    ApplyJsonSettings(ResponseTx<(), settings::patch::Error>, String),

Could we guarantee that this blob is well-formed by defining some appropriate type?:)

Code quote:

String

---

mullvad-daemon/src/settings/patch.rs line 56 at r1 (raw file):

            | Error::DeserializePatched(_) => Status::invalid_argument(error.to_string()),
            Error::Settings(error) => Status::from(error),
            error => Status::unknown(error.to_string()),

Could the named variable error be expanded to the currently-not-covered constructorsError::SerializeSettings(_) | Error::RecursionLimit? If Error ever was to change, it would be nice to trigger a compiler-error here

Code quote:

error

---

mullvad-daemon/src/settings/patch.rs line 172 at r1 (raw file):

            "existing overrides should be array",
        ));
    };

You can apply the same pattern of calling ok_or(Error::InvalidOrMissingValue(..)), as is done below

    let patch_array = patch.as_array().ok_or(Error::InvalidOrMissingValue(
        "relay overrides must be array",
    ))?;
    let current_array = current_settings
        .as_array()
        .ok_or(Error::InvalidOrMissingValue(
            "existing overrides should be array",
        ))?;

Code quote:

    let Some(patch_array) = patch.as_array() else {
        return Err(Error::InvalidOrMissingValue(
            "relay overrides must be array",
        ));
    };

    let Some(current_array) = current_settings.as_array() else {
        return Err(Error::InvalidOrMissingValue(
            "existing overrides should be array",
        ));
    };

[dlon]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-cli/src/cmds/import_settings.rs line 47 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I don't feel too comfortable hand-rolling this JSON-parser in this fashion. Is there no way serde could be used to work with structured data instead? 😊

We're not really parsing anything here. We're just figuring out when to stop reading from stdin.

---

mullvad-cli/src/cmds/import_settings.rs line 52 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Keeping track of braces and making sure that they are balanced is suitably done using a stack (Vec), pushing & popping braces as you go. Checking for balance is just checking if the stack is empty 😊

Done. (Used solution from AFK discussion.)

---

mullvad-daemon/src/lib.rs line 353 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Could we guarantee that this blob is well-formed by defining some appropriate type?:)

The function is supposed to allow clients to feed any string to the daemon. It's up to the daemon to handle all of the parsing.

---

mullvad-daemon/src/settings/patch.rs line 56 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Could the named variable error be expanded to the currently-not-covered constructorsError::SerializeSettings(_) | Error::RecursionLimit? If Error ever was to change, it would be nice to trigger a compiler-error here

Makes sense. Done.

---

mullvad-daemon/src/settings/patch.rs line 172 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

You can apply the same pattern of calling ok_or(Error::InvalidOrMissingValue(..)), as is done below

    let patch_array = patch.as_array().ok_or(Error::InvalidOrMissingValue(
        "relay overrides must be array",
    ))?;
    let current_array = current_settings
        .as_array()
        .ok_or(Error::InvalidOrMissingValue(
            "existing overrides should be array",
        ))?;

Done.

[linear]

DES-445 Add daemon support for settings patches

[dlon]

Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-cli/src/main.rs line 140 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

It would be great if this pattern of reading either from file or from stdin could be expressed at the type level, much like https://docs.rs/clap-stdin/latest/clap_stdin/struct.FileOrStdin.html. What do you think?

That does look nicer! But given how simple it is, and that we only use it once, it's probably not worth it? We also cannot just use that crate because it doesn't terminate when it's received a complete json string

[MarkusPettersson98]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-cli/src/main.rs line 140 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

That does look nicer! But given how simple it is, and that we only use it once, it's probably not worth it? We also cannot just use that crate because it doesn't terminate when it's received a complete json string

If we do it twice, I definitely think it's worth it. But for now this is okay 😊

---

mullvad-cli/src/cmds/import_settings.rs line 47 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

We're not really parsing anything here. We're just figuring out when to stop reading from stdin.

Got that after out talk AFK, so I feel pleased with this.

---

mullvad-daemon/src/lib.rs line 353 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

The function is supposed to allow clients to feed any string to the daemon. It's up to the daemon to handle all of the parsing.

That's fair

[Jontified]

Reviewed 8 of 10 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-daemon/src/settings/patch.rs line 171 at r2 (raw file):

            "existing overrides should be an array",
        ))?;
    let mut new_array = current_array.clone();

What's the point of cloning the current_array? In event of error changes to it will not be merged anyway.

[Jontified]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)