From fd156548e3bd2e9952dea221786bf5e2607b2de6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Mon, 18 Jan 2021 13:57:49 +0100 Subject: [PATCH 1/7] state of work --- Cargo.lock | 1 + Cargo.toml | 1 + src/agentconfig.rs | 83 ++++++++++++++++++++++++++++++++++++++++++++++ src/main.rs | 30 ++++++++++++++--- 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index acd6832..4defdc3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2552,6 +2552,7 @@ dependencies = [ "env_logger", "flate2", "handlebars", + "hostname", "k8s-openapi", "kube", "kube-derive", diff --git a/Cargo.toml b/Cargo.toml index 9ea69a1..c2dbe65 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,3 +30,4 @@ thiserror = "1.0" url = "2.2" pnet = "0.26.0" stackable_config = { git = "https://github.com/stackabletech/common.git", branch = "main" } +hostname = "0.3" diff --git a/src/agentconfig.rs b/src/agentconfig.rs index e455176..b8dd773 100644 --- a/src/agentconfig.rs +++ b/src/agentconfig.rs @@ -19,16 +19,49 @@ pub enum AgentConfigError { #[derive(Clone)] pub struct AgentConfig { + pub hostname: String, pub parcel_directory: PathBuf, pub config_directory: PathBuf, pub log_directory: PathBuf, pub server_ip_address: IpAddr, + pub server_port: u16, pub server_cert_file: Option, pub server_key_file: Option, pub tags: HashMap, } impl AgentConfig { + pub const HOSTNAME: ConfigOption = ConfigOption { + name: "hostname", + default: None, + required: false, + takes_argument: true, + help: + "The hostname to register the node under in Kubernetes - defaults to system hostname.", + documentation: "", + list: false, + }; + + pub const DATA_DIR: ConfigOption = ConfigOption { + name: "datadir", + default: Some("/etc/stackable-agent"), + required: false, + takes_argument: true, + help: "The directory where the stackable agent should keep its working data.", + documentation: "", + list: false, + }; + + pub const PLUGIN_DIR: ConfigOption = ConfigOption { + name: "plugindir", + default: Some("/etc/stackable-agent/plugins"), + required: false, + takes_argument: true, + help: "The directory to observe for new sockets to appear which can be used to communicate with CSI plugins.", + documentation: "", + list: false, + }; + pub const SERVER_IP_ADDRESS: ConfigOption = ConfigOption { name: "server-bind-ip", default: None, @@ -60,6 +93,16 @@ impl AgentConfig { list: false, }; + pub const SERVER_PORT: ConfigOption = ConfigOption { + name: "server-port", + default: Some("3000"), + required: false, + takes_argument: true, + help: "Port to listen on for callbacks.", + documentation: "", + list: false, + }; + pub const PACKAGE_DIR: ConfigOption = ConfigOption { name: "package-directory", default: Some("/opt/stackable/packages"), @@ -125,9 +168,13 @@ scheme of \"productname-productversion\". fn get_options() -> HashSet { [ + AgentConfig::HOSTNAME, + AgentConfig::DATA_DIR, + AgentConfig::PLUGIN_DIR, AgentConfig::SERVER_IP_ADDRESS, AgentConfig::SERVER_CERT_FILE, AgentConfig::SERVER_KEY_FILE, + AgentConfig::SERVER_PORT, AgentConfig::PACKAGE_DIR, AgentConfig::CONFIG_DIR, AgentConfig::LOG_DIR, @@ -201,6 +248,12 @@ scheme of \"productname-productversion\". }; None } + + fn default_hostname() -> anyhow::Result { + Ok(hostname::get()? + .into_string() + .map_err(|_| anyhow::anyhow!("invalid utf-8 hostname string"))?) + } } impl Configurable for AgentConfig { @@ -216,6 +269,14 @@ impl Configurable for AgentConfig { fn parse_values( parsed_values: HashMap>, RandomState>, ) -> Result { + // Parse hostname or lookup local hostname + let final_hostname = + AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::HOSTNAME) + .unwrap_or_else(|_| { + AgentConfig::default_hostname() + .unwrap_or_else(|_| panic!("Unable to get hostname!")) + }); + // Parse IP Address or lookup default let final_ip = if let Ok(ip) = AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::SERVER_IP_ADDRESS) @@ -228,6 +289,14 @@ impl Configurable for AgentConfig { }; info!("Selected {} as local address to listen on.", final_ip); + // Parse data directory from values + let final_data_dir = if let Ok(data_dir) = + AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::DATA_DIR) + { + PathBuf::from_str(&data_dir).unwrap_or_else(|_| {panic!("Error parsing valid data directory from string: {}", data_dir)}) + } + // Parse plugin directory from values + // Parse log directory let final_log_dir = if let Ok(log_dir) = AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::LOG_DIR) @@ -310,6 +379,18 @@ impl Configurable for AgentConfig { None }; + let mut final_port = if let Ok(server_port) = + AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::SERVER_PORT) + { + u16::from_str(&server_port) + .unwrap_or_else(|_| panic!("Error parsing webserver port from [{}], aborting.")) + } else { + // This should not be necessary, as the default should be provided by clap, + // it is more _just in case_ + u16::from_str(&AgentConfig::SERVER_PORT.default.unwrap_or("3000")) + .unwrap_or_else(|_| panic!("Error parsing webserver port from [{}], aborting.")) + }; + let mut final_tags: HashMap = HashMap::new(); if let Some(Some(tags)) = parsed_values.get(&AgentConfig::TAG) { for tag in tags { @@ -330,10 +411,12 @@ impl Configurable for AgentConfig { } Ok(AgentConfig { + hostname: final_hostname, parcel_directory: final_parcel_dir, config_directory: final_config_dir, log_directory: final_log_dir, server_ip_address: final_ip, + server_port: final_port, server_cert_file: final_server_cert_file, server_key_file: final_server_key_file, tags: final_tags, diff --git a/src/main.rs b/src/main.rs index 5a3bd37..feaa7d7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,13 +3,14 @@ use std::ffi::OsString; use kube::config::Config as KubeConfig; use kube::config::KubeConfigOptions; -use kubelet::config::Config; +use kubelet::config::{Config, ServerConfig}; use kubelet::Kubelet; use log::{info, warn}; use stackable_config::ConfigBuilder; use crate::agentconfig::AgentConfig; use crate::provider::StackableProvider; +use std::path::PathBuf; mod agentconfig; mod provider; @@ -46,19 +47,40 @@ async fn main() -> anyhow::Result<()> { export_env("NODE_LABELS", &node_labels); - if let Some(cert_file_path) = agent_config.server_cert_file { + if let Some(cert_file_path) = &agent_config.server_cert_file { export_env("KRUSTLET_CERT_FILE", cert_file_path.to_str().unwrap()); } else { warn!("Not exporting server cert file path, as non was specified that could be converted to a String."); } - if let Some(key_file_path) = agent_config.server_key_file { + if let Some(key_file_path) = &agent_config.server_key_file { export_env("KRUSTLET_PRIVATE_KEY_FILE", key_file_path.to_str().unwrap()); } else { warn!("Not exporting server key file path, as non was specified that could be converted to a String."); } info!("args: {:?}", env::args()); - let krustlet_config = Config::new_from_flags(env!("CARGO_PKG_VERSION")); + + let krustlet_config_comp = Config::new_from_flags(env!("CARGO_PKG_VERSION")); + let server_config = ServerConfig { + addr: agent_config.server_ip_address.clone(), + port: agent_config.server_port, + cert_file: agent_config.server_cert_file.unwrap_or(Default::default()), + private_key_file: agent_config.server_key_file.unwrap_or(Default::default()), + }; + + let krustlet_config = Config { + node_ip: agent_config.server_ip_address.clone(), + hostname: agent_config.hostname.clone(), + node_name: agent_config.hostname.clone(), + server_config, + data_dir: PathBuf::from("/home/sliebau/.krustlet"), + node_labels: agent_config.tags, + max_pods: 0, + bootstrap_file: PathBuf::from("/etc/kubernetes/bootstrap-kubelet.conf"), + allow_local_modules: false, + insecure_registries: None, + plugins_dir: PathBuf::from("/home/sliebau/.krustlet/plugins"), + }; let kubeconfig = KubeConfig::from_kubeconfig(&KubeConfigOptions::default()) .await From a1856520adbaf9fb51a33fbc9e0da71f0b771db8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 19 Jan 2021 14:05:09 +0100 Subject: [PATCH 2/7] Added bootstrap file as parameter. Pulled out documentation strings into separate files, which allows editing them as proper adoc files with preview, instead of string literals in Rust code. This makes writing documentation _a lot_ easier. --- Cargo.toml | 1 - src/agentconfig.rs | 104 ++++++++++++------ src/config_documentation/bootstrap_file.adoc | 0 .../config_directory.adoc | 11 ++ src/config_documentation/data_directory.adoc | 0 src/config_documentation/hostname.adoc | 0 src/config_documentation/log_directory.adoc | 5 + src/config_documentation/no_config.adoc | 0 .../package_directory.adoc | 6 + .../plugin_directory.adoc | 0 .../server_cert_file.adoc | 0 .../server_ip_address.adoc | 0 src/config_documentation/server_key_file.adoc | 0 src/config_documentation/server_port.adoc | 0 src/config_documentation/tags.adoc | 3 + src/main.rs | 7 +- 16 files changed, 101 insertions(+), 36 deletions(-) create mode 100644 src/config_documentation/bootstrap_file.adoc create mode 100644 src/config_documentation/config_directory.adoc create mode 100644 src/config_documentation/data_directory.adoc create mode 100644 src/config_documentation/hostname.adoc create mode 100644 src/config_documentation/log_directory.adoc create mode 100644 src/config_documentation/no_config.adoc create mode 100644 src/config_documentation/package_directory.adoc create mode 100644 src/config_documentation/plugin_directory.adoc create mode 100644 src/config_documentation/server_cert_file.adoc create mode 100644 src/config_documentation/server_ip_address.adoc create mode 100644 src/config_documentation/server_key_file.adoc create mode 100644 src/config_documentation/server_port.adoc create mode 100644 src/config_documentation/tags.adoc diff --git a/Cargo.toml b/Cargo.toml index c2dbe65..b944113 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,7 +10,6 @@ edition = "2018" # We will look to move this to officially released versions as soon as possible kubelet = { git="https://github.com/deislabs/krustlet.git", rev="ac218b38ba564de806568e49d9e38aaef9f41537", default-features = true, features= ["derive", "cli"] } oci-distribution = { git="https://github.com/deislabs/krustlet.git", rev="ac218b38ba564de806568e49d9e38aaef9f41537"} - k8s-openapi = { version = "0.9", default-features = false, features = ["v1_18"] } kube = { version= "0.42", default-features = false, features = ["native-tls"] } kube-derive = "0.43" diff --git a/src/agentconfig.rs b/src/agentconfig.rs index b8dd773..840dba2 100644 --- a/src/agentconfig.rs +++ b/src/agentconfig.rs @@ -23,6 +23,9 @@ pub struct AgentConfig { pub parcel_directory: PathBuf, pub config_directory: PathBuf, pub log_directory: PathBuf, + pub bootstrap_file: PathBuf, + pub data_directory: PathBuf, + pub plugin_directory: PathBuf, pub server_ip_address: IpAddr, pub server_port: u16, pub server_cert_file: Option, @@ -38,27 +41,37 @@ impl AgentConfig { takes_argument: true, help: "The hostname to register the node under in Kubernetes - defaults to system hostname.", - documentation: "", + documentation: include_str!("config_documentation/hostname.adoc"), list: false, }; pub const DATA_DIR: ConfigOption = ConfigOption { - name: "datadir", - default: Some("/etc/stackable-agent"), + name: "data-directory", + default: Some("/etc/stackable/agent"), required: false, takes_argument: true, help: "The directory where the stackable agent should keep its working data.", - documentation: "", + documentation: include_str!("config_documentation/data_directory.adoc"), list: false, }; pub const PLUGIN_DIR: ConfigOption = ConfigOption { - name: "plugindir", - default: Some("/etc/stackable-agent/plugins"), + name: "plugin-directory", + default: Some("/etc/stackable/agent/plugins"), required: false, takes_argument: true, help: "The directory to observe for new sockets to appear which can be used to communicate with CSI plugins.", - documentation: "", + documentation: include_str!("config_documentation/plugin_directory.adoc"), + list: false, + }; + + pub const BOOTSTRAP_FILE: ConfigOption = ConfigOption { + name: "bootstrap-file", + default: Some("/etc/kubernetes/bootstrap-kubelet.conf"), + required: false, + takes_argument: true, + help: "The bootstrap file to use in case Kubernetes bootstraping is used to add the agent.", + documentation: include_str!("config_documentation/bootstrap_file.adoc"), list: false, }; @@ -68,7 +81,7 @@ impl AgentConfig { required: false, takes_argument: true, help: "The local IP to register as the node's ip with the apiserver. Will be automatically set to the first address of the first non-loopback interface if not specified.", - documentation: "", + documentation: include_str!("config_documentation/server_ip_address.adoc"), list: false, }; @@ -78,7 +91,7 @@ impl AgentConfig { required: false, takes_argument: true, help: "The certificate file for the local webserver which the Krustlet starts.", - documentation: "", + documentation: include_str!("config_documentation/server_cert_file.adoc"), list: false, }; @@ -89,7 +102,7 @@ impl AgentConfig { takes_argument: true, help: "Private key file (in PKCS8 format) to use for the local webserver the Krustlet starts.", - documentation: "", + documentation: include_str!("config_documentation/server_key_file.adoc"), list: false, }; @@ -99,7 +112,7 @@ impl AgentConfig { required: false, takes_argument: true, help: "Port to listen on for callbacks.", - documentation: "", + documentation: include_str!("config_documentation/server_port.adoc"), list: false, }; @@ -109,12 +122,7 @@ impl AgentConfig { required: false, takes_argument: true, help: "The base directory under which installed packages will be stored.", - documentation: "This directory will serve as starting point for packages that are needed by \ - pods assigned to this node.\n Packages will be downloaded into the \"_download\" folder at the -top level of this folder as archives and remain there for potential future use.\n\ - Archives will the be extracted directly into this folder in subdirectories following the naming -scheme of \"productname-productversion\". - The agent will need full access to this directory and tries to create it if it does not exist.", + documentation: include_str!("config_documentation/package_directory.adoc"), list: false, }; @@ -124,12 +132,7 @@ scheme of \"productname-productversion\". required: false, takes_argument: true, help: "The base directory under which configuration will be generated for all executed services.", - documentation: "This directory will serve as starting point for all log files which this service creates.\ - Every service will get its own subdirectories created within this directory - for every service start a \ - new subdirectory will be created to show a full history of configuration that was used for this service.\n - ConfigMaps that are mounted into the pod that describes this service will be created relative to these run \ - directories - unless the mounts specify an absolute path, in which case it is allowed to break out of this directory.\n\n\ - The agent will need full access to this directory and tries to create it if it does not exist.", + documentation: include_str!("config_documentation/config_directory.adoc"), list: false, }; @@ -139,10 +142,7 @@ scheme of \"productname-productversion\". required: false, takes_argument: true, help: "The base directory under which log files will be placed for all services.", - documentation: "This directory will serve as starting point for all log files which this service creates.\ - Every service will get its own subdirectory created within this directory.\n - Anything that is then specified in the log4j config or similar files will be resolved relatively to this directory.\n\n\ - The agent will need full access to this directory and tries to create it if it does not exist.", + documentation: include_str!("config_documentation/log_directory.adoc"), list: false, }; @@ -152,7 +152,7 @@ scheme of \"productname-productversion\". required: false, takes_argument: false, help: "If this option is specified, any file referenced in AGENT_CONF environment variable will be ignored.", - documentation: "", + documentation: include_str!("config_documentation/no_config.adoc"), list: false, }; @@ -162,7 +162,7 @@ scheme of \"productname-productversion\". required: false, takes_argument: true, help: "A \"key=value\" pair that should be assigned to this agent as tag. This can be specified multiple times to assign additional tags.", - documentation: "Tags are the main way of identifying nodes to assign services to later on.", + documentation: include_str!("config_documentation/tags.adoc"), list: true }; @@ -291,11 +291,48 @@ impl Configurable for AgentConfig { // Parse data directory from values let final_data_dir = if let Ok(data_dir) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::DATA_DIR) + AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::DATA_DIR) { - PathBuf::from_str(&data_dir).unwrap_or_else(|_| {panic!("Error parsing valid data directory from string: {}", data_dir)}) - } + PathBuf::from_str(&data_dir).unwrap_or_else(|_| { + panic!( + "Error parsing valid data directory from string: {}", + data_dir + ) + }) + } else { + // Should not happen due to default value being assigned to the parameter + PathBuf::from("") + }; + + // Parse bootstrap file from values + let final_bootstrap_file = if let Ok(bootstrap_file) = + AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::BOOTSTRAP_FILE) + { + PathBuf::from_str(&bootstrap_file).unwrap_or_else(|_| { + panic!( + "Error parsing valid data directory from string: {}", + bootstrap_file + ) + }) + } else { + // Should not happen due to default value being assigned to the parameter + PathBuf::from("") + }; + // Parse plugin directory from values + let final_plugin_dir = if let Ok(plugin_dir) = + AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::PLUGIN_DIR) + { + PathBuf::from_str(&plugin_dir).unwrap_or_else(|_| { + panic!( + "Error parsing valid plugin directory from string: {}", + plugin_dir + ) + }) + } else { + // Should not happen due to default value being assigned to the parameter + PathBuf::from("") + }; // Parse log directory let final_log_dir = if let Ok(log_dir) = @@ -414,7 +451,10 @@ impl Configurable for AgentConfig { hostname: final_hostname, parcel_directory: final_parcel_dir, config_directory: final_config_dir, + data_directory: final_data_dir, + plugin_directory: final_plugin_dir, log_directory: final_log_dir, + bootstrap_file: final_bootstrap_file, server_ip_address: final_ip, server_port: final_port, server_cert_file: final_server_cert_file, diff --git a/src/config_documentation/bootstrap_file.adoc b/src/config_documentation/bootstrap_file.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/config_directory.adoc b/src/config_documentation/config_directory.adoc new file mode 100644 index 0000000..0551dbf --- /dev/null +++ b/src/config_documentation/config_directory.adoc @@ -0,0 +1,11 @@ +This directory will serve as starting point for all log files which this service creates. + +Every service will get its own subdirectories created within this directory - for every service start a +new subdirectory will be created to show a full history of configuration that was used for this service. + +ConfigMaps which are specified in the pod that describes this service will be created relative to these run +directories - unless the mounts specify an absolute path, in which case it is allowed to break out of this directory. + +WARNING: This allows anybody who can specify pods more or less full access to the file system on the machine running the agent! + +The agent will need full access to this directory and tries to create it if it does not exist. \ No newline at end of file diff --git a/src/config_documentation/data_directory.adoc b/src/config_documentation/data_directory.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/hostname.adoc b/src/config_documentation/hostname.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/log_directory.adoc b/src/config_documentation/log_directory.adoc new file mode 100644 index 0000000..10264a1 --- /dev/null +++ b/src/config_documentation/log_directory.adoc @@ -0,0 +1,5 @@ +This directory will serve as starting point for all log files which this service creates. +Every service will get its own subdirectory created within this directory. +Anything that is then specified in the log4j config or similar files will be resolved relatively to this directory. + +The agent will need full access to this directory and tries to create it if it does not exist. \ No newline at end of file diff --git a/src/config_documentation/no_config.adoc b/src/config_documentation/no_config.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/package_directory.adoc b/src/config_documentation/package_directory.adoc new file mode 100644 index 0000000..b619ac3 --- /dev/null +++ b/src/config_documentation/package_directory.adoc @@ -0,0 +1,6 @@ +This directory will serve as starting point for packages that are needed by pods assigned to this node.\n Packages will be downloaded into the "_download" folder at the top level of this folder as archives and remain there for potential future use. + +Archives will the be extracted directly into this folder in subdirectories following the naming +scheme of "productname-productversion". + +The agent will need full access to this directory and tries to create it if it does not exist. \ No newline at end of file diff --git a/src/config_documentation/plugin_directory.adoc b/src/config_documentation/plugin_directory.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/server_cert_file.adoc b/src/config_documentation/server_cert_file.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/server_ip_address.adoc b/src/config_documentation/server_ip_address.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/server_key_file.adoc b/src/config_documentation/server_key_file.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/server_port.adoc b/src/config_documentation/server_port.adoc new file mode 100644 index 0000000..e69de29 diff --git a/src/config_documentation/tags.adoc b/src/config_documentation/tags.adoc new file mode 100644 index 0000000..8d814eb --- /dev/null +++ b/src/config_documentation/tags.adoc @@ -0,0 +1,3 @@ +A "key=value" pair that should be assigned to this agent as tag. This can be specified multiple times to assign additional tags. + +Tags are the main way of identifying nodes to assign services to later on. \ No newline at end of file diff --git a/src/main.rs b/src/main.rs index feaa7d7..755bf98 100644 --- a/src/main.rs +++ b/src/main.rs @@ -73,13 +73,14 @@ async fn main() -> anyhow::Result<()> { hostname: agent_config.hostname.clone(), node_name: agent_config.hostname.clone(), server_config, - data_dir: PathBuf::from("/home/sliebau/.krustlet"), + data_dir: agent_config.data_directory, + plugins_dir: agent_config.plugin_directory, node_labels: agent_config.tags, - max_pods: 0, + // TODO: Discuss whether we want this configurable or leave it at a high number for now + max_pods: 110, bootstrap_file: PathBuf::from("/etc/kubernetes/bootstrap-kubelet.conf"), allow_local_modules: false, insecure_registries: None, - plugins_dir: PathBuf::from("/home/sliebau/.krustlet/plugins"), }; let kubeconfig = KubeConfig::from_kubeconfig(&KubeConfigOptions::default()) From 773d81a94f07a1c8b067ad58cc976ec4dd34ebeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 20 Jan 2021 14:47:09 +0100 Subject: [PATCH 3/7] Removed unused initialization of comparison config object. Added parsing of bootstrapfile from command line. --- src/main.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 755bf98..863e684 100644 --- a/src/main.rs +++ b/src/main.rs @@ -60,7 +60,6 @@ async fn main() -> anyhow::Result<()> { } info!("args: {:?}", env::args()); - let krustlet_config_comp = Config::new_from_flags(env!("CARGO_PKG_VERSION")); let server_config = ServerConfig { addr: agent_config.server_ip_address.clone(), port: agent_config.server_port, @@ -78,7 +77,7 @@ async fn main() -> anyhow::Result<()> { node_labels: agent_config.tags, // TODO: Discuss whether we want this configurable or leave it at a high number for now max_pods: 110, - bootstrap_file: PathBuf::from("/etc/kubernetes/bootstrap-kubelet.conf"), + bootstrap_file: agent_config.bootstrap_file, allow_local_modules: false, insecure_registries: None, }; From 90274bca604c70ddee5b1abff3f2b809c9c72a79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Jan 2021 11:21:17 +0100 Subject: [PATCH 4/7] Refactored parsing of paths from config into an extra fn and implemented collecting all parse errors before panicing. --- src/agentconfig.rs | 222 ++++++++++++++++++++++----------------------- src/main.rs | 5 +- 2 files changed, 110 insertions(+), 117 deletions(-) diff --git a/src/agentconfig.rs b/src/agentconfig.rs index 840dba2..f15679c 100644 --- a/src/agentconfig.rs +++ b/src/agentconfig.rs @@ -1,3 +1,4 @@ +use anyhow::anyhow; use std::collections::hash_map::RandomState; use std::collections::{HashMap, HashSet}; use std::net::IpAddr; @@ -9,12 +10,14 @@ use pnet::datalink; use stackable_config::{ConfigOption, Configurable, Configuration}; use thiserror::Error; -use crate::agentconfig::AgentConfigError::WrongArgumentCount; +use crate::agentconfig::AgentConfigError::{ArgumentParseError, WrongArgumentCount}; #[derive(Error, Debug)] pub enum AgentConfigError { #[error("Wrong number of arguments found for config option {}!", .option.name)] WrongArgumentCount { option: ConfigOption }, + #[error("Unable to parse value for parameter [{}]!", .name)] + ArgumentParseError { name: String }, } #[derive(Clone)] @@ -47,7 +50,7 @@ impl AgentConfig { pub const DATA_DIR: ConfigOption = ConfigOption { name: "data-directory", - default: Some("/etc/stackable/agent"), + default: Some("/var/stackable/agent/data"), required: false, takes_argument: true, help: "The directory where the stackable agent should keep its working data.", @@ -180,6 +183,7 @@ impl AgentConfig { AgentConfig::LOG_DIR, AgentConfig::NO_CONFIG, AgentConfig::TAG, + AgentConfig::BOOTSTRAP_FILE, ] .iter() .cloned() @@ -221,6 +225,38 @@ impl AgentConfig { }) } + /// Helper method to retrieve a path from the config and convert this to a PathBuf directly. + /// This method assumes that a default value has been specified for this option and panics if + /// no value can be retrieved (should only happen if assigning the default value fails or + /// one was not specified) + /// + /// # Panics + /// This function panics if the parsed_values object does not contain a value for the key. + /// This is due to the fact that we expect a default value to be defined for these parameters, + /// so if we do not get a value that default value has not been defined or something else went + /// badly wrong. + fn get_with_default( + parsed_values: &HashMap>>, + option: &ConfigOption, + error_list: &mut Vec, + ) -> Result { + T::from_str( + &AgentConfig::get_exactly_one_string(&parsed_values, option).unwrap_or_else(|_| { + panic!( + "No value present for parameter {} even though it should have a default value!", + option.name + ) + }), + ) + .map_err(|_| { + let error = ArgumentParseError { + name: option.name.to_string(), + }; + error_list.push(error); + anyhow!("Error for parameter: {}", option.name) + }) + } + /// This tries to find the first non loopback interface with an ip address assigned. /// This should usually be the default interface according to: /// @@ -250,9 +286,9 @@ impl AgentConfig { } fn default_hostname() -> anyhow::Result { - Ok(hostname::get()? + hostname::get()? .into_string() - .map_err(|_| anyhow::anyhow!("invalid utf-8 hostname string"))?) + .map_err(|_| anyhow::anyhow!("invalid utf-8 hostname string")) } } @@ -289,104 +325,55 @@ impl Configurable for AgentConfig { }; info!("Selected {} as local address to listen on.", final_ip); - // Parse data directory from values - let final_data_dir = if let Ok(data_dir) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::DATA_DIR) - { - PathBuf::from_str(&data_dir).unwrap_or_else(|_| { - panic!( - "Error parsing valid data directory from string: {}", - data_dir - ) - }) - } else { - // Should not happen due to default value being assigned to the parameter - PathBuf::from("") - }; + let mut error_list = vec![]; + + // Parse directory/file parameters + // PathBuf::from_str returns an infallible as Error, so cannot fail, hence unwrap is save + // to use for PathBufs here + + // Parse data directory from values, add any error that occured to the list of errors + let final_data_dir = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::DATA_DIR, + error_list.as_mut(), + ); // Parse bootstrap file from values - let final_bootstrap_file = if let Ok(bootstrap_file) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::BOOTSTRAP_FILE) - { - PathBuf::from_str(&bootstrap_file).unwrap_or_else(|_| { - panic!( - "Error parsing valid data directory from string: {}", - bootstrap_file - ) - }) - } else { - // Should not happen due to default value being assigned to the parameter - PathBuf::from("") - }; + let final_bootstrap_file = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::BOOTSTRAP_FILE, + error_list.as_mut(), + ); + debug!("bootstrapfile: {:?}", final_bootstrap_file); // Parse plugin directory from values - let final_plugin_dir = if let Ok(plugin_dir) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::PLUGIN_DIR) - { - PathBuf::from_str(&plugin_dir).unwrap_or_else(|_| { - panic!( - "Error parsing valid plugin directory from string: {}", - plugin_dir - ) - }) - } else { - // Should not happen due to default value being assigned to the parameter - PathBuf::from("") - }; + let final_plugin_dir = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::PLUGIN_DIR, + error_list.as_mut(), + ); + debug!("plugindir: {:?}", final_plugin_dir); // Parse log directory - let final_log_dir = if let Ok(log_dir) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::LOG_DIR) - { - PathBuf::from_str(&log_dir).unwrap_or_else(|_| { - panic!("Error parsing valid log directory from string: {}", log_dir) - }) - } else { - PathBuf::from_str( - AgentConfig::LOG_DIR - .default - .expect("Invalid default value for log directory option!"), - ) - .unwrap_or_else(|_| panic!("Unable to get log directory from options!".to_string())) - }; + let final_log_dir = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::LOG_DIR, + error_list.as_mut(), + ); // Parse config directory - let final_config_dir = if let Ok(config_dir) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::CONFIG_DIR) - { - PathBuf::from_str(&config_dir).unwrap_or_else(|_| { - panic!( - "Error parsing valid config directory from string: {}", - config_dir - ) - }) - } else { - PathBuf::from_str( - AgentConfig::CONFIG_DIR - .default - .expect("Invalid default value for config directory option!"), - ) - .unwrap_or_else(|_| panic!("Unable to get config directory from options!".to_string())) - }; + let final_config_dir = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::CONFIG_DIR, + error_list.as_mut(), + ); // Parse parcel directory - let final_parcel_dir = if let Ok(parcel_dir) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::PACKAGE_DIR) - { - PathBuf::from_str(&parcel_dir).unwrap_or_else(|_| { - panic!( - "Error parsing valid parcel directory from string: {}", - parcel_dir - ) - }) - } else { - PathBuf::from_str( - AgentConfig::PACKAGE_DIR - .default - .expect("Invalid default value for parcel directory option!"), - ) - .unwrap_or_else(|_| panic!("Unable to get parcel directory from options!".to_string())) - }; + let final_package_dir = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::PACKAGE_DIR, + error_list.as_mut(), + ); // Parse cert file let final_server_cert_file = if let Ok(server_cert_file) = @@ -416,17 +403,11 @@ impl Configurable for AgentConfig { None }; - let mut final_port = if let Ok(server_port) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::SERVER_PORT) - { - u16::from_str(&server_port) - .unwrap_or_else(|_| panic!("Error parsing webserver port from [{}], aborting.")) - } else { - // This should not be necessary, as the default should be provided by clap, - // it is more _just in case_ - u16::from_str(&AgentConfig::SERVER_PORT.default.unwrap_or("3000")) - .unwrap_or_else(|_| panic!("Error parsing webserver port from [{}], aborting.")) - }; + let final_port = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::SERVER_PORT, + error_list.as_mut(), + ); let mut final_tags: HashMap = HashMap::new(); if let Some(Some(tags)) = parsed_values.get(&AgentConfig::TAG) { @@ -436,10 +417,9 @@ impl Configurable for AgentConfig { // We want to avoid any "unpredictable" behavior like ignoring a malformed // key=value pair with just a log message -> so we panic if this can't be // parsed - panic!(format!( - "Unable to parse value [{}] for option --tag as key=value pair!", - tag - )) + error_list.push(ArgumentParseError { + name: AgentConfig::TAG.name.to_string(), + }); } else { // This might panic, but really shouldn't, as we've checked the size of the array final_tags.insert(split[0].to_string(), split[1].to_string()); @@ -447,16 +427,30 @@ impl Configurable for AgentConfig { } } + // Panic if we encountered any errors during parsing of the values + if !error_list.is_empty() { + panic!( + "Error parsing command line parameters:\n{}", + error_list + .into_iter() + .map(|thiserror| format!("{:?}\n", thiserror)) + .collect::() + ); + } + + // These unwraps are ok to panic, if one of them barfs then something went horribly wrong + // above, as we should have paniced in a "controlled fashion" from the conditional block + // right before this Ok(AgentConfig { hostname: final_hostname, - parcel_directory: final_parcel_dir, - config_directory: final_config_dir, - data_directory: final_data_dir, - plugin_directory: final_plugin_dir, - log_directory: final_log_dir, - bootstrap_file: final_bootstrap_file, + parcel_directory: final_package_dir.unwrap(), + config_directory: final_config_dir.unwrap(), + data_directory: final_data_dir.unwrap(), + plugin_directory: final_plugin_dir.unwrap(), + log_directory: final_log_dir.unwrap(), + bootstrap_file: final_bootstrap_file.unwrap(), server_ip_address: final_ip, - server_port: final_port, + server_port: final_port.unwrap(), server_cert_file: final_server_cert_file, server_key_file: final_server_key_file, tags: final_tags, diff --git a/src/main.rs b/src/main.rs index 863e684..aa4dbf9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,7 +10,6 @@ use stackable_config::ConfigBuilder; use crate::agentconfig::AgentConfig; use crate::provider::StackableProvider; -use std::path::PathBuf; mod agentconfig; mod provider; @@ -68,9 +67,9 @@ async fn main() -> anyhow::Result<()> { }; let krustlet_config = Config { - node_ip: agent_config.server_ip_address.clone(), + node_ip: agent_config.server_ip_address, hostname: agent_config.hostname.clone(), - node_name: agent_config.hostname.clone(), + node_name: agent_config.hostname, server_config, data_dir: agent_config.data_directory, plugins_dir: agent_config.plugin_directory, From 606af1f55e855f616006bf500c56b2041dada8bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Jan 2021 12:07:46 +0100 Subject: [PATCH 5/7] Fixed two clippy warnings. --- src/agentconfig.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/agentconfig.rs b/src/agentconfig.rs index f15679c..162a85b 100644 --- a/src/agentconfig.rs +++ b/src/agentconfig.rs @@ -212,12 +212,12 @@ impl AgentConfig { parsed_values.get(option) ); if let Some(Some(list_value)) = parsed_values.get(option) { - if list_value.len() != 1 { - error!("Got additional, unexpected values for parameter!"); - } else { + if list_value.len() == 1 { // We've checked that the list has exactly one value at this point, so no errors should // occur after this point - but you never know return Ok(list_value[0].to_string()); + } else { + error!("Got additional, unexpected values for parameter!"); } } Err(WrongArgumentCount { @@ -413,16 +413,16 @@ impl Configurable for AgentConfig { if let Some(Some(tags)) = parsed_values.get(&AgentConfig::TAG) { for tag in tags { let split: Vec<&str> = tag.split('=').collect(); - if split.len() != 2 { + if split.len() == 2 { + // This might panic, but really shouldn't, as we've checked the size of the array + final_tags.insert(split[0].to_string(), split[1].to_string()); + } else { // We want to avoid any "unpredictable" behavior like ignoring a malformed // key=value pair with just a log message -> so we panic if this can't be // parsed error_list.push(ArgumentParseError { name: AgentConfig::TAG.name.to_string(), }); - } else { - // This might panic, but really shouldn't, as we've checked the size of the array - final_tags.insert(split[0].to_string(), split[1].to_string()); } } } From ed4e2e622c4cb2a6f6147a266cce444b1510ed70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Jan 2021 14:25:04 +0100 Subject: [PATCH 6/7] Removed config option for plugin directory. --- src/agentconfig.rs | 22 ---------------------- src/main.rs | 2 +- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/src/agentconfig.rs b/src/agentconfig.rs index 162a85b..66aa119 100644 --- a/src/agentconfig.rs +++ b/src/agentconfig.rs @@ -28,7 +28,6 @@ pub struct AgentConfig { pub log_directory: PathBuf, pub bootstrap_file: PathBuf, pub data_directory: PathBuf, - pub plugin_directory: PathBuf, pub server_ip_address: IpAddr, pub server_port: u16, pub server_cert_file: Option, @@ -58,16 +57,6 @@ impl AgentConfig { list: false, }; - pub const PLUGIN_DIR: ConfigOption = ConfigOption { - name: "plugin-directory", - default: Some("/etc/stackable/agent/plugins"), - required: false, - takes_argument: true, - help: "The directory to observe for new sockets to appear which can be used to communicate with CSI plugins.", - documentation: include_str!("config_documentation/plugin_directory.adoc"), - list: false, - }; - pub const BOOTSTRAP_FILE: ConfigOption = ConfigOption { name: "bootstrap-file", default: Some("/etc/kubernetes/bootstrap-kubelet.conf"), @@ -173,7 +162,6 @@ impl AgentConfig { [ AgentConfig::HOSTNAME, AgentConfig::DATA_DIR, - AgentConfig::PLUGIN_DIR, AgentConfig::SERVER_IP_ADDRESS, AgentConfig::SERVER_CERT_FILE, AgentConfig::SERVER_KEY_FILE, @@ -344,15 +332,6 @@ impl Configurable for AgentConfig { &AgentConfig::BOOTSTRAP_FILE, error_list.as_mut(), ); - debug!("bootstrapfile: {:?}", final_bootstrap_file); - - // Parse plugin directory from values - let final_plugin_dir = AgentConfig::get_with_default( - &parsed_values, - &AgentConfig::PLUGIN_DIR, - error_list.as_mut(), - ); - debug!("plugindir: {:?}", final_plugin_dir); // Parse log directory let final_log_dir = AgentConfig::get_with_default( @@ -446,7 +425,6 @@ impl Configurable for AgentConfig { parcel_directory: final_package_dir.unwrap(), config_directory: final_config_dir.unwrap(), data_directory: final_data_dir.unwrap(), - plugin_directory: final_plugin_dir.unwrap(), log_directory: final_log_dir.unwrap(), bootstrap_file: final_bootstrap_file.unwrap(), server_ip_address: final_ip, diff --git a/src/main.rs b/src/main.rs index aa4dbf9..51e276b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -72,7 +72,7 @@ async fn main() -> anyhow::Result<()> { node_name: agent_config.hostname, server_config, data_dir: agent_config.data_directory, - plugins_dir: agent_config.plugin_directory, + plugins_dir: Default::default(), node_labels: agent_config.tags, // TODO: Discuss whether we want this configurable or leave it at a high number for now max_pods: 110, From bab35852b374f810d9d5d97d74e8d5ee6d34e5c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Jan 2021 14:41:49 +0100 Subject: [PATCH 7/7] Fixed comments --- src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 51e276b..2d3d0d1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -62,8 +62,8 @@ async fn main() -> anyhow::Result<()> { let server_config = ServerConfig { addr: agent_config.server_ip_address.clone(), port: agent_config.server_port, - cert_file: agent_config.server_cert_file.unwrap_or(Default::default()), - private_key_file: agent_config.server_key_file.unwrap_or(Default::default()), + cert_file: agent_config.server_cert_file.unwrap_or_default(), + private_key_file: agent_config.server_key_file.unwrap_or_default(), }; let krustlet_config = Config {