Conversation
…f ignoring them (#1847) Closes #1840. ## The defect `ps --services --format json` printed plain text and exited 0. A script doing `--format json | jq` received `w`, which is not JSON, and the exit code said everything went fine. `ps -q --services` did the same: `-q` asks for container IDs and got a service name. What made it inconsistent rather than merely undocumented is that the documented pair refused correctly. `-q --format` errored at parse time with exit 2 while the other two combinations were accepted and dropped, so from outside there was no way to tell which flags were honoured. ## The control The test enumerates the combinations rather than spot-checking one, because a test that only covered the documented pair would have passed while the bug stood. Measured on the built binary: | Combination | Before | After | |---|---|---| | `-q --format json` | exit 2 | exit 2 | | `--services --format json` | exit 0, plain text | **exit 2** | | `--services -q` | exit 0, service name | **exit 2** | | `--services` alone | exit 0, `w` | exit 0, `w` | | `--format json` alone | exit 0, JSON | exit 0, JSON | | `-q` alone | exit 0, ids | exit 0, ids | The last three are in the test so the fix could not be a blunt "reject anything with `--services`". `--format` carries `default_value_t`, so it was worth measuring rather than assuming that a default does not trigger the conflict: it does not. ## Sabotage My first attempt was inert and the reason is worth recording: **clap conflicts are symmetric.** Removing `services_only` from `quiet`'s list left `services_only`'s own list enforcing the same pair, so all 21 tests still passed and nothing had actually been sabotaged. Removing it from **both** sides restores the defect (`ps --services --format json` back to exit 0) and drops exactly one assertion, `ps_output_flags_conflict_in_every_pair_and_survive_alone`, with the other 20 green. Gates, all rc=0: `cargo test --locked --all-features --test cli_flags` (21 passed), `cargo test --locked --lib --all-features`, `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Closes #1837. ## The defect `ps` printed the container name; fourteen subcommands that take a target accept the **service** name and reject it. The table handed the reader the one identifier none of them takes, and never showed the one they want. ``` $ podup -f c.yml -p nmtest ps NAME IMAGE nmtest-web-1 docker.io/library/alpine:latest $ podup -f c.yml -p nmtest logs nmtest-web-1 podup: error: service 'nmtest-web-1' not found ``` ## Why this is a display change and nothing more The data was already there. `ps --format json` returned `"Service": "web"` in every row, and `podup images` already printed a `SERVICE` column. `ps` was the outlier among podup's own commands, not only against `docker compose ps`. ``` $ podup -f c.yml -p v37 ps NAME IMAGE SERVICE CREATED STATUS v37-web-1 docker.io/library/alpine:latest web Less than a second ago Up 0s ``` ## What is deliberately not in it No subcommand was widened to accept a container name. `docker compose logs <container>` does not work either, so accepting it would be a deliberate divergence rather than compatibility, and that decision has not been taken. That also narrows the invariant in #1846: "every identifier printed is accepted" is stronger than the reference implementation. What holds, and what this test asserts, is that the row carries **a** name the target-taking commands accept. ## Sabotage Making the `SERVICE` cell print the container name, which is the original defect, drops exactly `ps_service_column_value_round_trips_through_ps_and_logs` and leaves the other 11 green. ## Collateral, reported rather than silently edited `tests/engine_integration/cli_lifecycle.rs` sliced the row by column index (`cells[2..up]`) to reconstruct the CREATED cell. Adding a column between IMAGE and CREATED moves that boundary, so the slice and its comment change with it. `status_col` moved from 3 to 4 for the same reason. Gates, all rc=0: `cargo test --locked --all-features --test output_contract` (12 passed), `cargo test --locked --lib --all-features` (2028 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…gin (#1849) Closes #1841. ## The defect `secret_in_environment` inspected the value **after** interpolation, so the same file passed the gate when a variable was unset and failed when it was exported: ``` $ env -u SECRETO podup -f z.yml audit --strict -> no findings, exit 0 $ SECRETO=valor podup -f z.yml audit --strict -> finding, exit 1 ``` A gate whose verdict depends on the caller's shell is not a gate: the same commit passes on a laptop and fails in CI, and the compose files are identical. ## The decision Flag it always, whatever the origin. The risk is the same in both shapes: the value reaches the container's environment whether it was written literally or injected through `${VAR}`. ## Measured after the change Each row run twice, once with the variable exported and once with it unset, counting `secret_in_environment` lines: | Input | unset | exported | Stable | |---|---|---|---| | `DB_PASSWORD: literal-secreto` | 2 | 2 | yes | | `DB_PASSWORD: ${SEC}` | 2 | 2 | **yes**, was 0 / 2 | | `EMPTY_PASS: ""` | 0 | 0 | exemption kept | | `PASSTHROUGH: "true"` | 0 | 0 | exemption kept | | `DB_PASSWORD:` (bare) | 0 | 0 | exemption kept | The counts are 2 rather than 1 because of #1838, which doubles every diagnostic and is being fixed separately. The property this PR is about is the column on the right. ## The message It said `DB_PASSWORD carries a hard-coded value`, which was false for a file containing `${SEC}`. A reader who opened the file to find the hard-coded value would not find one, which costs the check its credibility on the cases where it is right. It now reads `DB_PASSWORD is set in compose; move it to secrets:`, true for both origins. `hard-coded` no longer appears anywhere in the output. ## Sabotage Restoring the exemption for an unresolved `${VAR}` drops exactly `audit_secret_in_environment_verdict_is_independent_of_var_export` and leaves the other 6 green. ## On #1833 `POSTGRES_PASSWORD_FILE: /run/secrets/...` has the `PASSWORD` segment and a non-empty value, so it is flagged before and after. That is a different exemption, it belongs to #1833, and this change neither fixes nor worsens it. Gates, all rc=0: `cargo test --locked --all-features --test audit_exit_codes` (7 passed), `cargo test --locked --lib --all-features` (2026 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Refs #1844. Fixes the compression half; the issue stays open for the memory half. ## The change `cp` built its tar inside a `GzEncoder` at `Compression::default()` and handed the result to a Unix socket on the same machine. The archive is plain tar now. ## Measured, 220 MB file, alternating order across rounds | | `podman cp` | `podup cp` | |---|---|---| | before | 234-248 ms | 4998-5173 ms | | after | 231-501 ms | **483-495 ms** | From roughly 21x to roughly parity. One round came in under podman. ## What did not change, and is reported rather than glossed Peak RSS is **473960 KB**, against 456052 KB before and podman's 48216 KB. Removing the encoder's output buffer did not move it, which is the expected result: the archive is still built whole in memory before anything is sent. That ceiling is deliberately out of scope here. Streaming is a much larger change to the same code path that #1808 is still unresolved in, and mixing the two is how a diagnosis gets lost. **#1844 stays open for the memory half**, and now carries the measurement. ## The coupling this surfaced `sent_entries` read the archive back through `flate2::read::GzDecoder`. A writer that stops compressing and a reader that keeps decompressing do not meet, so `verify.rs` and its tests move with `archive.rs`. That is the path #1777 and #1808 live in, so it was checked rather than assumed. ## Correctness - sha256 of the 220 MB file on the host and inside the container after the copy: identical. - The eight live `cp_flags` tests pass against a real Podman 5.7.0, with `PODUP_REQUIRE_PODMAN=1` so a skip would have failed instead of passing quietly. ## Sabotage My first attempt did not compile, so it proved nothing and I discarded it. A sabotage that re-gzips the finished tar, and does compile, drops five tests: `pack_path_archive_is_not_gzip_compressed` by name, plus four in `upload_tests` that fail because the reader no longer decompresses. That second group is the coupling above, caught by the suite rather than by review. The shape assertion reads the first two bytes for the gzip magic `0x1f 0x8b` rather than trusting the constructor, so it survives a refactor that changes how the encoder is built. Gates, all rc=0: `cargo test --locked --lib --all-features` (2027 passed), the live `cp_flags` (8 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…t carries it (#1853) Closes #1831. P0: this makes the 5.9.5 release run on `main` red and fails both checks on every pull request opened since. ## The defect The script asked for `workflows/${workflow}/runs?branch=${branch}`, filtered by `head_sha`, sorted by `created_at` and took the newest. It did not filter by event. A release pull request goes from `develop` into `main`, so its head branch is `develop` and its run appears under `?branch=develop` beside the push run for the same commit. Being newer, it wins. For `develop`'s head `20937cc`: | Run | Event | Conclusion | |---|---|---| | #2539 | `pull_request` | failure | | #2538 | `push` | **success** | The script read #2539. That run is #1830, the release pull request, and its `ci.yml` concluded failure **because this very job failed inside it**. The gate read its own run, failed, made that run red, and every later reader saw a red the gate created. #1809 was opened to close exactly this loop in its other form. ## Why it is not cosmetic On run 35538230895, the push run for `83315af` on `main` and the 5.9.5 release commit, `branch health (develop)` is the **only** failing job. `main`'s history records the release as red, about the state of a branch that was green, as reported by a run that branch did not trigger. ## The fix Both the runs call and the jq filter select `event=push`. A branch's health is the verdict of the run that branch triggered; a `pull_request` run that merely carries the branch as its head is a different question. Nothing else moved: the polling bound, the cancelled-is-red rule and the `HEAD_SHA` gating from #1827 are unchanged. Three defects in a row have come out of this file and widening the change is how a fourth arrives. ## Sabotage Removing the filter from **both** places, because removing one leaves the other enforcing it, drops four assertions: ``` FAIL newer pull_request failure cannot shadow older push success for head (gate must not read a run the branch did not trigger): exit 0 FAIL verdict line names run #60 (the push run, not the pull_request run #62) FAIL verdict line does NOT name run #62 (the pull_request run) FAIL pull_request run URL #62 must NOT appear ``` The suite also carries the assertion that stops the lazy fix: a **newer push failure** over an older push success still fails, so this cannot become "prefer whatever succeeded". Gates, all rc=0: `bash tests/shell/branch-health-conclusion.test.sh` (59 passed), `cargo test --locked --test 'workflow_*'` (33 passed), `cargo fmt --all --check`, `shellcheck` on the script and its test, `actionlint` on the reusable. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…rop it (#1851) Closes #1839. ## The decision, taken by the owner 1. `--no-warn` silences the port-publishing warning. Publishing on `0.0.0.0` is host binding, which the flag's documentation already promised to cover. 2. Read-only commands stop emitting it. `config` keeps it and stays exempt from the flag. ## Measured on the built binary | Command | without the flag | with `--no-warn` | |---|---|---| | `up -d` | 1 | **0** | | `ps` | **0** | 0 | | `logs w` | **0** | 0 | | `port w 80` | **0** | 0 | | `top w` | **0** | 0 | | `config` | 1 | 1 | Before this change every row except `config` read 1 / 1. ## What is deliberately unchanged `privileged has reduced effect under rootless Podman` is an informational note about what rootless does, not a host-binding warning, so it stays outside the flag: two warnings without it, one with it, exactly as before. Widening `--no-warn` to cover it is a separate decision nobody has taken. ## Why read-only commands and not just the flag `podup port w 80` on a service publishing 18080 and 18081 warned about **18080**, which is not the port that was asked about. A command that starts nothing has no bind to confirm, and there was no way to silence it there because the flag's scope never reached those commands. ## Sabotage | Sabotage | Assertions that fell | |---|---| | `--no-warn` stops reaching `up` | `up_no_warn_silences_the_port_exposure_warning` | | read-only commands warn again | the three `logs`/`port`/`top` assertions | `tests/host_binding_warnings.rs` already mentioned `--no-warn` thirty times but only covered `config` and `generate quadlet`, which is why nothing caught this. It now covers `up` and the read-only commands too. Gates, all rc=0: `cargo test --locked --all-features --test host_binding_warnings` (17 passed), `cargo test --locked --lib --all-features` (2029 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…pass (#1852) Closes #1838. ## The defect A single unset variable referenced once produced two warnings, and four under `audit`. The multiplier was not per-variable or per-occurrence: the compose document is interpolated more than once per command, once for the diagnostics pass that detects keys the typed model drops and again for the real render, and every pass warned. ## Measured, counting `variable is not set` lines | Input | `config` | `audit` | |---|---|---| | one variable once | 2 -> **1** | 4 -> **1** | | two different variables | 4 -> **2** | 8 -> **2** | | same variable three times | 6 -> **1** | -> **1** | ## The second question, answered deliberately The issue left open whether three references to the same unset variable should warn three times or once. It warns **once**: it is one piece of missing information, not three, and that matches docker compose reporting once per variable per file. The reasoning is in the code rather than only here. ## The assertions are counts A `contains` assertion passed at two warnings and would have passed at ten. That is exactly why nothing caught this, so the tests assert exact numbers. ## Sabotage, and a measurement error worth recording My first sabotage removed the guard in `nested_raw.rs` and **all 19 tests still passed**. There are two guards, one there and one in `compose/mod.rs`, so removing one leaves the other silencing the pass: nothing had actually been sabotaged. Same shape as the clap-conflict symmetry in #1847. Removing **both** drops exactly four assertions, each named for the property: - `unset_variable_once_warns_once_on_config_and_audit` - `bare_unset_variable_once_warns_once_on_config_and_audit` - `two_unset_variables_warn_twice_on_config_and_audit` - `same_unset_variable_three_times_warns_once_on_config_and_audit` I also nearly sent this back as incomplete. My first reading measured `audit` at 3 rather than 1, because `grep "is not set"` also matches two audit findings whose own text contains that phrase (`pids_limit is not set`, `userns_mode is not set`). The fix was complete; my pattern was too loose. Gates, all rc=0: `cargo test --locked --all-features --test cli_diagnostics` (19 passed), `cargo test --locked --lib --all-features` (2026 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`. --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…or (#1854) Closes #1842. ## The defect Deleting a watched file printed ``` podup: warning: watch action failed: build error: No such file or directory (os error 2) when getting metadata for /tmp/d5/./src/f.txt ``` and left the file inside the container. The rule's action is `sync`, so `build error` is the wrong category; nothing failed, a file was deleted; and `docs/commands.md` says *"Read the warnings, not the status"*, which makes that line the only signal the design leaves open. ## Measured by hand on the built binary | Check | Result | |---|---| | file deleted on the host | **gone** from the container | | target directory `/app` | **survives** | | lines containing `build error` | **0** | | path with a literal `./` | **0** (`/tmp/v42/src/f.txt`) | | new message | `removed /app/f.txt from v42-a-1` | The scope is bounded and the doc table now says so: only entries inside the rule's target, never the target directory itself. ## The test was blind, and that is the part worth reading The first version of this change **passed with the feature disabled**. I turned `if is_remove_event(...)` into `if false && is_remove_event(...)`, rebuilt, and with the real binary the file stayed inside the container with zero `removed` lines. The behaviour was gone. The live test still passed, three runs in a row. The reason was in the test's own comment: ```rust // Second: delete the file on the host and run the removal flow that the // live dispatch runs on a Remove event. let remove_outcome = engine.test_remove_from_container(...).await; ``` It said it ran the flow the live dispatch runs, and it called the leaf directly. `is_remove_event`, the event plumbing and the path mapping were all skipped, so the test passed whatever that path did. The same shape as the gap #1825 was opened for, where `userns_tests.rs` called `create_and_start` and never created a pod. The test now starts the watcher the way a user does. The identical sabotage fails it: ``` thread 'watch_delete::watch_sync_propagates_host_deletions_to_the_container' panicked: the host-side deletion did not propagate; /app/f.txt is still present in the container ``` Gates, all rc=0: the live test three times in a row (11.11s, 10.71s, 10.80s) with `PODUP_REQUIRE_PODMAN=1`, `cargo test --locked --lib --all-features` (2032 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…t of the environment (#1855) Closes #1833. ## The defect, as reported `podup audit` reported `secret_in_environment` for `POSTGRES_PASSWORD_FILE: /run/secrets/<name>`, which is the pattern that **removes** the secret from the environment. The check flagged the correct configuration, so the only way to get a clean audit was to stop using the convention the official Postgres, MariaDB and MySQL images document. The reporter runs `audit --strict` as a deployment gate and had to carve the finding class out by hand. ## The rule, and why this one The exemption is **the `_FILE` suffix on the key**, not the `/run/secrets/` prefix on the value. The value is a path to a file the application reads at runtime. A path is not a secret, so the key carrying it is not a secret in the environment. That holds wherever the path points, which is why the check trusts the convention rather than verifying the destination: a `_FILE` key pointing at a custom mount, or anywhere else, is still a path. Verifying what the path points at, its permissions or its provenance, is a different audit concern. Matching on the last segment rather than a literal `_FILE` keeps it robust across the spellings in use: `POSTGRES_PASSWORD_FILE`, `MY_KEY_FILE`, `KEY-FILE`, `KeyFile`. ## Measured, each row run twice | Input | unset | exported | |---|---|---| | `POSTGRES_PASSWORD_FILE: /run/secrets/x` | not flagged | not flagged | | `PASSWORD_FILE: /etc/passwd` | not flagged | not flagged | | `DB_PASSWORD: literal` | flagged | flagged | | `DB_PASSWORD: ${SEC}` | flagged | flagged | The last row is #1841, which landed earlier today: the verdict must not depend on whether the variable is exported. It does not. ## Sabotage Disabling the exemption drops exactly the two assertions named for it: `..._does_not_flag_file_suffix_pointing_at_secrets_mount` and `..._does_not_flag_file_suffix_pointing_outside_secrets_mount`. The second one exists because the decision to trust the convention rather than the path is the debatable half, so it is pinned rather than implied. Gates, all rc=0: `cargo test --locked --all-features --test audit_exit_codes` (9 passed), `cargo test --locked --lib --all-features` (2032 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…wered (#1857) Refs #1808. This does not fix that issue; it makes the next attempt start from a measurement. ## Why The error a caller saw was ``` the upload to /tmp could not be confirmed: the container runtime closed the connection without a response and the destination did not change ``` which names nothing anyone can check. The entry that actually failed was reported through `tracing::debug!`, and the integration suite's env filter does not emit debug, so the Podman 6 job log carried no trace of it. I grepped run 35526977642 for the entry name, the stat and the warning text and found nothing. That cost two rounds on #1808. Both attempts at the Podman 6 failure were made without knowing which of the archive's five entries was refused (`a.txt`, an empty file, a symlink, a dangling symlink, the directory) or what the runtime had answered about it. It was never even established that the link was the failing entry. ## What changed The error names the path, what was expected of it, and what the stat said: ``` payload/link in /tmp is not what was uploaded; expected Link("a.txt"), runtime answered PathStat { ... } ``` Better for a user too: the old message told an operator that something they cannot see did not match something they cannot name. ## What did NOT change, and how that is evidenced No confirmation rule moved. `entry_landed`, `LinkCheck`, and the mode and size comparisons are untouched: the diff contains no line touching `LinkCheck::`, `is_symlink`, `is_regular`, `is_dir`, `stat.size` or `link_target`. The existing confirmation tests pass unchanged, which is the evidence rather than the claim. ## Sabotage Reverting the message to a generic one drops four assertions, each named for what it protects: - `the_refusal_names_the_entry_path` - `the_refusal_carries_the_stat_that_was_read` - `the_refusal_for_a_404_without_stat_explains_the_missing_stat` - `a_refused_entry_message_names_the_path_and_the_stat` Gates, all rc=0: `cargo test --locked --lib --all-features` (2038 passed), the live `cp_flags` with `PODUP_REQUIRE_PODMAN=1` (8 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
#1856) Closes #1832. ## The decision Refuse, unless the compose file declares `external: true`. The project name is the isolation boundary; a project that wants to share a network says so. ## Measured on the built binary Two projects, `p1` and `p2`, both declaring `name: red-compartida`: | | Result | |---|---| | `p2 up` without `external` | **rc=1**, `network 'red-compartida' already exists and is labelled podup...` | | `p2 up` with `external: true` | rc=0, attaches | | `p2 down` afterwards | `p1`'s network **survives** | The teardown half is the one that loses data. With the owner's containers stopped libpod would honour the DELETE, so the guard fires before the request lands rather than relying on the removal failing. ## The part that goes beyond the issue, and deserves your eye **An unlabelled network is refused too.** A network created by hand, or by another tool, now needs `external: true`: ``` $ podman network create red-ajena $ podup -f p3.yml up -d podup: error: unsupported feature: network 'red-ajena' already exists and carries no podup label ``` The reasoning in the code is that the label is the only ownership evidence there is, and "no one owns it" is not a safe state to delete from. That is defensible and it is consistent, but the reported case was cross-project adoption, not hand-made networks. Anyone with `name: some-existing-network` and no `external: true` breaks on upgrade, and that is worth a deliberate yes rather than arriving as a side effect. `external: true` does work for that case, measured: rc=0. ## Sabotage Disabling the ownership comparison in the teardown path drops exactly `down_refuses_to_remove_a_foreign_labelled_network`, with the other three green. My first attempt did not compile, so it proved nothing and I discarded it rather than reading its silence as a passing control. Gates, all rc=0: the live `network_ownership` suite three times in a row (4 passed each, 21.04s / 21.40s / 20.83s) with `PODUP_REQUIRE_PODMAN=1`, `cargo test --locked --lib --all-features` (2038 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features`. --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Refs #1846. Writes four of the five; the fifth belongs to the harness and stays open. ## Why these rather than eight tests Eight defects were found in one afternoon by running the published binary against a real Podman, all of them shipping and none known. Writing eight assertions fixes eight bugs. The four below are stated as properties over the surface, so each covers a family. | Invariant | What it asserts | |---|---| | round trip | every row a listing prints carries at least one identifier the target-taking commands accept, read from the rows rather than hardcoded | | count, not match | diagnostics assert an exact number of lines | | ambient independence | the same input gives the same verdict whatever the caller's environment | | accept or refuse | every pair of output-shaping flags parses or errors; none is accepted with a flag dropped | ## They hold today, so the question is whether they bite All four pass on `develop`, because the fixes that landed this afternoon closed them. A test that passes on a fixed tree proves nothing by itself, so I reintroduced two of today's defects: **#1840, flags silently dropped.** Two assertions fail, and the second is the point of this change: ``` ps_output_flags_conflict_in_every_pair_and_survive_alone <- the specific one from #1847 every_command_with_output_flags_rejects_or_parses_every_pair <- the family one ``` **#1841, audit's verdict depending on the environment.** `audit_secret_in_environment_verdict_is_independent_of_var_export` fails. ## A correction carried into the code The round-trip invariant as first written on the issue said "every identifier a command prints is accepted". That is stronger than the reference implementation: `docker compose ps` prints a container name and `docker compose logs <container>` rejects it, so demanding it would require a divergence rather than a fix. The wording here is the one that holds and still catches a future column that prints something nothing accepts. Gates, all rc=0: the four suites (8 + 20 + 22 + 13 = 63 passed), `cargo test --locked --lib --all-features` (2032 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…drift (#1859) Closes #1836. ## What it does ``` $ podup audit --list-checks privileged privileged: true grants extended host privileges. host_namespace a host-binding namespace mode shares the host's or another container's namespace. dangerous_capability cap_add carries a capability from the dangerous list... ... ``` Eleven checks, text or `--format json`, exit 0, no compose file needed. Measured in an empty directory. ## The identifiers are the contract Drift detection is worthless if the listing invents a second vocabulary, so the ids are the strings findings actually carry. Measured: an audit of a dirty compose emitted 9 ids, and all 9 are in the listing. The test asserts **both directions**, which is more than the issue asked for and is the part worth keeping: - every id the audit emits is named by the registry, so a check function whose hardcoded id drifts is caught; - every registry entry fires on the dirty fixture, so an entry whose check stopped emitting its id is caught too. ## It does not repeat #1840 `--list-checks --strict` conflicts at parse time with exit 2 rather than accepting the flag and ignoring it. ## Sabotage, after two of mine that proved nothing Renaming the emitted `unpinned_image` id fails the integration suite. Recording the two attempts that did not, because the lesson is mine rather than the code's: - the first deleted the registry line and **did not compile**, so its silence meant nothing; - the second renamed one of two emitter branches, and my fixture exercised the other, so the drift I thought I had planted was never reachable. Gates, all rc=0: `cargo test --locked --all-features --test audit_list_checks --test audit_exit_codes` (7 + 4 passed), `cargo test --locked --lib --all-features` (2032 passed), `cargo fmt --all --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
`podup cp` opened a progress row and left it on a static `Copying` until the copy landed. A multi-gigabyte directory looked the same as a hung socket for the whole transfer. Both directions now feed a shared byte counter as data flows: the container→host reader counts what it drains out of the archive stream, the host→container PUT counts body chunks as they go out. A task reads the counter every 100 ms and rewrites the verb through the same `progress::start` path `up`, `pull` and `build` use, so a tty repaints in place while a redirected stderr collapses to the one line the plain sink prints at `end`. The closing verb carries the final total. **Sabotage, measured.** Three rounds, because the first two passed on shape alone: | Change | Tests that fail | |---|---| | emitter verb drops the byte figure | 2 | | `ByteCounter::add` stops accumulating | 5 | | the stream reader adds `0` instead of `n` | 1 — and **0 before this PR's last assertion existed** | The third one is the point. Every verb stays well-formed when the producer stops counting: `Copied 0B` starts with `Copied `, is longer than `Copied`, and passes every shape assertion. The piped-cp test now reads the closing figure as a quantity against the 256 KiB fixture, so the producer side is pinned and not just the frame. Closes #1845 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…#1863) `entry_landed` confirmed an uploaded symlink as soon as the destination was a symlink, whatever it pointed at. A copy cut mid-stream over a tree with a same-named link pointing elsewhere was reported as landed. **What Podman actually sends**, measured on the socket with `curl -I` before writing the fix (Podman 5.7.0): | link | points at | `size` | `linkTarget` | |---|---|---|---| | `/tmp/link-a` | `/etc/hostname` | 13 | `/etc/hostname` | | `/tmp/d/rel` | `../etc/hosts` | 12 | `/tmp/etc/hosts` | | `/tmp/d/dangling` | `nothing-here` | 12 | `/tmp/d/nothing-here` | `linkTarget` is normalised: a relative target is joined to the link's directory and `..` is resolved lexically. A literal comparison against the text in the tar would refuse **every relative link** and fail every directory copy containing one, so the sent side is normalised the same way first. `size` is the length of the link *text*, so it is compared as well, with no normalisation needed. A runtime that sends no `linkTarget` falls back to the type check as `LinkCheck::Fallback`, distinct from `Confirmed` in the log. The normaliser splits on `/` by hand rather than via `Path::components`, which splits on `\` on Windows: a container path is POSIX on every host, and the platform path type would have given the Linux and Windows lanes different verdicts. A test pins `a\b` as one component; it is the Windows lane that exercises it. **Regular files:** size plus mode-type, unchanged. No checksum in the stat, and Podman 6 reports mtime to whole seconds (already recorded on `PathStat`). **Sabotage:** target comparison forced true, 3 tests fail. Normalisation returning its input, 6 fail, three of them through the upload path. Closes #1808 --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…ct (#1864) Nine comments across the tree explained code by pointing at a document that is not in the repository: "the brief calls this out", "the cases the brief names", "when the brief was written". Someone opening the file learns nothing from them. A review checklist that searched for the word missed all nine, so this moves the rule from the checklist into CI. - Every occurrence is rewritten to say what the code does or what the test pins. Seven files, comments only. - `.github/scripts/check-planning-words.sh` fails on the whole word, case-insensitive, in any tracked file except `CHANGELOG.md`, and annotates file and line. - It runs as a step in **`Format & lint`**, which is already a required check. A new job would report and block nothing until a ruleset named it. - `tests/shell/check-planning-words.test.sh` runs the real script in a throwaway repo, reading the exit status directly: clean tree passes; planted comment fails with file and line; case ignored; `briefly` passes; changelog exempt; untracked file not read. Wired into `lint-shell.yml`'s test command and both path filters. **Sabotage:** dropping `-w` fails the `briefly` case; dropping the `exit 1` fails the two cases that must be red. Open PRs are unaffected: none of them touches the lines rewritten here, so their merge refs come out clean. --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
`podup audit` had no opinion on port exposure, while `up` and `config` have warned about it since the port-exposure diagnostic landed. An operator running the audit as a deployment gate got a pass on a compose file that publishes Postgres on every interface. The check reads the same predicate the diagnostic does rather than restating it: `ports_published_on_all_interfaces` is now the single source of truth in `internal/compose/diagnostics/ignored_fields.rs`, and both surfaces format their own message from the `PortExposure` it returns. A future compose shape that counts as published reaches the audit and the warning together. Thresholds are the diagnostic's, unchanged. An explicit host IP is not flagged, including `0.0.0.0` and a private LAN address; a container-only short form is not a publish; a range emits one finding with the range string as its label. One behaviour changes in the diagnostic: `":5432"` used to emit `port is published on every interface`, with the label missing. It is now skipped, and a test pins both halves. **Sabotage, measured:** emptying the shared predicate stops the binary emitting the finding (`podup audit` on a `5432:5432` fixture goes from one `port_published_on_all_interfaces` line to zero) and fails six of the sixteen new audit tests plus four of the diagnostic's. Removing the empty-host guard fails the test that pins it. Note on running them: the audit tests live in the binary target, so `cargo test --lib` does not include them. `cargo test` with no target flag does. Closes #1835 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
`podup audit` had no opinion on bind mounts. A compose file that mounts the container runtime socket into a service — which hands it the ability to start sibling containers as the daemon's user — passed `--strict` silently. `sensitive_bind_mount` fires on: - **the runtime socket**, matched exactly, with stronger wording (the file is the attack); - **a short list of host directories**, matched as a prefix so a subpath is the same finding: `/proc`, `/sys`, `/dev`, `/etc`, `/boot`, `/root`, and the runtime directories that hold a socket. `:ro` still fires. Relative paths and the project directory do not. **The rootless socket** is matched by shape: `/run/user/<uid>/podman/podman.sock` is the one a rootless operator has and the one podup connects to by default, and the uid varies. The first version missed it while its comment called `/run/podman/podman.sock` the rootless socket — that is the system socket, and the comment now says so. The uid must be all digits; nothing else under `/run/user/` is flagged (session bus, audio, display sockets are mounted on purpose by desktop containers). | Mount | Findings | |---|---| | `/run/podman/podman.sock` | 1 | | `/var/run/docker.sock` | 1 | | `/run/user/1000/podman/podman.sock` | 1 | | `/run/user/1000/pulse/native` | 0 | Measured on the built binary. Built on top of #1859's registry: the check lives in its own `check_sensitive_bind.rs` and has a `CHECK_REGISTRY` entry. Both dirty fixtures now plant a rootless socket, because #1859's test requires every registry entry to fire. It did its job here: without the fixture line the test failed and named the new id. **Sabotage:** rootless matcher returning nothing fails both rootless tests and the registry test; dropping the all-digits uid rule fails the test that pins what is left alone. Closes #1834 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
`podup up` created the project network before it validated the services, so an `up` rejected for `pid: "evil"` or `ipc: "bogus"` left `<project>_default` on the host. Reproduced on 5.9.5 in #1867. `run_up` now runs `pre_validate_spec` for every enabled service before creating any network, volume, secret, pod or container. Same function `create_and_start` already called per service, not a second rule set. To run it that early, the validator can no longer take the device access strings the spec builder computes, since building them stats host nodes. It reads them through the engine's own parsers: `parse_device_cgroup_rule` as it is (no filesystem access), and `device_spec_parts`, the `host:container:access` split moved out of `parse_device` so both call one function. A rule the engine drops as malformed (`c 1:3 rwx extra`) is not validated either. **One thing worth checking in review:** the two live tests in `error_surfacing.rs` list networks **before** the `down` that cleans up. Running `down` first, which is what the first version of these assertions did, removes the network being asserted on and passes whether or not the defect is fixed. **Sabotage** (`--no-fail-fast`): | Change | Fails | |---|---| | remove the up-front loop | 3 fake-socket tests + both live tests (Podman 5.7.0) | | validate only the first service | the two-service test | | `device_spec_parts` drops the access | the `devices:` rejection test + 2 `parse_device` tests | This also unblocks #1860: the integration leak scan found this defect as two of its seven leaks. Closes #1867 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…#1870) pin-watch opened #1869: the `debian:trixie` image both .deb builds pin by digest had moved upstream. Dependabot cannot bump it, since the `github-actions` ecosystem does not touch a workflow's `container:` image. - Both pins move together, `release.yml`'s container and the `reusable-rust-debian.yml` default: `tests/workflow_debian_image.rs` requires them equal. - The value is the OCI **image index** digest, read from the registry rather than copied from the issue, so the pin stays multi-arch: ``` content-type: application/vnd.oci.image.index.v1+json docker-content-digest: sha256:9cc080028c43b27d2074d63a5f9caf7166d731494965616c1a6d2827a004585c ``` - Two comments beside the pins described a disagreement between them that no longer exists; they now say what enforces the agreement. `tests/workflow_debian_image.rs`: 3/3. Closes #1869 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
The integration suite left resources on the host on every run and nothing noticed: 125 containers, 189 networks and several hundred images had piled up on one machine, and the containers alone held 58 of the 128 inotify instances a user may open (#1843). **Why a separate binary.** The first version of this PR scanned from inside `engine_integration` and failed on `podman-vm (5)`: cargo runs one binary's tests in parallel, so a `z_`-named scanner reported two resources of a test that finished after it. The scan now lives in `integration_leak_scan`, run as a second `cargo test` process once the suite has exited, whether it passed or not. `engine_integration` hands over its PID through `target/podup-leak-scan-pid`. **The lane fails** when the scan reports a leak, when its marker is missing, and when the PID file was never written (`rc=nopid`), since the scan exits cleanly without a PID and would otherwise pass having scanned nothing. I drove the verify snippet with all four marker shapes before pushing. **Guarding the scanner:** one test plants a network under its own prefix and requires the scanner to name it; another drives `podman` into a non-zero exit and requires that to fail the scan rather than read as an empty list. Emptying the scanner or making it swallow the exit each fails one of them. **What it found, and what is fixed here:** seven leaks on its first local run. Two were a podup defect, fixed in #1868. Five were tests that never tore down; they now bind a `DownGuard` that runs `down -v` on drop and prints the error if `down` fails. Two build tests hold a `TestImage` guard. **Measured:** the whole live suite, 218/218 on Podman 5.7.0, then the scan with that run's PID: nothing. A manual listing of containers, networks, volumes, pods and images with the run's prefix was empty. Closes #1843 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
The Podman 5 VM already moved container logs to `k8s-file` because Fedora 44's journald binding cannot open its library (every logs request came back `500 unable to open a handle to the library`). The events backend reads the same journal through the same binding and was left on journald. `niche::engine_events_stream_connects` failed on that leg three runs in a row on #1871, with this, once #1871 made the test print what it got: ``` Err(Podman(Api { status: 500, message: "unable to open a handle to the library" })) ``` The logs error word for word. This sets `events_logger = "file"` next to the existing `log_driver = "k8s-file"`, for both legs like the log driver. **What this does not explain:** the same test passed on the Podman 5 leg of other pull requests between those three runs (#1860, #1870). I don't know why it is consistent on #1871 only. This removes the journal dependency the error names; it does not claim to explain the pattern. The lane runs on this PR because it edits its own workflow file. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…ce (#1871) podup labelled containers and networks with `podup.project` but not the images it built, so `podman image prune --filter label=podup.project=<p>` matched nothing. In production that is about 1.3 GB per deploy of a multi-stage build, reclaimable only by an unfiltered prune that also takes other projects' images (#1866). Every build now sends `podup.project` and `podup.service` in `labels` (final image) **and** as a repeated `layerLabel=<key>=<value>` (every intermediate stage). Measured on the Podman 5.7.0 socket, three-stage build: | query | new images | labelled | |---|---|---| | `labels` only | 4 | 1 | | + `layerLabel` | 4 | **4** | podup's keys go in after the user's, as the container path already does, so a user `podup.project` cannot put this project's images in reach of another project's prune. **Visible side effect:** with labels always present, buildah adds a LABEL step, so a two-instruction Containerfile reports `STEP 1/3`. `tests/build_contract.rs` pins it. **Sabotage** (`--no-fail-fast`): | Change | Fails | |---|---| | drop `layerLabel` | 3 query tests + the live test | | `layerLabel` without `podup.project` | the live test, naming the unlabelled image | | podup's labels before the user's | the override test | The live test diffs every image id before and after the build, instead of selecting by the label it is checking. Its drop guard lists ids by label and then removes them (`podman rmi` has no `--filter`). Closes #1866 --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…#1873) `podup cp` host to container built the whole tar into a `Vec<u8>` before sending it: 437 MB of RSS for a 220 MB copy, and a copy larger than free memory failed (#1844). The packer now runs on a blocking thread and writes into a bounded channel whose receiver is the PUT body. Measured with the release binary against Podman 5.7.0, sha256 checked inside the container after every copy: | source | before | after | |---|---|---| | 220 MB | 0.48–0.55 s, **437 MB** RSS | 0.13–0.16 s, **10.6 MB** RSS | | 1 GB | — | 0.58–0.59 s, **10.2–10.6 MB** RSS | RSS does not grow with the file. **Confirmation without re-reading the archive.** The landed-check used to walk the tar it had just sent; a streamed body is gone after the PUT. The packer now records each entry as it appends it. There is **one walk per path**: `pack_path` (cp) and `build_sync_tar` (watch sync) each take a writer and a recorder, and production and tests call the same function, with a channel writer or a `Vec<u8>`. A test requires the recorded list to equal what `sent_entries` reads back from the bytes written. **`cp -L`** follows the link through that recorder and records the target's kind, so the confirmation asks for what was sent. An earlier version of this branch lost `-L`; only the live test caught it, and there are now packer tests for link and tree, with and without `-L`. **Sabotage** (`--no-fail-fast`): symlinks skipped in the archive but recorded, 4 fail; dropped from the recorder, 5; mid-pack error swallowed, 2; `follow_link` ignored, 2. Live: `cp` group 12/12, `watch` group 10/10. **Not a regression, recorded separately:** the live `watch` group is flaky under heavy parallelism on `develop` too (7/10 at 16 threads on both trees, 10/10 at 4 on both). I am opening an issue for it rather than changing the watch loop here. Closes #1844 Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
Bumps the three version stamps together, as `release.yml` requires: `Cargo.toml`, `Cargo.lock` and a new `debian/changelog` entry covering the 24 commits since 5.9.5. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.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.
Release 5.9.6: the 24 commits on
developsince 5.9.5. The user-visible changes are in thedebian/changelogentry added in #1875.Headline items:
cphost to container streams the archive: a 220 MB copy went from 437 MB to about 10 MB of resident memory, and a copy larger than free memory no longer fails (perf(cp): stream the upload instead of building the archive in memory #1873).podup.projectandpodup.service, intermediate stages included, so one project's build output can be pruned by label (fix(build): label every image podup builds with its project and service #1871).uprejects an invalid service before creating anything (fix(up): reject an invalid service before creating anything #1868); a project no longer adopts another project's network (fix(network): refuse to adopt or remove a network another project owns #1856).audit:--list-checks, and newport_published_on_all_interfacesandsensitive_bind_mountchecks (feat(audit): add --list-checks so an integrator can detect check-set drift #1859, feat(audit): flag a port published on every host interface #1861, feat(audit): flag a bind mount of a sensitive host path #1865).It also carries the branch-health fix (#1853) to
main, whose push run has been red since 5.9.5 for that reason alone.