Deno: Configurable HTTP client

[asyncmax]

This is a separate issue following up the conversation in #2301.

Currently, Deno doesn't support self-signed server certificates which are used by almost all Kubernetes deployments, including AWS and Google.

Also, TLS client authentication is a very common setup in Kubernetes.

To support this feature, we need to be able to specify these TLS parameters via Deno API somehow (I am using parameter names of Node TLS module for explanation):

• ca: Certificate authority bundle to verify the server's certificate. Required if the server uses a self-signed certificate.
• cert: Client certificate. Required if the server is configured to use the client certificate authentication.
• key: Client private key. Required if the server is configured to use the client certificate authentication.
• rejectUnauthorized: If set to false, server certificate verification is disabled and ca is not required. Useful for debugging/prototyping.

As a reference, node-fetch accepts a proprietary option agent and we can set these TLS parameters to agent. As you know, HTTP Agent in Node is mainly used for connection pool management, but can be hacked to do a lot more - in my opinion, this is not necessarily a good thing though.

After giving it some thoughts, I believe the simplest way to support this feature with minimal API surface is to add new options to fetch. Adding non-standard options to fetch might be a deal-breaker here, but it makes sense because each server has its own requirements for the TLS connection and it is the fetch that accepts server URL in the first place. All other technical details such as associating TLS parameters with already cached TLS sessions should be managed by the system transparently.

[bartlomieju]

So my initial thought was to expose const httpClient = new Deno.HttpClient() API which would create a new resource (HttpClient). That resource stores reqwest::Client instance and is simply proxying all interesting methods.

struct HttpClient(reqwest::Client) {}

impl Resource for HttpClient {}

impl HttpClient {
  pub fn new(...) {}
  pub fn request(...) {}
  ....
}

class HttpClient {
   constructor(...) {
       this.rid = sendSync(dispatch.OP_CREATE_HTTP_CLIENT, { // whatever config you want });
   }

   async request(...) {
       const response = await sendAsync(dispatch.OP_SEND_REQUEST, { rid, ... });
       ...
   }

   close() {
       close(this.rid);
   }
}

NOTE: Because HttpClient is a resource it must be closed manually by user otherwise it will stay in resource table.

To support this feature, we need to be able to specify these TLS parameters via Deno API somehow (I am using parameter names of Node TLS module for explanation):

• ca: Certificate authority bundle to verify the server's certificate. Required if the server uses a self-signed certificate.
• cert: Client certificate. Required if the server is configured to use the client certificate authentication.
• key: Client private key. Required if the server is configured to use the client certificate authentication.
• rejectUnauthorized: If set to false, server certificate verification is disabled and ca is not required. Useful for debugging/prototyping.

Those needs can be addressed using following APIs on reqwest::ClientBuilder:

• https://docs.rs/reqwest/0.10.0-alpha.2/reqwest/struct.ClientBuilder.html#method.add_root_certificate
• https://docs.rs/reqwest/0.10.0-alpha.2/reqwest/struct.ClientBuilder.html#method.identity
• https://docs.rs/reqwest/0.10.0-alpha.2/reqwest/struct.ClientBuilder.html#method.danger_accept_invalid_certs

Additionally storing HttpClient as a resource enables the client to keep its own connection pool to reuse connections if the user wants so.

There's a plethora of configuration that can potentially be exposed in Deno. Thanks to our abstractions it shall be straight-forward, but at the same time it would need good test coverage for each option.

Anyway, implementing requested changes is IMO low hanging fruit and a "good first issue". I can provide some guidance to anyone wanting to implement it by describing necessary steps.

[rudoi]

:wave: hi @bartlomieju, I'm interested in taking this on as good-first-issue if you're open to providing some guidance! Familiarizing myself with the codebase as best I can in the meantime. :pizza:

[brandonkal]

I've essentially built a @kubernetes/client-deno. So I am very keen on seeing these options made available on the fetch API so I can use this without depending on kubectl proxy.

@rudoi I haven't explored the rust side of deno much but the client is created here https://github.com/denoland/deno/blob/cde4dbb35132848ffece59ef9cfaccff32347124/cli/ops/fetch.rs#L36-L37.

That would need to accept the additional options.

[rudoi]

@brandonkal fascinating! a Deno K8S client was exactly what I was thinking about getting into when I found this issue.

I think @bartlomieju was suggesting it may be a better option for the long term to have a reusable http client in the form of Deno.HttpClient. This would provide a bunch of configuration options that could be expanded over time, and perhaps it would allow for more than just fetch/get.

I've started working on implementing it in this fashion, though it will take some time as I'm very new to the repo and somewhat new to rust.

In y'all's view, is there a benefit to implementing these configuration options specifically within the existing fetch API?

[brandonkal]

I'm all for exposing reqwest::Client on the JavaScript side and that may be the way to go. Though I ultimately prefer fetch for all calls because it is standard and familiar. The credentials field would be a natural place to accept these options. i.e. chrome supports https://developer.mozilla.org/en-US/docs/Web/API/PasswordCredential and I expect this will be expanded to other options in the future. So it is reasonable to accept a DenoCredential there.

One of the problems with @kubernetes/client-node is that it is very dependent on the request library and isn't browser compatible. So I am a little concerned something similar will happen in Deno though that's likely not avoidable.

[ry]

After giving it some thoughts, I believe the simplest way to support this feature with minimal API surface is to add new options to fetch. Adding non-standard options to fetch might be a deal-breaker here, but it makes sense because each server has its own requirements for the TLS connection and it is the fetch that accepts server URL in the first place. All other technical details such as associating TLS parameters with already cached TLS sessions should be managed by the system transparently.

I would very much like to avoid adding a new API for making HTTP requests just to support this relatively niche (yet important!) use-case.

Adding a non-standard option like node_fetch does with agent would break our browser compatibility promise. Maybe it's worth it in this case. Or maybe we could have something like:

let fetch = Deno.createNonStandardFetch({ ca, key, cert });
await fetch(url);

[lucacasonato]

I think we can prevent breaking the browser compat promise by making the option that you pass to fetch reliant on the Deno namespace. So something along the lines of:

const agent = new Deno.HTTPAgent({ ca, key, cert });
await fetch(url, { agent });

This means you could only use the non standard option if you also use the Deno namespace, which in turn means you are opting out of browser compat.

[rudoi]

I see, I think it makes sense to start out working with fetch alone rather than going for an entire HttpClient structure. I'll start working on something that looks more like what @lucacasonato / @ry suggested and we can all talk about it in the PR and go from there. :D

[ry]

@lucacasonato Oh - yes that solves the problem.

[kitsonk]

oooh... great idea @lucacasonato 👏