[Infrastructure UI] Improve metrics settings error handling (elastic#…

…146272) Closes [elastic#145238](elastic#145238) ## Summary These changes add validation to the Metric Indices passed into the Metrics settings page. New validation is added both in the UI and in the endpoint, performing the following checks: - Index pattern is not an empty string - Index pattern does not contain empty spaces (start, middle, end) (the pattern is not trimmed) - Index pattern does not contain empty entries, comma-separated values should have an acceptable value. In case the value is not valid, the UI will render an appropriate error message. If the `PATCH /api/metrics/source/{sourceId}` request to update the value is manually sent with an invalid value, the server will respond with a 400 status code and an error message. Also, for retro compatibility and to not block the user when the configuration can't be successfully retrieved, in case of internal error the `GET /api/metrics/source/{sourceId}` will return a 404 and on the UI, instead of rendering a blank page, the user will see the empty form and will be able to re-insert the right values. ## Testing Navigate to `Inventory`-> Click on `Settings` on the topbar -> Start writing different metric indices in the Metric Indices field. ### Editing Metric Indices validation https://user-images.githubusercontent.com/34506779/203763021-0f4d8926-ffa4-448a-a038-696732158f4e.mov ### Missing/Broken configuration response https://user-images.githubusercontent.com/34506779/203763120-ffc91cd3-9bf4-43da-a04f-5561ceabf591.mov Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: kibanamachine <[email protected]>
tonyghiani · Nov 30, 2022 · ddcbf73 · ddcbf73
1 parent a44304e
commit ddcbf73
Show file tree

Hide file tree

Showing 8 changed files with 161 additions and 50 deletions.
diff --git a/packages/kbn-io-ts-utils/index.ts b/packages/kbn-io-ts-utils/index.ts
@@ -6,9 +6,11 @@
  * Side Public License, v 1.
  */
 
+export type { IndexPatternType } from './src/index_pattern_rt';
 export type { NonEmptyStringBrand } from './src/non_empty_string_rt';
 
 export { deepExactRt } from './src/deep_exact_rt';
+export { indexPatternRt } from './src/index_pattern_rt';
 export { jsonRt } from './src/json_rt';
 export { mergeRt } from './src/merge_rt';
 export { strictKeysRt } from './src/strict_keys_rt';

diff --git a/packages/kbn-io-ts-utils/src/index_pattern_rt/index.test.ts b/packages/kbn-io-ts-utils/src/index_pattern_rt/index.test.ts
@@ -0,0 +1,35 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { indexPatternRt } from '.';
+import { isRight } from 'fp-ts/lib/Either';
+
+describe('indexPatternRt', () => {
+  test('passes on valid index pattern strings', () => {
+    expect(isRight(indexPatternRt.decode('logs-*'))).toBe(true);
+    expect(isRight(indexPatternRt.decode('logs-*,filebeat-*'))).toBe(true);
+  });
+
+  test('fails if the pattern is an empty string', () => {
+    expect(isRight(indexPatternRt.decode(''))).toBe(false);
+  });
+
+  test('fails if the pattern contains empty spaces', () => {
+    expect(isRight(indexPatternRt.decode(' '))).toBe(false);
+    expect(isRight(indexPatternRt.decode(' logs-*'))).toBe(false);
+    expect(isRight(indexPatternRt.decode('logs-* '))).toBe(false);
+    expect(isRight(indexPatternRt.decode('logs-*, filebeat-*'))).toBe(false);
+  });
+
+  test('fails if the pattern contains empty comma-separated entries', () => {
+    expect(isRight(indexPatternRt.decode(',logs-*'))).toBe(false);
+    expect(isRight(indexPatternRt.decode('logs-*,'))).toBe(false);
+    expect(isRight(indexPatternRt.decode('logs-*,,filebeat-*'))).toBe(false);
+    expect(isRight(indexPatternRt.decode('logs-*,,,filebeat-*'))).toBe(false);
+  });
+});
diff --git a/packages/kbn-io-ts-utils/src/index_pattern_rt/index.ts b/packages/kbn-io-ts-utils/src/index_pattern_rt/index.ts
@@ -0,0 +1,36 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+import * as rt from 'io-ts';
+
+export const isEmptyString = (value: string) => value === '';
+
+export const containsSpaces = (value: string) => value.includes(' ');
+
+export const containsEmptyEntries = (value: string) => value.split(',').some(isEmptyString);
+
+export const validateIndexPattern = (indexPattern: string) => {
+  return (
+    !isEmptyString(indexPattern) &&
+    !containsSpaces(indexPattern) &&
+    !containsEmptyEntries(indexPattern)
+  );
+};
+
+export interface IndexPatternBrand {
+  readonly IndexPattern: unique symbol;
+}
+
+type IndexPattern = rt.Branded<string, IndexPatternBrand>;
+
+export const indexPatternRt = rt.brand(
+  rt.string,
+  (pattern): pattern is IndexPattern => validateIndexPattern(pattern),
+  'IndexPattern'
+);
+
+export type IndexPatternType = rt.TypeOf<typeof indexPatternRt>;
diff --git a/x-pack/plugins/infra/common/metrics_sources/index.ts b/x-pack/plugins/infra/common/metrics_sources/index.ts
@@ -5,6 +5,7 @@
  * 2.0.
  */
 
+import { indexPatternRt } from '@kbn/io-ts-utils';
 import * as rt from 'io-ts';
 import {
   SourceConfigurationRT,
@@ -28,6 +29,11 @@ export type MetricsSourceConfigurationProperties = rt.TypeOf<
   typeof metricsSourceConfigurationPropertiesRT
 >;
 
+export const partialMetricsSourceConfigurationReqPayloadRT = rt.partial({
+  ...metricsSourceConfigurationPropertiesRT.type.props,
+  metricAlias: indexPatternRt,
+});
+
 export const partialMetricsSourceConfigurationPropertiesRT = rt.partial({
   ...metricsSourceConfigurationPropertiesRT.type.props,
 });

diff --git a/x-pack/plugins/infra/public/pages/metrics/settings/indices_configuration_form_state.ts b/x-pack/plugins/infra/public/pages/metrics/settings/indices_configuration_form_state.ts
@@ -6,9 +6,13 @@
  */
 
 import { ReactNode, useCallback, useMemo, useState } from 'react';
+
 import {
+  aggregateValidationErrors,
   createInputFieldProps,
   createInputRangeFieldProps,
+  validateInputFieldHasNotEmptyEntries,
+  validateInputFieldHasNotEmptySpaces,
   validateInputFieldNotEmpty,
 } from './input_fields';
 
@@ -41,7 +45,7 @@ export const useIndicesConfigurationFormState = ({
   const nameFieldProps = useMemo(
     () =>
       createInputFieldProps({
-        errors: validateInputFieldNotEmpty(formState.name),
+        errors: aggregateValidationErrors<string>(validateInputFieldNotEmpty)(formState.name),
         name: 'name',
         onChange: (name) => setFormStateChanges((changes) => ({ ...changes, name })),
         value: formState.name,
@@ -51,7 +55,11 @@ export const useIndicesConfigurationFormState = ({
   const metricAliasFieldProps = useMemo(
     () =>
       createInputFieldProps({
-        errors: validateInputFieldNotEmpty(formState.metricAlias),
+        errors: aggregateValidationErrors<string>(
+          validateInputFieldNotEmpty,
+          validateInputFieldHasNotEmptyEntries,
+          validateInputFieldHasNotEmptySpaces
+        )(formState.metricAlias),
         name: 'metricAlias',
         onChange: (metricAlias) => setFormStateChanges((changes) => ({ ...changes, metricAlias })),
         value: formState.metricAlias,
@@ -62,7 +70,7 @@ export const useIndicesConfigurationFormState = ({
   const anomalyThresholdFieldProps = useMemo(
     () =>
       createInputRangeFieldProps({
-        errors: validateInputFieldNotEmpty(formState.anomalyThreshold),
+        errors: aggregateValidationErrors(validateInputFieldNotEmpty)(formState.anomalyThreshold),
         name: 'anomalyThreshold',
         onChange: (anomalyThreshold) =>
           setFormStateChanges((changes) => ({ ...changes, anomalyThreshold })),

diff --git a/x-pack/plugins/infra/public/pages/metrics/settings/input_fields.tsx b/x-pack/plugins/infra/public/pages/metrics/settings/input_fields.tsx
@@ -22,6 +22,10 @@ export interface InputFieldProps<
 
 export type FieldErrorMessage = string | JSX.Element;
 
+export type ValidationHandlerList<ValueType> = Array<
+  (value: ValueType) => FieldErrorMessage | false
+>;
+
 export const createInputFieldProps = <
   Value extends string = string,
   FieldElement extends HTMLInputElement = HTMLInputElement
@@ -83,12 +87,39 @@ export const createInputRangeFieldProps = <
   value,
 });
 
-export const validateInputFieldNotEmpty = (value: React.ReactText) =>
-  value === ''
-    ? [
-        <FormattedMessage
-          id="xpack.infra.sourceConfiguration.fieldEmptyErrorMessage"
-          defaultMessage="The field must not be empty"
-        />,
-      ]
-    : [];
+export const aggregateValidationErrors =
+  <ValueType extends ReactText = ReactText>(
+    ...validationHandlers: ValidationHandlerList<ValueType>
+  ) =>
+  (value: ValueType) =>
+    validationHandlers.map((validator) => validator(value)).filter(Boolean) as FieldErrorMessage[];
+
+const isEmptyString = (value: ReactText) => value === '';
+
+const containsSpaces = (value: string) => value.includes(' ');
+
+const containsEmptyEntries = (value: string) => value.split(',').some(isEmptyString);
+
+export const validateInputFieldNotEmpty = (value: ReactText) =>
+  isEmptyString(value) && (
+    <FormattedMessage
+      id="xpack.infra.sourceConfiguration.fieldEmptyErrorMessage"
+      defaultMessage="The field must not be empty."
+    />
+  );
+
+export const validateInputFieldHasNotEmptyEntries = (value: string) =>
+  containsEmptyEntries(value) && (
+    <FormattedMessage
+      id="xpack.infra.sourceConfiguration.fieldContainEmptyEntryErrorMessage"
+      defaultMessage="The field must not include empty comma-separated values."
+    />
+  );
+
+export const validateInputFieldHasNotEmptySpaces = (value: string) =>
+  containsSpaces(value) && (
+    <FormattedMessage
+      id="xpack.infra.sourceConfiguration.fieldContainSpacesErrorMessage"
+      defaultMessage="The field must not include spaces."
+    />
+  );
diff --git a/x-pack/plugins/infra/public/pages/metrics/settings/source_configuration_settings.tsx b/x-pack/plugins/infra/public/pages/metrics/settings/source_configuration_settings.tsx
@@ -15,7 +15,7 @@ import {
 } from '@elastic/eui';
 import { i18n } from '@kbn/i18n';
 import { FormattedMessage } from '@kbn/i18n-react';
-import React, { useCallback, useMemo } from 'react';
+import React, { useCallback } from 'react';
 import { Prompt } from '@kbn/observability-plugin/public';
 import { SourceLoadingPage } from '../../../components/source_loading_page';
 import { useSourceContext } from '../../../containers/metrics_source';
@@ -75,19 +75,13 @@ export const SourceConfigurationSettings = ({
     formStateChanges,
   ]);
 
-  const isWriteable = useMemo(
-    () => shouldAllowEdit && source && source.origin !== 'internal',
-    [shouldAllowEdit, source]
-  );
+  const isWriteable = shouldAllowEdit && (!Boolean(source) || source?.origin !== 'internal');
 
   const { hasInfraMLCapabilities } = useInfraMLCapabilitiesContext();
 
   if ((isLoading || isUninitialized) && !source) {
     return <SourceLoadingPage />;
   }
-  if (!source?.configuration) {
-    return null;
-  }
 
   return (
     <MetricsPageTemplate

diff --git a/x-pack/plugins/infra/server/routes/metrics_sources/index.ts b/x-pack/plugins/infra/server/routes/metrics_sources/index.ts
@@ -13,9 +13,9 @@ import { hasData } from '../../lib/sources/has_data';
 import { createSearchClient } from '../../lib/create_search_client';
 import { AnomalyThresholdRangeError } from '../../lib/sources/errors';
 import {
-  partialMetricsSourceConfigurationPropertiesRT,
   metricsSourceConfigurationResponseRT,
   MetricsSourceStatus,
+  partialMetricsSourceConfigurationReqPayloadRT,
 } from '../../../common/metrics_sources';
 
 export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) => {
@@ -35,24 +35,33 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) =>
       const { sourceId } = request.params;
       const soClient = (await requestContext.core).savedObjects.client;
 
-      const [source, metricIndicesExist, indexFields] = await Promise.all([
-        libs.sources.getSourceConfiguration(soClient, sourceId),
-        libs.sourceStatus.hasMetricIndices(requestContext, sourceId),
-        libs.fields.getFields(requestContext, sourceId, 'METRICS'),
-      ]);
+      try {
+        const [source, metricIndicesExist, indexFields] = await Promise.all([
+          libs.sources.getSourceConfiguration(soClient, sourceId),
+          libs.sourceStatus.hasMetricIndices(requestContext, sourceId),
+          libs.fields.getFields(requestContext, sourceId, 'METRICS'),
+        ]);
 
-      if (!source) {
-        return response.notFound();
-      }
+        if (!source) {
+          return response.notFound();
+        }
 
-      const status: MetricsSourceStatus = {
-        metricIndicesExist,
-        indexFields,
-      };
+        const status: MetricsSourceStatus = {
+          metricIndicesExist,
+          indexFields,
+        };
 
-      return response.ok({
-        body: metricsSourceConfigurationResponseRT.encode({ source: { ...source, status } }),
-      });
+        return response.ok({
+          body: metricsSourceConfigurationResponseRT.encode({ source: { ...source, status } }),
+        });
+      } catch (error) {
+        return response.customError({
+          statusCode: error.statusCode ?? 500,
+          body: {
+            message: error.message ?? 'An unexpected error occurred',
+          },
+        });
+      }
     }
   );
 
@@ -64,13 +73,13 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) =>
         params: schema.object({
           sourceId: schema.string(),
         }),
-        body: createValidationFunction(partialMetricsSourceConfigurationPropertiesRT),
+        body: createValidationFunction(partialMetricsSourceConfigurationReqPayloadRT),
       },
     },
     framework.router.handleLegacyErrors(async (requestContext, request, response) => {
       const { sources } = libs;
       const { sourceId } = request.params;
-      const patchedSourceConfigurationProperties = request.body;
+      const sourceConfigurationPayload = request.body;
 
       try {
         const soClient = (await requestContext.core).savedObjects.client;
@@ -84,18 +93,8 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) =>
 
         const sourceConfigurationExists = sourceConfiguration.origin === 'stored';
         const patchedSourceConfiguration = await (sourceConfigurationExists
-          ? sources.updateSourceConfiguration(
-              soClient,
-              sourceId,
-              // @ts-ignore
-              patchedSourceConfigurationProperties
-            )
-          : sources.createSourceConfiguration(
-              soClient,
-              sourceId,
-              // @ts-ignore
-              patchedSourceConfigurationProperties
-            ));
+          ? sources.updateSourceConfiguration(soClient, sourceId, sourceConfigurationPayload)
+          : sources.createSourceConfiguration(soClient, sourceId, sourceConfigurationPayload));
 
         const [metricIndicesExist, indexFields] = await Promise.all([
           libs.sourceStatus.hasMetricIndices(requestContext, sourceId),