From 44e18975bd0738e47872280a28793d38b93924ec Mon Sep 17 00:00:00 2001 From: Matthew Clarke Date: Wed, 30 Sep 2020 16:07:30 +0100 Subject: [PATCH] add better error handling (#2664) --- plugins/kubernetes-backend/package.json | 1 + .../src/service/KubernetesFetcher.test.ts | 196 +++++++++++++++--- .../src/service/KubernetesFetcher.ts | 67 +++++- ...ubernetesObjectsByServiceIdHandler.test.ts | 64 +++--- .../getKubernetesObjectsByServiceIdHandler.ts | 8 +- .../kubernetes-backend/src/service/router.ts | 3 + plugins/kubernetes-backend/src/types/types.ts | 19 +- .../KubernetesContent/ErrorPanel.test.tsx | 83 ++++++++ .../KubernetesContent/ErrorPanel.tsx | 65 ++++++ .../KubernetesContent/KubernetesContent.tsx | 97 ++++++--- 10 files changed, 503 insertions(+), 100 deletions(-) create mode 100644 plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.test.tsx create mode 100644 plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.tsx diff --git a/plugins/kubernetes-backend/package.json b/plugins/kubernetes-backend/package.json index e1f7a39592..4ff8149322 100644 --- a/plugins/kubernetes-backend/package.json +++ b/plugins/kubernetes-backend/package.json @@ -30,6 +30,7 @@ "express-promise-router": "^3.0.3", "fs-extra": "^9.0.0", "helmet": "^4.0.0", + "lodash": "^4.17.15", "morgan": "^1.10.0", "stream-buffers": "^3.0.2", "winston": "^3.2.1", diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 0109b12f36..8f390f309b 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -18,28 +18,83 @@ import { getVoidLogger } from '@backstage/backend-common'; import { KubernetesClientBasedFetcher } from './KubernetesFetcher'; describe('KubernetesClientProvider', () => { + let clientMock: any; + let kubernetesClientProvider: any; + let sut: KubernetesClientBasedFetcher; + beforeEach(() => { jest.resetAllMocks(); - }); - - it('should return pods, services', async () => { - const clientMock: any = { + clientMock = { listPodForAllNamespaces: jest.fn(), listServiceForAllNamespaces: jest.fn(), }; - const kubernetesClientProvider: any = { + kubernetesClientProvider = { getCoreClientByClusterDetails: jest.fn(() => clientMock), getAppsClientByClusterDetails: jest.fn(() => clientMock), getAutoscalingClientByClusterDetails: jest.fn(() => clientMock), getNetworkingBeta1Client: jest.fn(() => clientMock), }; - const sut = new KubernetesClientBasedFetcher({ + sut = new KubernetesClientBasedFetcher({ kubernetesClientProvider, logger: getVoidLogger(), }); + }); + const testErrorResponse = async (errorResponse: any, expectedResult: any) => { + clientMock.listPodForAllNamespaces.mockResolvedValueOnce({ + body: { + items: [ + { + metadata: { + name: 'pod-name', + }, + }, + ], + }, + }); + + clientMock.listServiceForAllNamespaces.mockRejectedValue(errorResponse); + + const result = await sut.fetchObjectsByServiceId( + 'some-service', + { + name: 'cluster1', + url: 'http://localhost:9999', + serviceAccountToken: undefined, + }, + new Set(['pods', 'services']), + ); + + expect(result).toStrictEqual({ + errors: [expectedResult], + responses: [ + { + type: 'pods', + resources: [ + { + metadata: { + name: 'pod-name', + }, + }, + ], + }, + ], + }); + + expect(clientMock.listPodForAllNamespaces.mock.calls.length).toBe(1); + expect(clientMock.listServiceForAllNamespaces.mock.calls.length).toBe(1); + + expect( + kubernetesClientProvider.getAppsClientByClusterDetails.mock.calls.length, + ).toBe(2); + expect( + kubernetesClientProvider.getCoreClientByClusterDetails.mock.calls.length, + ).toBe(2); + }; + + it('should return pods, services', async () => { clientMock.listPodForAllNamespaces.mockResolvedValueOnce({ body: { items: [ @@ -74,28 +129,31 @@ describe('KubernetesClientProvider', () => { new Set(['pods', 'services']), ); - expect(result).toStrictEqual([ - { - type: 'pods', - resources: [ - { - metadata: { - name: 'pod-name', + expect(result).toStrictEqual({ + errors: [], + responses: [ + { + type: 'pods', + resources: [ + { + metadata: { + name: 'pod-name', + }, }, - }, - ], - }, - { - type: 'services', - resources: [ - { - metadata: { - name: 'service-name', + ], + }, + { + type: 'services', + resources: [ + { + metadata: { + name: 'service-name', + }, }, - }, - ], - }, - ]); + ], + }, + ], + }); expect(clientMock.listPodForAllNamespaces.mock.calls.length).toBe(1); expect(clientMock.listServiceForAllNamespaces.mock.calls.length).toBe(1); @@ -107,4 +165,90 @@ describe('KubernetesClientProvider', () => { kubernetesClientProvider.getCoreClientByClusterDetails.mock.calls.length, ).toBe(2); }); + it('should throw error on unknown type', () => { + expect(() => + sut.fetchObjectsByServiceId( + 'some-service', + { + name: 'cluster1', + url: 'http://localhost:9999', + serviceAccountToken: undefined, + }, + new Set(['foo']), + ), + ).toThrow('unrecognised type=foo'); + + expect(clientMock.listPodForAllNamespaces.mock.calls.length).toBe(0); + expect(clientMock.listServiceForAllNamespaces.mock.calls.length).toBe(0); + + expect( + kubernetesClientProvider.getAppsClientByClusterDetails.mock.calls.length, + ).toBe(0); + expect( + kubernetesClientProvider.getCoreClientByClusterDetails.mock.calls.length, + ).toBe(0); + }); + // they're in testErrorResponse + // eslint-disable-next-line jest/expect-expect + it('should return pods, unauthorized error', async () => { + await testErrorResponse( + { + response: { + statusCode: 401, + request: { + uri: { + pathname: '/some/path', + }, + }, + }, + }, + { + errorType: 'UNAUTHORIZED_ERROR', + resourcePath: '/some/path', + statusCode: 401, + }, + ); + }); + // they're in testErrorResponse + // eslint-disable-next-line jest/expect-expect + it('should return pods, system error', async () => { + await testErrorResponse( + { + response: { + statusCode: 500, + request: { + uri: { + pathname: '/some/path', + }, + }, + }, + }, + { + errorType: 'SYSTEM_ERROR', + resourcePath: '/some/path', + statusCode: 500, + }, + ); + }); + // they're in testErrorResponse + // eslint-disable-next-line jest/expect-expect + it('should return pods, unknown error', async () => { + await testErrorResponse( + { + response: { + statusCode: 900, + request: { + uri: { + pathname: '/some/path', + }, + }, + }, + }, + { + errorType: 'UNKNOWN_ERROR', + resourcePath: '/some/path', + statusCode: 900, + }, + ); + }); }); diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts index 8890db7b13..f66a45816a 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import http from 'http'; import { AppsV1Api, AutoscalingV1Api, @@ -34,7 +35,11 @@ import { ClusterDetails, KubernetesObjectTypes, FetchResponse, + FetchResponseWrapper, + KubernetesFetchError, + KubernetesErrorTypes, } from '..'; +import lodash, { Dictionary } from 'lodash'; export interface Clients { core: CoreV1Api; @@ -48,6 +53,46 @@ export interface KubernetesClientBasedFetcherOptions { logger: Logger; } +type FetchResult = FetchResponse | KubernetesFetchError; + +const isError = (fr: FetchResult): fr is KubernetesFetchError => + fr.hasOwnProperty('errorType'); + +function fetchResultsToResponseWrapper( + results: FetchResult[], +): FetchResponseWrapper { + const groupBy: Dictionary = lodash.groupBy(results, value => { + return isError(value) ? 'errors' : 'responses'; + }); + + return { + errors: groupBy.errors ?? [], + responses: groupBy.responses ?? [], + } as FetchResponseWrapper; // TODO would be nice to get rid of this 'as' +} + +const statusCodeToErrorType = (statusCode: number): KubernetesErrorTypes => { + switch (statusCode) { + case 401: + return 'UNAUTHORIZED_ERROR'; + case 500: + return 'SYSTEM_ERROR'; + default: + return 'UNKNOWN_ERROR'; + } +}; + +const captureKubernetesErrorsRethrowOthers = (e: any): KubernetesFetchError => { + if (e.response && e.response.statusCode) { + return { + errorType: statusCodeToErrorType(e.response.statusCode), + statusCode: e.response.statusCode, + resourcePath: e.response.request.uri.pathname, + }; + } + throw e; +}; + export class KubernetesClientBasedFetcher implements KubernetesFetcher { private readonly kubernetesClientProvider: KubernetesClientProvider; private readonly logger: Logger; @@ -64,12 +109,14 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { serviceId: string, clusterDetails: ClusterDetails, objectTypesToFetch: Set, - ): Promise { - return Promise.all( - Array.from(objectTypesToFetch).map(type => { - return this.fetchByObjectType(serviceId, clusterDetails, type); - }), - ); + ): Promise { + const fetchResults = Array.from(objectTypesToFetch).map(type => { + return this.fetchByObjectType(serviceId, clusterDetails, type).catch( + captureKubernetesErrorsRethrowOthers, + ); + }); + + return Promise.all(fetchResults).then(fetchResultsToResponseWrapper); } // TODO could probably do with a tidy up @@ -122,7 +169,9 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { private singleClusterFetch( clusterDetails: ClusterDetails, - fn: (client: Clients) => Promise<{ body: { items: Array } }>, + fn: ( + client: Clients, + ) => Promise<{ body: { items: Array }; response: http.IncomingMessage }>, ): Promise> { const core = this.kubernetesClientProvider.getCoreClientByClusterDetails( clusterDetails, @@ -138,8 +187,8 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { ); this.logger.debug(`calling cluster=${clusterDetails.name}`); - return fn({ core, apps, autoscaling, networkingBeta1 }).then(result => { - return result.body.items; + return fn({ core, apps, autoscaling, networkingBeta1 }).then(({ body }) => { + return body.items; }); } diff --git a/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.test.ts b/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.test.ts index 3f8081571a..56da3235cc 100644 --- a/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.test.ts +++ b/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.test.ts @@ -26,38 +26,41 @@ const getClusterByServiceId = jest.fn(); const mockFetch = (mock: jest.Mock) => { mock.mockImplementation((serviceId: string, clusterDetails: ClusterDetails) => - Promise.resolve([ - { - type: 'pods', - resources: [ - { - metadata: { - name: `my-pods-${serviceId}-${clusterDetails.name}`, + Promise.resolve({ + errors: [], + responses: [ + { + type: 'pods', + resources: [ + { + metadata: { + name: `my-pods-${serviceId}-${clusterDetails.name}`, + }, }, - }, - ], - }, - { - type: 'configmaps', - resources: [ - { - metadata: { - name: `my-configmaps-${serviceId}-${clusterDetails.name}`, + ], + }, + { + type: 'configmaps', + resources: [ + { + metadata: { + name: `my-configmaps-${serviceId}-${clusterDetails.name}`, + }, }, - }, - ], - }, - { - type: 'services', - resources: [ - { - metadata: { - name: `my-services-${serviceId}-${clusterDetails.name}`, + ], + }, + { + type: 'services', + resources: [ + { + metadata: { + name: `my-services-${serviceId}-${clusterDetails.name}`, + }, }, - }, - ], - }, - ]), + ], + }, + ], + }), ); }; @@ -96,6 +99,7 @@ describe('handleGetKubernetesObjectsByServiceId', () => { cluster: { name: 'test-cluster', }, + errors: [], resources: [ { resources: [ @@ -166,6 +170,7 @@ describe('handleGetKubernetesObjectsByServiceId', () => { cluster: { name: 'test-cluster', }, + errors: [], resources: [ { resources: [ @@ -203,6 +208,7 @@ describe('handleGetKubernetesObjectsByServiceId', () => { cluster: { name: 'other-cluster', }, + errors: [], resources: [ { resources: [ diff --git a/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.ts b/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.ts index 552e083bd7..1690b8be40 100644 --- a/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.ts +++ b/plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.ts @@ -40,6 +40,7 @@ const DEFAULT_OBJECTS = new Set([ 'ingresses', ]); +// Fans out the request to all clusters that the service lives in, aggregates their responses together export const handleGetKubernetesObjectsByServiceId: GetKubernetesObjectsByServiceIdHandler = async ( serviceId, fetcher, @@ -50,7 +51,9 @@ export const handleGetKubernetesObjectsByServiceId: GetKubernetesObjectsByServic const clusterDetails = await clusterLocator.getClusterByServiceId(serviceId); logger.info( - `serviceId=${serviceId} clusterDetails=${clusterDetails.map(c => c.name)}`, + `serviceId=${serviceId} clusterDetails=[${clusterDetails + .map(c => c.name) + .join(', ')}]`, ); return Promise.all( @@ -62,7 +65,8 @@ export const handleGetKubernetesObjectsByServiceId: GetKubernetesObjectsByServic cluster: { name: cd.name, }, - resources: result, + resources: result.responses, + errors: result.errors, }; }); }), diff --git a/plugins/kubernetes-backend/src/service/router.ts b/plugins/kubernetes-backend/src/service/router.ts index d038c1ed7d..bdb14bb74d 100644 --- a/plugins/kubernetes-backend/src/service/router.ts +++ b/plugins/kubernetes-backend/src/service/router.ts @@ -74,6 +74,9 @@ export const makeRouter = ( ); res.send(response); } catch (e) { + logger.error( + `action=retrieveObjectsByServiceId service=${serviceId}, error=${e}`, + ); res.status(500).send({ error: e.message }); } }); diff --git a/plugins/kubernetes-backend/src/types/types.ts b/plugins/kubernetes-backend/src/types/types.ts index e469b2b1b5..0cb42eec1a 100644 --- a/plugins/kubernetes-backend/src/types/types.ts +++ b/plugins/kubernetes-backend/src/types/types.ts @@ -34,12 +34,18 @@ export interface ClusterDetails { export interface ClusterObjects { cluster: { name: string }; resources: FetchResponse[]; + errors: KubernetesFetchError[]; } export interface ObjectsByServiceIdResponse { items: ClusterObjects[]; } +export interface FetchResponseWrapper { + errors: KubernetesFetchError[]; + responses: FetchResponse[]; +} + export type FetchResponse = | PodFetchResponse | ServiceFetchResponse @@ -102,10 +108,21 @@ export interface KubernetesFetcher { serviceId: string, clusterDetails: ClusterDetails, objectTypesToFetch: Set, - ): Promise; + ): Promise; } // Used to locate which cluster(s) a service is running on export interface KubernetesClusterLocator { getClusterByServiceId(serviceId: string): Promise; } + +export type KubernetesErrorTypes = + | 'UNAUTHORIZED_ERROR' + | 'SYSTEM_ERROR' + | 'UNKNOWN_ERROR'; + +export interface KubernetesFetchError { + errorType: KubernetesErrorTypes; + statusCode?: number; + resourcePath?: string; +} diff --git a/plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.test.tsx b/plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.test.tsx new file mode 100644 index 0000000000..6985423a10 --- /dev/null +++ b/plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.test.tsx @@ -0,0 +1,83 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { render } from '@testing-library/react'; +import { wrapInTestApp } from '@backstage/test-utils'; +import { ErrorPanel } from './ErrorPanel'; + +describe('ErrorPanel', () => { + it('render with error message', async () => { + const { getByText } = render( + wrapInTestApp( + , + ), + ); + + // title + expect( + getByText( + 'There was an error retrieving some Kubernetes resources for the entity: THIS_ENTITY', + ), + ).toBeInTheDocument(); + + // message + expect(getByText('Errors: SOME_ERROR_MESSAGE')).toBeInTheDocument(); + }); + it('render with cluster errors', async () => { + const { getByText } = render( + wrapInTestApp( + , + ), + ); + + // title + expect( + getByText( + 'There was an error retrieving some Kubernetes resources for the entity: THIS_ENTITY', + ), + ).toBeInTheDocument(); + + // message + expect(getByText('Errors:')).toBeInTheDocument(); + expect(getByText('Cluster: THIS_CLUSTER')).toBeInTheDocument(); + expect( + getByText( + "Error fetching Kubernetes resource: 'some/resource', error: SYSTEM_ERROR", + ), + ).toBeInTheDocument(); + }); +}); diff --git a/plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.tsx b/plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.tsx new file mode 100644 index 0000000000..1fe5714b09 --- /dev/null +++ b/plugins/kubernetes/src/components/KubernetesContent/ErrorPanel.tsx @@ -0,0 +1,65 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { WarningPanel } from '@backstage/core'; +import { Typography } from '@material-ui/core'; +import { ClusterObjects } from '../../../../kubernetes-backend/src'; + +const clustersWithErrorsToErrorMessage = ( + clustersWithErrors: ClusterObjects[], +): React.ReactNode => { + return clustersWithErrors.map((c, i) => { + return ( +
+ {`Cluster: ${c.cluster.name}`} + {c.errors.map((e, j) => { + return ( + + {`Error fetching Kubernetes resource: '${e.resourcePath}', error: ${e.errorType}`} + + ); + })} +
+
+ ); + }); +}; + +type ErrorPanelProps = { + entityName: string; + errorMessage?: string; + clustersWithErrors?: ClusterObjects[]; + children?: React.ReactNode; +}; + +export const ErrorPanel = ({ + entityName, + errorMessage, + clustersWithErrors, +}: ErrorPanelProps) => ( + + {clustersWithErrors && ( +
Errors: {clustersWithErrorsToErrorMessage(clustersWithErrors)}
+ )} + {errorMessage && ( + Errors: {errorMessage} + )} + +); diff --git a/plugins/kubernetes/src/components/KubernetesContent/KubernetesContent.tsx b/plugins/kubernetes/src/components/KubernetesContent/KubernetesContent.tsx index b926d92dc7..6119884c43 100644 --- a/plugins/kubernetes/src/components/KubernetesContent/KubernetesContent.tsx +++ b/plugins/kubernetes/src/components/KubernetesContent/KubernetesContent.tsx @@ -14,8 +14,8 @@ * limitations under the License. */ -import React, { useEffect, useState } from 'react'; -import { Grid } from '@material-ui/core'; +import React, { ReactElement, useEffect, useState } from 'react'; +import { Grid, TabProps } from '@material-ui/core'; import { CardTab, Content, @@ -44,6 +44,7 @@ import { Services } from '../Services'; import { ConfigMaps } from '../ConfigMaps'; import { Ingresses } from '../Ingresses'; import { HorizontalPodAutoscalers } from '../HorizontalPodAutoscalers'; +import { ErrorPanel } from './ErrorPanel'; interface GroupedResponses extends DeploymentTriple { services: V1Service[]; @@ -94,8 +95,6 @@ const groupResponses = (fetchResponse: FetchResponse[]) => { ); }; -// TODO proper error handling - type KubernetesContentProps = { entity: Entity; children?: React.ReactNode }; export const KubernetesContent = ({ entity }: KubernetesContentProps) => { @@ -117,12 +116,33 @@ export const KubernetesContent = ({ entity }: KubernetesContentProps) => { }); }, [entity.metadata.name, kubernetesApi]); + const clustersWithErrors = + kubernetesObjects?.items.filter(r => r.errors.length > 0) ?? []; + return ( - {kubernetesObjects === undefined && } - {error !== undefined &&
{error}
} + {kubernetesObjects === undefined && error === undefined && ( + + )} + + {/* errors retrieved from the kubernetes clusters */} + {clustersWithErrors.length > 0 && ( + + )} + + {/* other errors */} + {error !== undefined && ( + + )} + {kubernetesObjects?.items.map((item, i) => ( @@ -151,6 +171,43 @@ const Cluster = ({ clusterObjects }: ClusterProps) => { const hpas = groupedResponses.horizontalPodAutoscalers; const ingresses = groupedResponses.ingresses; + const tabs: ReactElement[] = [ + + + , + + + , + ]; + + if (configMaps.length > 0) { + tabs.push( + + + , + ); + } + if (hpas.length > 0) { + tabs.push( + + + , + ); + } + if (ingresses.length > 0) { + tabs.push( + + + , + ); + } + return ( <> { onChange={handleChange} title={clusterObjects.cluster.name} > - - - - - - - {configMaps && ( - - - - )} - {hpas && ( - - - - )} - {ingresses && ( - - - - )} + {tabs} );