Fix naming issues (#374)

* Initial commit: check workload and agent naming conventions at server start-up * Check for improper naming when ank apply is being called. * Add unit tests for naming issues * Provide better error messages * Fix agent name constraint and wrongly modified config file * Update requirements and add stests for wrong names * Add stest for long workload name * Fix regex * Apply suggestions from code review Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com> * Simplify path checking * Trace and update docs * Fix agent name checking in CLI * Update doc/docs/usage/quickstart.md Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com> * Update server and agent requirements * Update server/doc/swdesign/README.md Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com> * Fix agent cli name parsing --------- Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>
eclipse-ankaios · Sep 26, 2024 · 32219bb · 32219bb
1 parent 0af7daf
commit 32219bb
Show file tree

Hide file tree

Showing 20 changed files with 454 additions and 41 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/agent/Cargo.toml b/agent/Cargo.toml
@@ -39,6 +39,7 @@ serde_json = "1.0"
 uuid = { version = "1.3", features = ["v4", "fast-rng"] }
 sha256 = "1.5"
 umask = "2.1.0"
+regex = "1.10"
 
 [dev-dependencies]
 common = { path = "../common", features = ["test_utils"] }

diff --git a/agent/doc/swdesign/README.md b/agent/doc/swdesign/README.md
@@ -125,6 +125,20 @@ The following diagram shows the startup sequence of the Ankaios Agent:
 
 ![Startup](plantuml/seq_startup.svg)
 
+#### Agent naming convention
+`swdd~agent-naming-convention~1`
+
+Status: approved
+
+The Ankaios CLI shall enforce agent names which respect the naming convention defined in the common library.
+
+Tags:
+- AgentManager
+
+Needs:
+- impl
+- stest
+
 #### Agent communicates only with the Server
 `swdd~agent-shall-use-interfaces-to-server~1`
 

diff --git a/agent/src/cli.rs b/agent/src/cli.rs
@@ -12,17 +12,32 @@
 //
 // SPDX-License-Identifier: Apache-2.0
 
+use regex::Regex;
 use std::path::Path;
 
 #[cfg_attr(test, mockall_double::double)]
 use crate::control_interface::Directory;
 use crate::control_interface::FileSystemError;
 use clap::Parser;
+use common::objects::state::STR_RE_AGENT;
 use common::DEFAULT_SERVER_ADDRESS;
 
 const DEFAULT_RUN_FOLDER: &str = "/tmp/ankaios/";
 const RUNFOLDER_SUFFIX: &str = "_io";
 
+// [impl->swdd~agent-naming-convention~1]
+fn validate_agent_name(name: &str) -> Result<String, String> {
+    let re = Regex::new(STR_RE_AGENT).unwrap();
+    if re.is_match(name) {
+        Ok(name.to_string())
+    } else {
+        Err(format!(
+            "Agent name '{}' is invalid. It shall contain only regular upper and lowercase characters (a-z and A-Z), numbers and the symbols '-' and '_'.",
+            name
+        ))
+    }
+}
+
 // [impl->swdd~agent-supports-cli-argument-for-insecure-communication~1]
 // [impl->swdd~agent-supports-pem-file-paths-as-cli-arguments~1]
 #[derive(Parser, Debug)]
@@ -31,8 +46,9 @@ const RUNFOLDER_SUFFIX: &str = "_io";
         about="Ankaios - your friendly automotive workload orchestrator.\nWhat can the agent do for you?")
 ]
 pub struct Arguments {
-    #[clap(short = 'n', long = "name")]
+    #[clap(short = 'n', long = "name", value_parser = clap::builder::ValueParser::new(validate_agent_name))]
     /// The name to use for the registration with the server. Every agent has to register with a unique name.
+    /// Agent name shall contain only regular upper and lowercase characters (a-z and A-Z), numbers and the symbols "-" and "_".
     pub agent_name: String,
     #[clap(short = 's', long = "server-url", default_value_t = DEFAULT_SERVER_ADDRESS.to_string())]
     /// The server url.

diff --git a/agent/src/main.rs b/agent/src/main.rs
@@ -55,6 +55,7 @@ async fn main() {
     env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));
 
     let args = cli::parse();
