feat(canopy): build reporting schemas from migrated restores - #135
feat(canopy): build reporting schemas from migrated restores#135dannash100 wants to merge 15 commits into
Conversation
|
🤖 Known gap, left until canopy#553 is deployed: the build outcome isn't reported back, so no pair settles, the worklist re-dispatches every pass, and a failed build raises no check.
Order: deploy canopy#553, rebuild, then |
| /// than one it went looking for. | ||
| #[expect( | ||
| clippy::too_many_arguments, | ||
| reason = "internal builder with tightly-coupled params" |
There was a problem hiding this comment.
Meh, make a struct, it will be way clearer than 10 positional params
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary Below consensus threshold (4 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent) |
|
|
||
| /// The canopy group this replica's data belongs to. Required both to build | ||
| /// against the right configuration and to register the result. | ||
| fn build_group(replica: &PostgresPhysicalReplica) -> Option<Uuid> { |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
build_group reads only the replica CR's canopy.../group label, but the verification path deliberately does not trust that: canopy_ids + fill_missing_labels/namespace_labels (verification.rs:207-286) fall back to the namespace's copy of the same label set precisely because a CR can be missing it. Here a replica without the label silently records NO_GROUP and skips the build entirely — the restore is discarded and nothing is ever built for that group/version pair. Reuse the same namespace fallback (or read spec.canopy_source.group, which the syncer always sets) instead of the bare label lookup.
|
|
||
| let reader_secret_name = replica.creds_secret_name(); | ||
| let secrets: Api<Secret> = Api::namespaced(client.clone(), namespace); | ||
| let reader_secret = secrets.get(&reader_secret_name).await?; |
There was a problem hiding this comment.
[Bugs & Correctness] critical
The pre-creation path of the build gate has no failure exit: secrets.get(...).await? and discover_restore_database(...).await? propagate errors out of reconcile_schema_build, and the gate only ever returns Ok(true) once a Job has been created and observed settled. If the reader secret is missing or the restore's Postgres is unreachable at this moment, the reconcile errors, requeues, and re-enters the same branch forever — the restore never leaves Switching, and for an ephemeral reporting-schema replica it is never discarded either. That directly contradicts the stated invariant that a failed build does not fail the restore (the Job's active_deadline_seconds only bounds a Job that got created). Wrap these two calls so a failure records a SchemaBuildResult { built: false, error: ... } and returns Ok(true), as the NO_GROUP branch already does.
| /// [`ensure_build_job`] a no-op and settles that pair as having produced no | ||
| /// schema without ever running. | ||
| pub async fn delete_build_job(client: &Client, namespace: &str, job_name: &str) { | ||
| let jobs: Api<Job> = Api::namespaced(client.clone(), namespace); |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
jobs.delete(job_name, &Default::default()) deletes the Job with no propagation policy, which for batch/v1 orphans its pods — the completed build pod is left behind holding its 2Gi memory limit's worth of node accounting and never gets collected, since its owner is gone. The repo already knows this: replica.rs:1679 uses kube::api::DeleteParams::background() with the comment "background propagation so its pods are GC'd too". Use DeleteParams::background() here too.
|
|
||
| let Some(group) = build_group(replica) else { | ||
| record_schema_build( | ||
| client, |
There was a problem hiding this comment.
[Design & Architecture] suggestion
In the create path the group check runs last, after fetching the credentials secret and after discover_restore_database, which opens a connection to the restore. When the replica carries no parseable group label, all of that work is done and thrown away to record NO_GROUP. build_group(replica) is a pure label read — hoist it above the secret/discovery block (or fold it into build_to_do, which is already the "has this reconcile anything to do" predicate) so the cheap precondition gates the expensive work. That also removes the second build_group call in the Succeeded arm, which currently re-derives the same value and duplicates the NO_GROUP handling.
| } | ||
|
|
||
| /// Whether the build Job has finished, and how. | ||
| pub enum BuildOutcome { |
There was a problem hiding this comment.
[Design & Architecture] suggestion
BuildOutcome has no variant for "no Job yet": build_outcome returns Running both when the Job is absent and when it is active. The caller then has to re-get_opt inside the Running arm to tell the two apart (replica.rs:1796), and ensure_build_job does a third get_opt before creating. Three API round-trips and a control flow where the comment // Not created yet on the first pass through. sits above a check that means the opposite. The existing migration gate (reconcile_schema_migration, replica.rs:2059) does one get_opt and matches Some(job)/else-create. Either add a NotStarted variant or return Option<BuildStatus>, and drop the duplicate existence checks.
| pub snapshot_results: Arc<CallbackStore>, | ||
| /// In-memory store for schema migration results POSTed by jobs. | ||
| pub schema_migration_results: Arc<CallbackStore>, | ||
| /// Reporting schemas posted back by build Jobs, keyed by namespace and |
There was a problem hiding this comment.
[Performance] suggestion
schema_build_results is an unbounded in-memory map whose entries are up to 32 MiB each, and nothing evicts an entry unless the build gate reaches its Succeeded/Failed branch. A payload posted after the pair is Settled, after the restore was deleted or switched over, or for a replica whose switching_restore is None, stays in the map for the operator's lifetime — one leaked entry per replica, at 32 MiB apiece, versus the kilobyte-sized payloads the other CallbackStores hold. Consider evicting on replica/restore teardown or storing entries with an insertion timestamp and dropping stale ones during reconcile.
| StatusCode::NO_CONTENT | ||
| } | ||
|
|
||
| async fn post_schema_build_results( |
There was a problem hiding this comment.
[Security] critical
post_schema_build_results is unauthenticated and unvalidated: the handler accepts any body for any {namespace}/{replica} path and stores it verbatim, and reconcile_schema_build then ships whatever is under that key to canopy via register_reporting_schema as a group-scoped artifact of the exact version — SQL that downstream consumers execute. Any pod that can reach the operator's HTTP port (including the third-party builderImage, which is handed this URL and is by design outside pgro's trust) can race or simply overwrite the real build's output and get arbitrary SQL published under another deployment's group. The other callbacks share the no-auth shape but only ever feed status text; this one is a publish path, so it needs its own proof of origin — e.g. mint a per-build nonce, pass it to the Job as the callback path segment or a bearer header, and reject a POST whose token doesn't match the currently-running build for that replica.
| axum::routing::post(post_schema_migration_results), | ||
| ) | ||
| .route( | ||
| "/api/v1/schema-build-results/{namespace}/{replica}", |
There was a problem hiding this comment.
[Security] suggestion
The 32 MiB body limit combined with CallbackStore's unbounded HashMap makes this endpoint a cheap memory-exhaustion vector: the key is caller-supplied ({namespace}/{replica} is never checked against an existing replica or a build that is actually running), and an entry is only ever removed by a reconcile that finds a matching build Job. A few dozen POSTs to invented names pin gigabytes in the operator process for its lifetime, with no eviction path. Reject the callback when there is no in-flight build Job for that namespace/replica (or at minimum when the replica does not exist), and cap the number of retained entries.
| env: Some(vec![ | ||
| env_literal("TAMANU_DL_DB_URL", &host), | ||
| env_literal("TAMANU_DL_DB_USER", user), | ||
| env_literal("TAMANU_DL_DB_PASSWORD", password), |
There was a problem hiding this comment.
[Security] critical
The restore's Postgres password is embedded as a literal env value in the build Job spec, so the plaintext credential is stored in the Job/Pod object (visible to anyone with get jobs/get pods in the namespace, to kubectl describe, to audit logs and to etcd) rather than only in the Secret. Every other Job in this repo takes credentials by reference — see src/controllers/restore/migration.rs:104-105 and src/controllers/replica/schema_migration.rs:203-213, which use env_from_secret_name(..., reader_secret_name, "password"). Do the same here: pass replica.creds_secret_name() into SchemaBuildArgs and build TAMANU_DL_DB_USER/TAMANU_DL_DB_PASSWORD with env_from_secret_name, dropping the secrets.get(...)/read_secret_field fetch of the password in reconcile_schema_build (the username is still needed for discover_restore_database, the password is not needed in operator memory to template the Job).
| /// Canopy authorises this exact path, and a build whose registration misses it | ||
| /// reports a healthy restore and publishes nothing, so the drift is silent. | ||
| fn registration_uri(version: &str, group: Uuid, run_id: Option<Uuid>) -> String { | ||
| let mut uri = format!("/artifacts/{version}/reporting-schema/any?group={group}"); |
There was a problem hiding this comment.
[Security] critical
version is interpolated raw into the registration URI with no validation or percent-encoding. MigrationTarget.version (src/types/restore.rs:149) is a free-form String taken from the canopy worklist entry or the CRD — nothing anywhere validates it as semver. A value containing /, ?, # or & rewrites the request target: e.g. 2.60.0?group=<other-group>&x= yields /artifacts/2.60.0?group=OTHER&x=/reporting-schema/any?group=REAL, where the first ? starts the query and the group scoping is attacker-chosen, and ../.. walks to a different canopy endpoint entirely. Since the tests in this file assert the exact authorised path is the only thing protecting group scoping, reject or encode the version before building the URI — validate it against a strict [A-Za-z0-9.+-] / semver pattern and return Error::Canopy otherwise, or percent-encode the path segment.
|
🦸 Review Hero Summary Below consensus threshold (15 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent) |
2 participants
A reporting schema follows from a Tamanu version's schema and a group's configuration together, so it can only be built against a database of that group at that version. A migrated restore is the only place one exists.
reporting-schemaintent: restore, migrate to the version canopy names, build before switchover, register, discard.builder_imagenames. pgro hands it a database, version and group and takes back SQL over a callback, since a Job's termination message is 4 KiB.🦸 Review Hero