Merge pull request #20 from zvonkok/kata-cdi

implemented correct k8s validation

Cargo.toml

@@ -28,19 +28,21 @@ path = "src/bin/validate.rs" # Path to the source file
 [dependencies]
 oci-spec = { git = "https://github.com/containers/oci-spec-rs.git", rev = "d338cf8", features = ["runtime"] }
 anyhow = "1.0"
-clap = "4.5.3"
+clap = "4.5.4"
 serde_json = "1.0"
 jsonschema = "0.18.0"
 once_cell = "1.8.0"
 notify = "6.1.1"
 serde = "1.0.131"
 serde_derive = "1.0.131"
 libc = "0.2.112"
-nix = "0.29.0"
+nix = "0.24.2"
 lazy_static = "1.2"
 path-clean = "1.0.1"
 serde_yaml = "0.9.33"
-semver = "1.0.23"
+semver = "1.0.22"
+regex = "1.5.4"
+const_format = "0.2.32"
 
 [dev-dependencies]
 tempfile = "3.1.0"

src/annotations.rs

@@ -41,8 +41,8 @@ pub(crate) fn update_annotations(
 // are returned along with a non-nil error. The annotations are expected
 // to be formatted by, or in a compatible fashion to UpdateAnnotations().
 #[allow(dead_code)]
-pub(crate) fn parse_annotations(
-    annotations: HashMap<String, String>,
+pub fn parse_annotations(
+    annotations: &HashMap<String, String>,
 ) -> Result<(Vec<String>, Vec<String>)> {
     let mut keys: Vec<String> = Vec::new();
     let mut devices: Vec<String> = Vec::new();
@@ -58,7 +58,7 @@ pub(crate) fn parse_annotations(
             }
             devices.push(device.to_string());
         }
-        keys.push(k);
+        keys.push(k.to_string());
     }
 
     Ok((keys, devices))
@@ -169,8 +169,8 @@ mod tests {
                 .to_string(),
         );
 
-        assert!(parse_annotations(cdi_devices.clone()).is_ok());
-        let (keys, devices) = parse_annotations(cdi_devices).unwrap();
+        assert!(parse_annotations(&cdi_devices).is_ok());
+        let (keys, devices) = parse_annotations(&cdi_devices).unwrap();
         assert_eq!(keys.len(), 3);
         assert_eq!(devices.len(), 3);
     }

src/device.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeMap, HashMap};
+use std::collections::BTreeMap;
 
 use anyhow::{anyhow, Context, Result};
 use oci_spec::runtime as oci;
@@ -8,7 +8,7 @@ use crate::{
     parser::{qualified_name, validate_device_name},
     spec::Spec,
     specs::config::Device as CDIDevice,
-    validations::validate_spec_annotations,
+    internal::validation::validate::validate_spec_annotations,
 };
 
 // Device represents a CDI device of a Spec.
