Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce naming conventions #374

Merged
merged 18 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

116 changes: 115 additions & 1 deletion ank/src/cli_commands/apply_manifests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,16 @@ pub fn update_request_obj(
) -> 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();
cur_obj
.clone()
.check_if_provided_path_exists(workload_path)
krucod3 marked this conversation as resolved.
Show resolved Hide resolved
.map_err(|err| {
format!(
"Got error `{}`. This may be caused by improper naming. Ankaios supports names defined by '^[a-zA-Z0-9_-]+[a-zA-Z0-9_-]*$'.",
krucod3 marked this conversation as resolved.
Show resolved Hide resolved
err
)
})?;
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 {
Expand Down Expand Up @@ -779,4 +788,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());
}
}
1 change: 1 addition & 0 deletions common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 = []
Expand Down
34 changes: 34 additions & 0 deletions common/doc/swdesign/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,40 @@ Needs:
- impl
- utest

#### Workload naming convention
`swdd~common-workload-naming-convention~1`
krucod3 marked this conversation as resolved.
Show resolved Hide resolved

Status: approved

The Common library shall provide functionality for enforcing a workload name that matches <^[a-zA-Z0-9_-]+[a-zA-Z0-9_-]*$> and has a length of maximum 63 characters.
HorjuRares marked this conversation as resolved.
Show resolved Hide resolved

Rationale: A consistent naming manner assures stability in usage, compatibility with Ankaios internal structure and compliance to internet standards (RFC-1123).
HorjuRares marked this conversation as resolved.
Show resolved Hide resolved

Tags:
- Objects

Needs:
- impl
- utest
- stest

#### Agent naming convention
`swdd~common-workload-naming-convention~1`
HorjuRares marked this conversation as resolved.
Show resolved Hide resolved

Status: approved

