TUN-8792: Make diag/system endpoint always return a JSON

## Summary
Change the system information collector and respective http handler so that it always returns a JSON.

Closes [TUN-8792](https://jira.cfdata.org/browse/TUN-8792)

Author: lmpn

diagnostic/client.go

@@ -141,7 +141,7 @@ func (client *httpClient) GetSystemInformation(ctx context.Context, writer io.Wr
 		return err
 	}
 
-	return copyToWriter(response, writer)
+	return copyJSONToWriter(response, writer)
 }
 
 func (client *httpClient) GetMetrics(ctx context.Context, writer io.Writer) error {

diagnostic/handlers.go

@@ -59,6 +59,11 @@ func (handler *Handler) InstallEndpoints(router *http.ServeMux) {
 	router.HandleFunc(systemInformationEndpoint, handler.SystemHandler)
 }
 
+type SystemInformationResponse struct {
+	Info *SystemInformation `json:"info"`
+	Err  error              `json:"errors"`
+}
+
 func (handler *Handler) SystemHandler(writer http.ResponseWriter, request *http.Request) {
 	logger := handler.log.With().Str(collectorField, systemCollectorName).Logger()
 	logger.Info().Msg("Collection started")
@@ -69,30 +74,15 @@ func (handler *Handler) SystemHandler(writer http.ResponseWriter, request *http.
 
 	defer cancel()
 
-	info, rawInfo, err := handler.systemCollector.Collect(ctx)
-	if err != nil {
-		logger.Error().Err(err).Msg("error occurred whilst collecting system information")
-
-		if rawInfo != "" {
-			logger.Info().Msg("using raw information fallback")
-			bytes := []byte(rawInfo)
-			writeResponse(writer, bytes, &logger)
-		} else {
-			logger.Error().Msg("no raw information available")
-			writer.WriteHeader(http.StatusInternalServerError)
-		}
-
-		return
-	}
+	info, err := handler.systemCollector.Collect(ctx)
 
-	if info == nil {
-		logger.Error().Msgf("system information collection is nil")
-		writer.WriteHeader(http.StatusInternalServerError)
+	response := SystemInformationResponse{
+		Info: info,
+		Err:  err,
 	}
 
 	encoder := json.NewEncoder(writer)
-
-	err = encoder.Encode(info)
+	err = encoder.Encode(response)
 	if err != nil {
 		logger.Error().Err(err).Msgf("error occurred whilst serializing information")
 		writer.WriteHeader(http.StatusInternalServerError)

diagnostic/handlers_test.go

@@ -4,10 +4,10 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"io"
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"runtime"
 	"testing"
 
 	"github.com/google/uuid"
@@ -20,11 +20,13 @@ import (
 	"github.com/cloudflare/cloudflared/tunnelstate"
 )
 
-type SystemCollectorMock struct{}
+type SystemCollectorMock struct {
+	systemInfo *diagnostic.SystemInformation
+	err        error
+}
 
 const (
 	systemInformationKey = "sikey"
-	rawInformationKey    = "rikey"
 	errorKey             = "errkey"
 )
 
@@ -46,24 +48,8 @@ func newTrackerFromConns(t *testing.T, connections []tunnelstate.IndexedConnecti
 	return tracker
 }
 
-func setCtxValuesForSystemCollector(
-	systemInfo *diagnostic.SystemInformation,
-	rawInfo string,
-	err error,
-) context.Context {
-	ctx := context.Background()
-	ctx = context.WithValue(ctx, systemInformationKey, systemInfo)
-	ctx = context.WithValue(ctx, rawInformationKey, rawInfo)
-	ctx = context.WithValue(ctx, errorKey, err)
-
-	return ctx
-}
-
-func (*SystemCollectorMock) Collect(ctx context.Context) (*diagnostic.SystemInformation, string, error) {
-	si, _ := ctx.Value(systemInformationKey).(*diagnostic.SystemInformation)
-	ri, _ := ctx.Value(rawInformationKey).(string)
-	err, _ := ctx.Value(errorKey).(error)
-	return si, ri, err
+func (collector *SystemCollectorMock) Collect(context.Context) (*diagnostic.SystemInformation, error) {
+	return collector.systemInfo, collector.err
 }
 
 func TestSystemHandler(t *testing.T) {
@@ -73,7 +59,6 @@ func TestSystemHandler(t *testing.T) {
 	tests := []struct {
 		name       string
 		systemInfo *diagnostic.SystemInformation
-		rawInfo    string
 		err        error
 		statusCode int
 	}{
@@ -82,47 +67,39 @@ func TestSystemHandler(t *testing.T) {
 			systemInfo: diagnostic.NewSystemInformation(
 				0, 0, 0, 0,
 				"string", "string", "string", "string",
-				"string", "string", nil,
+				"string", "string",
+				runtime.Version(), runtime.GOARCH, nil,
 			),
-			rawInfo:    "",
+
 			err:        nil,
 			statusCode: http.StatusOK,
 		},
-		{
-			name: "on error and raw info", systemInfo: nil,
-			rawInfo: "raw info", err: errors.New("an error"), statusCode: http.StatusOK,
-		},
 		{
 			name: "on error and no raw info", systemInfo: nil,
-			rawInfo: "", err: errors.New("an error"), statusCode: http.StatusInternalServerError,
-		},
-		{
-			name: "malformed response", systemInfo: nil, rawInfo: "", err: nil, statusCode: http.StatusInternalServerError,
+			err: errors.New("an error"), statusCode: http.StatusOK,
 		},
 	}
 
 	for _, tCase := range tests {
 		t.Run(tCase.name, func(t *testing.T) {
 			t.Parallel()
-			handler := diagnostic.NewDiagnosticHandler(&log, 0, &SystemCollectorMock{}, uuid.New(), uuid.New(), nil, map[string]string{}, nil)
+			handler := diagnostic.NewDiagnosticHandler(&log, 0, &SystemCollectorMock{
+				systemInfo: tCase.systemInfo,
+				err:        tCase.err,
+			}, uuid.New(), uuid.New(), nil, map[string]string{}, nil)
 			recorder := httptest.NewRecorder()
-			ctx := setCtxValuesForSystemCollector(tCase.systemInfo, tCase.rawInfo, tCase.err)
-			request, err := http.NewRequestWithContext(ctx, http.MethodGet, "/diag/syste,", nil)
+			ctx := context.Background()
+			request, err := http.NewRequestWithContext(ctx, http.MethodGet, "/diag/system", nil)
 			require.NoError(t, err)
 			handler.SystemHandler(recorder, request)
 
 			assert.Equal(t, tCase.statusCode, recorder.Code)
 			if tCase.statusCode == http.StatusOK && tCase.systemInfo != nil {
-				var response diagnostic.SystemInformation
-
+				var response diagnostic.SystemInformationResponse
 				decoder := json.NewDecoder(recorder.Body)
-				err = decoder.Decode(&response)
-				require.NoError(t, err)
-				assert.Equal(t, tCase.systemInfo, &response)
-			} else if tCase.statusCode == http.StatusOK && tCase.rawInfo != "" {
-				rawBytes, err := io.ReadAll(recorder.Body)
+				err := decoder.Decode(&response)
 				require.NoError(t, err)
-				assert.Equal(t, tCase.rawInfo, string(rawBytes))
+				assert.Equal(t, tCase.systemInfo, response.Info)
 			}
 		})
 	}

diagnostic/system_collector.go

@@ -1,6 +1,82 @@
 package diagnostic
 
-import "context"
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"strings"
+)
+
+type SystemInformationError struct {
+	Err     error  `json:"error"`
+	RawInfo string `json:"rawInfo"`
+}
+
+func (err SystemInformationError) Error() string {
+	return err.Err.Error()
+}
+
+func (err SystemInformationError) MarshalJSON() ([]byte, error) {
+	s := map[string]string{
+		"error":   err.Err.Error(),
+		"rawInfo": err.RawInfo,
+	}
+
+	return json.Marshal(s)
+}
+
+type SystemInformationGeneralError struct {
+	OperatingSystemInformationError error
+	MemoryInformationError          error
+	FileDescriptorsInformationError error
+	DiskVolumeInformationError      error
+}
+
+func (err SystemInformationGeneralError) Error() string {
+	builder := &strings.Builder{}
+	builder.WriteString("errors found:")
+
+	if err.OperatingSystemInformationError != nil {
+		builder.WriteString(err.OperatingSystemInformationError.Error() + ", ")
+	}
+
+	if err.MemoryInformationError != nil {
+		builder.WriteString(err.MemoryInformationError.Error() + ", ")
+	}
+
+	if err.FileDescriptorsInformationError != nil {
+		builder.WriteString(err.FileDescriptorsInformationError.Error() + ", ")
+	}
+
+	if err.DiskVolumeInformationError != nil {
+		builder.WriteString(err.DiskVolumeInformationError.Error() + ", ")
+	}
+
+	return builder.String()
+}
+
+func (err SystemInformationGeneralError) MarshalJSON() ([]byte, error) {
+	data := map[string]SystemInformationError{}
+
+	var sysErr SystemInformationError
+	if errors.As(err.OperatingSystemInformationError, &sysErr) {
+		data["operatingSystemInformationError"] = sysErr
+	}
+
+	if errors.As(err.MemoryInformationError, &sysErr) {
+		data["memoryInformationError"] = sysErr
+	}
+
+	if errors.As(err.FileDescriptorsInformationError, &sysErr) {
+		data["fileDescriptorsInformationError"] = sysErr
+	}
+
+	if errors.As(err.DiskVolumeInformationError, &sysErr) {
+		data["diskVolumeInformationError"] = sysErr
+	}
+
+	return json.Marshal(data)
+}
 
 type DiskVolumeInformation struct {
 	Name        string `json:"name"`        // represents the filesystem in linux/macos or device name in windows
@@ -17,17 +93,19 @@ func NewDiskVolumeInformation(name string, maximum, current uint64) *DiskVolumeI
 }
 
 type SystemInformation struct {
-	MemoryMaximum         uint64                   `json:"memoryMaximum"`         // represents the maximum memory of the system in kilobytes
-	MemoryCurrent         uint64                   `json:"memoryCurrent"`         // represents the system's memory in use in kilobytes
-	FileDescriptorMaximum uint64                   `json:"fileDescriptorMaximum"` // represents the maximum number of file descriptors of the system
-	FileDescriptorCurrent uint64                   `json:"fileDescriptorCurrent"` // represents the system's file descriptors in use
-	OsSystem              string                   `json:"osSystem"`              // represents the operating system name i.e.: linux, windows, darwin
-	HostName              string                   `json:"hostName"`              // represents the system host name
-	OsVersion             string                   `json:"osVersion"`             // detailed information about the system's release version level
-	OsRelease             string                   `json:"osRelease"`             // detailed information about the system's release
-	Architecture          string                   `json:"architecture"`          // represents the system's hardware platform i.e: arm64/amd64
-	CloudflaredVersion    string                   `json:"cloudflaredVersion"`    // the runtime version of cloudflared
-	Disk                  []*DiskVolumeInformation `json:"disk"`
+	MemoryMaximum         uint64                   `json:"memoryMaximum,omitempty"`         // represents the maximum memory of the system in kilobytes
+	MemoryCurrent         uint64                   `json:"memoryCurrent,omitempty"`         // represents the system's memory in use in kilobytes
+	FileDescriptorMaximum uint64                   `json:"fileDescriptorMaximum,omitempty"` // represents the maximum number of file descriptors of the system
+	FileDescriptorCurrent uint64                   `json:"fileDescriptorCurrent,omitempty"` // represents the system's file descriptors in use
+	OsSystem              string                   `json:"osSystem,omitempty"`              // represents the operating system name i.e.: linux, windows, darwin
+	HostName              string                   `json:"hostName,omitempty"`              // represents the system host name
+	OsVersion             string                   `json:"osVersion,omitempty"`             // detailed information about the system's release version level
+	OsRelease             string                   `json:"osRelease,omitempty"`             // detailed information about the system's release
+	Architecture          string                   `json:"architecture,omitempty"`          // represents the system's hardware platform i.e: arm64/amd64
+	CloudflaredVersion    string                   `json:"cloudflaredVersion,omitempty"`    // the runtime version of cloudflared
+	GoVersion             string                   `json:"goVersion,omitempty"`
+	GoArch                string                   `json:"goArch,omitempty"`
+	Disk                  []*DiskVolumeInformation `json:"disk,omitempty"`
 }
 
 func NewSystemInformation(
@@ -40,7 +118,9 @@ func NewSystemInformation(
 	osVersion,
 	osRelease,
 	architecture,
-	cloudflaredVersion string,
+	cloudflaredVersion,
+	goVersion,
+	goArchitecture string,
 	disk []*DiskVolumeInformation,
 ) *SystemInformation {
 	return &SystemInformation{
@@ -54,17 +134,17 @@ func NewSystemInformation(
 		osRelease,
 		architecture,
 		cloudflaredVersion,
+		goVersion,
+		goArchitecture,
 		disk,
 	}
 }
 
 type SystemCollector interface {
 	// If the collection is successful it will return `SystemInformation` struct,
-	// an empty string, and a nil error.
-	// In case there is an error a string with the raw data will be returned
-	// however the returned string not contain all the data points.
+	// and a nil error.
 	//
 	// This function expects that the caller sets the context timeout to prevent
 	// long-lived collectors.
-	Collect(ctx context.Context) (*SystemInformation, string, error)
+	Collect(ctx context.Context) (*SystemInformation, error)
 }