From b4d43bbdb21a720d6d1b4a98d12649195eb03999 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 04:31:00 +0000 Subject: [PATCH 1/4] fix(operator): bound plan retention per phase so churn cannot evict history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Terminal plans were trimmed as one pool of 10 by creation time. Superseded is generated churn — every replan supersedes its predecessor — so on an active policy it filled the pool and deleted the Applied plans, which are the record of what actually executed against the database. The least informative terminal state was evicting the most informative one. Bounds are now per phase: Applied 25, Failed and Rejected 10 shared, Superseded 3. Pending, Approved and Applying are live and never evicted. Applied additionally carries an age floor of 30 days, so the retained span is a stated period rather than a function of how often the policy applies, and a ceiling of 200 so that promise cannot become unbounded growth. One oldest-first pass: stop at the count bound, spare anything inside the floor along the way, unless the survivors would breach the ceiling. Eviction is decided by a pure `plans_to_evict(plans, retention, now)`, the same shape as `classify_open_candidates`. These bounds are only reachable with hundreds of objects, so an integration test could not see them. `max_plans: Option` becomes `Option`. Both call sites still pass None; surfacing it on the CRD or the chart is left to #194. Mutation-checked: dropping the age floor fails two tests, dropping the ceiling fails one. Refs #194 Claude-Session: https://claude.ai/code/session_01RGdj9MJHTYinybQDE6Zmop --- CHANGELOG.md | 2 + crates/pgroles-operator/src/plan.rs | 427 ++++++++++++++++++--- docs/src/pages/docs/operator-candidates.md | 26 ++ 3 files changed, 402 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4043775..8ef9156e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Plan retention is bounded per phase, so replan churn no longer evicts the record of what ran.** Terminal plans were trimmed as one pool of 10 by creation time. `Superseded` is generated churn — every replan supersedes its predecessor — so on an active policy it filled the pool and deleted the `Applied` plans, which are the audit record of what actually executed against the database. The least informative state was evicting the most informative one. The bounds are now `Applied` 25 (never fewer than 30 days' worth, hard ceiling 200), `Failed` and `Rejected` 10 shared, `Superseded` 3; `Pending`, `Approved` and `Applying` are live and never evicted. The age floor makes the retained span a stated period rather than a function of how often a policy applies, and the ceiling stops that promise becoming unbounded growth. `pgroles.io/keep=true` still exempts a plan from every bound. Not yet configurable. (#194) + - **`spec.mode: plan` is renamed to `spec.mode: observe`, with a deprecation window.** "Plan" now names exactly one thing, the `PostgresPolicyPlan` resource; the `ApprovalIgnored` reason `PlanModeNeverExecutes` is now `ObserveModeNeverExecutes`. The old value keeps working: `mode: plan` stays an accepted schema value with identical behaviour, so a GitOps controller re-applying an existing manifest is unaffected by the upgrade. A policy using it reports a `ModeValueDeprecated` condition, warns in the operator log, and counts toward `pgroles.deprecated.mode_plan`. **Upgrade:** change `mode: plan` to `mode: observe` in your manifests at your convenience — a future release removes the `plan` value, and that removal will be the breaking change. diff --git a/crates/pgroles-operator/src/plan.rs b/crates/pgroles-operator/src/plan.rs index 378bfe88..f7460376 100644 --- a/crates/pgroles-operator/src/plan.rs +++ b/crates/pgroles-operator/src/plan.rs @@ -84,8 +84,46 @@ const ORPHAN_GRACE_SECS: i64 = 60; /// Best-effort cleanup should never block a fresh reconcile for long. const CLEANUP_TIMEOUT_SECS: u64 = 5; -/// Default maximum number of historical plans to retain per policy. -const DEFAULT_MAX_PLANS: usize = 10; +/// How many terminal plans of each kind retention keeps for one policy. +/// +/// Split by phase rather than pooled, because the phases are not worth the +/// same and the cheapest one is the one generated fastest. `Superseded` is +/// churn: every replan supersedes its predecessor, so on an active policy a +/// single pool fills with records of plans that never ran and evicts the +/// `Applied` ones, which are the record of what did. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct PlanRetention { + /// `Applied` plans kept once they are older than `applied_min_age_secs`. + pub applied: usize, + /// Hard ceiling on `Applied` plans, age floor notwithstanding. Without it + /// a policy applying continuously would grow the count without limit for + /// the whole floor period; the floor is a promise about history, not a + /// licence to keep everything. + pub applied_ceiling: usize, + /// An `Applied` plan younger than this is kept whatever `applied` says, + /// so the audit trail spans a stated period instead of however long the + /// policy's churn rate happens to make it. + pub applied_min_age_secs: i64, + /// `Failed` and `Rejected` plans. Both are decisions worth reading back — + /// why something did not run, and who declined it. + pub decided: usize, + /// `Superseded` plans. Deliberately small: it is the least informative + /// terminal state and the one generating the pressure. A couple is enough + /// to see what a replan replaced. + pub superseded: usize, +} + +impl Default for PlanRetention { + fn default() -> Self { + Self { + applied: 25, + applied_ceiling: 200, + applied_min_age_secs: 30 * 24 * 60 * 60, + decided: 10, + superseded: 3, + } + } +} /// How recently a Failed plan must have been created (in seconds) for the /// dedup check to consider it a match. Plans older than this are ignored so @@ -1286,11 +1324,11 @@ pub(crate) async fn execute_changes_in_transaction( pub async fn cleanup_old_plans_best_effort( client: &Client, policy: &PostgresPolicy, - max_plans: Option, + retention: Option, ) { match tokio::time::timeout( Duration::from_secs(CLEANUP_TIMEOUT_SECS), - cleanup_old_plans(client, policy, max_plans), + cleanup_old_plans(client, policy, retention), ) .await { @@ -1303,19 +1341,20 @@ pub async fn cleanup_old_plans_best_effort( } } -/// Clean up old plans for a policy, retaining at most `max_plans` terminal plans. +/// Clean up old plans for a policy under [`PlanRetention`]. /// -/// Terminal plans are those in Applied, Failed, Superseded, or Rejected phase; -/// Pending, Approved, and Applying plans are retained. Status-less plans and -/// SQL ConfigMaps older than a short grace period are treated as stale orphans. +/// Terminal plans — Applied, Failed, Superseded, Rejected — are bounded per +/// phase; Pending, Approved, and Applying plans are never evicted, because +/// they are still live. Status-less plans and SQL ConfigMaps older than a +/// short grace period are treated as stale orphans. pub async fn cleanup_old_plans( client: &Client, policy: &PostgresPolicy, - max_plans: Option, + retention: Option, ) -> Result<(), ReconcileError> { let namespace = policy.namespace().ok_or(ReconcileError::NoNamespace)?; let policy_name = policy.name_any(); - let max_plans = max_plans.unwrap_or(DEFAULT_MAX_PLANS); + let retention = retention.unwrap_or_default(); let plans_api: Api = Api::namespaced(client.clone(), &namespace); let selector = policy_selector(&policy_name); @@ -1346,51 +1385,20 @@ pub async fn cleanup_old_plans( } } - // Collect terminal plans sorted by creation timestamp (oldest first). - // `pgroles.io/keep=true` exempts an object from the bound: retention is a - // cap on unbounded growth, not a policy about what an operator may keep. - let mut terminal_plans: Vec<&PostgresPolicyPlan> = existing_plans - .iter() - .filter(|plan| !is_retention_exempt(*plan)) - .filter(|plan| { - plan.status - .as_ref() - .map(|s| { - matches!( - s.phase, - PlanPhase::Applied - | PlanPhase::Failed - | PlanPhase::Superseded - | PlanPhase::Rejected - ) - }) - .unwrap_or(false) - }) - .collect(); - - if terminal_plans.len() > max_plans { - // Sort by creation timestamp ascending (oldest first). - terminal_plans.sort_by(|a, b| { - let a_time = a.metadata.creation_timestamp.as_ref(); - let b_time = b.metadata.creation_timestamp.as_ref(); - a_time.cmp(&b_time) - }); - - let plans_to_delete = terminal_plans.len() - max_plans; - for plan in terminal_plans.into_iter().take(plans_to_delete) { - let plan_name = plan.name_any(); - info!( + for plan in plans_to_evict(&existing_plans, retention, now_ts) { + let plan_name = plan.name_any(); + info!( + plan = %plan_name, + policy = %policy_name, + phase = ?plan.status.as_ref().map(|status| &status.phase), + "cleaning up old plan" + ); + if let Err(err) = plans_api.delete(&plan_name, &DeleteParams::default()).await { + tracing::warn!( plan = %plan_name, - policy = %policy_name, - "cleaning up old plan" + %err, + "failed to delete old plan during cleanup" ); - if let Err(err) = plans_api.delete(&plan_name, &DeleteParams::default()).await { - tracing::warn!( - plan = %plan_name, - %err, - "failed to delete old plan during cleanup" - ); - } } } @@ -1491,6 +1499,94 @@ fn should_patch_existing_plan_status(plan: &PostgresPolicyPlan) -> bool { .unwrap_or(true) } +/// Which terminal plans retention evicts this pass. +/// +/// Pure and takes `now` as an argument, so the policy is testable without a +/// cluster — the bounds are the part worth pinning, and they are invisible in +/// an integration test that would have to create hundreds of objects to reach +/// them. +/// +/// `pgroles.io/keep=true` exempts an object from every bound: retention caps +/// unbounded growth, it is not a policy about what an operator may keep. +fn plans_to_evict( + plans: &[PostgresPolicyPlan], + retention: PlanRetention, + now_ts: i64, +) -> Vec<&PostgresPolicyPlan> { + let mut applied = Vec::new(); + let mut decided = Vec::new(); + let mut superseded = Vec::new(); + + for plan in plans.iter().filter(|plan| !is_retention_exempt(*plan)) { + let Some(phase) = plan.status.as_ref().map(|status| &status.phase) else { + continue; + }; + match phase { + PlanPhase::Applied => applied.push(plan), + PlanPhase::Failed | PlanPhase::Rejected => decided.push(plan), + PlanPhase::Superseded => superseded.push(plan), + // Still live. Never evicted, however old. + PlanPhase::Pending | PlanPhase::Approved | PlanPhase::Applying => {} + } + } + + let mut evict = oldest_beyond(&mut decided, retention.decided); + evict.extend(oldest_beyond(&mut superseded, retention.superseded)); + + // Applied is the audit record of what actually ran, so its count bound + // yields to the age floor. Walk oldest first and stop once the count bound + // is met; spare anything still inside the floor along the way, unless the + // survivors would breach the ceiling. Sparing is what makes the retained + // span a promise rather than a function of how often the policy applies. + sort_oldest_first(&mut applied); + let mut remaining = applied.len(); + for plan in applied { + if remaining <= retention.applied { + break; + } + let inside_floor = plan_age_secs(plan, now_ts) < retention.applied_min_age_secs; + if inside_floor && remaining <= retention.applied_ceiling { + continue; + } + evict.push(plan); + remaining -= 1; + } + + evict +} + +/// Sort oldest first and return everything past `keep`. +fn oldest_beyond<'a>( + plans: &mut Vec<&'a PostgresPolicyPlan>, + keep: usize, +) -> Vec<&'a PostgresPolicyPlan> { + if plans.len() <= keep { + return Vec::new(); + } + sort_oldest_first(plans); + plans[..plans.len() - keep].to_vec() +} + +fn sort_oldest_first(plans: &mut [&PostgresPolicyPlan]) { + plans.sort_by(|a, b| { + a.metadata + .creation_timestamp + .as_ref() + .cmp(&b.metadata.creation_timestamp.as_ref()) + }); +} + +/// Age in seconds, or 0 for a plan with no creation timestamp — treating an +/// unknown age as brand new keeps it, which is the safe direction for a +/// deletion decision. +fn plan_age_secs(plan: &PostgresPolicyPlan, now_ts: i64) -> i64 { + plan.metadata + .creation_timestamp + .as_ref() + .map(|timestamp| now_ts.saturating_sub(timestamp.0.as_second())) + .unwrap_or(0) +} + fn is_stale_statusless_plan(plan: &PostgresPolicyPlan, now_ts: i64) -> bool { plan.status.is_none() && is_stale_object(plan, now_ts) } @@ -2397,6 +2493,231 @@ mod tests { } /// Build a plan in `phase`, having recorded a failure `age_secs` ago. + /// A terminal plan of `phase`, created `age_secs` ago. + fn retained_plan( + name: &str, + phase: PlanPhase, + age_secs: i64, + now_ts: i64, + ) -> PostgresPolicyPlan { + let mut plan = PostgresPolicyPlan::new(name, test_plan_spec()); + plan.metadata.creation_timestamp = + Some(k8s_openapi::apimachinery::pkg::apis::meta::v1::Time( + jiff::Timestamp::from_second(now_ts - age_secs).expect("epoch second in range"), + )); + plan.status = Some(PostgresPolicyPlanStatus { + phase, + ..Default::default() + }); + plan + } + + fn evicted_names( + plans: &[PostgresPolicyPlan], + retention: PlanRetention, + now_ts: i64, + ) -> Vec { + let mut names: Vec = plans_to_evict(plans, retention, now_ts) + .into_iter() + .map(|plan| plan.name_any()) + .collect(); + names.sort(); + names + } + + /// The bug this split exists for: replan churn must not evict the record + /// of what actually ran. Under one pooled bound the Superseded plans, being + /// newer, kept every slot and the Applied ones were deleted. + #[test] + fn superseded_churn_does_not_evict_applied_plans() { + let now = 1_700_000_000; + let retention = PlanRetention::default(); + let year = 400 * 24 * 60 * 60; + + let mut plans = vec![retained_plan("applied-old", PlanPhase::Applied, year, now)]; + // Far more superseded plans than any pooled bound would have allowed, + // all newer than the applied one. + for i in 0..50 { + plans.push(retained_plan( + &format!("superseded-{i:03}"), + PlanPhase::Superseded, + 1000 - i, + now, + )); + } + + let evicted = evicted_names(&plans, retention, now); + assert!( + !evicted.contains(&"applied-old".to_string()), + "an applied plan must survive any amount of replan churn" + ); + assert_eq!( + evicted.len(), + 50 - retention.superseded, + "every superseded plan past the small bound is evicted" + ); + } + + #[test] + fn each_terminal_phase_is_bounded_separately() { + let now = 1_700_000_000; + let retention = PlanRetention::default(); + let year = 400 * 24 * 60 * 60; + + let mut plans = Vec::new(); + for (prefix, phase, count) in [ + ("applied", PlanPhase::Applied, retention.applied + 4), + ("failed", PlanPhase::Failed, retention.decided + 4), + ( + "superseded", + PlanPhase::Superseded, + retention.superseded + 4, + ), + ] { + for i in 0..count { + // Older than the age floor, so only the count bounds apply. + plans.push(retained_plan( + &format!("{prefix}-{i:03}"), + phase.clone(), + year + count as i64 - i as i64, + now, + )); + } + } + + let evicted = evicted_names(&plans, retention, now); + for prefix in ["applied", "failed", "superseded"] { + let count = evicted.iter().filter(|n| n.starts_with(prefix)).count(); + assert_eq!(count, 4, "{prefix} should lose exactly its 4 excess plans"); + } + } + + /// Failed and Rejected share one bound, so a policy that fails repeatedly + /// cannot bury the record of a plan a reviewer declined. + #[test] + fn failed_and_rejected_share_the_decided_bound() { + let now = 1_700_000_000; + let retention = PlanRetention::default(); + let year = 400 * 24 * 60 * 60; + + let mut plans = vec![retained_plan( + "rejected", + PlanPhase::Rejected, + year * 2, + now, + )]; + for i in 0..retention.decided { + plans.push(retained_plan( + &format!("failed-{i:03}"), + PlanPhase::Failed, + year - i as i64, + now, + )); + } + + // One over the shared bound, and the rejected plan is the oldest. + assert_eq!( + evicted_names(&plans, retention, now), + vec!["rejected".to_string()] + ); + } + + /// The age floor is what makes the retained span a promise rather than a + /// function of how often the policy applies. + #[test] + fn the_age_floor_keeps_applied_plans_past_the_count_bound() { + let now = 1_700_000_000; + let retention = PlanRetention::default(); + + let recent: Vec = (0..retention.applied + 10) + .map(|i| { + retained_plan( + &format!("applied-{i:03}"), + PlanPhase::Applied, + retention.applied_min_age_secs - 1 - i as i64, + now, + ) + }) + .collect(); + assert!( + evicted_names(&recent, retention, now).is_empty(), + "nothing inside the floor is evicted, even past the count bound" + ); + + // One second older and the same plan is outside the promise. + let stale: Vec = (0..retention.applied + 10) + .map(|i| { + retained_plan( + &format!("applied-{i:03}"), + PlanPhase::Applied, + retention.applied_min_age_secs + 1 + i as i64, + now, + ) + }) + .collect(); + assert_eq!(evicted_names(&stale, retention, now).len(), 10); + } + + /// The floor promises history; it does not license unbounded growth. + #[test] + fn the_ceiling_overrides_the_age_floor() { + let now = 1_700_000_000; + let retention = PlanRetention::default(); + + let plans: Vec = (0..retention.applied_ceiling + 40) + .map(|i| { + retained_plan( + &format!("applied-{i:04}"), + PlanPhase::Applied, + // All well inside the floor. + 60 + i as i64, + now, + ) + }) + .collect(); + + assert_eq!( + evicted_names(&plans, retention, now).len(), + 40, + "the ceiling trims back to itself, not to the count bound" + ); + } + + #[test] + fn live_plans_and_kept_plans_are_never_evicted() { + let now = 1_700_000_000; + let retention = PlanRetention::default(); + let year = 400 * 24 * 60 * 60; + + let mut plans = Vec::new(); + for (i, phase) in [PlanPhase::Pending, PlanPhase::Approved, PlanPhase::Applying] + .into_iter() + .enumerate() + { + plans.push(retained_plan(&format!("live-{i}"), phase, year * 2, now)); + } + // Enough superseded plans to blow past the bound many times over. + for i in 0..40 { + plans.push(retained_plan( + &format!("superseded-{i:03}"), + PlanPhase::Superseded, + year, + now, + )); + } + let mut kept = retained_plan("kept", PlanPhase::Superseded, year * 3, now); + kept.metadata + .labels + .get_or_insert_with(Default::default) + .insert("pgroles.io/keep".to_string(), "true".to_string()); + plans.push(kept); + + let evicted = evicted_names(&plans, retention, now); + assert!(!evicted.iter().any(|name| name.starts_with("live-"))); + assert!(!evicted.contains(&"kept".to_string())); + assert_eq!(evicted.len(), 40 - retention.superseded); + } + fn plan_failed_at(phase: PlanPhase, age_secs: i64, now_ts: i64) -> PostgresPolicyPlan { let mut plan = PostgresPolicyPlan::new("plan", test_plan_spec()); plan.status = Some(PostgresPolicyPlanStatus { diff --git a/docs/src/pages/docs/operator-candidates.md b/docs/src/pages/docs/operator-candidates.md index 63c61fb8..0c7da5b9 100644 --- a/docs/src/pages/docs/operator-candidates.md +++ b/docs/src/pages/docs/operator-candidates.md @@ -410,6 +410,32 @@ are pruned by the same bounded retention loop as plans; label a candidate `pgroles.io/keep=true` to exempt it. Plans also expire after a TTL — an approval is not an indefinite authorisation. +### What plan retention keeps + +Terminal plans are bounded per phase, not as one pool, because the phases are +not worth the same and the cheapest one is generated fastest. Every replan +supersedes its predecessor, so under a single bound `Superseded` records — of +plans that never ran — would evict the `Applied` ones that record what did. + +| Phase | Retained | Notes | +| --- | --- | --- | +| `Applied` | 25, and never fewer than 30 days' worth | Hard ceiling of 200 | +| `Failed`, `Rejected` | 10, shared | Why something did not run, and who declined it | +| `Superseded` | 3 | Enough to see what a replan replaced | +| `Pending`, `Approved`, `Applying` | all | Still live; never evicted | + +The oldest go first within each bucket. `Applied` additionally has an age +floor: a plan inside it is kept even once the count is exceeded, so the audit +trail spans a stated period instead of however long the policy's churn rate +happens to make it. The ceiling overrides the floor — the floor is a promise +about history, not a licence to keep everything — and a policy applying hard +enough to reach 200 within the floor period will start losing its oldest. + +`pgroles.io/keep=true` exempts a plan from every one of these bounds. + +These values are not yet configurable; see +[#194](https://github.com/hardbyte/pgroles/issues/194). + ## Bounding open candidates Retention prunes what is already finished. Two separate bounds apply to From 4ef4af5dc3bcf9f7c8c93722cc001f3f863f8a4c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 04:50:38 +0000 Subject: [PATCH 2/4] feat(operator): make plan retention reachable via operator environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retention bounds pgroles applies per policy were unreachable configurability: max_plans looked like a knob, but nothing ever passed Some, and there was no CRD field, Helm value, or environment variable (#194 item 3). The per-phase split turned that one number into five, so "expose the bound" needed an actual decision. The decision: operator-level environment variables, no CRD fields. Retention caps object growth in the cluster — an operational bound like the open-candidate budget, the candidate TTL, and the EPHEMERAL_ACCESS_* ceilings, all of which are operator-level — not per-policy intent. The per-object need is already served by pgroles.io/keep=true, which is finer-grained than a per-policy number. And five v1alpha1 fields whose interactions users must understand are far harder to remove than env vars, on no user demand. All five are exposed rather than a subset because they are one policy: publishing the count bound while hiding the floor and ceiling would document a promise without its qualifying clauses. PLAN_RETENTION_APPLIED, PLAN_RETENTION_APPLIED_MIN_AGE (same s/m/h syntax as the EPHEMERAL_ACCESS_* durations), PLAN_RETENTION_APPLIED_- CEILING, PLAN_RETENTION_DECIDED, and PLAN_RETENTION_SUPERSEDED resolve once at startup into a PlanRetention carried on OperatorContext to both cleanup call sites. An invalid value refuses startup with the variable named — a CrashLoopBackOff someone sees, instead of retention quietly running with different bounds, discovered when the plan someone wanted is already gone. A ceiling below the count bound is rejected for the same reason: eviction stops at the count first, so it could never take effect. Parsing is a pure from_lookup, so the tests never touch process-global env state. Mutation-checked: ignoring PLAN_RETENTION_APPLIED fails the override test, dropping the ceiling-vs-count validation fails its test, and letting an invalid MIN_AGE fall back to the default fails its test. Documented next to candidate retention in operator-candidates, linked from operator-plan-approval, and listed in the chart's operator.env reference (README regenerated with helm-docs). Closes #194 Claude-Session: https://claude.ai/code/session_01RGdj9MJHTYinybQDE6Zmop --- CHANGELOG.md | 2 +- charts/pgroles-operator/README.md | 2 +- charts/pgroles-operator/values.yaml | 18 +- crates/pgroles-operator/src/candidate.rs | 1 + crates/pgroles-operator/src/context.rs | 13 ++ crates/pgroles-operator/src/ephemeral.rs | 2 +- crates/pgroles-operator/src/main.rs | 29 ++- crates/pgroles-operator/src/plan.rs | 221 +++++++++++++++++- crates/pgroles-operator/src/reconciler.rs | 8 +- docs/src/pages/docs/operator-candidates.md | 23 +- docs/src/pages/docs/operator-plan-approval.md | 5 + 11 files changed, 306 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ef9156e..05e5bb26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **Plan retention is bounded per phase, so replan churn no longer evicts the record of what ran.** Terminal plans were trimmed as one pool of 10 by creation time. `Superseded` is generated churn — every replan supersedes its predecessor — so on an active policy it filled the pool and deleted the `Applied` plans, which are the audit record of what actually executed against the database. The least informative state was evicting the most informative one. The bounds are now `Applied` 25 (never fewer than 30 days' worth, hard ceiling 200), `Failed` and `Rejected` 10 shared, `Superseded` 3; `Pending`, `Approved` and `Applying` are live and never evicted. The age floor makes the retained span a stated period rather than a function of how often a policy applies, and the ceiling stops that promise becoming unbounded growth. `pgroles.io/keep=true` still exempts a plan from every bound. Not yet configurable. (#194) +- **Plan retention is bounded per phase, so replan churn no longer evicts the record of what ran.** Terminal plans were trimmed as one pool of 10 by creation time. `Superseded` is generated churn — every replan supersedes its predecessor — so on an active policy it filled the pool and deleted the `Applied` plans, which are the audit record of what actually executed against the database. The least informative state was evicting the most informative one. The bounds are now `Applied` 25 (never fewer than 30 days' worth, hard ceiling 200), `Failed` and `Rejected` 10 shared, `Superseded` 3; `Pending`, `Approved` and `Applying` are live and never evicted. The age floor makes the retained span a stated period rather than a function of how often a policy applies, and the ceiling stops that promise becoming unbounded growth. `pgroles.io/keep=true` still exempts a plan from every bound. Each bound is operator-level configuration — `PLAN_RETENTION_APPLIED`, `PLAN_RETENTION_APPLIED_MIN_AGE`, `PLAN_RETENTION_APPLIED_CEILING`, `PLAN_RETENTION_DECIDED`, `PLAN_RETENTION_SUPERSEDED` on the operator environment, replacing the `max_plans` parameter that nothing could ever set — and an invalid value refuses operator startup with the variable named. Deliberately not a `PostgresPolicy` field: retention caps object growth in the cluster, and the per-object need is what the `keep` label is for. (#194) - **`spec.mode: plan` is renamed to `spec.mode: observe`, with a deprecation window.** "Plan" now names exactly one thing, the `PostgresPolicyPlan` resource; the `ApprovalIgnored` reason `PlanModeNeverExecutes` is now `ObserveModeNeverExecutes`. The old value keeps working: `mode: plan` stays an accepted schema value with identical behaviour, so a GitOps controller re-applying an existing manifest is unaffected by the upgrade. A policy using it reports a `ModeValueDeprecated` condition, warns in the operator log, and counts toward `pgroles.deprecated.mode_plan`. **Upgrade:** change `mode: plan` to `mode: observe` in your manifests at your convenience — a future release removes the `plan` value, and that removal will be the breaking change. diff --git a/charts/pgroles-operator/README.md b/charts/pgroles-operator/README.md index f99f975b..d90fa5e1 100644 --- a/charts/pgroles-operator/README.md +++ b/charts/pgroles-operator/README.md @@ -65,7 +65,7 @@ these policies, pinned to the `pgroles-system` namespace and the | fullnameOverride | string | `""` | Replaces the full generated name in resource names. | | nameOverride | string | `""` | Replaces the generated name in resource names, keeping the release prefix. | | operator.affinity | object | `{}` | Affinity rules for operator pod scheduling. | -| operator.env | list | `[{"name":"RUST_LOG","value":"info,pgroles_operator=debug"}]` | Additional environment variables for the operator container. This is also where operator-wide settings live, since the operator is configured by environment rather than by flags. Notable variables: `OTEL_EXPORTER_OTLP_ENDPOINT` enables OTLP metrics and logs, including structured ephemeral-access audit events; `OTEL_LOGS_EXPORTER=none` disables the log half when another agent already ships container logs off-cluster; `EPHEMERAL_ACCESS_MAXIMUM_DURATION` (default `24h`) and `EPHEMERAL_ACCESS_MAX_PENDING_TTL` (default `1h`) are the only cluster-wide ceilings on ephemeral access, and an access policy exceeding either is rejected with `Accepted=False`. | +| operator.env | list | `[{"name":"RUST_LOG","value":"info,pgroles_operator=debug"}]` | Additional environment variables for the operator container. This is also where operator-wide settings live, since the operator is configured by environment rather than by flags. Notable variables: `OTEL_EXPORTER_OTLP_ENDPOINT` enables OTLP metrics and logs, including structured ephemeral-access audit events; `OTEL_LOGS_EXPORTER=none` disables the log half when another agent already ships container logs off-cluster; `EPHEMERAL_ACCESS_MAXIMUM_DURATION` (default `24h`) and `EPHEMERAL_ACCESS_MAX_PENDING_TTL` (default `1h`) are the only cluster-wide ceilings on ephemeral access, and an access policy exceeding either is rejected with `Accepted=False`; the `PLAN_RETENTION_*` variables bound how many terminal plans are kept per policy — `PLAN_RETENTION_APPLIED` (default `25`), `PLAN_RETENTION_APPLIED_MIN_AGE` (default `720h`), `PLAN_RETENTION_APPLIED_CEILING` (default `200`), `PLAN_RETENTION_DECIDED` (default `10`, Failed and Rejected shared) and `PLAN_RETENTION_SUPERSEDED` (default `3`) — and an invalid value refuses operator startup with the variable named. | | operator.http.port | int | `8080` | Port serving the `/livez` and `/readyz` probes. | | operator.image.pullPolicy | string | `"IfNotPresent"` | Image pull policy for the operator container. | | operator.image.repository | string | `"ghcr.io/hardbyte/pgroles-operator"` | Operator container image repository. | diff --git a/charts/pgroles-operator/values.yaml b/charts/pgroles-operator/values.yaml index 38d26ae6..0f7def71 100644 --- a/charts/pgroles-operator/values.yaml +++ b/charts/pgroles-operator/values.yaml @@ -69,7 +69,13 @@ operator: # `EPHEMERAL_ACCESS_MAXIMUM_DURATION` (default `24h`) and # `EPHEMERAL_ACCESS_MAX_PENDING_TTL` (default `1h`) are the only cluster-wide # ceilings on ephemeral access, and an access policy exceeding either is - # rejected with `Accepted=False`. + # rejected with `Accepted=False`; the `PLAN_RETENTION_*` variables bound how + # many terminal plans are kept per policy — `PLAN_RETENTION_APPLIED` (default + # `25`), `PLAN_RETENTION_APPLIED_MIN_AGE` (default `720h`), + # `PLAN_RETENTION_APPLIED_CEILING` (default `200`), `PLAN_RETENTION_DECIDED` + # (default `10`, Failed and Rejected shared) and `PLAN_RETENTION_SUPERSEDED` + # (default `3`) — and an invalid value refuses operator startup with the + # variable named. env: - name: RUST_LOG value: "info,pgroles_operator=debug" @@ -83,6 +89,16 @@ operator: # value: 24h # - name: EPHEMERAL_ACCESS_MAX_PENDING_TTL # value: 1h + # - name: PLAN_RETENTION_APPLIED + # value: "25" + # - name: PLAN_RETENTION_APPLIED_MIN_AGE + # value: 720h + # - name: PLAN_RETENTION_APPLIED_CEILING + # value: "200" + # - name: PLAN_RETENTION_DECIDED + # value: "10" + # - name: PLAN_RETENTION_SUPERSEDED + # value: "3" # -- Annotations to add to the operator pod. podAnnotations: {} diff --git a/crates/pgroles-operator/src/candidate.rs b/crates/pgroles-operator/src/candidate.rs index 2a72f771..9b15c37d 100644 --- a/crates/pgroles-operator/src/candidate.rs +++ b/crates/pgroles-operator/src/candidate.rs @@ -742,6 +742,7 @@ async fn plan_against_target( target.target_identity(), &summary, &password_source_versions, + ctx.plan_retention, Some(CandidatePlanBinding { candidate, content_digest: &content_digest, diff --git a/crates/pgroles-operator/src/context.rs b/crates/pgroles-operator/src/context.rs index 1fcc53a0..b00ec176 100644 --- a/crates/pgroles-operator/src/context.rs +++ b/crates/pgroles-operator/src/context.rs @@ -16,6 +16,7 @@ use tokio::sync::{Mutex, RwLock}; use crate::crd::{ConnectionAuth, ConnectionSpec, SecretKeySelector}; use crate::observability::OperatorObservability; +use crate::plan::PlanRetention; use crate::request_index::RequestIndex; /// Minimum pool size required for reconciliation. @@ -414,6 +415,9 @@ pub struct OperatorContext { /// Optional namespace which bounds every operator watch and list. pub watch_namespace: Option, + /// Terminal-plan retention bounds, shared by every policy reconcile. + pub plan_retention: PlanRetention, + /// Fetches short-lived provider-backed database passwords. gcp_token_provider: Arc, } @@ -435,11 +439,20 @@ impl OperatorContext { observability, request_index, watch_namespace, + plan_retention: PlanRetention::default(), database_locks: Arc::new(Mutex::new(HashMap::new())), gcp_token_provider: Arc::new(MetadataGcpAccessTokenProvider::default()), } } + /// Replace the default plan retention bounds — typically with the + /// environment-derived values resolved once at startup by + /// [`PlanRetention::from_env`]. + pub fn with_plan_retention(mut self, plan_retention: PlanRetention) -> Self { + self.plan_retention = plan_retention; + self + } + /// Try to acquire the in-process lock for the given database identity. /// /// Returns `Some(guard)` if no other reconcile is in progress for this diff --git a/crates/pgroles-operator/src/ephemeral.rs b/crates/pgroles-operator/src/ephemeral.rs index f3a0aed2..15e200f3 100644 --- a/crates/pgroles-operator/src/ephemeral.rs +++ b/crates/pgroles-operator/src/ephemeral.rs @@ -451,7 +451,7 @@ fn set_condition( conditions.push(condition); } -fn parse_duration(value: &str) -> Result { +pub(crate) fn parse_duration(value: &str) -> Result { let value = value.trim(); if value.is_empty() { return Err(EphemeralError::Invalid( diff --git a/crates/pgroles-operator/src/main.rs b/crates/pgroles-operator/src/main.rs index eedf8a0b..f576cc5a 100644 --- a/crates/pgroles-operator/src/main.rs +++ b/crates/pgroles-operator/src/main.rs @@ -27,6 +27,7 @@ use pgroles_operator::ephemeral::{ use pgroles_operator::observability::{ OperatorObservability, init_log_provider_from_env, serve_health, }; +use pgroles_operator::plan::PlanRetention; use pgroles_operator::reconciler::{error_policy, reconcile}; use pgroles_operator::request_index::RequestIndex; @@ -140,14 +141,28 @@ async fn main() -> anyhow::Result<()> { } let request_index = RequestIndex::default(); + // Resolved once here so a malformed value refuses startup with the exact + // variable named, instead of surfacing later as retention quietly running + // with bounds the environment did not ask for. + let plan_retention = PlanRetention::from_env()?; + if plan_retention != PlanRetention::default() { + info!( + ?plan_retention, + "plan retention bounds set from environment" + ); + } + // Create the shared operator context. - let ctx = Arc::new(OperatorContext::new_with_runtime_config( - client.clone(), - observability.clone(), - event_recorder, - request_index.clone(), - watch_namespace.clone(), - )); + let ctx = Arc::new( + OperatorContext::new_with_runtime_config( + client.clone(), + observability.clone(), + event_recorder, + request_index.clone(), + watch_namespace.clone(), + ) + .with_plan_retention(plan_retention), + ); // Watch all PostgresPolicy resources across all namespaces. let policies: Api = match &watch_namespace { diff --git a/crates/pgroles-operator/src/plan.rs b/crates/pgroles-operator/src/plan.rs index f7460376..9bbed7cd 100644 --- a/crates/pgroles-operator/src/plan.rs +++ b/crates/pgroles-operator/src/plan.rs @@ -91,6 +91,12 @@ const CLEANUP_TIMEOUT_SECS: u64 = 5; /// churn: every replan supersedes its predecessor, so on an active policy a /// single pool fills with records of plans that never ran and evicts the /// `Applied` ones, which are the record of what did. +/// +/// The bounds are operator-level configuration: each field can be overridden +/// by its `PLAN_RETENTION_*` environment variable (see [`Self::from_env`]). +/// They are deliberately not per-policy CRD fields — retention caps object +/// growth in the cluster, it is not policy intent, and the per-object need is +/// served by the `pgroles.io/keep=true` label. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct PlanRetention { /// `Applied` plans kept once they are older than `applied_min_age_secs`. @@ -125,6 +131,126 @@ impl Default for PlanRetention { } } +impl PlanRetention { + /// Overrides [`PlanRetention::applied`]. + pub const ENV_APPLIED: &'static str = "PLAN_RETENTION_APPLIED"; + /// Overrides [`PlanRetention::applied_ceiling`]. + pub const ENV_APPLIED_CEILING: &'static str = "PLAN_RETENTION_APPLIED_CEILING"; + /// Overrides [`PlanRetention::applied_min_age_secs`]. Takes the same + /// `s`/`m`/`h` duration syntax as the `EPHEMERAL_ACCESS_*` ceilings. + pub const ENV_APPLIED_MIN_AGE: &'static str = "PLAN_RETENTION_APPLIED_MIN_AGE"; + /// Overrides [`PlanRetention::decided`]. + pub const ENV_DECIDED: &'static str = "PLAN_RETENTION_DECIDED"; + /// Overrides [`PlanRetention::superseded`]. + pub const ENV_SUPERSEDED: &'static str = "PLAN_RETENTION_SUPERSEDED"; + + /// Resolve the retention bounds from the process environment. + /// + /// Unset variables keep their defaults. An invalid value is an error, not + /// a fallback: this is meant to be called once at startup, where refusing + /// to start is the only failure mode an operator of the operator will + /// actually see. Retention that silently ran with different bounds than + /// the environment asked for would only be discovered when the plan + /// someone wanted was already deleted. + pub fn from_env() -> Result { + Self::from_lookup(|variable| std::env::var(variable).ok()) + } + + /// [`Self::from_env`] over an arbitrary lookup, so parsing and validation + /// are testable without mutating process-global environment state. + fn from_lookup( + lookup: impl Fn(&str) -> Option, + ) -> Result { + let defaults = Self::default(); + let retention = Self { + applied: count_bound(&lookup, Self::ENV_APPLIED, defaults.applied)?, + applied_ceiling: count_bound( + &lookup, + Self::ENV_APPLIED_CEILING, + defaults.applied_ceiling, + )?, + applied_min_age_secs: age_bound( + &lookup, + Self::ENV_APPLIED_MIN_AGE, + defaults.applied_min_age_secs, + )?, + decided: count_bound(&lookup, Self::ENV_DECIDED, defaults.decided)?, + superseded: count_bound(&lookup, Self::ENV_SUPERSEDED, defaults.superseded)?, + }; + // A ceiling below the count bound could never take effect — eviction + // already stops at the count — so the configuration says one thing + // and the operator would do another. Reject it instead. + if retention.applied_ceiling < retention.applied { + return Err(PlanRetentionConfigError::CeilingBelowCount { + ceiling: retention.applied_ceiling, + count: retention.applied, + }); + } + Ok(retention) + } +} + +/// A count bound from `lookup`, or `default` when the variable is unset. +fn count_bound( + lookup: impl Fn(&str) -> Option, + variable: &'static str, + default: usize, +) -> Result { + match lookup(variable) { + None => Ok(default), + Some(value) => value + .trim() + .parse() + .map_err(|_| PlanRetentionConfigError::InvalidCount { variable, value }), + } +} + +/// An age bound in seconds from `lookup`, or `default` when the variable is +/// unset. The value uses the same duration syntax as the `EPHEMERAL_ACCESS_*` +/// variables. +fn age_bound( + lookup: impl Fn(&str) -> Option, + variable: &'static str, + default: i64, +) -> Result { + let Some(value) = lookup(variable) else { + return Ok(default); + }; + let invalid = |detail: String| PlanRetentionConfigError::InvalidDuration { + variable, + value: value.clone(), + detail, + }; + let duration = crate::ephemeral::parse_duration(&value).map_err(|error| { + invalid(match error { + crate::ephemeral::EphemeralError::Invalid(detail) => detail, + other => other.to_string(), + }) + })?; + i64::try_from(duration.as_secs()).map_err(|_| invalid("duration is too large".to_string())) +} + +/// Why [`PlanRetention::from_env`] refused the environment's values. +#[derive(Debug, thiserror::Error)] +pub enum PlanRetentionConfigError { + #[error("{variable} must be a non-negative integer, got {value:?}")] + InvalidCount { + variable: &'static str, + value: String, + }, + #[error("{variable} is not a valid duration ({detail}), got {value:?}")] + InvalidDuration { + variable: &'static str, + value: String, + detail: String, + }, + #[error( + "PLAN_RETENTION_APPLIED_CEILING ({ceiling}) is below PLAN_RETENTION_APPLIED ({count}); \ + a ceiling under the count bound can never take effect" + )] + CeilingBelowCount { ceiling: usize, count: usize }, +} + /// How recently a Failed plan must have been created (in seconds) for the /// dedup check to consider it a match. Plans older than this are ignored so /// that retries after the user fixes the environment are not blocked. @@ -451,6 +577,7 @@ pub async fn create_or_update_plan( target_identity: &TargetIdentity, change_summary: &ChangeSummary, password_source_versions: &BTreeMap, + plan_retention: PlanRetention, candidate: Option>, ) -> Result { let namespace = policy.namespace().ok_or(ReconcileError::NoNamespace)?; @@ -507,7 +634,7 @@ pub async fn create_or_update_plan( // Candidate plans are pruned by candidate retention (their owner cascades), // never by the policy's plan retention, which would not see them anyway. if candidate.is_none() { - cleanup_old_plans_best_effort(client, policy, None).await; + cleanup_old_plans_best_effort(client, policy, plan_retention).await; } let plans_api: Api = Api::namespaced(client.clone(), &namespace); @@ -1324,7 +1451,7 @@ pub(crate) async fn execute_changes_in_transaction( pub async fn cleanup_old_plans_best_effort( client: &Client, policy: &PostgresPolicy, - retention: Option, + retention: PlanRetention, ) { match tokio::time::timeout( Duration::from_secs(CLEANUP_TIMEOUT_SECS), @@ -1350,11 +1477,10 @@ pub async fn cleanup_old_plans_best_effort( pub async fn cleanup_old_plans( client: &Client, policy: &PostgresPolicy, - retention: Option, + retention: PlanRetention, ) -> Result<(), ReconcileError> { let namespace = policy.namespace().ok_or(ReconcileError::NoNamespace)?; let policy_name = policy.name_any(); - let retention = retention.unwrap_or_default(); let plans_api: Api = Api::namespaced(client.clone(), &namespace); let selector = policy_selector(&policy_name); @@ -2718,6 +2844,93 @@ mod tests { assert_eq!(evicted.len(), 40 - retention.superseded); } + /// [`PlanRetention::from_lookup`] over a fixed variable table. + fn retention_from(vars: &[(&str, &str)]) -> Result { + PlanRetention::from_lookup(|variable| { + vars.iter() + .find(|(name, _)| *name == variable) + .map(|(_, value)| value.to_string()) + }) + } + + #[test] + fn unset_retention_variables_keep_the_defaults() { + assert_eq!( + retention_from(&[]).expect("an empty environment is valid"), + PlanRetention::default() + ); + } + + #[test] + fn each_retention_variable_overrides_its_bound() { + let resolved = retention_from(&[ + (PlanRetention::ENV_APPLIED, "7"), + (PlanRetention::ENV_APPLIED_CEILING, "70"), + (PlanRetention::ENV_APPLIED_MIN_AGE, "36h"), + (PlanRetention::ENV_DECIDED, "4"), + (PlanRetention::ENV_SUPERSEDED, "1"), + ]) + .expect("all five values are valid"); + + let expected = PlanRetention { + applied: 7, + applied_ceiling: 70, + applied_min_age_secs: 36 * 60 * 60, + decided: 4, + superseded: 1, + }; + // Guard against this test going vacuous: if the fixture ever equalled + // the defaults, a lookup that ignored the environment entirely would + // still pass the assertion below. + assert_ne!(expected, PlanRetention::default()); + assert_eq!(resolved, expected); + } + + #[test] + fn an_invalid_retention_count_is_rejected_naming_the_variable() { + let error = retention_from(&[(PlanRetention::ENV_DECIDED, "many")]) + .expect_err("a non-numeric count must be rejected, not defaulted"); + assert!( + error.to_string().contains(PlanRetention::ENV_DECIDED), + "the error must name the variable to fix: {error}" + ); + } + + #[test] + fn an_invalid_retention_min_age_is_rejected_naming_the_variable() { + // Days are deliberately not a unit — this is the same syntax as the + // EPHEMERAL_ACCESS_* durations, and "30d" must fail loudly rather + // than resolve to something else. + let error = retention_from(&[(PlanRetention::ENV_APPLIED_MIN_AGE, "30d")]) + .expect_err("an unsupported duration unit must be rejected, not defaulted"); + assert!( + error + .to_string() + .contains(PlanRetention::ENV_APPLIED_MIN_AGE), + "the error must name the variable to fix: {error}" + ); + } + + #[test] + fn a_ceiling_below_the_applied_count_is_rejected() { + // Eviction stops at the count bound before the ceiling is consulted, + // so a smaller ceiling could never take effect; accepting it would + // mean running with different bounds than the environment states. + retention_from(&[ + (PlanRetention::ENV_APPLIED, "50"), + (PlanRetention::ENV_APPLIED_CEILING, "10"), + ]) + .expect_err("a ceiling below the count bound must be rejected"); + + // Equal is the degenerate-but-coherent form: the floor never spares + // anything, and that is exactly what the configuration says. + retention_from(&[ + (PlanRetention::ENV_APPLIED, "50"), + (PlanRetention::ENV_APPLIED_CEILING, "50"), + ]) + .expect("a ceiling equal to the count bound is valid"); + } + fn plan_failed_at(phase: PlanPhase, age_secs: i64, now_ts: i64) -> PostgresPolicyPlan { let mut plan = PostgresPolicyPlan::new("plan", test_plan_spec()); plan.status = Some(PostgresPolicyPlanStatus { diff --git a/crates/pgroles-operator/src/reconciler.rs b/crates/pgroles-operator/src/reconciler.rs index d63c5661..ee8cbdeb 100644 --- a/crates/pgroles-operator/src/reconciler.rs +++ b/crates/pgroles-operator/src/reconciler.rs @@ -1103,7 +1103,8 @@ async fn reconcile_apply_inner( // Release advisory lock (always, even on error). advisory_lock.release().await; - crate::plan::cleanup_old_plans_best_effort(&ctx.kube_client, resource, None).await; + crate::plan::cleanup_old_plans_best_effort(&ctx.kube_client, resource, ctx.plan_retention) + .await; result } @@ -1411,6 +1412,7 @@ async fn apply_under_lock( &target_identity, &summary, &applied_password_source_versions, + ctx.plan_retention, None, ) .await?; @@ -1530,6 +1532,7 @@ async fn apply_under_lock( &target_identity, &summary, &applied_password_source_versions, + ctx.plan_retention, None, ) .await?; @@ -1866,6 +1869,7 @@ async fn apply_under_lock( &target_identity, &summary, &applied_password_source_versions, + ctx.plan_retention, None, ) .await?; @@ -2197,6 +2201,7 @@ async fn apply_under_lock( &target_identity, &summary, &applied_password_source_versions, + ctx.plan_retention, None, ) .await?; @@ -2340,6 +2345,7 @@ async fn apply_under_lock( &target_identity, &summary, &applied_password_source_versions, + ctx.plan_retention, None, ) .await?; diff --git a/docs/src/pages/docs/operator-candidates.md b/docs/src/pages/docs/operator-candidates.md index 0c7da5b9..b94b7abf 100644 --- a/docs/src/pages/docs/operator-candidates.md +++ b/docs/src/pages/docs/operator-candidates.md @@ -433,8 +433,27 @@ enough to reach 200 within the floor period will start losing its oldest. `pgroles.io/keep=true` exempts a plan from every one of these bounds. -These values are not yet configurable; see -[#194](https://github.com/hardbyte/pgroles/issues/194). +### Configuring the bounds + +Each bound is operator-level configuration, set by environment variable on the +operator Deployment (`operator.env` in the Helm chart) — the same mechanism as +the `EPHEMERAL_ACCESS_*` ceilings, and the same `s`/`m`/`h` duration syntax: + +| Variable | Default | Sets | +| --- | --- | --- | +| `PLAN_RETENTION_APPLIED` | `25` | `Applied` count bound | +| `PLAN_RETENTION_APPLIED_MIN_AGE` | `720h` (30 days) | `Applied` age floor | +| `PLAN_RETENTION_APPLIED_CEILING` | `200` | `Applied` hard ceiling; must be at least `PLAN_RETENTION_APPLIED` | +| `PLAN_RETENTION_DECIDED` | `10` | `Failed` + `Rejected` shared bound | +| `PLAN_RETENTION_SUPERSEDED` | `3` | `Superseded` bound | + +An invalid value refuses operator startup with the variable named, rather than +silently running with bounds the environment did not ask for. + +There is deliberately no per-policy retention field on `PostgresPolicy`. +Retention caps object growth in the cluster — an operational concern, like the +open-candidate budget and TTL above — not per-policy intent; the per-object +need ("this specific record matters") is what `pgroles.io/keep=true` is for. ## Bounding open candidates diff --git a/docs/src/pages/docs/operator-plan-approval.md b/docs/src/pages/docs/operator-plan-approval.md index aff9b12b..1a438f82 100644 --- a/docs/src/pages/docs/operator-plan-approval.md +++ b/docs/src/pages/docs/operator-plan-approval.md @@ -255,6 +255,11 @@ For `password.generate` roles the operator synthesizes material in memory while planning and creates the Kubernetes Secret only during post-approval execution — see [Passwords and planning](#passwords-and-planning). +Terminal plans are not kept forever: per-phase bounds prune the oldest, with +`Applied` plans kept the longest. See [what plan retention +keeps](/docs/operator-candidates#what-plan-retention-keeps) for the bounds, +how to configure them, and the `pgroles.io/keep=true` exemption. + ### Who may decide The trust model has two layers, and both matter: From 581d2ee658522853c3a8effa02f96079214a6b75 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 05:02:48 +0000 Subject: [PATCH 3/4] fix(operator): anchor the applied retention floor to appliedAt and refuse non-Unicode config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both verified against the code. The Applied age floor measured a plan's age from creationTimestamp, but a plan can sit Pending or Approved for arbitrarily long before a reviewer decides it. A plan approved after the floor period read as already outside the floor at the moment it executed, so the cleanup that runs right after execution could delete it — the exact history the floor promises. The floor now runs from status.appliedAt, falling back to the creation timestamp when it is absent or unparseable so older records keep the previous behaviour rather than reading as infinitely old. from_env read variables with std::env::var(..).ok(), which maps VarError::NotUnicode to None — a set-but-malformed value silently took the default, the exact failure the startup validation exists to prevent. A non-Unicode value now refuses startup naming the variable, like every other invalid value. Mutation-checked: re-anchoring the floor to creation time fails the appliedAt test; degrading NotUnicode to unset fails the env_read test. Refs #194 Claude-Session: https://claude.ai/code/session_01RGdj9MJHTYinybQDE6Zmop --- crates/pgroles-operator/src/plan.rs | 127 ++++++++++++++++++++- docs/src/pages/docs/operator-candidates.md | 8 +- 2 files changed, 130 insertions(+), 5 deletions(-) diff --git a/crates/pgroles-operator/src/plan.rs b/crates/pgroles-operator/src/plan.rs index 9bbed7cd..e5dd2924 100644 --- a/crates/pgroles-operator/src/plan.rs +++ b/crates/pgroles-operator/src/plan.rs @@ -153,7 +153,19 @@ impl PlanRetention { /// the environment asked for would only be discovered when the plan /// someone wanted was already deleted. pub fn from_env() -> Result { - Self::from_lookup(|variable| std::env::var(variable).ok()) + let mut resolved = std::collections::BTreeMap::new(); + for variable in [ + Self::ENV_APPLIED, + Self::ENV_APPLIED_CEILING, + Self::ENV_APPLIED_MIN_AGE, + Self::ENV_DECIDED, + Self::ENV_SUPERSEDED, + ] { + if let Some(value) = env_read(variable, std::env::var(variable))? { + resolved.insert(variable, value); + } + } + Self::from_lookup(|variable| resolved.get(variable).cloned()) } /// [`Self::from_env`] over an arbitrary lookup, so parsing and validation @@ -190,6 +202,25 @@ impl PlanRetention { } } +/// Classify one raw environment read: present, absent, or refused. +/// +/// `NotUnicode` is the one `VarError` that must not degrade to "unset": a +/// variable someone set, however malformed, silently taking the default is +/// exactly the failure mode this configuration's startup validation exists +/// to prevent. +fn env_read( + variable: &'static str, + read: Result, +) -> Result, PlanRetentionConfigError> { + match read { + Ok(value) => Ok(Some(value)), + Err(std::env::VarError::NotPresent) => Ok(None), + Err(std::env::VarError::NotUnicode(_)) => { + Err(PlanRetentionConfigError::NotUnicode { variable }) + } + } +} + /// A count bound from `lookup`, or `default` when the variable is unset. fn count_bound( lookup: impl Fn(&str) -> Option, @@ -249,6 +280,8 @@ pub enum PlanRetentionConfigError { a ceiling under the count bound can never take effect" )] CeilingBelowCount { ceiling: usize, count: usize }, + #[error("{variable} is set to a value that is not valid Unicode")] + NotUnicode { variable: &'static str }, } /// How recently a Failed plan must have been created (in seconds) for the @@ -1670,7 +1703,7 @@ fn plans_to_evict( if remaining <= retention.applied { break; } - let inside_floor = plan_age_secs(plan, now_ts) < retention.applied_min_age_secs; + let inside_floor = applied_age_secs(plan, now_ts) < retention.applied_min_age_secs; if inside_floor && remaining <= retention.applied_ceiling { continue; } @@ -1702,6 +1735,23 @@ fn sort_oldest_first(plans: &mut [&PostgresPolicyPlan]) { }); } +/// Age of an `Applied` plan for the retention floor, measured from +/// `status.appliedAt`. The floor promises post-application history, and a +/// plan can sit `Pending` or `Approved` for arbitrarily long before a +/// reviewer decides it — measured from creation, a plan approved late would +/// read as already outside the floor and be evicted by the cleanup that runs +/// right after it executes. Falls back to [`plan_age_secs`] when `appliedAt` +/// is absent or unparseable, so plans recorded before it was set keep the +/// creation-time behaviour rather than reading as infinitely old. +fn applied_age_secs(plan: &PostgresPolicyPlan, now_ts: i64) -> i64 { + plan.status + .as_ref() + .and_then(|status| status.applied_at.as_deref()) + .and_then(parse_rfc3339_epoch_secs) + .map(|applied_ts| now_ts.saturating_sub(applied_ts)) + .unwrap_or_else(|| plan_age_secs(plan, now_ts)) +} + /// Age in seconds, or 0 for a plan with no creation timestamp — treating an /// unknown age as brand new keeps it, which is the safe direction for a /// deletion decision. @@ -2911,6 +2961,79 @@ mod tests { ); } + #[test] + fn the_applied_floor_runs_from_when_the_plan_applied_not_when_it_was_created() { + // A plan can sit Pending or Approved for longer than the entire floor + // before a reviewer decides it. Measured from creation, such a plan is + // already outside the floor the moment it executes, and the cleanup + // that follows execution deletes it — the exact history the floor + // promises to keep. + let now = 1_700_000_000; + let retention = PlanRetention::default(); + + // Created far outside the floor, applied just now. Enough plans to + // exceed the count bound, but well under the ceiling, so only the + // floor can be what spares them. + let excess = 10; + assert!( + retention.applied + excess <= retention.applied_ceiling, + "this test must not lean on the ceiling to pass" + ); + let plans: Vec = (0..retention.applied + excess) + .map(|i| { + let mut plan = retained_plan( + &format!("applied-{i:03}"), + PlanPhase::Applied, + retention.applied_min_age_secs * 3 + i as i64, + now, + ); + plan.status + .as_mut() + .expect("retained_plan always sets a status") + .applied_at = Some( + jiff::Timestamp::from_second(now - 60 - i as i64) + .expect("epoch second in range") + .to_string(), + ); + plan + }) + .collect(); + + assert!( + evicted_names(&plans, retention, now).is_empty(), + "a plan applied inside the floor is kept, however old the object is" + ); + } + + #[test] + fn a_non_unicode_environment_value_is_rejected_not_defaulted() { + use std::os::unix::ffi::OsStringExt; + + let read = Err(std::env::VarError::NotUnicode( + std::ffi::OsString::from_vec(vec![b'2', b'5', 0xff]), + )); + let error = env_read(PlanRetention::ENV_APPLIED, read) + .expect_err("a set-but-non-Unicode value must be rejected, not read as unset"); + assert!( + error.to_string().contains(PlanRetention::ENV_APPLIED), + "the error must name the variable to fix: {error}" + ); + + assert_eq!( + env_read( + PlanRetention::ENV_APPLIED, + Err(std::env::VarError::NotPresent) + ) + .expect("absent is not an error"), + None + ); + assert_eq!( + env_read(PlanRetention::ENV_APPLIED, Ok("25".to_string())) + .expect("a Unicode value is not an error"), + Some("25".to_string()) + ); + } + #[test] fn a_ceiling_below_the_applied_count_is_rejected() { // Eviction stops at the count bound before the ceiling is consulted, diff --git a/docs/src/pages/docs/operator-candidates.md b/docs/src/pages/docs/operator-candidates.md index b94b7abf..02cd8c1e 100644 --- a/docs/src/pages/docs/operator-candidates.md +++ b/docs/src/pages/docs/operator-candidates.md @@ -425,9 +425,11 @@ plans that never ran — would evict the `Applied` ones that record what did. | `Pending`, `Approved`, `Applying` | all | Still live; never evicted | The oldest go first within each bucket. `Applied` additionally has an age -floor: a plan inside it is kept even once the count is exceeded, so the audit -trail spans a stated period instead of however long the policy's churn rate -happens to make it. The ceiling overrides the floor — the floor is a promise +floor, measured from `status.appliedAt` — not from creation, since a plan can +wait on a reviewer for arbitrarily long before it executes: a plan applied +inside the floor is kept even once the count is exceeded, so the audit trail +spans a stated period instead of however long the policy's churn rate happens +to make it. The ceiling overrides the floor — the floor is a promise about history, not a licence to keep everything — and a policy applying hard enough to reach 200 within the floor period will start losing its oldest. From e544fc477890023c53bc44c91de59f17ba2b5459 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 06:12:41 +0000 Subject: [PATCH 4/4] fix(operator): one lifecycle model for plan and candidate retention Two gaps in the retention contract, both reported on review. Candidate-owned Applied plans were outside it. A plan derived from a candidate is owned by that candidate, so it is excluded from policy plan cleanup, and cleanup_terminal_candidates bounded terminal candidates at a flat 10 by creation time. Deleting one cascades to its plan and SQL ConfigMap, so the eleventh terminal proposal could delete a minutes-old Applied plan straight through the advertised bounds. A keep label on the plan was inert against it: the cascade takes the child when the parent goes. Pruning now decides on the candidate-and-plans pair. A candidate owning an Applied plan is held to the same PlanRetention bounds as any other applied plan; everything else is proposal churn on the flat bound. A keep label on either object exempts the pair. When the plans cannot be read the pass prunes nothing rather than cascade blind. The Applied ceiling also evicted by creation time while the age floor measured from appliedAt, so above the ceiling a plan created before a long review but applied moments ago sorted first and could be deleted by the cleanup that runs right after it executes. Both now read applied_epoch_secs, so one notion of when a plan applied drives the floor and the order. Mutation-checked: sorting by creation fails above_the_ceiling_eviction_order_follows_when_plans_applied_not_when_created; dropping the child-plan keep fails a_keep_label_on_the_child_plan_protects_the_candidate; treating Applied-owning candidates as churn fails that and proposal_churn_cannot_prune_a_promoted_candidate_with_a_fresh_applied_plan. Refs #194 Claude-Session: https://claude.ai/code/session_01RGdj9MJHTYinybQDE6Zmop --- CHANGELOG.md | 2 +- crates/pgroles-operator/src/candidate.rs | 291 ++++++++++++++++++++- crates/pgroles-operator/src/plan.rs | 130 +++++++-- docs/src/pages/docs/operator-candidates.md | 32 ++- 4 files changed, 402 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05e5bb26..bd736ecf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **Plan retention is bounded per phase, so replan churn no longer evicts the record of what ran.** Terminal plans were trimmed as one pool of 10 by creation time. `Superseded` is generated churn — every replan supersedes its predecessor — so on an active policy it filled the pool and deleted the `Applied` plans, which are the audit record of what actually executed against the database. The least informative state was evicting the most informative one. The bounds are now `Applied` 25 (never fewer than 30 days' worth, hard ceiling 200), `Failed` and `Rejected` 10 shared, `Superseded` 3; `Pending`, `Approved` and `Applying` are live and never evicted. The age floor makes the retained span a stated period rather than a function of how often a policy applies, and the ceiling stops that promise becoming unbounded growth. `pgroles.io/keep=true` still exempts a plan from every bound. Each bound is operator-level configuration — `PLAN_RETENTION_APPLIED`, `PLAN_RETENTION_APPLIED_MIN_AGE`, `PLAN_RETENTION_APPLIED_CEILING`, `PLAN_RETENTION_DECIDED`, `PLAN_RETENTION_SUPERSEDED` on the operator environment, replacing the `max_plans` parameter that nothing could ever set — and an invalid value refuses operator startup with the variable named. Deliberately not a `PostgresPolicy` field: retention caps object growth in the cluster, and the per-object need is what the `keep` label is for. (#194) +- **Plan retention is bounded per phase, so replan churn no longer evicts the record of what ran.** Terminal plans were trimmed as one pool of 10 by creation time. `Superseded` is generated churn — every replan supersedes its predecessor — so on an active policy it filled the pool and deleted the `Applied` plans, which are the audit record of what actually executed against the database. The least informative state was evicting the most informative one. The bounds are now `Applied` 25 (never fewer than 30 days' worth, hard ceiling 200), `Failed` and `Rejected` 10 shared, `Superseded` 3; `Pending`, `Approved` and `Applying` are live and never evicted. The age floor makes the retained span a stated period rather than a function of how often a policy applies, and the ceiling stops that promise becoming unbounded growth. `pgroles.io/keep=true` still exempts a plan from every bound. Each bound is operator-level configuration — `PLAN_RETENTION_APPLIED`, `PLAN_RETENTION_APPLIED_MIN_AGE`, `PLAN_RETENTION_APPLIED_CEILING`, `PLAN_RETENTION_DECIDED`, `PLAN_RETENTION_SUPERSEDED` on the operator environment, replacing the `max_plans` parameter that nothing could ever set — and an invalid value refuses operator startup with the variable named. Deliberately not a `PostgresPolicy` field: retention caps object growth in the cluster, and the per-object need is what the `keep` label is for. The `Applied` bounds measure — and order — by `status.appliedAt`, not object creation, so a plan that waited on a reviewer is not already outside its floor the moment it executes. They also govern terminal-candidate pruning: deleting a candidate cascades to the plan it owns, so a promoted candidate owning an `Applied` plan is held to the `Applied` bounds instead of the flat terminal-candidate bound, and `pgroles.io/keep=true` on either the candidate or its plan exempts the pair. (#194) - **`spec.mode: plan` is renamed to `spec.mode: observe`, with a deprecation window.** "Plan" now names exactly one thing, the `PostgresPolicyPlan` resource; the `ApprovalIgnored` reason `PlanModeNeverExecutes` is now `ObserveModeNeverExecutes`. The old value keeps working: `mode: plan` stays an accepted schema value with identical behaviour, so a GitOps controller re-applying an existing manifest is unaffected by the upgrade. A policy using it reports a `ModeValueDeprecated` condition, warns in the operator log, and counts toward `pgroles.deprecated.mode_plan`. **Upgrade:** change `mode: plan` to `mode: observe` in your manifests at your convenience — a future release removes the `plan` value, and that removal will be the breaking change. diff --git a/crates/pgroles-operator/src/candidate.rs b/crates/pgroles-operator/src/candidate.rs index 9b15c37d..0e9ce7fa 100644 --- a/crates/pgroles-operator/src/candidate.rs +++ b/crates/pgroles-operator/src/candidate.rs @@ -69,7 +69,11 @@ use crate::reconciler::{ReconcileError, ResolvedPassword}; /// Maximum terminal candidates retained per policy before the oldest are /// pruned. Deleting a candidate cascades to the plan it owns and to that -/// plan's SQL ConfigMap. +/// plan's SQL ConfigMap — which is why this flat bound governs only +/// candidates whose plans never executed. A candidate owning an `Applied` +/// plan is held to [`crate::plan::PlanRetention`]'s applied bounds instead, +/// so proposal churn cannot delete execution history through the owner +/// object (see [`terminal_candidates_to_prune`]). const DEFAULT_MAX_TERMINAL_CANDIDATES: usize = 10; /// Maximum *open* candidates per policy that are planned in one pass. @@ -1435,33 +1439,57 @@ async fn supersede_candidate_plan( Ok(()) } -/// Prune terminal candidates beyond the retention bound. +/// Prune terminal candidates beyond the retention bounds. /// /// Plans cascade: each is owned by its candidate, so deleting the candidate -/// takes the plan and its SQL ConfigMap with it. `pgroles.io/keep=true` exempts -/// a candidate, and this is best-effort — retention must never block planning. +/// takes the plan and its SQL ConfigMap with it. That cascade is why pruning +/// reads the plans first — see [`terminal_candidates_to_prune`] for the +/// decision. Best-effort throughout: retention must never block planning, and +/// when the plans cannot be read this pass prunes nothing rather than prune +/// blind and cascade away an `Applied` plan retention promises to keep. async fn cleanup_terminal_candidates( ctx: &OperatorContext, namespace: &str, candidates: &[PostgresPolicyCandidate], ) { - let mut terminal: Vec<&PostgresPolicyCandidate> = candidates + let terminal: Vec<&PostgresPolicyCandidate> = candidates .iter() .filter(|candidate| candidate_phase(candidate).is_terminal()) - .filter(|candidate| !is_retention_exempt(*candidate)) .collect(); - if terminal.len() <= DEFAULT_MAX_TERMINAL_CANDIDATES { + let retention = ctx.plan_retention; + // The buckets below partition the terminal set, so when the whole set + // fits inside every count bound nothing can be pruned — skip the plan + // read on that common path. + if terminal.len() <= DEFAULT_MAX_TERMINAL_CANDIDATES && terminal.len() <= retention.applied { return; } - terminal.sort_by(|a, b| { - a.metadata - .creation_timestamp - .cmp(&b.metadata.creation_timestamp) - }); + + let plans: Vec = + match Api::::namespaced(ctx.kube_client.clone(), namespace) + .list(&ListParams::default()) + .await + { + Ok(list) => list.items, + Err(err) => { + tracing::warn!(%err, "could not read plans; skipping terminal-candidate pruning"); + return; + } + }; + let records: Vec<(&PostgresPolicyCandidate, Vec<&PostgresPolicyPlan>)> = terminal + .into_iter() + .map(|candidate| { + let uid = candidate.metadata.uid.clone().unwrap_or_default(); + let owned: Vec<&PostgresPolicyPlan> = plans + .iter() + .filter(|plan| !uid.is_empty() && crate::plan::is_owned_by_uid(*plan, &uid)) + .collect(); + (candidate, owned) + }) + .collect(); let api: Api = Api::namespaced(ctx.kube_client.clone(), namespace); - let excess = terminal.len() - DEFAULT_MAX_TERMINAL_CANDIDATES; - for candidate in terminal.into_iter().take(excess) { + let now_ts = Timestamp::now().as_second(); + for candidate in terminal_candidates_to_prune(&records, retention, now_ts) { let name = candidate.name_any(); info!(candidate = %name, "pruning terminal candidate"); if let Err(err) = api.delete(&name, &DeleteParams::default()).await { @@ -1470,6 +1498,75 @@ async fn cleanup_terminal_candidates( } } +/// Which terminal candidates to prune, given the plans each one owns. +/// +/// Deleting a candidate cascades to every plan it owns, so the decision is +/// made per candidate-and-plans pair: +/// +/// - `pgroles.io/keep=true` on the candidate **or on any plan it owns** +/// exempts the pair. The cascade cannot honour a keep on the child if the +/// parent goes, so a kept child must protect its parent. +/// - A candidate owning an `Applied` plan is the provenance of an execution +/// record, so it is held to the [`crate::plan::PlanRetention`] bounds for +/// `Applied` plans — same count, age floor and ceiling, ordered by when the plan +/// applied — not to the flat terminal bound, which proposal churn would +/// otherwise use to delete fresh execution history through the owner +/// object. +/// - Every other terminal candidate is proposal churn — nothing it owns ever +/// ran — bounded by [`DEFAULT_MAX_TERMINAL_CANDIDATES`], oldest first by +/// creation. +fn terminal_candidates_to_prune<'a>( + records: &[(&'a PostgresPolicyCandidate, Vec<&'a PostgresPolicyPlan>)], + retention: crate::plan::PlanRetention, + now_ts: i64, +) -> Vec<&'a PostgresPolicyCandidate> { + let mut churn: Vec<&PostgresPolicyCandidate> = Vec::new(); + let mut applied: Vec<(&PostgresPolicyCandidate, &PostgresPolicyPlan)> = Vec::new(); + for (candidate, plans) in records { + if is_retention_exempt(*candidate) || plans.iter().any(|plan| is_retention_exempt(*plan)) { + continue; + } + let applied_plan = plans.iter().find(|plan| { + plan.status + .as_ref() + .is_some_and(|status| status.phase == PlanPhase::Applied) + }); + match applied_plan { + Some(plan) => applied.push((candidate, plan)), + None => churn.push(candidate), + } + } + + let mut prune: Vec<&PostgresPolicyCandidate> = Vec::new(); + if churn.len() > DEFAULT_MAX_TERMINAL_CANDIDATES { + churn.sort_by(|a, b| { + a.metadata + .creation_timestamp + .cmp(&b.metadata.creation_timestamp) + }); + let excess = churn.len() - DEFAULT_MAX_TERMINAL_CANDIDATES; + prune.extend(churn.into_iter().take(excess)); + } + + // Plan names are unique within the namespace, so they key the way back + // from an evicted plan to the candidate that owns it. + let evicted_plans: BTreeSet = crate::plan::applied_plans_to_evict( + applied.iter().map(|(_, plan)| *plan).collect(), + retention, + now_ts, + ) + .into_iter() + .map(|plan| plan.name_any()) + .collect(); + prune.extend( + applied + .into_iter() + .filter(|(_, plan)| evicted_plans.contains(&plan.name_any())) + .map(|(candidate, _)| candidate), + ); + prune +} + /// Read the `Ready` condition reason from a candidate status, for tests and /// status consumers that only care about the current verdict. pub fn ready_reason(status: &PostgresPolicyCandidateStatus) -> Option<&str> { @@ -1536,6 +1633,172 @@ mod tests { /// The TTL in hours, so fixtures can sit either side of it precisely. const TTL_HOURS: i64 = DEFAULT_OPEN_CANDIDATE_TTL.as_hours(); + // ----------------------------------------------------------------- + // Terminal-candidate pruning + // ----------------------------------------------------------------- + + /// A terminal candidate created `age_secs` ago. + fn terminal_candidate( + name: &str, + phase: CandidatePhase, + age_secs: i64, + now_ts: i64, + ) -> PostgresPolicyCandidate { + let mut candidate = candidate(name, PolicyContent::default()); + candidate.metadata.uid = Some(format!("{name}-uid")); + candidate.metadata.creation_timestamp = + Some(k8s_openapi::apimachinery::pkg::apis::meta::v1::Time( + jiff::Timestamp::from_second(now_ts - age_secs).expect("epoch second in range"), + )); + candidate.status = Some(PostgresPolicyCandidateStatus { + phase, + ..Default::default() + }); + candidate + } + + /// An `Applied` plan that executed `applied_age_secs` ago. + fn applied_plan(name: &str, applied_age_secs: i64, now_ts: i64) -> PostgresPolicyPlan { + let spec = crate::crd::PostgresPolicyPlanSpec { + policy_ref: crate::crd::PolicyPlanRef { + name: "orders".to_string(), + }, + policy_generation: 1, + reconciliation_mode: crate::crd::CrdReconciliationMode::Authoritative, + owned_roles: Vec::new(), + owned_schemas: Vec::new(), + managed_database_identity: "default/db/DATABASE_URL".to_string(), + origin: None, + scope: None, + }; + let mut plan = PostgresPolicyPlan::new(name, spec); + plan.status = Some(crate::crd::PostgresPolicyPlanStatus { + phase: PlanPhase::Applied, + applied_at: Some( + jiff::Timestamp::from_second(now_ts - applied_age_secs) + .expect("epoch second in range") + .to_string(), + ), + ..Default::default() + }); + plan + } + + fn keep_plan(mut plan: PostgresPolicyPlan) -> PostgresPolicyPlan { + plan.metadata + .labels + .get_or_insert_with(Default::default) + .insert("pgroles.io/keep".to_string(), "true".to_string()); + plan + } + + fn pruned_names( + records: &[(&PostgresPolicyCandidate, Vec<&PostgresPolicyPlan>)], + retention: crate::plan::PlanRetention, + now_ts: i64, + ) -> Vec { + let mut names: Vec = terminal_candidates_to_prune(records, retention, now_ts) + .into_iter() + .map(|candidate| candidate.name_any()) + .collect(); + names.sort(); + names + } + + /// The recommended workflow's failure mode: every change lands as a + /// candidate, so terminal proposals accumulate fast, and under one flat + /// creation-time bound the eleventh proposal deletes the oldest terminal + /// candidate — cascading to the Applied plan it owns, straight through + /// the retention promise made for Applied plans. + #[test] + fn proposal_churn_cannot_prune_a_promoted_candidate_with_a_fresh_applied_plan() { + let now = 1_700_000_000; + let retention = crate::plan::PlanRetention::default(); + + // The promoted candidate is the oldest object in the set by a wide + // margin, and its plan applied a minute ago. + let promoted = terminal_candidate("promoted", CandidatePhase::Promoted, 100_000, now); + let promoted_plan = applied_plan("promoted-plan", 60, now); + + let churn: Vec = (0..DEFAULT_MAX_TERMINAL_CANDIDATES + 2) + .map(|i| { + terminal_candidate( + &format!("churn-{i:03}"), + CandidatePhase::Superseded, + 1_000 - i as i64, + now, + ) + }) + .collect(); + + let mut records: Vec<(&PostgresPolicyCandidate, Vec<&PostgresPolicyPlan>)> = + vec![(&promoted, vec![&promoted_plan])]; + records.extend( + churn + .iter() + .map(|candidate| (candidate, Vec::<&PostgresPolicyPlan>::new())), + ); + + // Guard against a vacuous pass: the promoted candidate must be the + // oldest by creation, so a flat creation-time bound over the whole + // set — the behaviour this test exists to reject — would prune it + // first. + assert!( + churn + .iter() + .all(|c| c.metadata.creation_timestamp > promoted.metadata.creation_timestamp), + "the fixture must make the promoted candidate the oldest object" + ); + + let pruned = pruned_names(&records, retention, now); + assert_eq!( + pruned, + vec!["churn-000".to_string(), "churn-001".to_string()], + "exactly the excess churn goes, oldest first" + ); + assert!( + !pruned.contains(&"promoted".to_string()), + "a promoted candidate with a fresh Applied plan is execution history, not churn" + ); + } + + /// Deleting the candidate cascades to its plans, so `pgroles.io/keep=true` + /// on the child plan has to protect the pair — a keep the cascade would + /// ignore is not a keep. + #[test] + fn a_keep_label_on_the_child_plan_protects_the_candidate() { + let now = 1_700_000_000; + // A floor of zero and a bound of one, so the applied bucket must + // evict — only exemptions can spare anything here. + let retention = crate::plan::PlanRetention { + applied: 1, + applied_ceiling: 1, + applied_min_age_secs: 0, + ..Default::default() + }; + + let oldest = terminal_candidate("p-old", CandidatePhase::Promoted, 900, now); + let oldest_plan = keep_plan(applied_plan("p-old-plan", 300, now)); + let middle = terminal_candidate("p-mid", CandidatePhase::Promoted, 800, now); + let middle_plan = applied_plan("p-mid-plan", 200, now); + let newest = terminal_candidate("p-new", CandidatePhase::Promoted, 700, now); + let newest_plan = applied_plan("p-new-plan", 100, now); + + let records: Vec<(&PostgresPolicyCandidate, Vec<&PostgresPolicyPlan>)> = vec![ + (&oldest, vec![&oldest_plan]), + (&middle, vec![&middle_plan]), + (&newest, vec![&newest_plan]), + ]; + + // p-old's plan is the earliest execution, so without the keep it is + // the first eviction; pruning the *younger* p-mid instead is what + // proves the child's label protected its parent. + assert_eq!( + pruned_names(&records, retention, now), + vec!["p-mid".to_string()] + ); + } + fn keep(mut candidate: PostgresPolicyCandidate) -> PostgresPolicyCandidate { candidate .metadata diff --git a/crates/pgroles-operator/src/plan.rs b/crates/pgroles-operator/src/plan.rs index e5dd2924..f4ed73b1 100644 --- a/crates/pgroles-operator/src/plan.rs +++ b/crates/pgroles-operator/src/plan.rs @@ -1691,13 +1691,34 @@ fn plans_to_evict( let mut evict = oldest_beyond(&mut decided, retention.decided); evict.extend(oldest_beyond(&mut superseded, retention.superseded)); + evict.extend(applied_plans_to_evict(applied, retention, now_ts)); + evict +} - // Applied is the audit record of what actually ran, so its count bound - // yields to the age floor. Walk oldest first and stop once the count bound - // is met; spare anything still inside the floor along the way, unless the - // survivors would breach the ceiling. Sparing is what makes the retained - // span a promise rather than a function of how often the policy applies. - sort_oldest_first(&mut applied); +/// Which `Applied` plans the retention bounds evict from `applied`. +/// +/// Applied is the audit record of what actually ran, so its count bound +/// yields to the age floor. Walk oldest first and stop once the count bound +/// is met; spare anything still inside the floor along the way, unless the +/// survivors would breach the ceiling. Sparing is what makes the retained +/// span a promise rather than a function of how often the policy applies. +/// +/// Both the order and the floor measure from [`applied_epoch_secs`] — one +/// notion of "when did this apply" drives the whole walk. Ordering by +/// creation instead would evict a plan created before a long review but +/// applied moments ago, ahead of older executions whose objects happen to be +/// newer. +/// +/// Shared with terminal-candidate pruning: deleting a promoted candidate +/// cascades to the `Applied` plan it owns, so the candidate is held to these +/// same bounds (see `candidate::terminal_candidates_to_prune`). +pub(crate) fn applied_plans_to_evict( + mut applied: Vec<&PostgresPolicyPlan>, + retention: PlanRetention, + now_ts: i64, +) -> Vec<&PostgresPolicyPlan> { + applied.sort_by_key(|plan| applied_epoch_secs(plan)); + let mut evict = Vec::new(); let mut remaining = applied.len(); for plan in applied { if remaining <= retention.applied { @@ -1710,7 +1731,6 @@ fn plans_to_evict( evict.push(plan); remaining -= 1; } - evict } @@ -1735,31 +1755,35 @@ fn sort_oldest_first(plans: &mut [&PostgresPolicyPlan]) { }); } -/// Age of an `Applied` plan for the retention floor, measured from -/// `status.appliedAt`. The floor promises post-application history, and a -/// plan can sit `Pending` or `Approved` for arbitrarily long before a -/// reviewer decides it — measured from creation, a plan approved late would -/// read as already outside the floor and be evicted by the cleanup that runs -/// right after it executes. Falls back to [`plan_age_secs`] when `appliedAt` -/// is absent or unparseable, so plans recorded before it was set keep the -/// creation-time behaviour rather than reading as infinitely old. -fn applied_age_secs(plan: &PostgresPolicyPlan, now_ts: i64) -> i64 { +/// When an `Applied` plan applied, as epoch seconds: `status.appliedAt`, +/// falling back to the creation timestamp when it is absent or unparseable so +/// plans recorded before it was set keep the creation-time behaviour rather +/// than reading as infinitely old. `None` when neither instant is known. +/// +/// This is the single notion of "when did this apply" behind both the +/// retention floor and the eviction order — the floor promises +/// post-application history, and a plan can sit `Pending` or `Approved` for +/// arbitrarily long before a reviewer decides it, so measuring from creation +/// would read a plan approved late as already outside that promise. +fn applied_epoch_secs(plan: &PostgresPolicyPlan) -> Option { plan.status .as_ref() .and_then(|status| status.applied_at.as_deref()) .and_then(parse_rfc3339_epoch_secs) - .map(|applied_ts| now_ts.saturating_sub(applied_ts)) - .unwrap_or_else(|| plan_age_secs(plan, now_ts)) + .or_else(|| { + plan.metadata + .creation_timestamp + .as_ref() + .map(|timestamp| timestamp.0.as_second()) + }) } -/// Age in seconds, or 0 for a plan with no creation timestamp — treating an -/// unknown age as brand new keeps it, which is the safe direction for a -/// deletion decision. -fn plan_age_secs(plan: &PostgresPolicyPlan, now_ts: i64) -> i64 { - plan.metadata - .creation_timestamp - .as_ref() - .map(|timestamp| now_ts.saturating_sub(timestamp.0.as_second())) +/// Age of an `Applied` plan for the retention floor — see +/// [`applied_epoch_secs`]. An unknown instant is age 0: brand new is the safe +/// direction for a deletion decision. +fn applied_age_secs(plan: &PostgresPolicyPlan, now_ts: i64) -> i64 { + applied_epoch_secs(plan) + .map(|applied_ts| now_ts.saturating_sub(applied_ts)) .unwrap_or(0) } @@ -3005,6 +3029,60 @@ mod tests { ); } + #[test] + fn above_the_ceiling_eviction_order_follows_when_plans_applied_not_when_created() { + // Above the ceiling the floor is overridden and the walk evicts from + // the front, so the sort order decides who dies. Ordered by creation, + // a plan created before a long review but applied moments ago sorts + // first and is deleted by the cleanup right after it executes, while + // older executions whose objects happen to be newer survive. + let now = 1_700_000_000; + let retention = PlanRetention::default(); + let excess = 5; + let count = retention.applied_ceiling + excess; + + // Creation order is the exact reverse of applied order: plan 0 is the + // oldest object but the most recent execution. All applied inside the + // floor, so only the ceiling forces eviction. + let plans: Vec = (0..count) + .map(|i| { + let mut plan = retained_plan( + &format!("applied-{i:04}"), + PlanPhase::Applied, + retention.applied_min_age_secs * 3 + (count - i) as i64, + now, + ); + plan.status + .as_mut() + .expect("retained_plan always sets a status") + .applied_at = Some( + jiff::Timestamp::from_second(now - 100 - i as i64) + .expect("epoch second in range") + .to_string(), + ); + plan + }) + .collect(); + + let evicted = evicted_names(&plans, retention, now); + assert_eq!( + evicted.len(), + excess, + "the ceiling trims exactly the excess" + ); + for i in 0..excess { + let earliest_executed = format!("applied-{:04}", count - 1 - i); + assert!( + evicted.contains(&earliest_executed), + "the earliest executions go first, whatever their objects' creation order" + ); + } + assert!( + !evicted.contains(&"applied-0000".to_string()), + "the most recent execution must survive even as the oldest object by creation" + ); + } + #[test] fn a_non_unicode_environment_value_is_rejected_not_defaulted() { use std::os::unix::ffi::OsStringExt; diff --git a/docs/src/pages/docs/operator-candidates.md b/docs/src/pages/docs/operator-candidates.md index 02cd8c1e..44177fce 100644 --- a/docs/src/pages/docs/operator-candidates.md +++ b/docs/src/pages/docs/operator-candidates.md @@ -404,11 +404,19 @@ step one; it never performs the cutover itself. ## Retention Candidates carry an `ownerReference` to their parent policy, and each derived -plan is owned by its candidate, so pruning cascades. Terminal candidates — -`Promoted=True`, or `Superseded=True` for any reason including `PlanDenied` — -are pruned by the same bounded retention loop as plans; label a candidate -`pgroles.io/keep=true` to exempt it. Plans also expire after a TTL — an -approval is not an indefinite authorisation. +plan is owned by its candidate, so deleting a candidate cascades to its plan +and that plan's SQL ConfigMap. Pruning therefore decides on the pair, never +the candidate alone. A terminal candidate that owns no `Applied` plan — +superseded proposals of every kind, and promotions whose execution ran on +another plan — is proposal churn, bounded at 10 per policy, oldest first by +creation. A terminal candidate that owns an `Applied` plan is the provenance +of an execution record: it is held to the `Applied` bounds in the table below +— same count, age floor, and ceiling, ordered by when its plan applied — so +filing more proposals can never delete the record of what ran through the +owner object. +`pgroles.io/keep=true` on the candidate **or on its plan** exempts the pair. +Plans also expire after a TTL — an approval is not an indefinite +authorisation. ### What plan retention keeps @@ -424,13 +432,13 @@ plans that never ran — would evict the `Applied` ones that record what did. | `Superseded` | 3 | Enough to see what a replan replaced | | `Pending`, `Approved`, `Applying` | all | Still live; never evicted | -The oldest go first within each bucket. `Applied` additionally has an age -floor, measured from `status.appliedAt` — not from creation, since a plan can -wait on a reviewer for arbitrarily long before it executes: a plan applied -inside the floor is kept even once the count is exceeded, so the audit trail -spans a stated period instead of however long the policy's churn rate happens -to make it. The ceiling overrides the floor — the floor is a promise -about history, not a licence to keep everything — and a policy applying hard +The oldest go first within each bucket. For `Applied`, both the eviction +order and the age floor measure from `status.appliedAt` — not from creation, +since a plan can wait on a reviewer for arbitrarily long before it executes. +A plan applied inside the floor is kept even once the count is exceeded, so +the audit trail spans a stated period instead of however long the policy's +churn rate happens to make it. The ceiling overrides the floor — the floor is +a promise about history, not a licence to keep everything — and a policy applying hard enough to reach 200 within the floor period will start losing its oldest. `pgroles.io/keep=true` exempts a plan from every one of these bounds.