The Common library shall provide functionality for enforcing an agent name that matches <^[a-zA-Z0-9_-]*$>.
krucod3 marked this conversation as resolved.
Show resolved Hide resolved

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`

Expand Down
121 changes: 108 additions & 13 deletions common/src/objects/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//
// SPDX-License-Identifier: Apache-2.0

use regex::Regex;
use serde::{Deserialize, Serialize};

use std::collections::HashMap;
Expand All @@ -22,6 +23,9 @@ use crate::objects::StoredWorkloadSpec;
use api::ank_base;

const CURRENT_API_VERSION: &str = "v0.1";
const MAX_CHARACTERS_WORKLOAD_NAME: usize = 63;
const STR_RE_WORKLOAD: &str = r"^[a-zA-Z0-9_-]+*$";
const STR_RE_AGENT: &str = r"^[a-zA-Z0-9_-]*$";

// [impl->swdd~common-object-representation~1]
// [impl->swdd~common-object-serialization~1]
Expand Down Expand Up @@ -75,8 +79,41 @@ impl TryFrom<ank_base::State> for State {
}

impl State {
pub fn is_compatible_format(api_version: &String) -> bool {
api_version == CURRENT_API_VERSION
pub fn verify_format(provided_state: &State) -> Result<(), String> {
if provided_state.api_version != CURRENT_API_VERSION {
return Err(format!(
"Unsupported API version. Received '{}', expected '{}'",
provided_state.api_version,
State::default().api_version
));
}

let re_workloads = Regex::new(STR_RE_WORKLOAD).unwrap();
let re_agent = Regex::new(STR_RE_AGENT).unwrap();

for (workload_name, workload_spec) in &provided_state.workloads {
if !re_workloads.is_match(workload_name.as_str()) {
return Err(format!(
"Unsupported workload name. Received '{}', expected to have characters in {}",
workload_name, STR_RE_WORKLOAD
));
}
if workload_name.len() > MAX_CHARACTERS_WORKLOAD_NAME {
return Err(format!(
"Workload name length {} exceeds the maximum limit of {} characters",
workload_name.len(),
MAX_CHARACTERS_WORKLOAD_NAME
));
}
if !re_agent.is_match(workload_spec.agent.as_str()) {
return Err(format!(
"Unsupported agent name. Received '{}', expected to have characters in {}",
workload_spec.agent, STR_RE_AGENT
));
}
}

Ok(())
}
}

Expand All @@ -94,12 +131,12 @@ impl State {
#[cfg(test)]
mod tests {

use std::collections::HashMap;

use super::{CURRENT_API_VERSION, MAX_CHARACTERS_WORKLOAD_NAME, STR_RE_AGENT, STR_RE_WORKLOAD};
use api::ank_base;
use std::collections::HashMap;

use crate::{
objects::State,
objects::{State, StoredWorkloadSpec},
test_utils::{generate_test_proto_state, generate_test_state},
};

Expand Down Expand Up @@ -140,20 +177,78 @@ mod tests {
let state_compatible_version = State {
..Default::default()
};
assert!(State::is_compatible_format(
&state_compatible_version.api_version
));
assert_eq!(State::verify_format(&state_compatible_version), Ok(()));
}

#[test]
fn utest_state_rejects_incompatible_state() {
fn utest_state_rejects_incompatible_state_on_api_version() {
let api_version = "incompatible_version".to_string();
let state_incompatible_version = State {
api_version: "incompatible_version".to_string(),
api_version: api_version.clone(),
..Default::default()
};
assert!(!State::is_compatible_format(
&state_incompatible_version.api_version
));
assert_eq!(
State::verify_format(&state_incompatible_version),
Err(format!(
"Unsupported API version. Received '{}', expected '{}'",
api_version, CURRENT_API_VERSION
))
);
}

#[test]
fn utest_state_rejects_incompatible_state_on_workload_name() {
let workload_name = "nginx.test".to_string();
let state_incompatible_version = State {
api_version: "v0.1".to_string(),
workloads: HashMap::from([(workload_name.clone(), StoredWorkloadSpec::default())]),
};
assert_eq!(
State::verify_format(&state_incompatible_version),
Err(format!(
"Unsupported workload name. Received '{}', expected to have characters in {}",
workload_name, STR_RE_WORKLOAD
))
);
}

#[test]
fn utest_state_rejects_incompatible_state_on_inordinately_long_workload_name() {
let workload_name = "workload_name_is_too_long_for_ankaios_to_accept_it_and_I_don_t_know_what_else_to_write".to_string();
let state_incompatible_version = State {
api_version: "v0.1".to_string(),
workloads: HashMap::from([(workload_name.clone(), StoredWorkloadSpec::default())]),
};
assert_eq!(
State::verify_format(&state_incompatible_version),
Err(format!(
"Workload name length {} exceeds the maximum limit of {} characters",
workload_name.len(),
MAX_CHARACTERS_WORKLOAD_NAME
))
);
}

#[test]
fn utest_state_rejects_incompatible_state_on_agent_name() {
let agent_name = "agent_A.test".to_string();
let state_incompatible_version = State {
api_version: "v0.1".to_string(),
workloads: HashMap::from([(
"sample".to_string(),
StoredWorkloadSpec {
agent: agent_name.clone(),
..Default::default()
},
)]),
};
assert_eq!(
State::verify_format(&state_incompatible_version),
Err(format!(
"Unsupported agent name. Received '{}', expected to have characters in {}",
agent_name, STR_RE_AGENT
))
);
}

#[test]
Expand Down
9 changes: 9 additions & 0 deletions common/src/state_manipulation/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,15 @@ impl Object {
}
Some(current_obj)
}

pub fn check_if_provided_path_exists(&mut self, path: &Path) -> Result<(), String> {
if self.get_as_mapping(path).is_none() {
krucod3 marked this conversation as resolved.
Show resolved Hide resolved
return Err(
"The provided path could not be found as a mapping inside the object".to_string(),
);
}
Ok(())
}
}

//////////////////////////////////////////////////////////////////////////////
Expand Down
Loading