Fix(agent): Improve tunnel preservation during agent upgrade when using tunnel. Housekeeping of shellhub-agent files in own directory - #5943
Conversation
fbab19e to
a7beeba
Compare
a7beeba to
1e42d5b
Compare
ltan10
changed the title
1e42d5b to
50f40b7
Compare
otavio
left a comment
otavio
left a comment
There was a problem hiding this comment.
The biggest problem I foresee here is migrating the field-deployed agents.
So even though I agree with the idea of this change, we need to consider how we will migrate the existing agents to work with this new approach or provide a backward-compatible way for them to keep working.
How do you foresee solving this issue?
|
With all the testing. I've stumbled upon this myself. The only persistent remaining file is just Two methods for transition:
@otavio someone should be able to replicate my findings for confirmation. EDIT: |
|
@otavio You are correct that users using the tunnel to upgrade their agent will have their tunnel ceremoniously terminated when migrating from the runc binary to go-native binary. I tried sending the shell install script to background and including using nohup to ignore sighup. However still no luck as the child process executing the agent install did not appear to initiate. This observation was the same for an standalone agent upgrade made through the tunnel for the following situations
With all tunnels terminated when users used shellhub tunnel to upgrade the agent |
|
Found a work around, can someone review please and test the containerised installation/upgrade. Problem: Running the installation under systemd-run worked, but once again was not verbose, and increased complexity in managing the existing service once finished. Solution: Changes: |
ad7c9d8 to
0d222f4
Compare
ltan10
changed the title
88ffb2d to
0d222f4
Compare
0d222f4 to
0d6ec33
Compare
@otavio I noticed that the migration from non native shellhub-agent to the go-native shellhub-agent triggers the same issue of a new pending device join request due to the change in location of shellhub.key from I have provided a migration path for users who don't wish to deal with just removing old device and re-accepting the new device above. By moving the shellhub-agent service restart to end of installation, this allows users to upgrade the agent over a shellhub tunnel as the tunnel is not terminated when the service is stopped at the start of the installation process. |
0d6ec33 to
35b4a99
Compare
35b4a99 to
5148282
Compare
5148282 to
ae06a1d
Compare
|
/review |
|
@ltan10 You are not authorized to request an explicit review. If you believe this PR needs a new automated review round, please tag the |
|
@shellhub-io/admin |
|
/review |
|
Claude finished @otavio's task in 23s —— View job Code Review CompleteReviewed 3 files across code quality, security, testing, Go patterns, and architecture — no issues found. The code looks good as-is. Review notes:
If you've addressed feedback and want a new review, tag |
ae06a1d to
6d88f23
Compare
|
Rebased branch. Proposed change completed. @otavio any chance PR to be merged for next release? |
6d88f23 to
772ff28
Compare
|
@ltan10 There is still a conflict that needs to be resolved. Can you take a look? Besides this, I am wondering what tests of the upgrade you have done? |
772ff28 to
6ec1a39
Compare
|
@otavio rebased to the master branch to resolve the conflicts. I've also updated the comments to make more sense. As for the testing, I have only performed the For an upgrade context, I have tested the upgrade over an active remote ShellHub SSH tunnel multiple times, and was successful each of those times. However like with every remote configuration there is a chance that it may fail. Every upgrade for runc-agent and go-agent always rewrites it's shellhub-agent.env as is normal behavior. Observations for the tests are mentioned in previous comments. I'll try to summarize them here. When upgrading from the runc-agent to "ungrouped" go-agent or "ungrouped" go-agent to this "grouped" version, the existing private key is not migrated. As a result, the new agent will generate a new key, and the device will be treated as a new device registration and await acceptance on the dashboard. Subsequently any existing A migration guide is included in the PR description with steps to preserve the existing key and avoid re-registration. I have NOT tested the |
fe93e24 to
ea1a9bd
Compare
Create directory if doesn't exist.
agent installer: avoid service disruption during upgrade over SSH tunnel - Remove pre-install service disable step - Enable service without starting it - Restart service after installation completes This allows upgrades over an active SSH tunnel while ensuring the service restarts with the updated binary and re-establishes the tunnel.
6ec1a39 to
d5427ac
Compare
otavio
left a comment
otavio
left a comment
There was a problem hiding this comment.
Thanks for digging into this, and especially for the systemctl restart-at-the-end finding — the SIGHUP-over-the-tunnel diagnosis in the thread is correct and that part of the change is the right shape.
I've left inline comments on the specific lines. Below are the items that have no line in the diff to hang off.
1. No code-level migration path (blocking)
This is the point I raised earlier in the thread, and it is still open. The PR gives a migration path in prose only: the user must hand-create /etc/shellhub-agent/ and copy the key before upgrading. Every field-deployed agent whose operator doesn't read the release notes re-enrols as a new pending device, leaving the old accepted device behind with a clashing hostname to be deleted by hand.
We can't ship a silent identity change for existing installs. The agent should fall back to the legacy location when the new one is absent — read /etc/shellhub.key (and /opt/shellhub/shellhub.key) if /etc/shellhub-agent/shellhub.key does not exist, and ideally migrate it on first run. Same for the env file. That turns the migration guide into a safety net rather than a prerequisite.
2. agent/pkg/agentd/envfile.go:11 still points at the old path
const defaultEnvFilePath = "/etc/shellhub-agent.env"This is the only production caller (agent/pkg/agentd/agent.go:171, via LoadConfigFromEnv), so the runtime never reads the new env file except through systemd's EnvironmentFile=. Two consequences:
- Regression on a fresh install:
shellhub-agent loginand any manual non-systemd invocation now fail —LoadConfigFromEnvrequiresSHELLHUB_PRIVATE_KEY/SHELLHUB_SERVER_ADDRESSand the fallback file it was given in 6f57bd0 no longer exists at that path. - Split brain on upgrade: systemd reads the new file, while the stale
/etc/shellhub-agent.envsurvives and is what hand-run invocations read — pointing at the old key path.
3. Docs still document the old paths
ui/apps/docs/src/pages/get-started/install/{standalone,docker,podman,raspberry-pi}.mdx still reference /etc/shellhub.key.
4. The new defaults have no test coverage
install.bats passes (82/82), but only because it always exports PRIVATE_KEY (line 20) — it never exercises the defaults, in either direction. That is why the three stale defaults flagged inline slipped through. Worth a case that asserts the default path per install method.
5. Commit messages
Per our code style, the reason behind a workaround lives in the commit message, not in a comment — see the inline notes. Concretely:
0aa4b7b("Create directory if doesn't exist.") gives no rationale at all for the path move: no migration story, no reason for the directory mode. Itschore(agent)scope also mislabels changes toinstall.shandagent/packaging/.d5427acsays "Remove pre-install service disable step" but omits why — the SIGHUP mechanism currently survives only in the banned comment. That explanation is the valuable part; please move it into the commit body in full.
Happy to re-review once the migration fallback and the stale paths are sorted.
|
|
||
| const ( | ||
| agentEnvFile = "/etc/shellhub-agent.env" | ||
| agentEnvFile = "/etc/shellhub-agent/shellhub-agent.env" |
There was a problem hiding this comment.
agentEnvFile now points at /etc/shellhub-agent/shellhub-agent.env, but agent/pkg/agentd/envfile.go:11 still has:
const defaultEnvFilePath = "/etc/shellhub-agent.env"That is the only production caller (agent/pkg/agentd/agent.go:171 via LoadConfigFromEnv), so the agent runtime never reads the file this installer writes — only systemd does, through EnvironmentFile=. Result: shellhub-agent login and any manual invocation break on a fresh install, and on an upgrade the stale /etc/shellhub-agent.env keeps pointing at the old key path.
More generally, the layout is now described by two hardcoded strings in two packages. A single seam in agent/pkg/agentd (ConfigDir() / EnvFilePath() / PrivateKeyPath()) consumed by both the installer and the runtime would have made this class of drift impossible — and there are several more instances of it in install.sh below.
| installCmd.Flags().String( | ||
| "private-key", | ||
| "/etc/shellhub-agent/shellhub.key", | ||
| "Path to the agent private key file", | ||
| ) |
There was a problem hiding this comment.
Changing the default is the easy half. There is no code that looks at the legacy /etc/shellhub.key (or /opt/shellhub/shellhub.key), so an existing device upgraded without the manual steps from the PR description generates a fresh key here and re-enrols as a new pending device.
Please add a fallback: if the new path does not exist and a legacy key does, use (or migrate) the legacy one. The prose guide should be a convenience, not a prerequisite for keeping a device's identity.
| // Do not disable/stop the service here. During an upgrade over the SSH tunnel, | ||
| // stopping the service disconnects the session and causes the agent to receive | ||
| // SIGHUP, which can cause the installation to fail. The service is restarted | ||
| // at the end of the install/upgrade procedure. |
There was a problem hiding this comment.
Our code style allows exactly two kinds of comment: a doc comment on an exported declaration, and a suppression's reason. This is neither — and it's the hardest case of the rule, because it documents code that is no longer here, so nothing in the file is named by it.
The content is genuinely valuable, though: "stopping the service kills the SSH session, the agent takes SIGHUP and the install dies" is exactly the kind of reason that has nowhere in the code to live. Please move it verbatim into the commit body of d5427ac, where git blame reaches it from the line that raised the question. Right now the commit says only "Remove pre-install service disable step", so deleting this comment loses the reason entirely.
| return fmt.Errorf("failed to resolve symlinks: %w", err) | ||
| } | ||
|
|
||
| if err := os.MkdirAll(filepath.Dir(agentEnvFile), 0755); err != nil { |
There was a problem hiding this comment.
Two things on this line.
Mode. This directory holds the agent private key and a 0600 env file carrying SHELLHUB_INSTALL_KEY and the tenant ID. 0755 leaves it world-traversable and makes the per-file bits the only thing protecting those secrets. 0700 is the right mode for a root-owned secret directory.
Style. The rest of this file uses Go octal literals (0o600 at line 207, 0o644 at line 217); please match.
| if err := os.MkdirAll(filepath.Dir(agentEnvFile), 0755); err != nil { | |
| if err := os.MkdirAll(filepath.Dir(agentEnvFile), 0o700); err != nil { |
| // NOTE: Only enable the service here to maintain the SSH tunnel if one is active. | ||
| // The service will be restarted after the upgrade. |
There was a problem hiding this comment.
Same as above — not a doc comment on an exported symbol and not a suppression reason, so it doesn't survive our comment rule. The enable-without---now decision and its pairing with the restart below belongs in the commit message.
| // Finally, restart the service with the newly installed version. | ||
| if err := exec.Command("systemctl", "restart", agentServiceName).Run(); err != nil { | ||
| return fmt.Errorf("failed to restart service: %w", err) | ||
| } |
There was a problem hiding this comment.
The comment goes the way of the other two.
On the code: this restart is also a failure seam worth handling. standalone_install() in install.sh reacts to a non-zero exit from install by running $SUDO rm -f "$INSTALL_BIN" (line 428) — but by this point the unit has already been enabled and its service file written. A restart failure therefore leaves an enabled unit whose ExecStart points at a binary that was just deleted, and the next boot fails silently.
Either disable the unit on the error path here, or don't enable until the restart has succeeded.
Also worth capturing stderr from these exec.Command calls (CombinedOutput) — as written, a failure reaches the user as a bare "failed to restart service" with nothing to act on.
|
|
||
| if [ -z "$MODE" ]; then | ||
| ARGS="$ARGS -e SHELLHUB_PRIVATE_KEY=${PRIVATE_KEY:-/host/etc/shellhub.key}" | ||
| ARGS="$ARGS -e SHELLHUB_PRIVATE_KEY=${PRIVATE_KEY:-/host/etc/shellhub-agent/shellhub.key}" |
There was a problem hiding this comment.
This default moved, but the matching one 35 lines down in the same function did not — line 244 still reads:
_CKEY="${PRIVATE_KEY:-/host/etc/shellhub.key}"
enroll_agent_interactively "$WRAPPER_PATH" "${_CKEY#/host}"That is the path enroll_agent_interactively waits on. With no explicit PRIVATE_KEY, the container now writes the key to the new location while the wait loop at line 123 polls the old one, times out after 30s, and reintroduces the login-vs-daemon key race that loop exists to prevent.
(GitHub won't let me comment on line 244 — it isn't in the diff.)
|
|
||
| if [ -z "$MODE" ]; then | ||
| ARGS="$ARGS -e SHELLHUB_PRIVATE_KEY=${PRIVATE_KEY:-/host/etc/shellhub.key}" | ||
| ARGS="$ARGS -e SHELLHUB_PRIVATE_KEY=${PRIVATE_KEY:-/host/etc/shellhub-agent/shellhub.key}" |
There was a problem hiding this comment.
Same problem as the podman path: line 333 still has _CKEY="${PRIVATE_KEY:-/host/etc/shellhub.key}", so the enrollment wait polls the old location while the container writes the new one.
(Line 333 isn't in the diff, so I can't comment on it directly.)
| sudo snap set shellhub server-address="$SERVER_ADDRESS" | ||
| sudo snap set shellhub tenant-id="$TENANT_ID" | ||
| sudo snap set shellhub private-key="${PRIVATE_KEY:-/etc/shellhub.key}" | ||
| sudo snap set shellhub private-key="${PRIVATE_KEY:-/etc/shellhub-agent/shellhub.key}" |
There was a problem hiding this comment.
This one is correct, but its sibling in standalone_install() was missed — line 437 still reads:
enroll_agent_interactively "$SUDO $INSTALL_BIN" "${PRIVATE_KEY:-/etc/shellhub.key}"So the native installer writes the key to /etc/shellhub-agent/shellhub.key (per the new flag default) while enrollment waits on /etc/shellhub.key.
That is 3 of 6 defaults left stale across this file. Rather than chasing each literal, consider a single variable near the top — DEFAULT_PRIVATE_KEY=/etc/shellhub-agent/shellhub.key — that every method derives from.
(Line 437 isn't in the diff, hence the comment here.)
2 participants
What kind of change does this PR introduce?
Description:
/etc/shellhub-agent//etc/shellhub.keyand/etc/shellhub-agent.envor/opt/shellhub/shellhub.keyto/etc/shellhub-agent/shellhub.keyprior to performing installation/upgrade/etc/shellhub-agentMigration Guide *Optional*
The following guide also applies when migrating from
runc-shellhub-agentto the nativego-shellhub-agent.The following steps are only needed if you want to retain the same device unique ID in ShellHub and avoid creating a new pending request.
If the migration steps are not performed, the device will be registered as a new device, and the pending request will need to be accepted.
/etc/shellhub-agent/directoryshellhub.keyfrom/opt/shellhub-agent/shelhlub.keyor/etc/shellhub.keyto/etc/shellhub-agent/shellhub.key/opt/shellhub-agent/and/etc/shellhub.keyand/etc/shellhub-agent.env