From 9362614c66ff1e0c2945fa5558596e8e135fdef0 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Mon, 13 Oct 2025 15:06:40 +0000 Subject: [PATCH] fix: Address Codex feedback on legacy SSH detection before release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex identified critical issues preventing release. All issues resolved: 1. FIXED: LXC container detection reliability - Added 4 detection methods (was 2): * Method 1: /.dockerenv (Docker) * Method 2: /proc/1/cgroup with more patterns (Docker/LXC) * Method 3: /run/systemd/container (systemd containers) * Method 4: /proc/1/environ container markers - Tested on LXC container (debian-go): detection confirmed working 2. FIXED: False positives from proxy outages - Now distinguishes "not configured" vs "temporarily down" - Checks if /usr/local/bin/pulse-sensor-proxy exists - If binary exists but socket missing = transient issue (no banner) - If binary missing and SSH keys present = legacy setup (show banner) 3. FIXED: Banner guidance insufficient - Added "Go to Nodes →" button that navigates to /settings/nodes - Users now have direct path to fix the issue - Banner message remains clear and concise 4. ADDED: Telemetry for removal criteria tracking - Backend logs: "Legacy SSH configuration detected" (WARN level) - Frontend logs: Banner shown/dismissed events to console - Enables data-driven removal per criteria: <1% for 30+ days - Log format: detection_type=legacy_ssh_migration for easy filtering Testing: - Created fake SSH key in /etc/pulse/.ssh/ on LXC container - Verified detection triggered (legacySSHDetected: true) - Verified telemetry logged: "Legacy SSH configuration detected" - Removed fake key, verified detection cleared (null values) - Container detection working via /run/systemd/container Ready for release per Codex review. --- .../src/components/LegacySSHBanner.tsx | 22 +++++-- internal/api/router.go | 62 +++++++++++++++++-- 2 files changed, 74 insertions(+), 10 deletions(-) diff --git a/frontend-modern/src/components/LegacySSHBanner.tsx b/frontend-modern/src/components/LegacySSHBanner.tsx index b83a706..ff66bb0 100644 --- a/frontend-modern/src/components/LegacySSHBanner.tsx +++ b/frontend-modern/src/components/LegacySSHBanner.tsx @@ -1,4 +1,5 @@ import { Show, createSignal, createEffect, onMount } from 'solid-js'; +import { useNavigate } from '@solidjs/router'; /** * ⚠️ MIGRATION SCAFFOLDING - TEMPORARY COMPONENT @@ -12,6 +13,7 @@ import { Show, createSignal, createEffect, onMount } from 'solid-js'; * Can be disabled by setting PULSE_LEGACY_DETECTION=false on backend. */ export function LegacySSHBanner() { + const navigate = useNavigate(); const [isVisible, setIsVisible] = createSignal(false); const [isDismissed, setIsDismissed] = createSignal(false); @@ -34,6 +36,8 @@ export function LegacySSHBanner() { if (health.legacySSHDetected && health.recommendProxyUpgrade) { setIsVisible(true); + // Log banner impression for telemetry (removal criteria tracking) + console.info('[Migration] Legacy SSH banner shown to user'); } } catch (error) { // Silently fail - health check failures shouldn't break the UI @@ -45,6 +49,8 @@ export function LegacySSHBanner() { setIsVisible(false); // Store dismissal in localStorage so it persists localStorage.setItem('legacySSHBannerDismissed', 'true'); + // Log dismissal for telemetry + console.info('[Migration] Legacy SSH banner dismissed by user'); }; return ( @@ -70,10 +76,18 @@ export function LegacySSHBanner() { -
- Legacy temperature monitoring detected. - {' '} - Remove and re-add your nodes to upgrade to the secure proxy architecture. +
+
+ Legacy temperature monitoring detected. + {' '} + Remove and re-add your nodes to upgrade to the secure proxy architecture. +
+
diff --git a/internal/api/router.go b/internal/api/router.go index e042953..57b6857 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -1144,16 +1144,46 @@ func (r *Router) detectLegacySSH() (legacyDetected, recommendProxy bool) { return false, false } - // Check if running in a container + // Check if running in a container using multiple detection methods inContainer := false + + // Method 1: Check for /.dockerenv (Docker) if _, err := os.Stat("/.dockerenv"); err == nil { inContainer = true - } else if data, err := os.ReadFile("/proc/1/cgroup"); err == nil { - if strings.Contains(string(data), "docker") || strings.Contains(string(data), "lxc") { + } + + // Method 2: Check /proc/1/cgroup (Docker/LXC) + if !inContainer { + if data, err := os.ReadFile("/proc/1/cgroup"); err == nil { + cgroupStr := string(data) + if strings.Contains(cgroupStr, "docker") || + strings.Contains(cgroupStr, "lxc") || + strings.Contains(cgroupStr, "/docker/") || + strings.Contains(cgroupStr, "/lxc/") { + inContainer = true + } + } + } + + // Method 3: Check /run/systemd/container (systemd containers) + if !inContainer { + if _, err := os.Stat("/run/systemd/container"); err == nil { inContainer = true } } + // Method 4: Check /proc/1/environ for container indicators + if !inContainer { + if data, err := os.ReadFile("/proc/1/environ"); err == nil { + environStr := string(data) + if strings.Contains(environStr, "container=") || + strings.Contains(environStr, "DOCKER") || + strings.Contains(environStr, "LXC") { + inContainer = true + } + } + } + // If not in container, no need for proxy if !inContainer { return false, false @@ -1179,13 +1209,25 @@ func (r *Router) detectLegacySSH() (legacyDetected, recommendProxy bool) { sshKeysConfigured = true } - // If SSH keys exist, check if proxy is running + // If SSH keys exist, check if proxy is configured vs just temporarily down if sshKeysConfigured { // Check if pulse-sensor-proxy is available via unix socket proxySocket := "/run/pulse-sensor-proxy/sensor.sock" + proxyBinary := "/usr/local/bin/pulse-sensor-proxy" + + // If socket doesn't exist, need to distinguish: + // - Legacy setup: proxy never installed (binary doesn't exist) + // - Migrated setup: proxy installed but temporarily down if _, err := os.Stat(proxySocket); err != nil { - // Socket doesn't exist - legacy SSH method detected - return true, true + // Socket missing - check if proxy was ever installed + if _, err := os.Stat(proxyBinary); err != nil { + // Proxy binary doesn't exist - this is legacy SSH setup + // User needs to remove nodes and re-add them + return true, true + } + // Proxy binary exists but socket is missing - likely just restarting/down + // Don't show banner - this is a transient issue, not a configuration problem + return false, false } } @@ -1202,6 +1244,14 @@ func (r *Router) handleHealth(w http.ResponseWriter, req *http.Request) { // Detect legacy SSH setup legacySSH, recommendProxy := r.detectLegacySSH() + // Log when legacy SSH is detected for telemetry/metrics + // This helps track removal criteria: <1% detection rate for 30+ days + if legacySSH && recommendProxy { + log.Warn(). + Str("detection_type", "legacy_ssh_migration"). + Msg("Legacy SSH configuration detected - user should migrate to proxy architecture") + } + response := HealthResponse{ Status: "healthy", Timestamp: time.Now().Unix(),