LEV-1803: Move the Linux Tentacle image from Debian 11 to Ubuntu 22.04 +semver: major - #1292
Conversation
|
|
58edddc to
be3248d
Compare
todthomson
changed the title
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
46c13f7 to
383b878
Compare
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
@claude review (and give me ALL your findings + split off your findings on build-and-test-linux-docker-image.sh into a separate section as it's a TEST not code that actually runs in production) |
Local Claude reviewReviewed the PR from a local checkout: built the image, ran the script, and verified each claim against a real container rather than reading the diff. Findings and what was done about them below. Everything actioned landed in 00304c6. Verified sound (no change needed)
Findings1.
|
| Package | In base | Candidate | Verdict |
|---|---|---|---|
ca-certificates |
— | 20260601~22.04.1 | must list |
libicu70 |
— | 70.1-2 | must list |
libssl3 |
3.0.2-0ubuntu1.26 | 3.0.2-0ubuntu1.29 | listing is what patches it |
libc6, libgcc-s1, libgssapi-krb5-2, libstdc++6, zlib1g |
current | current | genuine no-ops |
So five were dropped and three kept. Dropping libssl3 as "already present" would have shipped OpenSSL at .26 instead of .29 — verified the built image still reports libssl3 3.0.2-0ubuntu1.29 after the trim. The distinction, and the warning that a no-op today stops being one as soon as the base lags behind jammy-security, is now in the Dockerfile.
Worth saying plainly: explicitly listing base-image packages is not pure noise, it's what pulls security updates. Trim this list with care.
5. Test script placement — moved and referenced
819 lines at the repo root, not referenced from any readme and not wired into CI, so nothing would ever run it. Now at testing/docker-linux/build-and-test-linux-docker-image.sh, with references added to README.md (under Build, Test and Delivery), testing/README.md (new Linux Docker Image section with the stage table and licence notes), and docker/readme.md (under Additional Information — that file is the customer-facing Docker Hub readme, so the reference is scoped to contributors).
The move needed a code fix: REPO_DIR resolved from the script's own directory, which would have pointed at testing/docker-linux and broken every relative path. It now resolves two levels up and asserts it found the repo root.
Still not wired into TeamCity. There is a Test: Windows Docker Image config but no Linux equivalent, so runtime regressions in this image remain uncovered by CI — worth a follow-up.
6. Smaller script issues — all fixed
grep -qacross two files.grep -q "^dockremap:" /etc/subuid /etc/subgidexits 0 on the first match in either file, so it would have passed with only one of the two present. Confirmed with a two-file test. Now checked separately.- Weak version assertion.
SHORT_VERSION="${BUILD_NUMBER%%-*}"reduced to9.2.4324and only asserted the machines JSON contained it.EXPECTED_VERSIONis now hoisted out of the smoke stage and reused, so it compares the full FullSemVer. - Empty env values.
OCTOPUS_SERVER_BASE64_LICENSE=andMASTER_KEY=were passed as empty strings; both are now omitted entirely when unset. - Work done under
--skip-e2e. Passwords were minted and temp files written even when stage 4 was skipped. Now all of it lives ine2e_prepare, called from stage 4 only. Verified--skip-e2ecreates no temp files. - Indentation. The licence branch sat at column 0 inside the stage 4 block, leaving a bare
fi/fipair. Reindented.
Verification after the changes
Rebuilt and re-ran the full script from its new location — 47/47 passing, same as before the review:
- Smoke, 37/37. Notably the eight runtime-dependency assertions still pass with only three named in the Dockerfile, which is the safety net working as intended. The built image reports
libssl3 3.0.2-0ubuntu1.29, so trimming the list cost no security patches. - e2e, 10/10, licensed. Both Tentacles registered in 20s, health-checked in 5s,
TentaclePassive/TentacleActiveas expected. This exercised every stage 4 change: dropping the emptyMASTER_KEYdid not upset the server container (up in 85s), the licence still reaches it through the env file, and the version assertion now matches the full9.2.4324-lev-tod-...-ubuntustring off the wire rather than just the9.2.4324prefix. - Teardown leaves no containers, no network and no temp files behind.
bash -nandshellcheck -S warningclean on both shell files.
e2e_prepare was also exercised in isolation both ways: with OCTOPUS_SERVER_BASE64_LICENSE unset the server env file contains only ADMIN_PASSWORD and DB_CONNECTION_STRING, and with it set the licence key appears. Files are 0600 and removed by the trap.
🤖 Generated with Claude Code
|
@claude review (and give me ALL your findings + split off your findings on |
There was a problem hiding this comment.
All five findings from the earlier rounds of this review are fixed correctly. Verified in the code: resolve_license now strips \n\r before the value reaches the env-file; e2e_teardown is split into an unconditional e2e_cleanup_stale/e2e_remove_stack plus a --keep-gated e2e_teardown for the EXIT trap; wait_for now takes trailing container names and fails fast on an exited state; the previously-unguarded ENVIRONMENTS=$(...)/MACHINES=$(...) captures now fall back to || VAR=""; and all e2e container/network names carry a $$-based suffix (E2E_RUN_ID) to avoid collisions between concurrent runs.
Extended reasoning...
Overview
This PR moves the Linux Tentacle Docker base image from debian:11-slim to ubuntu:22.04, updates install-docker.sh's apt repository setup for Ubuntu/jammy (including fixing a genuinely broken if [ iptables ... ] conditional), and adds a large new local verification script (~979+ lines) covering deb build, image build, smoke tests, and an end-to-end SQL Server + Octopus Server + Tentacle registration test.
Security risks
No new risks beyond what was already reviewed in prior rounds: secrets (SQL SA password, Octopus admin password, license) are generated per-run and passed via 0600 --env-file, never -e, and the license is scrubbed from dumped logs. The license-newline fix in this round closes a correctness bug that could have corrupted the env-file, not a security hole per se.
Level of scrutiny
This is now the third review round on this PR. In the first two rounds I raised one 🔴 (license newline corruption) and four 🟡 findings (teardown guard, wait_for fail-fast, unguarded API captures, concurrent-run name collisions). This round I re-read the current diff and confirmed each of those five is fixed with a correct implementation (spot-checked the actual code, not just the commit message claims), plus a bonus fix (find_deb's || true for the "or empty" contract under pipefail) that was not part of my original findings but is consistent with the stated intent.
Other factors
No third-party CHANGES_REQUESTED or unresolved objections are indicated in the timeline beyond my own prior findings, which are now resolved by code, not just by author self-resolution of the threads. Given everything raised was substantively addressed and verified against the actual diff, and the change itself (base image swap plus a local-only test script) is well-scoped and thoroughly self-tested (50/50 per the author's verification), I'm approving.
There was a problem hiding this comment.
All five findings from the previous review round have been genuinely fixed in code (not just resolved as threads): the licence value is stripped of embedded newlines before being written to the env file, wait_for now takes container names and fails fast if one exits instead of burning the full timeout, teardown is split into an unconditional e2e_cleanup_stale and a --keep-gated e2e_teardown so repeated --keep runs no longer collide, every octopus_api/machines_json capture in the e2e and smoke stages is now guarded, and all e2e container/network names carry the script's PID so concurrent runs don't collide. I re-checked each of these directly against the current file and they hold up. No new issues found.
Extended reasoning...
Overview
This is a follow-up push on a PR I've already reviewed three times. The prior round produced 5 inline findings on testing/docker-linux/build-and-test-linux-docker-image.sh: one 🔴 (license value with embedded newlines corrupting the env file) and four 🟡 (teardown/pre-run cleanup sharing one --keep guard, wait_for not detecting an exited container the way wait_for_log does, several unguarded VAR=$(...) captures under set -euo pipefail that could abort the script mid-stage, and fixed e2e container/network names colliding across concurrent runs).
Verification of fixes
I did not rely on thread-resolution metadata (all threads were resolved by the author or this bot's own prior session, which the instructions correctly treat as non-independent). Instead I re-read the current file directly:
- License:
OCTOPUS_LICENSE=$(printf '%s' "$OCTOPUS_LICENSE" | tr -d '\n\r')at line 463, before it reachesnew_env_file/server_env. - Teardown split:
e2e_remove_stack(actual removal),e2e_cleanup_stale(unconditional, pre-run, filtered to this run's own names), ande2e_teardown(--keep-gated, used only by the EXIT trap). wait_fornow accepts trailing container names and checksdocker inspect -f '{{.State.Status}}'forexitedafter each poll, failing fast with a clear message instead of sitting out the full timeout.- The
ENVIRONMENTS=$(...)andMACHINES=$(...)captures are now guarded with|| ENVIRONMENTS=""/|| MACHINES="", matching the pattern already used in the smoke stage. - E2E names are suffixed with
E2E_RUN_ID="$$", and pre-run cleanup only targets names matching this run's PID, avoiding both the previous collision and a new one that an unconditional pre-run cleanup would have introduced.
The commit message for 7c521347c6dc also documents a bonus fix (find_deb silently aborting under pipefail on a clean checkout with --skip-deb) that lines up with what the diff shows.
Security risks
No new security-relevant surface was introduced since the last review; the one security-relevant finding (license value corruption) is fixed. Secrets continue to be passed via 0600 --env-file rather than -e/argv, as previously described.
Level of scrutiny and other factors
The production-facing files (docker/linux/Dockerfile, docker/linux/install-scripts/install-docker.sh) are unchanged from what I reviewed previously and match the PR description precisely — base image swap, package name changes for jammy, deb822 Docker repo source, and the dead if [ iptables -nL ... ] conditional replaced with a fixed nft selection (correctly identified as a no-op behavior change, since the original test was always false). The large test script is where all substantive risk lived, and that risk has been addressed through three iterations of review and fix. Given the fixes are verified directly in code and no new issues were found, I'm approving rather than deferring again.
xwipeoutx
left a comment
xwipeoutx
left a comment
There was a problem hiding this comment.
This all looks good to me.
Squashed from seven commits. Their descriptions follow, in order.
== Move the Linux Tentacle Dockerfile from Debian 11 to Ubuntu 22.04
Switching the base image alone is not enough: the package names in the
Dockerfile were Debian 11 (bullseye) specific and the Docker apt source
was hardcoded to the Debian repo, so the image failed to build.
- Use the Ubuntu 22.04 (jammy) names for the .NET runtime dependencies:
libicu67 -> libicu70 and libssl1.1 -> libssl3, neither of which exists
on jammy at all (the build died with "Couldn't find any package by
regex 'libssl1.1'"). Also libgcc1 -> libgcc-s1; libgcc1 survives on
jammy only as a transitional dummy package. The .deb's own dependency
is already satisfied by libssl3 (libssl1.0.0 | libssl1.0.2 |
libssl1.1 | libssl3), so no packaging change is needed.
- Point install-docker.sh at download.docker.com/linux/ubuntu. It used
the debian repo, and since `lsb_release -cs` now returns "jammy" there
is no such suite under the Debian path, so the docker-in-docker
install would have failed.
Verified end to end with build-docker-linux.sh: Ubuntu 22.04.5 LTS,
OpenSSL 3.0.2, tentacle and dockerd both working, and a Tentacle from
the image configures itself and connects to a real Octopus Server.
== Add build-docker-linux.sh to build and verify the Linux image locally
The Linux Tentacle image is built in TeamCity, not by NUKE in this repo,
so there was no local way to check a change to docker/linux/Dockerfile
short of pushing. This reproduces that chain and then tests the result.
Stages, matching TeamFireAndMotion_OctopusTentacle_TentacleVLatest_net80:
1. deb - PackDebianPackage, restricted to linux-x64
2. image - the same docker-compose.build.yml command TeamCity runs,
with BUILD_NUMBER derived from the .deb filename
3. smoke - 37 assertions over the image contents and behaviour
4. e2e - SQL Server + Octopus Server + a listening and a polling
Tentacle built from the fresh image
Notes on why some of this is shaped the way it is:
- NUKE resolves every [ParameterFromPasswordStore] at start-up, which
shells out to the 1Password CLI and hangs when `op` cannot prompt.
Those secrets only feed the SBOM target, so the script opts out with
OCTOPUS__Tests__SecretManagerEnabled=False.
- Everything is pinned to linux/amd64, since the .deb is amd64 and this
needs to work from an arm64 (Apple Silicon) checkout.
- The existing Pester suite could not be reused: docker/Tests and
docker-compose.yml are Windows-container based and assume a server
with imported data, so stage 4 is new.
- An unlicensed Octopus Server enforces a Targets limit of 0 on
2026.4.x, so no target can register. With no licence the stage
asserts configuration and connectivity up to the licence refusal;
set OCTOPUS_SERVER_BASE64_LICENSE to assert full registration,
comms style and health status instead.
== Source a licence in build-docker-linux.sh so the e2e test can register
An unlicensed Octopus Server allows 0 targets, so stage 4 could never
prove the thing it exists to prove: that a Tentacle built from this
image registers and health-checks. Resolve a licence instead, from
OCTOPUS_SERVER_BASE64_LICENSE or, failing that, the same 1Password item
the Octopus Server repo's ./environment.sh setup uses:
op://software licencing/octopus deploy ultimate license key base64
The 1Password lookup only runs when stdin is a TTY. `op` has to prompt
to authenticate, so with nothing to prompt it hangs or fails - the same
trap that stalls NUKE's own 1Password lookup in stage 1 - and gating on
a TTY keeps it out of CI runners, background shells and agents without
anyone having to remember a flag. --no-1password is a hard override.
Handling of the licence itself:
- Passed to the container via a 0600 --env-file, not -e, so it stays
out of `ps` output.
- Never echoed; only its length is logged.
- dump_logs scrubs it, because the server image logs its own
`license --licenseBase64 <value>` invocation and would otherwise
print the licence verbatim if the server failed to start.
Verified: 47/47 with a licence, registration in 15s and health checks
in 5s, and no occurrence of the licence anywhere in the run log. The
TTY gate was unit-tested both ways - skipped with no TTY, reached under
a real pty. Without a licence the stage still falls back to asserting
configuration and connectivity up to the licence refusal (44/44).
== Rename build-docker-linux.sh to build-and-test-linux-docker-image.sh
Most of the script is assertions, not build steps, so a name leading with
"build" hid its main purpose: anyone editing docker/linux/Dockerfile had
no cue that this is the thing to run. The new name also matches TeamCity's
own vocabulary for the equivalent Windows job ("Test: Windows Docker
image") and says which of the repo's three Dockerfiles it covers.
== Generate the e2e container passwords instead of hardcoding them
The SQL Server SA and Octopus admin passwords were fixed literals in the
script. They now come from random_password(): 20 characters, fresh per
run, never written anywhere but the containers they are made for.
Alphanumeric only, because the value is interpolated into a SQL Server
connection string, a `sqlcmd -P` argument and a JSON request body, and
punctuation would need escaping differently in each of the three.
With punctuation excluded, at least one upper, one lower and one digit
is guaranteed rather than left to chance: SQL Server's SA policy requires
characters from three of its four categories, so a random draw missing
one would fail the container at start-up. /dev/urandom is read in fixed
chunks so `head` finishes before `tr` and the pipeline cannot be
SIGPIPE'd into tripping `set -o pipefail`.
Verified: generator gives 20 chars, correct charset and all three classes
over 1000 draws, 200/200 distinct; full run 47/47 green, so SQL Server
accepts a symbol-free SA password and Octopus accepts a random
alphanumeric admin password through both install and API login.
== Pass every e2e secret via --env-file instead of -e
The licence already went in through an env file; the generated SQL SA and
Octopus admin passwords did not, so they sat in `ps` output for the life
of each container. Covers all five places a secret previously reached a
command line:
- SQL Server container -> E2E_ENV_SQL
- Octopus Server container -> E2E_ENV_SERVER (licence, admin password and
the connection string that embeds the SA one)
- both Tentacle containers -> E2E_ENV_AGENT
- the curl helper container -> E2E_ENV_API; the helper now reads
OCTO_USER/OCTO_PASS from its environment
rather than taking them as argv
- the SQL readiness probe -> reads $MSSQL_SA_PASSWORD inside the
container, so it is off the `docker exec`
argv too
One file per consumer, so no container is handed a credential it has no
use for. All are 0600 and removed by e2e_rm_temp, which both EXIT traps
now share - including the --skip-e2e path, where previously only the
helper was cleaned up.
Docker takes env-file values literally, which suits the connection
string's ';' and '=' characters with no quoting needed.
Verified: no secret remains on any `-e` flag (remaining ones are
ACCEPT_EULA, MSSQL_PID, ADMIN_USERNAME, MASTER_KEY, ServerUrl/Port,
Target* and the API-FAKE placeholder); full run 47/47 green; temp files
gone afterwards.
== Document the dead iptables-legacy branch in install-docker.sh
`if [ iptables -nL > /dev/null 2>&1 ]` never succeeds: `[ ]` is the test
builtin, not a subshell, so iptables is never executed. Test just compares
the literal strings `iptables` and `-nL`, and with two arguments expects
the first to be a unary operator, which `iptables` is not - so it errors
and returns non-zero. The legacy branch is therefore dead code and nft is
always selected.
Comment only. Fixing it (drop the brackets) would flip docker-in-docker's
firewall backend from nft to legacy on most hosts, which needs its own
testing. The behaviour predates the move off Debian, so it is not a
regression from that change.
+semver: major
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dockerfile: - `apt install ./tentacle_*.deb` -> `apt-get install -y --no-install-recommends`. Without `-y`, apt aborts as soon as the .deb needs a dependency the earlier layers did not already install, which is the exact invariant the base image change moves. - Drop libc6, libgcc-s1, libgssapi-krb5-2, libstdc++6 and zlib1g from the runtime dependency list; ubuntu:22.04 already ships all five at the newest version jammy offers, so naming them installed nothing. ca-certificates, libicu70 and libssl3 stay - the first two are absent from the base image, and libssl3 is present but at 3.0.2-0ubuntu1.26 when jammy-security has 3.0.2-0ubuntu1.29, so naming it is what pulls that security update in. Documented in the file so the list is not "tidied" again by mistake. install-docker.sh: - Delete the dead `if [ iptables -nL > /dev/null 2>&1 ]` conditional and its unreachable iptables-legacy branch, and set nft unconditionally. `[` is the test builtin, so iptables never ran and test failed with "unary operator expected" - nft has always been selected, making this a no-op at runtime. Also gets shellcheck parsing the file again; it was erroring with SC1073/SC1072 and bailing out mid-file. Test script: - Move to testing/docker-linux/, and resolve the repo root two levels up rather than from the script's own directory. - Reference it from README.md, docker/readme.md and testing/README.md. - Check /etc/subuid and /etc/subgid separately; `grep -q a b` exits 0 on the first match in either file, so the combined form passed with only one of the two. - Hoist EXPECTED_VERSION out of the smoke stage and use it for the on-the-wire version assertion, instead of a bare numeric prefix that could match a coincidental substring. - Only mint credentials, read 1Password and write temp files when stage 4 is actually going to run. - Omit OCTOPUS_SERVER_BASE64_LICENSE and MASTER_KEY entirely when empty, rather than passing them as empty values. - Indent the licence branch to match the block it sits in. Verified: image rebuilds clean and all 37 smoke tests still pass. The smoke tests assert all eight runtime dependencies are present regardless of what installed them, and libssl3 is still 3.0.2-0ubuntu1.29 in the built image after the list was trimmed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… layer both_healthy counted occurrences of `"HealthStatus":"Healthy"|"HasWarnings"` across the whole /api/machines/all payload and asserted there were at least two, without tying either status to a machine. With only the two Tentacles registered it could not really lie, but a stale machine left behind by an earlier run would let one healthy record stand in for a Tentacle that was still unhealthy. machine_health now resolves the status of a named machine by splitting the payload into one line per machine first, and both_healthy checks each Tentacle by name. awk does the split because sed's replacement-side `\n` is not portable to the BSD sed on macOS. Also add the `apt-get clean` the first apt layer was missing; the other two layers in the Dockerfile already had it. Verified: 47/47 (37 smoke + 10 licensed e2e), including "both Tentacles reported Healthy" through the new per-machine path. libssl3 in the built image is still 3.0.2-0ubuntu1.29 and the image is still 1.17 GB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up on the four remaining review findings.
install-docker.sh - three packages dropped from the repository setup:
- apt-transport-https is a transitional stub on jammy ("transitional
package for https support", depending only on apt >= 1.5~alpha4); apt
2.4 ships /usr/lib/apt/methods/https itself.
- gnupg was only needed for `gpg --dearmor`. The key is now fetched
ASCII-armoured and referenced directly from Signed-By, verified with
gpgv, which apt itself depends on.
- lsb-release is replaced by /etc/os-release. It was also the largest of
the three, though not for the reason first assumed: it does pull in
Python 3.10, but so does networkd-dispatcher via docker-ce's apparmor
chain, so Python stays in the image either way.
The repository is now declared the way Docker's own Ubuntu instructions
do it: the armoured key under /etc/apt/keyrings, referenced from a deb822
.sources file rather than a one-line .list. Verified against a bare
ubuntu:22.04 with only ca-certificates and curl added - InRelease fetched
and verified, docker-ce resolvable, no apt warnings.
install-docker.sh - one package added:
- iproute2, for `ip`, which dockerd-entrypoint.sh uses in _tls_san.
dockerd does not need it (confirmed: the daemon starts, reports no
missing binaries and runs a nested container without it), but without
it a dind TLS certificate is issued with DNS SANs only - measured as
DNS:docker,DNS:<hostname>,DNS:localhost against
DNS:...,IP:127.0.0.1,IP:172.17.0.2,IP:::1 with it - so a client
connecting to the daemon by IP fails hostname verification.
Test script:
- resolve_license now strips newlines from the licence. An embedded one
split the --env-file entry in two, handing the server a truncated
licence plus a garbage second variable, and broke the sed that scrubs
the licence from dumped container logs ("sed: 1: s|AAAA", exit 1). Both
confirmed against the old code.
- Smoke tests follow the repository move: they read docker.sources rather
than docker.list, assert the Signed-By path, check the keyring is
present, readable and armoured, and check `ip` is on PATH.
Verified: 50/50 (40 smoke + 10 licensed e2e), image 1.17 GB -> 1.16 GB,
213 packages. shellcheck -S warning clean on both shell files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All four remaining bot findings; the fifth (licence newlines, the only 🔴) was already fixed in 916b0c2. e2e_teardown did two jobs behind one --keep guard: pre-run cleanup and EXIT-trap teardown. After a --keep run, the next --keep run returned early from the pre-run call and then died on a `docker run --name` conflict. Split into e2e_remove_stack (the actual removal), e2e_cleanup_stale (unconditional, pre-run, and says what it removed) and e2e_teardown (--keep-gated, EXIT trap only). wait_for now takes optional trailing container names and fails immediately if one has exited, the way wait_for_log already did. The four call sites have 420-900s timeouts, so a container that died on startup previously sat out the whole wait and reported a timeout that hid the real cause. Measured 0s instead of 300s on a container that exits immediately. The condition is still evaluated before the exit check, so a container that satisfies the test and then exits still counts as ready. Every `VAR=$(...)` capture in the smoke and e2e stages is now guarded. Unguarded, a transient docker or API failure exited the shell at the point of capture, losing dump_logs, the test tally and the pass/fail verdict - which defeats the point of fail() returning 0 so a run always reports. A dead Docker daemon is still caught up front by the `docker info` check, so this cannot mass-fail for that reason. Found while doing that, and not in the bot's list: find_deb documents "or empty if there are none", but with no match the glob is passed through, `ls` fails, and under pipefail `DEB_PATH=$(find_deb)` aborted the script before reaching the `die` that explains what is missing - so --skip-deb on a clean checkout exited silently. Fixed in find_deb itself. Every Docker name in stage 4 now carries the script's PID as a suffix, so concurrent runs on one host cannot collide. This matters for the CI work in LEV-1833, where a PR build and a nightly can overlap on a shared agent. Note that fixing the --keep issue above made this worse before fixing it here: an unconditional pre-run cleanup would reliably destroy a concurrent run's live stack. Pre-run cleanup is now scoped to this run's own names. Verified: 50/50 (40 smoke + 10 licensed e2e), suffixed names registering and health-checking end to end, clean teardown. shellcheck -S warning clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bb3376b to
964a092
Compare
|
Closing this (approved) PR, as this code will come in with the code in stacked PR: #1293. |
3 participants
Summary (Tod-written)
Moves
docker/linux/Dockerfilefromdebian:11-slimtoubuntu:22.04.This fixes the broken
Build: Linux Docker imageand anything else that depends on that Dockerfile.This (IMHO) is a breaking change (i.e. who knows who is depending on Tentacle having something Debian 11 specific that isn't included in Ubuntu 22.04 e.g.
libssl1.1so we need to rev the major version, which I will do as part of the merge commit).Tests still broken in this PR are fixed in the next (stacked) PR: #1293.
Why Ubuntu 22.04 specifically
22.04 is deliberate, for two reasons:
libgcc1->libgcc-s1,libicu67->libicu70,libssl1.1->libssl3) plus repointing Docker's apt repo at/linux/ubuntu. Nothing in the.deb, the entrypoint scripts or the image's behaviour had to change with it.docker/kubernetes-agent-tentacle/Dockerfilealready uses viaruntime-deps:8.0-bookworm-slim- would leave the Linux image on a distro we can't build on.Ubuntu 24.04 is the eventual destination (it costs
libicu70->libicu74), but there's no 24.04 build agent yet either, so that's a follow-up rather than part of this.Why the runtime dependency list shrank
libc6,libgcc-s1,libgssapi-krb5-2,libstdc++6andzlib1gwere dropped from the firstapt-get install.ubuntu:22.04already ships all five at the newest version jammy offers, so naming them installed nothing.ca-certificates,libicu70andlibssl3stay, and the reason differs for each: the first two are genuinely absent from the base image, whilelibssl3is in the base but at3.0.2-0ubuntu1.26when jammy-security has3.0.2-0ubuntu1.29. Naminglibssl3is precisely what pulls that security update in, so dropping it as "already present" would have shipped an unpatched OpenSSL. That distinction, and the warning that a no-op today stops being one as soon as the base image lags behind jammy-security, is now written into the Dockerfile so the list doesn't get "tidied" again by mistake.The smoke tests still assert all eight packages are present in the built image regardless of what installed them, so a future base that stops shipping one of them fails there rather than at runtime.
Details (Claude-authored)
Moves
docker/linux/Dockerfiletoubuntu:22.04. The base image swap alone doesn't build — two Debian-specific things had to change with it.Not a one-line change
libicu67andlibssl1.1don't exist on jammy at all — the build failed withE: Couldn't find any package by regex 'libssl1.1'. Nowlibicu70andlibssl3, pluslibgcc1→libgcc-s1(libgcc1survives on jammy only as a transitional dummy). The.deb's own dependency line already acceptslibssl3, so no packaging change was needed.install-docker.shpointed at the Debian apt repo.lsb_release -csnow returnsjammy, and there's no such suite underdownload.docker.com/linux/debian— confirmed404 Not Found→E: The repository ... does not have a Release file. Since that script runsset -eux, docker-in-docker would have failed to install. Repointed at/linux/ubuntu.Both failures would have gone red in
Build: Linux Docker image, so CI would have caught them — but only at build time, which is the gap the script below closes.Verification
The Linux image is built in TeamCity, not by NUKE here, so there was no local way to check a Dockerfile change short of pushing.
testing/docker-linux/build-and-test-linux-docker-image.sh(added here) reproduces that chain and tests the result — 50/50 passing:PackDebianPackage, linux-x64 onlydocker-compose.build.ymlcommand TeamCity runsThe e2e stage registers both Tentacles, asserts
TentaclePassive/TentacleActive, waits for health checks, and confirms the server reads the built version off the wire. Registration in 15s, health in 5s.Confirmed in the image: Ubuntu 22.04.5 LTS, OpenSSL 3.0.2,
tentacleanddockerdboth working.Worth knowing: there is a
Test: Windows Docker Imageconfig in TeamCity but no Linux equivalent —Build: Linux Docker imagefeeds straight intoBuild: Docker manifestwith nothing starting a container. So build-time regressions are covered today; runtime ones are not. Wiring this script in is a sensible follow-up.Secret handling in the test script
OCTOPUS_SERVER_BASE64_LICENSE, else the same 1Password item./environment.sh setupuses — and only when stdin is a TTY, sinceopcan't prompt in CI or under an agent.--no-1passwordis a hard override. Without a licence the stage falls back to asserting configuration and connectivity up to the licence refusal.[A-Za-z0-9]), not hardcoded. Alphanumeric because the value lands in a SQL connection string, asqlcmd -Pargument and a JSON body; one upper, one lower and one digit are guaranteed rather than left to chance, since SQL Server's SA policy needs three of its four categories.0600 --env-file, never-e, so none appear inps. One file per consumer. The licence is additionally scrubbed from dumped container logs, because the server image logs its own--licenseBase64invocation.Notes for review
Product.wxsand.nuke/build.schema.json— NUKE side-effects, noted in the script header./etc/apt/keyrings/docker.ascand is referenced from a deb822.sourcesfile rather than a one-line.list. That letapt-transport-httpsgo (a transitional stub on jammy; apt 2.4 ships thehttpsmethod itself),gnupggo (only ever needed forgpg --dearmor— apt reads the armoured key directly and verifies it withgpgv, which apt depends on), andlsb-releasego (replaced by/etc/os-release). Verified against a bareubuntu:22.04with onlyca-certificatesandcurladded: InRelease fetched and verified,docker-ceresolvable, zero apt warnings. Image 1.17 GB → 1.16 GB, 213 packages. Note Python 3.10 stays regardless — it arrives vianetworkd-dispatcherindocker-ce's apparmor chain, not vialsb-release.iproute2added.dockerddoes not need it — confirmed by running the image--privileged, where the daemon starts, reports no missing binaries and runs a nested container without it. Butdockerd-entrypoint.shusesipin_tls_san, and without it a dind TLS certificate is issued with DNS SANs only (measured:DNS:docker,DNS:<hostname>,DNS:localhost, against…,IP:127.0.0.1,IP:172.17.0.2,IP:::1with it), so a client connecting to the daemon by IP fails hostname verification. Upstreamdocker:dindships it for the same reason.docker-ce-cliRecommends (verified present: buildx v0.37.0, compose v5.5.1).install-docker.shhadif [ iptables -nL > /dev/null 2>&1 ], which istestcomparing two literal strings rather than running the command — it fails with "unary operator expected" (exit 2), so theiptables-legacybranch was unreachable and nft has always been selected. The conditional and its dead branch are now deleted and nft is set unconditionally, which is a no-op at runtime and gets shellcheck to parse the file again (it was erroring with SC1073/SC1072 and bailing out mid-file). Actually selecting legacy would flip dind's firewall backend and still needs its own testing, so that is untouched.apt install ./tentacle_*.debis nowapt-get install -y --no-install-recommends ./tentacle_*.deb. Without-y, apt aborts (Do you want to continue? [Y/n] Abort., exit 1) the moment the.debneeds a dependency the earlier layers didn't already install — the exact invariant this base-image change moves. It also drops apt's "does not have a stable CLI interface" warning from the build log.Follow-ups raised
Test: Linux Docker Imageconfiguration that runs this script, so runtime regressions in the image are caught rather than shipped. Probing the image for this PR also disproved the script's own assumption that privileged dind cannot run under emulation:dockerdcame up (29.8.0, overlayfs) and ran a nested container, so asserting "the daemon starts and can run a container" is both feasible and the highest-value assertion missing today. Theiproute2gap above went unnoticed precisely because nothing exercises dind.mcr.microsoft.com/dotnet/runtime-depsasdocker/kubernetes-agent-tentacle/Dockerfilealready does. Deliberately not done here: it needs the whole dind payload re-verified on a new base, and it depends on LEV-1833 for anything better than hand-verification.CI status
The three red integration-test checks are not from this change —
Build: Linux Docker imagepasses:Integration Test: Linux (not sudo)— 38 tests failing with Tentacle child processes aborting (SIGABRT). Also failing onrefs/heads/main(main-nightly24129127) and on unrelated PRs Update dependency Octopus.1Password.Sdk to 0.0.407 #1187 and Add ability to observe the number of connections halibut opens for polling connections #1285. Last green main was 6 Sep; this branch's first build already carried all 38.Integration Test: Windows— all 861 tests pass, the Nuke step exits 1. The three prior builds of this branch were green. Flake.🤖 Generated with Claude Code