Load config from well-known locations + add `init` command for initializing config #582

QuinnWilton · 2024-02-21T01:05:50Z

Description

This adds the start of an init command for configuring Homestar from the CLI. When run interactively, it currently provides a wizard for configuring the keys used by homestar, however it can also be run non-interactively using values configured through CLI flags.

It also includes allowances for outputting a generated configuration over stdout, and the start of some patterns for handling some common CLI knobs, like --no-input, --quiet, and --force, though these are only likely to see more use as we expose more configuration options to this interactive wizard.

There's still a lot of polish that can be done around how we handle error reporting and interrupts, like control+c, during interactive usage, but this is a start that should make it a lot easier to begin expanding the configuration options we expose to the user.

Link to issue

Please add a link to any relevant issues/tickets.

Screenshots/Screencaps

homestar-runtime/Cargo.toml

homestar-runtime/src/main.rs

homestar-runtime/src/cli/init.rs

homestar-runtime/Cargo.toml

QuinnWilton · 2024-02-21T22:15:38Z

I pushed up some small tweaks, including converting InquireError to a miette report, so that we're able to cleanup the error reporting on a SIGINT received during prompting. Here's how that looks now:

Exceptional errors still currently panic, which I think is possibly the right call for now, until we do the work of translating those into a miette error that directs users to file an issue in the repo or something.

codecov · 2024-02-21T22:29:22Z

Codecov Report

Attention: Patch coverage is 65.33742% with 113 lines in your changes are missing coverage. Please review.

Project coverage is 69.05%. Comparing base (90c6246) to head (9c8b07a).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #582      +/-   ##
==========================================
- Coverage   69.17%   69.05%   -0.12%     
==========================================
  Files          95       98       +3     
  Lines       12953    13491     +538     
==========================================
+ Hits         8960     9316     +356     
- Misses       3993     4175     +182

Files	Coverage Δ
homestar-runtime/src/main.rs	`77.14% <100.00%> (+0.67%)`	⬆️
homestar-runtime/src/settings/libp2p_config.rs	`100.00% <100.00%> (ø)`
homestar-runtime/src/settings.rs	`98.45% <98.11%> (+0.27%)`	⬆️
homestar-runtime/src/cli.rs	`58.66% <0.00%> (-4.20%)`	⬇️
homestar-runtime/src/settings/pubkey_config.rs	`75.60% <76.66%> (+3.47%)`	⬆️
homestar-runtime/src/cli/init.rs	`56.89% <56.89%> (ø)`

... and 5 files with indirect coverage changes

bgins

Nice work on this! Looks great ✨

zeeshanlakhani · 2024-02-22T03:26:42Z

@QuinnWilton could we add some integration/CLI (no runtime needed) tests to have tests around the functionality here?

zeeshanlakhani

Added some comments thus far.

Q: Shouldn't we also have the option to generate a PEM/DER file for users (not just provide your own) as another option? Essentially, replace the OpenSSL genkey we've/users have been doing manually? I thought this was one goal of the work?

❤️ the experience btw.

homestar-runtime/Cargo.toml

homestar-runtime/src/cli/init.rs

homestar-runtime/src/settings.rs

homestar-runtime/src/cli/init.rs

QuinnWilton · 2024-02-27T19:51:37Z

Added some comments thus far.

Q: Shouldn't we also have the option to generate a PEM/DER file for users (not just provide your own) as another option? Essentially, replace the OpenSSL genkey we've/users have been doing manually? I thought this was one goal of the work?

❤️ the experience btw.

I don't see any APIs exposed by the libp2p-identity crate for encoding keys as PEM/DER files. Are you aware of anything that I may have missed? Or were you imagining that we'd pull in another crypto crate / shell out to OpenSSL for that?

Either way, I just pushed up fixes for all of your other comments.

homestar-runtime/tests/cli.rs

For windows: $USERPROFILE/.config/homestar/settings.toml For macos + linux: $XDG_CONFIG_HOME/homestar/settings.toml or: $HOME/.config/homestar/settings.toml

