Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SloGlobalDiagnosis check to SLO List and SLO Create pages #157488

Merged
merged 4 commits into from
May 12, 2023
Merged

Add SloGlobalDiagnosis check to SLO List and SLO Create pages #157488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd81f81
Add SloGlobalDiagnosis check to SLO List and SLO Create pages
CoenWarmer May 12, 2023
cffdc2d
Update tests
CoenWarmer May 12, 2023
cb6e5fe
Update x-pack/plugins/observability/public/hooks/slo/use_fetch_global…
CoenWarmer May 12, 2023
f506486
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions x-pack/plugins/observability/public/hooks/slo/helpers/convert_error_for_use_in_toast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* 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; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export function convertErrorForUseInToast(error: Error) {
const newError = {
...error,
message: Object(error).body.message,
CoenWarmer marked this conversation as resolved.
Show resolved Hide resolved
};

return newError;
}
86 changes: 86 additions & 0 deletions x-pack/plugins/observability/public/hooks/slo/use_fetch_global_diagnosis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* 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; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import {
QueryObserverResult,
RefetchOptions,
RefetchQueryFilters,
useQuery,
} from '@tanstack/react-query';
import { i18n } from '@kbn/i18n';
import type { PublicLicenseJSON } from '@kbn/licensing-plugin/public';
import type { SecurityGetUserPrivilegesResponse } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import { useKibana } from '../../utils/kibana_react';
import { convertErrorForUseInToast } from './helpers/convert_error_for_use_in_toast';

interface SloGlobalDiagnosisResponse {
licenseAndFeatures: PublicLicenseJSON;
userPrivileges: SecurityGetUserPrivilegesResponse;
sloResources: {
[x: string]: 'OK' | 'NOT_OK';
};
}

export interface UseFetchSloGlobalDiagnoseResponse {
isInitialLoading: boolean;
isLoading: boolean;
isRefetching: boolean;
isSuccess: boolean;
isError: boolean;
globalSloDiagnosis: SloGlobalDiagnosisResponse | undefined;
refetch: <TPageData>(
options?: (RefetchOptions & RefetchQueryFilters<TPageData>) | undefined
) => Promise<QueryObserverResult<SloGlobalDiagnosisResponse | undefined, unknown>>;
}

export function useFetchSloGlobalDiagnosis(): UseFetchSloGlobalDiagnoseResponse {
const {
http,
notifications: { toasts },
} = useKibana().services;

const { isInitialLoading, isLoading, isError, isSuccess, isRefetching, data, refetch } = useQuery(
{
queryKey: ['fetchSloGlobalDiagnosis'],
queryFn: async ({ signal }) => {
try {
const response = await http.get<SloGlobalDiagnosisResponse>(
'/internal/observability/slos/_diagnosis',
{
query: {},
signal,
}
);

return response;
} catch (error) {
throw convertErrorForUseInToast(error);
}
},
keepPreviousData: true,
refetchOnWindowFocus: false,
retry: false,
onError: (error: Error) => {
toasts.addError(error, {
title: i18n.translate('xpack.observability.slo.globalDiagnosis.errorNotification', {
defaultMessage: 'You do not have the right permissions to use this feature.',
}),
});
Comment on lines +67 to +72
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically we rely on an error response coming from the API to consider the permission as insufficient?
I guess it works for now, but for 8.9 I would probably move this logic into the different SLO endpoints (create, update, etc..) and handle the 403 from there, returning a special error response, so we guarantee a universal experience on both UI and API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this can be improved.

The bug report mentioned disabling the "Create SLO" button on the Welcome page when no permissions are found, so we at least need this call to do that, not only on mutating endpoints.

},
}
);

return {
globalSloDiagnosis: data,
isLoading,
isInitialLoading,
isRefetching,
isSuccess,
isError,
refetch,
};
}
8 changes: 5 additions & 3 deletions x-pack/plugins/observability/public/pages/slo_edit/slo_edit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,18 @@ import { usePluginContext } from '../../hooks/use_plugin_context';
import { useBreadcrumbs } from '../../hooks/use_breadcrumbs';
import { useFetchSloDetails } from '../../hooks/slo/use_fetch_slo_details';
import { useLicense } from '../../hooks/use_license';
import { SloEditForm } from './components/slo_edit_form';
import { FeedbackButton } from '../../components/slo/feedback_button/feedback_button';
import { useCapabilities } from '../../hooks/slo/use_capabilities';
import { useFetchSloGlobalDiagnosis } from '../../hooks/slo/use_fetch_global_diagnosis';
import { FeedbackButton } from '../../components/slo/feedback_button/feedback_button';
import { SloEditForm } from './components/slo_edit_form';

