GH 731 - review (#362)

* begining with test server_initializer_collected_params_combine_vlcs_properly

* removed GatheredParams from server_initializer_collected_params, added bools for data_dir, config_file and real_user to MultiConfig, fixing the test

* add ComputedVcl struct and its implementation, add VclType enum, to be able to push into Vcl Vector, add processing in MultiConfig, add processing in server_initializer_collected_params

* back to VirtualCommandLine type without enum, passing different types to MultiConfig and selecting them to computed_value_names on behalf of their type

* fixed data flow with Evironment and Command line arguments, need to fix config file arguments data

* changes in test to access config.file for various scenarios, add machinery to determine if real-user is specified in config-file

* try to get rid of redundant MultiConfig construction in determine_user_specific_data

* handle computed arguments in multiconfig, closure for server_initializer_collected_params, closure for keeping in node_configurator, final removal of redundant construction of multiconfig

* computing env_args and cmd_args in create_preorientation_args fn!

* fix passing test server_initializer_collected_params_can_read_parameters_from_config_file, some optimization

* fixed VclArg problem with withdrawing data from VirtualCommandline, sorted server_initializer_collected_params and determine_user_specific_data fncs, new fns value_opt for VclArg and tests for them

* finalize preparing guts for multiconfig in server_initializer_collected_params, added handling for tilde in data_directory and config_file paths

* fix windows test of real_user soecified

* renaming, adding TODOs, removing resolved TODOS

* fixing tests for windows os

* fixed on windows

* formatting

* fixed real user for windows

* removed filling specified and unspecified boxes from server_initializer_collected_params, fixed formatting for get_real_user_from_vcl

* last fixes of extract_values_and_fill_boxes, and test server_initializer_collected_params_handle_config_file_from_commandline_and_real_user_from_config_file_with_data_dir_started_by_tilde

* fixed windows test for tilde

* fix tilde test for windows

* renaming of tests and functions

* remove GatheredParams (the goal), optimizing is_user_specified in MultiConfig, dealing with comments

* remove value_opt form VclArg, remove test for that, change MultiConfig and schema (config-file no longer defaulted), change value_from_vcl to multiconfig and value_m

* resolving comments from review

* optimization of get_config_file_from_mc fn

* fixed the config_path reference bin replace_dots fn

* solving dereferencing path in replace_dots fn

* solving config_path in replace_dots fn

* fixed get_config_file_from_mc fn - optimized for readability

* formatting

* removed println, and formated

* fixed windows tests

* formatting

* workaround for actual_server_drop test

* forgotten u64 for another test in connection_termination_test.rs

* add one more request to actual_serve_drop

* remove forgotten piece of code

* fixing comments from review2

* remove redundant lifetime, and remove convertion to PathBuf

* fixing absolute path for tests of handle config-file and data directory relative

* run the actions

* merge master into GH-731

* fix ENV and CLAP Guards in tests

* fix Windows panic message for server_initializer_collected_params_handles_only_path_in_config_file_param

* formatting

* removing debugging println from accountant

* fix CLI UI issue with config file

* remove canonicalize() from test uses home_dir() from dirs crate. Produces malfuncioning behaviour

* Reformatted some code, added a comment

* formatting, fix order of Vcls in setup_reporter, add assertion to node_configurator_standard

---------

Co-authored-by: Dan Wiebe <[email protected]>

Author: czarte

masq_lib/src/multi_config.rs

@@ -63,9 +63,10 @@ impl<'a> MultiConfig<'a> {
     ) -> Result<MultiConfig<'a>, ConfiguratorError> {
         let initial: Box<dyn VirtualCommandLine> =
             Box::new(CommandLineVcl::new(vec![String::new()]));
-        let merged: Box<dyn VirtualCommandLine> = vcls
+        let merged = vcls
             .into_iter()
             .fold(initial, |so_far, vcl| merge(so_far, vcl));
+
         let arg_matches = match schema
             .clone()
             .get_matches_from_safe(merged.args().into_iter())
@@ -78,6 +79,7 @@ impl<'a> MultiConfig<'a> {
                 _ => return Err(Self::make_configurator_error(e)),
             },
         };
+
         Ok(MultiConfig { arg_matches })
     }
 
