Skip to content

Commit 6683898

Browse files
committed
feat: migrate to Server-Side Apply (SSA) and enforce controller ownership
Fixes: #258 Migrate resource creation and updates from client-side POST/PUT to Server-Side Apply (SSA) where appropriate, and strengthen ownership semantics with controller references throughout. This commit establishes ownership of various fields and CRD's, to prevent accidental overwrites or race conditions by other controllers It doesn't migrate all patches to SSA, if we plan to migrate completely to SSA, there would be significant code rework, and honestly, its not necessary for now. All resources created by the operator now use `controller: true` in ownerReferences, enabling Kubernetes garbage collection: - TEC owns: Deployments, Services, ConfigMaps, Jobs, ApprovedImages - Machine owns: AttestationKey (transferred from TEC via merge patch once its approved automatically). - AttestationKey owns: Secret (with controller: true + finalizer) `generate_owner_reference` renamed to `generate_owner_controller_reference` to reflect that all owner refs are now controller refs with `blockOwnerDeletion: true`. All controllers are owners, but not all owners have to be controllers. Kubernetes mandates only one controller. Multiple owners are fine for Garbage Collection and delete cascades. New `apply_resource!` macro replaces `create_or_info_if_exists!` for idempotent resource creation via SSA (PATCH with Patch::Apply). `update_reference_values`: uses full-data SSA patch on trustee-data ConfigMap (includes all keys: kbs-config.toml, policy.rego, reference-values.json) to prevent the field manager from implicitly relinquishing keys not in a partial patch. If a field manager (controller owning a field of a CRD) makes a SSA patch, all fields of that patch are considered owned, and any previously owned fields not part of the patch are released from its control. `update_attestation_keys`: uses full-object SSA patch on the Deployment (sends entire deployment.spec) to avoid stripping required fields like spec.selector. Metadata fields (managedFields, resourceVersion, uid) are excluded from the SSA body to prevent 400 errors. `do_mount_secret`: kept as GET+mutate+replace (PUT) since it follows a read-modify-write pattern on the same field manager. Migrating this is significant code rework. Owner transfer in `approve_ak` uses Merge patch (not SSA) because SSA would upsert into the ownerReferences array, leaving stale references of both TEC. In the end TEC and Machine would co-exist as controllers, which we don't want. Merge patch replaces the entire ownerReferences field atomically. Single field manager `trusted-cluster-operator` used consistently via `FIELD_MANAGER` constant. register-server uses its own `register-server` field manager for Machine creation (POST with fieldManager query param, not SSA). `update_status!` macro now requires explicit type and field manager parameters, enabling force-apply on the status subresource where multiple writers exist (e.g. ApprovedImage status). - Duplicate volume entries: `do_mount_secret` now checks for existing volumes before adding, preventing 422 errors on reconciler retry. - Secret controller filtering: `update_attestation_keys` and `secret_reconcile` now require `controller: true` on AttestationKey owner refs, not just `kind == "AttestationKey"`. - ConfigMap data loss: full-data SSA patch on trustee-data prevents kbs-config.toml and policy.rego from being garbage-collected when only reference-values.json is updated. Added unit tests validating SSA body content, ownership, idempotency, field manager usage, and filtering logic across trustee, reference values, attestation key register, and register server modules. Commit message co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 04e7986 commit 6683898

13 files changed

Lines changed: 928 additions & 187 deletions

File tree

attestation-key-register/src/main.rs

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ use uuid::Uuid;
1616

1717
use trusted_cluster_operator_lib::endpoints::ATTESTATION_KEY_REGISTER_RESOURCE;
1818
use trusted_cluster_operator_lib::{
19-
generate_owner_reference, get_trusted_execution_cluster, AttestationKey, AttestationKeySpec,
19+
self, generate_owner_controller_reference, get_trusted_execution_cluster, AttestationKey,
20+
AttestationKeySpec,
2021
};
2122

