From 980a029f21a334d6410212fbd88b9fca76bfc1dc Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 21 Mar 2023 02:45:30 +0000 Subject: [PATCH] Refactor dev tool console to use opensearch-js client to send request (#3544) Refactor dev tool console to use opensearch-js client to interact with OpenSearch Signed-off-by: Su (cherry picked from commit de0634495ebace79453b50421e8d147104a7b562) Signed-off-by: github-actions[bot] # Conflicts: # CHANGELOG.md --- .../api/console/proxy/create_handler.ts | 173 +++++------------- .../api/console/proxy/tests/body.test.ts | 41 ++--- .../api/console/proxy/tests/headers.test.ts | 31 ++-- .../api/console/proxy/tests/params.test.ts | 28 ++- .../proxy/tests/proxy_fallback.test.ts | 77 -------- .../console/proxy/tests/query_string.test.ts | 36 ++-- 6 files changed, 119 insertions(+), 267 deletions(-) delete mode 100644 src/plugins/console/server/routes/api/console/proxy/tests/proxy_fallback.test.ts diff --git a/src/plugins/console/server/routes/api/console/proxy/create_handler.ts b/src/plugins/console/server/routes/api/console/proxy/create_handler.ts index 9f564d9c8b81..2ed943f4576a 100644 --- a/src/plugins/console/server/routes/api/console/proxy/create_handler.ts +++ b/src/plugins/console/server/routes/api/console/proxy/create_handler.ts @@ -28,22 +28,12 @@ * under the License. */ -import { Agent, IncomingMessage } from 'http'; -import * as url from 'url'; -import { pick, trimStart, trimEnd } from 'lodash'; - import { OpenSearchDashboardsRequest, RequestHandler } from 'opensearch-dashboards/server'; +import { trimStart } from 'lodash'; -import { OpenSearchConfigForProxy } from '../../../../types'; -import { - getOpenSearchProxyConfig, - ProxyConfigCollection, - proxyRequest, - setHeaders, -} from '../../../../lib'; +import { ResponseError } from '@opensearch-project/opensearch/lib/errors'; +import { ApiResponse } from '@opensearch-project/opensearch/'; -// TODO: find a better way to get information from the request like remoteAddress and remotePort -// for forwarding. // eslint-disable-next-line @osd/eslint/no-restricted-paths import { ensureRawRequest } from '../../../../../../../core/server/http/router'; @@ -51,54 +41,6 @@ import { RouteDependencies } from '../../../'; import { Body, Query } from './validation_config'; -function toURL(base: string, path: string) { - const urlResult = new url.URL(`${trimEnd(base, '/')}/${trimStart(path, '/')}`); - // Appending pretty here to have OpenSearch do the JSON formatting, as doing - // in JS can lead to data loss (7.0 will get munged into 7, thus losing indication of - // measurement precision) - if (!urlResult.searchParams.get('pretty')) { - urlResult.searchParams.append('pretty', 'true'); - } - return urlResult; -} - -function filterHeaders(originalHeaders: object, headersToKeep: string[]): object { - const normalizeHeader = function (header: any) { - if (!header) { - return ''; - } - header = header.toString(); - return header.trim().toLowerCase(); - }; - - // Normalize list of headers we want to allow in upstream request - const headersToKeepNormalized = headersToKeep.map(normalizeHeader); - - return pick(originalHeaders, headersToKeepNormalized); -} - -function getRequestConfig( - headers: object, - opensearchConfig: OpenSearchConfigForProxy, - proxyConfigCollection: ProxyConfigCollection, - uri: string -): { agent: Agent; timeout: number; headers: object; rejectUnauthorized?: boolean } { - const filteredHeaders = filterHeaders(headers, opensearchConfig.requestHeadersWhitelist); - const newHeaders = setHeaders(filteredHeaders, opensearchConfig.customHeaders); - - if (proxyConfigCollection.hasConfig()) { - return { - ...proxyConfigCollection.configForUri(uri), - headers: newHeaders, - } as any; - } - - return { - ...getOpenSearchProxyConfig(opensearchConfig), - headers: newHeaders, - }; -} - function getProxyHeaders(req: OpenSearchDashboardsRequest) { const headers = Object.create(null); @@ -124,12 +66,26 @@ function getProxyHeaders(req: OpenSearchDashboardsRequest) { return headers; } +function toUrlPath(path: string) { + let urlPath = `/${trimStart(path, '/')}`; + // Appending pretty here to have OpenSearch do the JSON formatting, as doing + // in JS can lead to data loss (7.0 will get munged into 7, thus losing indication of + // measurement precision) + if (!urlPath.includes('?pretty')) { + urlPath += '?pretty=true'; + } + return urlPath; +} + export const createHandler = ({ log, proxy: { readLegacyOpenSearchConfig, pathFilters, proxyConfigCollection }, }: RouteDependencies): RequestHandler => async (ctx, request, response) => { const { body, query } = request; const { path, method } = query; + const client = ctx.core.opensearch.client.asCurrentUser; + + let opensearchResponse: ApiResponse; if (!pathFilters.some((re) => re.test(path))) { return response.forbidden({ @@ -140,77 +96,46 @@ export const createHandler = ({ }); } - const legacyConfig = await readLegacyOpenSearchConfig(); - const { hosts } = legacyConfig; - let opensearchIncomingMessage: IncomingMessage; - - for (let idx = 0; idx < hosts.length; ++idx) { - const host = hosts[idx]; - try { - const uri = toURL(host, path); - - // Because this can technically be provided by a settings-defined proxy config, we need to - // preserve these property names to maintain BWC. - const { timeout, agent, headers, rejectUnauthorized } = getRequestConfig( - request.headers, - legacyConfig, - proxyConfigCollection, - uri.toString() - ); - - const requestHeaders = { - ...headers, - ...getProxyHeaders(request), - }; - - opensearchIncomingMessage = await proxyRequest({ - method: method.toLowerCase() as any, - headers: requestHeaders, - uri, - timeout, - payload: body, - rejectUnauthorized, - agent, + try { + const requestHeaders = { + ...getProxyHeaders(request), + }; + + opensearchResponse = await client.transport.request( + { path: toUrlPath(path), method, body }, + { headers: requestHeaders } + ); + + const { statusCode, body: responseContent, warnings } = opensearchResponse; + + if (method.toUpperCase() !== 'HEAD') { + return response.custom({ + statusCode: statusCode!, + body: responseContent, + headers: { + warning: warnings || '', + }, }); - - break; - } catch (e) { - // If we reached here it means we hit a lower level network issue than just, for e.g., a 500. - // We try contacting another node in that case. - log.error(e); - if (idx === hosts.length - 1) { - log.warn(`Could not connect to any configured OpenSearch node [${hosts.join(', ')}]`); - return response.customError({ - statusCode: 502, - body: e, - }); - } - // Otherwise, try the next host... } - } - - const { - statusCode, - statusMessage, - headers: { warning }, - } = opensearchIncomingMessage!; - if (method.toUpperCase() !== 'HEAD') { return response.custom({ statusCode: statusCode!, - body: opensearchIncomingMessage!, + body: `${statusCode} - ${responseContent}`, headers: { - warning: warning || '', + warning: warnings || '', + 'Content-Type': 'text/plain', }, }); + } catch (e: any) { + log.error(e); + const isResponseErrorFlag = isResponseError(e); + return response.customError({ + statusCode: isResponseErrorFlag ? e.statusCode : 502, + body: isResponseErrorFlag ? JSON.stringify(e.meta.body) : `502.${e.statusCode || 0}`, + }); } +}; - return response.custom({ - statusCode: statusCode!, - body: `${statusCode} - ${statusMessage}`, - headers: { - warning: warning || '', - 'Content-Type': 'text/plain', - }, - }); +const isResponseError = (error: any): error is ResponseError => { + return Boolean(error && error.body && error.statusCode && error.header); }; diff --git a/src/plugins/console/server/routes/api/console/proxy/tests/body.test.ts b/src/plugins/console/server/routes/api/console/proxy/tests/body.test.ts index 92349984dade..a7fb88a8bdaa 100644 --- a/src/plugins/console/server/routes/api/console/proxy/tests/body.test.ts +++ b/src/plugins/console/server/routes/api/console/proxy/tests/body.test.ts @@ -31,23 +31,31 @@ import { getProxyRouteHandlerDeps } from './mocks'; import expect from '@osd/expect'; -import { Readable } from 'stream'; -import { opensearchDashboardsResponseFactory } from '../../../../../../../../core/server'; +import { + IScopedClusterClient, + opensearchDashboardsResponseFactory, +} from '../../../../../../../../core/server'; import { createHandler } from '../create_handler'; -import * as requestModule from '../../../../../lib/proxy_request'; -import { createResponseStub } from './stubs'; + +import { coreMock, opensearchServiceMock } from '../../../../../../../../core/server/mocks'; describe('Console Proxy Route', () => { let request: any; + let opensearchClient: DeeplyMockedKeys; beforeEach(() => { request = (method: string, path: string, response: string) => { - (requestModule.proxyRequest as jest.Mock).mockResolvedValue(createResponseStub(response)); + const mockResponse = opensearchServiceMock.createSuccessTransportRequestPromise(response); + + const requestHandlerContextMock = coreMock.createRequestHandlerContext(); + opensearchClient = requestHandlerContextMock.opensearch.client; + + opensearchClient.asCurrentUser.transport.request.mockResolvedValueOnce(mockResponse); const handler = createHandler(getProxyRouteHandlerDeps({})); return handler( - {} as any, + { core: requestHandlerContextMock, dataSource: {} as any }, { headers: {}, query: { method, path }, @@ -57,15 +65,6 @@ describe('Console Proxy Route', () => { }; }); - const readStream = (s: Readable) => - new Promise((resolve) => { - let v = ''; - s.on('data', (data) => { - v += data; - }); - s.on('end', () => resolve(v)); - }); - afterEach(async () => { jest.resetAllMocks(); }); @@ -74,36 +73,36 @@ describe('Console Proxy Route', () => { describe('GET request', () => { it('returns the exact body', async () => { const { payload } = await request('GET', '/', 'foobar'); - expect(await readStream(payload)).to.be('foobar'); + expect(payload).to.be('foobar'); }); }); describe('POST request', () => { it('returns the exact body', async () => { const { payload } = await request('POST', '/', 'foobar'); - expect(await readStream(payload)).to.be('foobar'); + expect(payload).to.be('foobar'); }); }); describe('PUT request', () => { it('returns the exact body', async () => { const { payload } = await request('PUT', '/', 'foobar'); - expect(await readStream(payload)).to.be('foobar'); + expect(payload).to.be('foobar'); }); }); describe('DELETE request', () => { it('returns the exact body', async () => { const { payload } = await request('DELETE', '/', 'foobar'); - expect(await readStream(payload)).to.be('foobar'); + expect(payload).to.be('foobar'); }); }); describe('HEAD request', () => { it('returns the status code and text', async () => { - const { payload } = await request('HEAD', '/'); + const { payload } = await request('HEAD', '/', 'OK'); expect(typeof payload).to.be('string'); expect(payload).to.be('200 - OK'); }); describe('mixed casing', () => { it('returns the status code and text', async () => { - const { payload } = await request('HeAd', '/'); + const { payload } = await request('HeAd', '/', 'OK'); expect(typeof payload).to.be('string'); expect(payload).to.be('200 - OK'); }); diff --git a/src/plugins/console/server/routes/api/console/proxy/tests/headers.test.ts b/src/plugins/console/server/routes/api/console/proxy/tests/headers.test.ts index 488d52a4fc8b..a1964d160e2c 100644 --- a/src/plugins/console/server/routes/api/console/proxy/tests/headers.test.ts +++ b/src/plugins/console/server/routes/api/console/proxy/tests/headers.test.ts @@ -32,25 +32,21 @@ jest.mock('../../../../../../../../core/server/http/router/request', () => ({ ensureRawRequest: jest.fn(), })); -import { opensearchDashboardsResponseFactory } from '../../../../../../../../core/server'; - +import { + IScopedClusterClient, + opensearchDashboardsResponseFactory, +} from '../../../../../../../../core/server'; // eslint-disable-next-line @osd/eslint/no-restricted-paths import { ensureRawRequest } from '../../../../../../../../core/server/http/router/request'; - import { getProxyRouteHandlerDeps } from './mocks'; - import expect from '@osd/expect'; -import * as requestModule from '../../../../../lib/proxy_request'; - import { createHandler } from '../create_handler'; - -import { createResponseStub } from './stubs'; +import { coreMock } from '../../../../../../../../core/server/mocks'; describe('Console Proxy Route', () => { let handler: ReturnType; beforeEach(() => { - (requestModule.proxyRequest as jest.Mock).mockResolvedValue(createResponseStub('')); handler = createHandler(getProxyRouteHandlerDeps({})); }); @@ -59,7 +55,10 @@ describe('Console Proxy Route', () => { }); describe('headers', () => { + let opensearchClient: DeeplyMockedKeys; it('forwards the remote header info', async () => { + const requestHandlerContextMock = coreMock.createRequestHandlerContext(); + opensearchClient = requestHandlerContextMock.opensearch.client; (ensureRawRequest as jest.Mock).mockReturnValue({ // This mocks the shape of the hapi request object, will probably change info: { @@ -75,7 +74,7 @@ describe('Console Proxy Route', () => { }); await handler( - {} as any, + { core: requestHandlerContextMock, dataSource: {} as any }, { headers: {}, query: { @@ -86,16 +85,16 @@ describe('Console Proxy Route', () => { opensearchDashboardsResponseFactory ); - expect((requestModule.proxyRequest as jest.Mock).mock.calls.length).to.be(1); - const [[{ headers }]] = (requestModule.proxyRequest as jest.Mock).mock.calls; + const [[, opts]] = opensearchClient.asCurrentUser.transport.request.mock.calls; + const headers = opts?.headers; expect(headers).to.have.property('x-forwarded-for'); - expect(headers['x-forwarded-for']).to.be('0.0.0.0'); + expect(headers!['x-forwarded-for']).to.be('0.0.0.0'); expect(headers).to.have.property('x-forwarded-port'); - expect(headers['x-forwarded-port']).to.be('1234'); + expect(headers!['x-forwarded-port']).to.be('1234'); expect(headers).to.have.property('x-forwarded-proto'); - expect(headers['x-forwarded-proto']).to.be('http'); + expect(headers!['x-forwarded-proto']).to.be('http'); expect(headers).to.have.property('x-forwarded-host'); - expect(headers['x-forwarded-host']).to.be('test'); + expect(headers!['x-forwarded-host']).to.be('test'); }); }); }); diff --git a/src/plugins/console/server/routes/api/console/proxy/tests/params.test.ts b/src/plugins/console/server/routes/api/console/proxy/tests/params.test.ts index da6b83f54dab..80523c8031df 100644 --- a/src/plugins/console/server/routes/api/console/proxy/tests/params.test.ts +++ b/src/plugins/console/server/routes/api/console/proxy/tests/params.test.ts @@ -28,16 +28,24 @@ * under the License. */ -import { opensearchDashboardsResponseFactory } from '../../../../../../../../core/server'; +import { + IScopedClusterClient, + opensearchDashboardsResponseFactory, +} from '../../../../../../../../core/server'; import { getProxyRouteHandlerDeps } from './mocks'; -import { createResponseStub } from './stubs'; -import * as requestModule from '../../../../../lib/proxy_request'; import expect from '@osd/expect'; import { createHandler } from '../create_handler'; +import { coreMock, opensearchServiceMock } from '../../../../../../../../core/server/mocks'; describe('Console Proxy Route', () => { let handler: ReturnType; + let requestHandlerContextMock: any; + let opensearchClient: DeeplyMockedKeys; + beforeEach(() => { + requestHandlerContextMock = coreMock.createRequestHandlerContext(); + opensearchClient = requestHandlerContextMock.opensearch.client; + }); afterEach(() => { jest.resetAllMocks(); @@ -52,7 +60,7 @@ describe('Console Proxy Route', () => { ); const { status } = await handler( - {} as any, + { core: requestHandlerContextMock, dataSource: {} as any }, { query: { method: 'POST', path: '/baz/id' } } as any, opensearchDashboardsResponseFactory ); @@ -66,16 +74,16 @@ describe('Console Proxy Route', () => { getProxyRouteHandlerDeps({ proxy: { pathFilters: [/^\/foo\//, /^\/bar\//] } }) ); - (requestModule.proxyRequest as jest.Mock).mockResolvedValue(createResponseStub('foo')); + const mockResponse = opensearchServiceMock.createSuccessTransportRequestPromise('foo'); + opensearchClient.asCurrentUser.transport.request.mockResolvedValueOnce(mockResponse); const { status } = await handler( - {} as any, + { core: requestHandlerContextMock, dataSource: {} as any }, { headers: {}, query: { method: 'POST', path: '/foo/id' } } as any, opensearchDashboardsResponseFactory ); expect(status).to.be(200); - expect((requestModule.proxyRequest as jest.Mock).mock.calls.length).to.be(1); }); }); describe('all match', () => { @@ -84,16 +92,16 @@ describe('Console Proxy Route', () => { getProxyRouteHandlerDeps({ proxy: { pathFilters: [/^\/foo\//] } }) ); - (requestModule.proxyRequest as jest.Mock).mockResolvedValue(createResponseStub('foo')); + const mockResponse = opensearchServiceMock.createSuccessTransportRequestPromise('foo'); + opensearchClient.asCurrentUser.transport.request.mockResolvedValueOnce(mockResponse); const { status } = await handler( - {} as any, + { core: requestHandlerContextMock, dataSource: {} as any }, { headers: {}, query: { method: 'GET', path: '/foo/id' } } as any, opensearchDashboardsResponseFactory ); expect(status).to.be(200); - expect((requestModule.proxyRequest as jest.Mock).mock.calls.length).to.be(1); }); }); }); diff --git a/src/plugins/console/server/routes/api/console/proxy/tests/proxy_fallback.test.ts b/src/plugins/console/server/routes/api/console/proxy/tests/proxy_fallback.test.ts deleted file mode 100644 index 585f76066fad..000000000000 --- a/src/plugins/console/server/routes/api/console/proxy/tests/proxy_fallback.test.ts +++ /dev/null @@ -1,77 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - * - * Any modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you 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 { duration } from 'moment'; -import { getProxyRouteHandlerDeps } from './mocks'; - -import { opensearchDashboardsResponseFactory } from '../../../../../../../../core/server'; -import * as requestModule from '../../../../../lib/proxy_request'; -import { createHandler } from '../create_handler'; - -describe('Console Proxy Route', () => { - afterEach(async () => { - jest.resetAllMocks(); - }); - - describe('fallback behaviour', () => { - it('falls back to all configured endpoints regardless of error', async () => { - // Describe a situation where all three configured nodes reject - (requestModule.proxyRequest as jest.Mock).mockRejectedValueOnce(new Error('ECONNREFUSED')); - (requestModule.proxyRequest as jest.Mock).mockRejectedValueOnce(new Error('EHOSTUNREACH')); - (requestModule.proxyRequest as jest.Mock).mockRejectedValueOnce(new Error('ESOCKETTIMEDOUT')); - - const handler = createHandler( - getProxyRouteHandlerDeps({ - proxy: { - readLegacyOpenSearchConfig: async () => ({ - requestTimeout: duration(30000), - customHeaders: {}, - requestHeadersWhitelist: [], - hosts: ['http://localhost:9201', 'http://localhost:9202', 'http://localhost:9203'], - }), - }, - }) - ); - - const response = await handler( - {} as any, - { - headers: {}, - query: { method: 'get', path: 'test' }, - } as any, - opensearchDashboardsResponseFactory - ); - - expect(response.status).toBe(502); - // Return the message from the OpenSearch node we attempted last. - expect(response.payload.message).toBe('ESOCKETTIMEDOUT'); - }); - }); -}); diff --git a/src/plugins/console/server/routes/api/console/proxy/tests/query_string.test.ts b/src/plugins/console/server/routes/api/console/proxy/tests/query_string.test.ts index 9b73457f3f89..4b4c412d4cd2 100644 --- a/src/plugins/console/server/routes/api/console/proxy/tests/query_string.test.ts +++ b/src/plugins/console/server/routes/api/console/proxy/tests/query_string.test.ts @@ -28,25 +28,26 @@ * under the License. */ -import { opensearchDashboardsResponseFactory } from '../../../../../../../../core/server'; +import { + IScopedClusterClient, + opensearchDashboardsResponseFactory, +} from '../../../../../../../../core/server'; import { getProxyRouteHandlerDeps } from './mocks'; -import { createResponseStub } from './stubs'; -import * as requestModule from '../../../../../lib/proxy_request'; - -import expect from '@osd/expect'; - import { createHandler } from '../create_handler'; +import { coreMock } from '../../../../../../../../core/server/mocks'; describe('Console Proxy Route', () => { let request: any; + let opensearchClient: DeeplyMockedKeys; beforeEach(() => { - (requestModule.proxyRequest as jest.Mock).mockResolvedValue(createResponseStub('foo')); + const requestHandlerContextMock = coreMock.createRequestHandlerContext(); + opensearchClient = requestHandlerContextMock.opensearch.client; request = async (method: string, path: string) => { const handler = createHandler(getProxyRouteHandlerDeps({})); return handler( - {} as any, + { core: requestHandlerContextMock, dataSource: {} as any }, { headers: {}, query: { method, path } } as any, opensearchDashboardsResponseFactory ); @@ -62,25 +63,22 @@ describe('Console Proxy Route', () => { describe('contains full url', () => { it('treats the url as a path', async () => { await request('GET', 'http://evil.com/test'); - expect((requestModule.proxyRequest as jest.Mock).mock.calls.length).to.be(1); - const [[args]] = (requestModule.proxyRequest as jest.Mock).mock.calls; - expect(args.uri.href).to.be('http://localhost:9200/http://evil.com/test?pretty=true'); + const [[args]] = opensearchClient.asCurrentUser.transport.request.mock.calls; + expect(args.path).toBe('/http://evil.com/test?pretty=true'); }); }); describe('starts with a slash', () => { - it('combines well with the base url', async () => { + it('keeps as it is', async () => { await request('GET', '/index/id'); - expect((requestModule.proxyRequest as jest.Mock).mock.calls.length).to.be(1); - const [[args]] = (requestModule.proxyRequest as jest.Mock).mock.calls; - expect(args.uri.href).to.be('http://localhost:9200/index/id?pretty=true'); + const [[args]] = opensearchClient.asCurrentUser.transport.request.mock.calls; + expect(args.path).toBe('/index/id?pretty=true'); }); }); describe(`doesn't start with a slash`, () => { - it('combines well with the base url', async () => { + it('adds slash to path before sending request', async () => { await request('GET', 'index/id'); - expect((requestModule.proxyRequest as jest.Mock).mock.calls.length).to.be(1); - const [[args]] = (requestModule.proxyRequest as jest.Mock).mock.calls; - expect(args.uri.href).to.be('http://localhost:9200/index/id?pretty=true'); + const [[args]] = opensearchClient.asCurrentUser.transport.request.mock.calls; + expect(args.path).toBe('/index/id?pretty=true'); }); }); });