vrr/Pulse
2
0
Fork 0
mirror of https://github.com/rcourtman/Pulse.git synced 2026-10-19 02:28:59 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 13

fix(pbs): skip superuser-only node lookup for tokens

Browse source
This commit is contained in:
rcourtman 2026-08-09 12:25:07 +01:00
parent 2a95ae35c3
commit 70f7ca9f6f
6 changed files with 157 additions and 169 deletions
Download patch file Download diff file Expand all files Collapse all files

46
docs/release-control/v6/internal/subsystems/monitoring.md
View file

@ -860,11 +860,16 @@ only graft them onto fixture data.
20. Add or change PBS API transport, optional node identity collection, or PBS
HTTP retry classification through `pkg/pbs/client.go`. HTTP status decisions
must use the concrete client error status rather than rendered error or body
text. Node identity permission suppression is limited to 401 and 403;
429, 5xx, decoding, cancellation, timeout, and network failures remain
transient and retry on the next polling call. Concurrent callers must share
one in-flight `/nodes` request so immediate retry does not create a request
storm.
text. PBS exposes the compatibility `/nodes` node-name endpoint only to a
direct `root@pam` session: API tokens, including tokens owned by root, and
non-superuser password sessions must return the typed unavailable-for-auth
result without issuing a request. Callers must treat that result as absent
optional identity evidence without repetitive failure logging, not as a
reason to request broader credentials.
For an eligible direct-root session, 401, 403, 429, 5xx, decoding,
cancellation, timeout, and network failures remain transient and retry on
the next polling call. Concurrent callers must share one in-flight `/nodes`
request so immediate retry does not create a request storm.
## Forbidden Paths
@ -985,10 +990,12 @@ only graft them onto fixture data.
`TestCrashedTrueNASAppContainerStillRaisesIncident` and
`TestStoppedTrueNASAppContainerStateStaysExited` in
`internal/truenas/provider_oneshot_containers_test.go`.
15. Keep PBS client HTTP error status structural and the node-name retry policy
explicit. Package proof must cover 401/403 deferral, 429/5xx and network
retry, response bodies containing permission-like text, recovery, caching,
and concurrent single-flight behavior under the race detector.
15. Keep PBS client HTTP error status structural and the node-name authentication
boundary explicit. Package proof must cover zero `/nodes` I/O for API-token
and non-superuser sessions, direct-root 401/403, 429/5xx, and network retry,
response bodies containing permission-like text, recovery, caching, and
concurrent single-flight behavior under the race detector. Monitoring proof
must cover a healthy API-token poll without a `/nodes` request.
## Current State
@ -1045,14 +1052,19 @@ captures the hostname the PBS node reports about itself (`GET /nodes`) on
`models.PBSInstance.NodeName` as machine-identity evidence for connected-system
grouping; node-name fetch failure is partial data like the other optional
collections, never a poll failure. `pkg/pbs/client.go` preserves HTTP status in
its concrete API error. Only 401 and 403 defer another node-name request for 30
minutes; 429, 5xx, malformed responses, cancellation, timeout, and network
errors retry on the next call regardless of error-body wording. A successful
node name remains cached for the client lifetime, and concurrent callers join
one in-flight request so a transient response produces one bounded request per
polling wave rather than one request per caller. The `GetNodeName` tests in
`pkg/pbs/client_http_test.go` are the focused retry, recovery, cache, and race
proof.
its concrete API error. Because PBS restricts the compatibility `/nodes`
endpoint to direct superuser sessions, API-token and non-superuser password
clients return typed unavailable-for-auth evidence without network I/O; Pulse
does not ask operators to broaden credentials for this optional grouping hint.
For an eligible direct `root@pam` session, 401, 403, 429, 5xx, malformed
responses, cancellation, timeout, and network errors retry on the next call
regardless of error-body wording. A successful node name remains cached for the
client lifetime, and concurrent callers join one in-flight request so a
transient response produces one bounded request per polling wave rather than
one request per caller. The `GetNodeName` tests in
`pkg/pbs/client_http_test.go` are the focused authentication-boundary, retry,
recovery, cache, and race proof; monitoring coverage also proves a token poll
never requests `/nodes`.
### Host snapshots carry integration provenance; doctor copy is user-facing

9
internal/monitoring/monitor_pbs_coverage_test.go
View file