+
     let server_url = match args.insecure {
         true => args.server_url.replace("http[s]", "http"),
         false => args.server_url.replace("http[s]", "https"),

diff --git a/ank/src/cli_commands/apply_manifests.rs b/ank/src/cli_commands/apply_manifests.rs
@@ -16,7 +16,7 @@ use super::{CliCommands, InputSourcePair};
 use crate::cli_commands::State;
 use crate::cli_error::CliError;
 use crate::{cli::ApplyArgs, output_debug};
-use common::objects::CompleteState;
+use common::objects::{CompleteState, STR_RE_WORKLOAD};
 use common::state_manipulation::{Object, Path};
 use std::collections::HashSet;
 
@@ -26,6 +26,8 @@ use self::tests::get_input_sources_mock as get_input_sources;
 #[cfg(not(test))]
 use super::get_input_sources;
 
+const WORKLOAD_LEVEL: usize = 1;
+
 // [impl->swdd~cli-apply-supports-ankaios-manifest~1]
 pub fn parse_manifest(manifest: &mut InputSourcePair) -> Result<(Object, Vec<Path>), String> {
     let state_obj_parsing_check: serde_yaml::Value = serde_yaml::from_reader(&mut manifest.1)
@@ -95,8 +97,11 @@ pub fn update_request_obj(
     paths: &[Path],
 ) -> Result<(), String> {
     for workload_path in paths.iter() {
-        let workload_name = &workload_path.parts()[1];
-        let cur_workload_spec = cur_obj.get(workload_path).unwrap().clone();
+        let workload_name = &workload_path.parts()[WORKLOAD_LEVEL];
+        if !cur_obj.check_if_provided_path_exists(workload_path) {
+            return Err(format!("The provided path does not exist! This may be caused by improper naming. Ankaios supports names defined by '{}'", STR_RE_WORKLOAD));
+        }
+        let cur_workload_spec = cur_obj.get(workload_path).unwrap();
         if req_obj.get(workload_path).is_none() {
             let _ = req_obj.set(workload_path, cur_workload_spec.clone());
         } else {
@@ -779,4 +784,109 @@ mod tests {
             .await;
         assert!(apply_result.is_ok());
     }
+
+    #[tokio::test]
+    async fn utest_apply_manifest_invalid_names() {
+        let _guard = crate::test_helper::MOCKALL_CONTEXT_SYNC
+            .get_lock_async()
+            .await;
+
+        let manifest_content = io::Cursor::new(
+            b"apiVersion: \"v0.1\"\nworkloads:
+            simple.manifest1:
+              runtime: podman
+              agent: agent_A
+              runtimeConfig: \"\"
+                ",
+        );
+
+        let mut manifest_data = String::new();
+        let _ = manifest_content.clone().read_to_string(&mut manifest_data);
+
+        let updated_state = CompleteState {
+            desired_state: serde_yaml::from_str(&manifest_data).unwrap(),
+            ..Default::default()
+        };
+
+        let mut mock_server_connection = MockServerConnection::default();
+        mock_server_connection
+            .expect_update_state()
+            .with(
+                eq(updated_state.clone()),
+                eq(vec!["desiredState.workloads.simple.manifest1".to_string()]),
+            )
+            .return_once(|_, _| {
+                Ok(UpdateStateSuccess {
+                    added_workloads: vec!["simple_manifest1.abc.agent_B".to_string()],
+                    deleted_workloads: vec![],
+                })
+            });
+        mock_server_connection
+            .expect_get_complete_state()
+            .with(eq(vec![]))
+            .return_once(|_| {
+                Ok((ank_base::CompleteState::from(CompleteState {
+                    desired_state: updated_state.desired_state,
+                    ..Default::default()
+                }))
+                .into())
+            });
+        mock_server_connection
+            .expect_take_missed_from_server_messages()
+            .return_once(|| {
+                vec![
+                    FromServer::Response(ank_base::Response {
+                        request_id: OTHER_REQUEST_ID.into(),
+                        response_content: Some(ank_base::response::ResponseContent::Error(
+                            Default::default(),
+                        )),
+                    }),
+                    FromServer::UpdateWorkloadState(UpdateWorkloadState {
+                        workload_states: vec![WorkloadState {
+                            instance_name: "simple_manifest1.abc.agent_B".try_into().unwrap(),
+                            execution_state: ExecutionState {
+                                state: objects::ExecutionStateEnum::Running(RunningSubstate::Ok),
+                                ..Default::default()
+                            },
+                        }],
+                    }),
+                ]
+            });
+        mock_server_connection
+            .expect_read_next_update_workload_state()
+            .return_once(|| {
+                Ok(UpdateWorkloadState {
+                    workload_states: vec![WorkloadState {
+                        instance_name: "simple_manifest1.abc.agent_B".try_into().unwrap(),
+                        execution_state: ExecutionState {
+                            state: objects::ExecutionStateEnum::Running(RunningSubstate::Ok),
+                            ..Default::default()
+                        },
+                    }],
+                })
+            });
+
+        let mut cmd = CliCommands {
+            _response_timeout_ms: RESPONSE_TIMEOUT_MS,
+            no_wait: false,
+            server_connection: mock_server_connection,
+        };
+
+        FAKE_GET_INPUT_SOURCE_MOCK_RESULT_LIST
+            .lock()
+            .unwrap()
+            .push_back(Ok(vec![(
+                "manifest.yml".to_string(),
+                Box::new(manifest_content),
+            )]));
+
+        let apply_result = cmd
+            .apply_manifests(ApplyArgs {
+                agent_name: None,
+                delete_mode: false,
+                manifest_files: vec!["manifest_yaml".to_string()],
+            })
+            .await;
+        assert!(apply_result.is_err());
+    }
 }
diff --git a/api/proto/ank_base.proto b/api/proto/ank_base.proto
@@ -227,6 +227,7 @@ message State {
 
 /**
 * This is a workaround for proto not supporing optional maps
+* Workload names shall not be shorter than 1 symbol longer then 63 symbols and can contain only regular characters, digits, the "-" and "_" symbols.
 */
 message WorkloadMap {
     map<string, Workload> workloads = 1;

diff --git a/common/Cargo.toml b/common/Cargo.toml
@@ -23,6 +23,7 @@ serde = { version = "1.0", features = ["derive"] }
 serde_yaml = "0.9"
 log = "0.4"
 sha256 = "1.5"
+regex = "1.10"
 
 [features]
 default = []

diff --git a/common/doc/swdesign/README.md b/common/doc/swdesign/README.md
@@ -332,6 +332,45 @@ Needs:
 - impl
 - utest
 
+#### Workload naming convention
+`swdd~common-workload-naming-convention~1`
+
+Status: approved
+
+The Common library shall provide functionality for enforcing a workload name to:
+* contain only regular upper and lowercase characters (a-z and A-Z), numbers and the symbols "-" and "_"
+* have a minimal length of 1 character
+* have a maximal length of 63 characters
+
+Rationale:
+A consistent naming manner assures stability in usage, compatibility with Ankaios internal structure and compliance to internet standards (RFC-1123).
+
+Tags:
+- Objects
+
+Needs:
+- impl
+- utest
+- stest
+
+#### Agent naming convention
+`swdd~common-agent-naming-convention~1`
+
+Status: approved
+
+The Common library shall provide functionality for enforcing an agent name to contain only regular upper and lowercase characters (a-z and A-Z), numbers and the symbols "-" and "_".
+
+Rationale:
+A consistent naming manner allows a flawless usage of the Ankaios CLI and does not tamper with the internal structure of Ankaios.
+
+Tags:
+- Objects
+
+Needs:
+- impl
+- utest
+- stest
+
 #### Provide common conversions between Ankaios and protobuf
 `swdd~common-conversions-between-ankaios-and-proto~1`
 

diff --git a/common/src/objects/mod.rs b/common/src/objects/mod.rs
@@ -16,8 +16,9 @@
 
 // [impl->swdd~common-conversions-between-ankaios-and-proto~1]
 
-mod state;
+pub mod state;
 pub use state::State;
+pub use state::{STR_RE_AGENT, STR_RE_WORKLOAD};
 
 mod complete_state;
 pub use complete_state::CompleteState;