Fix: Prevent single node auth failure from disabling global SSH temperature collection

- Removed global legacySSHDisabled flag that was triggered by any single node auth failure
- Changed disableLegacySSHOnAuthFailure to only log warnings
- Fixed potential context leak in monitor.go
- Updated tests to reflect removal of global disable logic
This commit is contained in:
courtmanr@gmail.com 2025-11-23 22:24:15 +00:00
parent 35392c1f96
commit b34e18e839
3 changed files with 13 additions and 21 deletions

View file

@ -6160,6 +6160,7 @@ func (m *Monitor) pollPVEInstance(ctx context.Context, instanceName string, clie
} }
if effectiveStatus == "online" && m.tempCollector != nil && tempMonitoringEnabled { if effectiveStatus == "online" && m.tempCollector != nil && tempMonitoringEnabled {
tempCtx, tempCancel := context.WithTimeout(ctx, 30*time.Second) // Increased to accommodate SSH operations via proxy tempCtx, tempCancel := context.WithTimeout(ctx, 30*time.Second) // Increased to accommodate SSH operations via proxy
defer tempCancel()
// Determine SSH hostname to use (most robust approach): // Determine SSH hostname to use (most robust approach):
// Prefer the resolved host for this node, with cluster overrides when available. // Prefer the resolved host for this node, with cluster overrides when available.
@ -6199,7 +6200,6 @@ func (m *Monitor) pollPVEInstance(ctx context.Context, instanceName string, clie
// Use HTTP proxy if configured for this instance, otherwise fall back to socket/SSH // Use HTTP proxy if configured for this instance, otherwise fall back to socket/SSH
temp, err := m.tempCollector.CollectTemperatureWithProxy(tempCtx, sshHost, node.Node, instanceCfg.TemperatureProxyURL, instanceCfg.TemperatureProxyToken) temp, err := m.tempCollector.CollectTemperatureWithProxy(tempCtx, sshHost, node.Node, instanceCfg.TemperatureProxyURL, instanceCfg.TemperatureProxyToken)
tempCancel()
if err == nil && temp != nil && temp.Available { if err == nil && temp != nil && temp.Available {
// Get the current CPU temperature (prefer package, fall back to max) // Get the current CPU temperature (prefer package, fall back to max)

View file

@ -45,7 +45,6 @@ type TemperatureCollector struct {
proxyCooldownUntil time.Time proxyCooldownUntil time.Time
proxyHostStates map[string]*proxyHostState proxyHostStates map[string]*proxyHostState
missingKeyWarned atomic.Bool missingKeyWarned atomic.Bool
legacySSHDisabled atomic.Bool
} }
type proxyHostState struct { type proxyHostState struct {
@ -184,10 +183,6 @@ func (tc *TemperatureCollector) CollectTemperatureWithProxy(ctx context.Context,
return &models.Temperature{Available: false}, nil return &models.Temperature{Available: false}, nil
} }
if tc.legacySSHDisabled.Load() {
return &models.Temperature{Available: false}, nil
}
if strings.TrimSpace(tc.sshKeyPath) == "" { if strings.TrimSpace(tc.sshKeyPath) == "" {
tc.logMissingSSHKey(nil) tc.logMissingSSHKey(nil)
return &models.Temperature{Available: false}, nil return &models.Temperature{Available: false}, nil
@ -347,13 +342,12 @@ func (tc *TemperatureCollector) disableLegacySSHOnAuthFailure(err error, nodeNam
return false return false
} }
if tc.legacySSHDisabled.CompareAndSwap(false, true) { // Do not disable globally on single node failure
log.Warn(). log.Warn().
Str("node", nodeName). Str("node", nodeName).
Str("host", host). Str("host", host).
Err(err). Err(err).
Msg("Disabling legacy SSH temperature collection after authentication failure; configure pulse-sensor-proxy or adjust SSH access.") Msg("SSH temperature collection failed due to authentication error; check SSH keys")
}
return true return true
} }

View file

@ -623,20 +623,18 @@ func TestDisableLegacySSHOnAuthFailure(t *testing.T) {
collector := &TemperatureCollector{} collector := &TemperatureCollector{}
if !collector.disableLegacySSHOnAuthFailure(fmt.Errorf("ssh command failed: Permission denied (publickey)."), "node-1", "host-1") { if !collector.disableLegacySSHOnAuthFailure(fmt.Errorf("ssh command failed: Permission denied (publickey)."), "node-1", "host-1") {
t.Fatalf("expected authentication errors to disable legacy SSH") t.Fatalf("expected authentication errors to be detected")
}
if !collector.legacySSHDisabled.Load() {
t.Fatalf("expected legacy SSH to be marked disabled")
} }
// legacySSHDisabled check removed as we no longer globally disable SSH
// Repeated auth errors should still return true but not change the flag. // Repeated auth errors should still return true
if !collector.disableLegacySSHOnAuthFailure(fmt.Errorf("permission denied"), "node-1", "host-1") { if !collector.disableLegacySSHOnAuthFailure(fmt.Errorf("permission denied"), "node-1", "host-1") {
t.Fatalf("expected repeated authentication errors to continue reporting disabled state") t.Fatalf("expected repeated authentication errors to be detected")
} }
// Non-authentication errors should not trigger disablement. // Non-authentication errors should not trigger detection
if collector.disableLegacySSHOnAuthFailure(fmt.Errorf("connection timed out"), "node-1", "host-1") { if collector.disableLegacySSHOnAuthFailure(fmt.Errorf("connection timed out"), "node-1", "host-1") {
t.Fatalf("expected non-authentication errors to leave legacy SSH enabled") t.Fatalf("expected non-authentication errors to be ignored")
} }
} }