From 75a240ad1c0227635fc7f4eb13f35fd7b7e0152f Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 13:15:14 +0100 Subject: [PATCH] Classify offline Proxmox node failures accurately Treat node-scoped HTTP 595 responses as debug-level resource unavailability instead of repeated authentication warnings. Preserve warnings and returned errors for real credential failures. Refs #1794. Contract-Neutral: corrects internal log severity without changing API, resource, or extension contracts Change-source: pulse-maintainer --- .../v6/internal/subsystems/monitoring.md | 10 ++++ pkg/proxmox/client.go | 49 ++++++++++++---- pkg/proxmox/client_request_test.go | 57 +++++++++++++++++++ 3 files changed, 106 insertions(+), 10 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 7bfec6107..5593de542 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -3422,3 +3422,13 @@ levels continue to route by channel, including Apprise; this compatibility path must not silently skip a supported destination. Monitoring broadcasts the escalated alert after dispatch but does not reinterpret destination identity, retry semantics, acknowledgement, or the critical-repeat cadence. + +### Proxmox node unavailability is not credential evidence + +The Proxmox client treats HTTP 595 from a node-scoped API path as a resource +availability failure. This is the pveproxy response when a cluster member is +offline or unreachable, so it remains available at debug level without +emitting the repeated authentication warning used for cluster-scoped 595, +401, and 403 responses. Returned errors retain their compatibility shape; +`pkg/proxmox/client_request_test.go` pins both sides of the log classification +(#1794). diff --git a/pkg/proxmox/client.go b/pkg/proxmox/client.go index d217d7a3a..aa0443d7b 100644 --- a/pkg/proxmox/client.go +++ b/pkg/proxmox/client.go @@ -21,6 +21,37 @@ import ( const maxResponseBodyBytes int64 = 8 << 20 // 8 MiB +type apiErrorLogLevel uint8 + +const ( + apiErrorLogNone apiErrorLogLevel = iota + apiErrorLogDebug + apiErrorLogWarn +) + +func isNodeScopedAPIPath(path string) bool { + parts := strings.Split(strings.Trim(path, "/"), "/") + for i, part := range parts { + if part == "nodes" && i+1 < len(parts) { + return true + } + } + return false +} + +func classifyAPIErrorLog(status int, path string) (apiErrorLogLevel, string) { + if status == http.StatusForbidden && strings.Contains(path, "/apt/update") { + return apiErrorLogDebug, "Proxmox permission error (optional endpoint)" + } + if status == 595 && isNodeScopedAPIPath(path) { + return apiErrorLogDebug, "Proxmox node resource unavailable" + } + if status == 595 || status == http.StatusUnauthorized || status == http.StatusForbidden { + return apiErrorLogWarn, "Proxmox authentication error" + } + return apiErrorLogNone, "" +} + func readResponseBodyLimited(r io.Reader) ([]byte, error) { body, err := io.ReadAll(io.LimitReader(r, maxResponseBodyBytes+1)) if err != nil { @@ -559,7 +590,7 @@ func (c *Client) requestWithRetry(ctx context.Context, method, path string, data } else if resp.StatusCode == 595 { // 595 can mean authentication failed OR trying to access an offline node in a cluster // Check if this is a node-specific endpoint - if strings.Contains(req.URL.Path, "/nodes/") && strings.Count(req.URL.Path, "/") > 3 { + if isNodeScopedAPIPath(req.URL.Path) { // This looks like a node-specific resource request apiErr = fmt.Errorf("API error 595: Cannot access node resource - node may be offline or credentials may be invalid") } else { @@ -571,17 +602,14 @@ func (c *Client) requestWithRetry(ctx context.Context, method, path string, data apiErr = fmt.Errorf("API error %d: %s", resp.StatusCode, string(body)) } - // Log auth issues for debugging (595 is Proxmox "no ticket" error) - if resp.StatusCode == 595 || resp.StatusCode == 401 || resp.StatusCode == 403 { - // Some endpoints are optional and may return 403 if the token is intentionally - // scoped read-only. Avoid warning-level log spam for those. + // Classify the failed operation by its expected impact. A node-scoped 595 + // is the normal pveproxy response when a cluster member is offline; the + // successful cluster request already proves the credential is usable. + if level, msg := classifyAPIErrorLog(resp.StatusCode, req.URL.Path); level != apiErrorLogNone { event := log.Warn() - msg := "Proxmox authentication error" - if resp.StatusCode == 403 && strings.Contains(req.URL.Path, "/apt/update") { + if level == apiErrorLogDebug { event = log.Debug() - msg = "Proxmox permission error (optional endpoint)" } - event. Str("url", req.URL.String()). Int("status", resp.StatusCode). @@ -591,7 +619,8 @@ func (c *Client) requestWithRetry(ctx context.Context, method, path string, data Msg(msg) } - // Wrap with appropriate error type + // Preserve the established returned-error classification for callers; + // this distinction changes only the emitted diagnostic severity. if resp.StatusCode == 401 || resp.StatusCode == 403 || resp.StatusCode == 595 { // Import errors package at top of file return nil, fmt.Errorf("authentication error: %w", apiErr) diff --git a/pkg/proxmox/client_request_test.go b/pkg/proxmox/client_request_test.go index 85d36133d..2e3edd4a4 100644 --- a/pkg/proxmox/client_request_test.go +++ b/pkg/proxmox/client_request_test.go @@ -70,6 +70,63 @@ func TestClientRequest_595NodeSpecific(t *testing.T) { } } +func TestClassifyAPIErrorLog(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + status int + path string + wantLevel apiErrorLogLevel + wantMsg string + }{ + { + name: "offline node resource is diagnostic", + status: 595, + path: "/api2/json/nodes/proxmox1/lxc/217/config", + wantLevel: apiErrorLogDebug, + wantMsg: "Proxmox node resource unavailable", + }, + { + name: "cluster scoped 595 remains authentication warning", + status: 595, + path: "/api2/json/cluster/status", + wantLevel: apiErrorLogWarn, + wantMsg: "Proxmox authentication error", + }, + { + name: "unauthorized remains authentication warning", + status: http.StatusUnauthorized, + path: "/api2/json/nodes", + wantLevel: apiErrorLogWarn, + wantMsg: "Proxmox authentication error", + }, + { + name: "optional apt permission remains diagnostic", + status: http.StatusForbidden, + path: "/api2/json/nodes/proxmox1/apt/update", + wantLevel: apiErrorLogDebug, + wantMsg: "Proxmox permission error (optional endpoint)", + }, + { + name: "server error is not logged here", + status: http.StatusInternalServerError, + path: "/api2/json/nodes", + wantLevel: apiErrorLogNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + gotLevel, gotMsg := classifyAPIErrorLog(tt.status, tt.path) + if gotLevel != tt.wantLevel || gotMsg != tt.wantMsg { + t.Fatalf("classifyAPIErrorLog(%d, %q) = (%v, %q), want (%v, %q)", tt.status, tt.path, gotLevel, gotMsg, tt.wantLevel, tt.wantMsg) + } + }) + } +} + func TestClientRequest_595Auth(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(595)