fix(postgresql): declare KubeBlocks 1.2 API floor#3225
Open
weicao wants to merge 66 commits into
Open
Conversation
…arameter change The reconfigure reloader script fired three curl calls (PATCH /config, then POST /reload or /restart) with -s and no status checking. When patroni rejected a parameter change or was unreachable, the script still exited 0, so the reconfigure action reported success while the desired parameter state was never applied. This is exactly the half-wired reconfigure path docs/addon-api/08-parameters-and-config.md warns about: the wrong desired state gets carried to new pods on restart/scale/upgrade. Changes: - wrap all patroni REST calls in patroni_api(), which propagates curl connection failures and any non-2xx HTTP response as a script failure (the action wrapper runs with set -eu, so the action now fails), and logs the patroni response body for diagnosis - skip /reload | /restart when the config PATCH already failed - restructure into update_parameter() with the __SOURCED__ guard used by sibling addons so the script is unit-testable - add shellspec coverage: parameter routing (PG GUC vs patroni DCS vs restart-class) and error propagation (4xx, unreachable, failed reload/restart); the script previously had zero coverage - add missing trailing newline in patroni-parameter.yaml Fixes #3006 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… instead of reporting success Both Selective restore paths could report success without restoring anything: - pgdumpall-restore.sh: when the backup artifact was absent from the repository, the single if-block was skipped and the script fell through to an implicit exit 0 — a missing backup restored nothing and the restore job succeeded. - pgdump-restore.sh: ran without set -e, so a failed datasafed pull (missing/corrupt artifact) did not stop the script; and the 'errors ignored on restore' grep forced exit 0 for every conflict policy, including FAIL. - actionset-pgdump.yaml: conflict_policy was declared in parametersSchema (enum CONTINUE/FAIL/DROP) and branched on by the restore script, but was never listed in restore.withParameters, so the env was never injected and the DROP/FAIL branches were dead code — every restore silently ran as CONTINUE. Changes: - both restore scripts verify the backup artifact exists in the repository before pulling and exit 1 with a clear error otherwise - pgdump-restore.sh runs under set -e; the pg_restore exit code is captured explicitly and the ignored-errors success downgrade now applies only to non-FAIL policies - pg_restore stderr goes through a synchronous tee instead of an async process substitution, so the log the policy decision greps is complete by the time it is read - conflict_policy added to restore.withParameters - shellspec coverage for both scripts (10 examples): missing artifact, success path, psql/pg_restore/datasafed failures, and CONTINUE vs FAIL policy semantics Fixes #3008 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… success switchover.sh fired the patroni /switchover request with curl -s and never looked at the result (two in-code TODOs acknowledged this): - connection errors and non-2xx responses were swallowed; the action exited 0 and KubeBlocks marked the switchover succeeded even when patroni rejected it - the result was never confirmed, violating the 05b lifecycle contract that the no-candidate path must verify the switchover outcome itself - the leader query matched only role=="leader", so standby clusters (standby_leader) could never resolve a leader - when patroni was unreachable, the empty leader name fell into the "not primary" branch and the script silently exited 0 Changes, following the single-shot bounded-action pattern: - request_switchover() checks the HTTP status: 2xx proceeds, 5xx (e.g. 503 no good candidates) defers as transient, other codes fail hard; connection errors fail with classification - verify_switchover() positively confirms the outcome within a bounded window (4 x 2s): leadership must leave the old leader, and when a candidate was requested it must be the new leader - classified stderr diagnostics (phase + next-retry-safe) on every failure path - leader resolution matches leader and standby_leader - unresolved leader is a classified failure, not a silent success; leader/label skew reports success only when the desired end state is positively observed (candidate already leader, or leadership already moved with no candidate) - cmpd switchover action pins timeoutSeconds: 50 (script worst case ~45s under the 60s kbagent clamp) - switchover_spec.sh rewritten: 13 examples covering success with and without candidate, standby_leader, 503/412/unreachable, unconfirmed result, and wrong-leader outcomes (previous spec had 1 live case and ~120 lines of commented-out remnants) Fixes #3007 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p retry logic
Both Continuous backup ActionSets injected set -e ahead of long-running
archiver scripts whose error handling is written for set +e semantics:
- actionset-wal-g-pitr.yaml + wal-g-archive.sh: uploadMissingLogs'
tracking-file retry system ('eval wal-g wal-push; exit_code=$?' and
the failure branch that preserves the tracking file) was unreachable
— the first failed wal-push killed the whole process. Same for
uploadDoneHistoryWALs and for check_pg_process' bounded 3-probe loop
whose rescue path (upload remaining WALs before exit) never ran.
During a failover window the Continuous backup pod crash-looped
instead of retrying gracefully as designed.
- actionset-postgresql-pitr.yaml + postgresql-pitr-backup.sh: same
pattern (check_pg_process retry, per-file upload tolerance).
Per the audit rule in the addon-docs guide (audit what set -e was
blocking before touching the wrapper), the one place where set -e was
load-bearing is postgresql-pitr-backup.sh upload_wal_log: it pushed
and then renamed .ready to .done unchecked, and set -e was the only
thing preventing a failed push from being marked done (which would let
PostgreSQL recycle a never-uploaded WAL segment and hole the PITR
chain). That statement now has an explicit success gate before the
wrapper change.
Changes:
- both Continuous backupData wrappers: set -e removed (pipefail kept),
with a comment stating the error contract is script-owned; the
restore-side wrapper of actionset-postgresql-pitr keeps set -e
(one-shot job, fail-fast is correct there)
- postgresql-pitr-backup.sh: .ready -> .done rename only after a
successful push; failed uploads logged and kept for retry; cd and
pg_switch_wal failures logged instead of silently ignored
- wal-g-archive.sh: bootstrap (mkdir/cp of wal-g env) fails fast
explicitly, since the wrapper no longer does it
- new specs (11 examples) extract the archiver functions via awk shim
(scripts run infinite loops at top level) and verify: failed upload
keeps .ready + tracking file, successful upload renames and clears,
recent tracking file suppresses immediate retry, 3-probe failure
runs the rescue upload then exits 1, and a failed upload is never
marked .done
Fixes #3010
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The addon's authentication chain was pinned to md5 on PG 12-17: password_encryption = 'md5' in every config template except pg18, pgbouncer auth_type = md5, and pg_hba md5 entries. PostgreSQL has defaulted to scram-sha-256 since v14, and v18 (which this addon ships) deprecates md5 password storage. Staying on md5 was an adaptation-to-tooling choice that diverges from the database's own practice (deep-review finding 二.11, issue priority approved by weston). Changes: - password_encryption = 'scram-sha-256' in pg12/14/15/16/17 config templates (pg18 already had it); PG >= 10 supports SCRAM, so every supported version takes this - pgbouncer auth_type md5 -> scram-sha-256 (both the ini template and the bitnami PGBOUNCER_AUTH_TYPE env): with SCRAM-stored verifiers in pg_shadow, an md5 auth_type cannot verify auth_query accounts at all, so this flip is required for consistency, not just hygiene; the userlist entry is plaintext and works under either type - pg_hba keeps the dual-mode 'md5' method (PostgreSQL automatically performs SCRAM on the wire when the stored verifier is SCRAM), so accounts created by older addon versions with md5-stored passwords keep working after an upgrade; the stale 'TODO: check if it should trust all' comment is replaced with the actual rationale - spilo initdb 'auth-host: md5' kept for the same dual-mode reason - new auth_contract_spec.sh (6 examples) pins the posture: every version template stores SCRAM, pgbouncer is SCRAM, remote pg_hba entries are md5 (dual) and never trust Upgrade note: on clusters created before this change, accounts whose passwords are md5-stored keep working for direct PostgreSQL connections (dual-mode hba) but cannot authenticate through pgbouncer via auth_query until their password is re-set (ALTER USER ... PASSWORD re-hashes with the new password_encryption). This matches PostgreSQL's own documented md5->scram migration path. Fixes 二.11 of the 2026-07-03 deep review Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The portable date -r stub tried the BSD stat form first; on GNU coreutils 'stat -f %m <file>' is not an error (it prints the filesystem mount point), so the fallback never fired and CI's tracking-file age arithmetic got a path string instead of an mtime. Try GNU 'stat -c %Y' first, fall back to BSD 'stat -f %m'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per wangyelei's review: the no-candidate path now relies on patroni's synchronous /switchover response alone — the checked 2xx already confirms completion, and roleProbe proves role convergence afterwards. The candidate path keeps the bounded verification that the requested candidate actually became leader. HTTP error propagation (4xx/5xx, unreachable) is unchanged on both paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aths
Two credential-bearing strings went through eval:
- accountProvision ran 'eval statement="${KB_ACCOUNT_STATEMENT}"' to
expand the ${KB_ACCOUNT_NAME}/${KB_ACCOUNT_PASSWORD} placeholders,
putting the password through a full shell parse: metacharacters in a
user-supplied secret execute instead of substituting. Replaced with
bash pattern substitution, whose replacement is literal (same
contract mariadb-galera implements with printf|sed). Verified: a
password of x";touch /tmp/pwned;"$(id) stays a literal string.
- wal-g-archive.sh built an 'env NAME="value" ...' prefix from the
wal-g env dir (including DATASAFED_ENCRYPTION_PASS_PHRASE) and
eval'd it, re-parsing values and exposing the passphrase on the
command line. Replaced with envdir on the same directory — the
pattern this addon already uses in five other call sites (the
archive_command itself among them), so tool availability in the
image is already proven.
SQL-literal boundary: system-account passwords are generated with
numSymbols: 0, so quote characters cannot reach the SQL string today;
this PR fixes the shell layer, not SQL quoting.
Interaction note: PR #3036 (approved, unmerged) touches other parts of
wal-g-archive.sh and adds its spec; diffs do not overlap. Whichever
merges second, I will follow up with the one-line envdir stub in the
spec.
Fixes #3052
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…host and config version lookup
Two 'works only under default naming' assumptions:
- wal-g-backup.sh / wal-g-incremental-backup.sh assembled the primary
Service host as ${CLUSTER_COMPONENT_NAME}-${COMPONENT_NAME}; the
Service is <cluster>-<comp>-<serviceName> and the cmpd pins
serviceName: postgresql, so this only resolved when the component
short name happened to be 'postgresql'. The suffix is now the fixed
serviceName. (DP_DB_HOST is not a substitute here: pg_switch_wal()
must run on the primary and the BPT target has
fallbackRole: secondary.)
- pg14/15/16-config.tpl matched $spec.name == 'postgresql' literally
to find serviceVersion, silently falling back to 14.0/15.0/16.0 for
any other component name — which silently rendered the older
shared_preload_libraries list (no pg_duckdb). The match now also
accepts componentDef prefix 'postgresql', covering
direct-componentDef clusters with custom names; the per-major
default remains only for the legitimate omitted-serviceVersion case.
Note for review: these config templates render in KB's config engine;
hasPrefix/default are sprig functions, availability asserted from the
templates' existing sprig usage (semverCompare/div/min).
Fixes #3058
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pping misses Four ActionSets defaulted IMAGE_TAG to 14.8.0-pgvector-v0.6.1. The BPT's per-method versionMapping overrides it for every serviceVersion declared in the chart, so the default only ever fired on a mapping miss (e.g. a custom ComponentVersion minor) — silently running a PG 16/17/18 cluster's backup/restore tooling with the PG 14 image. The defaults are removed; on a miss the image now renders with a literal $(IMAGE_TAG) tag and the job fails visibly at image resolution, naming the unexpanded variable. pgdumpall's env block became empty and is dropped. wal-g / wal-g-incremental are unaffected (fixed walgImage helper, no IMAGE_TAG). Fixes #3062 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…omparison numeric
- restore_command moved WAL segments out of the staging dir as
recovery consumed them ('mv ${PITR_DIR}/%f %p'); a mid-recovery
restart (pod eviction, OOM — routine on Kubernetes) re-requests
earlier segments through restore_command and they are gone, so the
restore can never complete. The .history branch already used cp,
conceding the hazard. Now cp for all files, matching PostgreSQL's
own documented restore_command examples. Disk note: fetch-wal-log
stages the entire window in PITR_DIR up front, so the peak is
already paid; cp adds only transient duplication in pg_wal bounded
by recovery checkpoint recycling.
- fetch-wal-log's stop condition used [[ $timestamp > $restore_time ]]
— lexicographic. Both operands are epoch seconds (currently equal
width, so accidentally correct); now -gt.
- new pitr_restore_spec.sh (3 examples): runs the same concatenation
the ActionSet builds (set -e + common + fetch + restore) and pins
the cp restore_command + staging/idempotent-retry behavior; the
timestamp case uses a 9-digit vs 10-digit epoch pair that fails
under lexicographic comparison and passes under -gt.
Fixes #3071
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p is set
The archive-wal Continuous prepareData (postgresql-pitr-restore.sh) runs
after the Full restore in a PITR chain and expects the base payload to
still live in DATA_DIR (it reads ${DATA_DIR}/pg_wal to pick the fetch
start segment). Unconditional staging into DATA_DIR.old plus hook/signal
arming broke that chain: the Continuous job failed on the empty directory
on every retry, and the leaked kb_restore.signal armed a bootstrap branch
whose legacy hook mishandles --replica.
Mirror wal-g-restore.sh: when DP_RESTORE_TIMESTAMP/DP_RESTORE_TIME is set,
verify and finish with the data left in place; the Continuous stage owns
recovery configuration and hook setup for the chain. Adds the
combined-state spec case (timestamp set => data unstaged, no hook/signal).
Per integrated-tree review: #3112 (comment)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PgBouncer's auth_type semantics are asymmetric to the server's pg_hba: auth_type = md5 is the dual-mode setting (it performs SCRAM automatically when the secret fetched via auth_query is a SCRAM verifier), while auth_type = scram-sha-256 is SCRAM-only and cannot use md5-hashed secrets. Pinning scram-sha-256 locked accounts created by older addon versions (md5-stored verifiers) out of the pooler path after an upgrade — the exact scenario the pg_hba md5 retention in this PR is designed to protect. Revert the ini tpl and PGBOUNCER_AUTH_TYPE to md5, and pin the corrected contract (including the cmpd/ini alignment) in auth_contract_spec. Per integrated-tree review: #3043 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… patsub_replacement and escape SQL quotes
bash >= 5.2 enables patsub_replacement by default: in an unquoted pattern
substitution replacement, & expands to the matched pattern and backslash
escapes — a password containing either was silently rewritten (account
created with a wrong password; verified 5.3 vs 3.2). Disable the shopt
before substituting (no-op on older bash).
Also close the SQL-literal boundary in the same block: escape embedded
single quotes ('' escape, portable form via a quote variable — the
backslash-escaped replacement form itself misbehaves on bash 3.2) so a
quote in a user-supplied password can no longer break the statement, leak
through psql's error context, or inject SQL in the superuser session.
Per integrated-tree review: #3054 (comment)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
added 21 commits
July 12, 2026 14:07
# Conflicts: # addons/postgresql/dataprotection/pgdumpall-restore.sh
leon-ape
reviewed
Jul 20, 2026
leon-ape
left a comment
leon-ape
left a comment
Contributor
There was a problem hiding this comment.
The current head is not a compatibility-annotation-only change: it also changes backup/restore, PITR, switchover, reconfigure, startup, authentication, and metrics runtime paths across 52 files. The reported validation is static/ShellSpec only and contains no current-head runtime evidence for these paths, so it does not cover the code that would be merged.
added 2 commits
July 21, 2026 22:20
# Conflicts: # addons/postgresql/dataprotection/postgresql-pitr-restore.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The PostgreSQL 1.2 chart renders the KubeBlocks 1.2 reconfigure contract:
ComponentDefinition.spec.configs[].reconfigure.execParametersDefinitioncomponent/service/template bindingsbut
Chart.yamlstill advertisesaddon.kubeblocks.io/kubeblocks-version: ">=1.0.0".A read-only server dry-run against KubeBlocks 1.0.3 rejected all six versioned ComponentDefinitions on the unknown
reconfigurefield and warned on the newer ParametersDefinition fields. Installing by the old annotation therefore cannot preserve the chart's reconfigure behavior.KubeBlocks 1.0 remains supported by the separate
release-1.0chart line, which uses the native PDreloadAction.shellTriggercontract.Change
>=1.2.0./kb-scripts/update-parameter.shpath;Verification
helm lint addons/postgresql: PASSgit diff --check: PASSThis PR changes the declared compatibility contract and offline regression coverage only. It does not upgrade any KubeBlocks environment or rewrite existing runtime-test results.