The comments erroneously listed only ed25519 as an allowed public key type, however secp256k1 keys are also supported.

This skips writing the newly generated configuration file to disk, and instead outputs it over stdout

This is in preparation for including richer forms of interactivity around options like keypair generation and management.

This suppresses all auxiliary output, like that intended for humans, to aid in non-interactive use from a script.

This runs the command in non-interactive mode, by disabling all prompts, and instead erroring if interactive inpupt is required.

Even though Inquire will already error if we attempt to prompt for input without being attached to a TTY, explicitly running the command in non-interactive mode will ensure that our error messaging will be consistent whether or not a TTY is available

QuinnWilton · 2024-03-06T01:51:39Z

@zeeshanlakhani I just pushed up PEM generation for ed25519, improved PEM parsing for ed25519, and the tests you requested! Headed to dinner now, but merge conflicts are fixed and I'll check CI in the morning 😊

zeeshanlakhani · 2024-03-06T15:30:51Z

homestar-runtime/src/cli/init.rs

+                    PubkeyConfig::GenerateFromSeed(RNGSeed::new(key_type, seed.0))
+                }
+                PubkeyConfigOption::FromFile => {
+                    // HACK: We need to manually instantiate the struct with a custom formatter,


not sure we need to call this a hack vs constraints on the type/impls?

I guess my other Q is if we had to use PathBuf, but I'm guessing yes.

We don't necessarily need PathBuf, but my original reasoning was:

Supporting non-UTF-8 paths

Hooking into Inquire's error handling flow, for handling parse errors by displaying an error and re-prompting the user

(1) may not apply because we still construct the PathBuf by going through the parser closure, which is over a &str, and so will only contain UTF-8 codepoints anyway.

(2) also doesn't look important, because it seems like the FromStr impl for PathBuf is Infallible, so we shouldn't need to worry about error handling there.

Want me to rip this out and just return the input as a String?

@QuinnWilton yeah, seeing your responses, I think we can rip this out easily and go with String.

Cool, I'll do that, thanks!

zeeshanlakhani · 2024-03-06T15:38:20Z

@QuinnWilton Code is looking pretty good, but there was a new setting added in main that's missing here after rebase: enable_resolve_receipts_in_background. Once that's updated, I'll do a run-through and check the DX experience.

This makes it easier to reuse the flag for other operations

This resolves an issue with loading keys from PEM files generated using some implementations of openssl, like libressl, which concatenate parameters onto the raw key material, and preventing libp2p from recognizing the serialization as containing a valid encoding of the key.

bgins · 2024-03-07T20:50:46Z

Hey @QuinnWilton! Trying out the new PEM key file features. They work great, and I love them. ❤️

I have a couple of minor DX requests:

Could we update the help text to indicate that the --key-file flag will generate a key if an existing key does not exist at the path?
When generating a key file with the --key-file flag and selecting the Secp256k1 option, an unsupported error is thrown (as expected) but an empty key file is created. Could we clean up and remove the key file in this error case?

QuinnWilton · 2024-03-11T18:12:41Z

Hey @QuinnWilton! Trying out the new PEM key file features. They work great, and I love them. ❤️

I have a couple of minor DX requests:

Could we update the help text to indicate that the --key-file flag will generate a key if an existing key does not exist at the path?

When generating a key file with the --key-file flag and selecting the Secp256k1 option, an unsupported error is thrown (as expected) but an empty key file is created. Could we clean up and remove the key file in this error case?

Just pushed up these two changes; thanks!

zeeshanlakhani · 2024-03-11T20:34:09Z

@QuinnWilton last minor thing:

Could we generate the pem in the config directory by default if no file is given to key-file and override the default path if run again?

zeeshanlakhani

@QuinnWilton left a few last small changes/details. Great work.

homestar-runtime/src/cli.rs

homestar-runtime/src/cli/init.rs

QuinnWilton requested a review from a team as a code owner February 21, 2024 01:05

bgins reviewed Feb 21, 2024

View reviewed changes

homestar-runtime/Cargo.toml Outdated Show resolved Hide resolved

bgins reviewed Feb 21, 2024

View reviewed changes

homestar-runtime/src/main.rs Outdated Show resolved Hide resolved

homestar-runtime/src/cli/init.rs Outdated Show resolved Hide resolved

homestar-runtime/Cargo.toml Outdated Show resolved Hide resolved

QuinnWilton force-pushed the quinn/config branch from 689ae4a to 8964d8a Compare February 21, 2024 22:12

bgins approved these changes Feb 22, 2024

View reviewed changes

zeeshanlakhani reviewed Feb 22, 2024

View reviewed changes

QuinnWilton force-pushed the quinn/config branch from 71ec5f3 to 0ab433b Compare February 27, 2024 19:48

zeeshanlakhani requested a review from bgins February 29, 2024 02:45

zeeshanlakhani reviewed Mar 1, 2024

View reviewed changes

homestar-runtime/tests/cli.rs Show resolved Hide resolved

QuinnWilton force-pushed the quinn/config branch from 0ab433b to bb429e8 Compare March 6, 2024 01:10

QuinnWilton added 10 commits March 5, 2024 17:48

feat: load settings from a well-known config file

2aa5aa2

For windows: $USERPROFILE/.config/homestar/settings.toml For macos + linux: $XDG_CONFIG_HOME/homestar/settings.toml or: $HOME/.config/homestar/settings.toml

feat: add init command for initializing a settings file

ccb07d8

docs: fix comments listing supported public key types

626ad97

The comments erroneously listed only ed25519 as an allowed public key type, however secp256k1 keys are also supported.

feat: support --dry-run for init command

9cc736e

This skips writing the newly generated configuration file to disk, and instead outputs it over stdout

refactor: move handling of init command to init.rs

283f672

This is in preparation for including richer forms of interactivity around options like keypair generation and management.

feat: add --quiet flag to init command

10ab5d8

This suppresses all auxiliary output, like that intended for humans, to aid in non-interactive use from a script.

feat: add --force to init command for forcing destructive operations

a45e16b

feat: add --no-input to init command

d6128ad

This runs the command in non-interactive mode, by disabling all prompts, and instead erroring if interactive inpupt is required.

refactor: change --config flag to --output for init command

13bee1c

QuinnWilton force-pushed the quinn/config branch from bb429e8 to 50a2543 Compare March 6, 2024 01:50

zeeshanlakhani reviewed Mar 6, 2024

View reviewed changes

QuinnWilton added 2 commits March 6, 2024 12:04

feat: support configuring key using init command

982e7d4

feat: output a cleaner error when an invalid seed is passed to init

e6c0dec

QuinnWilton added 14 commits March 6, 2024 12:04

fix: only constrain inquire and derive_builder by minor version

11a77f1

chore: sort imports in cli/init.rs

b99b436

refactor: wrap all init args in InitArgs and consolidate handling

3c9ff9b

refactor: remove unneeded #[allow(dead_code)] in settings.rs

1034eaa

chore: remove out of date TODO in init.rs

3680c50

test: add simple tests for init command

e8635b2

refactor: remove unneeded defaults.toml

45a70c8

refactor: remove KeyTypeArg in favor of using KeyType

15fdeb2

refactor: split force field out of OutputMode::File

d11ed72

This makes it easier to reuse the flag for other operations

feat: support generating PEM encoded ed25519 keys from init

862a28f

test: add a test for writing the generated config file + key

456fb87

fix: parse ed25519 keys using the old strategy as a fallback

9a40313

refactor: prompt for the key file as a String instead of PathBuf

5543e86

QuinnWilton force-pushed the quinn/config branch from 50a2543 to 5543e86 Compare March 6, 2024 22:47

QuinnWilton added 2 commits March 11, 2024 11:11

docs: update help text for key-file to say it'll generate a key

d20087f

fix: cleanup empty key file when bailing out of generating secp256k1

c44ba07

feat: default key-file path to output directory

93b2db0

zeeshanlakhani approved these changes Mar 12, 2024

View reviewed changes

homestar-runtime/src/cli.rs Outdated Show resolved Hide resolved

homestar-runtime/src/cli/init.rs Show resolved Hide resolved

QuinnWilton added 2 commits March 12, 2024 11:42

docs: document that if unset, a default path is used with key-file

937fe5e

docs: document that a random seed will be chosen if key-seed is unset

ede1323

bgins reviewed Mar 12, 2024

View reviewed changes

homestar-runtime/src/cli/init.rs Outdated Show resolved Hide resolved

feat: log the creation of the key file in init

9c8b07a

QuinnWilton force-pushed the quinn/config branch from 392c517 to 9c8b07a Compare March 12, 2024 19:26

QuinnWilton merged commit fc1f4e7 into main Mar 12, 2024
33 checks passed

QuinnWilton deleted the quinn/config branch March 12, 2024 19:43

zeeshanlakhani mentioned this pull request Mar 13, 2024

Add init command to generate keypairs and settings file #422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load config from well-known locations + add `init` command for initializing config #582

Load config from well-known locations + add `init` command for initializing config #582

QuinnWilton commented Feb 21, 2024

QuinnWilton commented Feb 21, 2024

codecov bot commented Feb 21, 2024 •

edited

Loading

bgins left a comment

zeeshanlakhani commented Feb 22, 2024

zeeshanlakhani left a comment •

edited by QuinnWilton

Loading

QuinnWilton commented Feb 27, 2024

QuinnWilton commented Mar 6, 2024

zeeshanlakhani Mar 6, 2024

zeeshanlakhani Mar 6, 2024

QuinnWilton Mar 6, 2024

zeeshanlakhani Mar 6, 2024

QuinnWilton Mar 6, 2024

zeeshanlakhani commented Mar 6, 2024 •

edited

Loading

bgins commented Mar 7, 2024

QuinnWilton commented Mar 11, 2024

zeeshanlakhani commented Mar 11, 2024

zeeshanlakhani left a comment

Load config from well-known locations + add init command for initializing config #582

Load config from well-known locations + add init command for initializing config #582

Conversation

QuinnWilton commented Feb 21, 2024

Description

Link to issue

Screenshots/Screencaps

QuinnWilton commented Feb 21, 2024

codecov bot commented Feb 21, 2024 • edited Loading

Codecov Report

bgins left a comment

Choose a reason for hiding this comment

zeeshanlakhani commented Feb 22, 2024

zeeshanlakhani left a comment • edited by QuinnWilton Loading

Choose a reason for hiding this comment

QuinnWilton commented Feb 27, 2024

QuinnWilton commented Mar 6, 2024

zeeshanlakhani Mar 6, 2024

Choose a reason for hiding this comment

zeeshanlakhani Mar 6, 2024

Choose a reason for hiding this comment

QuinnWilton Mar 6, 2024

Choose a reason for hiding this comment

zeeshanlakhani Mar 6, 2024

Choose a reason for hiding this comment

QuinnWilton Mar 6, 2024

Choose a reason for hiding this comment

zeeshanlakhani commented Mar 6, 2024 • edited Loading

bgins commented Mar 7, 2024

QuinnWilton commented Mar 11, 2024

zeeshanlakhani commented Mar 11, 2024

zeeshanlakhani left a comment

Choose a reason for hiding this comment

Load config from well-known locations + add `init` command for initializing config #582

Load config from well-known locations + add `init` command for initializing config #582

codecov bot commented Feb 21, 2024 •

edited

Loading

zeeshanlakhani left a comment •

edited by QuinnWilton

Loading

zeeshanlakhani commented Mar 6, 2024 •

edited

Loading