From f7ccaef866387918ec819555d3f878b14ac1ba7a Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 23 Nov 2022 17:48:19 -0500 Subject: [PATCH 01/15] convert fetchObjectsForService mocks to msw this will allow some dramatic refactoring Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 462 ++++++++---------- 1 file changed, 197 insertions(+), 265 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index c2560e58b0..5fd9c9bf83 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -16,8 +16,12 @@ import { getVoidLogger } from '@backstage/backend-common'; import { KubernetesClientBasedFetcher } from './KubernetesFetcher'; +import { KubernetesClientProvider } from './KubernetesClientProvider'; import { ObjectToFetch } from '../types/types'; import { topPods } from '@kubernetes/client-node'; +import { MockedRequest, rest } from 'msw'; +import { setupServer } from 'msw/node'; +import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; jest.mock('@kubernetes/client-node', () => ({ ...jest.requireActual('@kubernetes/client-node'), @@ -56,45 +60,43 @@ const POD_METRICS_FIXTURE = { describe('KubernetesFetcher', () => { describe('fetchObjectsForService', () => { - let clientMock: any; - let kubernetesClientProvider: any; let sut: KubernetesClientBasedFetcher; + const worker = setupServer(); + setupRequestMockHandlers(worker); - beforeEach(() => { - jest.resetAllMocks(); - clientMock = { - listClusterCustomObject: jest.fn(), - listNamespacedCustomObject: jest.fn(), - addInterceptor: jest.fn(), - }; - - kubernetesClientProvider = { - getCustomObjectsClient: jest.fn(() => clientMock), - }; - - sut = new KubernetesClientBasedFetcher({ - kubernetesClientProvider, - logger: getVoidLogger(), - }); - }); + const labels = (req: MockedRequest): object => { + const selectorParam = req.url.searchParams.get('labelSelector'); + if (selectorParam) { + const [key, value] = selectorParam.split('='); + return { [key]: value }; + } + return {}; + }; const testErrorResponse = async ( errorResponse: any, expectedResult: any, ) => { - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'pod-name', - }, - }, - ], - }, - }); - - clientMock.listClusterCustomObject.mockRejectedValue(errorResponse); + worker.use( + rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + ctx.json({ + items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + }), + ), + ), + rest.get('http://localhost:9999/api/v1/services', (_, res, ctx) => { + return res( + ctx.status(errorResponse.response.statusCode), + ctx.json({ + kind: 'Status', + apiVersion: 'v1', + status: 'Failure', + code: errorResponse.response.statusCode, + }), + ); + }), + ); const result = await sut.fetchObjectsForService({ serviceId: 'some-service', @@ -118,66 +120,41 @@ describe('KubernetesFetcher', () => { { metadata: { name: 'pod-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, }, }, ], }, ], }); - - expect(clientMock.listClusterCustomObject.mock.calls.length).toBe(2); - - expect(clientMock.listClusterCustomObject.mock.calls[0]).toEqual([ - '', - 'v1', - 'pods', - '', - false, - '', - '', - 'backstage.io/kubernetes-id=some-service', - ]); - - expect(clientMock.listClusterCustomObject.mock.calls[1]).toEqual([ - '', - 'v1', - 'services', - '', - false, - '', - '', - 'backstage.io/kubernetes-id=some-service', - ]); - - expect( - kubernetesClientProvider.getCustomObjectsClient.mock.calls.length, - ).toBe(2); }; - it('should return pods, services', async () => { - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'pod-name', - }, - }, - ], - }, + beforeEach(() => { + sut = new KubernetesClientBasedFetcher({ + kubernetesClientProvider: new KubernetesClientProvider(), + logger: getVoidLogger(), }); + }); - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'service-name', - }, - }, - ], - }, - }); + it('should return pods, services', async () => { + worker.use( + rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + ctx.json({ + items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + }), + ), + ), + rest.get('http://localhost:9999/api/v1/services', (req, res, ctx) => + res( + ctx.json({ + items: [ + { metadata: { name: 'service-name', labels: labels(req) } }, + ], + }), + ), + ), + ); const result = await sut.fetchObjectsForService({ serviceId: 'some-service', @@ -201,6 +178,7 @@ describe('KubernetesFetcher', () => { { metadata: { name: 'pod-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, }, }, ], @@ -211,77 +189,44 @@ describe('KubernetesFetcher', () => { { metadata: { name: 'service-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, }, }, ], }, ], }); - - expect(clientMock.listClusterCustomObject.mock.calls.length).toBe(2); - - expect(clientMock.listClusterCustomObject.mock.calls[0]).toEqual([ - '', - 'v1', - 'pods', - '', - false, - '', - '', - 'backstage.io/kubernetes-id=some-service', - ]); - - expect(clientMock.listClusterCustomObject.mock.calls[1]).toEqual([ - '', - 'v1', - 'services', - '', - false, - '', - '', - 'backstage.io/kubernetes-id=some-service', - ]); - - expect( - kubernetesClientProvider.getCustomObjectsClient.mock.calls.length, - ).toBe(2); }); it('should return pods, services and customobjects', async () => { - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'pod-name', - }, - }, - ], - }, - }); - - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'service-name', - }, - }, - ], - }, - }); - - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'something-else', - }, - }, - ], - }, - }); + worker.use( + rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + ctx.json({ + items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + }), + ), + ), + rest.get('http://localhost:9999/api/v1/services', (req, res, ctx) => + res( + ctx.json({ + items: [ + { metadata: { name: 'service-name', labels: labels(req) } }, + ], + }), + ), + ), + rest.get( + 'http://localhost:9999/apis/some-group/v2/things', + (req, res, ctx) => + res( + ctx.json({ + items: [ + { metadata: { name: 'something-else', labels: labels(req) } }, + ], + }), + ), + ), + ); const result = await sut.fetchObjectsForService({ serviceId: 'some-service', @@ -312,6 +257,7 @@ describe('KubernetesFetcher', () => { { metadata: { name: 'pod-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, }, }, ], @@ -322,6 +268,7 @@ describe('KubernetesFetcher', () => { { metadata: { name: 'service-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, }, }, ], @@ -332,51 +279,13 @@ describe('KubernetesFetcher', () => { { metadata: { name: 'something-else', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, }, }, ], }, ], }); - - expect(clientMock.listClusterCustomObject.mock.calls.length).toBe(3); - - expect(clientMock.listClusterCustomObject.mock.calls[0]).toEqual([ - '', - 'v1', - 'pods', - '', - false, - '', - '', - 'backstage.io/kubernetes-id=some-service', - ]); - - expect(clientMock.listClusterCustomObject.mock.calls[1]).toEqual([ - '', - 'v1', - 'services', - '', - false, - '', - '', - 'backstage.io/kubernetes-id=some-service', - ]); - - expect(clientMock.listClusterCustomObject.mock.calls[2]).toEqual([ - 'some-group', - 'v2', - 'things', - '', - false, - '', - '', - 'backstage.io/kubernetes-id=some-service', - ]); - - expect( - kubernetesClientProvider.getCustomObjectsClient.mock.calls.length, - ).toBe(3); }); // they're in testErrorResponse // eslint-disable-next-line jest/expect-expect @@ -385,16 +294,11 @@ describe('KubernetesFetcher', () => { { response: { statusCode: 400, - request: { - uri: { - pathname: '/some/path', - }, - }, }, }, { errorType: 'BAD_REQUEST', - resourcePath: '/some/path', + resourcePath: '/api/v1/services', statusCode: 400, }, ); @@ -406,16 +310,11 @@ describe('KubernetesFetcher', () => { { response: { statusCode: 401, - request: { - uri: { - pathname: '/some/path', - }, - }, }, }, { errorType: 'UNAUTHORIZED_ERROR', - resourcePath: '/some/path', + resourcePath: '/api/v1/services', statusCode: 401, }, ); @@ -427,16 +326,11 @@ describe('KubernetesFetcher', () => { { response: { statusCode: 500, - request: { - uri: { - pathname: '/some/path', - }, - }, }, }, { errorType: 'SYSTEM_ERROR', - resourcePath: '/some/path', + resourcePath: '/api/v1/services', statusCode: 500, }, ); @@ -448,46 +342,36 @@ describe('KubernetesFetcher', () => { { response: { statusCode: 900, - request: { - uri: { - pathname: '/some/path', - }, - }, }, }, { errorType: 'UNKNOWN_ERROR', - resourcePath: '/some/path', + resourcePath: '/api/v1/services', statusCode: 900, }, ); }); - it('should always add a labelSelector query', async () => { - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'pod-name', - }, - }, - ], - }, - }); + it('should respect labelSelector', async () => { + worker.use( + rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + ctx.json({ + items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + }), + ), + ), + rest.get('http://localhost:9999/api/v1/services', (req, res, ctx) => + res( + ctx.json({ + items: [ + { metadata: { name: 'service-name', labels: labels(req) } }, + ], + }), + ), + ), + ); - clientMock.listClusterCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'service-name', - }, - }, - ], - }, - }); - - await sut.fetchObjectsForService({ + const result = await sut.fetchObjectsForService({ serviceId: 'some-service', clusterDetails: { name: 'cluster1', @@ -496,41 +380,65 @@ describe('KubernetesFetcher', () => { authProvider: 'serviceAccount', }, objectTypesToFetch: OBJECTS_TO_FETCH, - labelSelector: '', + labelSelector: 'service-label=value', customResources: [], }); - const mockCall = clientMock.listClusterCustomObject.mock.calls[0]; - const actualSelector = mockCall[mockCall.length - 1]; - const expectedSelector = 'backstage.io/kubernetes-id=some-service'; - expect(actualSelector).toBe(expectedSelector); + expect(result).toStrictEqual({ + errors: [], + responses: [ + { + type: 'pods', + resources: [ + { + metadata: { + name: 'pod-name', + labels: { 'service-label': 'value' }, + }, + }, + ], + }, + { + type: 'services', + resources: [ + { + metadata: { + name: 'service-name', + labels: { 'service-label': 'value' }, + }, + }, + ], + }, + ], + }); }); it('should use namespace if provided', async () => { - clientMock.listNamespacedCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'pod-name', - }, - }, - ], - }, - }); + worker.use( + rest.get( + 'http://localhost:9999/api/v1/namespaces/some-namespace/pods', + (req, res, ctx) => + res( + ctx.json({ + items: [ + { metadata: { name: 'pod-name', labels: labels(req) } }, + ], + }), + ), + ), + rest.get( + 'http://localhost:9999/api/v1/namespaces/some-namespace/services', + (req, res, ctx) => + res( + ctx.json({ + items: [ + { metadata: { name: 'service-name', labels: labels(req) } }, + ], + }), + ), + ), + ); - clientMock.listNamespacedCustomObject.mockResolvedValueOnce({ - body: { - items: [ - { - metadata: { - name: 'service-name', - }, - }, - ], - }, - }); - - await sut.fetchObjectsForService({ + const result = await sut.fetchObjectsForService({ serviceId: 'some-service', clusterDetails: { name: 'cluster1', @@ -544,9 +452,33 @@ describe('KubernetesFetcher', () => { customResources: [], }); - const mockCall = clientMock.listNamespacedCustomObject.mock.calls[0]; - const namespace = mockCall[2]; - expect(namespace).toBe('some-namespace'); + expect(result).toStrictEqual({ + errors: [], + responses: [ + { + type: 'pods', + resources: [ + { + metadata: { + name: 'pod-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, + }, + }, + ], + }, + { + type: 'services', + resources: [ + { + metadata: { + name: 'service-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, + }, + }, + ], + }, + ], + }); }); }); From d31fe286250a82d56986740edda9d8baa7d8aeb8 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Tue, 29 Nov 2022 07:35:50 -0500 Subject: [PATCH 02/15] backfill test for loading default kubeconfig Strictly speaking, the `loadFromDefault` method on `KubeConfig` as it is being called in `KubernetesClientProvider` has many other behaviours that this test does not exercise -- it has logic for reading kubeconfig files (based on an env var or default location in a home directory), provisions for running on Windows, and even contacting an apiserver on localhost. I'm making a breaking assumption here that these other scenarios are not important for Backstage users, and the one that really matters is the in-cluster one described in this test. Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 5fd9c9bf83..9984c66cc5 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -22,6 +22,7 @@ import { topPods } from '@kubernetes/client-node'; import { MockedRequest, rest } from 'msw'; import { setupServer } from 'msw/node'; import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; +import mockFs from 'mock-fs'; jest.mock('@kubernetes/client-node', () => ({ ...jest.requireActual('@kubernetes/client-node'), @@ -480,6 +481,73 @@ describe('KubernetesFetcher', () => { ], }); }); + describe('Backstage running on k8s', () => { + const initialHost = process.env.KUBERNETES_SERVICE_HOST; + const initialPort = process.env.KUBERNETES_SERVICE_PORT; + afterEach(() => { + process.env.KUBERNETES_SERVICE_HOST = initialHost; + process.env.KUBERNETES_SERVICE_PORT = initialPort; + mockFs.restore(); + }); + it('makes in-cluster requests when cluster details has no token', async () => { + process.env.KUBERNETES_SERVICE_HOST = '10.10.10.10'; + process.env.KUBERNETES_SERVICE_PORT = '443'; + mockFs({ + '/var/run/secrets/kubernetes.io/serviceaccount/ca.crt': '', + '/var/run/secrets/kubernetes.io/serviceaccount/token': + 'allowed-token', + }); + worker.use( + rest.get('https://10.10.10.10/api/v1/pods', (req, res, ctx) => + req.headers.get('Authorization') === 'Bearer allowed-token' + ? res( + ctx.json({ + items: [ + { metadata: { name: 'pod-name', labels: labels(req) } }, + ], + }), + ) + : res(ctx.status(403)), + ), + ); + + const result = await sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'overridden-to-in-cluster', + url: 'http://ignored', + authProvider: 'serviceAccount', + }, + objectTypesToFetch: new Set([ + { + group: '', + apiVersion: 'v1', + plural: 'pods', + objectType: 'pods', + }, + ]), + labelSelector: '', + customResources: [], + }); + + expect(result).toStrictEqual({ + errors: [], + responses: [ + { + type: 'pods', + resources: [ + { + metadata: { + name: 'pod-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, + }, + }, + ], + }, + ], + }); + }); + }); }); describe('fetchPodMetricsByNamespaces', () => { From 15ad4b238952581e2612c35ee1ccb6095f06f28d Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Tue, 29 Nov 2022 09:06:59 -0500 Subject: [PATCH 03/15] stub k8s API checks bearer tokens Also, use idiomatic ResponseTransformers to clean up the individual tests. Finally, modify the "unauthorized error handling" spec to provide an invalid token to an otherwise-realistic stub k8s API. Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 177 ++++++++++++------ 1 file changed, 123 insertions(+), 54 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 9984c66cc5..3bd38a4993 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -19,7 +19,13 @@ import { KubernetesClientBasedFetcher } from './KubernetesFetcher'; import { KubernetesClientProvider } from './KubernetesClientProvider'; import { ObjectToFetch } from '../types/types'; import { topPods } from '@kubernetes/client-node'; -import { MockedRequest, rest } from 'msw'; +import { + MockedRequest, + RestContext, + ResponseTransformer, + compose, + rest, +} from 'msw'; import { setupServer } from 'msw/node'; import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; import mockFs from 'mock-fs'; @@ -73,6 +79,37 @@ describe('KubernetesFetcher', () => { } return {}; }; + const checkToken = ( + req: MockedRequest, + ctx: RestContext, + token: string, + ): ResponseTransformer => { + switch (req.headers.get('Authorization')) { + case `Bearer ${token}`: + return ctx.status(200); + default: + return compose( + ctx.status(401), + ctx.json({ + kind: 'Status', + apiVersion: 'v1', + code: 401, + }), + ); + } + }; + const withLabels = ( + req: MockedRequest, + ctx: RestContext, + body: { items: { metadata: object }[] }, + ): ResponseTransformer => + ctx.json({ + ...body, + items: body.items.map(item => ({ + ...item, + metadata: { ...item.metadata, labels: labels(req) }, + })), + }); const testErrorResponse = async ( errorResponse: any, @@ -81,8 +118,9 @@ describe('KubernetesFetcher', () => { worker.use( rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => res( - ctx.json({ - items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], }), ), ), @@ -141,17 +179,17 @@ describe('KubernetesFetcher', () => { worker.use( rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => res( - ctx.json({ - items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], }), ), ), rest.get('http://localhost:9999/api/v1/services', (req, res, ctx) => res( - ctx.json({ - items: [ - { metadata: { name: 'service-name', labels: labels(req) } }, - ], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'service-name' } }], }), ), ), @@ -202,17 +240,17 @@ describe('KubernetesFetcher', () => { worker.use( rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => res( - ctx.json({ - items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], }), ), ), rest.get('http://localhost:9999/api/v1/services', (req, res, ctx) => res( - ctx.json({ - items: [ - { metadata: { name: 'service-name', labels: labels(req) } }, - ], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'service-name' } }], }), ), ), @@ -220,10 +258,9 @@ describe('KubernetesFetcher', () => { 'http://localhost:9999/apis/some-group/v2/things', (req, res, ctx) => res( - ctx.json({ - items: [ - { metadata: { name: 'something-else', labels: labels(req) } }, - ], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'something-else' } }], }), ), ), @@ -304,21 +341,56 @@ describe('KubernetesFetcher', () => { }, ); }); - // they're in testErrorResponse - // eslint-disable-next-line jest/expect-expect it('should return pods, unauthorized error', async () => { - await testErrorResponse( - { - response: { + worker.use( + rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], + }), + ), + ), + rest.get('http://localhost:9999/api/v1/services', (req, res, ctx) => + res(checkToken(req, ctx, 'other-token')), + ), + ); + + const result = await sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'cluster1', + url: 'http://localhost:9999', + serviceAccountToken: 'token', + authProvider: 'serviceAccount', + }, + objectTypesToFetch: OBJECTS_TO_FETCH, + labelSelector: '', + customResources: [], + }); + + expect(result).toStrictEqual({ + errors: [ + { + errorType: 'UNAUTHORIZED_ERROR', + resourcePath: '/api/v1/services', statusCode: 401, }, - }, - { - errorType: 'UNAUTHORIZED_ERROR', - resourcePath: '/api/v1/services', - statusCode: 401, - }, - ); + ], + responses: [ + { + type: 'pods', + resources: [ + { + metadata: { + name: 'pod-name', + labels: { 'backstage.io/kubernetes-id': 'some-service' }, + }, + }, + ], + }, + ], + }); }); // they're in testErrorResponse // eslint-disable-next-line jest/expect-expect @@ -356,17 +428,17 @@ describe('KubernetesFetcher', () => { worker.use( rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => res( - ctx.json({ - items: [{ metadata: { name: 'pod-name', labels: labels(req) } }], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], }), ), ), rest.get('http://localhost:9999/api/v1/services', (req, res, ctx) => res( - ctx.json({ - items: [ - { metadata: { name: 'service-name', labels: labels(req) } }, - ], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'service-name' } }], }), ), ), @@ -419,10 +491,9 @@ describe('KubernetesFetcher', () => { 'http://localhost:9999/api/v1/namespaces/some-namespace/pods', (req, res, ctx) => res( - ctx.json({ - items: [ - { metadata: { name: 'pod-name', labels: labels(req) } }, - ], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], }), ), ), @@ -430,10 +501,9 @@ describe('KubernetesFetcher', () => { 'http://localhost:9999/api/v1/namespaces/some-namespace/services', (req, res, ctx) => res( - ctx.json({ - items: [ - { metadata: { name: 'service-name', labels: labels(req) } }, - ], + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'service-name' } }], }), ), ), @@ -499,15 +569,14 @@ describe('KubernetesFetcher', () => { }); worker.use( rest.get('https://10.10.10.10/api/v1/pods', (req, res, ctx) => - req.headers.get('Authorization') === 'Bearer allowed-token' - ? res( - ctx.json({ - items: [ - { metadata: { name: 'pod-name', labels: labels(req) } }, - ], - }), - ) - : res(ctx.status(403)), + res( + checkToken(req, ctx, 'allowed-token'), + withLabels(req, ctx, { + items: [ + { metadata: { name: 'pod-name', labels: labels(req) } }, + ], + }), + ), ), ); From 030f9cb5c1caa2d30cb5341c64bb4b3183b5227a Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Tue, 29 Nov 2022 09:29:47 -0500 Subject: [PATCH 04/15] backfill test for non-kubernetes error Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 3bd38a4993..8449a1716d 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -424,6 +424,40 @@ describe('KubernetesFetcher', () => { }, ); }); + it('fails on a network error', async () => { + worker.use( + rest.get('http://badurl.does.not.exist/api/v1/pods', (_, res) => + res.networkError('getaddrinfo ENOTFOUND badurl.does.not.exist'), + ), + rest.get( + 'http://badurl.does.not.exist/api/v1/services', + (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'service-name' } }], + }), + ), + ), + ); + + const result = sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'cluster1', + url: 'http://badurl.does.not.exist', + serviceAccountToken: 'token', + authProvider: 'serviceAccount', + }, + objectTypesToFetch: OBJECTS_TO_FETCH, + labelSelector: '', + customResources: [], + }); + + await expect(result).rejects.toThrow( + 'getaddrinfo ENOTFOUND badurl.does.not.exist', + ); + }); it('should respect labelSelector', async () => { worker.use( rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => From 7e2cd3c3579606f93f90cbeec251c1a26ee1dc6d Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Tue, 29 Nov 2022 13:20:38 -0500 Subject: [PATCH 05/15] backfill test for warning log Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 8449a1716d..0fc832e7bd 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -68,6 +68,7 @@ const POD_METRICS_FIXTURE = { describe('KubernetesFetcher', () => { describe('fetchObjectsForService', () => { let sut: KubernetesClientBasedFetcher; + const logger = getVoidLogger(); const worker = setupServer(); setupRequestMockHandlers(worker); @@ -171,7 +172,7 @@ describe('KubernetesFetcher', () => { beforeEach(() => { sut = new KubernetesClientBasedFetcher({ kubernetesClientProvider: new KubernetesClientProvider(), - logger: getVoidLogger(), + logger, }); }); @@ -341,7 +342,8 @@ describe('KubernetesFetcher', () => { }, ); }); - it('should return pods, unauthorized error', async () => { + it('should return pods and unauthorized error, logging a warning', async () => { + const warn = jest.spyOn(logger, 'warn'); worker.use( rest.get('http://localhost:9999/api/v1/pods', (req, res, ctx) => res( @@ -391,6 +393,9 @@ describe('KubernetesFetcher', () => { }, ], }); + expect(warn).toHaveBeenCalledWith( + 'statusCode=401 for resource /api/v1/services body=[{"kind":"Status","apiVersion":"v1","code":401}]', + ); }); // they're in testErrorResponse // eslint-disable-next-line jest/expect-expect From df716f5ed5a766636a9f257860b54ca9de146944 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Tue, 29 Nov 2022 15:30:30 -0500 Subject: [PATCH 06/15] metrics tests use msw instead of mocks Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 303 +++++++++++------- 1 file changed, 189 insertions(+), 114 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 0fc832e7bd..db318b4d0c 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -18,7 +18,6 @@ import { getVoidLogger } from '@backstage/backend-common'; import { KubernetesClientBasedFetcher } from './KubernetesFetcher'; import { KubernetesClientProvider } from './KubernetesClientProvider'; import { ObjectToFetch } from '../types/types'; -import { topPods } from '@kubernetes/client-node'; import { MockedRequest, RestContext, @@ -30,11 +29,6 @@ import { setupServer } from 'msw/node'; import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; import mockFs from 'mock-fs'; -jest.mock('@kubernetes/client-node', () => ({ - ...jest.requireActual('@kubernetes/client-node'), - topPods: jest.fn(), -})); - const OBJECTS_TO_FETCH = new Set([ { group: '', @@ -50,67 +44,69 @@ const OBJECTS_TO_FETCH = new Set([ }, ]); -const POD_METRICS_FIXTURE = { - containers: [], - cpu: { - currentUsage: 100, - limitTotal: 102, - requestTotal: 101, +const POD_METRICS_FIXTURE = [ + { + type: 'podstatus', + resources: [ + { + CPU: { CurrentUsage: 0, LimitTotal: 1, RequestTotal: 0.5 }, + Memory: { + CurrentUsage: 0, + LimitTotal: 1000000000n, + RequestTotal: 512000000n, + }, + }, + ], }, - memory: { - currentUsage: '1000', - limitTotal: '1002', - requestTotal: '1001', - }, - pod: {}, -}; +]; describe('KubernetesFetcher', () => { + const worker = setupServer(); + setupRequestMockHandlers(worker); + + const labels = (req: MockedRequest): object => { + const selectorParam = req.url.searchParams.get('labelSelector'); + if (selectorParam) { + const [key, value] = selectorParam.split('='); + return { [key]: value }; + } + return {}; + }; + const checkToken = ( + req: MockedRequest, + ctx: RestContext, + token: string, + ): ResponseTransformer => { + switch (req.headers.get('Authorization')) { + case `Bearer ${token}`: + return ctx.status(200); + default: + return compose( + ctx.status(401), + ctx.json({ + kind: 'Status', + apiVersion: 'v1', + code: 401, + }), + ); + } + }; + const withLabels = ( + req: MockedRequest, + ctx: RestContext, + body: T, + ): ResponseTransformer => + ctx.json({ + ...body, + items: body.items.map(item => ({ + ...item, + metadata: { ...item.metadata, labels: labels(req) }, + })), + }); + describe('fetchObjectsForService', () => { let sut: KubernetesClientBasedFetcher; const logger = getVoidLogger(); - const worker = setupServer(); - setupRequestMockHandlers(worker); - - const labels = (req: MockedRequest): object => { - const selectorParam = req.url.searchParams.get('labelSelector'); - if (selectorParam) { - const [key, value] = selectorParam.split('='); - return { [key]: value }; - } - return {}; - }; - const checkToken = ( - req: MockedRequest, - ctx: RestContext, - token: string, - ): ResponseTransformer => { - switch (req.headers.get('Authorization')) { - case `Bearer ${token}`: - return ctx.status(200); - default: - return compose( - ctx.status(401), - ctx.json({ - kind: 'Status', - apiVersion: 'v1', - code: 401, - }), - ); - } - }; - const withLabels = ( - req: MockedRequest, - ctx: RestContext, - body: { items: { metadata: object }[] }, - ): ResponseTransformer => - ctx.json({ - ...body, - items: body.items.map(item => ({ - ...item, - metadata: { ...item.metadata, labels: labels(req) }, - })), - }); const testErrorResponse = async ( errorResponse: any, @@ -611,9 +607,7 @@ describe('KubernetesFetcher', () => { res( checkToken(req, ctx, 'allowed-token'), withLabels(req, ctx, { - items: [ - { metadata: { name: 'pod-name', labels: labels(req) } }, - ], + items: [{ metadata: { name: 'pod-name' } }], }), ), ), @@ -659,25 +653,63 @@ describe('KubernetesFetcher', () => { }); describe('fetchPodMetricsByNamespaces', () => { - let kubernetesClientProvider: any; let sut: KubernetesClientBasedFetcher; beforeEach(() => { - jest.resetAllMocks(); - - kubernetesClientProvider = { - getMetricsClient: jest.fn(), - getCoreClientByClusterDetails: jest.fn(), - }; - sut = new KubernetesClientBasedFetcher({ - kubernetesClientProvider, + kubernetesClientProvider: new KubernetesClientProvider(), logger: getVoidLogger(), }); }); it('should return pod metrics', async () => { - (topPods as jest.Mock).mockResolvedValue(POD_METRICS_FIXTURE); + worker.use( + rest.get( + 'http://localhost:9999/api/v1/namespaces/:namespace/pods', + (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [ + { + metadata: { name: 'pod-name' }, + spec: { + containers: [ + { + name: 'container-name', + resources: { + requests: { cpu: '500m', memory: '512M' }, + limits: { cpu: '1000m', memory: '1G' }, + }, + }, + ], + }, + }, + ], + }), + ), + ), + rest.get( + 'http://localhost:9999/apis/metrics.k8s.io/v1beta1/namespaces/:namespace/pods', + (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [ + { + metadata: { name: 'pod-name' }, + containers: [ + { + name: 'container-name', + usage: { cpu: '0', memory: '0' }, + }, + ], + }, + ], + }), + ), + ), + ); const result = await sut.fetchPodMetricsByNamespaces( { @@ -686,36 +718,85 @@ describe('KubernetesFetcher', () => { serviceAccountToken: 'token', authProvider: 'serviceAccount', }, - new Set(['ns-a', 'ns-b']), + new Set(['ns-a']), ); - expect(result).toStrictEqual({ + expect(result).toMatchObject({ errors: [], - responses: [ - { - type: 'podstatus', - resources: POD_METRICS_FIXTURE, - }, - { - type: 'podstatus', - resources: POD_METRICS_FIXTURE, - }, - ], + responses: POD_METRICS_FIXTURE, }); }); it('should return pod metrics and error', async () => { - const topPodsMock = topPods as jest.Mock; - topPodsMock - .mockResolvedValueOnce(POD_METRICS_FIXTURE) - .mockRejectedValueOnce({ - response: { - statusCode: 404, - request: { - uri: { - pathname: '/some/path', - }, - }, - }, - }); + worker.use( + rest.get( + 'http://localhost:9999/api/v1/namespaces/ns-a/pods', + (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [ + { + metadata: { name: 'pod-name' }, + spec: { + containers: [ + { + name: 'container-name', + resources: { + requests: { cpu: '500m', memory: '512M' }, + limits: { cpu: '1000m', memory: '1G' }, + }, + }, + ], + }, + }, + ], + }), + ), + ), + rest.get( + 'http://localhost:9999/apis/metrics.k8s.io/v1beta1/namespaces/ns-a/pods', + (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [ + { + metadata: { name: 'pod-name' }, + containers: [ + { + name: 'container-name', + usage: { cpu: '0', memory: '0' }, + }, + ], + }, + ], + }), + ), + ), + rest.get( + 'http://localhost:9999/api/v1/namespaces/ns-b/pods', + (_, res, ctx) => + res( + ctx.status(404), + ctx.json({ + kind: 'Status', + apiVersion: 'v1', + code: 404, + }), + ), + ), + rest.get( + 'http://localhost:9999/apis/metrics.k8s.io/v1beta1/namespaces/ns-b/pods', + (_, res, ctx) => + res( + ctx.status(404), + ctx.json({ + kind: 'Status', + apiVersion: 'v1', + code: 404, + }), + ), + ), + ); const result = await sut.fetchPodMetricsByNamespaces( { @@ -726,21 +807,15 @@ describe('KubernetesFetcher', () => { }, new Set(['ns-a', 'ns-b']), ); - expect(result).toStrictEqual({ - errors: [ - { - errorType: 'NOT_FOUND', - resourcePath: '/some/path', - statusCode: 404, - }, - ], - responses: [ - { - type: 'podstatus', - resources: POD_METRICS_FIXTURE, - }, - ], - }); + + expect(result.errors).toStrictEqual([ + { + errorType: 'NOT_FOUND', + resourcePath: '/apis/metrics.k8s.io/v1beta1/namespaces/ns-b/pods', + statusCode: 404, + }, + ]); + expect(result.responses).toMatchObject(POD_METRICS_FIXTURE); }); }); }); From 99ca12978dd6ddd20f4d24ebbf259645bbb2f3ce Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Mon, 12 Dec 2022 18:02:33 -0500 Subject: [PATCH 07/15] spy on https calls to check caData This is a little awkward, but at the moment msw doesn't give much of a means of interacting with the non-WHATWG-Fetch-spec aspects of a request. Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index db318b4d0c..b5ff2f35c3 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -520,6 +520,176 @@ describe('KubernetesFetcher', () => { ], }); }); + describe('when server uses TLS', () => { + let httpsRequest: jest.SpyInstance; + beforeAll(() => { + httpsRequest = jest.spyOn( + // this is pretty egregious reverse engineering of msw. + // If the SetupServerApi constructor was exported, we wouldn't need + // to be quite so hacky here + (worker as any).interceptor.interceptors[0].modules.get('https'), + 'request', + ); + }); + beforeEach(() => { + httpsRequest.mockClear(); + }); + it('should trust specified caData', async () => { + worker.use( + rest.get('https://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], + }), + ), + ), + ); + + await sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'cluster1', + url: 'https://localhost:9999', + serviceAccountToken: 'token', + authProvider: 'serviceAccount', + caData: 'MOCKCA', + }, + objectTypesToFetch: new Set([ + { + group: '', + apiVersion: 'v1', + plural: 'pods', + objectType: 'pods', + }, + ]), + labelSelector: '', + customResources: [], + }); + + expect(httpsRequest).toHaveBeenCalledTimes(1); + const [[{ agent }]] = httpsRequest.mock.calls; + expect(agent.options.ca.toString('base64')).toMatch('MOCKCA'); + }); + it('should use default chain of trust when caData is unspecified', async () => { + worker.use( + rest.get('https://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], + }), + ), + ), + ); + + await sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'cluster1', + url: 'https://localhost:9999', + serviceAccountToken: 'token', + authProvider: 'serviceAccount', + }, + objectTypesToFetch: new Set([ + { + group: '', + apiVersion: 'v1', + plural: 'pods', + objectType: 'pods', + }, + ]), + labelSelector: '', + customResources: [], + }); + + expect(httpsRequest).toHaveBeenCalledTimes(1); + const [[{ agent }]] = httpsRequest.mock.calls; + expect(agent.options.ca).toBeUndefined(); + }); + describe('with a CA file on disk', () => { + afterEach(() => { + mockFs.restore(); + }); + it('should trust contents of specified caFile', async () => { + mockFs({ + '/path/to/ca.crt': 'MOCKCA', + }); + worker.use( + rest.get('https://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], + }), + ), + ), + ); + + await sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'cluster1', + url: 'https://localhost:9999', + serviceAccountToken: 'token', + authProvider: 'serviceAccount', + caFile: '/path/to/ca.crt', + }, + objectTypesToFetch: new Set([ + { + group: '', + apiVersion: 'v1', + plural: 'pods', + objectType: 'pods', + }, + ]), + labelSelector: '', + customResources: [], + }); + + expect(httpsRequest).toHaveBeenCalledTimes(1); + const [[{ agent }]] = httpsRequest.mock.calls; + expect(agent.options.ca.toString()).toEqual('MOCKCA'); + }); + }); + it('should accept unauthorized certs when skipTLSVerify is set', async () => { + worker.use( + rest.get('https://localhost:9999/api/v1/pods', (req, res, ctx) => + res( + checkToken(req, ctx, 'token'), + withLabels(req, ctx, { + items: [{ metadata: { name: 'pod-name' } }], + }), + ), + ), + ); + + await sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'cluster1', + url: 'https://localhost:9999', + serviceAccountToken: 'token', + authProvider: 'serviceAccount', + skipTLSVerify: true, + }, + objectTypesToFetch: new Set([ + { + group: '', + apiVersion: 'v1', + plural: 'pods', + objectType: 'pods', + }, + ]), + labelSelector: '', + customResources: [], + }); + + expect(httpsRequest).toHaveBeenCalledTimes(1); + const [[{ agent }]] = httpsRequest.mock.calls; + expect(agent.options.rejectUnauthorized).toBe(false); + }); + }); it('should use namespace if provided', async () => { worker.use( rest.get( From f54362ad5713358100e5947f8a26d1287b0e544e Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Tue, 29 Nov 2022 13:52:09 -0500 Subject: [PATCH 08/15] use node-fetch for k8s resources The main advantage this has over using the list*CustomObject methods is that it won't reject non-OK statuses, so we won't have to be so careful about catching and rethrowing. Personally I find it a bit clearer to see the actual requests rather than the client methods with all those empty arguments, and a side benefit is that this implementation uses the fetch API -- under the covers, @kubernetes/client-node is using the deprecated request library. Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.ts | 119 ++++++++++++------ 1 file changed, 80 insertions(+), 39 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts index fd6400529b..db2090a155 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts @@ -14,7 +14,13 @@ * limitations under the License. */ -import { topPods } from '@kubernetes/client-node'; +import { + Cluster, + KubeConfig, + User, + bufferFromFileOrString, + topPods, +} from '@kubernetes/client-node'; import lodash, { Dictionary } from 'lodash'; import { Logger } from 'winston'; import { @@ -32,6 +38,9 @@ import { PodStatusFetchResponse, } from '@backstage/plugin-kubernetes-common'; import { KubernetesClientProvider } from './KubernetesClientProvider'; +import fetch, { Headers, RequestInit } from 'node-fetch'; +import * as https from 'https'; +import fs from 'fs-extra'; export interface KubernetesClientBasedFetcherOptions { kubernetesClientProvider: KubernetesClientProvider; @@ -149,50 +158,82 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { labelSelector: string, objectType: KubernetesObjectTypes, namespace?: string, - ): Promise { - const customObjects = - this.kubernetesClientProvider.getCustomObjectsClient(clusterDetails); - - customObjects.addInterceptor((requestOptions: any) => { - requestOptions.uri = requestOptions.uri.replace('/apis//v1/', '/api/v1/'); - }); - + ): Promise { + const { group, apiVersion, plural } = resource; + const encode = (s: string) => encodeURIComponent(s); + let resourcePath = group + ? `/apis/${encode(group)}/${encode(apiVersion)}` + : `/api/${encode(apiVersion)}`; if (namespace) { - return customObjects - .listNamespacedCustomObject( - resource.group, - resource.apiVersion, - namespace, - resource.plural, - '', - false, - '', - '', - labelSelector, - ) - .then(r => { + resourcePath += `/namespaces/${encode(namespace)}`; + } + resourcePath += `/${encode(plural)}`; + + const headers: Headers = new Headers({ + Accept: 'application/json', + 'Content-Type': 'application/json', + }); + const fetchOptions: RequestInit = { + method: 'GET', + }; + let token: Buffer | string; + let url: URL; + if (clusterDetails.serviceAccountToken) { + url = new URL(`${clusterDetails.url}${resourcePath}`); + + if (url.protocol === 'https:') { + fetchOptions.agent = new https.Agent({ + ca: + bufferFromFileOrString( + clusterDetails.caFile, + clusterDetails.caData, + ) ?? undefined, + rejectUnauthorized: !clusterDetails.skipTLSVerify, + }); + } + + token = clusterDetails.serviceAccountToken; + } else { + const kc = new KubeConfig(); + kc.loadFromCluster(); + // loadFromCluster never fails (unless an exception is thrown) and is + // guaranteed to populate the cluster/user/context correctly + const cluster = kc.getCurrentCluster() as Cluster; + const user = kc.getCurrentUser() as User; + url = new URL(`${cluster.server}${resourcePath}`); + + if (url.protocol === 'https:') { + fetchOptions.agent = new https.Agent({ + ca: fs.readFileSync(cluster.caFile as string), + }); + } + + token = fs.readFileSync(user.authProvider.config.tokenFile); + } + + headers.set('Authorization', `Bearer ${token}`); + fetchOptions.headers = headers; + url.search = `labelSelector=${labelSelector}`; + + return fetch(url.toString(), fetchOptions).then(r => { + return r.json().then(j => { + if (r.ok) { return { type: objectType, - resources: (r.body as any).items, + resources: j.items, }; - }); - } - return customObjects - .listClusterCustomObject( - resource.group, - resource.apiVersion, - resource.plural, - '', - false, - '', - '', - labelSelector, - ) - .then(r => { + } + this.logger.warn( + `statusCode=${ + r.status + } for resource ${resourcePath} body=[${JSON.stringify(j)}]`, + ); return { - type: objectType, - resources: (r.body as any).items, + errorType: statusCodeToErrorType(r.status), + statusCode: r.status, + resourcePath, }; }); + }); } } From fcf8d33014886ed2e0f3dc379f314a6e9e8bcb94 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 30 Nov 2022 10:16:12 -0500 Subject: [PATCH 09/15] node-fetch-based implementation for pod metrics which eliminates the need for the KubernetesClientProvider Signed-off-by: Jamie Klassen --- plugins/kubernetes-backend/api-report.md | 16 -- .../src/service/KubernetesBuilder.ts | 2 - .../service/KubernetesClientProvider.test.ts | 95 ------- .../src/service/KubernetesClientProvider.ts | 87 ------- .../src/service/KubernetesFetcher.test.ts | 3 - .../src/service/KubernetesFetcher.ts | 243 ++++++++++-------- .../kubernetes-backend/src/service/index.ts | 1 - 7 files changed, 129 insertions(+), 318 deletions(-) delete mode 100644 plugins/kubernetes-backend/src/service/KubernetesClientProvider.test.ts delete mode 100644 plugins/kubernetes-backend/src/service/KubernetesClientProvider.ts diff --git a/plugins/kubernetes-backend/api-report.md b/plugins/kubernetes-backend/api-report.md index 2161df7c9a..4ec043a8f9 100644 --- a/plugins/kubernetes-backend/api-report.md +++ b/plugins/kubernetes-backend/api-report.md @@ -5,21 +5,17 @@ ```ts import { CatalogApi } from '@backstage/catalog-client'; import { Config } from '@backstage/config'; -import { CoreV1Api } from '@kubernetes/client-node'; import { Credentials } from 'aws-sdk'; -import { CustomObjectsApi } from '@kubernetes/client-node'; import type { CustomResourceMatcher } from '@backstage/plugin-kubernetes-common'; import { Duration } from 'luxon'; import { Entity } from '@backstage/catalog-model'; import express from 'express'; import type { FetchResponse } from '@backstage/plugin-kubernetes-common'; import type { JsonObject } from '@backstage/types'; -import { KubeConfig } from '@kubernetes/client-node'; import type { KubernetesFetchError } from '@backstage/plugin-kubernetes-common'; import { KubernetesRequestAuth } from '@backstage/plugin-kubernetes-common'; import type { KubernetesRequestBody } from '@backstage/plugin-kubernetes-common'; import { Logger } from 'winston'; -import { Metrics } from '@kubernetes/client-node'; import type { ObjectsByEntityResponse } from '@backstage/plugin-kubernetes-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import type { RequestHandler } from 'express'; @@ -262,18 +258,6 @@ export type KubernetesBuilderReturn = Promise<{ serviceLocator: KubernetesServiceLocator; }>; -// @alpha (undocumented) -export class KubernetesClientProvider { - // (undocumented) - getCoreClientByClusterDetails(clusterDetails: ClusterDetails): CoreV1Api; - // (undocumented) - getCustomObjectsClient(clusterDetails: ClusterDetails): CustomObjectsApi; - // (undocumented) - getKubeConfig(clusterDetails: ClusterDetails): KubeConfig; - // (undocumented) - getMetricsClient(clusterDetails: ClusterDetails): Metrics; -} - // @alpha export interface KubernetesClustersSupplier { getClusters(): Promise; diff --git a/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts b/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts index c8ff98f592..1e7b704ec5 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts @@ -34,7 +34,6 @@ import { ObjectsByEntityRequest, ServiceLocatorMethod, } from '../types/types'; -import { KubernetesClientProvider } from './KubernetesClientProvider'; import { DEFAULT_OBJECTS, KubernetesFanOutHandler, @@ -211,7 +210,6 @@ export class KubernetesBuilder { protected buildFetcher(): KubernetesFetcher { this.fetcher = new KubernetesClientBasedFetcher({ - kubernetesClientProvider: new KubernetesClientProvider(), logger: this.env.logger, }); diff --git a/plugins/kubernetes-backend/src/service/KubernetesClientProvider.test.ts b/plugins/kubernetes-backend/src/service/KubernetesClientProvider.test.ts deleted file mode 100644 index 0f73d129b3..0000000000 --- a/plugins/kubernetes-backend/src/service/KubernetesClientProvider.test.ts +++ /dev/null @@ -1,95 +0,0 @@ -/* - * Copyright 2020 The Backstage Authors - * - * 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 '@backstage/backend-common'; -import { KubernetesClientProvider } from './KubernetesClientProvider'; -import { ClusterDetails } from '../types/types'; -import * as https from 'https'; -import mockFs from 'mock-fs'; - -describe('KubernetesClientProvider', () => { - beforeEach(() => { - jest.resetAllMocks(); - }); - afterEach(() => { - mockFs.restore(); - }); - - it('can get core client by cluster details', () => { - const sut = new KubernetesClientProvider(); - const getKubeConfig = jest.spyOn(sut, 'getKubeConfig'); - - const clusterDetails: ClusterDetails = { - name: 'cluster-name', - url: 'http://localhost:9999', - serviceAccountToken: 'TOKEN', - authProvider: 'serviceAccount', - }; - const result = sut.getCoreClientByClusterDetails(clusterDetails); - - expect(result.basePath).toBe('http://localhost:9999'); - // These fields aren't on the type but are there - const auth = (result as any).authentications.default; - expect(auth.users[0].token).toBe('TOKEN'); - expect(auth.clusters[0].name).toBe('cluster-name'); - expect(auth.clusters[0].skipTLSVerify).toBe(false); - - expect(getKubeConfig).toHaveBeenCalledTimes(1); - }); - - it('can get custom objects client by cluster details', () => { - const sut = new KubernetesClientProvider(); - const getKubeConfig = jest.spyOn(sut, 'getKubeConfig'); - - const clusterDetails: ClusterDetails = { - name: 'cluster-name', - url: 'http://localhost:9999', - serviceAccountToken: 'TOKEN', - authProvider: 'serviceAccount', - skipTLSVerify: false, - }; - const result = sut.getCustomObjectsClient(clusterDetails); - - expect(result.basePath).toBe('http://localhost:9999'); - // These fields aren't on the type but are there - const auth = (result as any).authentications.default; - expect(auth.users[0].token).toBe('TOKEN'); - expect(auth.clusters[0].name).toBe('cluster-name'); - - expect(getKubeConfig).toHaveBeenCalledTimes(1); - }); - - it('respects caFile', async () => { - mockFs({ - '/path/to/ca.crt': 'my-ca', - }); - const clusterDetails: ClusterDetails = { - name: 'cluster-name', - url: 'https://localhost:9999', - authProvider: 'serviceAccount', - serviceAccountToken: 'TOKEN', - caFile: '/path/to/ca.crt', - }; - const kubeConfig = new KubernetesClientProvider().getKubeConfig( - clusterDetails, - ); - - const options: https.RequestOptions = {}; - await kubeConfig.applytoHTTPSOptions(options); - - expect(options.ca?.toString()).toEqual('my-ca'); - }); -}); diff --git a/plugins/kubernetes-backend/src/service/KubernetesClientProvider.ts b/plugins/kubernetes-backend/src/service/KubernetesClientProvider.ts deleted file mode 100644 index 14c8e422fc..0000000000 --- a/plugins/kubernetes-backend/src/service/KubernetesClientProvider.ts +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Copyright 2020 The Backstage Authors - * - * 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 { - Cluster, - Context, - CoreV1Api, - CustomObjectsApi, - KubeConfig, - Metrics, - User, -} from '@kubernetes/client-node'; -import { ClusterDetails } from '../types/types'; - -/** - * - * @alpha - */ -export class KubernetesClientProvider { - // visible for testing - getKubeConfig(clusterDetails: ClusterDetails): KubeConfig { - const cluster: Cluster = { - name: clusterDetails.name, - server: clusterDetails.url, - skipTLSVerify: clusterDetails.skipTLSVerify || false, - caData: clusterDetails.caData, - caFile: clusterDetails.caFile, - }; - - // TODO configure - const user: User = { - name: 'backstage', - token: clusterDetails.serviceAccountToken, - }; - - const context: Context = { - name: `${clusterDetails.name}`, - user: user.name, - cluster: cluster.name, - }; - - const kc: KubeConfig = new KubeConfig(); - if (clusterDetails.serviceAccountToken) { - kc.loadFromOptions({ - clusters: [cluster], - users: [user], - contexts: [context], - currentContext: context.name, - }); - } else { - kc.loadFromDefault(); - } - - return kc; - } - - getCoreClientByClusterDetails(clusterDetails: ClusterDetails): CoreV1Api { - const kc = this.getKubeConfig(clusterDetails); - - return kc.makeApiClient(CoreV1Api); - } - - getMetricsClient(clusterDetails: ClusterDetails): Metrics { - const kc = this.getKubeConfig(clusterDetails); - - return new Metrics(kc); - } - - getCustomObjectsClient(clusterDetails: ClusterDetails): CustomObjectsApi { - const kc = this.getKubeConfig(clusterDetails); - - return kc.makeApiClient(CustomObjectsApi); - } -} diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index b5ff2f35c3..9a1833cc4c 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -16,7 +16,6 @@ import { getVoidLogger } from '@backstage/backend-common'; import { KubernetesClientBasedFetcher } from './KubernetesFetcher'; -import { KubernetesClientProvider } from './KubernetesClientProvider'; import { ObjectToFetch } from '../types/types'; import { MockedRequest, @@ -167,7 +166,6 @@ describe('KubernetesFetcher', () => { beforeEach(() => { sut = new KubernetesClientBasedFetcher({ - kubernetesClientProvider: new KubernetesClientProvider(), logger, }); }); @@ -827,7 +825,6 @@ describe('KubernetesFetcher', () => { beforeEach(() => { sut = new KubernetesClientBasedFetcher({ - kubernetesClientProvider: new KubernetesClientProvider(), logger: getVoidLogger(), }); }); diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts index db2090a155..f90b96644d 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts @@ -16,7 +16,9 @@ import { Cluster, + CoreV1Api, KubeConfig, + Metrics, User, bufferFromFileOrString, topPods, @@ -27,9 +29,7 @@ import { ClusterDetails, FetchResponseWrapper, KubernetesFetcher, - KubernetesObjectTypes, ObjectFetchParams, - ObjectToFetch, } from '../types/types'; import { FetchResponse, @@ -37,13 +37,11 @@ import { KubernetesErrorTypes, PodStatusFetchResponse, } from '@backstage/plugin-kubernetes-common'; -import { KubernetesClientProvider } from './KubernetesClientProvider'; -import fetch, { Headers, RequestInit } from 'node-fetch'; +import fetch, { RequestInit, Response } from 'node-fetch'; import * as https from 'https'; import fs from 'fs-extra'; export interface KubernetesClientBasedFetcherOptions { - kubernetesClientProvider: KubernetesClientProvider; logger: Logger; } @@ -81,14 +79,9 @@ const statusCodeToErrorType = (statusCode: number): KubernetesErrorTypes => { }; export class KubernetesClientBasedFetcher implements KubernetesFetcher { - private readonly kubernetesClientProvider: KubernetesClientProvider; private readonly logger: Logger; - constructor({ - kubernetesClientProvider, - logger, - }: KubernetesClientBasedFetcherOptions) { - this.kubernetesClientProvider = kubernetesClientProvider; + constructor({ logger }: KubernetesClientBasedFetcherOptions) { this.logger = logger; } @@ -97,16 +90,27 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { ): Promise { const fetchResults = Array.from(params.objectTypesToFetch) .concat(params.customResources) - .map(toFetch => { - return this.fetchResource( + .map(({ objectType, group, apiVersion, plural }) => + this.fetchResource( params.clusterDetails, - toFetch, + group, + apiVersion, + plural, + params.namespace, params.labelSelector || `backstage.io/kubernetes-id=${params.serviceId}`, - toFetch.objectType, - params.namespace, - ).catch(this.captureKubernetesErrorsRethrowOthers.bind(this)); - }); + ).then( + (r: Response): Promise => + r.ok + ? r.json().then( + ({ items }): FetchResponse => ({ + type: objectType, + resources: items, + }), + ) + : this.handleUnsuccessfulResponse(r), + ), + ); return Promise.all(fetchResults).then(fetchResultsToResponseWrapper); } @@ -115,51 +119,65 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { clusterDetails: ClusterDetails, namespaces: Set, ): Promise { - const metricsClient = - this.kubernetesClientProvider.getMetricsClient(clusterDetails); - const coreApi = - this.kubernetesClientProvider.getCoreClientByClusterDetails( - clusterDetails, - ); - - const fetchResults = Array.from(namespaces).map(ns => - topPods(coreApi, metricsClient, ns) - .then(r => { - return { + const fetchResults = Array.from(namespaces).map(async ns => { + const [podMetrics, podList] = await Promise.all([ + this.fetchResource( + clusterDetails, + 'metrics.k8s.io', + 'v1beta1', + 'pods', + ns, + ), + this.fetchResource(clusterDetails, '', 'v1', 'pods', ns), + ]); + if (podMetrics.ok && podList.ok) { + return topPods( + { + listPodForAllNamespaces: () => + podList.json().then(b => ({ body: b })), + } as unknown as CoreV1Api, + { + getPodMetrics: () => podMetrics.json(), + } as unknown as Metrics, + ).then( + (resources): PodStatusFetchResponse => ({ type: 'podstatus', - resources: r, - } as PodStatusFetchResponse; - }) - .catch(this.captureKubernetesErrorsRethrowOthers.bind(this)), - ); + resources, + }), + ); + } else if (podMetrics.ok) { + return this.handleUnsuccessfulResponse(podList); + } + return this.handleUnsuccessfulResponse(podMetrics); + }); return Promise.all(fetchResults).then(fetchResultsToResponseWrapper); } - private captureKubernetesErrorsRethrowOthers(e: any): KubernetesFetchError { - if (e.response && e.response.statusCode) { - this.logger.warn( - `statusCode=${e.response.statusCode} for resource ${ - e.response.request.uri.pathname - } body=[${JSON.stringify(e.response.body)}]`, - ); - return { - errorType: statusCodeToErrorType(e.response.statusCode), - statusCode: e.response.statusCode, - resourcePath: e.response.request.uri.pathname, - }; - } - throw e; + private async handleUnsuccessfulResponse( + res: Response, + ): Promise { + const resourcePath = new URL(res.url).pathname; + this.logger.warn( + `statusCode=${ + res.status + } for resource ${resourcePath} body=[${await res.text()}]`, + ); + return { + errorType: statusCodeToErrorType(res.status), + statusCode: res.status, + resourcePath, + }; } private fetchResource( clusterDetails: ClusterDetails, - resource: ObjectToFetch, - labelSelector: string, - objectType: KubernetesObjectTypes, + group: string, + apiVersion: string, + plural: string, namespace?: string, - ): Promise { - const { group, apiVersion, plural } = resource; + labelSelector?: string, + ): Promise { const encode = (s: string) => encodeURIComponent(s); let resourcePath = group ? `/apis/${encode(group)}/${encode(apiVersion)}` @@ -169,71 +187,68 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { } resourcePath += `/${encode(plural)}`; - const headers: Headers = new Headers({ - Accept: 'application/json', - 'Content-Type': 'application/json', - }); - const fetchOptions: RequestInit = { - method: 'GET', - }; - let token: Buffer | string; - let url: URL; - if (clusterDetails.serviceAccountToken) { - url = new URL(`${clusterDetails.url}${resourcePath}`); + const [url, requestInit]: [URL, RequestInit] = + clusterDetails.serviceAccountToken + ? this.fetchArgsFromClusterDetails(clusterDetails) + : this.fetchArgsInCluster(); - if (url.protocol === 'https:') { - fetchOptions.agent = new https.Agent({ - ca: - bufferFromFileOrString( - clusterDetails.caFile, - clusterDetails.caData, - ) ?? undefined, - rejectUnauthorized: !clusterDetails.skipTLSVerify, - }); - } - - token = clusterDetails.serviceAccountToken; - } else { - const kc = new KubeConfig(); - kc.loadFromCluster(); - // loadFromCluster never fails (unless an exception is thrown) and is - // guaranteed to populate the cluster/user/context correctly - const cluster = kc.getCurrentCluster() as Cluster; - const user = kc.getCurrentUser() as User; - url = new URL(`${cluster.server}${resourcePath}`); - - if (url.protocol === 'https:') { - fetchOptions.agent = new https.Agent({ - ca: fs.readFileSync(cluster.caFile as string), - }); - } - - token = fs.readFileSync(user.authProvider.config.tokenFile); + url.pathname = resourcePath; + if (labelSelector) { + url.search = `labelSelector=${labelSelector}`; } - headers.set('Authorization', `Bearer ${token}`); - fetchOptions.headers = headers; - url.search = `labelSelector=${labelSelector}`; + return fetch(url, requestInit); + } - return fetch(url.toString(), fetchOptions).then(r => { - return r.json().then(j => { - if (r.ok) { - return { - type: objectType, - resources: j.items, - }; - } - this.logger.warn( - `statusCode=${ - r.status - } for resource ${resourcePath} body=[${JSON.stringify(j)}]`, - ); - return { - errorType: statusCodeToErrorType(r.status), - statusCode: r.status, - resourcePath, - }; + private fetchArgsFromClusterDetails( + clusterDetails: ClusterDetails, + ): [URL, RequestInit] { + const requestInit: RequestInit = { + method: 'GET', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + Authorization: `Bearer ${clusterDetails.serviceAccountToken}`, + }, + }; + + const url: URL = new URL(clusterDetails.url); + if (url.protocol === 'https:') { + requestInit.agent = new https.Agent({ + ca: + bufferFromFileOrString( + clusterDetails.caFile, + clusterDetails.caData, + ) ?? undefined, + rejectUnauthorized: !clusterDetails.skipTLSVerify, }); - }); + } + return [url, requestInit]; + } + private fetchArgsInCluster(): [URL, RequestInit] { + const kc = new KubeConfig(); + kc.loadFromCluster(); + // loadFromCluster is guaranteed to populate the cluster/user/context + const cluster = kc.getCurrentCluster() as Cluster; + const user = kc.getCurrentUser() as User; + + const token = fs.readFileSync(user.authProvider.config.tokenFile); + + const requestInit: RequestInit = { + method: 'GET', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + Authorization: `Bearer ${token}`, + }, + }; + + const url = new URL(cluster.server); + if (url.protocol === 'https:') { + requestInit.agent = new https.Agent({ + ca: fs.readFileSync(cluster.caFile as string), + }); + } + return [url, requestInit]; } } diff --git a/plugins/kubernetes-backend/src/service/index.ts b/plugins/kubernetes-backend/src/service/index.ts index 70ff530bee..620f788ac2 100644 --- a/plugins/kubernetes-backend/src/service/index.ts +++ b/plugins/kubernetes-backend/src/service/index.ts @@ -15,7 +15,6 @@ */ export * from './KubernetesBuilder'; -export * from './KubernetesClientProvider'; export { DEFAULT_OBJECTS } from './KubernetesFanOutHandler'; export { HEADER_KUBERNETES_CLUSTER, KubernetesProxy } from './KubernetesProxy'; export * from './router'; From 0ad476a720f520b267e446f2d64c7cafdd8b5462 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 30 Nov 2022 13:17:40 -0500 Subject: [PATCH 10/15] gracefully surface FetchErrors Signed-off-by: Jamie Klassen --- .../service/KubernetesFanOutHandler.test.ts | 101 ++++++++++++++++++ .../src/service/KubernetesFanOutHandler.ts | 9 ++ plugins/kubernetes-common/api-report.md | 27 +++-- plugins/kubernetes-common/src/types.ts | 11 +- .../src/components/ErrorPanel/ErrorPanel.tsx | 3 +- 5 files changed, 141 insertions(+), 10 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts index 1af5371e53..fcb8eceb19 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts @@ -20,8 +20,14 @@ import { CustomResource, FetchResponseWrapper, ObjectFetchParams, + KubernetesServiceLocator, } from '../types/types'; import { KubernetesFanOutHandler } from './KubernetesFanOutHandler'; +import { KubernetesClientBasedFetcher } from './KubernetesFetcher'; +import { rest } from 'msw'; +import { setupServer } from 'msw/node'; +import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; +import { ObjectsByEntityResponse } from '@backstage/plugin-kubernetes-common'; const fetchObjectsForService = jest.fn(); const fetchPodMetricsByNamespaces = jest.fn(); @@ -751,6 +757,101 @@ describe('getKubernetesObjectsByEntity', () => { ], }); }); + describe('with a real fetcher', () => { + const worker = setupServer(); + setupRequestMockHandlers(worker); + it('fetch error short-circuits requests to a single cluster, recovering across the fleet', async () => { + const pods = [{ metadata: { name: 'pod-name' } }]; + const services = [{ metadata: { name: 'service-name' } }]; + worker.use( + rest.get('https://works/api/v1/pods', (_, res, ctx) => + res(ctx.json({ items: pods })), + ), + rest.get('https://works/api/v1/services', (_, res, ctx) => + res(ctx.json({ items: services })), + ), + rest.get('https://fails/api/v1/pods', (_, res) => + res.networkError('socket error'), + ), + rest.get('https://fails/api/v1/services', (_, res, ctx) => + res(ctx.json({ items: services })), + ), + ); + const fleet: jest.Mocked = { + getClustersByEntity: jest.fn().mockResolvedValue({ + clusters: [ + { + name: 'works', + url: 'https://works', + authProvider: 'serviceAccount', + serviceAccountToken: 'token', + skipMetricsLookup: true, + }, + { + name: 'fails', + url: 'https://fails', + authProvider: 'serviceAccount', + serviceAccountToken: 'token', + skipMetricsLookup: true, + }, + ], + }), + }; + const logger = getVoidLogger(); + const sut = new KubernetesFanOutHandler({ + logger, + fetcher: new KubernetesClientBasedFetcher({ logger }), + serviceLocator: fleet, + customResources: [], + objectTypesToFetch: [ + { + group: '', + apiVersion: 'v1', + plural: 'pods', + objectType: 'pods', + }, + { + group: '', + apiVersion: 'v1', + plural: 'services', + objectType: 'services', + }, + ], + }); + + const result = await sut.getKubernetesObjectsByEntity({ + entity, + auth: {}, + }); + + const expected: ObjectsByEntityResponse = { + items: [ + { + cluster: { name: 'works' }, + resources: [ + { type: 'pods', resources: pods }, + { type: 'services', resources: services }, + ], + podMetrics: [], + errors: [], + }, + { + cluster: { name: 'fails' }, + resources: [], + podMetrics: [], + errors: [ + { + errorType: 'FETCH_ERROR', + message: + 'request to https://fails/api/v1/pods?labelSelector=backstage.io/kubernetes-id=test-component failed, reason: socket error', + }, + ], + }, + ], + }; + expect(result).toStrictEqual(expected); + }); + }); }); describe('getCustomResourcesByEntity', () => { diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts index 1fa9d23837..bc8881f95b 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts @@ -274,6 +274,15 @@ export class KubernetesFanOutHandler { namespace, }) .then(result => this.getMetricsForPods(clusterDetailsItem, result)) + .catch((e): responseWithMetrics => { + return [ + { + errors: [{ errorType: 'FETCH_ERROR', message: e.message }], + responses: [], + }, + [], + ]; + }) .then(r => this.toClusterObjects(clusterDetailsItem, r)); }), ).then(this.toObjectsByEntityResponse); diff --git a/plugins/kubernetes-common/api-report.md b/plugins/kubernetes-common/api-report.md index 8509fa97cc..fe8aa19966 100644 --- a/plugins/kubernetes-common/api-report.md +++ b/plugins/kubernetes-common/api-report.md @@ -184,14 +184,7 @@ export type KubernetesErrorTypes = | 'UNKNOWN_ERROR'; // @public (undocumented) -export interface KubernetesFetchError { - // (undocumented) - errorType: KubernetesErrorTypes; - // (undocumented) - resourcePath?: string; - // (undocumented) - statusCode?: number; -} +export type KubernetesFetchError = StatusError | RawFetchError; // @public (undocumented) export interface KubernetesRequestAuth { @@ -241,6 +234,14 @@ export interface PodStatusFetchResponse { type: 'podstatus'; } +// @public (undocumented) +export interface RawFetchError { + // (undocumented) + errorType: 'FETCH_ERROR'; + // (undocumented) + message: string; +} + // @public (undocumented) export interface ReplicaSetsFetchResponse { // (undocumented) @@ -265,6 +266,16 @@ export interface StatefulSetsFetchResponse { type: 'statefulsets'; } +// @public (undocumented) +export interface StatusError { + // (undocumented) + errorType: KubernetesErrorTypes; + // (undocumented) + resourcePath?: string; + // (undocumented) + statusCode?: number; +} + // @public (undocumented) export interface WorkloadsByEntityRequest { // (undocumented) diff --git a/plugins/kubernetes-common/src/types.ts b/plugins/kubernetes-common/src/types.ts index 3feddd7f4c..f723a46d16 100644 --- a/plugins/kubernetes-common/src/types.ts +++ b/plugins/kubernetes-common/src/types.ts @@ -223,12 +223,21 @@ export interface PodStatusFetchResponse { } /** @public */ -export interface KubernetesFetchError { +export type KubernetesFetchError = StatusError | RawFetchError; + +/** @public */ +export interface StatusError { errorType: KubernetesErrorTypes; statusCode?: number; resourcePath?: string; } +/** @public */ +export interface RawFetchError { + errorType: 'FETCH_ERROR'; + message: string; +} + /** @public */ export type KubernetesErrorTypes = | 'BAD_REQUEST' diff --git a/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx b/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx index fcdeeef393..1e55cd8d18 100644 --- a/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx +++ b/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx @@ -29,7 +29,8 @@ const clustersWithErrorsToErrorMessage = ( {c.errors.map((e, j) => { return ( - {`Error fetching Kubernetes resource: '${e.resourcePath}', error: ${e.errorType}, status code: ${e.statusCode}`} + {e.errorType !== 'FETCH_ERROR' && + `Error fetching Kubernetes resource: '${e.resourcePath}', error: ${e.errorType}, status code: ${e.statusCode}`} ); })} From a5a9d3318ae01d8e66ade8fbaa12271a3386cd92 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 30 Nov 2022 13:54:28 -0500 Subject: [PATCH 11/15] don't surface non-FetchErrors It makes sense to gracefully show an error that was actually related to fetching data from Kubernetes, but if the error is happening for some other exotic reason, the default error handling logic in Backstage should take over and log/manage it appropriately. Signed-off-by: Jamie Klassen --- .../service/KubernetesFanOutHandler.test.ts | 21 +++++++++++++++++ .../src/service/KubernetesFanOutHandler.ts | 23 +++++++++++-------- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts index fcb8eceb19..17a86eb30d 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts @@ -757,6 +757,27 @@ describe('getKubernetesObjectsByEntity', () => { ], }); }); + it('fails when fetcher rejects with a non-FetchError', async () => { + const nonFetchError = new Error('not a fetch error'); + getClustersByEntity.mockResolvedValue({ + clusters: [ + { + name: 'test-cluster', + authProvider: 'serviceAccount', + skipMetricsLookup: true, + }, + ], + }); + fetchObjectsForService.mockRejectedValue(nonFetchError); + + const sut = getKubernetesFanOutHandler([]); + + const result = sut.getKubernetesObjectsByEntity({ + entity, + auth: {}, + }); + await expect(result).rejects.toThrow(nonFetchError); + }); describe('with a real fetcher', () => { const worker = setupServer(); setupRequestMockHandlers(worker); diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts index bc8881f95b..a738c69bcc 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts @@ -274,15 +274,20 @@ export class KubernetesFanOutHandler { namespace, }) .then(result => this.getMetricsForPods(clusterDetailsItem, result)) - .catch((e): responseWithMetrics => { - return [ - { - errors: [{ errorType: 'FETCH_ERROR', message: e.message }], - responses: [], - }, - [], - ]; - }) + .catch( + (e): Promise => + e.name === 'FetchError' + ? Promise.resolve([ + { + errors: [ + { errorType: 'FETCH_ERROR', message: e.message }, + ], + responses: [], + }, + [], + ]) + : Promise.reject(e), + ) .then(r => this.toClusterObjects(clusterDetailsItem, r)); }), ).then(this.toObjectsByEntityResponse); From bacb5470b6a20212ebed7a20a510e0afc622ff13 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 30 Nov 2022 14:09:00 -0500 Subject: [PATCH 12/15] frontend displays new error types Signed-off-by: Jamie Klassen --- .../components/ErrorPanel/ErrorPanel.test.tsx | 84 +++++++++++++------ .../src/components/ErrorPanel/ErrorPanel.tsx | 5 +- 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.test.tsx b/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.test.tsx index 832d6eed91..0d75c09a11 100644 --- a/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.test.tsx +++ b/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.test.tsx @@ -20,36 +20,14 @@ 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 a problem retrieving some Kubernetes resources for the entity: THIS_ENTITY. This could mean that the Error Reporting card is not completely accurate.', - ), - ).toBeInTheDocument(); - - // message - expect(getByText('Errors: SOME_ERROR_MESSAGE')).toBeInTheDocument(); - }); - it('render with cluster errors', async () => { + it('displays path and status code when a cluster has an HTTP error', async () => { const { getByText } = render( wrapInTestApp( { ), ).toBeInTheDocument(); }); + it('displays message for non-HTTP-status-related fetch errors', async () => { + const { getByText } = render( + wrapInTestApp( + , + ), + ); + + // title + expect( + getByText( + 'There was a problem retrieving some Kubernetes resources for the entity: THIS_ENTITY. This could mean that the Error Reporting card is not completely accurate.', + ), + ).toBeInTheDocument(); + + // message + expect(getByText('Errors:')).toBeInTheDocument(); + expect(getByText('Cluster: THIS_CLUSTER')).toBeInTheDocument(); + expect( + getByText( + 'Error communicating with Kubernetes: FETCH_ERROR, message: description of error', + ), + ).toBeInTheDocument(); + }); + it('displays error message', async () => { + const { getByText } = render( + wrapInTestApp( + , + ), + ); + + // title + expect( + getByText( + 'There was a problem retrieving some Kubernetes resources for the entity: THIS_ENTITY. This could mean that the Error Reporting card is not completely accurate.', + ), + ).toBeInTheDocument(); + + // message + expect(getByText('Errors: SOME_ERROR_MESSAGE')).toBeInTheDocument(); + }); }); diff --git a/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx b/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx index 1e55cd8d18..d2cb88b9ca 100644 --- a/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx +++ b/plugins/kubernetes/src/components/ErrorPanel/ErrorPanel.tsx @@ -29,8 +29,9 @@ const clustersWithErrorsToErrorMessage = ( {c.errors.map((e, j) => { return ( - {e.errorType !== 'FETCH_ERROR' && - `Error fetching Kubernetes resource: '${e.resourcePath}', error: ${e.errorType}, status code: ${e.statusCode}`} + {e.errorType === 'FETCH_ERROR' + ? `Error communicating with Kubernetes: ${e.errorType}, message: ${e.message}` + : `Error fetching Kubernetes resource: '${e.resourcePath}', error: ${e.errorType}, status code: ${e.statusCode}`} ); })} From b008ccee54fbb1848e49e117551bfdf339257967 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 30 Nov 2022 22:56:06 -0500 Subject: [PATCH 13/15] throw when missing token and not on k8s Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 18 ++++++++++++++++++ .../src/service/KubernetesFetcher.ts | 18 ++++++++++++++---- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 9a1833cc4c..2311dacdc3 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -754,6 +754,24 @@ describe('KubernetesFetcher', () => { ], }); }); + describe('Backstage not running on k8s', () => { + it('fails if cluster details has no token', () => { + const result = sut.fetchObjectsForService({ + serviceId: 'some-service', + clusterDetails: { + name: 'unauthenticated-cluster', + url: 'http://ignored', + authProvider: 'serviceAccount', + }, + objectTypesToFetch: OBJECTS_TO_FETCH, + labelSelector: '', + customResources: [], + }); + return expect(result).rejects.toThrow( + "no bearer token for cluster 'unauthenticated-cluster' and not running in Kubernetes", + ); + }); + }); describe('Backstage running on k8s', () => { const initialHost = process.env.KUBERNETES_SERVICE_HOST; const initialPort = process.env.KUBERNETES_SERVICE_PORT; diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts index f90b96644d..b700c74e4c 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts @@ -15,6 +15,7 @@ */ import { + Config, Cluster, CoreV1Api, KubeConfig, @@ -187,10 +188,19 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { } resourcePath += `/${encode(plural)}`; - const [url, requestInit]: [URL, RequestInit] = - clusterDetails.serviceAccountToken - ? this.fetchArgsFromClusterDetails(clusterDetails) - : this.fetchArgsInCluster(); + let url: URL; + let requestInit: RequestInit; + if (clusterDetails.serviceAccountToken) { + [url, requestInit] = this.fetchArgsFromClusterDetails(clusterDetails); + } else if (fs.pathExistsSync(Config.SERVICEACCOUNT_TOKEN_PATH)) { + [url, requestInit] = this.fetchArgsInCluster(); + } else { + return Promise.reject( + new Error( + `no bearer token for cluster '${clusterDetails.name}' and not running in Kubernetes`, + ), + ); + } url.pathname = resourcePath; if (labelSelector) { From 7341131af5c33b1d11378ebb5baa05071caf2eb5 Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 14 Dec 2022 17:49:56 -0500 Subject: [PATCH 14/15] warning is a full sentence mentioning cluster name This seems like a more helpful log message for operators, especially those with many clusters. Signed-off-by: Jamie Klassen --- .../src/service/KubernetesFetcher.test.ts | 2 +- .../src/service/KubernetesFetcher.ts | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts index 2311dacdc3..065416f8c1 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.test.ts @@ -388,7 +388,7 @@ describe('KubernetesFetcher', () => { ], }); expect(warn).toHaveBeenCalledWith( - 'statusCode=401 for resource /api/v1/services body=[{"kind":"Status","apiVersion":"v1","code":401}]', + 'Received 401 status when fetching "/api/v1/services" from cluster "cluster1"; body=[{"kind":"Status","apiVersion":"v1","code":401}]', ); }); // they're in testErrorResponse diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts index b700c74e4c..6ef4397e9c 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts @@ -109,7 +109,7 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { resources: items, }), ) - : this.handleUnsuccessfulResponse(r), + : this.handleUnsuccessfulResponse(params.clusterDetails.name, r), ), ); @@ -147,22 +147,23 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { }), ); } else if (podMetrics.ok) { - return this.handleUnsuccessfulResponse(podList); + return this.handleUnsuccessfulResponse(clusterDetails.name, podList); } - return this.handleUnsuccessfulResponse(podMetrics); + return this.handleUnsuccessfulResponse(clusterDetails.name, podMetrics); }); return Promise.all(fetchResults).then(fetchResultsToResponseWrapper); } private async handleUnsuccessfulResponse( + clusterName: string, res: Response, ): Promise { const resourcePath = new URL(res.url).pathname; this.logger.warn( - `statusCode=${ + `Received ${ res.status - } for resource ${resourcePath} body=[${await res.text()}]`, + } status when fetching "${resourcePath}" from cluster "${clusterName}"; body=[${await res.text()}]`, ); return { errorType: statusCodeToErrorType(res.status), From 2db8acffe7e1c12e633eede6eccd619ce22e621d Mon Sep 17 00:00:00 2001 From: Jamie Klassen Date: Wed, 14 Dec 2022 17:55:53 -0500 Subject: [PATCH 15/15] add changeset Signed-off-by: Jamie Klassen --- .changeset/brave-eggs-impress.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/brave-eggs-impress.md diff --git a/.changeset/brave-eggs-impress.md b/.changeset/brave-eggs-impress.md new file mode 100644 index 0000000000..a0df24d8f9 --- /dev/null +++ b/.changeset/brave-eggs-impress.md @@ -0,0 +1,7 @@ +--- +'@backstage/plugin-kubernetes-backend': minor +'@backstage/plugin-kubernetes-common': minor +'@backstage/plugin-kubernetes': patch +--- + +Kubernetes plugin now gracefully surfaces transport-level errors (like DNS or timeout, or other socket errors) occurring while fetching data. This will be merged into any data that is fetched successfully, fixing a bug where the whole page would be empty if any fetch operation encountered such an error.