javascript icon indicating copy to clipboard operation
javascript copied to clipboard

not possible to set a client timeout

Open cdbkr opened this issue 1 year ago • 9 comments

Describe the bug At the moment, it seems not possible to set a client timeout since node-fetch requires passing an AbortController instance to the fetch request option's signal.

** Client Version ** 1.0.0-rc3

To Reproduce

Expected behavior It would be great to set a client's timeout when creating a k8sApi instance or invoking a request

cdbkr avatar Mar 12 '24 08:03 cdbkr

/assign

elizabeth-dev avatar Apr 22 '24 21:04 elizabeth-dev

hmm, I'm having a bit of trouble with this issue, not because of the implementation, but because how the library's API is structured

right now, the configuration object (where undoubtedly we would need to store the config value for the timeout) is built in this function, but we'd need to add a new parameter to makeApiClient to store that additional config parameter (and right now nothing more, although I'm considering the possibility of also adding a custom http.Agent option in a different PR), and although it wouldn't be breaking, it feels like a big enough change to try and get the opinion of some more experienced contributors.

tl;dr: in order to implement this feature, we'd need to change the signature of the makeApiClient method to accept an additional parameter, a config object. would this be acceptable?

elizabeth-dev avatar Apr 22 '24 21:04 elizabeth-dev

I would say yes but it should definitely be an optional argument and probably an object so that it's easier to extend and adapt in the future.

@brendanburns any concerns regarding this?

mstruebing avatar Apr 22 '24 21:04 mstruebing

I think it is totally fine to add an optional typed parameter to the makeApiClient signature. Thanks for checking!

brendandburns avatar Apr 22 '24 21:04 brendandburns

After starting to work on this and looking a bit more into it, I've come to the conclusion that this can not be implemented right now 😕. All the code that handles the HTTP requests is generated with the library openapi-generator-maven-plugin, and the client is completely coupled to that. I thought there would be some way to customize the template that generates the request (at least add some extra parameters for the timeout), but it doesn't look like it.

So 90% of the functionality for this feature needs to be done on the OpenAPI generator (and I'm assuming it'd need to be done across all the languages they support).

elizabeth-dev avatar Apr 24 '24 07:04 elizabeth-dev

@elizabeth-dev I did not look to deep into it, but I think this needs to be implemented in: https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/typescript

(or typescript-fetch/typescript-node, where you would be able to only change the code for a specific case and do not need to implement it for all. I may be wrong here though.

mstruebing avatar Apr 24 '24 13:04 mstruebing

I think that this is the place where you would want to add the timeout code:

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-fetch/runtime.mustache#L81

In general that project is pretty good about merging PRs for specific runtimes.

As I splunked through the code though, I realized you can also make the change here:

https://github.com/kubernetes-client/javascript/blob/release-1.x/src/config.ts#L470

And I'm pretty sure that it will work. That's the Configuration object that flows down through to fetch ultimately (if I tracked the code right)

brendandburns avatar Apr 24 '24 16:04 brendandburns

Has there been any progress on this issue? I'm encountering the following error: Error: Client network socket disconnected before secure TLS connection was established when attempting to fetch deployments from within the cluster. I suspect this might be related to an insufficient timeout setting. Could this be the case?

ylnsnv avatar Jun 25 '24 09:06 ylnsnv

Is this still being worked on? A way to set a timeout like in the version 0.22.3 with the addInterceptor function would be great. Also open to alternative options, any help is appreciated.

cesarandr avatar Mar 21 '25 08:03 cesarandr