From 2e01c23794f2d7ff392bc615d034f96f7dd5c7e2 Mon Sep 17 00:00:00 2001 From: Marco Antonio Ghiani Date: Thu, 1 Dec 2022 11:33:59 +0100 Subject: [PATCH] [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)](https://github.com/elastic/kibana/pull/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 "}},{"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 \r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","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 "}}]}] BACKPORT--> Co-authored-by: Marco Antonio Ghiani --- packages/kbn-io-ts-utils/BUILD.bazel | 1 + .../index_pattern_rt/package.json | 4 ++ packages/kbn-io-ts-utils/src/index.ts | 1 + .../src/index_pattern_rt/index.test.ts | 35 +++++++++++++ .../src/index_pattern_rt/index.ts | 36 ++++++++++++++ .../infra/common/metrics_sources/index.ts | 9 ++++ .../indices_configuration_form_state.ts | 30 +++++++++--- .../pages/metrics/settings/input_fields.tsx | 49 +++++++++++++++---- .../source_configuration_settings.tsx | 10 +--- .../server/routes/metrics_sources/index.ts | 49 +++++++++++-------- 10 files changed, 179 insertions(+), 45 deletions(-) create mode 100644 packages/kbn-io-ts-utils/index_pattern_rt/package.json create mode 100644 packages/kbn-io-ts-utils/src/index_pattern_rt/index.test.ts create mode 100644 packages/kbn-io-ts-utils/src/index_pattern_rt/index.ts diff --git a/packages/kbn-io-ts-utils/BUILD.bazel b/packages/kbn-io-ts-utils/BUILD.bazel index e5f1de4d07f63..b328a141a1b03 100644 --- 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 new file mode 100644 index 0000000000000..089772b991a67 --- /dev/null +++ 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" +} \ No newline at end of file diff --git a/packages/kbn-io-ts-utils/src/index.ts b/packages/kbn-io-ts-utils/src/index.ts index 88cfc063f738a..3066bc7e17434 100644 --- 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 new file mode 100644 index 0000000000000..0caf40812fb04 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..02696d28ec73a --- /dev/null +++ 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; + +export const indexPatternRt = rt.brand( + rt.string, + (pattern): pattern is IndexPattern => validateIndexPattern(pattern), + 'IndexPattern' +); + +export type IndexPatternType = rt.TypeOf; diff --git a/x-pack/plugins/infra/common/metrics_sources/index.ts b/x-pack/plugins/infra/common/metrics_sources/index.ts index a697c65e5a0aa..d0f71daadd54a 100644 --- 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 index ced87112e6e9a..955e33e665f02 100644 --- 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(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( + 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(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(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(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(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(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 index a7a842417ebc2..e324d73462638 100644 --- 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 = 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 === '' - ? [ - , - ] - : []; +export const aggregateValidationErrors = + ( + ...validationHandlers: ValidationHandlerList + ) => + (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) && ( + + ); + +export const validateInputFieldHasNotEmptyEntries = (value: string) => + containsEmptyEntries(value) && ( + + ); + +export const validateInputFieldHasNotEmptySpaces = (value: string) => + containsSpaces(value) && ( + + ); 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 index 0adf4ed6b5b45..00411d540266b 100644 --- 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 ; } - if (!source?.configuration) { - return null; - } return ( { @@ -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([