2223
#[derive(Parser)]
@@ -64,15 +65,17 @@ async fn handle_registration(
6465

6566
let api: Api<AttestationKey> = Api::default_namespaced(client.clone());
6667

67-
// Get the TrustedExecutionCluster to use as owner reference
68+
// TEC is owner-controller of the AttestationKey until the operator approves the key and transfers control to Machine, which then becomes the controller and manages lifecycle of the AttestationKey.
6869
let cluster = match get_trusted_execution_cluster(client.clone()).await {
6970
Ok(c) => c,
7071
Err(e) => return internal_error(e.context("Failed to get TrustedExecutionCluster")),
7172
};
7273

73-
let owner_reference = match generate_owner_reference(&cluster) {
74+
let owner_controller_reference = match generate_owner_controller_reference(&cluster) {
7475
Ok(o) => o,
75-
Err(e) => return internal_error(e.context("Failed to generate owner reference")),
76+
Err(e) => {
77+
return internal_error(e.context("Failed to generate owner-controller reference"))
78+
}
7679
};
7780

7881
match api.list(&Default::default()).await {
@@ -104,7 +107,7 @@ async fn handle_registration(
104107
let attestation_key = AttestationKey {
105108
metadata: ObjectMeta {
106109
name: Some(name.clone()),
107-
owner_references: Some(vec![owner_reference]),
110+
owner_references: Some(vec![owner_controller_reference]),
108111
..Default::default()
109112
},
110113
spec: AttestationKeySpec {
@@ -114,7 +117,17 @@ async fn handle_registration(
114117
status: None,
115118
};
116119

117-
match api.create(&Default::default(), &attestation_key).await {
120+
// Client side apply, as this is a one time operation for each attestation key, and one owner. SSA is mainly used for patching. Setting field manager, so that we can identify the source of the creation later for reconciliation by operator.
121+
match api
122+
.create(
123+
&kube::api::PostParams {
124+
field_manager: Some(trusted_cluster_operator_lib::FIELD_MANAGER.to_string()),
125+
..Default::default()
126+
},
127+
&attestation_key,
128+
)
129+
.await
130+
{
118131
Ok(created) => {
119132
let name = created.metadata.name.unwrap_or_default();
120133
info!("Successfully created AttestationKey: {name}",);

compute-pcrs/src/main.rs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ use std::{fs::File, io::Read};
1212

1313
use trusted_cluster_operator_lib::{conditions::INSTALLED_REASON, reference_values::*, *};
1414

15+
const FIELD_MANAGER: &str = "compute-pcrs";
16+
1517
#[derive(Parser)]
1618
#[command(version, about)]
1719
struct Args {
@@ -77,6 +79,16 @@ async fn main() -> Result<()> {
7779
let committed = committed_condition(INSTALLED_REASON, image.metadata.generation, &None);
7880
let conditions = Some(vec![committed]);
7981
let status = ApprovedImageStatus { conditions };
80-
update_status!(approved_images, &args.resource_name, status)?;
82+
83+
// As operator also updates the status field, while requeuing every 10 seconds, we need to force this update to avoid race condition.
84+
// TODO: Simplify ownership of this field.
85+
update_status!(
86+
approved_images,
87+
&args.resource_name,
88+
status,
89+
ApprovedImage,
90+
FIELD_MANAGER,
91+
force
92+
)?;
8193
Ok(())
8294
}

lib/src/lib.rs

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,31 @@ use conditions::*;
3030
use k8s_openapi::apimachinery::pkg::apis::meta::v1::{Condition, OwnerReference, Time};
3131
use kube::Resource;
3232

33+
pub const FIELD_MANAGER: &str = "trusted-cluster-operator";
34+
35+
// 2 arms for updating status, one for normal updates and one for force updates.
3336
#[macro_export]
3437
macro_rules! update_status {
35-
($api:ident, $name:expr, $status:expr) => {{
36-
let patch = kube::api::Patch::Merge(serde_json::json!({"status": $status}));
37-
$api.patch_status($name, &Default::default(), &patch).await
38+
($api:ident, $name:expr, $status:expr, $type:ty, $field_manager:expr) => {{
39+
let patch = kube::api::Patch::Apply(serde_json::json!({
40+
"apiVersion": <$type as kube::Resource>::api_version(&()),
41+
"kind": <$type as kube::Resource>::kind(&()),
42+
"status": $status
43+
}));
44+
let params = kube::api::PatchParams::apply($field_manager);
45+
$api.patch_status($name, &params, &patch).await
46+
.map_err(Into::<anyhow::Error>::into)
47+
}};
48+
($api:ident, $name:expr, $status:expr, $type:ty, $field_manager:expr, force) => {{
49+
let patch = kube::api::Patch::Apply(serde_json::json!({
50+
"apiVersion": <$type as kube::Resource>::api_version(&()),
51+
"kind": <$type as kube::Resource>::kind(&()),
52+
"status": $status
53+
}));
54+
let params = kube::api::PatchParams::apply($field_manager).force();
55+
$api.patch_status($name, &params, &patch).await
3856
.map_err(Into::<anyhow::Error>::into)
39-
}}
57+
}};
4058
}
4159

4260
pub fn condition_status(status: bool) -> String {
@@ -108,8 +126,8 @@ pub fn committed_condition(
108126
}
109127
}
110128

111-
/// Generate an OwnerReference for any Kubernetes resource
112-
pub fn generate_owner_reference<T: Resource<DynamicType = ()>>(
129+
/// OwnerReference with `controller: true` for lifecycle and garbage collection.
130+
pub fn generate_owner_controller_reference<T: Resource<DynamicType = ()>>(
113131
object: &T,
114132
) -> anyhow::Result<OwnerReference> {
115133
let name = object.meta().name.clone();

lib/src/reference_values.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ macro_rules! update_image_pcrs {
2929
let map = (PCR_CONFIG_FILE.to_string(), image_pcrs_json.to_string());
3030
let data = std::collections::BTreeMap::from([map]);
3131
$map.data = Some(data);
32+
33+
// Operator and compute-pcr's both write to this configmap, hence client-side apply makes sense here.
3234
$api.replace(PCR_CONFIG_MAP, &Default::default(), &$map)
3335
.await?
3436
};

0 commit comments

Comments
 (0)