Expose TCP connect() and recv() timeouts for HTTP client

[jgehrcke]

Relates to #96.

Even if for example a TCP connect() timeout error is supposed to be "left for the user to handle" we need a way to configure the relevant timeout constant for the underlying HTTP client. Same for recv() timeout (or "request timeout").

[brendandburns]

You can use any of the options available to the request module and pass them in as the options parameter when you make the API calls, the available options are all described here:

https://github.com/request/request#requestoptions-callback

Including the two timeout values that you are looking for.

[jgehrcke]

Thanks, Brendan! Based on the docs that you linked to this appears to be a single parameter, controlling both, connect() and recv() timeout:

timeout - integer containing number of milliseconds, controls two timeouts.

We usually want to be able to set those independently -- in most cases we want to set a short connect() timeout and a longer read/recv timeout. Well :-).

I'll try playing with that. Thanks again.

For the record: the HTTP client situation in the Javascript ecosystem seems to be rather ugly. For example, the HTTP client underlying this library here (no blame) is deprecated an doesn't provide fine-grained timeout control :-). Then there are other HTTP clients that move so fast that there are breaking changes with every release, and then again other HTTP clients that also don't bother much with timeout settings (mikeal/bent#59). I am currently liking https://github.com/sindresorhus/got for its timeout control, but it has other downsides.

Having seen some of that I appreciate even more than before the value of ecosystems that offer a mature standard library (and where a large fraction of the community has converged to using the same mature and maintained HTTP client library), where you can just move on and be productive. Ha.

[jgehrcke]

pass them in as the options parameter when you make the API calls

Say, I want to pass that when calling patchNamespacedConfigMap().

The current type definition (from master/src/gen/api/coreV1Api.ts) is

public async patchNamespacedConfigMap (name: string, namespace: string, body: object, pretty?: string, dryRun?: string, fieldManager?: string, force?: boolean, options: {headers: {[name: string]: string}} = {headers: {}}) 

with options being

options: {headers: {[name: string]: string}} = {headers: {}}

which of course results in a tsc compile error when I pass in an additional property timeout.

I would truly appreciate to work on a 'working example' with you.

[jgehrcke]

Using this code to try to achieve a connect() timeout of ~10 seconds:

    api.createNamespacedConfigMap(
      this.namespace,
      this.resource,
      undefined,
      undefined,
      undefined,
      {
        // @ts-ignore: timeout does not exist on type
        timeout: 10000
      }
    );

(Note the // @ts-ignore: timeout does not exist on type to deal with the problem described in #544 (comment)).

I was lucky enough to see a connect timeout thereafter:

2020-11-04T13:34:18.047Z info: set  <snip> config map in k8s cluster
2020-11-04T13:36:27.365Z info: retry config map creation/update soon (connect ETIMEDOUT 52.33.<snip>.<snip>:443)

Notice the time difference here, the connect() timeout did not hit in within ~10 seconds, but more like after ~2 minutes -- which is the "problem" I wanted to address by choosing a custom timeout constant.

@brendandburns is there anything obvious I am doing wrong / would you expect the code above to properly forward the timeout: 10000 parameter into the request client's options?

[brendandburns]

Should work given your code snippet. I will double check through the code and see if something is going wrong.

[brendandburns]

And I hear you on the http client, it's discussed in detail here:

#414

Part of the problem is the client generation is tied tightly to a particular HTTP library, and so changing the HTTP library means introducing breaking changes in the client API sigh

[jgehrcke]

I will double check through the code and see if something is going wrong.

Just a quick updated @brendandburns we kept seeing O(1 min) time to pass before the connect timeout hits in with the above's code. Didn't investigate further though.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.