fix(vmware): tolerate absent JSON values and surface API fault detail

The vSphere JSON API represents an absent or null property (for example the parent of a top-level folder) as a successful response with an empty body, which was decoded as a spurious 'not valid JSON' enrichment error. Non-2xx responses also carry a JSON fault body that was discarded. See #2070. Change-source: pulse-maintainer
2026-10-25 13:10:03 +00:00 · 2026-09-20 00:04:07 +01:00
parent 22a8084852
commit 1b7bb40b0e
3 changed files with 217 additions and 16 deletions
@@ -413,12 +413,9 @@ func (c *Client) getSessionScopedJSON(
 	switch resp.StatusCode {
 	case http.StatusOK:
 	default:
-		return classifyReadStatusCode(label, resp.StatusCode)
+		return classifyReadStatusCodeWithBody(label, resp.StatusCode, body)
 	}
-	if err := json.Unmarshal(body, target); err != nil {
-		return &ConnectionError{Category: "endpoint", Message: fmt.Sprintf("VMware %s response was not valid JSON", label)}
-	}
-	return nil
+	return decodeVIJSONBody(body, label, target)
 }

 // postSessionScopedJSON posts a JSON body to a session-authenticated vCenter
@@ -454,12 +451,9 @@ func (c *Client) postSessionScopedJSON(
 		return fmt.Errorf("read %s response: %w", label, readErr)
 	}
 	if resp.StatusCode != http.StatusOK {
-		return classifyReadStatusCode(label, resp.StatusCode)
+		return classifyReadStatusCodeWithBody(label, resp.StatusCode, responseBody)
 	}
