Collection hardening: correctness/security fixes, validation, packaging, and full test coverage (0.13.0) - #152
Draft
gene-redpanda wants to merge 69 commits into
Draft
Collection hardening: correctness/security fixes, validation, packaging, and full test coverage (0.13.0)#152gene-redpanda wants to merge 69 commits into
gene-redpanda wants to merge 69 commits into
Conversation
…book The restart_required test previously ran a copied-and-pasted version of the role's registration and restart-decision logic, which had already drifted from start-redpanda.yml and could not fail when the real file broke. The test playbook now includes the actual start-redpanda.yml, isolates the restart-detection tasks with --tags, and serves rpk output from a PATH mock, so the role's own register/set_fact flow is what gets asserted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The assert required fips-mode-setup to report *disabled* (rc 2), so the supported configuration -- OS FIPS enabled with fips_mode=enabled -- failed the play, while a disabled-FIPS host with fips_mode=permissive passed. The assertion now requires rc 0 whenever fips_mode is 'enabled', which also rejects the previously-accepted 'inconsistent' state (rc 1), and is guarded to RedHat like the check that feeds it. Covered by a new test that mocks /usr/bin/fips-mode-setup for all four rc/fips_mode combinations; the enabled/enabled case fails against the old assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
'Check if restart needed' and its no-auth twin both registered restart_required_rc; Ansible registers a result for skipped tasks too, so whichever twin was skipped overwrote the real result. With SASL enabled the no-auth twin always ran second, leaving a skipped-task stub whose missing stdout defaulted to empty, so a cluster-config change that required a restart never triggered one. The two tasks are now a single task using the conditional-environment idiom already used by the other rpk invocations in this file; both tags are kept so existing --tags/--skip-tags filters are unaffected. The new auth-enabled test cases fail against the twin-task version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The restart decision matched the strings 'Removed'/'1 upgraded' against package_result.results, but apt returns stdout (not results) and the dnf install is a loop whose results is a list of dicts -- so the clause was unsatisfiable on both platforms and redpanda_install_status=latest upgrades left every node running the old binary. The decision now uses the module's aggregate changed flag, which is set by both apt and the dnf loop, and remains gated on is_initialized so fresh installs don't double-restart. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pre/post-24.2 install twins in install-rp-deb.yml and install-rp-rpm.yml both registered package_result, and Ansible registers skipped tasks too -- so with a pinned pre-24.2 redpanda_version, the skipped post-split task overwrote the real result with changed=false. 'Stop redpanda after installation (for SASL bootstrap)' then never fired, letting the cluster initialize before the bootstrap superuser existed, and package upgrades on those pins could never trigger a restart. Each file now has a single install task whose package list is computed from needs_split_packages (the rpm path also gains a single dnf transaction instead of a per-package loop). The same latent pattern is removed from the nodeconfig first-run task and the airgap rpm installs. A new structural test parses every broker task file and fails on any register name used by more than one task, so this bug class can't return; it failed on four files before this change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both airgap install files gated the package split on the string comparison redpanda_version >= '24.2', and '24.10' sorts before '24.2' lexicographically -- so airgap installs of any double-digit minor (24.10.x, 24.11.x) ran the pre-split dpkg/rpm glob, skipping redpanda-rpk and redpanda-tuner and leaving rpk absent for everything downstream. The gate now uses the same version() test as the online install path. The new test drives the real set_fact from both airgap files across the version matrix; 24.10/24.11 fail against the string comparison. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The license check matched stdout against 'status.*valid', and 'valid' is a substring of 'invalid' (and matches 'not valid'), so a cluster with an expired or invalid license skipped every license-apply task while the play reported success -- leaving enterprise features (FIPS, SR authorization, OIDC) silently off. The match is now anchored to a status line whose value is exactly 'valid'. Covered by a new test that serves rpk license output from a PATH mock; the 'invalid' and 'not valid' cases fail against the substring match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
'Establish node id' piped rpk through sed/grep/awk with awk last, so an rpk error or an empty broker table still exited 0, and stdout|int turned the empty string into node id 0 -- after which safe-restart drained and un-drained broker 0 from the wrong host and restart detection read broker 0's row. The task now runs with pipefail, fails explicitly on empty output, and retries (node_id_retries x node_id_retry_delay, default 12x5s) to give a just-started broker time to serve cluster status -- the role previously had no readiness wait at all here. New tests cover the empty-table and rpk-error paths; both previously passed silently with node_id 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
install-certs wrote the node's TLS private key 0644 and the role wrote /etc/redpanda/redpanda.yaml and .bootstrap.yaml 0644; with SASL enabled redpanda.yaml carries rpk.pass and service-account SCRAM passwords, so any local user could read the superuser credentials and the node key. The key is now 0600 and both config writes use redpanda_config_file_mode (default 0640, owner/group redpanda) so operators can loosen it deliberately if they must. The new file-modes test deploys the real cert-install and first-run config tasks in the test container and stats the results; the key and config assertions fail against the 0644 versions. Test playbooks gain a tests/templates symlink so role templates resolve when task files are included from the tests directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ead env Three related bootstrap/entry fixes, each covered by a new red-first test: - The bootstrap superuser conf was written into /etc/redpanda.d without creating it, so on images where the package doesn't ship that directory the very first SASL bootstrap failed. The directory is now ensured. - 'Start redpanda (with bootstrap user for SASL)' set RP_BOOTSTRAP_USER in the task environment, which applies to the systemctl invocation and never reaches the redpanda unit -- misleading dead configuration whose forced no_log also hid start failures. The env is gone and the two start twins collapse into a single task (tags preserved). - The redpanda_version entry assert evaluated 'redpanda_version != ""' first, so a genuinely undefined variable produced a raw Jinja error instead of the assert's actionable fail_msg; and the proxied GPG import in configure-rpm-repository was gated on https_proxy_value while exporting rpm_proxy, running the proxy branch with no proxy when only the gate variable is set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t's state 'Set cluster config' (run_once) was gated on the per-host is_initialized fact, so when a freshly added node sorted first in the play its false value skipped cluster-config application for the entire cluster; the SASL service-account tasks had the inverse problem, attempting account creation against an existing cluster whenever a fresh node sorted first. A new cluster_is_initialized fact aggregates is_initialized across the play hosts (any member initialized = cluster initialized) and the run_once tasks now gate on it. The new test runs the real tasks with a fresh node deliberately first in the inventory and an initialized node second; cluster-config application is skipped entirely against the old gating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the installed version On RedHat hosts tasks/main.yml included configure-console.yml before install-console-rpm.yml, but the install task is what finalizes use_pre_v3_template from the actually-installed package version when redpanda_version == 'latest'. Configuring first could render a v3-shaped config for a 2.x console (or the reverse), leaving the service unable to start. The DEB path already installed before configuring. Reorder main.yml so configuration follows installation on both paths; handler/notify semantics are unchanged. Adds a structural test asserting both install includes precede the configure include, which fails against the old ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ot disk prepare-data-dir.yml only considers unpartitioned nvme* devices as data-dir candidates; when none match (no nvme devices, only partitioned nvme devices, or sdb-style device names), every mkfs/mount task is gated on the candidate list length and silently skips, the data path is then created on the root filesystem, and data-dir-perms.yml symlinks /var/lib/redpanda to it. The play reports green while Redpanda writes to the root volume -- silent data-placement corruption on any host whose device naming does not match the assumption. Add a hard-fail guard after candidate resolution: when no eligible device is found and redpanda_mount_dir (or its parent mount point) is not already backed by a mounted filesystem (per ansible_mounts), fail with an actionable message. New default allow_unmounted_data_dir=false is the documented escape hatch for knowingly running on the root disk. New tests inject ansible_devices/ansible_mounts fact variants and fail against the old code: all three no-eligible-device scenarios previously ran green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…setup The repository-configuration includes were gated on 'enable_airgap is false', a Jinja identity test that only matches the boolean False. Passing -e enable_airgap=false on the command line yields the STRING "false", so the gate never matched, repository configuration was silently skipped, and the subsequent console package install failed with no repo to pull from. Replace both gates with 'not (enable_airgap | bool)'. Adds isolation tags to the repo includes and their inner tasks plus a behavioral test that passes enable_airgap=false as a CLI string and asserts the repo include is reached on both the DEB and RPM paths; those cases fail against the old condition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… was back safe-restart returned as soon as systemd forked the new process, then immediately lifted maintenance mode and let the outer loop drain the next broker -- so a 3-node cluster could have two brokers effectively out at once (one replaying its log, one draining), losing quorum during a 'zero-downtime' restart. The restart is now followed by a wait for the admin API port and an 'rpk cluster health' gate (retries x delay configurable via restart_health_retries/restart_health_delay) that must report Healthy: true before maintenance mode is lifted; an unhealthy node fails the play with the health output visible (the previous blanket no_log on maintenance-mode tasks hid exactly this diagnostic and is removed here). The maintenance-mode and health commands also gain the conditional RPK_USER/RPK_PASS environment: they had none, so on SASL-enabled clusters they would fail with an auth error -- previously masked because restart detection never fired on SASL clusters at all. The new test drives the real safe-restart tasks with a mocked rpk: the health gate is absent entirely in the old version, and a never-healthy cluster must fail the play without lifting maintenance mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 'Add Redpanda DEB repository (source)' task in the console role wrote to the SAME dest as the binary-repo task and rendered the binary content variable, so it never configured deb-src at all - it just rewrote the binary repo line a second time. Because both writes carried identical content the overwrite was silent, but any future edit to either task would have had one write clobber the other. deb-src serves source packages this role has no use for, so the task is deleted along with the never-consumed rp_repo_signing_src_deb default and its _actual fact. Adds a structural test asserting no two tasks in configure-deb-repository.yml write the same dest and that no dangling rp_repo_signing_src_deb references remain; both assertions fail against the old code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two mount tasks and the mdadm array definition hardcoded /mnt/vectorized
while the data path task used {{ redpanda_mount_dir }}. Overriding
redpanda_mount_dir therefore mounted the volume at /mnt/vectorized but created
the data directory (and the /var/lib/redpanda symlink target) at the override
path on the root filesystem -- data off the volume even with a healthy disk.
Introduce redpanda_mount_point, defaulting to the parent directory of
redpanda_mount_dir, preserving the existing layout (/mnt/vectorized hosting
/mnt/vectorized/redpanda) while following any data-dir override. Documented in
defaults and the README, along with the previously undocumented
allow_unmounted_data_dir table row.
New check-mode tests assert the mount module's resolved target and the mdadm
mountpoint from runner events; they fail against the old code with the mount
landing at the hardcoded /mnt/vectorized despite redpanda_mount_dir being
overridden. The test playbook now loads the real role defaults so the
derivation under test is the production one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
start-console.yml used state: restarted with no condition and no become: every play bounced a healthy console even when nothing changed, and when something DID change the 'Reload systemd and restart console' handler fired too, restarting the service twice. The missing become also meant the task only worked when the whole play ran privileged. The start task now only ensures the service is running (state: started, become: true) and restarts are left to the notify handler. For the handler to cover config changes, the config-writing template task now notifies it - previously nothing did, which the unconditional restart masked. In the same task the config file mode drops from 0644 to 0600: the rendered config can carry SASL/login secrets via rpconsole overrides and the console user that owns it is the only reader. Adds structural tests for the start-task state/become, the config notify, and the config mode; all four assertions fail against the old code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Candidate selection for the data volume only required an nvme* name and zero partitions. An unpartitioned NVMe device directly hosting a mounted filesystem (cloud images commonly place / straight on the device) or claimed by another block layer (LVM/mdraid/dm-crypt holders) still qualified, so the role would run mkfs.xfs over a live root disk or array member. Candidates now must also have no holders and must not appear as the backing device of any ansible_mounts entry. New tests inject a root-hosting unpartitioned device and a holder-claimed device; both were selected as mkfs candidates by the old code and are now excluded (falling through to the no-eligible-device guard when nothing else qualifies). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
install-certs.yml references ca_cert_file, node_cert_file and
node_key_file, none of which have defaults. Running with
handle_cert_install enabled but any of them unset died mid-play on a raw
Jinja undefined-variable error ('ca_cert_file' is undefined) after the
console user, group and cert directory had already been created, leaving
the operator to reverse-engineer which variable was meant.
Add an entry assert whose fail_msg names all three required variables so
the failure happens before any changes and says exactly what to set. Adds
a behavioral test running install-certs.yml without cert vars that
requires the first failure to name every variable (fails against the old
code, which surfaces only the raw ca_cert_file error at 'Copy CA cert'),
plus a green-path run with all three provided.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The package-manager proxy tasks gated on '(create_pkg_mgr_proxy | default(false)) is true' and '(using_gcp | default(false)) is false'. Jinja's is-true/is-false tests check identity with the boolean singletons, so any string value -- which is what -e on the command line and INI inventories produce -- fails both tests. A user setting create_pkg_mgr_proxy=yes that way silently got no proxy configuration, and a string-valued using_gcp could never disable it either. Affects both the Debian and RPM dependency paths. Both conditions now coerce with | bool. New tests drive the isolated proxy task (new node_deps_proxy tag; the apt/dnf install tasks stay excluded) with create_pkg_mgr_proxy passed as the string "yes" and assert the proxy config is actually written; both fail against the old code with the task skipped. Skip behavior for a false flag and for GCP hosts is pinned as well. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
keystores_file is a control-node-relative path (it is the src of the copy task), but the stat deciding copy-vs-generate ran on the remote host, where the relative path resolves against the remote user's home. The staged keystore was therefore never found, and a self-signed keystore silently replaced the pre-built one on every run, restart-looping Connect. Delegate the stat to localhost so the check inspects the same path the copy reads. Adds an ansible-runner test (with a test-only connection plugin that reproduces ssh's remote-home cwd) proving a staged keystore is copied and generation is skipped; it fails against the old code because the remote-side stat misses the staged file. Also bakes the ansible test dependencies into the role's test image. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The RPM proxy task templated dnf_proxy.j2 over /etc/dnf/dnf.conf wholesale, replacing the entire file with a fixed set of options plus the proxy line. Any distro defaults or operator-managed settings (GPG policy, caching, download tuning, other proxies) were silently destroyed on every host where the proxy path runs. The task now manages only the [main] proxy key via community.general.ini_file (already a dependency of this collection); the dnf_proxy.j2 template is removed as unused. New tests pre-seed /etc/dnf/dnf.conf with operator settings, run the isolated proxy task, and assert those settings survive alongside the proxy key, plus that repeat runs do not duplicate the proxy line. The preservation test fails against the old code: the pre-seeded keys were wiped from the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
'Set Redpanda license (string)' passed the license as an rpk argument, so it was recorded in task invocations at default verbosity (event logs, CI output) and visible in ps on the target. The license now flows through a 0600 tempfile and rpk's --path flag, with the write no_log'd; the tempfile module also replaces a predictable path. The nightly-install tasks interpolate the Cloudsmith token into get_url/rpm_key URLs, which land in recorded module args -- they now carry no_log (gated on redpanda_broker_no_log so operators can opt into debugging output). Deliberately NOT added: blanket no_log on tasks that pass RPK_PASS via environment. Probing the harness showed task environments are inlined into the connection plugin's exec line, which no_log does not censor at -vvv (a documented Ansible limitation) -- so no_log there would buy no secrecy at the verbosity where the leak exists, while hiding the task output operators need. The sentinel test asserts the license never appears in default-verbosity output/events, and a structural test pins no_log on every token-interpolating nightly task. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The console role advertised an unstable channel (rp_key_deb_unstable was already in defaults) but the repo task files computed their *_actual facts from the stable variables unconditionally, so is_using_unstable: true was silently ignored and hosts always got the stable repos. Wire channel selection the way the broker role does: each *_actual fact is a ternary on is_using_unstable choosing stable vs unstable URLs, keyring paths and repo names, with the missing unstable defaults (RPM baseurls and metadata key, DEB keyring path and signing line) added alongside an explicit is_using_unstable: false default. Adds a behavioral test that tag-isolates the fact computation and asserts the facts point at the unstable channel when is_using_unstable is true and the stable channel when false; it fails against the old code, where the facts never switch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… password The self-signed keystore command rewrote the keystore on every run (no creates guard, always changed) and passed the keystore password as a pass:... command-line argument, exposing it in ansible logs, callback events, and ps on the host. Guard the command with creates so reruns are a no-op, hand the password to openssl via the environment (-password env:...), and mark the task no_log. Adds tests that fail against the old code: a double-run test recording openssl invocations through a PATH mock (old code invoked openssl twice and reported changed), and a sentinel-password scan over all runner events (old code leaked the password in the module invocation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Drop the deprecated, misspelled variable name from the system_setup role: the default() fallback on redpanda_mount_dir, the deprecation warning task in tasks/main.yml, and the README mention. BREAKING CHANGE: inventories still setting the misspelled name no longer have it honored; such hosts fall back to the default /mnt/vectorized/redpanda. Use redpanda_mount_dir instead. (The redpanda_broker role's copy of the shim is handled separately.) A structural test now asserts the misspelling appears nowhere in the role tree; it failed against the old code, flagging defaults/main.yml, tasks/main.yml and README.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
'Add Redpanda DEB repository (source)' wrote the binary repo line to the same dest as the binary-repo task -- a copy-paste artifact, so deb-src was never actually configured and the second write just re-asserted the first. The task is deleted along with the never-consumed rp_repo_signing_src_deb*/rp_conf_loc_deb* defaults. A structural test now fails any broker task file where two copy/template tasks write the same dest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The node's TLS private key was installed 0644, copied keystores and truststores 0644, and the generated keystore was forced to 0755 (by a task misnamed as a directory-existence check), so any local user could read the node's private key and password-protected stores. Key is now 0600, keystores and truststores 0640, and the misnamed task is renamed to what it does (enforce ownership and mode on the generated keystore). Adds a stat-based file-modes test that runs the real cert-install, truststore-copy and keystore tasks and fails against the old code on all four world-readable/world-writable modes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fallback The broker role declared redpanda_mount_dir (with the misspelled repdanda_mount_dir back-compat fallback) but no broker task or template ever read it -- the real consumer lives in system_setup, whose copy is authoritative. Keeping a second, independently-overridable declaration only enabled split-brain path configuration between the two roles. BREAKING CHANGE: repdanda_mount_dir is no longer honored anywhere; the system_setup role's redpanda_mount_dir is the single knob for the data volume path (its shim is removed in the system_setup changes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On current ansible-core, selectattr('state', 'eq', 'present') over an
item that has no state key yields an undefined marker that poisons the
whole loop expression, so user_management fails to render its loops
("can only concatenate list ... UndefinedMarker") as soon as any
user/role/acl entry omits or pins state. Filter to items where state is
defined before comparing it.
Also adds the user_management test scaffolding (Docker test container,
PATH-mock rpk with fixture-driven JSON output and a call recorder). The
new state-selection test fails against the old expressions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rpk security user list --format json emits a list of objects, but "Extract existing usernames" copied the list verbatim instead of mapping the username attribute. Membership checks against dicts never matched, so every existing user was re-created on every run and update_password never triggered the update path. Map the username attribute (keeping plain string lists working) as the roles path already does. The new idempotency test fails against the old code: alice was re-created and never updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The user/role/acl parse tasks piped possibly-empty stdout straight into from_json; | default([]) cannot catch the resulting parse error, so on a fresh cluster (rpk emitting nothing) the role hard-failed before creating anything. Guard each parse with a lazy inline conditional that treats empty stdout as an empty list. The new fresh-cluster test fails against the old code with "from_json failed: Expecting value". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Jinja filters bind tighter than +, so the assign-members loop applied subelements only to the state=present half of the concatenated role list. Roles without an explicit state (the common case) arrived as bare dicts and the task crashed resolving item.0.name, failing the play for any membership definition. Parenthesize the concatenation so subelements pairs every selected role. The new test fails against the old expression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The config excluded tests/** (which matched nothing -- role tests live at roles/*/tests/) and a Taskfile that does not exist. Role test trees and the collection-level tests directory are now excluded explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Exercises create, update, and delete user paths plus role membership and ACL loops with sentinel passwords, then asserts the sentinels never appear in any runner event or stdout at default verbosity. Guards the env-based credential delivery, no_log placement, and non-secret loop labels against regression. (The leak itself was already closed by the earlier remove-visible-auth change; before the loop-render fixes this scenario could not even complete.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ction docs The root README pointed version bumps at a monorepo-era path and never listed the collection's roles; docs/README.md was a stale copy of the deployment-automation repository's README, documenting playbooks (provision-node.yml, hosts.ini workflows) and variables (grafana_admin_pass, skip_node, aws_region) that do not exist in this collection. The root README now carries a role index with FQCNs, installation and minimal-play examples, development/test instructions, and a release procedure that matches reality (including changelog generation); docs/README.md points at the per-role READMEs and documents cross-cutting behavior (rolling restarts, configuration re-assertion, the private_ip contract). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ster existing_acls and role_members_result were fetched and never consulted: Create ACLs and Assign members ran unconditionally with changed_when rc==0, so every run mutated and reported changed even when the cluster already matched the desired state. Diff desired vs existing via normalized ACL comparison keys and the registered role-describe output (new role filter plugins), creating/assigning only what is missing. The new converged-state test fails against the old code: acl create and role assign were invoked and the play reported changed tasks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e role tree The broker README was a misplaced repo-level getting-started guide referencing Dockerfiles that do not exist here and documenting none of the role's variables. It now documents the role's behavior and carries a variable table generated from meta/argument_specs.yml by scripts/generate-role-docs.py, with a collection-level test that fails whenever a README drifts from its spec. Also fixes the mechanism that kept resurrecting the deleted test double: the broker test Makefile copied mocks into /app/library, which is the volume-mounted role directory, so every containerized test run recreated roles/redpanda_broker/library/ on the host -- on the role's live module path. Mocks now stage at /opt/mock-library inside the container, and roles/*/library/ is gitignored as a backstop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The role referenced two variables defined nowhere (root_ca_dir, used throughout CA creation and truststore generation, and truststore_file_name, defined only in the connect role), so both create_demo_certs and create_keystore failed immediately outside one specific playbook layout; cert issuance additionally hardcoded tls/ca paths, splitting CA creation from signing under any non-default root_ca_dir. Both variables now have defaults and the hardcoded paths honor root_ca_dir. Re-runs no longer wipe index.txt/serial.txt (which reset the serial counter and re-issued serials already in circulation, hidden by changed_when false) -- CA init is guarded by creates, as are the CA key/cert generation steps whose chdir-relative creates paths never matched and re-ran every play. CA and node private keys are chmod 0600 (openssl wrote them world-readable). The truststore is no longer deleted and rebuilt every run, and keytool receives the store password via -storepass:env instead of a recorded command line. The dead keystore task files (byte-identical copies of the non-keystore variants that never produced a keystore -- the connect role owns keystore creation), the unrendered connect.conf.tpl, their defaults, and the stray None entry in main.yml's when list are removed. The role now has a containerized suite running the full chain with real openssl/keytool: standalone execution, chain verification, key modes, CA-db persistence, idempotent re-runs, and a storepass leak check all fail against the old role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d become rpk_base_url existed in defaults but nothing read it -- the download URL hardcoded the GitHub latest release inline, so pointing at an internal mirror silently kept downloading from GitHub, there was no way to pin an rpk version, and no checksum option existed for the unauthenticated zip. The URL now derives from rpk_base_url + rpk_version (rpk_url still overrides outright, as the README documents), with rpk_checksum passed through to the download when set. install-cert.yml also gains the become its sibling task file already had -- creating /opt/rpk/certs failed for non-root connections while the rpk half succeeded. The role gains its containerized suite; every URL-derivation case and the become check fail against the old role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four defects made the bundler unusable or hazardous. DEB URLs embedded basearch (the role's own instructions say uname -m, i.e. x86_64) where Debian filenames use amd64/arm64 -- every deb download 404ed; a deb_arch mapping now derives the right name. noarch and SRPMS URLs appended the host arch to the filename, so those downloads always 404ed and killed the bundle run; per-class suffixes (.noarch/.src) fix them, and the apt-style '=' separator conditioned on the control host's os_family is gone from RPM filenames (which also drops the role's needless dependence on gathered facts). The package-split gate compared versions as strings, so 24.10+ bundled without rpk/tuner. Downloads and archiving now happen in a per-run temp directory under download_directory: previously the tarball glob swept any pre-existing /tmp/redpanda*.deb into the bundle AND deleted it from the control node. Optional deb_checksums/rpm_checksums verify downloads. The dead os_distribution/os_distribution_major_version required-input asserts (consumed by nothing) are removed. New container suite covers URL construction per class/arch/version and a file://-driven bundle run with a decoy file; all cases fail against the old role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The role had no tests at all despite managing settings that change host panic behavior. The new suite pins the exact managed sysctl keys, their defaults, persistence/reload flags, and privilege escalation, so any drift is a deliberate, visible change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI previously exercised 5 of 10 roles and never ran ansible-lint, changelog lint, or any packaging check despite configs and Makefile targets existing for years. All ten role suites now run, plus a quality step enforcing lint (production profile), changelog fragments, artifact packaging, and README/argument-spec sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The repo carried 113 ansible-lint violations against its own production profile config (which CI never ran). All are now fixed: FQCN for builtin actions, honest changed_when on every mutating command (with noqa'd justifications for the two rpm invocations that have no module equivalent: airgap force-reinstall and read-only version query), truthy normalization, naming, permissions on the data directory, indentation, and the stale platform/schema fixes in redpanda_logging's meta. The generated changelogs/config.yaml is excluded from lint as antsibull owns its formatting. galaxy.yml bumps to 0.13.0 and antsibull-changelog release generates CHANGELOG.rst + changelogs/changelog.yaml from the accumulated fragments, closing the last lint finding (no-changelog). All ten role suites and the packaging/docs checks pass after the sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… claim Three functional defects: the systemd logging override changed where the redpanda process logs but never triggered a redpanda restart (the only restart handler was notified by nothing and its failed_when: false would have masked real failures had it ever fired); the documented redpanda_logging_systemd_max_level had no effect (now wired as LogLevelMax in the unit drop-in) while redpanda_logging_systemd_forward_to_syslog promised behavior that cannot be set per-service at all (journald-wide option) and is removed; and the initial log file used file state=touch, reporting changed on every run (now copy with force: false -- create once, enforce attributes idempotently). New structural tests pin the handler wiring, the wired knob, the removed var, and the idempotent creation; all five fail against the old role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Nine of ten roles had no meta/main.yml at all -- no author, license, platform or min-ansible metadata on Galaxy. Every role now declares galaxy_info with current platforms and min_ansible_version '2.18' (matching meta/runtime.yml), with the connect role scoped to RHEL-family platforms per its install contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every role now validates its load-bearing inputs via meta/argument_specs.yml -- types, choices and required-ness enforced at role entry with named errors instead of mid-play Jinja tracebacks or silent misbehavior. Each role carries a containerized validation test proving a valid short-circuited invocation passes and bad inputs fail naming the variable; all bad-input cases pass silently without the specs. Role READMEs are rewritten around variable tables generated from the specs (six of them previously documented none of their variables, or variables that did not exist), kept in sync by the docs drift check. Also: user_management's admin credentials now follow the broker's sasl_superuser_* values when set (previously a divergent 'change-me' default silently failed authentication after a vault-set broker password), and the redpanda_logging role's systemd knobs, restart wiring, and log-file idempotency are fixed (LogLevelMax wired, the per-service-impossible forward-to-syslog variable removed, the never-notified restart handler replaced with real notifications, touch replaced with create-once semantics). All ten role suites, ansible-lint (production profile), packaging and docs checks pass; the 0.13.0 changelog covers the additions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
'Establish node id' grepped rpk's broker table for the node's private IP, but with advertise_public_ips (set by every provisioning playbook in the deployment-automation harness) the table shows the advertised public addresses -- so the lookup came back empty on every node. Under the old code that silently became node_id 0 cluster-wide, quietly corrupting maintenance-mode and restart targeting; after the recent hardening it fails loudly, which is exactly how the new SASL end-to-end lane caught it. The grep now matches the advertised_ip fact, falling back to the private IP when it is unset. The node-id test gains an advertised-address case (broker table showing public addresses); the live lane failure was the red for this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ACL-diffing filters lived in the role's filter_plugins directory, which Ansible loads for standalone roles but not for roles delivered inside a collection -- installing the collection and running the role failed with 'No filter named user_management_acl_keys' the moment ACL state was compared (caught live by the SASL end-to-end lane). The filters now live at the collection level (plugins/filter/), where collection roles resolve them unqualified in production; the role's test harness mounts the plugins directory and points ANSIBLE_FILTER_PLUGINS at it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unqualified filter names do not resolve from packaged collections on ansible-core 2.20 (verified empirically both inside role context on a live cluster and via ad-hoc lookup); only the fully-qualified name works. The tasks now use redpanda.cluster.<filter>, and the container harness exposes the mounted plugins under a synthetic ansible_collections tree so FQCN references resolve there too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every pytest-based test file ended with pytest.main(...), which returns the exit code without exiting -- so 'python file.py' (how the Makefiles and CI invoke tests) exited 0 even when tests failed. All 55 files now raise SystemExit(pytest.main(...)). Rerunning every suite with honest exit codes exposed two hidden breakages, both fixed: the cluster-wide initialization fact crashed on tag-filtered runs where the per-host fact was never set (it now tolerates undefined per-host state), and client_config's bad-input spec cases could never produce named validation errors because Ansible's str type coerces nearly anything (the cases are removed with an explanatory note -- that spec's value is documentation and defaults). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 task
CI installs the latest ansible-lint (26.8), which catches != ''/== '' comparisons in when-clauses that 25.11 missed; the eight flagged conditions now use length checks. No behavior change: for these variables an empty string and an absent value both mean 'not provided'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A floating ansible-lint broke the build when 26.8 added empty-string-compare patterns; quality gates should fail on code changes, not tool releases. Bump the pins deliberately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gene-redpanda
force-pushed
the
collection-hardening
branch
from
August 12, 2026 17:08
d420055 to
9784263
Compare
…ert too strict Both found by the first CI build to actually exercise the candidate collection (deployment-automation build 447). The jmx-exporter config merge piped the template lookup into from_json, but 2.18's native types return an already-parsed mapping where 2.20 returns a string -- the merge now guards on the type, same pattern the broker config merge uses. The console cert entry assert required node_key_file, which deploy playbooks legitimately omit (key installation is optional and its copy task already gates on the variable); the assert now requires only ca_cert_file and node_cert_file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ks were removed The keystore-variant task files deleted earlier were byte-identical to the standard csr/issue files but ran under the create_keystore gate -- which is the only gate set when the role runs against connect hosts, so they were the de-facto mechanism issuing connect node certificates before truststore generation (caught by the tiered-connect lane in deployment-automation build 447: keytool FileNotFound on node.crt). CSR generation and cert issuance now run under either flag; the truststore and CA gates are unchanged and the duplicate files stay deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Hardens the collection end to end: correctness and security fixes across all ten roles, input validation, packaging repair, test coverage for every role, release machinery, and rewritten docs. 65 commits, each fix developed test-first (failing test → fix → green). Prepared as 0.13.0.
Correctness fixes (selected)
rpk cluster healthbefore lifting maintenance mode, and its rpk calls authenticate on SASL clustersadvertise_public_ipsclusters (the broker table shows advertised addresses); maintenance-mode and restart targeting were quietly wrongallow_unmounted_data_direscape hatch), and the mountpoint honorsredpanda_mount_dir/tmpfilesplugins/filter/)Security
redpanda.yaml/.bootstrap.yaml(carry SASL credentials) now default 0640ps); Cloudsmith-token URLs nowno_log; user passwords no longer leak through loop labelsroles/redpanda_broker/library/(live module path in consumer plays) no longer ships, and the harness no longer regenerates itGuardrails, packaging, testing, docs
meta/argument_specs.yml+meta/main.ymlfor all ten roles — bad inputs fail at role entry with named errors; specs validated by per-role testsbuild_ignore(a dev-checkout build previously shipped ~40 MB of venv/IDE/test scaffolding), declaredcommunity.general/ansible.posixdependencies, corrected repository/issues URLs, required Galaxy tagrequires_ansible >= 2.18pytest.main()false-greened every suite)Validation
Beyond the containerized suites: validated end to end on live AWS via the companion deployment-automation SASL lane (redpanda-data/deployment-automation PR to follow) — SASL provisioning, user/ACL reconciliation with real enforcement checks, and a SASL-authenticated rolling restart with data survival, run from the built collection artifact. That run is what surfaced the advertised-address and filter-packaging bugs fixed here.
Breaking changes
repdanda_mount_dirfallback removed (redpanda_mount_dirin system_setup is the single knob)allow_unmounted_data_dir: truerestores old behavior)redpanda_config_file_modeto loosen)🤖 Generated with Claude Code