From 2a1eb5549e0a46d9888d0a5c37c21ca5bf061a1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 18 Nov 2022 15:03:16 +0100 Subject: [PATCH] memoize middlewares so we don't churn resources excessively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- plugins/kubernetes-backend/api-report.md | 2 +- .../src/service/KubernetesBuilder.ts | 2 +- .../src/service/KubernetesProxy.test.ts | 7 +- .../src/service/KubernetesProxy.ts | 135 +++++++++--------- 4 files changed, 76 insertions(+), 70 deletions(-) diff --git a/plugins/kubernetes-backend/api-report.md b/plugins/kubernetes-backend/api-report.md index 6fd5675a8d..b88b3f2a02 100644 --- a/plugins/kubernetes-backend/api-report.md +++ b/plugins/kubernetes-backend/api-report.md @@ -354,7 +354,7 @@ export type KubernetesObjectTypes = export class KubernetesProxy { constructor(logger: Logger, clusterSupplier: KubernetesClustersSupplier); // (undocumented) - proxyRequestHandler: RequestHandler; + createRequestHandler(): RequestHandler; } // @alpha diff --git a/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts b/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts index a5e67ebe2a..c8ff98f592 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesBuilder.ts @@ -299,7 +299,7 @@ export class KubernetesBuilder { }); }); - router.use('/proxy', proxy.proxyRequestHandler); + router.use('/proxy', proxy.createRequestHandler()); addResourceRoutesToRouter(router, catalogApi, objectsProvider); diff --git a/plugins/kubernetes-backend/src/service/KubernetesProxy.test.ts b/plugins/kubernetes-backend/src/service/KubernetesProxy.test.ts index 5d4eaa795b..4254bb33da 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesProxy.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesProxy.test.ts @@ -13,8 +13,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import 'buffer'; +import 'buffer'; import { getVoidLogger } from '@backstage/backend-common'; import { NotFoundError } from '@backstage/errors'; import { getMockReq, getMockRes } from '@jest-mock/express'; @@ -24,7 +24,6 @@ import request from 'supertest'; import { rest } from 'msw'; import { setupServer } from 'msw/node'; import { setupRequestMockHandlers } from '@backstage/backend-test-utils'; - import { ClusterDetails, KubernetesClustersSupplier } from '../types/types'; import { APPLICATION_JSON, @@ -76,7 +75,7 @@ describe('KubernetesProxy', () => { const req = buildMockRequest('test', 'api'); const { res, next } = getMockRes(); - await expect(proxy.proxyRequestHandler(req, res, next)).rejects.toThrow( + await expect(proxy.createRequestHandler()(req, res, next)).rejects.toThrow( NotFoundError, ); }); @@ -101,7 +100,7 @@ describe('KubernetesProxy', () => { authProvider: 'serviceAccount', }, ] as ClusterDetails[]); - const app = express().use('/mountpath', proxy.proxyRequestHandler); + const app = express().use('/mountpath', proxy.createRequestHandler()); const requestPromise = request(app) .get('/mountpath/api') .set(HEADER_KUBERNETES_CLUSTER, 'cluster1'); diff --git a/plugins/kubernetes-backend/src/service/KubernetesProxy.ts b/plugins/kubernetes-backend/src/service/KubernetesProxy.ts index ace07247b1..9d88d2ab84 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesProxy.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesProxy.ts @@ -23,10 +23,7 @@ import { } from '@backstage/errors'; import { bufferFromFileOrString } from '@kubernetes/client-node'; import type { Request, RequestHandler } from 'express'; -import { - createProxyMiddleware, - Options as ProxyMiddlewareOptions, -} from 'http-proxy-middleware'; +import { createProxyMiddleware } from 'http-proxy-middleware'; import { Logger } from 'winston'; import { ClusterDetails, KubernetesClustersSupplier } from '../types/types'; @@ -45,79 +42,89 @@ export const HEADER_KUBERNETES_CLUSTER: string = 'X-Kubernetes-Cluster'; * @alpha */ export class KubernetesProxy { + private readonly middlewareForClusterName = new Map(); + constructor( private readonly logger: Logger, private readonly clusterSupplier: KubernetesClustersSupplier, ) {} - public proxyRequestHandler: RequestHandler = async (req, res, next) => { - const requestedCluster = this.getKubernetesRequestedCluster(req); - - const clusterDetails = await this.getClusterDetails(requestedCluster); - - const clusterUrl = new URL(clusterDetails.url); - const options: ProxyMiddlewareOptions = { - logProvider: () => this.logger, - secure: !clusterDetails.skipTLSVerify, - target: { - protocol: clusterUrl.protocol, - host: clusterUrl.hostname, - port: clusterUrl.port, - ca: bufferFromFileOrString('', clusterDetails.caData)?.toString(), - }, - pathRewrite: { [`^${req.baseUrl}`]: '' }, - onError: (error: Error) => { - const wrappedError = new ForwardedError( - `Cluster '${requestedCluster}' request error`, - error, - ); - - this.logger.error(wrappedError); - - const body: ErrorResponseBody = { - error: serializeError(wrappedError, { - includeStack: process.env.NODE_ENV === 'development', - }), - request: { method: req.method, url: req.originalUrl }, - response: { statusCode: 500 }, - }; - - res.status(500).json(body); - }, + public createRequestHandler(): RequestHandler { + return async (req, res, next) => { + const middleware = await this.getMiddleware(req); + middleware(req, res, next); }; + } - // Probably too risky without permissions protecting this endpoint - // if (clusterDetails.serviceAccountToken) { - // options.headers = { - // Authorization: `Bearer ${clusterDetails.serviceAccountToken}`, - // }; - // } - createProxyMiddleware(options)(req, res, next); - }; + // We create one middleware per remote cluster and hold on to them, because + // the secure property isn't possible to decide on a per-request basis with a + // single middleware instance - and we don't expect it to change over time. + private async getMiddleware(originalReq: Request): Promise { + const originalCluster = await this.getClusterForRequest(originalReq); + let middleware = this.middlewareForClusterName.get(originalCluster.name); + if (!middleware) { + // Probably too risky without permissions protecting this endpoint + // if (cluster.serviceAccountToken) { + // options.headers = { + // Authorization: `Bearer ${cluster.serviceAccountToken}`, + // }; + // } - private getKubernetesRequestedCluster(req: Request): string { - const requestedClusterName = req.header(HEADER_KUBERNETES_CLUSTER); + const logger = this.logger.child({ cluster: originalCluster.name }); + middleware = createProxyMiddleware({ + logProvider: () => logger, + secure: !originalCluster.skipTLSVerify, + router: async req => { + // Re-evaluate the cluster on each request, in case it has changed + const cluster = await this.getClusterForRequest(req); + const url = new URL(cluster.url); + return { + protocol: url.protocol, + host: url.hostname, + port: url.port, + ca: bufferFromFileOrString('', cluster.caData)?.toString(), + }; + }, + pathRewrite: { [`^${originalReq.baseUrl}`]: '' }, + onError: (error, req, res) => { + const wrappedError = new ForwardedError( + `Cluster '${originalCluster.name}' request error`, + error, + ); - if (!requestedClusterName) { + logger.error(wrappedError); + + const body: ErrorResponseBody = { + error: serializeError(wrappedError, { + includeStack: process.env.NODE_ENV === 'development', + }), + request: { method: req.method, url: req.originalUrl }, + response: { statusCode: 500 }, + }; + + res.status(500).json(body); + }, + }); + + this.middlewareForClusterName.set(originalCluster.name, middleware); + } + + return middleware; + } + + private async getClusterForRequest(req: Request): Promise { + const clusterName = req.header(HEADER_KUBERNETES_CLUSTER); + if (!clusterName) { throw new InputError(`Missing '${HEADER_KUBERNETES_CLUSTER}' header.`); } - return requestedClusterName; - } - - private async getClusterDetails( - requestedCluster: string, - ): Promise { - const clusters = await this.clusterSupplier.getClusters(); - - const clusterDetail = clusters.find( - cluster => cluster.name === requestedCluster, - ); - - if (!clusterDetail) { - throw new NotFoundError(`Cluster '${requestedCluster}' not found`); + const cluster = await this.clusterSupplier + .getClusters() + .then(clusters => clusters.find(c => c.name === clusterName)); + if (!cluster) { + throw new NotFoundError(`Cluster '${clusterName}' not found`); } - return clusterDetail; + return cluster; } }