@ -98,6 +98,9 @@ func TestPollPBSInstanceDoesNotQueryExcludedDatastoreDetails(t *testing.T) {
mu.Lock()
defer mu.Unlock()
for _, path := range requestedPaths {
if path == "/api2/json/nodes" {
t.Fatalf("API-token poll queried the superuser-only node-name endpoint: %s", path)
}
if strings.Contains(path, "exthdd1500gb") {
t.Fatalf("excluded datastore received a detail request: %s", path)
}
@ -128,11 +131,7 @@ func TestPollPBSInstanceCapturesReportedNodeName(t *testing.T) {
}))
defer server.Close()
client, err := pbs.NewClient(pbs.ClientConfig{
Host: server.URL,
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
})
client, err := pbs.NewClient(pbs.ClientConfig{Host: server.URL, User: "root@pam"})
if err != nil {
t.Fatalf("NewClient failed: %v", err)
}

3
internal/monitoring/monitor_pbs_pmg.go
View file

@ -2,6 +2,7 @@ package monitoring
import (
"context"
stderrors "errors"
"fmt"
"strings"
"time"
@ -324,7 +325,7 @@ func (m *Monitor) pollPBSInstance(ctx context.Context, instanceName string, clie
// address is an IP or DNS alias the agent never reports.
if nodeName, nodeNameErr := client.GetNodeName(ctx); nodeNameErr == nil {
pbsInst.NodeName = strings.TrimSpace(nodeName)
} else if debugEnabled {
} else if !stderrors.Is(nodeNameErr, pbs.ErrNodeNameUnavailableForAuth) && debugEnabled {
log.Debug().Err(nodeNameErr).Str("instance", instanceName).Msg("could not get PBS node name")
}

47
pkg/pbs/client.go
View file

@ -40,12 +40,11 @@ type Client struct {
config ClientConfig
// GetNodeName caching. The node hostname is stable for the lifetime of a
// connection, and polling /nodes every cycle makes PBS log a 403 every few
// seconds when the token lacks permission (refs #1691).
nodeNameMu sync.Mutex
cachedNodeName string
nodeNameRetryAfter time.Time
nodeNameAttempt *nodeNameAttempt
// connection. PBS exposes /nodes as a superuser-only compatibility endpoint,
// so GetNodeName rejects API-token and non-superuser sessions before I/O.
nodeNameMu sync.Mutex
cachedNodeName string
nodeNameAttempt *nodeNameAttempt
}
type nodeNameAttempt struct {
@ -55,10 +54,11 @@ type nodeNameAttempt struct {
err error
}
// nodeNamePermissionRetryInterval throttles /nodes retries after a permission
// denial. Long enough to stop per-cycle 403 spam in the PBS syslog, short
// enough to self-heal once an operator widens the token's ACL.
const nodeNamePermissionRetryInterval = 30 * time.Minute
// ErrNodeNameUnavailableForAuth means the current PBS authentication mode
// cannot access the superuser-only /nodes compatibility endpoint. Callers
// should treat this as absent optional identity evidence, not as a connection
// or permission failure that the operator can repair by widening an API token.
var ErrNodeNameUnavailableForAuth = errors.New("PBS node name is unavailable for this authentication mode")
func (c *Client) apiBaseURL() (*url.URL, error) {
if c.baseAPIURL != nil {
@ -658,23 +658,23 @@ func (c *Client) GetVersion(ctx context.Context) (*Version, error) {
return &result.Data, nil
}
// GetNodeName returns the PBS node's hostname. The result is cached for the
// lifetime of the client: the hostname cannot change underneath an open
// connection, and re-fetching /nodes on every poll cycle floods the PBS syslog
// with 403s when the token cannot read it (refs #1691). Permission denials
// defer the next attempt by nodeNamePermissionRetryInterval; transient
// failures retry on the next call.
// GetNodeName returns the PBS node's hostname. PBS exposes /nodes as a
// superuser-only compatibility endpoint, so only a direct root@pam session may
// perform the request. API tokens and non-superuser sessions return
// ErrNodeNameUnavailableForAuth without network I/O (refs #1691). Successful
// results are cached for the client lifetime, transient failures retry on the
// next call, and concurrent callers share one in-flight request.
func (c *Client) GetNodeName(ctx context.Context) (string, error) {
if !c.canReadNodeName() {
return "", ErrNodeNameUnavailableForAuth
}
c.nodeNameMu.Lock()
if c.cachedNodeName != "" {
name := c.cachedNodeName
c.nodeNameMu.Unlock()
return name, nil
}
if !c.nodeNameRetryAfter.IsZero() && time.Now().Before(c.nodeNameRetryAfter) {
c.nodeNameMu.Unlock()
return "", fmt.Errorf("node name unavailable: /nodes permission denied, retry deferred")
}
if attempt := c.nodeNameAttempt; attempt != nil {
attempt.joined++
c.nodeNameMu.Unlock()
@ -696,9 +696,6 @@ func (c *Client) GetNodeName(ctx context.Context) (string, error) {
attempt.err = err
if err == nil {
c.cachedNodeName = name
c.nodeNameRetryAfter = time.Time{}
} else if isPBSPermissionError(err) {
c.nodeNameRetryAfter = time.Now().Add(nodeNamePermissionRetryInterval)
}
c.nodeNameAttempt = nil
close(attempt.done)
@ -707,6 +704,10 @@ func (c *Client) GetNodeName(ctx context.Context) (string, error) {
return name, err
}
func (c *Client) canReadNodeName() bool {
return c.auth.tokenName == "" && c.auth.user == "root" && c.auth.realm == "pam"
}
func (c *Client) fetchNodeName(ctx context.Context) (string, error) {
log.Debug().Msg("PBS GetNodeName: fetching node name")

24
pkg/pbs/client_full_coverage_test.go
View file

@ -383,7 +383,7 @@ func TestClient_GetNodeStatus_JsonError(t *testing.T) {
}
func TestClient_GetNodeName(t *testing.T) {
client, server := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/api2/json/nodes" {
http.Error(w, "bad path", http.StatusNotFound)
return
@ -393,8 +393,9 @@ func TestClient_GetNodeName(t *testing.T) {
{"node": "pve1"},
},
})
})
}))
defer server.Close()
client := newRootNodeNameClient(t, server.URL)
name, err := client.GetNodeName(context.Background())
if err != nil {
@ -406,12 +407,13 @@ func TestClient_GetNodeName(t *testing.T) {
}
func TestClient_GetNodeName_Empty(t *testing.T) {
client, server := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
json.NewEncoder(w).Encode(map[string]any{
"data": []map[string]string{},
})
})
}))
defer server.Close()
client := newRootNodeNameClient(t, server.URL)
_, err := client.GetNodeName(context.Background())
if err == nil {
@ -1143,7 +1145,9 @@ func TestClient_ListMethods_NetworkError(t *testing.T) {
if _, err := client.ListBackupSnapshots(context.Background(), "s", "", "vm", "1"); err == nil {
t.Error("ListBackupSnapshots: expected network error")
}
if _, err := client.GetNodeName(context.Background()); err == nil {
nodeNameClient := newRootNodeNameClient(t, "http://example.com")
nodeNameClient.httpClient = &http.Client{Transport: &errorTransport{}}
if _, err := nodeNameClient.GetNodeName(context.Background()); err == nil {
t.Error("GetNodeName: expected network error")
}
}
@ -1505,19 +1509,21 @@ func TestListMethods_Errors(t *testing.T) {
}
func TestGetNodeName_Errors(t *testing.T) {
client1, server1 := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
server1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
http.Error(w, "server error", http.StatusInternalServerError)
})
}))
defer server1.Close()
client1 := newRootNodeNameClient(t, server1.URL)
_, err := client1.GetNodeName(context.Background())
if err == nil {
t.Error("Expected server error for GetNodeName, got nil")
}
client2, server2 := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
server2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte("invalid json"))
})
}))
defer server2.Close()
client2 := newRootNodeNameClient(t, server2.URL)
_, err = client2.GetNodeName(context.Background())
if err == nil {
t.Error("Expected JSON error for GetNodeName, got nil")

197
pkg/pbs/client_http_test.go
View file

@ -22,6 +22,19 @@ func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {
return f(req)
}
func newRootNodeNameClient(t *testing.T, host string) *Client {
t.Helper()
client, err := NewClient(ClientConfig{
Host: host,
User: "root@pam",
Timeout: 2 * time.Second,
})
if err != nil {
t.Fatalf("NewClient: %v", err)
}
return client
}
func TestNewClient_TokenAuth_SetsAuthorizationHeader(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/api2/json/version" {
@ -449,15 +462,7 @@ func TestClient_GetNodeName_CachesResult(t *testing.T) {
}))
defer server.Close()
client, err := NewClient(ClientConfig{
Host: server.URL,
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
Timeout: 2 * time.Second,
})
if err != nil {
t.Fatalf("NewClient: %v", err)
}
client := newRootNodeNameClient(t, server.URL)
for i := 0; i < 3; i++ {
name, err := client.GetNodeName(context.Background())
@ -473,7 +478,7 @@ func TestClient_GetNodeName_CachesResult(t *testing.T) {
}
}
func TestClient_GetNodeName_PermissionDenialDefersRetry(t *testing.T) {
func TestClient_GetNodeName_SuperuserPermissionFailureRetries(t *testing.T) {
for _, status := range []int{http.StatusUnauthorized, http.StatusForbidden} {
t.Run(http.StatusText(status), func(t *testing.T) {
var nodesCalls atomic.Int64
@ -481,21 +486,18 @@ func TestClient_GetNodeName_PermissionDenialDefersRetry(t *testing.T) {
if r.URL.Path != "/api2/json/nodes" {
t.Fatalf("unexpected path: %s", r.URL.Path)
}
nodesCalls.Add(1)
w.WriteHeader(status)
_, _ = w.Write([]byte(`{"message":"access denied"}`))
if nodesCalls.Add(1) == 1 {
w.WriteHeader(status)
_, _ = w.Write([]byte(`{"message":"access denied"}`))
return
}
_ = json.NewEncoder(w).Encode(map[string]any{
"data": []map[string]any{{"node": "pbs-recovered"}},
})
}))
defer server.Close()
client, err := NewClient(ClientConfig{
Host: server.URL,
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
Timeout: 2 * time.Second,
})
if err != nil {
t.Fatalf("NewClient: %v", err)
}
client := newRootNodeNameClient(t, server.URL)
_, firstErr := client.GetNodeName(context.Background())
if firstErr == nil {
@ -504,13 +506,15 @@ func TestClient_GetNodeName_PermissionDenialDefersRetry(t *testing.T) {
if got, ok := pbsHTTPStatus(firstErr); !ok || got != status {
t.Fatalf("first GetNodeName status = (%d, %v), want (%d, true): %v", got, ok, status, firstErr)
}
for i := 0; i < 2; i++ {
if _, err := client.GetNodeName(context.Background()); err == nil {
t.Fatalf("deferred GetNodeName call %d: expected error", i)
}
name, err := client.GetNodeName(context.Background())
if err != nil {
t.Fatalf("second GetNodeName: %v", err)
}
if got := nodesCalls.Load(); got != 1 {
t.Fatalf("/nodes hit %d times, want 1 (%d defers retries)", got, status)
if name != "pbs-recovered" {
t.Fatalf("second GetNodeName = %q, want pbs-recovered", name)
}
if got := nodesCalls.Load(); got != 2 {
t.Fatalf("/nodes hit %d times, want 2 (%d is retryable for direct root session)", got, status)
}
})
}
@ -545,15 +549,7 @@ func TestClient_GetNodeName_TransientHTTPFailuresRetryAndRecover(t *testing.T) {
}))
defer server.Close()
client, err := NewClient(ClientConfig{
Host: server.URL,
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
Timeout: 2 * time.Second,
})
if err != nil {
t.Fatalf("NewClient: %v", err)
}
client := newRootNodeNameClient(t, server.URL)
_, firstErr := client.GetNodeName(context.Background())
if firstErr == nil {
@ -562,13 +558,6 @@ func TestClient_GetNodeName_TransientHTTPFailuresRetryAndRecover(t *testing.T) {
if got, ok := pbsHTTPStatus(firstErr); !ok || got != tc.status {
t.Fatalf("first GetNodeName status = (%d, %v), want (%d, true): %v", got, ok, tc.status, firstErr)
}
client.nodeNameMu.Lock()
retryAfter := client.nodeNameRetryAfter
client.nodeNameMu.Unlock()
if !retryAfter.IsZero() {
t.Fatalf("transient status %d set permission retry deferral to %s", tc.status, retryAfter)
}
name, err := client.GetNodeName(context.Background())
if err != nil {
t.Fatalf("recovery GetNodeName: %v", err)
@ -584,15 +573,7 @@ func TestClient_GetNodeName_TransientHTTPFailuresRetryAndRecover(t *testing.T) {
}
func TestClient_GetNodeName_NetworkFailureRetriesAndRecovers(t *testing.T) {
client, err := NewClient(ClientConfig{
Host: "http://pbs.example.test",
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
Timeout: 2 * time.Second,
})
if err != nil {
t.Fatalf("NewClient: %v", err)
}
client := newRootNodeNameClient(t, "http://pbs.example.test")
var calls atomic.Int64
client.httpClient.Transport = roundTripFunc(func(req *http.Request) (*http.Response, error) {
@ -610,12 +591,6 @@ func TestClient_GetNodeName_NetworkFailureRetriesAndRecovers(t *testing.T) {
if _, err := client.GetNodeName(context.Background()); err == nil {
t.Fatal("first GetNodeName: expected network error")
}
client.nodeNameMu.Lock()
retryAfter := client.nodeNameRetryAfter
client.nodeNameMu.Unlock()
if !retryAfter.IsZero() {
t.Fatalf("network error set permission retry deferral to %s", retryAfter)
}
name, err := client.GetNodeName(context.Background())
if err != nil {
@ -629,53 +604,55 @@ func TestClient_GetNodeName_NetworkFailureRetriesAndRecovers(t *testing.T) {
}
}
func TestClient_GetNodeName_PermissionDeferralExpiresAndRecovers(t *testing.T) {
var nodesCalls atomic.Int64
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if nodesCalls.Add(1) == 1 {
w.WriteHeader(http.StatusForbidden)
_, _ = w.Write([]byte(`{"message":"access denied"}`))
return
}
_ = json.NewEncoder(w).Encode(map[string]any{
"data": []map[string]any{{"node": "pbs-after-acl-repair"}},
func TestClient_GetNodeName_UnsupportedAuthNeverCallsNodes(t *testing.T) {
tests := []struct {
name string
config ClientConfig
}{
{
name: "API token including root owner",
config: ClientConfig{
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
},
},
{
name: "non-superuser password identity",
config: ClientConfig{
User: "admin@pbs",
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
var nodesCalls atomic.Int64
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
nodesCalls.Add(1)
t.Fatalf("unsupported auth requested %s", r.URL.Path)
}))
defer server.Close()
tc.config.Host = server.URL
tc.config.Timeout = 2 * time.Second
client, err := NewClient(tc.config)
if err != nil {
t.Fatalf("NewClient: %v", err)
}
for i := 0; i < 3; i++ {
name, err := client.GetNodeName(context.Background())
if name != "" {
t.Fatalf("GetNodeName call %d = %q, want empty", i, name)
}
if !errors.Is(err, ErrNodeNameUnavailableForAuth) {
t.Fatalf("GetNodeName call %d error = %v, want ErrNodeNameUnavailableForAuth", i, err)
}
}
if got := nodesCalls.Load(); got != 0 {
t.Fatalf("/nodes hit %d times, want 0", got)
}
})
}))
defer server.Close()
client, err := NewClient(ClientConfig{
Host: server.URL,
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
Timeout: 2 * time.Second,
})
if err != nil {
t.Fatalf("NewClient: %v", err)
}
if _, err := client.GetNodeName(context.Background()); err == nil {
t.Fatal("first GetNodeName: expected permission error")
}
if _, err := client.GetNodeName(context.Background()); err == nil {
t.Fatal("deferred GetNodeName: expected error")
}
if got := nodesCalls.Load(); got != 1 {
t.Fatalf("/nodes hit %d times during deferral, want 1", got)
}
client.nodeNameMu.Lock()
client.nodeNameRetryAfter = time.Now().Add(-time.Second)
client.nodeNameMu.Unlock()
name, err := client.GetNodeName(context.Background())
if err != nil {
t.Fatalf("GetNodeName after deferral expiry: %v", err)
}
if name != "pbs-after-acl-repair" {
t.Fatalf("GetNodeName after deferral expiry = %q, want pbs-after-acl-repair", name)
}
if got := nodesCalls.Load(); got != 2 {
t.Fatalf("/nodes hit %d times after recovery, want 2", got)
}
}
@ -699,15 +676,7 @@ func TestClient_GetNodeName_ConcurrentTransientFailureIsSingleFlight(t *testing.
}))
defer server.Close()
client, err := NewClient(ClientConfig{
Host: server.URL,
TokenName: "root@pam!pulse-token",
TokenValue: "secret",
Timeout: 2 * time.Second,
})
if err != nil {
t.Fatalf("NewClient: %v", err)
}
client := newRootNodeNameClient(t, server.URL)
start := make(chan struct{})
errs := make(chan error, callers)