Handle ES ping failures more gracefully (#7626)

## Which problem is this PR solving?
- Resolves #7625 

## Description of the changes
- Added test cases for the changes done in the #7422 

## How was this change tested?
- `/usr/bin/go test -timeout 30s
-coverprofile=/tmp/vscode-gokdhNQo/go-code-cover
github.com/jaegertracing/jaeger/internal/storage/elasticsearch/config`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: Jakub Stasiak <[email protected]>
Signed-off-by: Tushar Anand <[email protected]>
Co-authored-by: Jakub Stasiak <[email protected]>

Author: neoandmatrix

internal/storage/elasticsearch/config/config.go

@@ -252,10 +252,23 @@ func NewClient(ctx context.Context, c *Configuration, logger *zap.Logger, metric
 
 	if c.Version == 0 {
 		// Determine ElasticSearch Version
-		pingResult, _, err := rawClient.Ping(c.Servers[0]).Do(ctx)
+		pingResult, pingStatus, err := rawClient.Ping(c.Servers[0]).Do(ctx)
 		if err != nil {
 			return nil, err
 		}
+
+		// Non-2xx responses aren't reported as errors by the ping code (7.0.32 version of
+		// the elastic client).
+		if pingStatus < 200 || pingStatus >= 300 {
+			return nil, fmt.Errorf("ElasticSearch server %s returned HTTP %d, expected 2xx", c.Servers[0], pingStatus)
+		}
+
+		// The deserialization in the ping implementation may succeed even if the response
+		// contains no relevant properties and we may get empty values in that case.
+		if pingResult.Version.Number == "" {
+			return nil, fmt.Errorf("ElasticSearch server %s returned invalid ping response", c.Servers[0])
+		}
+
 		esVersion, err := strconv.Atoi(string(pingResult.Version.Number[0]))
 		if err != nil {
 			return nil, err

internal/storage/elasticsearch/config/config_test.go

@@ -507,6 +507,149 @@ func TestNewClient(t *testing.T) {
 	}
 }
 
+func TestNewClientPingErrorHandling(t *testing.T) {
+	tests := []struct {
+		name           string
+		serverResponse []byte
+		statusCode     int
+		expectedError  string
+	}{
+		{
+			name:           "ping returns 404 status",
+			serverResponse: mockEsServerResponseWithVersion0,
+			statusCode:     404,
+			expectedError:  "ElasticSearch server",
+		},
+		{
+			name:           "ping returns 500 status",
+			serverResponse: mockEsServerResponseWithVersion0,
+			statusCode:     500,
+			expectedError:  "ElasticSearch server",
+		},
+		{
+			name:           "ping returns 300 status",
+			serverResponse: mockEsServerResponseWithVersion0,
+			statusCode:     300,
+			expectedError:  "ElasticSearch server",
+		},
+		{
+			name:           "ping returns empty version number",
+			serverResponse: []byte(`{"Version": {"Number": ""}}`),
+			statusCode:     200,
+			expectedError:  "invalid ping response",
+		},
+
+		{
+			name:           "ping returns valid 200 status with version",
+			serverResponse: mockEsServerResponseWithVersion0,
+			statusCode:     200,
+			expectedError:  "",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			testServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
+				assert.Contains(t, []string{http.MethodGet, http.MethodHead}, req.Method)
+				res.WriteHeader(test.statusCode)
+				res.Write(test.serverResponse)
+			}))
+			defer testServer.Close()
+
+			config := &Configuration{
+				Servers:            []string{testServer.URL},
+				LogLevel:           "error",
+				DisableHealthCheck: true,
+			}
+
+			logger := zap.NewNop()
+			metricsFactory := metrics.NullFactory
+			client, err := NewClient(context.Background(), config, logger, metricsFactory)
+
+			if test.expectedError != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), test.expectedError)
+				require.Nil(t, client)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, client)
+				err = client.Close()
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestNewClientVersionDetection(t *testing.T) {
+	tests := []struct {
+		name            string
+		serverResponse  []byte
+		expectedVersion uint
+		expectedError   string
+	}{
+		{
+			name: "version number with letters",
+			serverResponse: []byte(`{
+                "Version": {
+                    "Number": "7.x.1"
+                }
+            }`),
+			expectedVersion: 7,
+			expectedError:   "",
+		},
+		{
+			name: "empty version number should fail validation",
+			serverResponse: []byte(`{
+                "Version": {
+                    "Number": ""
+                }
+            }`),
+			expectedError: "invalid ping response",
+		},
+		{
+			name: "version number as numeric should fail JSON parsing",
+			serverResponse: []byte(`{
+                "Version": {
+                    "Number": 7
+                }
+            }`),
+			expectedError: "cannot unmarshal number",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			testServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, _ *http.Request) {
+				res.WriteHeader(http.StatusOK)
+				res.Write(test.serverResponse)
+			}))
+			defer testServer.Close()
+
+			config := &Configuration{
+				Servers:            []string{testServer.URL},
+				LogLevel:           "error",
+				DisableHealthCheck: true,
+			}
+
+			logger := zap.NewNop()
+			metricsFactory := metrics.NullFactory
+			client, err := NewClient(context.Background(), config, logger, metricsFactory)
+
+			if test.expectedError != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), test.expectedError)
+				require.Nil(t, client)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, client)
+				assert.Equal(t, test.expectedVersion, config.Version)
+				err = client.Close()
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
 func TestApplyDefaults(t *testing.T) {
 	source := &Configuration{
 		RemoteReadClusters: []string{"cluster1", "cluster2"},