unit test(operator): Add comprehensive unit tests for trustee module

[6-dehan]

This commit introduces a full suite of 15 unit tests for the functions within trustee.rs, validating pure logic, error handling, and both simple and complex Kubernetes API interactions using a mocked client.

• test_get_image_pcrs_success: Verifies that a valid JSON string in a ConfigMap is correctly deserialized into the ImagePcrs struct.
• test_get_image_pcrs_no_data: Ensures an error is returned when the ConfigMap's data field is missing.
• test_get_image_pcrs_invalid_json: Confirms that an error is propagated when the data contains an invalid JSON string.
• test_generate_luks_key_returns_correct_size: A sanity check to validate that generate_luks_key runs without errors and returns a key of the expected 32-byte length.

These tests validate the idempotency and error handling of functions that perform a single create operation, primarily testing the info_if_exists! macro.

• test_create_rv_config_map_success: Verifies the function returns Ok(()) on a successful API response (200 OK).
• test_create_rv_config_map_already_exists: Verifies the function correctly handles a 409 Conflict and returns Ok(()), confirming idempotency.
• test_create_rv_config_map_generic_error: Ensures a generic API error (e.g., 500) is properly propagated as an Err.
• test_generate_resource_policy_success: Validates the success path for the generate_resource_policy function.
• test_generate_kbs_https_certificate_success: Validates the success path for the generate_kbs_https_certificate function.
• test_generate_kbs_configurations_success: Validates the success path for the generate_kbs_configurations function.
• test_generate_attestation_policy_success: Validates the success path for the generate_attestation_policy function.
• test_generate_kbs_success: Validates the success path for the generate_kbs function.

These tests use a stateful mock client to simulate entire operational flows involving multiple API calls.

• test_recompute_reference_values_flow: Verifies the complete GET (PCRs) -> GET (RV map) -> PUT (RV map) sequence executes successfully.
• test_generate_secret_flow_success: Validates the full CREATE (Secret) -> GET (KbsConfig) -> PATCH (KbsConfig) workflow for adding a new secret.
• test_generate_secret_already_present_in_spec: Tests the boundary condition where a secret ID already exists in the KbsConfig spec, ensuring the function exits early without making a redundant PATCH call.

[6-dehan]

Hi @Jakob-Naucke, could you please help review this when you're free?

$ cargo test -p operator
warning: unused import: `chrono::Utc`
 --> operator/src/reference_values.rs:2:5
  |
2 | use chrono::Utc;
  |     ^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: `operator` (bin "operator" test) generated 1 warning (run `cargo fix --bin "operator" --tests` to apply 1 suggestion)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.60s
     Running unittests src/main.rs (target/debug/deps/operator-b20e9e14510aee9a)

running 15 tests
test trustee::tests::test_create_rv_config_map_already_exists ... ok
test trustee::tests::test_create_rv_config_map_generic_error ... ok
test trustee::tests::test_create_rv_config_map_success ... ok
test trustee::tests::test_generate_attestation_policy_success ... ok
test trustee::tests::test_generate_kbs_success ... ok
test trustee::tests::test_generate_kbs_https_certificate_success ... ok
test trustee::tests::test_generate_kbs_configurations_success ... ok
test trustee::tests::test_get_image_pcrs_no_data ... ok
test trustee::tests::test_get_image_pcrs_invalid_json ... ok
test trustee::tests::test_get_image_pcrs_success ... ok
test trustee::tests::test_generate_resource_policy_success ... ok
test trustee::tests::test_recompute_reference_values_flow ... ok
test trustee::tests::test_generate_luks_key_returns_correct_size ... ok
test trustee::tests::test_generate_secret_already_present_in_spec ... ok
test trustee::tests::test_generate_secret_flow_success ... ok

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

[Jakob-Naucke]

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

on which revision did you have these test successes? the oldest one gives me

error[E0277]: the trait bound `KbsConfigSpec: Default` is not satisfied
   --> operator/src/trustee.rs:795:27
    |
795 |                         ..Default::default()
    |                           ^^^^^^^^^^^^^^^^^^ the trait `Default` is not implemented for `KbsConfigSpec`

and I think the added line in the newest revision in crds isn't supposed to be there.

Maybe it's easiest if we start from a revision where the tests passed, even if the code needs a little cleanup.

[Jakob-Naucke]

Here's a minimal diff that at least gives me 13/15 passes:

diff --git a/crds/src/lib.rs b/crds/src/lib.rs
index 3cc38f8..1d0c614 100644
--- a/crds/src/lib.rs
+++ b/crds/src/lib.rs
@@ -19 +18,0 @@ pub struct ConfidentialClusterSpec {
-#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
@@ -33 +32 @@ pub struct Trustee {
-#[derive(CustomResource, Debug, Clone, Deserialize, Serialize, JsonSchema)]
+#[derive(CustomResource, Default, Debug, Clone, Deserialize, Serialize, JsonSchema)]
diff --git a/operator/src/trustee.rs b/operator/src/trustee.rs
index 10f75e2..18b21a1 100644
--- a/operator/src/trustee.rs
+++ b/operator/src/trustee.rs
@@ -683 +683 @@ mod tests {
-            ..Default::default()
+            spec: KbsConfigSpec::default(),
@@ -791,0 +792 @@ mod tests {
+                    metadata: ObjectMeta::default(),
@@ -797 +797,0 @@ mod tests {
-                    ..Default::default()
@@ -807 +807,4 @@ mod tests {
-                let kbs_config = KbsConfig::default();
+                let kbs_config = KbsConfig {
+                    metadata: ObjectMeta::default(),
+                    spec: KbsConfigSpec::default(),
+                };
@@ -862 +865 @@ mod tests {
-                    ..Default::default()
+                    metadata: ObjectMeta::default(),

Is that a good point to start or did you mean something else?

[Jakob-Naucke]

• Can you check in your Cargo.lock too?
• The linter is failing because the code isn't formatted right. Can you run cargo fmt and update?
• Can you add a test in .github to run these tests in the CI?

Also tarpaulin (coverage tool) says || operator/src/trustee.rs: 152/169 which is pretty good 👍

[Jakob-Naucke]

This only tests that the function returns Ok(_), not what it actually sends to the API. Testing that is trickier because you would also need to instrument the mock client to allow for verifying its input. Maybe testing that is overkill -- @alicefr would like your opinion on this.

If it isn't necessary, this and other snippets like it can IMO also just be

Suggested change

	let created_cm_json = serde_json::to_string(&ConfigMap {
	metadata: ObjectMeta {
	name: Some("dummy-cm".to_string()),
	..Default::default()
	},
	..Default::default()
	})
	.unwrap();
	let created_cm_json = serde_json::to_string(&ConfigMap::default()).unwrap();

[alicefr]

Yes, I agree with Jakob. We want to test here the function logic. Like how it behave with the happy path or with errors

[6-dehan]

okay, adjusted a little bit, please help to review again. thanks

[Jakob-Naucke]

Suggested change

	} else if req.method() == Method::GET && req.uri().path().contains("test-rv-map") {
	// This is the GET request for the target RV ConfigMap
	let cm = ConfigMap::default();
	Response::builder()
	.status(StatusCode::OK)
	.body(Body::from(
	serde_json::to_string(&cm).unwrap().into_bytes(),
	))
	.unwrap()
	} else if req.method() == Method::PUT && req.uri().path().contains("test-rv-map") {
	// This is the REPLACE (PUT) request for the target RV ConfigMap
	let cm = ConfigMap::default(); // Return a success response
	Response::builder()
	.status(StatusCode::OK)
	.body(Body::from(
	serde_json::to_string(&cm).unwrap().into_bytes(),
	))
	.unwrap()
	} else {
	// For any unexpected request, return 404 Not Found
	Response::builder()
	.status(StatusCode::NOT_FOUND)
	.body(Body::empty())
	} else {
	let cm = ConfigMap::default();
	Response::builder()
	.status(StatusCode::OK)
	.body(Body::from(
	serde_json::to_string(&cm).unwrap().into_bytes(),
	))

The GET and PUT responses are exactly the same, and I don't think it's necessary to mock the 404 for other paths (not that the other tests above would do that).

[6-dehan]

changed. Please check.

[Jakob-Naucke]

can be a const with the one above

[6-dehan]

changed.

[Jakob-Naucke]

Same here. Maybe the mock clients can also be unified further? There's still a lot of duplication here.

[alicefr]

can we use the struct instead of the hardcoded json?

[6-dehan]

adjusted.

[alicefr]

can we explicitly check the error type rather then a generic error

[6-dehan]

updated. Please help review again if it works.

[alicefr]

@6-dehan comments are good, but here they are too many and it makes harder to read the code. Please, remove them

[alicefr]

this isn't correct, it always creates the client for the default namespace

[6-dehan]

Adjusted, please help to review it again.

[alicefr]

@6-dehan please always run rustfmt on all the files

[alicefr]

I think we need to create a module with a type for the client. Additionally, for the first tests the goal is to build a clean infrastructure, not to simply add thousand tests. So, as a first PR I would you to create a clean module that create the the mock client and that you target few tests with different behaviours

[alicefr]

After running rustfmt, I used this patch. I hope it can give you an idea what I mean as clean module:

From 1bbda9f9ec24d95419bb9279bc2ad9a07dd1c96b Mon Sep 17 00:00:00 2001
From: Alice Frosi <afrosi@redhat.com>
Date: Tue, 23 Sep 2025 15:34:04 +0200
Subject: [PATCH] Create MockClient

Replace the mock_client with the MockClient type

Signed-off-by: Alice Frosi <afrosi@redhat.com>
---
 operator/src/trustee.rs | 173 ++++++++++++++++++++++++++++++++++------
 1 file changed, 149 insertions(+), 24 deletions(-)

diff --git a/operator/src/trustee.rs b/operator/src/trustee.rs
index 6f9fa8a..84575fa 100644
--- a/operator/src/trustee.rs
+++ b/operator/src/trustee.rs
@@ -434,11 +434,122 @@ mod tests {
     use crds::{KbsConfig, KbsConfigSpec, Trustee};
     use http::{Method, Request, Response, StatusCode};
     use k8s_openapi::api::core::v1::{ConfigMap, Secret};
+    use kube::api::ApiResource;
     use kube::client::Body;
     use kube::error::ErrorResponse;
     use std::convert::Infallible;
     use tower::service_fn;
 
+    macro_rules! assert_kube_api_error {
+        ($err:expr, $code:expr, $reason:expr, $message:expr, $status:expr) => {{
+            let kube_error = $err
+                .downcast_ref::<kube::Error>()
+                .expect(&format!("Expected kube::Error, got: {:?}", $err));
+
+            if let kube::Error::Api(error_response) = kube_error {
+                assert_eq!(error_response.code, $code);
+                assert_eq!(error_response.reason, $reason);
+                assert_eq!(error_response.message, $message);
+                assert_eq!(error_response.status, $status);
+            } else {
+                assert!(false, "Expected kube::Error::Api, got: {:?}", kube_error);
+            }
+        }};
+    }
+    pub struct MockClient<T>
+    where
+        T: serde::Serialize + for<'de> serde::Deserialize<'de>,
+    {
+        response_data: T,
+        status_code: StatusCode,
+        namespace: String,
+    }
+    impl<T> MockClient<T>
+    where
+        T: serde::Serialize + for<'de> serde::Deserialize<'de> + Clone + Send + 'static,
+    {
+        pub fn new(status_code: StatusCode, response_data: T, namespace: String) -> Self {
+            Self {
+                response_data,
+                status_code,
+                namespace,
+            }
+        }
+        pub fn into_client(self) -> Client {
+            let response_json = serde_json::to_string(&self.response_data).unwrap();
+            let (kind, name) = serde_json::from_str::<serde_json::Value>(&response_json)
+                .map(|json_value| {
+                    let kind = json_value
+                        .get("kind")
+                        .and_then(|v| v.as_str())
+                        .unwrap_or("Unknown")
+                        .to_string();
+                    let name = json_value
+                        .get("metadata")
+                        .and_then(|m| m.get("name"))
+                        .and_then(|n| n.as_str())
+                        .unwrap_or("Unknown")
+                        .to_string();
+                    (kind, name)
+                })
+                .unwrap_or(("Unknown".to_string(), "Unknown".to_string()));
+            let plural = kind.to_lowercase() + "s";
+            let namespace = self.namespace.clone();
+
+            let mock_svc = service_fn(move |_req: Request<Body>| {
+                let body = if self.status_code == StatusCode::OK {
+                    let response_json = serde_json::to_string(&self.response_data).unwrap();
+                    Body::from(response_json.into_bytes())
+                } else {
+                    let error_response = match self.status_code {
+                        StatusCode::CONFLICT => ErrorResponse {
+                            status: "Failure".to_string(),
+                            message: format!("{plural} \"{name}\" already exists"),
+                            reason: "AlreadyExists".to_string(),
+                            code: 409,
+                        },
+                        StatusCode::INTERNAL_SERVER_ERROR => ErrorResponse {
+                            status: "Failure".to_string(),
+                            message: "internal server error".to_string(),
+                            reason: "ServerTimeout".to_string(),
+                            code: 500,
+                        },
+                        StatusCode::NOT_FOUND => ErrorResponse {
+                            status: "Failure".to_string(),
+                            message: "resource not found".to_string(),
+                            reason: "NotFound".to_string(),
+                            code: 404,
+                        },
+                        StatusCode::BAD_REQUEST => ErrorResponse {
+                            status: "Failure".to_string(),
+                            message: "bad request".to_string(),
+                            reason: "BadRequest".to_string(),
+                            code: 400,
+                        },
+                        _ => ErrorResponse {
+                            status: "Failure".to_string(),
+                            message: format!(
+                                "error with status code {}",
+                                self.status_code.as_u16()
+                            ),
+                            reason: "Unknown".to_string(),
+                            code: self.status_code.as_u16(),
+                        },
+                    };
+                    let error_json = serde_json::to_string(&error_response).unwrap();
+                    Body::from(error_json.into_bytes())
+                };
+
+                let response = Response::builder()
+                    .status(self.status_code)
+                    .body(body)
+                    .unwrap();
+                async move { Ok::<_, Infallible>(response) }
+            });
+            Client::new(mock_svc, namespace)
+        }
+    }
+
     // -----------------------------------------------------------------
     // Core helper functions for mocking the K8s API Server
     // -----------------------------------------------------------------
@@ -549,46 +660,60 @@ mod tests {
     // --- Tests for `create_reference_value_config_map` ---
     #[tokio::test]
     async fn test_create_rv_config_map_success() {
-        // 1. Prepare mock response: K8s API usually returns 200 OK with the created object on success.
-        let created_cm_json = serde_json::to_string(&ConfigMap {
+        let ns = "test".to_string();
+        let config = ConfigMap {
             metadata: ObjectMeta {
                 name: Some("test-rv-map".to_string()),
+                namespace: Some(ns.clone()),
                 ..Default::default()
             },
             ..Default::default()
-        })
-        .unwrap();
-
-        // 2. Create the mock client
-        let client = mock_client(StatusCode::OK, created_cm_json.into_bytes());
-
-        // 3. Call the function under test
+        };
+        let client = MockClient::new(StatusCode::OK, config, ns).into_client();
         let result = create_reference_value_config_map(client, "test-ns", "test-rv-map").await;
-
-        // 4. Assert the result: We expect the function to complete successfully without any errors.
         assert!(result.is_ok());
     }
 
     #[tokio::test]
     async fn test_create_rv_config_map_already_exists() {
-        // 1. Prepare the JSON body for a K8s API error response
-        let error_response = ErrorResponse {
-            status: "Failure".to_string(),
-            message: "configmaps \"test-rv-map\" already exists".to_string(),
-            reason: "AlreadyExists".to_string(),
-            code: 409,
+        let ns = "test".to_string();
+        let config = ConfigMap {
+            metadata: ObjectMeta {
+                name: Some("test-rv-map".to_string()),
+                namespace: Some(ns.clone()),
+                ..Default::default()
+            },
+            ..Default::default()
         };
-        let error_body = serde_json::to_string(&error_response).unwrap();
+        let client = MockClient::new(StatusCode::CONFLICT, config, ns).into_client();
+        let result = create_reference_value_config_map(client, "test-ns", "test-rv-map").await;
 
-        // 2. Create a mock client that returns 409 Conflict
-        let client = mock_client(StatusCode::CONFLICT, error_body.into_bytes());
+        assert!(result.is_ok());
+    }
+
+    #[tokio::test]
+    async fn test_create_rv_config_error() {
+        let ns = "test".to_string();
+        let config = ConfigMap {
+            metadata: ObjectMeta {
+                name: Some("test-rv-map".to_string()),
+                namespace: Some(ns.clone()),
+                ..Default::default()
+            },
+            ..Default::default()
+        };
+        let client = MockClient::new(StatusCode::INTERNAL_SERVER_ERROR, config, ns).into_client();
 
-        // 3. Call the function under test
         let result = create_reference_value_config_map(client, "test-ns", "test-rv-map").await;
 
-        // 4. Assert the result: Because the `info_if_exists!` macro catches the 409 error
-        // and treats it as non-fatal, we expect the function to still return Ok(()).
-        assert!(result.is_ok());
+        let err = result.unwrap_err();
+        assert_kube_api_error!(
+            err,
+            500,
+            "ServerTimeout",
+            "internal server error",
+            "Failure"
+        );
     }
 
     #[tokio::test]
-- 
2.51.0

This is a clean type that we can enhance for different behaviors. Although, the patches is targeting the trustee.rs, this functions and macro should be put into a separate directory and reused in the unit tests for trustee

[alicefr]

@6-dehan please close this PR in favor of #39, so we don't review twice the same code