export function SloEditPage() {
const {
application: { navigateToUrl },
http: { basePath },
} = useKibana().services;
const { hasWriteCapabilities } = useCapabilities();
const { isError: hasErrorInGlobalDiagnosis } = useFetchSloGlobalDiagnosis();
const { ObservabilityPageTemplate } = usePluginContext();

const { sloId } = useParams<{ sloId: string | undefined }>();
Expand All @@ -43,7 +45,7 @@ export function SloEditPage() {

const { slo, isInitialLoading } = useFetchSloDetails({ sloId });

if (hasRightLicense === false || !hasWriteCapabilities) {
if (hasRightLicense === false || !hasWriteCapabilities || hasErrorInGlobalDiagnosis) {
navigateToUrl(basePath.prepend(paths.observability.slos));
}

Expand Down
34 changes: 34 additions & 0 deletions x-pack/plugins/observability/public/pages/slos_welcome/slos_welcome.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { screen, waitFor } from '@testing-library/react';
import { render } from '../../utils/test_helper';
import { useKibana } from '../../utils/kibana_react';
import { useFetchSloList } from '../../hooks/slo/use_fetch_slo_list';
import { useFetchSloGlobalDiagnosis } from '../../hooks/slo/use_fetch_global_diagnosis';
import { useLicense } from '../../hooks/use_license';
import { SlosWelcomePage } from './slos_welcome';
import { emptySloList, sloList } from '../../data/slo/slo';
Expand All @@ -22,11 +23,13 @@ jest.mock('../../hooks/use_breadcrumbs');
jest.mock('../../hooks/use_license');
jest.mock('../../hooks/slo/use_fetch_slo_list');
jest.mock('../../hooks/slo/use_capabilities');
jest.mock('../../hooks/slo/use_fetch_global_diagnosis');

const useKibanaMock = useKibana as jest.Mock;
const useLicenseMock = useLicense as jest.Mock;
const useFetchSloListMock = useFetchSloList as jest.Mock;
const useCapabilitiesMock = useCapabilities as jest.Mock;
const useGlobalDiagnosisMock = useFetchSloGlobalDiagnosis as jest.Mock;

const mockNavigate = jest.fn();

Expand Down Expand Up @@ -54,6 +57,9 @@ describe('SLOs Welcome Page', () => {
it('renders the welcome message with subscription buttons', async () => {
useFetchSloListMock.mockReturnValue({ isLoading: false, sloList: emptySloList });
useLicenseMock.mockReturnValue({ hasAtLeast: () => false });
useGlobalDiagnosisMock.mockReturnValue({
isError: false,
});

render(<SlosWelcomePage />);

Expand All @@ -66,6 +72,9 @@ describe('SLOs Welcome Page', () => {
describe('when the correct license is found', () => {
beforeEach(() => {
useLicenseMock.mockReturnValue({ hasAtLeast: () => true });
useGlobalDiagnosisMock.mockReturnValue({
isError: false,
});
});

describe('when loading is done and no results are found', () => {
Expand All @@ -79,6 +88,24 @@ describe('SLOs Welcome Page', () => {
hasReadCapabilities: true,
});

render(<SlosWelcomePage />);

expect(screen.queryByTestId('slosPageWelcomePrompt')).toBeTruthy();

const createNewSloButton = screen.queryByTestId('o11ySloListWelcomePromptCreateSloButton');

expect(createNewSloButton).toBeDisabled();
});

it('disables the create slo button when no cluster permissions capabilities', async () => {
useCapabilitiesMock.mockReturnValue({
hasWriteCapabilities: true,
hasReadCapabilities: true,
});
useGlobalDiagnosisMock.mockReturnValue({
isError: true,
});

render(<SlosWelcomePage />);
expect(screen.queryByTestId('slosPageWelcomePrompt')).toBeTruthy();

Expand All @@ -87,6 +114,10 @@ describe('SLOs Welcome Page', () => {
});

it('should display the welcome message with a Create new SLO button which should navigate to the SLO Creation page', async () => {
useGlobalDiagnosisMock.mockReturnValue({
isError: false,
});

render(<SlosWelcomePage />);
expect(screen.queryByTestId('slosPageWelcomePrompt')).toBeTruthy();

Expand All @@ -103,6 +134,9 @@ describe('SLOs Welcome Page', () => {
describe('when loading is done and results are found', () => {
beforeEach(() => {
useFetchSloListMock.mockReturnValue({ isLoading: false, sloList });
useGlobalDiagnosisMock.mockReturnValue({
isError: false,
});
});

it('should navigate to the SLO List page', async () => {
Expand Down
9 changes: 6 additions & 3 deletions x-pack/plugins/observability/public/pages/slos_welcome/slos_welcome.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,19 @@ import { i18n } from '@kbn/i18n';
import { useKibana } from '../../utils/kibana_react';
import { useLicense } from '../../hooks/use_license';
import { usePluginContext } from '../../hooks/use_plugin_context';
import { useCapabilities } from '../../hooks/slo/use_capabilities';
import { useFetchSloList } from '../../hooks/slo/use_fetch_slo_list';
import { paths } from '../../config/paths';
import illustration from './assets/illustration.svg';
import { useCapabilities } from '../../hooks/slo/use_capabilities';
import { useFetchSloGlobalDiagnosis } from '../../hooks/slo/use_fetch_global_diagnosis';

export function SlosWelcomePage() {
const {
application: { navigateToUrl },
http: { basePath },
} = useKibana().services;
const { hasWriteCapabilities } = useCapabilities();
const { isError: hasErrorInGlobalDiagnosis } = useFetchSloGlobalDiagnosis();
const { ObservabilityPageTemplate } = usePluginContext();

const { hasAtLeast } = useLicense();
Expand All @@ -44,7 +46,8 @@ export function SlosWelcomePage() {
navigateToUrl(basePath.prepend(paths.observability.sloCreate));
};

const hasSlosAndHasPermissions = total > 0 && hasAtLeast('platinum') === true;
const hasSlosAndHasPermissions =
total > 0 && hasAtLeast('platinum') === true && !hasErrorInGlobalDiagnosis;

useEffect(() => {
if (hasSlosAndHasPermissions) {
Expand Down Expand Up @@ -110,7 +113,7 @@ export function SlosWelcomePage() {
fill
color="primary"
onClick={handleClickCreateSlo}
disabled={!hasWriteCapabilities}
disabled={!hasWriteCapabilities || hasErrorInGlobalDiagnosis}
>
{i18n.translate('xpack.observability.slo.sloList.welcomePrompt.buttonLabel', {
defaultMessage: 'Create SLO',
Expand Down
12 changes: 10 additions & 2 deletions x-pack/plugins/observability/server/routes/slo/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { badRequest } from '@hapi/boom';
import { badRequest, forbidden, failedDependency } from '@hapi/boom';
import {
createSLOParamsSchema,
deleteSLOParamsSchema,
Expand Down Expand Up @@ -275,7 +275,15 @@ const getDiagnosisRoute = createObservabilityServerRoute({
const esClient = (await context.core).elasticsearch.client.asCurrentUser;
const licensing = await context.licensing;

return getGlobalDiagnosis(esClient, licensing);
try {
const response = await getGlobalDiagnosis(esClient, licensing);
return response;
} catch (error) {
if (error.cause.statusCode === 403) {
throw forbidden('Insufficient Elasticsearch cluster permissions to access feature.');
}
throw failedDependency(error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this thing do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's one of HAPI Boom's methods to auto-format the response:

Boom.failedDependency([message], [data])
Returns a 424 Failed Dependency error where:

message - optional message.
data - optional additional error data.
Boom.failedDependency('an external resource failed');
Generates the following response payload:

{
    "statusCode": 424,
    "error": "Failed Dependency",
    "message": "an external resource failed"
}

https://hapi.dev/module/boom/api/?v=10.0.1#boomfaileddependencymessage-data

}
},
});

Expand Down
69 changes: 40 additions & 29 deletions x-pack/plugins/observability/server/services/slo/get_diagnosis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,19 @@ export async function getGlobalDiagnosis(
esClient: ElasticsearchClient,
licensing: LicensingApiRequestHandlerContext
) {
const licenseInfo = licensing.license.toJSON();
const userPrivileges = await esClient.security.getUserPrivileges();
const sloResources = await getSloResourcesDiagnosis(esClient);

return {
licenseAndFeatures: licenseInfo,
userPrivileges,
sloResources,
};
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this try catch is not needed if we are just throwing the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is needed.

const sloResources = await getSloResourcesDiagnosis(esClient); throws an error when ES has insufficient permissions, so if we don't throw here this function returns a normal object:

return {
    licenseAndFeatures: { something },
    userPrivileges: { something },
  	sloResources: { message: 'Some error message', statusCode: 403, ...etc }
};

const licenseInfo = licensing.license.toJSON();
const userPrivileges = await esClient.security.getUserPrivileges();
const sloResources = await getSloResourcesDiagnosis(esClient);

return {
licenseAndFeatures: licenseInfo,
userPrivileges,
sloResources,
};
} catch (error) {
throw error;
}
}

export async function getSloDiagnosis(
Expand Down Expand Up @@ -79,29 +83,36 @@ export async function getSloDiagnosis(
}

async function getSloResourcesDiagnosis(esClient: ElasticsearchClient) {
const indexTemplateExists = await esClient.indices.existsIndexTemplate({
name: SLO_INDEX_TEMPLATE_NAME,
});
try {
const indexTemplateExists = await esClient.indices.existsIndexTemplate({
name: SLO_INDEX_TEMPLATE_NAME,
});

const mappingsTemplateExists = await esClient.cluster.existsComponentTemplate({
name: SLO_COMPONENT_TEMPLATE_MAPPINGS_NAME,
});
const mappingsTemplateExists = await esClient.cluster.existsComponentTemplate({
name: SLO_COMPONENT_TEMPLATE_MAPPINGS_NAME,
});

const settingsTemplateExists = await esClient.cluster.existsComponentTemplate({
name: SLO_COMPONENT_TEMPLATE_SETTINGS_NAME,
});
const settingsTemplateExists = await esClient.cluster.existsComponentTemplate({
name: SLO_COMPONENT_TEMPLATE_SETTINGS_NAME,
});

let ingestPipelineExists = true;
try {
await esClient.ingest.getPipeline({ id: SLO_INGEST_PIPELINE_NAME });
let ingestPipelineExists = true;
try {
await esClient.ingest.getPipeline({ id: SLO_INGEST_PIPELINE_NAME });
} catch (err) {
ingestPipelineExists = false;
throw err;
}

return {
[SLO_INDEX_TEMPLATE_NAME]: indexTemplateExists ? OK : NOT_OK,
[SLO_COMPONENT_TEMPLATE_MAPPINGS_NAME]: mappingsTemplateExists ? OK : NOT_OK,
[SLO_COMPONENT_TEMPLATE_SETTINGS_NAME]: settingsTemplateExists ? OK : NOT_OK,
[SLO_INGEST_PIPELINE_NAME]: ingestPipelineExists ? OK : NOT_OK,
};
} catch (err) {
ingestPipelineExists = false;
if (err.meta.statusCode === 403) {
throw new Error('Insufficient permissions to access Elasticsearch Cluster', { cause: err });
}
}

return {
[SLO_INDEX_TEMPLATE_NAME]: indexTemplateExists ? OK : NOT_OK,
[SLO_COMPONENT_TEMPLATE_MAPPINGS_NAME]: mappingsTemplateExists ? OK : NOT_OK,
[SLO_COMPONENT_TEMPLATE_SETTINGS_NAME]: settingsTemplateExists ? OK : NOT_OK,
[SLO_INGEST_PIPELINE_NAME]: ingestPipelineExists ? OK : NOT_OK,
};
}