fix(snap): simplify snap hooks (#3988)

* fix(snap): simplify snap hooks

The `post-refresh` hook runs after initial snap installation as well, so
there is no need to call the `install` hook from within the
`post-refresh` hook; instead, the logic can simply be moved into the
`post-refresh` hook directly, and the `install` hook removed.

Also, the existing `install` hook logic looked for an insecure
configuration, and if found, replaced the entire configuration file with
a minimal default in the current format. But OpenShell does that default
behavior without any config file, so we may as well simply remove the
configuration file entirely to keep up-to-date with the current default
behavior. Let OpenShell create a configuration file if it needs to,
rather than auto-create one via the packaging scripts.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* fix(snap): remove the connect-plug-docker hook

The `openshell:docker` is auto-connected to the system `:docker` slot,
so there should not be a need to separately restart the gateway service
when the interface is connected.

For locally-built test snaps which were not published to the store, the
autoconnection is not made, but when the snap is installed, the gateway
will attempt to start anyway and fail to find any available compute
driver, so quickly restart until it hits the systemd start-limit, after
which systemd prevents the service from being started again. If a user
tries to manually connect their locally-built `openshell` snap to the
`:docker` slot, then the `connect-plug-docker` hook runs and triggers a
restart of the gateway, which will usually fail because the start limit
has already been hit. An error in the hook will thus cause the interface
connection to be undone, which is undesirable.

Thus, we can remove this hook entirely, and instead allow interface
connections to succeed as intended. The user still needs to manually
restart the gateway service after making a manual connection (as was the
case previously) and probably needs to `systemctl reset-failed` first,
but at least connection will succeed beforehand so they can proceed with
these steps.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* fix(snap): set refresh-mode: endure again, with manual restart

Return to the previous behavior before commit a67567e58, where the
gateway is not stopped before refreshes. The `post-refresh` hook
now restarts the gateway if the TLS configuration was corrected, so we
don't have to enforce restarting the gateway on every refresh even when
not necessary. Thus, set `refresh-mode: endure`, and let the hook decide
when the gateway needs to be restarted.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* fix(snap): update docs and tests to reflect snap hook changes

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* docs(snap): remove verbose explanation of snap gateway refresh behavior

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

---------

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

docs/about/installation.mdx

@@ -126,13 +126,15 @@ openshell status
 
 Keep the client key private.
 
-To install a locally built snap, connect its interfaces manually:
+To install a locally built snap, connect its interfaces manually. The gateway may reach systemd's start limit before Docker is connected, so reset the failed unit and restart the gateway after connecting the interfaces:
 
 ```shell
 sudo snap install ./openshell_*.snap --dangerous
 sudo snap connect openshell:log-observe
 sudo snap connect openshell:system-observe
 sudo snap connect openshell:docker :docker
+sudo systemctl reset-failed snap.openshell.gateway.service
+sudo snap restart openshell.gateway
 ```
 
 ## Kubernetes

python/openshell/release_formula_test.py

@@ -4,7 +4,6 @@
 from __future__ import annotations
 
 import re
-import stat
 import subprocess
 import sys
 from pathlib import Path
@@ -168,17 +167,6 @@ def test_snap_wrapper_uses_optional_gateway_config_without_generating_toml() ->
     assert 'exec "${SNAP}/bin/openshell-gateway" "$@"' in wrapper
 
 
-def test_snap_docker_connect_hook_restarts_gateway() -> None:
-    repo_root = Path(__file__).resolve().parents[2]
-    hook = repo_root / "snap/hooks/connect-plug-docker"
-
-    assert hook.is_file()
-    assert hook.stat().st_mode & stat.S_IXUSR
-    assert 'snapctl restart "${SNAP_INSTANCE_NAME}.gateway"' in hook.read_text(
-        encoding="utf-8"
-    )
-
-
 def test_rpm_spec_seeds_and_migrates_gateway_defaults() -> None:
     repo_root = Path(__file__).resolve().parents[2]
     spec = (repo_root / "openshell.spec").read_text(encoding="utf-8")

snap/hooks/connect-plug-docker

@@ -1,13 +0,0 @@
-#!/bin/sh
-# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
-# SPDX-License-Identifier: Apache-2.0
-
-# The gateway daemon can start when this plug is still disconnected. Restart it
-# after Docker access becomes available so driver auto-detection runs with the
-# socket exposed through the system :docker slot. This hook does not make normal
-# gateway startup conditional on Docker; it runs after an automatic or manual
-# Docker connection.
-
-set -eu
-
-snapctl restart "${SNAP_INSTANCE_NAME}.gateway"

snap/hooks/install

@@ -1,35 +0,0 @@
-#!/bin/sh
-# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
-# SPDX-License-Identifier: Apache-2.0
-
-# Create the mTLS default and replace insecure configs on refresh.
-
-set -eu
-
-config_file="${SNAP_COMMON}/gateway.toml"
-insecure='^[[:space:]]*(allow_unauthenticated_users|disable_tls)[[:space:]]*=[[:space:]]*true([[:space:]#]|$)'
-
-# Keep secure operator configs, symlinks, and directories. Replace a config
-# that explicitly allows plaintext or anonymous access, even if it has other
-# edits.
-if [ -L "$config_file" ]; then
-	exit 0
-elif [ -f "$config_file" ]; then
-	grep -Eq "$insecure" "$config_file" || exit 0
-	echo "openshell: replacing insecure gateway config with the mTLS default" >&2
-elif [ -e "$config_file" ]; then
-	exit 0
-fi
-
-mkdir -p "$SNAP_COMMON"
-umask 077
-temporary_file=$(mktemp "${config_file}.tmp.XXXXXX")
-trap 'rm -f "$temporary_file"' 0 HUP INT TERM
-cat >"$temporary_file" <<'CONFIG'
-[openshell]
-version = 2
-
-[openshell.gateway]
-CONFIG
-mv -f "$temporary_file" "$config_file"
-trap - 0 HUP INT TERM

snap/hooks/post-refresh

@@ -2,11 +2,29 @@
 # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 # SPDX-License-Identifier: Apache-2.0
 
-# Replace insecure gateway configs on refresh, then restart the gateway so the
-# new config takes effect even when the previous revision used
+# Remove insecure gateway configs on refresh, then restart the gateway so the
+# default config takes effect even when the previous revision used
 # refresh-mode: endure and kept its plaintext gateway running.
 
 set -eu
 
-"${SNAP}/meta/hooks/install"
+config_file="${SNAP_COMMON}/gateway.toml"
+insecure='^[[:space:]]*(allow_unauthenticated_users|disable_tls)[[:space:]]*=[[:space:]]*true([[:space:]#]|$)'
+
+# Keep secure operator configs, symlinks, and directories. Remove a config
+# that explicitly allows plaintext or anonymous access, even if it has other
+# edits.
+if [ ! -e "$config_file" ]; then
+	exit 0
+elif [ -L "$config_file" ]; then
+	exit 0
+elif [ -f "$config_file" ]; then
+	grep -Eq "$insecure" "$config_file" || exit 0
+	echo "openshell: removing insecure gateway config to use the mTLS default" >&2
+elif [ -e "$config_file" ]; then
+	exit 0
+fi
+
+rm -f "$config_file"
+
 snapctl restart "${SNAP_INSTANCE_NAME}.gateway"

snapcraft.yaml

@@ -88,9 +88,10 @@ apps:
   gateway:
     command: bin/openshell-gateway-wrapper
     daemon: simple
-    # Refresh must activate the migrated mTLS config immediately. This
-    # interrupts active sandbox sessions.
-    refresh-mode: restart
+    # refresh-mode: endure prevents snapd from restarting the gateway daemon
+    # during snap refreshes, which would kill active sandbox sessions.
+    # Operators must manually restart the service after a refresh if needed.
+    refresh-mode: endure
     # Snapd runs this daemon as root. The wrapper serves TLS from the bundle
     # generated in $SNAP_COMMON/tls, and the default config requires client
     # certificates; the installer copies the client bundle to the target

tasks/scripts/test-packaging-assets.sh

@@ -83,16 +83,14 @@ snapcraft="${ROOT}/snapcraft.yaml"
 snap_install_docs="${ROOT}/docs/about/installation.mdx"
 snap_canary="${ROOT}/.github/workflows/release-canary.yml"
 snap_repro="${ROOT}/nix/test-guest/scripts/snap-gateway-repro.sh"
-snap_docker_hook="${ROOT}/snap/hooks/connect-plug-docker"
-snap_install_hook="${ROOT}/snap/hooks/install"
+snap_post_refresh_hook="${ROOT}/snap/hooks/post-refresh"
 package_deb="${ROOT}/tasks/scripts/package-deb.sh"
 assert_file_exists "$snap_wrapper"
 assert_file_exists "$snapcraft"
 assert_file_exists "$snap_install_docs"
 assert_file_exists "$snap_canary"
 assert_file_exists "$snap_repro"
-assert_file_exists "$snap_docker_hook"
-assert_file_exists "$snap_install_hook"
+assert_file_exists "$snap_post_refresh_hook"
 assert_file_exists "$package_deb"
 assert_contains "$service" "ExecStartPre=/usr/bin/openshell-gateway config preflight"
 assert_contains "$package_deb" "\$src_dir/openshell-gateway.service"
@@ -114,28 +112,33 @@ for snap_file in \
   "$snap_install_docs" \
   "$snap_canary" \
   "$snap_repro" \
-  "$snap_docker_hook" \
-  "$snap_install_hook"; do
+  "$snap_post_refresh_hook"; do
   assert_not_contains "$snap_file" "docker:docker-daemon"
   assert_not_contains "$snap_file" "default-provider: docker"
 done
-if [[ ! -x "$snap_install_hook" ]]; then
-  echo "FAIL: Snap install hook must be executable" >&2
+if [[ -e "${ROOT}/snap/hooks/connect-plug-docker" ]]; then
+  echo "FAIL: obsolete Snap Docker connection hook must not exist" >&2
   exit 1
 fi
-assert_not_contains "$snap_install_hook" 'compute_driver'
-assert_not_contains "$snap_install_hook" 'allow_unauthenticated_users = true'
-assert_contains "$snapcraft" 'refresh-mode: restart'
-if [[ ! -x "$(dirname "$snap_install_hook")/post-refresh" ]]; then
+if [[ -e "${ROOT}/snap/hooks/install" ]]; then
+  echo "FAIL: obsolete Snap install hook must not exist" >&2
+  exit 1
+fi
+assert_contains "$snapcraft" 'refresh-mode: endure'
+if [[ ! -x "$snap_post_refresh_hook" ]]; then
   echo "FAIL: Snap post-refresh hook must be executable" >&2
   exit 1
 fi
 assert_not_contains "$ROOT/tasks/scripts/snap-gateway-wrapper.sh" 'OPENSHELL_DISABLE_TLS'
-bash "$ROOT/tasks/scripts/test-snap-install-hook.sh" "$snap_install_hook"
+bash "$ROOT/tasks/scripts/test-snap-post-refresh-hook.sh" "$snap_post_refresh_hook"
 assert_not_contains "$snap_install_docs" "snap connect openshell:home"
 assert_not_contains "$snap_install_docs" "snap connect openshell:network"
 assert_not_contains "$snap_install_docs" "snap connect openshell:network-bind"
 assert_contains "$snap_install_docs" "snap connect openshell:docker :docker"
+assert_contains "$snap_install_docs" "systemctl reset-failed snap.openshell.gateway.service"
+assert_contains "$snap_install_docs" "snap restart openshell.gateway"
+assert_contains "$snap_install_docs" "Snap refreshes keep the running gateway process active"
+assert_contains "$snap_install_docs" "install script refreshes and restarts the gateway automatically"
 assert_contains "$snap_canary" "install.sh | sh"
 assert_contains "$snap_canary" "ubuntu-snap-system-docker:"
 assert_contains "$snap_canary" "ubuntu-snap-docker-preflight:"

tasks/scripts/test-snap-install-hook.sh

@@ -1,142 +0,0 @@
-#!/usr/bin/env bash
-# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
-# SPDX-License-Identifier: Apache-2.0
-
-set -euo pipefail
-
-hook_input=${1:?Usage: test-snap-install-hook.sh <install-hook>}
-hook_dir=$(cd "$(dirname "$hook_input")" && pwd)
-hook="${hook_dir}/$(basename "$hook_input")"
-work=$(mktemp -d "${TMPDIR:-/tmp}/openshell snap install hook.XXXXXX")
-trap 'rm -rf "$work"' EXIT
-
-expected="${work}/expected.toml"
-cat >"$expected" <<'EOF'
-[openshell]
-version = 2
-
-[openshell.gateway]
-EOF
-
-legacy="${work}/legacy.toml"
-cat >"$legacy" <<'EOF'
-[openshell]
-version = 2
-
-[openshell.gateway]
-
-[openshell.gateway.auth]
-allow_unauthenticated_users = true
-EOF
-
-common="${work}/fresh"
-SNAP_COMMON="$common" "$hook"
-cmp -s "$expected" "$common/gateway.toml"
-if [[ -z $(find "$common/gateway.toml" -perm 600) ]]; then
-  echo "FAIL: install hook config must be mode 0600" >&2
-  exit 1
-fi
-
-printf '\noperator setting = true\n' >>"$common/gateway.toml"
-cp "$common/gateway.toml" "${work}/operator-before"
-SNAP_COMMON="$common" "$hook"
-cmp -s "${work}/operator-before" "$common/gateway.toml"
-
-common="${work}/legacy"
-mkdir -p "$common"
-cp "$legacy" "$common/gateway.toml"
-chmod 644 "$common/gateway.toml"
-SNAP_COMMON="$common" "$hook"
-if ! cmp -s "$expected" "$common/gateway.toml"; then
-  echo "FAIL: install hook must migrate the legacy unauthenticated config" >&2
-  exit 1
-fi
-if [[ -z $(find "$common/gateway.toml" -perm 600) ]]; then
-  echo "FAIL: migrated config must be mode 0600" >&2
-  exit 1
-fi
-
-common="${work}/legacy-edited"
-mkdir -p "$common"
-cp "$legacy" "$common/gateway.toml"
-printf '\n# operator note\n' >>"$common/gateway.toml"
-cp "$common/gateway.toml" "${work}/legacy-edited-before"
-SNAP_COMMON="$common" "$hook"
-cmp -s "$expected" "$common/gateway.toml"
-
-common="${work}/custom-insecure"
-mkdir -p "$common"
-cat >"$common/gateway.toml" <<'EOF'
-[openshell]
-version = 2
-
-[openshell.gateway]
-compute_driver = "docker"
-disable_tls = true # old local override
-
-[openshell.gateway.auth]
-allow_unauthenticated_users = true # old local override
-EOF
-cp "$common/gateway.toml" "${work}/custom-insecure-before"
-SNAP_COMMON="$common" "$hook"
-cmp -s "$expected" "$common/gateway.toml"
-
-common="${work}/custom-secure"
-mkdir -p "$common"
-cat >"$common/gateway.toml" <<'EOF'
-[openshell]
-version = 2
-
-[openshell.gateway]
-compute_driver = "docker"
-# allow_unauthenticated_users = true
-EOF
-cp "$common/gateway.toml" "${work}/custom-secure-before"
-SNAP_COMMON="$common" "$hook"
-cmp -s "${work}/custom-secure-before" "$common/gateway.toml"
-
-common="${work}/post-refresh"
-mkdir -p "$common" "${work}/snap/meta/hooks"
-cp "$hook" "${work}/snap/meta/hooks/install"
-cp "${work}/legacy-edited-before" "$common/gateway.toml"
-mkdir -p "${work}/bin"
-cat >"${work}/bin/snapctl" <<EOF
-#!/bin/sh
-printf '%s\\n' "\$*" >>"${work}/snapctl.log"
-EOF
-chmod 755 "${work}/bin/snapctl"
-PATH="${work}/bin:$PATH" SNAP="${work}/snap" SNAP_COMMON="$common" \
-  SNAP_INSTANCE_NAME=openshell "${hook_dir}/post-refresh"
-if ! cmp -s "$expected" "$common/gateway.toml"; then
-  echo "FAIL: post-refresh hook must migrate an edited insecure config" >&2
-  exit 1
-fi
-if [[ $(cat "${work}/snapctl.log") != "restart openshell.gateway" ]]; then
-  echo "FAIL: post-refresh hook must restart the gateway" >&2
-  cat "${work}/snapctl.log" >&2
-  exit 1
-fi
-
-common="${work}/broken-link"
-mkdir -p "$common"
-ln -s "${work}/missing-target" "$common/gateway.toml"
-SNAP_COMMON="$common" "$hook"
-if [[ $(readlink "$common/gateway.toml") != "${work}/missing-target" ]]; then
-  echo "FAIL: install hook replaced a broken operator symlink" >&2
-  exit 1
-fi
-
-common="${work}/directory"
-mkdir -p "$common/gateway.toml"
-SNAP_COMMON="$common" "$hook"
-if [[ ! -d "$common/gateway.toml" ]]; then
-  echo "FAIL: install hook replaced an operator-owned directory" >&2
-  exit 1
-fi
-
-if [[ -n $(find "$work" -name 'gateway.toml.pre-mtls*') ]]; then
-  echo "FAIL: install hook must not keep copies of replaced configs" >&2
-  exit 1
-fi
-
-echo "Snap install hook tests passed"

tasks/scripts/test-snap-post-refresh-hook.sh

@@ -0,0 +1,147 @@
+#!/usr/bin/env bash
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+set -euo pipefail
+
+hook_input=${1:?Usage: test-snap-post-refresh-hook.sh <post-refresh-hook>}
+hook_dir=$(cd "$(dirname "$hook_input")" && pwd)
+hook="${hook_dir}/$(basename "$hook_input")"
+work=$(mktemp -d "${TMPDIR:-/tmp}/openshell snap post-refresh hook.XXXXXX")
+trap 'rm -rf "$work"' EXIT
+
+mkdir -p "${work}/bin"
+cat >"${work}/bin/snapctl" <<EOF
+#!/bin/sh
+printf '%s\\n' "\$*" >>"${work}/snapctl.log"
+EOF
+chmod 755 "${work}/bin/snapctl"
+
+run_hook() {
+  local common=$1
+
+  PATH="${work}/bin:$PATH" SNAP_COMMON="$common" SNAP_INSTANCE_NAME=openshell "$hook"
+}
+
+assert_no_restart() {
+  local name=$1
+  local common=$2
+
+  rm -f "${work}/snapctl.log"
+  run_hook "$common"
+  if [[ -e "${work}/snapctl.log" ]]; then
+    echo "FAIL: post-refresh hook restarted the gateway for ${name}" >&2
+    cat "${work}/snapctl.log" >&2
+    exit 1
+  fi
+}
+
+assert_removed_and_restarted() {
+  local name=$1
+  local common=$2
+
+  rm -f "${work}/snapctl.log"
+  run_hook "$common"
+  if [[ -e "$common/gateway.toml" ]] || [[ -L "$common/gateway.toml" ]]; then
+    echo "FAIL: post-refresh hook did not remove ${name}" >&2
+    exit 1
+  fi
+  if [[ $(cat "${work}/snapctl.log") != "restart openshell.gateway" ]]; then
+    echo "FAIL: post-refresh hook did not restart the gateway once for ${name}" >&2
+    cat "${work}/snapctl.log" >&2
+    exit 1
+  fi
+}
+
+common="${work}/missing"
+mkdir -p "$common"
+assert_no_restart "a missing config" "$common"
+if [[ -e "$common/gateway.toml" ]] || [[ -L "$common/gateway.toml" ]]; then
+  echo "FAIL: post-refresh hook created a missing config" >&2
+  exit 1
+fi
+
+common="${work}/secure"
+mkdir -p "$common"
+cat >"$common/gateway.toml" <<'EOF'
+[openshell]
+version = 2
+
+[openshell.gateway]
+compute_driver = "docker"
+disable_tls = false
+
+[openshell.gateway.auth]
+allow_unauthenticated_users = false
+# allow_unauthenticated_users = true
+EOF
+cp "$common/gateway.toml" "${work}/secure-before"
+assert_no_restart "a secure config" "$common"
+cmp -s "${work}/secure-before" "$common/gateway.toml"
+
+common="${work}/unauthenticated"
+mkdir -p "$common"
+cat >"$common/gateway.toml" <<'EOF'
+[openshell.gateway.auth]
+allow_unauthenticated_users = true
+EOF
+assert_removed_and_restarted "an unauthenticated config" "$common"
+
+common="${work}/tls-disabled"
+mkdir -p "$common"
+cat >"$common/gateway.toml" <<'EOF'
+[openshell.gateway]
+disable_tls = true # old local override
+
+# operator note
+EOF
+assert_removed_and_restarted "an edited TLS-disabled config" "$common"
+
+common="${work}/broken-link"
+mkdir -p "$common"
+ln -s "${work}/missing-target" "$common/gateway.toml"
+assert_no_restart "a broken operator symlink" "$common"
+if [[ $(readlink "$common/gateway.toml") != "${work}/missing-target" ]]; then
+  echo "FAIL: post-refresh hook replaced a broken operator symlink" >&2
+  exit 1
+fi
+
+common="${work}/existing-link"
+mkdir -p "$common"
+printf '%s\n' 'disable_tls = true' >"${work}/linked-config.toml"
+ln -s "${work}/linked-config.toml" "$common/gateway.toml"
+assert_no_restart "an existing operator symlink" "$common"
+if [[ $(readlink "$common/gateway.toml") != "${work}/linked-config.toml" ]]; then
+  echo "FAIL: post-refresh hook replaced an existing operator symlink" >&2
+  exit 1
+fi
+
+common="${work}/directory"
+mkdir -p "$common/gateway.toml"
+assert_no_restart "an operator-owned directory" "$common"
+if [[ ! -d "$common/gateway.toml" ]]; then
+  echo "FAIL: post-refresh hook replaced an operator-owned directory" >&2
+  exit 1
+fi
+
+common="${work}/remove-failure"
+mkdir -p "$common" "${work}/failing-bin"
+printf '%s\n' 'disable_tls = true' >"$common/gateway.toml"
+cat >"${work}/failing-bin/rm" <<'EOF'
+#!/bin/sh
+exit 1
+EOF
+chmod 755 "${work}/failing-bin/rm"
+rm -f "${work}/snapctl.log"
+if PATH="${work}/failing-bin:${work}/bin:$PATH" SNAP_COMMON="$common" \
+  SNAP_INSTANCE_NAME=openshell "$hook"; then
+  echo "FAIL: post-refresh hook succeeded when config removal failed" >&2
+  exit 1
+fi
+if [[ -e "${work}/snapctl.log" ]]; then
+  echo "FAIL: post-refresh hook restarted after config removal failed" >&2
+  cat "${work}/snapctl.log" >&2
+  exit 1
+fi
+
+echo "Snap post-refresh hook tests passed"