From 957e5256e42b836b346deaa51cabe81b04814df2 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Mon, 30 Sep 2024 12:41:31 +0100 Subject: [PATCH 01/10] [cli] Improve cloud-init error message Print a better error message when multipass cannot read the cloud-init input file. --- src/client/cli/cmd/launch.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index d5f615d414..7591e23a3d 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -378,6 +378,7 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) if (parser->isSet(cloudInitOption)) { + constexpr auto err_msg_template = "Could not load cloud-init configuration: {}\n"; try { YAML::Node node; @@ -407,7 +408,8 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) } catch (const std::exception& e) { - cerr << "error loading cloud-init config: " << e.what() << "\n"; + auto err_detail = fmt::format("{}\n{}", e.what(), "Please ensure that Multipass can read it."); + fmt::println(cerr, err_msg_template, err_detail); return ParseCode::CommandLineError; } } From 7d074b08a5e533d5c445b6d61280fccf6d67ec2a Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Mon, 30 Sep 2024 12:46:43 +0100 Subject: [PATCH 02/10] [cli] Avoid catching generic `std::exception`s Catch YAML exceptions instead of generic exceptions on failure to deal with input cloud-init. --- src/client/cli/cmd/launch.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index 7591e23a3d..8baf312c03 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -406,12 +406,16 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) } request.set_cloud_init_user_data(YAML::Dump(node)); } - catch (const std::exception& e) + catch (const YAML::BadFile& e) { auto err_detail = fmt::format("{}\n{}", e.what(), "Please ensure that Multipass can read it."); fmt::println(cerr, err_msg_template, err_detail); return ParseCode::CommandLineError; } + catch (const YAML::Exception& e) + { + fmt::println(cerr, err_msg_template, e.what()); + } } if (parser->isSet(bridgedOption)) From 3024f37875a02645d73d6a12eb814bb07513fa99 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Mon, 30 Sep 2024 13:03:23 +0100 Subject: [PATCH 03/10] [cli] Homogenize additional cloud-init errors Treat an absent/irregular cloud-init input file as a bad file. --- src/client/cli/cmd/launch.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index 8baf312c03..5fefb1f5f0 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -397,10 +397,7 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) { auto file_type = fs::status(cloudInitFile.toStdString()).type(); if (file_type != fs::file_type::regular && file_type != fs::file_type::fifo) - { - cerr << "error: No such file: " << cloudInitFile.toStdString() << "\n"; - return ParseCode::CommandLineError; - } + throw YAML::BadFile{}; node = YAML::LoadFile(cloudInitFile.toStdString()); } From 481060f918389d5e68f44c7b7d66ff7b3dc8470a Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Mon, 30 Sep 2024 14:00:35 +0100 Subject: [PATCH 04/10] [cli] Add cloud-init path to error messages --- src/client/cli/cmd/launch.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index 5fefb1f5f0..b8e8d4a377 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -378,11 +378,11 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) if (parser->isSet(cloudInitOption)) { - constexpr auto err_msg_template = "Could not load cloud-init configuration: {}\n"; + constexpr auto err_msg_template = "Could not load cloud-init configuration from '{}': {}\n"; + const QString& cloudInitFile = parser->value(cloudInitOption); try { YAML::Node node; - const QString& cloudInitFile = parser->value(cloudInitOption); if (cloudInitFile == "-") { node = YAML::Load(term->read_all_cin()); @@ -406,12 +406,12 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) catch (const YAML::BadFile& e) { auto err_detail = fmt::format("{}\n{}", e.what(), "Please ensure that Multipass can read it."); - fmt::println(cerr, err_msg_template, err_detail); + fmt::println(cerr, err_msg_template, cloudInitFile, err_detail); return ParseCode::CommandLineError; } catch (const YAML::Exception& e) { - fmt::println(cerr, err_msg_template, e.what()); + fmt::println(cerr, err_msg_template, cloudInitFile, e.what()); } } From d85f8d070b645f27df505c6ba7b0948a9a8d38b3 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Mon, 30 Sep 2024 13:29:07 +0100 Subject: [PATCH 05/10] [tests] Adapt test to match new cloud-init error --- tests/test_cli_client.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_cli_client.cpp b/tests/test_cli_client.cpp index 76c7344d9f..2a80903ac9 100644 --- a/tests/test_cli_client.cpp +++ b/tests/test_cli_client.cpp @@ -1104,7 +1104,7 @@ TEST_F(Client, launch_cmd_cloudinit_option_fails_with_missing_file) EXPECT_THAT(send_command({"launch", "--cloud-init", missing_file}, trash_stream, cerr_stream), Eq(mp::ReturnCode::CommandLineError)); - EXPECT_NE(std::string::npos, cerr_stream.str().find("No such file")) << "cerr has: " << cerr_stream.str(); + EXPECT_NE(std::string::npos, cerr_stream.str().find("Could not load")) << "cerr has: " << cerr_stream.str(); EXPECT_NE(std::string::npos, cerr_stream.str().find(missing_file)) << "cerr has: " << cerr_stream.str(); } From 0f1a8c1e2eb5e4226606f598f39018172a8cc8a3 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Mon, 30 Sep 2024 13:29:27 +0100 Subject: [PATCH 06/10] [tests] Test a few more bad cloud-init cases --- tests/test_cli_client.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/test_cli_client.cpp b/tests/test_cli_client.cpp index 2a80903ac9..3dbd9fe4ee 100644 --- a/tests/test_cli_client.cpp +++ b/tests/test_cli_client.cpp @@ -1097,7 +1097,11 @@ TEST_F(Client, launch_cmd_cloudinit_option_with_valid_file_is_ok) EXPECT_THAT(send_command({"launch", "--cloud-init", qPrintable(tmpfile.fileName())}), Eq(mp::ReturnCode::Ok)); } -TEST_F(Client, launch_cmd_cloudinit_option_fails_with_missing_file) +struct BadCloudInitFile : public Client, WithParamInterface +{ +}; + +TEST_P(BadCloudInitFile, launch_cmd_cloudinit_option_fails_with_missing_file) { std::stringstream cerr_stream; auto missing_file{"/definitely/missing-file"}; @@ -1108,6 +1112,10 @@ TEST_F(Client, launch_cmd_cloudinit_option_fails_with_missing_file) EXPECT_NE(std::string::npos, cerr_stream.str().find(missing_file)) << "cerr has: " << cerr_stream.str(); } +INSTANTIATE_TEST_SUITE_P(Client, + BadCloudInitFile, + Values("/definitely/missing-file", "/dev/null", "/home", "/root/.bashrc")); + TEST_F(Client, launch_cmd_cloudinit_option_fails_no_value) { EXPECT_THAT(send_command({"launch", "--cloud-init"}), Eq(mp::ReturnCode::CommandLineError)); From d5ae797bf9666cd6c5839648b9d43ad458d375c7 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 10 Oct 2024 10:59:37 +0100 Subject: [PATCH 07/10] [cli] Remove extra newline in error message --- src/client/cli/cmd/launch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index b8e8d4a377..935a040e12 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -378,7 +378,7 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) if (parser->isSet(cloudInitOption)) { - constexpr auto err_msg_template = "Could not load cloud-init configuration from '{}': {}\n"; + constexpr auto err_msg_template = "Could not load cloud-init configuration from '{}': {}"; const QString& cloudInitFile = parser->value(cloudInitOption); try { From f350aa9626e3429bacc8e0f7562204384eef83a8 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 10 Oct 2024 11:03:17 +0100 Subject: [PATCH 08/10] [cli] Return error code on generic YAML exceptions --- src/client/cli/cmd/launch.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index 935a040e12..a1ce0b407a 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -412,6 +412,7 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) catch (const YAML::Exception& e) { fmt::println(cerr, err_msg_template, cloudInitFile, e.what()); + return ParseCode::CommandLineError; } } From b93f5b2f2b896010be2e29fd8b26863450353c90 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 10 Oct 2024 11:47:09 +0100 Subject: [PATCH 09/10] [cli] Adapt to new BadFile exception constructor Adapt to an updated yaml-cpp where the BadFile exception carries the name of the offending file. --- src/client/cli/cmd/launch.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index a1ce0b407a..6e641a5f16 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -378,10 +378,10 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) if (parser->isSet(cloudInitOption)) { - constexpr auto err_msg_template = "Could not load cloud-init configuration from '{}': {}"; - const QString& cloudInitFile = parser->value(cloudInitOption); + constexpr auto err_msg_template = "Could not load cloud-init configuration: {}"; try { + const QString& cloudInitFile = parser->value(cloudInitOption); YAML::Node node; if (cloudInitFile == "-") { @@ -395,23 +395,24 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) } else { - auto file_type = fs::status(cloudInitFile.toStdString()).type(); + auto cloud_init_file_stdstr = cloudInitFile.toStdString(); + auto file_type = fs::status(cloud_init_file_stdstr).type(); if (file_type != fs::file_type::regular && file_type != fs::file_type::fifo) - throw YAML::BadFile{}; + throw YAML::BadFile{cloud_init_file_stdstr}; - node = YAML::LoadFile(cloudInitFile.toStdString()); + node = YAML::LoadFile(cloud_init_file_stdstr); } request.set_cloud_init_user_data(YAML::Dump(node)); } catch (const YAML::BadFile& e) { auto err_detail = fmt::format("{}\n{}", e.what(), "Please ensure that Multipass can read it."); - fmt::println(cerr, err_msg_template, cloudInitFile, err_detail); + fmt::println(cerr, err_msg_template, err_detail); return ParseCode::CommandLineError; } catch (const YAML::Exception& e) { - fmt::println(cerr, err_msg_template, cloudInitFile, e.what()); + fmt::println(cerr, err_msg_template, e.what()); return ParseCode::CommandLineError; } } From 144e09e28174b1dcb0ade9764f3ed00b99e5e770 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 10 Oct 2024 11:47:59 +0100 Subject: [PATCH 10/10] [cli] Switch var name to snake_case --- src/client/cli/cmd/launch.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index 6e641a5f16..25f639a22b 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -381,21 +381,21 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser) constexpr auto err_msg_template = "Could not load cloud-init configuration: {}"; try { - const QString& cloudInitFile = parser->value(cloudInitOption); + const QString& cloud_init_file = parser->value(cloudInitOption); YAML::Node node; - if (cloudInitFile == "-") + if (cloud_init_file == "-") { node = YAML::Load(term->read_all_cin()); } - else if (cloudInitFile.startsWith("http://") || cloudInitFile.startsWith("https://")) + else if (cloud_init_file.startsWith("http://") || cloud_init_file.startsWith("https://")) { URLDownloader downloader{std::chrono::minutes{1}}; - auto downloaded_yaml = downloader.download(QUrl(cloudInitFile)); + auto downloaded_yaml = downloader.download(QUrl(cloud_init_file)); node = YAML::Load(downloaded_yaml.toStdString()); } else { - auto cloud_init_file_stdstr = cloudInitFile.toStdString(); + auto cloud_init_file_stdstr = cloud_init_file.toStdString(); auto file_type = fs::status(cloud_init_file_stdstr).type(); if (file_type != fs::file_type::regular && file_type != fs::file_type::fifo) throw YAML::BadFile{cloud_init_file_stdstr};