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

…146272) (#146699) # Backport This will backport the following commits from `main` to `7.17`: - [[Infrastructure UI] Improve metrics settings error handling (#146272)](#146272)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) \n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Marco Antonio Ghiani <[email protected]>"}},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/146272","number":146272,"mergeCommit":{"message":"[Infrastructure UI] Improve metrics settings error handling (#146272)\n\nCloses [#145238](https://github.com/elastic/kibana/issues/145238)\r\n\r\n## Summary\r\n\r\nThese changes add validation to the Metric Indices passed into the\r\nMetrics settings page.\r\nNew validation is added both in the UI and in the endpoint, performing\r\nthe following checks:\r\n- Index pattern is not an empty string\r\n- Index pattern does not contain empty spaces (start, middle, end) (the\r\npattern is not trimmed)\r\n- Index pattern does not contain empty entries, comma-separated values\r\nshould have an acceptable value.\r\n\r\nIn case the value is not valid, the UI will render an appropriate error\r\nmessage.\r\nIf the `PATCH /api/metrics/source/{sourceId}` request to update the\r\nvalue is manually sent with an invalid value, the server will respond\r\nwith a 400 status code and an error message.\r\n\r\nAlso, for retro compatibility and to not block the user when the\r\nconfiguration can't be successfully retrieved, in case of internal error\r\nthe `GET /api/metrics/source/{sourceId}` will return a 404 and on the\r\nUI, instead of rendering a blank page, the user will see the empty form\r\nand will be able to re-insert the right values.\r\n\r\n## Testing\r\n\r\nNavigate to `Inventory`-> Click on `Settings` on the topbar -> Start\r\nwriting different metric indices in the Metric Indices field.\r\n\r\n### Editing Metric Indices validation\r\n\r\n\r\nhttps://user-images.githubusercontent.com/34506779/203763021-0f4d8926-ffa4-448a-a038-696732158f4e.mov\r\n\r\n### Missing/Broken configuration response\r\n\r\n\r\nhttps://user-images.githubusercontent.com/34506779/203763120-ffc91cd3-9bf4-43da-a04f-5561ceabf591.mov\r\n\r\nCo-authored-by: Marco Antonio Ghiani <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"ddcbf73284d48b1f1f6eb9322f6f108f312f8e02"}},{"branch":"8.5","label":"v8.5.3","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"url":"https://github.com/elastic/kibana/pull/146674","number":146674,"state":"MERGED","mergeCommit":{"sha":"223121bc5d668f560f609a25a9e08778fa504c50","message":"[8.5] [Infrastructure UI] Improve metrics settings error handling (#146272) (#146674)\n\n# Backport\n\nThis will backport the following commits from `main` to `8.5`:\n- [[Infrastructure UI] Improve metrics settings error handling\n(#146272)](https://github.com/elastic/kibana/pull/146272)\n\n\n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Marco Antonio Ghiani <[email protected]>"}}]}] BACKPORT--> Co-authored-by: Marco Antonio Ghiani <[email protected]>
elastic · Dec 1, 2022 · 2e01c23 · 2e01c23
1 parent ebb132c
commit 2e01c23
Show file tree

Hide file tree

Showing 10 changed files with 179 additions and 45 deletions.
diff --git a/packages/kbn-io-ts-utils/BUILD.bazel b/packages/kbn-io-ts-utils/BUILD.bazel
@@ -24,6 +24,7 @@ filegroup(
 NPM_MODULE_EXTRA_FILES = [
   "package.json",
   "deep_exact_rt/package.json",
+  "index_pattern_rt/package.json",
   "iso_to_epoch_rt/package.json",
   "json_rt/package.json",
   "merge_rt/package.json",

diff --git a/packages/kbn-io-ts-utils/index_pattern_rt/package.json b/packages/kbn-io-ts-utils/index_pattern_rt/package.json
@@ -0,0 +1,4 @@
+{
+  "main": "../target_node/index_pattern_rt",
+  "types": "../target_types/index_pattern_rt"
+}
diff --git a/packages/kbn-io-ts-utils/src/index.ts b/packages/kbn-io-ts-utils/src/index.ts
@@ -7,6 +7,7 @@
  */
 
 export { deepExactRt } from './deep_exact_rt';
+export { indexPatternRt } from './index_pattern_rt';
 export { jsonRt } from './json_rt';
 export { mergeRt } from './merge_rt';
 export { strictKeysRt } from './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/index_pattern_rt';
 import * as rt from 'io-ts';
 import { omit } from 'lodash';
 import {
@@ -30,6 +31,14 @@ export type MetricsSourceConfigurationProperties = rt.TypeOf<
   typeof metricsSourceConfigurationPropertiesRT
 >;
 
+export const partialMetricsSourceConfigurationReqPayloadRT = rt.partial({
+  ...metricsSourceConfigurationPropertiesRT.type.props,
+  metricAlias: indexPatternRt,
+  fields: rt.partial({
+    ...metricsSourceConfigurationPropertiesRT.type.props.fields.type.props,
+  }),
+});
+
 export const partialMetricsSourceConfigurationPropertiesRT = rt.partial({
   ...metricsSourceConfigurationPropertiesRT.type.props,
   fields: rt.partial({

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';
 
@@ -46,7 +50,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,
@@ -56,7 +60,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,
@@ -66,7 +74,9 @@ export const useIndicesConfigurationFormState = ({
   const containerFieldFieldProps = useMemo(
     () =>
       createInputFieldProps({
-        errors: validateInputFieldNotEmpty(formState.containerField),
+        errors: aggregateValidationErrors<string>(validateInputFieldNotEmpty)(
+          formState.containerField
+        ),
         name: `containerField`,
         onChange: (containerField) =>
           setFormStateChanges((changes) => ({ ...changes, containerField })),
@@ -77,7 +87,7 @@ export const useIndicesConfigurationFormState = ({
   const hostFieldFieldProps = useMemo(
     () =>
       createInputFieldProps({
-        errors: validateInputFieldNotEmpty(formState.hostField),
+        errors: aggregateValidationErrors<string>(validateInputFieldNotEmpty)(formState.hostField),
         name: `hostField`,
         onChange: (hostField) => setFormStateChanges((changes) => ({ ...changes, hostField })),
         value: formState.hostField,
@@ -87,7 +97,7 @@ export const useIndicesConfigurationFormState = ({
   const podFieldFieldProps = useMemo(
     () =>
       createInputFieldProps({
-        errors: validateInputFieldNotEmpty(formState.podField),
+        errors: aggregateValidationErrors<string>(validateInputFieldNotEmpty)(formState.podField),
         name: `podField`,
         onChange: (podField) => setFormStateChanges((changes) => ({ ...changes, podField })),
         value: formState.podField,
@@ -97,7 +107,9 @@ export const useIndicesConfigurationFormState = ({
   const tiebreakerFieldFieldProps = useMemo(
     () =>
       createInputFieldProps({
-        errors: validateInputFieldNotEmpty(formState.tiebreakerField),
+        errors: aggregateValidationErrors<string>(validateInputFieldNotEmpty)(
+          formState.tiebreakerField
+        ),
         name: `tiebreakerField`,
         onChange: (tiebreakerField) =>
           setFormStateChanges((changes) => ({ ...changes, tiebreakerField })),
@@ -108,7 +120,9 @@ export const useIndicesConfigurationFormState = ({
   const timestampFieldFieldProps = useMemo(
     () =>
       createInputFieldProps({
-        errors: validateInputFieldNotEmpty(formState.timestampField),
+        errors: aggregateValidationErrors<string>(validateInputFieldNotEmpty)(
+          formState.timestampField
+        ),
         name: `timestampField`,
         onChange: (timestampField) =>
           setFormStateChanges((changes) => ({ ...changes, timestampField })),
@@ -119,7 +133,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, useContext, useMemo } from 'react';
+import React, { useCallback, useContext } from 'react';
 import { SourceLoadingPage } from '../../../components/source_loading_page';
 import { Source } from '../../../containers/metrics_source';
 import { useInfraMLCapabilitiesContext } from '../../../containers/ml/infra_ml_capabilities';
@@ -76,19 +76,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) => {
@@ -34,24 +34,33 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) =>
     async (requestContext, request, response) => {
       const { sourceId } = request.params;
 
-      const [source, metricIndicesExist, indexFields] = await Promise.all([
-        libs.sources.getSourceConfiguration(requestContext.core.savedObjects.client, sourceId),
-        libs.sourceStatus.hasMetricIndices(requestContext, sourceId),
-        libs.fields.getFields(requestContext, sourceId, 'METRICS'),
-      ]);
+      try {
+        const [source, metricIndicesExist, indexFields] = await Promise.all([
+          libs.sources.getSourceConfiguration(requestContext.core.savedObjects.client, 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',
+          },
+        });
+      }
     }
   );
 
@@ -63,13 +72,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 sourceConfiguration = await sources.getSourceConfiguration(
@@ -89,13 +98,13 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) =>
               requestContext.core.savedObjects.client,
               sourceId,
               // @ts-ignore
-              patchedSourceConfigurationProperties
+              sourceConfigurationPayload
             )
           : sources.createSourceConfiguration(
               requestContext.core.savedObjects.client,
               sourceId,
               // @ts-ignore
-              patchedSourceConfigurationProperties
+              sourceConfigurationPayload
             ));
 
         const [metricIndicesExist, indexFields] = await Promise.all([