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 <[email protected]>
openebs · Jan 26, 2024 · 715b666 · 715b666
1 parent 8acc863
commit 715b666
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 61 deletions.
diff --git 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<openapi::models::RestJsonError>,
     },
-    #[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
@@ -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::<Vec<_>>()[..] 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
@@ -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
@@ -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<Option<bool>>,
     ) -> Result<models::Node, RestError<RestJsonError>> {
-        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<models::Node, RestError<RestJsonError>> {
-        let node = client().unlabel(id.into(), label).await?;
+        let node = client().unlabel(id.into(), label_key).await?;
         Ok(node.into())
     }
 }