Skip to content

Add optional timeoutMs parameter to makeApiClient #2430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+39 −1
Open

Add optional timeoutMs parameter to makeApiClient #2430

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,18 @@ import {
AuthMethodsConfiguration,
Configuration,
createConfiguration,
Observable,
SecurityAuthentication,
ServerConfiguration,
} from './gen/index.js';
import { ResponseContext } from './gen/http/http.js';
import { OpenIDConnectAuth } from './oidc_auth.js';
import WebSocket from 'isomorphic-ws';
import child_process from 'node:child_process';
import { SocksProxyAgent } from 'socks-proxy-agent';
import { HttpProxyAgent, HttpProxyAgentOptions, HttpsProxyAgent, HttpsProxyAgentOptions } from 'hpagent';
import fetch from 'node-fetch';
import { from } from './gen/rxjsStub.js';

const SERVICEACCOUNT_ROOT: string = '/var/run/secrets/kubernetes.io/serviceaccount';
const SERVICEACCOUNT_CA_PATH: string = SERVICEACCOUNT_ROOT + '/ca.crt';
Expand Down Expand Up @@ -473,7 +477,12 @@ export class KubeConfig implements SecurityAuthentication {
);
}

public makeApiClient<T extends ApiType>(apiClientType: ApiConstructor<T>): T {
public makeApiClient<T extends ApiType>(
apiClientType: ApiConstructor<T>,
options?: {
timeoutMs?: number;
},
): T {
const cluster = this.getCurrentCluster();
if (!cluster) {
throw new Error('No active cluster!');
Expand All @@ -485,6 +494,35 @@ export class KubeConfig implements SecurityAuthentication {
const config: Configuration = createConfiguration({
baseServer: baseServerConfig,
authMethods: authConfig,
// This HTTP API implementation is a copy of ./gen/http/isomorphic-fetch.ts,
// extended with the timeout pass-through code.
httpApi: {
send(request: RequestContext): Observable<ResponseContext> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love pulling this full implementation into this library. Is there an easier way to inject the timeout?

Copy link
Author

@jannikschaper jannikschaper May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that too, it would be much nicer to inject the extra field without having to rip out the entire function. But I believe there's no other way. The auto-generated code passes the configuration object (without any timeout value, not even passed through) directly into fetch. Not even a dependency-injected fetch or anything that might be overwriteable, but a fetch directly imported from node-fetch.

Expand code screenshot

Original code screenshot

Maybe it's still possible to reach into the code via JavaScript black magic to overwrite the fetch implementation and amend the timeout parameter, but I have a hunch such code will be even uglier than copying the 22 lines of code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about changing the code generator itself? They're usually pretty good about merging changes quickly into the generator.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid that seeming rabbit hole. But maybe it is a better solution, I will look into it

const method = request.getHttpMethod().toString();
const body = request.getBody();

const resultPromise = fetch(request.getUrl(), {
method: method,
body: body as any,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: body as any,
body: body as BodyInit,

If we do this typecast we should at least cast into the correct type.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do that. Would lose the pure symmetry with the original code then. If you're aware that I deliberately copied the send implementation verbatim, trying to stay close to the original code, but still prefer to have the more correct type cast, I can do that!

headers: request.getHeaders(),
agent: request.getAgent(),
timeout: options?.timeoutMs,
}).then((resp: any) => {
const headers: { [name: string]: string } = {};
resp.headers.forEach((value: string, name: string) => {
headers[name] = value;
});

const body = {
text: () => resp.text(),
binary: () => resp.buffer(),
};
return new ResponseContext(resp.status, headers, body);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure but do we need a catch here as well or will this be catched by the client code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code, as is (without a catch) should behave identically to the status quo. I copied the send implementation verbatim from ./gen/http/isomorphic-fetch.ts (as noted above); only adding the timeout: options?.timeoutMs, line. Since the original code didn't need a catch, I argue this code must not need it either


return from<Promise<ResponseContext>>(resultPromise);
},
},
});

const apiClient = new apiClientType(config);
Expand Down