paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 29 Packages Projects Releases Wiki Activity

add better error handling (#2664)

Browse Source
This commit is contained in:
Matthew Clarke
2020-09-30 16:07:30 +01:00
committed by GitHub
parent 1fb8f1a93c
commit 44e18975bd
10 changed files with 503 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/kubernetes-backend/package.json
+1
View File
@@ -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",
plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts
+170 -26
View File
@@ -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<any>(['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,
},
);
});
});
plugins/kubernetes-backend/src/service/KubernetesFetcher.ts
+58 -9
View File
@@ -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<FetchResult[]> = 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<KubernetesObjectTypes>,
): Promise<FetchResponse[]> {
return Promise.all(
Array.from(objectTypesToFetch).map(type => {
return this.fetchByObjectType(serviceId, clusterDetails, type);
}),
);
): Promise<FetchResponseWrapper> {
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<T>(
clusterDetails: ClusterDetails,
fn: (client: Clients) => Promise<{ body: { items: Array<T> } }>,
fn: (
client: Clients,
) => Promise<{ body: { items: Array<T> }; response: http.IncomingMessage }>,
): Promise<Array<T>> {
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;
});
}
plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.test.ts
+35 -29
View File
@@ -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: [
plugins/kubernetes-backend/src/service/getKubernetesObjectsByServiceIdHandler.ts
+6 -2
View File
@@ -40,6 +40,7 @@ const DEFAULT_OBJECTS = new Set<KubernetesObjectTypes>([
'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,
};
});
}),
plugins/kubernetes-backend/src/service/router.ts
+3
View File
@@ -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 });
}
});
plugins/kubernetes-backend/src/types/types.ts
+18 -1
View File
@@ -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<KubernetesObjectTypes>,
): Promise<FetchResponse[]>;
): Promise<FetchResponseWrapper>;
}
// Used to locate which cluster(s) a service is running on
export interface KubernetesClusterLocator {
getClusterByServiceId(serviceId: string): Promise<ClusterDetails[]>;
}
export type KubernetesErrorTypes =
| 'UNAUTHORIZED_ERROR'
| 'SYSTEM_ERROR'
| 'UNKNOWN_ERROR';
export interface KubernetesFetchError {
errorType: KubernetesErrorTypes;
statusCode?: number;
resourcePath?: string;
}