@@ -224,6 +226,9 @@ impl NameOnlyVclArg {
 pub trait VirtualCommandLine {
     fn vcl_args(&self) -> Vec<&dyn VclArg>;
     fn args(&self) -> Vec<String>;
+    fn is_computed(&self) -> bool {
+        false
+    }
 }
 
 impl Debug for dyn VirtualCommandLine {
@@ -347,10 +352,19 @@ impl EnvironmentVcl {
     }
 }
 
+#[derive(Debug)]
 pub struct ConfigFileVcl {
     vcl_args: Vec<Box<dyn VclArg>>,
 }
 
+impl Clone for ConfigFileVcl {
+    fn clone(&self) -> Self {
+        ConfigFileVcl {
+            vcl_args: self.vcl_args.iter().map(|arg| arg.dup()).collect(),
+        }
+    }
+}
+
 impl VirtualCommandLine for ConfigFileVcl {
     fn vcl_args(&self) -> Vec<&dyn VclArg> {
         vcl_args_to_vcl_args(&self.vcl_args)
@@ -375,8 +389,8 @@ impl Display for ConfigFileVclError {
         match self {
             ConfigFileVclError::OpenError(path, _) => write!(
                 fmt,
-                "Couldn't open configuration file {:?}. Are you sure it exists?",
-                path
+                "Couldn't open configuration file \"{}\". Are you sure it exists?",
+                path.to_string_lossy()
             ),
             ConfigFileVclError::CorruptUtf8(path) => write!(
                 fmt,

masq_lib/src/shared_schema.rs

@@ -235,7 +235,6 @@ pub fn config_file_arg<'a>() -> Arg<'a, 'a> {
     Arg::with_name("config-file")
         .long("config-file")
         .value_name("FILE-PATH")
-        .default_value("config.toml")
         .min_values(0)
         .max_values(1)
         .required(false)

multinode_integration_tests/tests/connection_termination_test.rs

@@ -116,27 +116,28 @@ fn actual_server_drop() {
     let mut server = real_node.make_server(server_port);
     let masquerader = JsonMasquerader::new();
     let (stream_key, return_route_id) = arbitrary_context();
-    mock_node
-        .transmit_package(
-            mock_node.port_list()[0],
-            create_request_icp(
-                &mock_node,
-                &real_node,
-                stream_key,
-                return_route_id,
-                &server,
-                cluster.chain,
-            ),
-            &masquerader,
-            real_node.main_public_key(),
-            real_node.socket_addr(PortSelector::First),
-        )
-        .unwrap();
-    server.wait_for_chunk(Duration::from_secs(2)).unwrap();
-    server.send_chunk(HTTP_RESPONSE);
-    mock_node
-        .wait_for_package(&masquerader, Duration::from_secs(2))
-        .unwrap();
+    let index: u64 = 0;
+    request_server_payload(
+        index,
+        &cluster,
+        &real_node,
+        &mock_node,
+        &mut server,
+        &masquerader,
+        stream_key,
+        return_route_id,
+    );
+    let index: u64 = 1;
+    request_server_payload(
+        index,
+        &cluster,
+        &real_node,
+        &mock_node,
+        &mut server,
+        &masquerader,
+        stream_key,
+        return_route_id,
+    );
 
     server.shutdown();
 
@@ -165,6 +166,40 @@ fn actual_server_drop() {
     assert!(payload.sequenced_packet.last_data);
 }
 
+fn request_server_payload(
+    index: u64,
+    cluster: &MASQNodeCluster,
+    real_node: &MASQRealNode,
+    mock_node: &MASQMockNode,
+    server: &mut MASQNodeServer,
+    masquerader: &JsonMasquerader,
+    stream_key: StreamKey,
+    return_route_id: u32,
+) {
+    mock_node
+        .transmit_package(
+            mock_node.port_list()[0],
+            create_request_icp(
+                index,
+                &mock_node,
+                &real_node,
+                stream_key,
+                return_route_id,
+                &server,
+                cluster.chain,
+            ),
+            masquerader,
+            real_node.main_public_key(),
+            real_node.socket_addr(PortSelector::First),
+        )
+        .unwrap();
+    server.wait_for_chunk(Duration::from_secs(2)).unwrap();
+    server.send_chunk(HTTP_RESPONSE);
+    mock_node
+        .wait_for_package(masquerader, Duration::from_secs(2))
+        .unwrap();
+}
+
 #[test]
 // Given: Exit Node is real_node; originating Node is mock_node.
 // Given: A stream is established through the exit Node to a server.
@@ -178,10 +213,12 @@ fn reported_client_drop() {
     let mut server = real_node.make_server(server_port);
     let masquerader = JsonMasquerader::new();
     let (stream_key, return_route_id) = arbitrary_context();
+    let index: u64 = 0;
     mock_node
         .transmit_package(
             mock_node.port_list()[0],
             create_request_icp(
+                index,
                 &mock_node,
                 &real_node,
                 stream_key,
@@ -309,6 +346,7 @@ fn arbitrary_context() -> (StreamKey, u32) {
 }
 
 fn create_request_icp(
+    index: u64,
     originating_node: &MASQMockNode,
     exit_node: &MASQRealNode,
     stream_key: StreamKey,
@@ -343,7 +381,7 @@ fn create_request_icp(
             &node_lib::sub_lib::migrations::client_request_payload::MIGRATIONS,
             &ClientRequestPayload_0v1 {
                 stream_key,
-                sequenced_packet: SequencedPacket::new(Vec::from(HTTP_REQUEST), 0, false),
+                sequenced_packet: SequencedPacket::new(Vec::from(HTTP_REQUEST), index, false),
                 target_hostname: Some(format!("{}", server.local_addr().ip())),
                 target_port: server.local_addr().port(),
                 protocol: ProxyProtocol::HTTP,

node/src/accountant/db_access_objects/utils.rs

@@ -17,6 +17,7 @@ use rusqlite::{Row, Statement, ToSql};
 use std::fmt::{Debug, Display};
 use std::iter::FlatMap;
 use std::path::{Path, PathBuf};
+use std::string::ToString;
 use std::time::Duration;
 use std::time::SystemTime;
 

node/src/accountant/mod.rs

@@ -4523,7 +4523,6 @@ mod tests {
             let factory = Accountant::dao_factory(data_dir);
             factory.make();
         };
-
         assert_on_initialization_with_panic_on_migration(&data_dir, &act);
     }
 }