diff --git a/charts/__tests__/gardener-dashboard/runtime/dashboard/deployment.spec.js b/charts/__tests__/gardener-dashboard/runtime/dashboard/deployment.spec.js index 6d8d8bd371..484882b203 100644 --- a/charts/__tests__/gardener-dashboard/runtime/dashboard/deployment.spec.js +++ b/charts/__tests__/gardener-dashboard/runtime/dashboard/deployment.spec.js @@ -68,6 +68,46 @@ describe('gardener-dashboard', function () { expect(dashboardContainer.env).toMatchSnapshot() }) + it('should render the kube client request timeout environment variable', async function () { + const values = { + global: { + dashboard: { + kubeClient: { + requestTimeout: 30000, + }, + }, + }, + } + const documents = await renderTemplates(templates, values) + expect(documents).toHaveLength(1) + const [deployment] = documents + const dashboardContainer = deployment.spec.template.spec.containers[0] + expect(dashboardContainer.env).toEqual(expect.arrayContaining([{ + name: 'KUBE_CLIENT_REQUEST_TIMEOUT', + value: '30000', + }])) + }) + + it('should render kube client request timeout 0', async function () { + const values = { + global: { + dashboard: { + kubeClient: { + requestTimeout: 0, + }, + }, + }, + } + const documents = await renderTemplates(templates, values) + expect(documents).toHaveLength(1) + const [deployment] = documents + const dashboardContainer = deployment.spec.template.spec.containers[0] + expect(dashboardContainer.env).toEqual(expect.arrayContaining([{ + name: 'KUBE_CLIENT_REQUEST_TIMEOUT', + value: '0', + }])) + }) + it('should render the template with a sha256 tag', async function () { const tag = 'sha256:4d529c1' const values = { diff --git a/charts/gardener-dashboard/charts/runtime/templates/dashboard/deployment.yaml b/charts/gardener-dashboard/charts/runtime/templates/dashboard/deployment.yaml index 8cf40aee93..f9f0d221d9 100644 --- a/charts/gardener-dashboard/charts/runtime/templates/dashboard/deployment.yaml +++ b/charts/gardener-dashboard/charts/runtime/templates/dashboard/deployment.yaml @@ -194,6 +194,10 @@ spec: - name: KUBECONFIG value: {{ required ".Values.global.dashboard.projectedKubeconfig.baseMountPath is required" .Values.global.dashboard.projectedKubeconfig.baseMountPath }}/kubeconfig {{- end }} + {{- if ne .Values.global.dashboard.kubeClient.requestTimeout nil }} + - name: KUBE_CLIENT_REQUEST_TIMEOUT + value: {{ quote .Values.global.dashboard.kubeClient.requestTimeout }} + {{- end }} - name: METRICS_PORT value: {{ quote .Values.global.dashboard.metricsContainerPort }} - name: POD_NAME diff --git a/charts/gardener-dashboard/values.yaml b/charts/gardener-dashboard/values.yaml index 8b1a2c0e91..10ddaf5d2c 100644 --- a/charts/gardener-dashboard/values.yaml +++ b/charts/gardener-dashboard/values.yaml @@ -29,6 +29,11 @@ global: expirationSeconds: 43200 # 12 hours audience: '' + kubeClient: + # Optional total request timeout in milliseconds for Kubernetes API requests. + # Defaults to 60000 when unset. Set to 0 to disable. + requestTimeout: ~ + # If configured, the dashboard deployment uses a projected volume which presents the kubeconfig to the garden cluster. # projectedKubeconfig: # # Path the projected volume is mounted to. This is typically also the base path in the generic kubeconfig to refer to the token file. diff --git a/packages/kube-client/__tests__/cache.list-watcher.spec.js b/packages/kube-client/__tests__/cache.list-watcher.spec.js index 321e52c214..45c61199ee 100644 --- a/packages/kube-client/__tests__/cache.list-watcher.spec.js +++ b/packages/kube-client/__tests__/cache.list-watcher.spec.js @@ -47,6 +47,17 @@ describe('kube-client', () => { }]) }) + it('#list with signal', () => { + const signal = {} + listWatcher.setAbortSignal(signal) + expect(listWatcher.list({ b: 2 })).toBe(body) + expect(listFunc).toHaveBeenCalledTimes(1) + expect(listFunc.mock.calls[0]).toEqual([{ + signal, + searchParams: new URLSearchParams({ a: 1, b: 2 }), + }]) + }) + it('#watch', () => { const signal = {} listWatcher.setAbortSignal(signal) diff --git a/packages/kube-client/__tests__/index.spec.js b/packages/kube-client/__tests__/index.spec.js new file mode 100644 index 0000000000..fa550dcdb8 --- /dev/null +++ b/packages/kube-client/__tests__/index.spec.js @@ -0,0 +1,139 @@ +// +// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and Gardener contributors +// +// SPDX-License-Identifier: Apache-2.0 +// + +import { vi } from 'vitest' + +const ENV_NAME = 'KUBE_CLIENT_REQUEST_TIMEOUT' +const originalRequestTimeout = process.env[ENV_NAME] + +async function importKubeClientWithEnv (value) { + vi.resetModules() + if (value === undefined) { + delete process.env[ENV_NAME] + } else { + process.env[ENV_NAME] = value + } + + const { default: kubeClient } = await import('../lib/index.js') + const { default: request } = await import('@gardener-dashboard/request') + return { kubeClient, request } +} + +function requestTimeoutsFromExtendCalls (request) { + return request.extend.mock.calls.map(([clientConfig]) => clientConfig.requestTimeout) +} + +function expectAllClientsToUseRequestTimeout (request, requestTimeout) { + expect(request.extend).toHaveBeenCalled() + expect(requestTimeoutsFromExtendCalls(request)).toEqual( + expect.arrayContaining([requestTimeout]), + ) + expect(new Set(requestTimeoutsFromExtendCalls(request))).toEqual(new Set([requestTimeout])) +} + +describe('kube-client package defaults', () => { + afterEach(() => { + if (originalRequestTimeout === undefined) { + delete process.env[ENV_NAME] + } else { + process.env[ENV_NAME] = originalRequestTimeout + } + vi.resetModules() + vi.clearAllMocks() + }) + + it('should apply KUBE_CLIENT_REQUEST_TIMEOUT to the package-level dashboard client', async () => { + expect.hasAssertions() + const { request } = await importKubeClientWithEnv('1234') + + expectAllClientsToUseRequestTimeout(request, 1234) + }) + + it('should apply KUBE_CLIENT_REQUEST_TIMEOUT to user clients', async () => { + expect.hasAssertions() + const { kubeClient, request } = await importKubeClientWithEnv('2345') + + request.extend.mockClear() + kubeClient.createClient({ auth: { bearer: 'bearer' } }) + + expectAllClientsToUseRequestTimeout(request, 2345) + }) + + it('should apply KUBE_CLIENT_REQUEST_TIMEOUT to dashboard clients', async () => { + expect.hasAssertions() + const { kubeClient, request } = await importKubeClientWithEnv('3456') + + request.extend.mockClear() + kubeClient.createDashboardClient() + + expectAllClientsToUseRequestTimeout(request, 3456) + }) + + it('should allow per-client options to override KUBE_CLIENT_REQUEST_TIMEOUT', async () => { + expect.hasAssertions() + const { kubeClient, request } = await importKubeClientWithEnv('4567') + + request.extend.mockClear() + kubeClient.createDashboardClient({ requestTimeout: 7654 }) + + expectAllClientsToUseRequestTimeout(request, 7654) + }) + + it('should allow per-client requestTimeout 0 to override KUBE_CLIENT_REQUEST_TIMEOUT', async () => { + expect.hasAssertions() + const { kubeClient, request } = await importKubeClientWithEnv('4567') + + request.extend.mockClear() + kubeClient.createDashboardClient({ requestTimeout: 0 }) + + expectAllClientsToUseRequestTimeout(request, 0) + }) + + it('should ignore explicit undefined requestTimeout options when applying KUBE_CLIENT_REQUEST_TIMEOUT', async () => { + expect.hasAssertions() + const { kubeClient, request } = await importKubeClientWithEnv('4567') + + request.extend.mockClear() + kubeClient.createDashboardClient({ requestTimeout: undefined }) + + expectAllClientsToUseRequestTimeout(request, 4567) + }) + + it('should apply KUBE_CLIENT_REQUEST_TIMEOUT to derived kubeconfig clients', async () => { + expect.hasAssertions() + const { kubeClient, request } = await importKubeClientWithEnv('5678') + const { default: helper } = await import('./fixtures/helper.js') + const client = kubeClient.createDashboardClient() + const getSecretStub = vi.spyOn(client.core.secrets, 'get') + const testKubeconfig = helper.createTestKubeconfig({ token: 'bearer' }, { server: 'https://kubernetes:6443' }) + getSecretStub.mockReturnValue({ + data: { + kubeconfig: Buffer.from(testKubeconfig.toYAML()).toString('base64'), + }, + }) + + request.extend.mockClear() + await client.createKubeconfigClient({ namespace: 'namespace', name: 'name' }) + + expectAllClientsToUseRequestTimeout(request, 5678) + }) + + it('should allow KUBE_CLIENT_REQUEST_TIMEOUT 0 to disable request timeouts', async () => { + expect.hasAssertions() + const { request } = await importKubeClientWithEnv('0') + + expectAllClientsToUseRequestTimeout(request, 0) + }) + + it.each(['foo', '-1', '1.5', '2147483648'])('should fail fast for invalid KUBE_CLIENT_REQUEST_TIMEOUT value %s', async value => { + vi.resetModules() + process.env[ENV_NAME] = value + + await expect(import('../lib/index.js')).rejects.toThrow( + 'KUBE_CLIENT_REQUEST_TIMEOUT must be a non-negative integer <= 2147483647', + ) + }) +}) diff --git a/packages/kube-client/__tests__/mixins.spec.js b/packages/kube-client/__tests__/mixins.spec.js index 8aa17de05c..4048402615 100644 --- a/packages/kube-client/__tests__/mixins.spec.js +++ b/packages/kube-client/__tests__/mixins.spec.js @@ -94,10 +94,19 @@ describe('kube-client', () => { describe('Readable', () => { it('should get a resource', () => { const testObject = new TestObject() - const [url, { method, searchParams }] = testObject.get('name', {}) + const signal = new AbortController().signal + const [url, { method, searchParams, signal: forwardedSignal }] = testObject.get('name', { signal }) expect(url).toBe('dummies/name') expect(method).toBe('get') expect(searchParams.toString()).toBe('') + expect(forwardedSignal).toBe(signal) + }) + + it('should reject invalid get signals', () => { + const testObject = new TestObject() + expect(() => testObject.get('name', { signal: {} })).toThrow( + 'The parameter "signal" must be empty or an instance of AbortSignal', + ) }) it('should list a resource', async () => { @@ -272,10 +281,19 @@ describe('kube-client', () => { describe('Readable', () => { it('should get a resource', () => { const testObject = new TestObject() - const [url, { method, searchParams }] = testObject.get('namespace', 'name', {}) + const signal = new AbortController().signal + const [url, { method, searchParams, signal: forwardedSignal }] = testObject.get('namespace', 'name', { signal }) expect(url).toBe('namespaces/namespace/dummies/name') expect(method).toBe('get') expect(searchParams.toString()).toBe('') + expect(forwardedSignal).toBe(signal) + }) + + it('should reject invalid get signals', () => { + const testObject = new TestObject() + expect(() => testObject.get('namespace', 'name', { signal: {} })).toThrow( + 'The parameter "signal" must be empty or an instance of AbortSignal', + ) }) it('should list a resource', async () => { diff --git a/packages/kube-client/lib/cache/ListWatcher.js b/packages/kube-client/lib/cache/ListWatcher.js index e6525182b2..7291ba5243 100644 --- a/packages/kube-client/lib/cache/ListWatcher.js +++ b/packages/kube-client/lib/cache/ListWatcher.js @@ -37,6 +37,9 @@ class ListWatcher { list (query) { const searchParams = this.mergeSearchParams(query) const options = { searchParams } + if (this.signal) { + options.signal = this.signal + } return this.listFunc(options) } } diff --git a/packages/kube-client/lib/index.js b/packages/kube-client/lib/index.js index 0853653404..f37a0f9c03 100644 --- a/packages/kube-client/lib/index.js +++ b/packages/kube-client/lib/index.js @@ -5,22 +5,56 @@ // import assert from 'node:assert' -import Client from './Client.js' +import BaseClient from './Client.js' import Store from './cache/Store.js' import { Resources } from './resources/index.js' import kubeConfig from '@gardener-dashboard/kube-config' const { load } = kubeConfig +const MAX_TIMEOUT = 2_147_483_647 // Node.js TIMEOUT_MAX (2^31 - 1) + +function parseRequestTimeout (value) { + if (value === undefined || value === '') { + return undefined + } + const requestTimeout = /^\d+$/.test(value) ? Number(value) : NaN + if (!Number.isFinite(requestTimeout) || requestTimeout > MAX_TIMEOUT) { + throw TypeError(`KUBE_CLIENT_REQUEST_TIMEOUT must be a non-negative integer <= ${MAX_TIMEOUT}`) + } + return requestTimeout +} + +const requestTimeout = parseRequestTimeout(process.env.KUBE_CLIENT_REQUEST_TIMEOUT) +const defaultOptions = requestTimeout === undefined + ? {} + : { requestTimeout } + +class Client extends BaseClient { + constructor (clientConfig, options = {}) { + const { requestTimeout, ...rest } = options + + const resolvedOptions = { + ...defaultOptions, + ...rest, + } + + if (requestTimeout !== undefined) { + resolvedOptions.requestTimeout = requestTimeout + } + + super(clientConfig, resolvedOptions) + } +} const ac = new AbortController() const clientConfig = load(process.env, { signal: ac.signal }) -function createClient (options) { +function createClient (options = {}) { assert.ok(options.auth && options.auth.bearer, 'Client credentials are required') return new Client(clientConfig, options) } -function createDashboardClient (options) { +function createDashboardClient (options = {}) { return new Client(clientConfig, options) } @@ -29,7 +63,7 @@ function abortWatcher () { } // create a client instance for the gardener cluster with dashboard privileges -const dashboardClient = new Client(clientConfig) +const dashboardClient = createDashboardClient() export default { createClient, diff --git a/packages/kube-client/lib/mixins.js b/packages/kube-client/lib/mixins.js index f052f5b717..e716851595 100644 --- a/packages/kube-client/lib/mixins.js +++ b/packages/kube-client/lib/mixins.js @@ -60,15 +60,17 @@ ClusterScoped.Readable = superclass => class extends superclass { get (name, { searchParams, signal, ...options } = {}) { assertName(name) assertSearchParams(searchParams) + assertSignal(signal) assertOptions(options) const method = 'get' const url = clusterScopedUrl(this.constructor.names, name) searchParams = normalizeSearchParams(method, searchParams, options) - return this[http.request](url, { method, searchParams }) + return this[http.request](url, { method, searchParams, signal }) } list ({ searchParams, signal, ...options } = {}) { assertSearchParams(searchParams) + assertSignal(signal) assertOptions(options) const method = 'get' const url = clusterScopedUrl(this.constructor.names) @@ -83,16 +85,18 @@ NamespaceScoped.Readable = superclass => class extends superclass { assertNamespace(namespace) assertName(name) assertSearchParams(searchParams) + assertSignal(signal) assertOptions(options) const method = 'get' const url = namespaceScopedUrl(this.constructor.names, namespace, name) searchParams = normalizeSearchParams(method, searchParams, options) - return this[http.request](url, { method, searchParams }) + return this[http.request](url, { method, searchParams, signal }) } list (namespace, { searchParams, signal, ...options } = {}) { assertNamespace(namespace) assertSearchParams(searchParams) + assertSignal(signal) assertOptions(options) const method = 'get' const url = namespaceScopedUrl(this.constructor.names, namespace) @@ -103,6 +107,7 @@ NamespaceScoped.Readable = superclass => class extends superclass { listAllNamespaces ({ searchParams, signal, ...options } = {}) { assertSearchParams(searchParams) + assertSignal(signal) assertOptions(options) const method = 'get' const url = namespaceScopedUrl(this.constructor.names) diff --git a/packages/request/__tests__/acceptance.spec.js b/packages/request/__tests__/acceptance.spec.js index 8f96bff05a..c4ba22d266 100644 --- a/packages/request/__tests__/acceptance.spec.js +++ b/packages/request/__tests__/acceptance.spec.js @@ -4,7 +4,6 @@ // SPDX-License-Identifier: Apache-2.0 // -import { vi } from 'vitest' import http from 'http' import http2 from 'http2' import zlib from 'zlib' @@ -148,6 +147,18 @@ function createSecureServer ({ cert, key }) { } stream.respond(headers) await pipeline(streams) + } else if (path === '/delay') { + // Never respond — used to test requestTimeout with a real connection. + } else if (path === '/stall-body') { + stream.respond({ + [HTTP2_HEADER_STATUS]: statusCode, + [HTTP2_HEADER_CONTENT_TYPE]: 'application/json', + }) + } else if (path === '/stall-events') { + stream.respond({ + [HTTP2_HEADER_STATUS]: statusCode, + [HTTP2_HEADER_CONTENT_TYPE]: 'application/json', + }) } else { body = JSON.stringify({ headers, @@ -174,14 +185,6 @@ describe('Acceptance Tests', function () { let client let server - beforeAll(() => { - vi.useFakeTimers({ legacyFakeTimers: true }) - }) - - afterAll(() => { - vi.useRealTimers() - }) - beforeEach(async () => { server = await createSecureServer({ cert, key }) agent = new Agent({ @@ -297,6 +300,66 @@ describe('Acceptance Tests', function () { }) }) + describe('#request with requestTimeout', function () { + it('should timeout when response headers are not received in time', async function () { + const requestTimeout = 100 + const timeoutClient = new Client({ + url: server.origin, + agent, + ca: cert, + requestTimeout, + }) + + await expect(timeoutClient.request('delay')).rejects.toMatchObject({ + name: 'TimeoutError', + code: 'ETIMEDOUT', + message: `Request exceeded ${requestTimeout} ms for GET /delay`, + }) + }) + + it('should timeout when response body stalls', async function () { + const requestTimeout = 100 + const timeoutClient = new Client({ + url: server.origin, + agent, + ca: cert, + requestTimeout, + }) + + const response = await timeoutClient.fetch('stall-body') + + await expect(response.body()).rejects.toMatchObject({ + name: 'TimeoutError', + code: 'ETIMEDOUT', + message: `Request exceeded ${requestTimeout} ms for GET /stall-body`, + }) + }) + + it('should timeout when the response iterator stalls', async function () { + const requestTimeout = 100 + const timeoutClient = new Client({ + url: server.origin, + agent, + ca: cert, + requestTimeout, + }) + + const response = await timeoutClient.fetch('stall-events') + + const eventsPromise = (async () => { + for await (const event of response) { + expect(event).toBeDefined() + } + })() + + await expect(eventsPromise).rejects.toMatchObject({ + name: 'TimeoutError', + code: 'ETIMEDOUT', + message: `Request exceeded ${requestTimeout} ms for GET /stall-events`, + }) + }) + }) + describe('#stream', function () { it('should send a GET request', async function () { const stream = await client.stream('events/abcde') diff --git a/packages/request/__tests__/client.spec.js b/packages/request/__tests__/client.spec.js index 445bce601a..4cbb4335d3 100644 --- a/packages/request/__tests__/client.spec.js +++ b/packages/request/__tests__/client.spec.js @@ -9,6 +9,7 @@ import http2 from 'http2' import zlib from 'zlib' import { globalLogger as logger } from '@gardener-dashboard/logger' import client from '../lib/index.js' +import { isRequestTimeoutAbort, mapTimeoutAbortError } from '../lib/Client.js' const { Client, extend } = client @@ -36,14 +37,6 @@ describe('Client', () => { let client let stream - beforeAll(() => { - vi.useFakeTimers({ legacyFakeTimers: true }) - }) - - afterAll(() => { - vi.useRealTimers() - }) - beforeEach(() => { const mockBody = vi.fn().mockReturnValue({ foo: 'bar', @@ -98,7 +91,6 @@ describe('Client', () => { it('should create a new object', () => { expect(client).toBeInstanceOf(Client) expect(client.baseUrl.href).toBe(url.href) - expect(client.responseTimeout).toBe(15000) }) it('should throw a type error', () => { @@ -244,6 +236,107 @@ describe('Client', () => { expect(body).toEqual(stream.mockBody()) }) + it('should timeout when a request exceeds its deadline before headers arrive', async () => { + const requestTimeout = 10 + const message = `Request exceeded ${requestTimeout} ms for GET /test/foo/bar` + client = new Client({ + url, + agent, + requestTimeout, + }) + let signal + let resolveGetHeadersCalled + const getHeadersCalled = new Promise(resolve => { + resolveGetHeadersCalled = resolve + }) + agent.request.mockImplementation(async (headers, options) => { + signal = options.signal + signal.addEventListener('abort', () => { + const err = new Error('The operation was aborted') + err.name = 'AbortError' + err.code = 'ABORT_ERR' + err.cause = signal.reason + stream.destroy(err) + }, { once: true }) + return stream + }) + stream.getHeaders = vi.fn(() => { + resolveGetHeadersCalled() + return new Promise((resolve, reject) => { + stream.destroy.mockImplementation(reject) + }) + }) + + const promise = client.fetch('foo/bar?token=secret') + await getHeadersCalled + const err = await promise.catch(err => err) + + expect(err).toMatchObject({ + name: 'TimeoutError', + code: 'ETIMEDOUT', + message, + }) + expect(err.cause).toMatchObject({ + name: 'AbortError', + code: 'ABORT_ERR', + }) + expect(stream.getHeaders).toHaveBeenCalledTimes(1) + expect(stream.destroy).toHaveBeenCalledTimes(1) + expect(stream.destroy).toHaveBeenCalledWith(expect.objectContaining({ + name: 'AbortError', + code: 'ABORT_ERR', + })) + }) + + it('should not create a timeout signal when requestTimeout is 0', async () => { + await client.fetch('foo/bar', { requestTimeout: 0 }) + + expect(agent.request).toHaveBeenCalledTimes(1) + expect(agent.request.mock.calls[0][1].signal).toBeUndefined() + }) + + it('should surface a caller-triggered abort as AbortError, not TimeoutError', async () => { + const requestTimeout = 60_000 + client = new Client({ + url, + agent, + requestTimeout, + }) + const abortController = new AbortController() + let resolveGetHeadersCalled + const getHeadersCalled = new Promise(resolve => { + resolveGetHeadersCalled = resolve + }) + agent.request.mockImplementation(async (headers, options) => { + const signal = options.signal + signal.addEventListener('abort', () => { + const err = new Error('The operation was aborted') + err.name = 'AbortError' + err.code = 'ABORT_ERR' + err.cause = signal.reason + stream.destroy(err) + }, { once: true }) + return stream + }) + stream.getHeaders = vi.fn(() => { + resolveGetHeadersCalled() + return new Promise((resolve, reject) => { + stream.destroy.mockImplementation(reject) + }) + }) + + const promise = client.fetch('foo/bar', { signal: abortController.signal }) + await getHeadersCalled + abortController.abort() + const err = await promise.catch(err => err) + + expect(err).toMatchObject({ + name: 'AbortError', + code: 'ABORT_ERR', + }) + expect(err.name).not.toBe('TimeoutError') + }) + describe('when the server returns plain text for a JSON endpoint', () => { const contentType = 'text/plain' const chunks = ['foo', '-', 'bar'] @@ -317,6 +410,17 @@ describe('Client', () => { } client.fetch = vi.fn().mockResolvedValue(response) await expect(client.stream()).resolves.toBe(response) + expect(client.fetch).toHaveBeenCalledWith(undefined, { requestTimeout: 0 }) + }) + + it('should allow direct stream callers to override requestTimeout', async () => { + const statusCode = 200 + const response = { + statusCode, + } + client.fetch = vi.fn().mockResolvedValue(response) + await expect(client.stream('events', { requestTimeout: 100 })).resolves.toBe(response) + expect(client.fetch).toHaveBeenCalledWith('events', { requestTimeout: 100 }) }) it('should throw a NotFound error', async () => { @@ -375,4 +479,97 @@ describe('Client', () => { expect(buffer.toString('utf8')).toBe(data) }) }) + + describe('isRequestTimeoutAbort', () => { + it('should return false when timeoutSignal is undefined', () => { + expect(isRequestTimeoutAbort(new Error('boom'), undefined)).toBe(false) + }) + + it('should return false when timeout has not fired (reason is undefined)', () => { + const signal = AbortSignal.timeout(60_000) + expect(signal.reason).toBeUndefined() + expect(isRequestTimeoutAbort(new Error('boom'), signal)).toBe(false) + }) + + it('should return false when reason is not a TimeoutError (e.g. caller AbortError)', () => { + const controller = new AbortController() + controller.abort() + expect(controller.signal.reason).toBeDefined() + expect(controller.signal.reason.name).not.toBe('TimeoutError') + expect(isRequestTimeoutAbort(new Error('boom'), controller.signal)).toBe(false) + }) + + it('should return true when err is the timeout reason itself (direct case)', async () => { + const signal = AbortSignal.timeout(1) + await new Promise(resolve => setTimeout(resolve, 5)) + expect(signal.reason?.name).toBe('TimeoutError') + expect(isRequestTimeoutAbort(signal.reason, signal)).toBe(true) + }) + + it('should return true when err wraps the timeout reason as cause (ABORT_ERR case)', async () => { + const signal = AbortSignal.timeout(1) + await new Promise(resolve => setTimeout(resolve, 5)) + const wrapped = new Error('The operation was aborted') + wrapped.name = 'AbortError' + wrapped.code = 'ABORT_ERR' + wrapped.cause = signal.reason + expect(isRequestTimeoutAbort(wrapped, signal)).toBe(true) + }) + + it('should return false when err has ABORT_ERR code but cause is a different reason', async () => { + const signal = AbortSignal.timeout(1) + await new Promise(resolve => setTimeout(resolve, 5)) + const otherReason = new DOMException('other', 'TimeoutError') + const wrapped = new Error('The operation was aborted') + wrapped.name = 'AbortError' + wrapped.code = 'ABORT_ERR' + wrapped.cause = otherReason + expect(isRequestTimeoutAbort(wrapped, signal)).toBe(false) + }) + + it('should return false for unrelated errors when timeout has fired', async () => { + const signal = AbortSignal.timeout(1) + await new Promise(resolve => setTimeout(resolve, 5)) + const networkErr = Object.assign(new Error('socket reset'), { code: 'ECONNRESET' }) + expect(isRequestTimeoutAbort(networkErr, signal)).toBe(false) + }) + }) + + describe('mapTimeoutAbortError', () => { + const requestOptions = { method: 'GET', url: new URL('https://example.org/foo/bar') } + + it('should rewrite a timeout-triggered error as TimeoutError with descriptive message', async () => { + const signal = AbortSignal.timeout(1) + await new Promise(resolve => setTimeout(resolve, 5)) + const wrapped = Object.assign(new Error('The operation was aborted'), { + name: 'AbortError', + code: 'ABORT_ERR', + cause: signal.reason, + }) + const mapped = mapTimeoutAbortError(wrapped, requestOptions, 1, signal) + expect(mapped).toMatchObject({ + name: 'TimeoutError', + message: 'Request exceeded 1 ms for GET /foo/bar', + }) + expect(mapped.cause).toBe(wrapped) + }) + + it('should pass through caller AbortError unchanged', () => { + const controller = new AbortController() + controller.abort() + const timeoutSignal = AbortSignal.timeout(60_000) + const callerErr = Object.assign(new Error('aborted'), { + name: 'AbortError', + code: 'ABORT_ERR', + cause: controller.signal.reason, + }) + expect(mapTimeoutAbortError(callerErr, requestOptions, 60_000, timeoutSignal)).toBe(callerErr) + }) + + it('should pass through unrelated errors unchanged', () => { + const timeoutSignal = AbortSignal.timeout(60_000) + const err = new Error('boom') + expect(mapTimeoutAbortError(err, requestOptions, 60_000, timeoutSignal)).toBe(err) + }) + }) }) diff --git a/packages/request/lib/Client.js b/packages/request/lib/Client.js index 91149fda30..8d633a30d1 100644 --- a/packages/request/lib/Client.js +++ b/packages/request/lib/Client.js @@ -12,7 +12,7 @@ import zlib from 'zlib' import typeis from 'type-is/index.js' import { pick, omit } from 'lodash-es' import { globalLogger as logger } from '@gardener-dashboard/logger' -import { createHttpError, ParseError } from './errors.js' +import { createHttpError, ParseError, TimeoutError } from './errors.js' import agent from './Agent.js' import { pipeline } from 'stream' @@ -41,6 +41,67 @@ function setHeader (headers, key, value) { } const EOL = '\n' +const MAX_TIMEOUT = 2_147_483_647 // Node.js TIMEOUT_MAX (2^31 - 1) + +function combineSignals (a, b) { + if (!a) { + return b + } + if (!b) { + return a + } + return AbortSignal.any([a, b]) +} + +function createTimeoutSignal (requestTimeout) { + if (requestTimeout === 0) { + return undefined + } + if ( + !Number.isInteger(requestTimeout) || + requestTimeout < 0 || + requestTimeout > MAX_TIMEOUT + ) { + throw new TypeError(`requestTimeout must be a non-negative integer <= ${MAX_TIMEOUT}`) + } + return AbortSignal.timeout(requestTimeout) +} + +/** + * Detects whether an error originated from our requestTimeout firing, + * as opposed to a caller-supplied abort or unrelated failure. + * + * Used by mapTimeoutAbortError to decide whether to rewrite the error + * as a domain-level TimeoutError. Caller aborts stay as AbortError so + * consumers can distinguish "I cancelled" from "server too slow". + */ +function isRequestTimeoutAbort (err, timeoutSignal) { + // AbortSignal.timeout() sets signal.reason to a TimeoutError DOMException + // only after firing. Undefined reason or non-TimeoutError name means our + // timeout never triggered — the error must be from something else. + const reason = timeoutSignal?.reason + if (!reason || reason.name !== 'TimeoutError') { + return false + } + + // Node delivers timeout-triggered aborts in two shapes depending on which + // await throws (stream creation vs getHeaders vs body read): + // 1. The promise rejects with signal.reason directly. + // 2. Node wraps it in an AbortError with code 'ABORT_ERR' and cause=reason. + // Match both forms. + return err === reason || + (err?.code === 'ABORT_ERR' && err.cause === reason) +} + +function mapTimeoutAbortError (err, { method, url } = {}, requestTimeout, timeoutSignal) { + if (isRequestTimeoutAbort(err, timeoutSignal)) { + return new TimeoutError( + `Request exceeded ${requestTimeout} ms for ${method} ${url?.pathname ?? ''}`, + { cause: err }, + ) + } + return err +} class Client { #options @@ -65,10 +126,6 @@ class Client { : new URL(url) } - get responseTimeout () { - return this.#options.responseTimeout || 15 * 1000 - } - get responseType () { return this.#options.responseType } @@ -163,7 +220,16 @@ class Client { ) } - async fetch (path, { method, searchParams, headers, body, responseType = this.responseType, signal, ...options } = {}) { + async fetch (path, { + method, + searchParams, + headers, + body, + responseType = this.responseType, + signal, + requestTimeout = this.#options.requestTimeout ?? 60 * 1000, + ...options + } = {}) { headers = this.getRequestHeaders(path, { method, searchParams, @@ -180,117 +246,129 @@ class Client { } this.executeHooks('beforeRequest', requestOptions) - const stream = await this.#agent.request(headers, { - ...this.#defaultOptions, - ...options, - signal, - }) - if (body) { - stream.write(body) - } - stream.end() + const timeoutSignal = createTimeoutSignal(requestTimeout) + const effectiveSignal = combineSignals(signal, timeoutSignal) + const mapError = err => mapTimeoutAbortError(err, requestOptions, requestTimeout, timeoutSignal) - const { createDecompressor, concat, transformFactory } = this.constructor + try { + const stream = await this.#agent.request(headers, { + ...this.#defaultOptions, + ...options, + signal: effectiveSignal, + }) + if (body) { + stream.write(body) + } + stream.end() - headers = await stream.getHeaders() - const decompressor = createDecompressor(getHeader(headers, HTTP2_HEADER_CONTENT_ENCODING)) + const { createDecompressor, concat, transformFactory } = this.constructor - return { - request: { options: requestOptions }, - headers, - get statusCode () { - return getHeader(this.headers, HTTP2_HEADER_STATUS) - }, - get ok () { - return this.statusCode >= 200 && this.statusCode < 300 - }, - get redirected () { - return this.statusCode >= 300 && this.statusCode < 400 - }, - get contentType () { - return getHeader(this.headers, HTTP2_HEADER_CONTENT_TYPE) - }, - get contentLength () { - return getHeader(this.headers, HTTP2_HEADER_CONTENT_LENGTH) - }, - get type () { - if (['json', 'text'].includes(responseType)) { - return responseType - } - return typeis.is(this.contentType, ['json', 'text']) - }, - destroy (error) { - stream.destroy(error) - }, - body () { - const streams = [ - stream, - async source => { - const text = await concat(source) - switch (this.type) { - case 'text': - return text - case 'json': - try { - return JSON.parse(text) - } catch (err) { - logger.error('Failed to parse response body: %s', text) - if (this.ok) { - throw new ParseError(err.message, { - headers, - rawBody: text, - }) - } - // return the raw body text if the response status is not ok (keep the original http error in this case) + headers = await stream.getHeaders() + const decompressor = createDecompressor(getHeader(headers, HTTP2_HEADER_CONTENT_ENCODING)) + + return { + request: { options: requestOptions }, + headers, + get statusCode () { + return getHeader(this.headers, HTTP2_HEADER_STATUS) + }, + get ok () { + return this.statusCode >= 200 && this.statusCode < 300 + }, + get redirected () { + return this.statusCode >= 300 && this.statusCode < 400 + }, + get contentType () { + return getHeader(this.headers, HTTP2_HEADER_CONTENT_TYPE) + }, + get contentLength () { + return getHeader(this.headers, HTTP2_HEADER_CONTENT_LENGTH) + }, + get type () { + if (['json', 'text'].includes(responseType)) { + return responseType + } + return typeis.is(this.contentType, ['json', 'text']) + }, + destroy (error) { + stream.destroy(error) + }, + body () { + const streams = [ + stream, + async source => { + const text = await concat(source) + switch (this.type) { + case 'text': return text - } - default: - return Buffer.from(text, 'utf8') - } - }, - ] - if (decompressor) { - streams.splice(1, 0, decompressor) - } - return new Promise((resolve, reject) => { - pipeline(streams, (err, body) => { - if (err) { - reject(err) - } else { - resolve(body) - } + case 'json': + try { + return JSON.parse(text) + } catch (err) { + logger.error('Failed to parse response body: %s', text) + if (this.ok) { + throw new ParseError(err.message, { + headers, + rawBody: text, + }) + } + // return the raw body text if the response status is not ok (keep the original http error in this case) + return text + } + default: + return Buffer.from(text, 'utf8') + } + }, + ] + if (decompressor) { + streams.splice(1, 0, decompressor) + } + return new Promise((resolve, reject) => { + pipeline(streams, (err, body) => { + if (err) { + reject(mapError(err)) + } else { + resolve(body) + } + }) }) - }) - }, - async * [Symbol.asyncIterator] () { - let data = '' - const transform = transformFactory(this.type) - let readable = stream - if (decompressor) { - readable = pipeline(stream, decompressor, err => { - if (err) { - logger.debug('Stream decompress pipeline error: %s', err.message) + }, + async * [Symbol.asyncIterator] () { + let data = '' + const transform = transformFactory(this.type) + let readable = stream + if (decompressor) { + readable = pipeline(stream, decompressor, err => { + if (err) { + logger.debug('Stream decompress pipeline error: %s', err.message) + } + }) + } + readable.setEncoding('utf8') + try { + for await (const chunk of readable) { + data += chunk + let index + while ((index = data.indexOf(EOL)) !== -1) { + yield transform(data.slice(0, index)) + data = data.slice(index + 1) + } } - }) - } - readable.setEncoding('utf8') - for await (const chunk of readable) { - data += chunk - let index - while ((index = data.indexOf(EOL)) !== -1) { - yield transform(data.slice(0, index)) - data = data.slice(index + 1) + if (data && data.length) { + yield transform(data) + } + } catch (err) { + throw mapError(err) } - } - if (data && data.length) { - yield transform(data) - } - }, + }, + } + } catch (err) { + throw mapError(err) } } async stream (path, options) { - const response = await this.fetch(path, options) + const response = await this.fetch(path, { requestTimeout: 0, ...options }) const statusCode = response.statusCode if (statusCode >= 400) { throw createHttpError({ @@ -392,3 +470,4 @@ class Client { } export default Client +export { isRequestTimeoutAbort, mapTimeoutAbortError } diff --git a/packages/request/lib/errors.js b/packages/request/lib/errors.js index 3391a40bcc..8731280a03 100644 --- a/packages/request/lib/errors.js +++ b/packages/request/lib/errors.js @@ -9,8 +9,8 @@ import createError from 'http-errors' import { get } from 'lodash-es' class TimeoutError extends Error { - constructor (message) { - super(message) + constructor (message, options) { + super(message, options) this.name = this.constructor.name this.code = 'ETIMEDOUT' Error.captureStackTrace(this, this.constructor)