From 6399d53550529b461ef69425228120a35e6fac04 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Sat, 6 Jun 2026 13:14:21 +1200 Subject: [PATCH 1/5] Remove deprecated operator status fields --- CHANGELOG.md | 5 + charts/pgroles-operator/Chart.yaml | 10 +- .../crds/postgrespolicies.pgroles.io.yaml | 33 ++-- crates/pgroles-operator/src/crd.rs | 64 +++---- crates/pgroles-operator/src/reconciler.rs | 157 ------------------ docs/src/pages/docs/operator.md | 14 +- k8s/crd.yaml | 33 ++-- 7 files changed, 83 insertions(+), 233 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a158d56..e767ac6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Operator reconciles can now be requested immediately with `pgroles reconcile` or the `reconcile.pgroles.io/requestedAt` annotation.** The CLI annotates a `PostgresPolicy` and can optionally wait until `status.lastHandledReconcileAt` records that the operator successfully handled the request. The operator also includes the annotation value in its watch predicate, so changing only the request timestamp triggers a reconcile without mutating the policy spec. (#118) - **`pgroles render-bundle` composes a policy bundle into a single flat manifest.** Validates and composes the bundle (rejecting scope/ownership conflicts up front), then emits the resulting `PolicyManifest` as YAML with a provenance header recording the source bundle basename, the manifest schema version (`pgroles.manifest.v1`), and the fragments it composed. The output round-trips through `pgroles validate -f` / `diff -f` / `apply -f`, so a bundle can be composed in CI and the rendered manifest wrapped into a `PostgresPolicy` resource in a GitOps repo. Pre-rendering keeps cross-team and cross-environment fragment composition available to operator users without adding operator-side CRDs. The renderer is byte-deterministic across machines: the header records only the bundle file's basename (never an absolute or `pwd`-relative path), and the YAML body is post-processed to strip serde-emitted optional defaults (empty optional sequences, `null` scalars, and the default `role_pattern`) so the file doesn't churn under unrelated upgrades. Required fields like `Membership.members`, `Grant.privileges`, and `DefaultPrivilege.grant`, plus named empty profiles, are preserved even when empty so the rendered manifest always re-parses. `--check ` compares against an existing rendered file and exits with code 2 on drift, suitable as a CI gate that catches stale checked-in renders. The new [bundle composition guide](https://hardbyte.github.io/pgroles/docs/bundle-composition/) documents when to use each of the three workflows (single manifest, CLI bundle for direct apply, rendered bundle for the operator). Use `--no-header` to omit the header and `--output ` to write to a file. (#92) +### Changed + +- **Operator plans now require manual approval by default when `spec.approval` is omitted.** Set `approval: auto` explicitly to preserve immediate apply behavior. Deprecated `PostgresPolicy.status.planned_sql`, `planned_sql_truncated`, and `last_reconcile_time` were removed; use `PostgresPolicyPlan` resources for SQL review and `last_successful_reconcile_time` for the last successful reconcile timestamp. (#73) +- **The `PostgresPolicy` CRD now marks named arrays as Kubernetes map lists.** `spec.schemas` and `spec.roles` are keyed by `name`, and `spec.retirements` is keyed by `role`, improving server-side-apply and GitOps merge behavior without changing manifest authoring shape. (#126) + ## [0.7.7] - 2026-05-18 ### Added diff --git a/charts/pgroles-operator/Chart.yaml b/charts/pgroles-operator/Chart.yaml index 2a998f1..04e1d1d 100644 --- a/charts/pgroles-operator/Chart.yaml +++ b/charts/pgroles-operator/Chart.yaml @@ -5,8 +5,8 @@ description: >- Define roles, memberships, grants, and default privileges in YAML and let the operator converge your database to the desired state. type: application -version: 0.7.8 -appVersion: "0.7.8" +version: 0.7.9 +appVersion: "0.7.9" icon: https://raw.githubusercontent.com/hardbyte/pgroles/main/docs/public/logo.svg sources: - https://github.com/hardbyte/pgroles @@ -20,6 +20,12 @@ annotations: description: 'PostgresPolicy roles can now be marked external: true so provider-managed roles remain referenceable without pgroles creating, altering, dropping, password-managing, or pruning memberships granted from them.' - kind: added description: 'PostgresPolicy resources now support immediate reconcile requests through the reconcile.pgroles.io/requestedAt annotation; pgroles reconcile can set it and optionally wait for status.lastHandledReconcileAt.' + - kind: changed + description: 'PostgresPolicy plans now require manual approval by default when spec.approval is omitted; set approval: auto for immediate apply.' + - kind: changed + description: 'PostgresPolicy status no longer includes deprecated planned_sql, planned_sql_truncated, or last_reconcile_time fields; use PostgresPolicyPlan and last_successful_reconcile_time instead.' + - kind: changed + description: 'PostgresPolicy CRD marks schemas, roles, and retirements as Kubernetes map lists for better server-side-apply merge behavior.' artifacthub.io/category: database artifacthub.io/operator: "true" artifacthub.io/prerelease: "false" diff --git a/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml b/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml index d72aa21..03f0096 100644 --- a/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml +++ b/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml @@ -676,7 +676,11 @@ ], "type": "object" }, - "type": "array" + "type": "array", + "x-kubernetes-list-map-keys": [ + "role" + ], + "x-kubernetes-list-type": "map" }, "roles": { "default": [], @@ -790,7 +794,11 @@ ], "type": "object" }, - "type": "array" + "type": "array", + "x-kubernetes-list-map-keys": [ + "name" + ], + "x-kubernetes-list-type": "map" }, "schemas": { "default": [], @@ -824,7 +832,11 @@ ], "type": "object" }, - "type": "array" + "type": "array", + "x-kubernetes-list-map-keys": [ + "name" + ], + "x-kubernetes-list-type": "map" }, "suspend": { "default": false, @@ -1003,11 +1015,6 @@ "nullable": true, "type": "string" }, - "last_reconcile_time": { - "description": "Deprecated alias retained for compatibility with older status readers.", - "nullable": true, - "type": "string" - }, "last_successful_reconcile_time": { "description": "ISO 8601 timestamp of the last successful reconciliation.", "nullable": true, @@ -1040,16 +1047,6 @@ }, "type": "array" }, - "planned_sql": { - "description": "Planned SQL for the last successful plan-mode reconcile.", - "nullable": true, - "type": "string" - }, - "planned_sql_truncated": { - "default": false, - "description": "Whether `planned_sql` was truncated to fit safely in status.", - "type": "boolean" - }, "transient_failure_count": { "default": 0, "description": "Consecutive transient operational failures used for exponential backoff.", diff --git a/crates/pgroles-operator/src/crd.rs b/crates/pgroles-operator/src/crd.rs index 0267309..119be76 100644 --- a/crates/pgroles-operator/src/crd.rs +++ b/crates/pgroles-operator/src/crd.rs @@ -83,10 +83,18 @@ pub struct PostgresPolicySpec { /// Schema bindings that expand profiles into concrete roles/grants. #[serde(default)] + #[schemars(extend( + "x-kubernetes-list-type" = "map", + "x-kubernetes-list-map-keys" = ["name"] + ))] pub schemas: Vec, /// One-off role definitions. #[serde(default)] + #[schemars(extend( + "x-kubernetes-list-type" = "map", + "x-kubernetes-list-map-keys" = ["name"] + ))] pub roles: Vec, /// One-off grants. @@ -103,13 +111,16 @@ pub struct PostgresPolicySpec { /// Explicit role-retirement workflows for roles that should be removed. #[serde(default)] + #[schemars(extend( + "x-kubernetes-list-type" = "map", + "x-kubernetes-list-map-keys" = ["role"] + ))] pub retirements: Vec, /// Approval mode for plans: `auto` or `manual`. /// When `manual`, plans require explicit approval before execution. /// When `auto`, plans are approved and applied immediately. - /// When omitted, inferred from `mode`: `apply` → `auto`, `plan` → `manual`. - /// This ensures backward compatibility for existing `mode: apply` users. + /// When omitted, defaults to `manual`. #[serde(default, skip_serializing_if = "Option::is_none")] pub approval: Option, } @@ -152,15 +163,11 @@ pub enum ApprovalMode { } impl PostgresPolicySpec { - /// Resolve the effective approval mode, inferring from `mode` when not set. - /// `apply` → `Auto` (backward compat), `plan` → `Manual`. + /// Resolve the effective approval mode. Omitted approval defaults to manual. pub fn effective_approval(&self) -> ApprovalMode { match &self.approval { Some(mode) => mode.clone(), - None => match self.mode { - PolicyMode::Apply => ApprovalMode::Auto, - PolicyMode::Plan => ApprovalMode::Manual, - }, + None => ApprovalMode::Manual, } } } @@ -809,10 +816,6 @@ pub struct PostgresPolicyStatus { #[serde(default)] pub last_successful_reconcile_time: Option, - /// Deprecated alias retained for compatibility with older status readers. - #[serde(default)] - pub last_reconcile_time: Option, - /// Last force-reconcile annotation value handled by the operator. #[serde(default, rename = "lastHandledReconcileAt")] pub last_handled_reconcile_at: Option, @@ -825,14 +828,6 @@ pub struct PostgresPolicyStatus { #[serde(default)] pub last_reconcile_mode: Option, - /// Planned SQL for the last successful plan-mode reconcile. - #[serde(default)] - pub planned_sql: Option, - - /// Whether `planned_sql` was truncated to fit safely in status. - #[serde(default)] - pub planned_sql_truncated: bool, - /// Canonical identity of the managed database target. #[serde(default)] pub managed_database_identity: Option, @@ -3032,7 +3027,6 @@ params: status.observed_generation = generation; status.last_attempted_generation = generation; status.last_successful_reconcile_time = Some(now_rfc3339()); - status.last_reconcile_time = Some(now_rfc3339()); status.change_summary = Some(summary); status.last_error = None; @@ -3051,7 +3045,6 @@ params: // Verify timestamps set assert!(status.last_successful_reconcile_time.is_some()); - assert!(status.last_reconcile_time.is_some()); // Verify summary let summary = status.change_summary.as_ref().unwrap(); @@ -3763,6 +3756,23 @@ retirements: assert!(yaml.contains("pgplan"), "should have shortname pgplan"); } + #[test] + fn postgres_policy_crd_marks_named_lists_as_maps() { + let crd = PostgresPolicy::crd(); + let yaml = serde_yaml::to_string(&crd).expect("CRD should serialize to YAML"); + + for field_name in ["schemas", "roles"] { + assert!( + yaml.contains(&format!("{field_name}:\n")), + "CRD should contain spec.{field_name}" + ); + } + assert!(yaml.contains("x-kubernetes-list-type: map")); + assert!(yaml.contains("x-kubernetes-list-map-keys:")); + assert!(yaml.contains("- name")); + assert!(yaml.contains("- role")); + } + #[test] fn plan_phase_display() { assert_eq!(PlanPhase::Pending.to_string(), "Pending"); @@ -3779,7 +3789,7 @@ retirements: } #[test] - fn effective_approval_infers_from_mode() { + fn effective_approval_defaults_to_manual() { let base = PostgresPolicySpec { connection: ConnectionSpec { secret_ref: Some(SecretReference { @@ -3803,10 +3813,8 @@ retirements: approval: None, }; - // apply mode with no explicit approval → Auto - assert_eq!(base.effective_approval(), ApprovalMode::Auto); + assert_eq!(base.effective_approval(), ApprovalMode::Manual); - // plan mode with no explicit approval → Manual let plan = PostgresPolicySpec { mode: PolicyMode::Plan, ..base.clone() @@ -3870,8 +3878,8 @@ retirements: ); assert_eq!( spec.effective_approval(), - ApprovalMode::Auto, - "effective_approval should infer Auto from apply mode" + ApprovalMode::Manual, + "effective_approval should default to Manual when omitted" ); } diff --git a/crates/pgroles-operator/src/reconciler.rs b/crates/pgroles-operator/src/reconciler.rs index 73e5d67..c8b9a6d 100644 --- a/crates/pgroles-operator/src/reconciler.rs +++ b/crates/pgroles-operator/src/reconciler.rs @@ -53,9 +53,6 @@ const SQLSTATE_UNDEFINED_TABLE: &str = "42P01"; const SQLSTATE_UNDEFINED_FUNCTION: &str = "42883"; const SQLSTATE_UNDEFINED_OBJECT: &str = "42704"; -/// Maximum amount of rendered planned SQL stored in status. -const MAX_PLANNED_SQL_STATUS_BYTES: usize = 16 * 1024; - enum ReconcileOutcome { Reconciled, Planned, @@ -676,8 +673,6 @@ fn mark_reconcile_failure_status( && c.condition_type != "Conflict" }); status.change_summary = None; - status.planned_sql = None; - status.planned_sql_truncated = false; if clear_current_plan_ref { status.current_plan_ref = None; } @@ -715,8 +710,6 @@ async fn reconcile_apply_inner( .retain(|c| c.condition_type != "Reconciling" && c.condition_type != "Drifted"); status.last_attempted_generation = generation; status.last_error = None; - status.planned_sql = None; - status.planned_sql_truncated = false; status.transient_failure_count = 0; }) .await?; @@ -770,8 +763,6 @@ async fn reconcile_apply_inner( .conditions .retain(|c| c.condition_type != "Reconciling" && c.condition_type != "Drifted"); status.change_summary = None; - status.planned_sql = None; - status.planned_sql_truncated = false; status.last_error = Some(conflict_message.clone()); status.transient_failure_count = 0; }) @@ -987,7 +978,6 @@ async fn apply_under_lock( let summary = summarize_changes(&changes); let sql_ctx = detect_sql_context(pool, &inspect_config).await?; - let (planned_sql, planned_sql_truncated) = render_plan_sql_for_status(&changes, &sql_ctx); let effective_approval = resource.spec.effective_approval(); @@ -1051,7 +1041,6 @@ async fn apply_under_lock( plan_ref_name = Some(plan_name); } - // Still write deprecated planned_sql to status for backward compat. update_status(ctx, resource, |status| { status.set_condition(ready_condition(true, "Planned", &ready_message)); status.set_condition(drifted_condition( @@ -1068,11 +1057,8 @@ async fn apply_under_lock( status.observed_generation = generation; status.last_attempted_generation = generation; status.last_successful_reconcile_time = Some(crate::crd::now_rfc3339()); - status.last_reconcile_time = Some(crate::crd::now_rfc3339()); status.change_summary = Some(summary.clone()); status.last_reconcile_mode = Some(PolicyMode::Plan); - status.planned_sql = planned_sql.clone(); - status.planned_sql_truncated = planned_sql_truncated; status.last_error = None; status.transient_failure_count = 0; if let Some(ref plan_name) = plan_ref_name { @@ -1190,11 +1176,8 @@ async fn apply_under_lock( status.observed_generation = generation; status.last_attempted_generation = generation; status.last_successful_reconcile_time = Some(crate::crd::now_rfc3339()); - status.last_reconcile_time = Some(crate::crd::now_rfc3339()); status.change_summary = Some(summary); status.last_reconcile_mode = Some(PolicyMode::Apply); - status.planned_sql = None; - status.planned_sql_truncated = false; status.last_error = None; status.applied_password_source_versions = applied_password_source_versions; status.transient_failure_count = 0; @@ -1296,8 +1279,6 @@ async fn apply_under_lock( status.last_attempted_generation = generation; status.change_summary = Some(summary.clone()); status.last_reconcile_mode = Some(PolicyMode::Apply); - status.planned_sql = planned_sql.clone(); - status.planned_sql_truncated = planned_sql_truncated; status.last_error = None; status.transient_failure_count = 0; status.current_plan_ref = Some(crate::crd::PlanReference { @@ -1392,11 +1373,8 @@ async fn apply_under_lock( status.observed_generation = generation; status.last_attempted_generation = generation; status.last_successful_reconcile_time = Some(crate::crd::now_rfc3339()); - status.last_reconcile_time = Some(crate::crd::now_rfc3339()); status.change_summary = Some(summary); status.last_reconcile_mode = Some(PolicyMode::Apply); - status.planned_sql = None; - status.planned_sql_truncated = false; status.last_error = None; status.applied_password_source_versions = applied_password_source_versions; @@ -1473,8 +1451,6 @@ async fn apply_under_lock( }); status.last_attempted_generation = generation; status.change_summary = Some(summary.clone()); - status.planned_sql = planned_sql.clone(); - status.planned_sql_truncated = planned_sql_truncated; status.last_error = None; status.transient_failure_count = 0; }) @@ -1501,11 +1477,8 @@ async fn apply_under_lock( status.observed_generation = generation; status.last_attempted_generation = generation; status.last_successful_reconcile_time = Some(crate::crd::now_rfc3339()); - status.last_reconcile_time = Some(crate::crd::now_rfc3339()); status.change_summary = Some(summary); status.last_reconcile_mode = Some(PolicyMode::Apply); - status.planned_sql = None; - status.planned_sql_truncated = false; status.last_error = None; status.applied_password_source_versions = applied_password_source_versions; status.transient_failure_count = 0; @@ -1570,8 +1543,6 @@ async fn apply_under_lock( status.last_attempted_generation = generation; status.change_summary = Some(summary.clone()); status.last_reconcile_mode = Some(PolicyMode::Apply); - status.planned_sql = planned_sql.clone(); - status.planned_sql_truncated = planned_sql_truncated; status.last_error = None; status.transient_failure_count = 0; status.current_plan_ref = Some(crate::crd::PlanReference { @@ -1947,51 +1918,6 @@ async fn detect_sql_context( ) } -fn render_plan_sql_for_status( - changes: &[pgroles_core::diff::Change], - sql_ctx: &pgroles_core::sql::SqlContext, -) -> (Option, bool) { - if changes.is_empty() { - return (None, false); - } - - // Render each change individually so we can redact passwords. - let rendered: String = changes - .iter() - .flat_map(|change| { - if let pgroles_core::diff::Change::SetPassword { name, .. } = change { - vec![format!( - "ALTER ROLE {} PASSWORD '[REDACTED]';", - pgroles_core::sql::quote_ident(name) - )] - } else { - pgroles_core::sql::render_statements_with_context(change, sql_ctx) - } - }) - .collect::>() - .join("\n"); - - let (truncated, did_truncate) = truncate_status_text(&rendered, MAX_PLANNED_SQL_STATUS_BYTES); - (Some(truncated), did_truncate) -} - -fn truncate_status_text(text: &str, max_bytes: usize) -> (String, bool) { - if text.len() <= max_bytes { - return (text.to_string(), false); - } - - let marker = "\n-- truncated for status --"; - let target_len = max_bytes.saturating_sub(marker.len()); - let mut end = target_len.min(text.len()); - while end > 0 && !text.is_char_boundary(end) { - end -= 1; - } - - let mut truncated = text[..end].to_string(); - truncated.push_str(marker); - (truncated, true) -} - /// Emit a plan lifecycle event on the parent policy, logging warnings on failure. async fn emit_plan_event( ctx: &OperatorContext, @@ -2627,15 +2553,6 @@ mod tests { assert_eq!(summary.total, 3); } - #[test] - fn truncate_status_text_marks_truncation() { - let text = "x".repeat(MAX_PLANNED_SQL_STATUS_BYTES + 32); - let (truncated, did_truncate) = truncate_status_text(&text, MAX_PLANNED_SQL_STATUS_BYTES); - assert!(did_truncate); - assert!(truncated.len() <= MAX_PLANNED_SQL_STATUS_BYTES); - assert!(truncated.ends_with("-- truncated for status --")); - } - #[test] fn accumulate_summary_all_change_types() { use pgroles_core::diff::Change; @@ -2833,10 +2750,6 @@ mod tests { total: 1, ..Default::default() }), - planned_sql: Some( - "GRANT EXECUTE ON ALL ROUTINES IN SCHEMA \"app\" TO \"reader\";".into(), - ), - planned_sql_truncated: true, last_error: None, transient_failure_count: 3, current_plan_ref: Some(crate::crd::PlanReference { @@ -2883,8 +2796,6 @@ mod tests { "transient planning and stale conflict conditions should be cleared on degraded status" ); assert!(status.change_summary.is_none()); - assert!(status.planned_sql.is_none()); - assert!(!status.planned_sql_truncated); assert!(status.current_plan_ref.is_none()); assert_eq!(status.last_error.as_deref(), Some(message)); assert_eq!(status.transient_failure_count, 0); @@ -2896,8 +2807,6 @@ mod tests { current_plan_ref: Some(crate::crd::PlanReference { name: "approved-plan".into(), }), - planned_sql: Some("ALTER ROLE \"app\" LOGIN;".into()), - planned_sql_truncated: true, transient_failure_count: 2, ..Default::default() }; @@ -2917,8 +2826,6 @@ mod tests { .map(|plan| plan.name.as_str()), Some("approved-plan") ); - assert!(status.planned_sql.is_none()); - assert!(!status.planned_sql_truncated); assert_eq!( status.last_error.as_deref(), Some("SQL execution error: connection closed") @@ -3642,70 +3549,6 @@ mod tests { ); } - #[test] - fn render_plan_sql_for_status_redacts_passwords() { - let changes = vec![ - pgroles_core::diff::Change::CreateRole { - name: "app-svc".to_string(), - state: pgroles_core::model::RoleState { - login: true, - ..pgroles_core::model::RoleState::default() - }, - }, - pgroles_core::diff::Change::SetPassword { - name: "app-svc".to_string(), - password: "super_secret_p@ssw0rd!".to_string(), - }, - ]; - - let sql_ctx = pgroles_core::sql::SqlContext::default(); - let (sql, truncated) = render_plan_sql_for_status(&changes, &sql_ctx); - - let sql = sql.expect("expected non-empty planned SQL"); - assert!(!truncated); - assert!( - sql.contains("[REDACTED]"), - "status SQL should contain [REDACTED], got: {sql}" - ); - assert!( - !sql.contains("super_secret_p@ssw0rd!"), - "status SQL must NOT contain the actual password, got: {sql}" - ); - assert!( - sql.contains("CREATE ROLE"), - "status SQL should still contain non-password changes, got: {sql}" - ); - } - - #[test] - fn render_plan_sql_for_status_empty_changes_returns_none() { - let sql_ctx = pgroles_core::sql::SqlContext::default(); - let (sql, truncated) = render_plan_sql_for_status(&[], &sql_ctx); - assert!(sql.is_none()); - assert!(!truncated); - } - - #[test] - fn render_plan_sql_for_status_password_only_plan() { - let changes = vec![pgroles_core::diff::Change::SetPassword { - name: "db-user".to_string(), - password: "my_secret_pw".to_string(), - }]; - - let sql_ctx = pgroles_core::sql::SqlContext::default(); - let (sql, _) = render_plan_sql_for_status(&changes, &sql_ctx); - - let sql = sql.expect("expected non-empty planned SQL"); - assert!( - sql.contains("[REDACTED]"), - "password-only plan should still show redacted SQL" - ); - assert!( - !sql.contains("my_secret_pw"), - "password-only plan must NOT leak the password" - ); - } - #[test] fn error_reason_empty_password_secret() { let err = ReconcileError::EmptyPasswordSecret { diff --git a/docs/src/pages/docs/operator.md b/docs/src/pages/docs/operator.md index b651f1e..9493fcf 100644 --- a/docs/src/pages/docs/operator.md +++ b/docs/src/pages/docs/operator.md @@ -384,7 +384,7 @@ Generated Secrets are created in the same namespace as the `PostgresPolicy`, own - Passwords are only allowed on roles with `login: true`. - Exactly one of `password.secretRef` or `password.generate` must be set. -- Password values are redacted in operator logs and the `status.planned_sql` field. +- Password values are redacted in operator logs and `PostgresPolicyPlan` SQL previews. - If the referenced Secret or key is missing, the operator sets a `SecretMissing` or `SecretFetchFailed` status condition and retries on the normal interval. - Password updates are driven by password-source Secret changes. After a successful `apply`, unchanged password sources do not create permanent drift in later `plan` reconciles. - pgroles cannot detect direct password changes made in PostgreSQL outside the operator, because PostgreSQL does not expose comparable password state safely. @@ -478,8 +478,8 @@ In `plan` mode: - the operator connects to the database and computes the full diff normally - no PostgreSQL SQL is executed - `status.change_summary` records the pending changes -- `status.planned_sql` stores the rendered SQL, truncated if needed for status size safety - `status.current_plan_ref.name` points at the generated `PostgresPolicyPlan` +- the generated `PostgresPolicyPlan` stores the SQL preview inline or in a referenced ConfigMap - `Ready=True` with reason `Planned` - `Drifted=True` when changes are pending, `Drifted=False` when the database is already in sync - for `password.generate`, the controller does not create or recreate the generated Kubernetes Secret while running in `plan` mode @@ -512,18 +512,13 @@ status: current_plan_ref: name: plan-policy-plan-20260512-090308-118e50e437c9 last_reconcile_mode: plan - planned_sql: |- - CREATE ROLE "plan-preview-user" LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOREPLICATION NOBYPASSRLS; - COMMENT ON ROLE "plan-preview-user" IS 'Preview-only role'; - GRANT CONNECT ON DATABASE "postgres" TO "plan-preview-user"; - planned_sql_truncated: false ``` Use `suspend` when you want the controller to stop reconciling entirely. Use `plan` when you want it to keep inspecting and showing you what it would do. ### Plan approval resources -When `spec.approval: manual` is used with `mode: apply`, the operator creates a `PostgresPolicyPlan` and waits for approval instead of immediately executing the SQL. +In `mode: apply`, omitted `spec.approval` defaults to `manual`: the operator creates a `PostgresPolicyPlan` and waits for approval instead of immediately executing the SQL. Set `spec.approval: auto` explicitly if you want plans to be approved and applied immediately. ```yaml spec: @@ -531,7 +526,6 @@ spec: secretRef: name: postgres-credentials mode: apply - approval: manual ``` A generated plan looks like this: @@ -677,7 +671,7 @@ status: message: "Applied 5 changes" last_transition_time: "2026-03-06T10:30:00Z" observed_generation: 3 - last_reconcile_time: "2026-03-06T10:30:00Z" + last_successful_reconcile_time: "2026-03-06T10:30:00Z" lastHandledReconcileAt: "2026-03-06T10:31:00Z" transient_failure_count: 0 change_summary: diff --git a/k8s/crd.yaml b/k8s/crd.yaml index d72aa21..03f0096 100644 --- a/k8s/crd.yaml +++ b/k8s/crd.yaml @@ -676,7 +676,11 @@ ], "type": "object" }, - "type": "array" + "type": "array", + "x-kubernetes-list-map-keys": [ + "role" + ], + "x-kubernetes-list-type": "map" }, "roles": { "default": [], @@ -790,7 +794,11 @@ ], "type": "object" }, - "type": "array" + "type": "array", + "x-kubernetes-list-map-keys": [ + "name" + ], + "x-kubernetes-list-type": "map" }, "schemas": { "default": [], @@ -824,7 +832,11 @@ ], "type": "object" }, - "type": "array" + "type": "array", + "x-kubernetes-list-map-keys": [ + "name" + ], + "x-kubernetes-list-type": "map" }, "suspend": { "default": false, @@ -1003,11 +1015,6 @@ "nullable": true, "type": "string" }, - "last_reconcile_time": { - "description": "Deprecated alias retained for compatibility with older status readers.", - "nullable": true, - "type": "string" - }, "last_successful_reconcile_time": { "description": "ISO 8601 timestamp of the last successful reconciliation.", "nullable": true, @@ -1040,16 +1047,6 @@ }, "type": "array" }, - "planned_sql": { - "description": "Planned SQL for the last successful plan-mode reconcile.", - "nullable": true, - "type": "string" - }, - "planned_sql_truncated": { - "default": false, - "description": "Whether `planned_sql` was truncated to fit safely in status.", - "type": "boolean" - }, "transient_failure_count": { "default": 0, "description": "Consecutive transient operational failures used for exponential backoff.", From b729d8c5f7523fa1353476c8b05a58baa0c88ad2 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Sat, 6 Jun 2026 13:23:16 +1200 Subject: [PATCH 2/5] Address operator CRD review feedback --- CHANGELOG.md | 2 +- Cargo.lock | 8 ++++---- Cargo.toml | 6 +++--- crates/pgroles-operator/src/crd.rs | 27 +++++++++++++++++---------- 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e767ac6..c3e5191 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [0.7.8] - 2026-06-04 +## [0.7.9] - 2026-06-04 ### Added diff --git a/Cargo.lock b/Cargo.lock index b4bb7b5..e2200d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1974,7 +1974,7 @@ dependencies = [ [[package]] name = "pgroles-cli" -version = "0.7.8" +version = "0.7.9" dependencies = [ "anyhow", "assert_cmd", @@ -1996,7 +1996,7 @@ dependencies = [ [[package]] name = "pgroles-core" -version = "0.7.8" +version = "0.7.9" dependencies = [ "base64", "hmac 0.13.0", @@ -2013,7 +2013,7 @@ dependencies = [ [[package]] name = "pgroles-inspect" -version = "0.7.8" +version = "0.7.9" dependencies = [ "pgroles-core", "sqlx", @@ -2024,7 +2024,7 @@ dependencies = [ [[package]] name = "pgroles-operator" -version = "0.7.8" +version = "0.7.9" dependencies = [ "anyhow", "axum", diff --git a/Cargo.toml b/Cargo.toml index 494cfe1..262eb1f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ members = [ resolver = "2" [workspace.package] -version = "0.7.8" +version = "0.7.9" edition = "2024" authors = ["Brian Thorne "] license = "MIT" @@ -55,8 +55,8 @@ k8s-openapi = { version = "0.27.1", features = ["latest", "schemars"] } schemars = "1" # Internal crates -pgroles-core = { path = "crates/pgroles-core", version = "0.7.8" } -pgroles-inspect = { path = "crates/pgroles-inspect", version = "0.7.8" } +pgroles-core = { path = "crates/pgroles-core", version = "0.7.9" } +pgroles-inspect = { path = "crates/pgroles-inspect", version = "0.7.9" } [profile.release] strip = "symbols" diff --git a/crates/pgroles-operator/src/crd.rs b/crates/pgroles-operator/src/crd.rs index 119be76..5ea060f 100644 --- a/crates/pgroles-operator/src/crd.rs +++ b/crates/pgroles-operator/src/crd.rs @@ -3759,18 +3759,25 @@ retirements: #[test] fn postgres_policy_crd_marks_named_lists_as_maps() { let crd = PostgresPolicy::crd(); - let yaml = serde_yaml::to_string(&crd).expect("CRD should serialize to YAML"); - - for field_name in ["schemas", "roles"] { - assert!( - yaml.contains(&format!("{field_name}:\n")), - "CRD should contain spec.{field_name}" + let value = serde_yaml::to_value(&crd).expect("CRD should serialize to YAML value"); + let spec_properties = &value["spec"]["versions"][0]["schema"]["openAPIV3Schema"]["properties"] + ["spec"]["properties"]; + + for (field_name, map_key) in [ + ("schemas", "name"), + ("roles", "name"), + ("retirements", "role"), + ] { + let field_schema = &spec_properties[field_name]; + assert_eq!( + field_schema["x-kubernetes-list-type"], "map", + "spec.{field_name} should be a Kubernetes map-list" + ); + assert_eq!( + field_schema["x-kubernetes-list-map-keys"][0], map_key, + "spec.{field_name} should be keyed by {map_key}" ); } - assert!(yaml.contains("x-kubernetes-list-type: map")); - assert!(yaml.contains("x-kubernetes-list-map-keys:")); - assert!(yaml.contains("- name")); - assert!(yaml.contains("- role")); } #[test] From aed4fc634064a9a5632ec79d9b1aae8418e6c506 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Sat, 6 Jun 2026 13:39:34 +1200 Subject: [PATCH 3/5] Update E2E policies for manual approval default --- .github/workflows/ci.yml | 3 ++- k8s/samples/basic-policy.yaml | 1 + k8s/samples/disjoint-policy.yaml | 1 + k8s/samples/empty-password-secret-policy.yaml | 1 + k8s/samples/generated-password-policy.yaml | 1 + k8s/samples/insufficient-privileges-policy.yaml | 1 + k8s/samples/missing-password-secret-policy.yaml | 1 + k8s/samples/params-e2e-policy.yaml | 1 + k8s/samples/password-policy.yaml | 1 + k8s/samples/rotated-secret-policy.yaml | 1 + k8s/samples/schema-owner-policy.yaml | 1 + k8s/samples/second-disjoint-policy.yaml | 1 + scripts/generate-load-policy.sh | 1 + 13 files changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e3c3e1..da42479 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -259,7 +259,8 @@ jobs: wait_for_ready_status_reason plan-policy True Planned wait_for_drift_status plan-policy True kubectl get pgr plan-policy -o jsonpath='{.status.last_reconcile_mode}' | grep -qx "plan" - kubectl get pgr plan-policy -o jsonpath='{.status.planned_sql}' | grep -q 'CREATE ROLE \"plan-preview-user\"' + plan_name=$(wait_for_current_plan_ref plan-policy) + get_plan_sql "$plan_name" | grep -q 'CREATE ROLE "plan-preview-user"' assert_role_absent plan-preview-user wait_for_event_reason plan-policy DriftDetected diff --git a/k8s/samples/basic-policy.yaml b/k8s/samples/basic-policy.yaml index e268d5c..3f6ef75 100644 --- a/k8s/samples/basic-policy.yaml +++ b/k8s/samples/basic-policy.yaml @@ -10,6 +10,7 @@ spec: name: postgres-credentials # secretKey defaults to DATABASE_URL interval: "5m" + approval: auto default_owner: postgres profiles: diff --git a/k8s/samples/disjoint-policy.yaml b/k8s/samples/disjoint-policy.yaml index c8cde48..7927311 100644 --- a/k8s/samples/disjoint-policy.yaml +++ b/k8s/samples/disjoint-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: postgres-credentials interval: "5m" + approval: auto roles: - name: analytics diff --git a/k8s/samples/empty-password-secret-policy.yaml b/k8s/samples/empty-password-secret-policy.yaml index 0f21d04..011f1ea 100644 --- a/k8s/samples/empty-password-secret-policy.yaml +++ b/k8s/samples/empty-password-secret-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: postgres-credentials interval: "15s" + approval: auto roles: - name: empty-pw-user diff --git a/k8s/samples/generated-password-policy.yaml b/k8s/samples/generated-password-policy.yaml index 84e0c33..962145f 100644 --- a/k8s/samples/generated-password-policy.yaml +++ b/k8s/samples/generated-password-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: postgres-credentials interval: "15s" # short interval for E2E testing — use "5m" or longer in production + approval: auto roles: - name: genuser diff --git a/k8s/samples/insufficient-privileges-policy.yaml b/k8s/samples/insufficient-privileges-policy.yaml index bb42b89..9ffc8c5 100644 --- a/k8s/samples/insufficient-privileges-policy.yaml +++ b/k8s/samples/insufficient-privileges-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: limited-postgres-credentials interval: "5m" + approval: auto roles: - name: insufficient-privileges-user diff --git a/k8s/samples/missing-password-secret-policy.yaml b/k8s/samples/missing-password-secret-policy.yaml index 46a01d4..75bdb09 100644 --- a/k8s/samples/missing-password-secret-policy.yaml +++ b/k8s/samples/missing-password-secret-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: postgres-credentials interval: "15s" + approval: auto roles: - name: missing-pw-user diff --git a/k8s/samples/params-e2e-policy.yaml b/k8s/samples/params-e2e-policy.yaml index 68ab012..71282d4 100644 --- a/k8s/samples/params-e2e-policy.yaml +++ b/k8s/samples/params-e2e-policy.yaml @@ -18,6 +18,7 @@ spec: name: params-pg-credentials key: password interval: "15s" + approval: auto roles: - name: params-test-role diff --git a/k8s/samples/password-policy.yaml b/k8s/samples/password-policy.yaml index 005fd29..f3d63cc 100644 --- a/k8s/samples/password-policy.yaml +++ b/k8s/samples/password-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: postgres-credentials interval: "15s" + approval: auto roles: - name: password-user diff --git a/k8s/samples/rotated-secret-policy.yaml b/k8s/samples/rotated-secret-policy.yaml index afd1e02..af4065d 100644 --- a/k8s/samples/rotated-secret-policy.yaml +++ b/k8s/samples/rotated-secret-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: rotating-postgres-credentials interval: "5m" + approval: auto roles: - name: rotated-secret-user diff --git a/k8s/samples/schema-owner-policy.yaml b/k8s/samples/schema-owner-policy.yaml index 94ed4a0..89be0c1 100644 --- a/k8s/samples/schema-owner-policy.yaml +++ b/k8s/samples/schema-owner-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: postgres-credentials interval: "5m" + approval: auto schemas: - name: operator_owned_schema diff --git a/k8s/samples/second-disjoint-policy.yaml b/k8s/samples/second-disjoint-policy.yaml index 9f8a00d..2a80845 100644 --- a/k8s/samples/second-disjoint-policy.yaml +++ b/k8s/samples/second-disjoint-policy.yaml @@ -9,6 +9,7 @@ spec: secretRef: name: postgres-credentials interval: "5m" + approval: auto roles: - name: reporting diff --git a/scripts/generate-load-policy.sh b/scripts/generate-load-policy.sh index 9e23975..4f01b18 100755 --- a/scripts/generate-load-policy.sh +++ b/scripts/generate-load-policy.sh @@ -20,6 +20,7 @@ spec: secretRef: name: ${secret_name} interval: "5m" + approval: auto default_owner: postgres profiles: From 92d3dbe377c55883f926466d9a6afef6ca2eb100 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Sat, 6 Jun 2026 15:42:33 +1200 Subject: [PATCH 4/5] Update operator docs for manual approval default --- docs/src/pages/docs/adoption.md | 3 ++- docs/src/pages/docs/bundle-composition.md | 1 + docs/src/pages/docs/operator.md | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/src/pages/docs/adoption.md b/docs/src/pages/docs/adoption.md index 64e656a..e66a85e 100644 --- a/docs/src/pages/docs/adoption.md +++ b/docs/src/pages/docs/adoption.md @@ -43,7 +43,7 @@ spec: reconciliation_mode: additive ``` -The operator will report planned changes in the CRD status, including the full SQL. +The operator will report planned changes in status and store the SQL preview on the generated `PostgresPolicyPlan`. ### 3. Validate with diff @@ -62,6 +62,7 @@ Switch to `mode: apply` with `reconciliation_mode: additive`. This applies all n ```yaml spec: mode: apply + approval: auto reconciliation_mode: additive ``` diff --git a/docs/src/pages/docs/bundle-composition.md b/docs/src/pages/docs/bundle-composition.md index 852b3eb..abc3b52 100644 --- a/docs/src/pages/docs/bundle-composition.md +++ b/docs/src/pages/docs/bundle-composition.md @@ -108,6 +108,7 @@ spec: secretName: app-db secretKey: url mode: apply + approval: auto # ... paste the rendered manifest body here: default_owner: app_owner profiles: diff --git a/docs/src/pages/docs/operator.md b/docs/src/pages/docs/operator.md index 9493fcf..dee13ca 100644 --- a/docs/src/pages/docs/operator.md +++ b/docs/src/pages/docs/operator.md @@ -180,6 +180,7 @@ spec: interval: "5m" # reconciliation interval (supports 5m, 1h, 30s, 1h30m) suspend: false # set true to pause reconciliation mode: apply # apply changes, or use plan for non-mutating drift preview + approval: auto # auto-apply plans; omit or set manual to require approval reconciliation_mode: authoritative # authoritative | additive | adopt default_owner: app_owner From 926c9a96b9121db68d60cad45063062c841d84f9 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 8 Jun 2026 20:05:59 +1200 Subject: [PATCH 5/5] Handle legacy policy status cleanup --- .../crds/postgrespolicies.pgroles.io.yaml | 1 + crates/pgroles-operator/src/crd.rs | 29 +++++++++++ crates/pgroles-operator/src/reconciler.rs | 49 +++++++++++++++++-- docs/src/pages/docs/operator.md | 2 +- k8s/crd.yaml | 1 + 5 files changed, 78 insertions(+), 4 deletions(-) diff --git a/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml b/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml index 03f0096..f6f360a 100644 --- a/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml +++ b/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml @@ -67,6 +67,7 @@ "description": "Spec for a `PostgresPolicy` custom resource.\n\nDefines the desired state of PostgreSQL roles, grants, default privileges,\nand memberships for a single database connection.", "properties": { "approval": { + "default": "manual", "description": "Approval mode for plans generated by this policy.", "enum": [ "manual", diff --git a/crates/pgroles-operator/src/crd.rs b/crates/pgroles-operator/src/crd.rs index 5ea060f..28a1c9b 100644 --- a/crates/pgroles-operator/src/crd.rs +++ b/crates/pgroles-operator/src/crd.rs @@ -120,8 +120,10 @@ pub struct PostgresPolicySpec { /// Approval mode for plans: `auto` or `manual`. /// When `manual`, plans require explicit approval before execution. /// When `auto`, plans are approved and applied immediately. + /// Only affects `mode: apply`; `mode: plan` never executes SQL. /// When omitted, defaults to `manual`. #[serde(default, skip_serializing_if = "Option::is_none")] + #[schemars(extend("default" = "manual"))] pub approval: Option, } @@ -3778,6 +3780,11 @@ retirements: "spec.{field_name} should be keyed by {map_key}" ); } + + assert_eq!( + spec_properties["approval"]["default"], "manual", + "spec.approval should advertise the runtime default in the CRD schema" + ); } #[test] @@ -3906,6 +3913,28 @@ retirements: ); } + #[test] + fn status_with_removed_legacy_fields_still_deserializes() { + let json = serde_json::json!({ + "conditions": [], + "last_reconcile_time": "2026-06-01T00:00:00Z", + "last_successful_reconcile_time": "2026-06-01T00:00:00Z", + "planned_sql": "CREATE ROLE \"legacy\";", + "planned_sql_truncated": false, + "owned_roles": [], + "owned_schemas": [] + }); + + let status: PostgresPolicyStatus = serde_json::from_value(json) + .expect("old stored status fields should not block deserialization"); + + assert_eq!( + status.last_successful_reconcile_time.as_deref(), + Some("2026-06-01T00:00:00Z") + ); + assert!(status.current_plan_ref.is_none()); + } + #[test] fn effective_approval_explicit_auto_overrides_plan_mode() { let spec = PostgresPolicySpec { diff --git a/crates/pgroles-operator/src/reconciler.rs b/crates/pgroles-operator/src/reconciler.rs index c8b9a6d..f8baefe 100644 --- a/crates/pgroles-operator/src/reconciler.rs +++ b/crates/pgroles-operator/src/reconciler.rs @@ -1955,9 +1955,9 @@ where mutate(&mut status); - let patch = serde_json::json!({ - "status": status - }); + let patch = status_patch_with_removed_legacy_fields(&status).map_err(|err| { + ReconcileError::InvalidSpec(format!("failed to serialize status patch: {err}")) + })?; api.patch_status( &name, @@ -1975,6 +1975,23 @@ where Ok(()) } +fn status_patch_with_removed_legacy_fields( + status: &PostgresPolicyStatus, +) -> Result { + let mut status_value = serde_json::to_value(status)?; + if let Some(status_object) = status_value.as_object_mut() { + // JSON merge-patch only deletes fields that are explicitly set to null. + // Keep sending nulls until all pre-removal status keys have aged out of clusters. + status_object.insert("last_reconcile_time".to_string(), serde_json::Value::Null); + status_object.insert("planned_sql".to_string(), serde_json::Value::Null); + status_object.insert("planned_sql_truncated".to_string(), serde_json::Value::Null); + } + + Ok(serde_json::json!({ + "status": status_value, + })) +} + async fn detect_policy_conflict( ctx: &OperatorContext, resource: &PostgresPolicy, @@ -2833,6 +2850,32 @@ mod tests { assert_eq!(status.transient_failure_count, 3); } + #[test] + fn status_patch_explicitly_removes_legacy_status_fields() { + let status = PostgresPolicyStatus { + last_successful_reconcile_time: Some("2026-06-01T00:00:00Z".into()), + current_plan_ref: Some(crate::crd::PlanReference { + name: "current-plan".into(), + }), + ..Default::default() + }; + + let patch = status_patch_with_removed_legacy_fields(&status) + .expect("status should serialize to merge patch"); + + assert_eq!( + patch["status"]["last_successful_reconcile_time"], + "2026-06-01T00:00:00Z" + ); + assert_eq!(patch["status"]["current_plan_ref"]["name"], "current-plan"); + assert!(patch["status"].get("last_reconcile_time").is_some()); + assert!(patch["status"].get("planned_sql").is_some()); + assert!(patch["status"].get("planned_sql_truncated").is_some()); + assert!(patch["status"]["last_reconcile_time"].is_null()); + assert!(patch["status"]["planned_sql"].is_null()); + assert!(patch["status"]["planned_sql_truncated"].is_null()); + } + #[test] fn error_display_missing_database_objects_lists_schemas() { let err = ReconcileError::MissingDatabaseObjects("schema \"etl\", schema \"jobs\"".into()); diff --git a/docs/src/pages/docs/operator.md b/docs/src/pages/docs/operator.md index dee13ca..26f7a70 100644 --- a/docs/src/pages/docs/operator.md +++ b/docs/src/pages/docs/operator.md @@ -180,7 +180,7 @@ spec: interval: "5m" # reconciliation interval (supports 5m, 1h, 30s, 1h30m) suspend: false # set true to pause reconciliation mode: apply # apply changes, or use plan for non-mutating drift preview - approval: auto # auto-apply plans; omit or set manual to require approval + approval: auto # mode: apply only; omit or set manual to require approval reconciliation_mode: authoritative # authoritative | additive | adopt default_owner: app_owner diff --git a/k8s/crd.yaml b/k8s/crd.yaml index 03f0096..f6f360a 100644 --- a/k8s/crd.yaml +++ b/k8s/crd.yaml @@ -67,6 +67,7 @@ "description": "Spec for a `PostgresPolicy` custom resource.\n\nDefines the desired state of PostgreSQL roles, grants, default privileges,\nand memberships for a single database connection.", "properties": { "approval": { + "default": "manual", "description": "Approval mode for plans generated by this policy.", "enum": [ "manual",