-	if err := json.Unmarshal(responseBody, target); err != nil {
-		return &ConnectionError{Category: "endpoint", Message: fmt.Sprintf("VMware %s response was not valid JSON", label)}
-	}
-	return nil
+	return decodeVIJSONBody(responseBody, label, target)
 }

 func (c *Client) getAutomationJSON(
@@ -607,6 +601,16 @@ func (c *Client) loginVIJSON(ctx context.Context, release string, sessionManager
 }

 func classifyReadStatusCode(label string, statusCode int) error {
+	return classifyReadStatusCodeWithBody(label, statusCode, nil)
+}
+
+// classifyReadStatusCodeWithBody classifies a non-2xx read and, when the body is
+// present, appends the vSphere JSON API fault detail. The JSON API answers
+// client-side argument problems (an unknown argument name or shape) with an
+// HTTP 5xx plus a JSON fault body, so the faultstring/invalidProperty is the only
+// actionable part of the response; discarding it leaves the operator with an
+// opaque "HTTP 500".
+func classifyReadStatusCodeWithBody(label string, statusCode int, body []byte) error {
 	switch statusCode {
 	case http.StatusUnauthorized:
 		return &ConnectionError{Category: "auth", Message: fmt.Sprintf("VMware authentication failed while reading %s", label)}
@@ -617,10 +621,77 @@ func classifyReadStatusCode(label string, statusCode int) error {
 	case http.StatusServiceUnavailable:
 		return &ConnectionError{Category: "unavailable", Message: fmt.Sprintf("VMware %s is temporarily unavailable", label)}
 	default:
-		return &ConnectionError{Category: "endpoint", Message: fmt.Sprintf("VMware %s request failed with HTTP %d", label, statusCode)}
+		message := fmt.Sprintf("VMware %s request failed with HTTP %d", label, statusCode)
+		if detail := viJSONFaultDetail(body); detail != "" {
+			message = fmt.Sprintf("%s (%s)", message, detail)
+		}
+		return &ConnectionError{Category: "endpoint", Message: message}
 	}
 }

+// viJSONFault models the fault envelope the vSphere JSON API returns in the body
+// of a rejected request.
+type viJSONFault struct {
+	TypeName        string `json:"_typeName"`
+	FaultString     string `json:"faultstring"`
+	InvalidProperty string `json:"invalidProperty"`
+}
+
+const viJSONFaultDetailLimit = 200
+
+// viJSONFaultDetail extracts a bounded, single-line summary of a vSphere JSON
+// API fault body. It returns "" when the body is not a recognisable fault so the
+// caller falls back to the plain status line. The text is server-controlled, so
+// it is collapsed to one line and length-bounded before it reaches logs or the
+// connection status surface.
+func viJSONFaultDetail(body []byte) string {
+	trimmed := bytes.TrimSpace(body)
+	if len(trimmed) == 0 || trimmed[0] != '{' {
+		return ""
+	}
+	var fault viJSONFault
+	if err := json.Unmarshal(trimmed, &fault); err != nil {
+		return ""
+	}
+	parts := make([]string, 0, 3)
+	if value := viJSONFaultFieldText(fault.FaultString); value != "" {
+		parts = append(parts, value)
+	}
+	if value := viJSONFaultFieldText(fault.InvalidProperty); value != "" {
+		parts = append(parts, "invalidProperty="+value)
+	}
+	if value := viJSONFaultFieldText(fault.TypeName); value != "" {
+		parts = append(parts, "type="+value)
+	}
+	if len(parts) == 0 {
+		return ""
+	}
+	return strings.Join(parts, ", ")
+}
+
+func viJSONFaultFieldText(raw string) string {
+	raw = strings.Join(strings.Fields(raw), " ")
+	if len(raw) > viJSONFaultDetailLimit {
+		raw = raw[:viJSONFaultDetailLimit]
+	}
+	return raw
+}
+
+// decodeVIJSONBody decodes a vSphere JSON API response body into target. A
+// successful response with an empty body represents an absent/null value (for
+// example the parent of a top-level folder), not a malformed response, so it
+// leaves target at its zero value instead of raising a spurious "not valid JSON"
+// enrichment error.
+func decodeVIJSONBody(body []byte, label string, target any) error {
+	if len(bytes.TrimSpace(body)) == 0 {
+		return nil
+	}
+	if err := json.Unmarshal(body, target); err != nil {
+		return &ConnectionError{Category: "endpoint", Message: fmt.Sprintf("VMware %s response was not valid JSON", label)}
+	}
+	return nil
+}
+
 func classifyTransportError(stage string, err error) error {
 	if err == nil {
 		return nil
@@ -0,0 +1,133 @@
+package vmware
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestDecodeVIJSONBodyTreatsEmptySuccessAsAbsent(t *testing.T) {
+	var ref *viJSONReference
+	if err := decodeVIJSONBody([]byte("  \n\t "), "folder parent", &ref); err != nil {
+		t.Fatalf("empty success body should be tolerated, got %v", err)
+	}
+	if ref != nil {
+		t.Fatalf("empty success body should leave target nil, got %+v", ref)
+	}
+}
+
+func TestDecodeVIJSONBodyRejectsMalformedBody(t *testing.T) {
+	var ref *viJSONReference
+	err := decodeVIJSONBody([]byte("<html>nope</html>"), "folder parent", &ref)
+	if err == nil {
+		t.Fatal("expected malformed body to fail")
+	}
+	connectionErr, ok := err.(*ConnectionError)
+	if !ok {
+		t.Fatalf("expected ConnectionError, got %T", err)
+	}
+	if connectionErr.Category != "endpoint" || !strings.Contains(connectionErr.Message, "not valid JSON") {
+		t.Fatalf("unexpected error: %+v", connectionErr)
+	}
+}
+
+func TestVIJSONFaultDetailExtractsBoundedSummary(t *testing.T) {
+	body := []byte(`{"_typeName":"InvalidArgument","invalidProperty":"perfSpec","faultstring":"Unknown argument."}`)
+	got := viJSONFaultDetail(body)
+	for _, want := range []string{"Unknown argument.", "invalidProperty=perfSpec", "type=InvalidArgument"} {
+		if !strings.Contains(got, want) {
+			t.Fatalf("fault detail %q missing %q", got, want)
+		}
+	}
+	if strings.ContainsAny(got, "\n\r") {
+		t.Fatalf("fault detail must be single-line, got %q", got)
+	}
+}
+
+func TestVIJSONFaultDetailIgnoresNonFaultBody(t *testing.T) {
+	for _, body := range [][]byte{nil, []byte(""), []byte("internal error"), []byte(`{"foo":"bar"}`), []byte("not json")} {
+		if got := viJSONFaultDetail(body); got != "" {
+			t.Fatalf("body %q produced fault detail %q, want empty", body, got)
+		}
+	}
+}
+
+func TestClassifyReadStatusCodeWithBodySurfacesFault(t *testing.T) {
+	body := []byte(`{"faultstring":"Invalid URI format"}`)
+	err := classifyReadStatusCodeWithBody("vmware performance metrics", http.StatusInternalServerError, body)
+	connectionErr, ok := err.(*ConnectionError)
+	if !ok {
+		t.Fatalf("expected ConnectionError, got %T", err)
+	}
+	if connectionErr.Category != "endpoint" {
+		t.Fatalf("category = %q, want endpoint", connectionErr.Category)
+	}
+	if !strings.Contains(connectionErr.Message, "HTTP 500") || !strings.Contains(connectionErr.Message, "Invalid URI format") {
+		t.Fatalf("message %q should carry status and fault detail", connectionErr.Message)
+	}
+}
+
+func TestCollectEntityReferenceToleratesEmptyParentBody(t *testing.T) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("/sdk/vim25/9.0.0.0/Folder/group-h5/parent", func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	})
+	server := httptest.NewTLSServer(mux)
+	defer server.Close()
+
+	client, err := NewClient(ClientConfig{
+		Host:               server.URL,
+		Username:           "admin",
+		Password:           "secret",
+		InsecureSkipVerify: true,
+		Timeout:            5 * time.Second,
+	})
+	if err != nil {
+		t.Fatalf("NewClient: %v", err)
+	}
+
+	ref, err := client.collectEntityReference(context.Background(), "9.0.0.0", "vi-session", "Folder", "group-h5", "parent", "folder parent")
+	if err != nil {
+		t.Fatalf("empty parent body should not degrade the connection: %v", err)
+	}
+	if ref != nil {
+		t.Fatalf("expected nil parent reference, got %+v", ref)
+	}
+}
+
+func TestCollectEntityReferenceSurfacesFaultDetail(t *testing.T) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("/sdk/vim25/9.0.0.0/Folder/group-h5/parent", func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusInternalServerError)
+		_, _ = w.Write([]byte(`{"_typeName":"InvalidArgument","invalidProperty":"entity","faultstring":"Invalid MoRef field: entity"}`))
+	})
+	server := httptest.NewTLSServer(mux)
+	defer server.Close()
+
+	client, err := NewClient(ClientConfig{
+		Host:               server.URL,
+		Username:           "admin",
+		Password:           "secret",
+		InsecureSkipVerify: true,
+		Timeout:            5 * time.Second,
+	})
+	if err != nil {
+		t.Fatalf("NewClient: %v", err)
+	}
+
+	_, err = client.collectEntityReference(context.Background(), "9.0.0.0", "vi-session", "Folder", "group-h5", "parent", "folder parent")
+	if err == nil {
+		t.Fatal("expected fault response to fail")
+	}
+	connectionErr, ok := err.(*ConnectionError)
+	if !ok {
+		t.Fatalf("expected ConnectionError, got %T", err)
+	}
+	if !strings.Contains(connectionErr.Message, "Invalid MoRef field: entity") {
+		t.Fatalf("message %q should carry the fault detail", connectionErr.Message)
+	}
+}
@@ -320,12 +320,9 @@ func (c *Client) postVIJSONJSON(ctx context.Context, sessionID, path, label stri
 	switch resp.StatusCode {
 	case http.StatusOK:
 	default:
-		return classifyReadStatusCode(label, resp.StatusCode)
+		return classifyReadStatusCodeWithBody(label, resp.StatusCode, responseBody)
 	}
-	if err := json.Unmarshal(responseBody, target); err != nil {
-		return &ConnectionError{Category: "endpoint", Message: fmt.Sprintf("VMware %s response was not valid JSON", label)}
-	}
-	return nil
+	return decodeVIJSONBody(responseBody, label, target)
 }

 func perfMetricIDsForEntity(