@@ -82,7 +82,7 @@ impl Device {
         validate_device_name(&self.cdi_device.name).context("validate device name failed")?;
         let name = self.get_qualified_name();
 
-        let annotations: &HashMap<String, String> =
+        let annotations: &BTreeMap<String, String> =
             &<BTreeMap<String, String> as Clone>::clone(&self.cdi_device.annotations)
                 .into_iter()
                 .collect();

src/internal/validation/k8s/objectmeta.rs

@@ -1,7 +1,105 @@
-use anyhow::Result;
+use anyhow::{Error, Result};
 use std::collections::BTreeMap;
 
-pub fn validate_annotations(_annotations: &BTreeMap<String, String>, _path: &str) -> Result<()> {
-    // Implement the actual validation logic or import the function from your k8s module
-    Ok(())
+const TOTAL_ANNOTATION_SIZE_LIMIT: usize = 256 * 1024; // 256 kB
+
+use super::validation::is_qualified_name;
+
+pub fn validate_annotations(annotations: &BTreeMap<String, String>, path: &str) -> Result<()> {
+    let mut errs = Vec::new();
+
+    for k in annotations.keys() {
+        for msg in is_qualified_name(&k.to_lowercase()) {
+            errs.push(format!("{}.{} is invalid: {}", path, k, msg));
+        }
+    }
+
+    if let Err(err) = validate_annotations_size(annotations) {
+        errs.push(format!("{} is too long: {}", path, err));
+    }
+
+    if errs.is_empty() {
+        Ok(())
+    } else {
+        Err(Error::msg(errs.join(", ")))
+    }
+}
+
+fn validate_annotations_size(annotations: &BTreeMap<String, String>) -> Result<()> {
+    let total_size: usize = annotations.iter().map(|(k, v)| k.len() + v.len()).sum();
+
+    if total_size > TOTAL_ANNOTATION_SIZE_LIMIT {
+        Err(Error::msg(format!(
+            "annotations size {} is larger than limit {}",
+            total_size, TOTAL_ANNOTATION_SIZE_LIMIT
+        )))
+    } else {
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::super::super::validate::validate_spec_annotations;
+    use super::validate_annotations;
+
+    use std::collections::BTreeMap;
+
+    #[test]
+    fn test_validate_annotations() {
+        let mut annotations = BTreeMap::new();
+        annotations.insert(
+            "cdi.k8s.io/vfio17".to_string(),
+            "nvidia.com/gpu=0".to_string(),
+        );
+        annotations.insert(
+            "cdi.k8s.io/vfio18".to_string(),
+            "nvidia.com/gpu=1".to_string(),
+        );
+        annotations.insert(
+            "cdi.k8s.io/vfio19".to_string(),
+            "nvidia.com/gpu=all".to_string(),
+        );
+        let path = "test.annotations";
+
+        assert!(validate_annotations(&annotations, path).is_ok());
+
+        let mut large_annotations = BTreeMap::new();
+        let long_value = "CDI".repeat(super::TOTAL_ANNOTATION_SIZE_LIMIT + 1);
+        large_annotations.insert("CDIKEY".to_string(), long_value);
+        assert!(validate_annotations(&large_annotations, path).is_err());
+
+        let mut invalid_annotations = BTreeMap::new();
+        invalid_annotations.insert("inv$$alid_CDIKEY".to_string(), "inv$$alid_CDIVAL".to_string());
+        assert!(validate_annotations(&invalid_annotations, path).is_err());
+    }
+
+    #[test]
+    fn test_validate_spec_annotations() {
+        let mut annotations = BTreeMap::new();
+        annotations.insert(
+            "cdi.k8s.io/vfio17".to_string(),
+            "nvidia.com/gpu=0".to_string(),
+        );
+        annotations.insert(
+            "cdi.k8s.io/vfio18".to_string(),
+            "nvidia.com/gpu=1".to_string(),
+        );
+        annotations.insert(
+            "cdi.k8s.io/vfio19".to_string(),
+            "nvidia.com/gpu=all".to_string(),
+        );
+
+        assert!(validate_spec_annotations("", &annotations).is_ok());
+        assert!(validate_spec_annotations("CDITEST", &annotations).is_ok());
+
+        let mut large_annotations = BTreeMap::new();
+        let long_value = "CDI".repeat(super::TOTAL_ANNOTATION_SIZE_LIMIT + 1);
+        large_annotations.insert("CDIKEY".to_string(), long_value);
+        assert!(validate_spec_annotations("", &large_annotations).is_err());
+
+        let mut invalid_annotations = BTreeMap::new();
+        invalid_annotations.insert("inva$$lid_CDIKEY".to_string(), "inval$$id_CDIVAL".to_string());
+        assert!(validate_spec_annotations("CDITEST", &invalid_annotations).is_err());
+    }
 }

src/internal/validation/k8s/validation.rs

@@ -1 +1,208 @@
+use lazy_static::lazy_static;
+use regex::Regex;
+use const_format::concatcp;
 
+const QNAME_CHAR_FMT: &str = "[A-Za-z0-9]";
+const QNAME_EXT_CHAR_FMT: &str = "[-A-Za-z0-9_.]";
+const QUALIFIED_NAME_FMT: &str = concatcp!("(", QNAME_CHAR_FMT, QNAME_EXT_CHAR_FMT, "*)?", QNAME_CHAR_FMT);
+const QUALIFIED_NAME_ERR_MSG: &str = "must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character";
+const QUALIFIED_NAME_MAX_LENGTH: usize = 63;
+
+const LABEL_VALUE_FMT: &str = concatcp!("(", QUALIFIED_NAME_FMT, ")?");
+const LABEL_VALUE_ERR_MSG: &str = "a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character";
+const LABEL_VALUE_MAX_LENGTH: usize = 63;
+
+const DNS1123_LABEL_FMT: &str = "[a-z0-9]([-a-z0-9]*[a-z0-9])?";
+const DNS1123_LABEL_ERR_MSG: &str = "a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character";
+const DNS1123_LABEL_MAX_LENGTH: usize = 63;
+
+const DNS1123_SUBDOMAIN_FMT: &str = concatcp!(DNS1123_LABEL_FMT, "(\\.", DNS1123_LABEL_FMT, ")*");
+const DNS1123_SUBDOMAIN_ERR_MSG: &str = "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character";
+const DNS1123_SUBDOMAIN_MAX_LENGTH: usize = 253;
+
+const DNS1035_LABEL_FMT: &str = "[a-z]([-a-z0-9]*[a-z0-9])?";
+const DNS1035_LABEL_ERR_MSG: &str = "a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character";
+const DNS1035_LABEL_MAX_LENGTH: usize = 63;
+
+const WILDCARD_DNS1123_SUBDOMAIN_FMT: &str = concatcp!("\\*\\.", DNS1123_SUBDOMAIN_FMT);
+const WILDCARD_DNS1123_SUBDOMAIN_ERR_MSG: &str = "a wildcard DNS-1123 subdomain must start with '*.', followed by a valid DNS subdomain, which must consist of lower case alphanumeric characters, '-' or '.' and end with an alphanumeric character";
+
+lazy_static! {
+    static ref QUALIFIED_NAME_REGEXP: Regex =
+        Regex::new(&format!("^{}$", QUALIFIED_NAME_FMT)).unwrap();
+    static ref LABEL_VALUE_REGEXP: Regex = Regex::new(&format!("^{}$", LABEL_VALUE_FMT)).unwrap();
+    static ref DNS1123_LABEL_REGEXP: Regex =
+        Regex::new(&format!("^{}$", DNS1123_LABEL_FMT)).unwrap();
+    static ref DNS1123_SUBDOMAIN_REGEXP: Regex =
+        Regex::new(&format!("^{}$", DNS1123_SUBDOMAIN_FMT)).unwrap();
+    static ref DNS1035_LABEL_REGEXP: Regex =
+        Regex::new(&format!("^{}$", DNS1035_LABEL_FMT)).unwrap();
+    static ref WILDCARD_DNS1123_SUBDOMAIN_REGEXP: Regex =
+        Regex::new(&format!("^{}$", WILDCARD_DNS1123_SUBDOMAIN_FMT)).unwrap();
+}
+
+pub fn is_qualified_name(value: &str) -> Vec<String> {
+    let mut errs = Vec::new();
+    let parts: Vec<&str> = value.split('/').collect();
+    let name: &str;
+
+    match parts.len() {
+        1 => {
+            name = parts[0];
+        }
+        2 => {
+            let prefix = parts[0];
+            name = parts[1];
+            if prefix.is_empty() {
+                errs.push(format!("prefix part {}", empty_error()));
+            } else {
+                let msgs = is_dns1123_subdomain(prefix);
+                if msgs.is_empty() {
+                    errs.extend(prefix_each(&msgs, "prefix part "));
+                }
+            }
+        }
+        _ => {
+            return vec![format!(
+			"a qualified name {} with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')",
+			regex_error(QUALIFIED_NAME_ERR_MSG, QUALIFIED_NAME_FMT, &["MyName", "my.name", "123-abc"])
+		    )];
+        }
+    }
+
+    if name.is_empty() {
+        errs.push(format!("name part {}", empty_error()));
+    } else if name.len() > QUALIFIED_NAME_MAX_LENGTH {
+        errs.push(format!(
+            "name part {}",
+            max_len_error(QUALIFIED_NAME_MAX_LENGTH)
+        ));
+    }
+
+    if !QUALIFIED_NAME_REGEXP.is_match(name) {
+        errs.push(format!(
+            "name part {}",
+            regex_error(
+                QUALIFIED_NAME_ERR_MSG,
+                QUALIFIED_NAME_FMT,
+                &["MyName", "my.name", "123-abc"]
+            )
+        ));
+    }
+
+    errs
+}
+
+#[allow(dead_code)]
+fn is_valid_label_value(value: &str) -> Vec<String> {
+    let mut errs = Vec::new();
+    if value.len() > LABEL_VALUE_MAX_LENGTH {
+        errs.push(max_len_error(LABEL_VALUE_MAX_LENGTH));
+    }
+    if !LABEL_VALUE_REGEXP.is_match(value) {
+        errs.push(regex_error(
+            LABEL_VALUE_ERR_MSG,
+            LABEL_VALUE_FMT,
+            &["MyValue", "my_value", "12345"],
+        ));
+    }
+    errs
+}
+
+#[allow(dead_code)]
+fn is_dns1123_label(value: &str) -> Vec<String> {
+    let mut errs = Vec::new();
+    if value.len() > DNS1123_LABEL_MAX_LENGTH {
+        errs.push(max_len_error(DNS1123_LABEL_MAX_LENGTH));
+    }
+    if !DNS1123_LABEL_REGEXP.is_match(value) {
+        errs.push(regex_error(
+            DNS1123_LABEL_ERR_MSG,
+            DNS1123_LABEL_FMT,
+            &["my-name", "123-abc"],
+        ));
+    }
+    errs
+}
+
+fn is_dns1123_subdomain(value: &str) -> Vec<String> {
+    let mut errs = Vec::new();
+    if value.len() > DNS1123_SUBDOMAIN_MAX_LENGTH {
+        errs.push(max_len_error(DNS1123_SUBDOMAIN_MAX_LENGTH));
+    }
+    if !DNS1123_SUBDOMAIN_REGEXP.is_match(value) {
+        errs.push(regex_error(
+            DNS1123_SUBDOMAIN_ERR_MSG,
+            DNS1123_SUBDOMAIN_FMT,
+            &["example.com"],
+        ));
+    }
+    errs
+}
+
+#[allow(dead_code)]
+fn is_dns1035_label(value: &str) -> Vec<String> {
+    let mut errs = Vec::new();
+    if value.len() > DNS1035_LABEL_MAX_LENGTH {
+        errs.push(max_len_error(DNS1035_LABEL_MAX_LENGTH));
+    }
+    if !DNS1035_LABEL_REGEXP.is_match(value) {
+        errs.push(regex_error(
+            DNS1035_LABEL_ERR_MSG,
+            DNS1035_LABEL_FMT,
+            &["my-name", "abc-123"],
+        ));
+    }
+    errs
+}
+
+#[allow(dead_code)]
+fn is_wildcard_dns1123_subdomain(value: &str) -> Vec<String> {
+    let mut errs = Vec::new();
+    if value.len() > DNS1123_SUBDOMAIN_MAX_LENGTH {
+        errs.push(max_len_error(DNS1123_SUBDOMAIN_MAX_LENGTH));
+    }
+    if !WILDCARD_DNS1123_SUBDOMAIN_REGEXP.is_match(value) {
+        errs.push(regex_error(
+            WILDCARD_DNS1123_SUBDOMAIN_ERR_MSG,
+            WILDCARD_DNS1123_SUBDOMAIN_FMT,
+            &["*.example.com"],
+        ));
+    }
+    errs
+}
+
+fn max_len_error(max_length: usize) -> String {
+    format!("must be no more than {} characters", max_length)
+}
+
+fn regex_error(err_msg: &str, regex_fmt: &str, examples: &[&str]) -> String {
+    if examples.is_empty() {
+        format!("{} (regex used for validation is '{}')", err_msg, regex_fmt)
+    } else {
+        let examples_str = examples
+            .iter()
+            .map(|&e| format!("'{}'", e))
+            .collect::<Vec<String>>()
+            .join(" or ");
+        format!(
+            "{} (e.g. {}. regex used for validation is '{}')",
+            err_msg, examples_str, regex_fmt
+        )
+    }
+}
+
+fn empty_error() -> &'static str {
+    "must be non-empty"
+}
+
+fn prefix_each(msgs: &[String], prefix: &str) -> Vec<String> {
+    msgs.iter()
+        .map(|msg| format!("{}{}", prefix, msg))
+        .collect()
+}
+
+#[allow(dead_code)]
+fn inclusive_range_error(lo: usize, hi: usize) -> String {
+    format!("must be between {} and {}, inclusive", lo, hi)
+}