From 8d9f22a91b495e78bc688baa6a569dfcd9cd6b32 Mon Sep 17 00:00:00 2001 From: Chirag Rao <73184157+SpaceFace02@users.noreply.github.com> Date: Wed, 20 May 2026 15:45:25 +0530 Subject: [PATCH 1/4] tests: README.md update for more context around testing env variables --- tests/README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/README.md b/tests/README.md index 69ce205b..89668729 100644 --- a/tests/README.md +++ b/tests/README.md @@ -22,7 +22,10 @@ make install-kubevirt make integration-tests ``` -Each test can also be run independently using cargo test. Example: +Each test can also be run independently using cargo test. \ +Before running independent tests, run `make trusted-cluster-gen` to create the default boiler-plate yaml manifests for the TEC and make sure `TRUSTEE_IMAGE`, `APPROVED_IMAGE`, `TEST_IMAGE`, `RUST_LOG`, `REGISTRY`, `TAG` env variables are set. + +Example: ```bash $ cargo test test_trusted_execution_cluster_uninstall -- --no-capture ``` From 8a7e76d5a03daf658c9240ec1c6bdc108e4bbdd8 Mon Sep 17 00:00:00 2001 From: Chirag Rao <73184157+SpaceFace02@users.noreply.github.com> Date: Mon, 25 May 2026 12:28:44 +0530 Subject: [PATCH 2/4] feat: if condition exists, updating it, otherwise inserting it to the conditions list in the CRD, in the main reconcile function of operator, attestation key register ak reconcile and reference image_add_reconcile Fix: #220 --- operator/src/attestation_key_register.rs | 32 +-- operator/src/lib.rs | 45 +++++ operator/src/main.rs | 242 +++++++++++++++++++++-- operator/src/reference_values.rs | 21 +- test_utils/src/mock_client.rs | 16 +- 5 files changed, 299 insertions(+), 57 deletions(-) diff --git a/operator/src/attestation_key_register.rs b/operator/src/attestation_key_register.rs index 62083954..917d37d1 100644 --- a/operator/src/attestation_key_register.rs +++ b/operator/src/attestation_key_register.rs @@ -30,6 +30,7 @@ use crate::conditions::attestation_key_approved_condition; use crate::trustee; use operator::{ ControllerError, TLS_DIR, controller_error_policy, create_or_info_if_exists, read_certificate, + upsert_condition, }; const INTERNAL_ATTESTATION_KEY_REGISTER_PORT: i32 = 8001; @@ -200,31 +201,14 @@ async fn approve_ak(ak: &AttestationKey, machine: &Machine, client: Client) -> R let name = ak.metadata.name.clone().unwrap_or_default(); let aks: Api = Api::default_namespaced(client.clone()); - let is_approved = ak - .status - .as_ref() - .and_then(|s| s.conditions.as_ref()) - .map(|conditions| { - conditions - .iter() - .any(|c| c.type_ == "Approved" && c.status == "True") - }) - .unwrap_or(false); + let generation = ak.metadata.generation; + let approve_reason = ATTESTATION_KEY_MACHINE_APPROVE; + let condition = attestation_key_approved_condition(approve_reason, generation, &ak.status); + let mut conditions = ak.status.as_ref().and_then(|s| s.conditions.clone()); + let changed = upsert_condition(&mut conditions, condition); - if !is_approved { - let generation = ak.metadata.generation; - let approve_reason = ATTESTATION_KEY_MACHINE_APPROVE; - let condition = attestation_key_approved_condition(approve_reason, generation, &ak.status); - let mut conditions = ak - .status - .as_ref() - .and_then(|s| s.conditions.clone()) - .unwrap_or_default(); - conditions.push(condition); - - let status = AttestationKeyStatus { - conditions: Some(conditions), - }; + if changed { + let status = AttestationKeyStatus { conditions }; update_status!(aks, &name, status)?; info!("Approved attestation key {name}"); } diff --git a/operator/src/lib.rs b/operator/src/lib.rs index 3dcb0968..c5db0fd3 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -11,6 +11,8 @@ use anyhow::Result; use k8s_openapi::api::core::v1::{Secret, SecretVolumeSource, Volume, VolumeMount}; use k8s_openapi::apimachinery::pkg::apis::meta::v1::OwnerReference; +use k8s_openapi::apimachinery::pkg::apis::meta::v1::{Condition, Time}; +use k8s_openapi::jiff::Timestamp; use kube::{Api, Client, runtime::controller::Action}; use log::{info, warn}; use std::fmt::{Debug, Display}; @@ -94,3 +96,46 @@ pub async fn read_certificate( }; Ok(Some((volume, volume_mount))) } + +// TODO: Port this functionality to kube-rs API. +// Update condition if already present, otherwise append(insert) it into the conditions vector. +pub fn upsert_condition( + existing_conditions: &mut Option>, + new_condition: Condition, +) -> bool { + let conditions_vec = existing_conditions.get_or_insert_with(Vec::new); + + if let Some(existing) = conditions_vec + .iter_mut() + .find(|c| c.type_ == new_condition.type_) + { + let mut changed = false; + + // Being faithful to kubernetes API semantics, only update transition time if status changes. + if existing.status != new_condition.status { + existing.status = new_condition.status; + existing.last_transition_time = Time(Timestamp::now()); + changed = true; + } + + if existing.reason != new_condition.reason { + existing.reason = new_condition.reason; + changed = true; + } + + if existing.message != new_condition.message { + existing.message = new_condition.message; + changed = true; + } + + if existing.observed_generation != new_condition.observed_generation { + existing.observed_generation = new_condition.observed_generation; + changed = true; + } + + changed + } else { + conditions_vec.push(new_condition); + true + } +} diff --git a/operator/src/main.rs b/operator/src/main.rs index af331f3e..05b48682 100644 --- a/operator/src/main.rs +++ b/operator/src/main.rs @@ -16,7 +16,7 @@ use kube::runtime::watcher; use kube::{Api, Client}; use log::{error, info, warn}; -use operator::generate_owner_reference; +use operator::{generate_owner_reference, upsert_condition}; use trusted_cluster_operator_lib::{TrustedExecutionCluster, TrustedExecutionClusterStatus}; use trusted_cluster_operator_lib::{conditions::*, update_status}; @@ -107,7 +107,11 @@ async fn reconcile( let existing_status = &cluster.status; let address_condition = known_trustee_address_condition(known_address, generation, existing_status); - let mut conditions = Some(vec![address_condition]); + + // Get existing conditions or default to empty vector + let mut conditions = existing_status.as_ref().and_then(|s| s.conditions.clone()); + // Update or insert address condition to prevent rebuilding the status object from scratch every time the reconcile is called. + let _ = upsert_condition(&mut conditions, address_condition); let kube_client = ctx.client.clone(); let err = "trusted execution cluster had no name"; @@ -117,9 +121,12 @@ async fn reconcile( if cluster.metadata.deletion_timestamp.is_some() { info!("Registered deletion of TrustedExecutionCluster {name}"); let uninstalling_reason = NOT_INSTALLED_REASON_UNINSTALLING; - let condition = installed_condition(uninstalling_reason, generation, existing_status); - conditions.as_mut().unwrap().push(condition); - update_status!(clusters, name, TrustedExecutionClusterStatus { conditions })?; + let uninstall_condition = + installed_condition(uninstalling_reason, generation, existing_status); + let changed = upsert_condition(&mut conditions, uninstall_condition); + if changed { + update_status!(clusters, name, TrustedExecutionClusterStatus { conditions })?; + } return Ok(Action::await_change()); } @@ -137,29 +144,35 @@ async fn reconcile( "More than one TrustedExecutionCluster found in namespace {namespace}. \ trusted-cluster-operator does not support more than one TrustedExecutionCluster. Requeueing...", ); - let condition = + let non_unique_condition = installed_condition(NOT_INSTALLED_REASON_NON_UNIQUE, generation, existing_status); - conditions.as_mut().unwrap().push(condition); - update_status!(clusters, name, TrustedExecutionClusterStatus { conditions })?; + let changed = upsert_condition(&mut conditions, non_unique_condition); + if changed { + update_status!(clusters, name, TrustedExecutionClusterStatus { conditions })?; + } return Ok(Action::requeue(Duration::from_secs(60))); } info!("Setting up TrustedExecutionCluster {name}"); - let mut installing = conditions.clone(); - let installing_reason = NOT_INSTALLED_REASON_INSTALLING; - let condition = installed_condition(installing_reason, generation, existing_status); - installing.as_mut().unwrap().push(condition); - let status = TrustedExecutionClusterStatus { - conditions: installing, - }; - update_status!(clusters, name, status)?; + let installing_condition = + installed_condition(NOT_INSTALLED_REASON_INSTALLING, generation, existing_status); + let changed = upsert_condition(&mut conditions, installing_condition); + if changed { + let status = TrustedExecutionClusterStatus { + conditions: conditions.clone(), + }; + update_status!(clusters, name, status)?; + } install_trustee_configuration(kube_client.clone(), &cluster).await?; install_register_server(kube_client.clone(), &cluster).await?; install_attestation_key_register(kube_client, &cluster).await?; - let condition = installed_condition(INSTALLED_REASON, generation, existing_status); - conditions.as_mut().unwrap().push(condition); - update_status!(clusters, name, TrustedExecutionClusterStatus { conditions })?; + let installed_condition = installed_condition(INSTALLED_REASON, generation, existing_status); + let changed = upsert_condition(&mut conditions, installed_condition); + if changed { + let status = TrustedExecutionClusterStatus { conditions }; + update_status!(clusters, name, status)?; + } Ok(Action::await_change()) } @@ -358,7 +371,15 @@ mod tests { async fn test_reconcile_uninstalling() { let clos = async |req: Request, ctr| match req.method() { &Method::PATCH => { - assert_body_contains(req, NOT_INSTALLED_REASON_UNINSTALLING).await; + let uninstall_assertion_reason = "Request body did not contain Uninstall condition"; + assert_body_contains( + req, + &[( + NOT_INSTALLED_REASON_UNINSTALLING, + Some(uninstall_assertion_reason), + )], + ) + .await; Ok(serde_json::to_string(&dummy_cluster()).unwrap()) } _ => panic!("unexpected API interaction: {req:?}, counter {ctr}"), @@ -385,7 +406,9 @@ mod tests { // Watchers Ok(serde_json::to_string(&dummy_cluster()).unwrap()) } else if ctr == 4 && req.method() == Method::PATCH { - assert_body_contains(req, NOT_INSTALLED_REASON_NON_UNIQUE).await; + let err_msg = "Request body did not contain Non-Unique condition"; + assert_body_contains(req, &[(NOT_INSTALLED_REASON_NON_UNIQUE, Some(err_msg))]) + .await; Ok(serde_json::to_string(&dummy_cluster()).unwrap()) } else { panic!("unexpected API interaction: {req:?}, counter {ctr}"); @@ -410,4 +433,181 @@ mod tests { assert!(result.is_err()); }); } + + fn dummy_foreign_condition() -> Condition { + Condition { + type_: "ForeignCondition".to_string(), + status: "True".to_string(), + reason: "ExternalController".to_string(), + message: "Set by another controller".to_string(), + last_transition_time: Time(Timestamp::now()), + observed_generation: None, + } + } + + // Makes sure that uninstall trigger preserves foreign independent controller conditions, and our operator doesn't overwrite it in the reconcile function. Tests insert of our upsert_condition function. + #[tokio::test] + async fn test_reconcile_uninstall_preserves_foreign_controller_condition_by_inserting_owned_condition() + { + let foreign_condition = dummy_foreign_condition(); + + let clos = async |req: Request, ctr| match req.method() { + &Method::PATCH => { + assert_body_contains( + req, + &[ + ( + "ForeignCondition", + Some("Request body did not contain ForeignCondition"), + ), + ( + "ExternalController", + Some("Request body did not contain ExternalController"), + ), + ( + NOT_INSTALLED_REASON_UNINSTALLING, + Some("Request body did not contain Uninstall condition"), + ), + ], + ) + .await; + Ok(serde_json::to_string(&dummy_cluster()).unwrap()) + } + _ => panic!("unexpected API interaction: {req:?}, counter {ctr}"), + }; + + count_check!(1, clos, |client| { + let mut cluster = dummy_cluster(); + cluster.metadata.deletion_timestamp = Some(Time(Timestamp::now())); + cluster.status = Some(TrustedExecutionClusterStatus { + conditions: Some(vec![foreign_condition]), + }); + let result = reconcile(Arc::new(cluster), Arc::new(dummy_cluster_ctx(client))).await; + assert_eq!(result.unwrap(), Action::await_change()); + }); + } + + // Tests the update of our upsert functionality, preserving foreign conditions, and updating operator's owned condition. + // End to end unit test of the reconcile function, to ensure that new conditions are inserted and existing conditions are updated, without overwriting foreign conditions and creating conditions from scratch. + #[tokio::test] + async fn test_reconcile_install_preserves_foreign_condition_while_updating_owned_condition() { + let foreign_condition = dummy_foreign_condition(); + + let pre_existing_installed = Condition { + type_: INSTALLED_CONDITION.to_string(), + status: "False".to_string(), + reason: NOT_INSTALLED_REASON_INSTALLING.to_string(), + message: "Installation is in progress".to_string(), + last_transition_time: Time(Timestamp::now()), + observed_generation: None, + }; + + let clos = async |req: Request, _ctr| { + match *req.method() { + Method::GET => { + let object_list = ObjectList:: { + items: vec![dummy_cluster()], + types: Default::default(), + metadata: Default::default(), + }; + Ok(serde_json::to_string(&object_list).unwrap()) + } + Method::POST => Ok(serde_json::to_string(&dummy_cluster()).unwrap()), + Method::PATCH => { + let body = req.into_body().collect_bytes().await.unwrap().to_vec(); + let body = String::from_utf8_lossy(&body); + assert!( + body.contains("ForeignCondition"), + "Request body did not contain ForeignCondition, got: {body}" + ); + + if body.contains(INSTALLED_REASON) { + // Also assert that the installed condition is updated to True from False, and only 1 installed condition is updated and present. + let patch: serde_json::Value = serde_json::from_str(&body).unwrap(); + let conditions = patch["status"]["conditions"] + .as_array() + .expect("conditions should be an array"); + let installed: Vec<_> = conditions + .iter() + .filter(|c| c["type"] == "Installed") + .collect(); + assert_eq!( + installed.len(), + 1, + "Expected exactly one Installed condition, found {}", + installed.len() + ); + assert_eq!( + installed[0]["status"], "True", + "Installed condition should be updated to True" + ); + } + Ok(serde_json::to_string(&dummy_cluster()).unwrap()) + } + _ => panic!("unexpected API interaction: {req:?}"), + } + }; + + let request_count = Arc::new(std::sync::atomic::AtomicU32::new(0)); + let client = MockClient::new(clos, "test".to_string(), request_count).into_client(); + + let mut cluster = dummy_cluster(); + cluster.status = Some(TrustedExecutionClusterStatus { + conditions: Some(vec![pre_existing_installed, foreign_condition]), + }); + let result = reconcile(Arc::new(cluster), Arc::new(dummy_cluster_ctx(client))).await; + assert_eq!(result.unwrap(), Action::await_change()); + } + + // This test ensures that if the condition is not changed, the status is not patched. The transition_time and all other fields remain same. + #[tokio::test] + async fn test_reconcile_no_patch_when_conditions_unchanged() { + let clos1 = async |req: Request, _| match *req.method() { + Method::PATCH => Ok(serde_json::to_string(&dummy_cluster()).unwrap()), + _ => panic!("unexpected: {req:?}"), + }; + + // Deletion makes 1 patch status. + count_check!(1, clos1, |client| { + let mut cluster = dummy_cluster(); + cluster.metadata.deletion_timestamp = Some(Time(Timestamp::now())); + reconcile(Arc::new(cluster), Arc::new(dummy_cluster_ctx(client))) + .await + .unwrap(); + }); + + // Building the uninstalling cluster state. + let dummy = dummy_cluster(); + let existing_status = &dummy.status; // None + let generation = dummy.metadata.generation; + let known_address = dummy.spec.public_trustee_addr.is_some(); + + let mut conditions = None; + let _ = upsert_condition( + &mut conditions, + known_trustee_address_condition(known_address, generation, existing_status), + ); + let _ = upsert_condition( + &mut conditions, + installed_condition( + NOT_INSTALLED_REASON_UNINSTALLING, + generation, + existing_status, + ), + ); + + assert_eq!(conditions.as_ref().unwrap().len(), 2); + + let clos2 = async |req: Request, _| panic!("unexpected API call: {req:?}"); + + // Reconcile should not send another patch request, as conditions are exactly the same. + count_check!(0, clos2, |client| { + let mut cluster = dummy_cluster(); + cluster.metadata.deletion_timestamp = Some(Time(Timestamp::now())); + cluster.status = Some(TrustedExecutionClusterStatus { conditions }); + reconcile(Arc::new(cluster), Arc::new(dummy_cluster_ctx(client))) + .await + .unwrap(); + }); + } } diff --git a/operator/src/reference_values.rs b/operator/src/reference_values.rs index bf16fab4..b8dcbad6 100644 --- a/operator/src/reference_values.rs +++ b/operator/src/reference_values.rs @@ -33,7 +33,7 @@ use std::{collections::BTreeMap, sync::Arc, time::Duration}; use crate::trustee::{self, get_image_pcrs}; use operator::{ ControllerError, RvContextData, controller_error_policy, controller_info, - create_or_info_if_exists, + create_or_info_if_exists, upsert_condition, }; use trusted_cluster_operator_lib::{conditions::*, reference_values::*, *}; @@ -314,10 +314,15 @@ async fn image_add_reconcile( } }; let committed = committed_condition(reason, image.metadata.generation, &image.status); - let conditions = Some(vec![committed]); - let images: Api = Api::default_namespaced(kube_client); - update_status!(images, &name, ApprovedImageStatus { conditions }) - .map_err(|e| finalizer::Error::::ApplyFailed(e.into()))?; + + // Upserting the committed condition and keeping the existing conditions intact. + let mut conditions = image.status.as_ref().and_then(|s| s.conditions.clone()); + let changed = upsert_condition(&mut conditions, committed); + if changed { + let images: Api = Api::default_namespaced(kube_client); + update_status!(images, &name, ApprovedImageStatus { conditions }) + .map_err(|e| finalizer::Error::::ApplyFailed(e.into()))?; + } Ok(action) } @@ -366,7 +371,9 @@ pub async fn handle_new_image( return Ok(NOT_COMMITTED_REASON_NO_DIGEST); } let label = fetch_pcr_label(&image_ref).await; - let compute_pcrs = match label { + + // Whether to compute pcrs or not. + let should_compute_pcrs = match label { Err(ref e) => { warn!("Fetching PCR label for {image_ref} failed: {e}. Falling back to computation."); if is_pending(&ctx.client, resource_name).await? { @@ -380,7 +387,7 @@ pub async fn handle_new_image( } _ => false, }; - if compute_pcrs { + if should_compute_pcrs { return compute_fresh_pcrs(ctx, resource_name, boot_image) .await .map(|_| NOT_COMMITTED_REASON_COMPUTING); diff --git a/test_utils/src/mock_client.rs b/test_utils/src/mock_client.rs index 580054d3..aa0a2596 100644 --- a/test_utils/src/mock_client.rs +++ b/test_utils/src/mock_client.rs @@ -35,11 +35,10 @@ macro_rules! assert_kube_api_error { #[macro_export] macro_rules! count_check { ($expected:literal, $closure:ident, |$client:ident| $body:block) => { - use std::sync::atomic; - let count = std::sync::Arc::new(atomic::AtomicU32::new(0)); + let count = std::sync::Arc::new(std::sync::atomic::AtomicU32::new(0)); let $client = MockClient::new($closure, "test".to_string(), count.clone()).into_client(); $body - assert_eq!(count.load(atomic::Ordering::Acquire), $expected, "Endpoint call count mismatch"); + assert_eq!(count.load(std::sync::atomic::Ordering::Acquire), $expected, "Endpoint call count mismatch"); } } @@ -107,10 +106,17 @@ where } } -pub async fn assert_body_contains(req: Request, contains: &str) { +pub async fn assert_body_contains(req: Request, expected: &[(&str, Option<&str>)]) { let bytes = req.into_body().collect_bytes().await.unwrap().to_vec(); let body = String::from_utf8_lossy(&bytes); - assert!(body.contains(contains)); + + for (expected_reason, expected_err_msg) in expected { + let msg = expected_err_msg.unwrap_or("Given body doesn't contain the expected string"); + assert!( + body.contains(expected_reason), + "{msg}: expected '{expected_reason}', got: {body}" + ); + } } pub async fn test_create_success< From 97e2da1984186a605e4f12df2037c802517aa64c Mon Sep 17 00:00:00 2001 From: Chirag Rao <73184157+SpaceFace02@users.noreply.github.com> Date: Mon, 25 May 2026 12:29:38 +0530 Subject: [PATCH 3/4] Linting changes as of new toolchain, non breaking --- attestation-key-register/src/main.rs | 5 ++--- register-server/src/main.rs | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/attestation-key-register/src/main.rs b/attestation-key-register/src/main.rs index 90a001a9..a9d80a71 100644 --- a/attestation-key-register/src/main.rs +++ b/attestation-key-register/src/main.rs @@ -138,9 +138,8 @@ async fn main() { let service = app.into_make_service(); info!("Starting attestation key registration server on http://{addr}",); - let run = if args.cert_path.is_some() && args.key_path.is_some() { - let config = OpenSSLConfig::from_pem_file(args.cert_path.unwrap(), args.key_path.unwrap()) - .expect("invalid PEM files"); + let run = if let (Some(cert_path), Some(key_path)) = (args.cert_path, args.key_path) { + let config = OpenSSLConfig::from_pem_file(cert_path, key_path).expect("invalid PEM files"); axum_server::bind_openssl(addr, config).serve(service).await } else { axum_server::bind(addr).serve(service).await diff --git a/register-server/src/main.rs b/register-server/src/main.rs index 7fc80a9f..ea920ded 100644 --- a/register-server/src/main.rs +++ b/register-server/src/main.rs @@ -251,13 +251,13 @@ async fn main() { let service = app.into_make_service(); info!("Starting server on http://{addr}"); - let run = if args.cert_path.is_some() && args.key_path.is_some() { - let config = OpenSSLConfig::from_pem_file(args.cert_path.unwrap(), args.key_path.unwrap()) - .expect("invalid PEM files"); + let run = if let (Some(cert_path), Some(key_path)) = (args.cert_path, args.key_path) { + let config = OpenSSLConfig::from_pem_file(cert_path, key_path).expect("invalid PEM files"); axum_server::bind_openssl(addr, config).serve(service).await } else { axum_server::bind(addr).serve(service).await }; + run.expect("Server failed"); } From 824d2ee487da3539921c568af2d999ca5bc6a8da Mon Sep 17 00:00:00 2001 From: Chirag Rao <73184157+SpaceFace02@users.noreply.github.com> Date: Mon, 25 May 2026 12:30:03 +0530 Subject: [PATCH 4/4] Renamed to register-server-port for disambiguation from http, as the presence of a secret indicates http or https. --- operator/src/register_server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator/src/register_server.rs b/operator/src/register_server.rs index a9fe96d8..23b9badd 100644 --- a/operator/src/register_server.rs +++ b/operator/src/register_server.rs @@ -108,7 +108,7 @@ pub async fn create_register_server_service( spec: Some(ServiceSpec { selector: Some(labels), ports: Some(vec![ServicePort { - name: Some("http".to_string()), + name: Some("register-server-port".to_string()), port: register_server_port.unwrap_or(REGISTER_SERVER_PORT), target_port: Some(IntOrString::Int(REGISTER_SERVER_PORT)), protocol: Some("TCP".to_string()),