[Infrastructure UI] Improve metrics settings error handling (#146272)

Closes [#145238](https://github.com/elastic/kibana/issues/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 <marcoantonio.ghiani@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
This commit is contained in:
Marco Antonio Ghiani 2022-11-30 11:54:37 +01:00 committed by GitHub
parent a44304eb0e
commit ddcbf73284
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 162 additions and 51 deletions

View file

@ -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';

View file

@ -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);
});
});

View file

@ -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>;

View file

@ -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,
});

View file

@ -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 })),

View file

@ -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."
/>
);

View file

@ -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

View file

@ -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,
};
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',
},
});
}
const status: MetricsSourceStatus = {
metricIndicesExist,
indexFields,
};
return response.ok({
body: metricsSourceConfigurationResponseRT.encode({ source: { ...source, status } }),
});
}
);
@ -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),