From 715b66603b8283306d8c7717b7d4c3014f921daa Mon Sep 17 00:00:00 2001 From: Tiago Castro Date: Fri, 26 Jan 2024 20:19:07 +0000 Subject: [PATCH] refactor(node-label/api): doc the cli and openapi with key val Expands the parsing of label on the cli and returns different error messages. Splits the label into key and value. todo: is this the right approach here, where should we do the validation? kubectl seems to some validation, is there anything on the apiserver too? Signed-off-by: Tiago Castro --- control-plane/plugin/src/resources/error.rs | 6 +- control-plane/plugin/src/resources/node.rs | 117 +++++++++++++----- .../rest/openapi-specs/v0_api_spec.yaml | 18 ++- control-plane/rest/service/src/v0/nodes.rs | 25 +--- 4 files changed, 105 insertions(+), 61 deletions(-) diff --git a/control-plane/plugin/src/resources/error.rs b/control-plane/plugin/src/resources/error.rs index 932df628e..d76b730a5 100644 --- a/control-plane/plugin/src/resources/error.rs +++ b/control-plane/plugin/src/resources/error.rs @@ -25,12 +25,12 @@ pub enum Error { }, /// Error when node label request fails. #[snafu(display("Failed to get node {id}. Error {source}"))] - NodeLabelError { + NodeLabelRequest { id: String, source: openapi::tower::client::Error, }, - #[snafu(display("Empty node label {id}."))] - EmptyNodeLabelError { id: String }, + #[snafu(display("Invalid node label: {error}"))] + NodeLabelFormat { error: String }, /// Error when node uncordon request fails. #[snafu(display("Failed to uncordon node {id}. Error {source}"))] NodeUncordonError { diff --git a/control-plane/plugin/src/resources/node.rs b/control-plane/plugin/src/resources/node.rs index 3172dd9e2..03749abe4 100644 --- a/control-plane/plugin/src/resources/node.rs +++ b/control-plane/plugin/src/resources/node.rs @@ -555,6 +555,49 @@ impl Drain for Node { } } +fn allowed_topology_chars(key: char) -> bool { + key.is_ascii_alphanumeric() || matches!(key, '_' | '-' | '.') +} +fn validate_topology_key(key: &str) -> PluginResult { + if key.is_empty() { + return Err(Error::NodeLabelFormat { + error: "key must not be empty".to_string(), + }); + } + if key.starts_with('/') || key.ends_with('/') { + return Err(Error::NodeLabelFormat { + error: "/ can only be used in the middle of the key".to_string(), + }); + } + + if key.chars().filter(|c| c == &'/').count() > 1 { + return Err(Error::NodeLabelFormat { + error: "/ is only allowed once".to_string(), + }); + } + + if !key.chars().all(|c| allowed_topology_chars(c) || c == '/') { + return Err(Error::NodeLabelFormat { + error: "only ascii alphanumeric characters are allowed".to_string(), + }); + } + + Ok(()) +} +fn validate_topology_value(value: &str) -> PluginResult { + if value.is_empty() { + return Err(Error::NodeLabelFormat { + error: "value cannot be empty".to_string(), + }); + } + if !value.chars().all(allowed_topology_chars) { + return Err(Error::NodeLabelFormat { + error: "contains non ascii alphanumeric characters".to_string(), + }); + } + Ok(()) +} + #[async_trait(?Send)] impl Label for Node { type ID = NodeId; @@ -564,43 +607,49 @@ impl Label for Node { overwrite: bool, output: &utils::OutputFormat, ) -> PluginResult { - let result = match label.chars().last() { - Some(last_char) => { - // If the last character is a hyphen the it signifies that the lable needs to be - // removed. - if last_char == '-' { - RestClient::client() - .nodes_api() - .delete_node_label(id, &label[.. label.len() - 1]) - .await - } else { - RestClient::client() - .nodes_api() - .put_node_label(id, &label, Some(overwrite)) - .await - } - } - None => { - return Err(Error::EmptyNodeLabelError { id: id.to_string() }); + let result = if label.contains('=') { + let [key, value] = label.split('=').collect::>()[..] else { + return Err(Error::NodeLabelFormat { + error: "invalid format, cannot contain multiple assignments".to_string(), + }); + }; + + validate_topology_key(key)?; + validate_topology_value(value)?; + RestClient::client() + .nodes_api() + .put_node_label(id, key, value, Some(overwrite)) + .await + } else { + if label.len() < 2 || !label.ends_with('-') { + return Err(Error::NodeLabelFormat { + error: "At least one label value is required".to_string(), + }); } + let key = &label[.. label.len() - 1]; + validate_topology_key(key)?; + RestClient::client() + .nodes_api() + .delete_node_label(id, key) + .await }; - match result { - Ok(node) => match output { - OutputFormat::Yaml | OutputFormat::Json => { - // Print json or yaml based on output format. - utils::print_table(output, node.into_body()); - } - OutputFormat::None => { - // In case the output format is not specified, show a success message. - println!("Node {id} labelled successfully") - } - }, - Err(e) => { - return Err(Error::NodeLabelError { - id: id.to_string(), - source: e, - }); + let node = result + .map_err(|source| Error::NodeLabelRequest { + id: id.to_string(), + source, + })? + .into_body(); + + match output { + OutputFormat::Yaml | OutputFormat::Json => { + // Print json or yaml based on output format. + print_table(output, node); + } + OutputFormat::None => { + // In case the output format is not specified, show a success message. + let labels = node.spec.unwrap().labels.unwrap(); + println!("Node {id} labelled successfully. Current labels: {labels:?}"); } } Ok(()) diff --git a/control-plane/rest/openapi-specs/v0_api_spec.yaml b/control-plane/rest/openapi-specs/v0_api_spec.yaml index a4938096d..c8dcd1fad 100644 --- a/control-plane/rest/openapi-specs/v0_api_spec.yaml +++ b/control-plane/rest/openapi-specs/v0_api_spec.yaml @@ -319,7 +319,7 @@ paths: $ref: '#/components/responses/ServerError' security: - JWT: [] - '/nodes/{id}/label/{label}': + '/nodes/{id}/label/{key}={value}': put: tags: - Nodes @@ -334,12 +334,19 @@ paths: type: string example: io-engine-1 - in: path - name: label + name: key required: true schema: type: string description: |- - The label to the node. + The key of the label to be added. + - in: path + name: value + required: true + schema: + type: string + description: |- + The value of the label to be added. - in: query name: overwrite description: |- @@ -361,6 +368,7 @@ paths: $ref: '#/components/responses/ServerError' security: - JWT: [] + '/nodes/{id}/label/{key}': delete: tags: - Nodes @@ -373,12 +381,12 @@ paths: type: string example: io-engine-1 - in: path - name: label + name: key required: true schema: type: string description: |- - The key of the label to the node. + The key of the label to be removed. responses: '200': description: OK diff --git a/control-plane/rest/service/src/v0/nodes.rs b/control-plane/rest/service/src/v0/nodes.rs index 4cb6fbe70..f1c60c92a 100644 --- a/control-plane/rest/service/src/v0/nodes.rs +++ b/control-plane/rest/service/src/v0/nodes.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use super::*; use grpc::operations::node::traits::NodeOperations; @@ -60,31 +58,20 @@ impl apis::actix_server::Nodes for RestApi { } async fn put_node_label( - Path((id, label)): Path<(String, String)>, + Path((id, key, value)): Path<(String, String, String)>, Query(overwrite): Query>, ) -> Result> { - let mut label_map = HashMap::new(); - let mut iter = label.split('='); let overwrite = overwrite.unwrap_or(false); - - if let (Some(key), Some(value)) = (iter.next(), iter.next()) { - label_map.insert(key.trim().to_string(), value.trim().to_string()); - } else { - return Err(RestError::from(ReplyError { - kind: ReplyErrorKind::Internal, - resource: ResourceKind::Node, - source: "put_node_label".to_string(), - extra: format!("invalid label for node resource {}", label), - })); - } - let node = client().label(id.into(), label_map, overwrite).await?; + let node = client() + .label(id.into(), [(key, value)].into(), overwrite) + .await?; Ok(node.into()) } async fn delete_node_label( - Path((id, label)): Path<(String, String)>, + Path((id, label_key)): Path<(String, String)>, ) -> Result> { - let node = client().unlabel(id.into(), label).await?; + let node = client().unlabel(id.into(), label_key).await?; Ok(node.into()) } }