Proposal to swap deprecated request for node-fetch

[OliverMKing]

Proposal to swap deprecated request for node-fetch

Request is fully deprecated requiring us to switch libraries (see #414 for more information).

@brendanburns outlined some of the clear options forward. This change should ideally be a long-term solution leaving us with the option of either modifying the node-typescript open-api-generator to use a different request library or migrate to one of the other generator options.

Modifying an outdated node-typescript library does not seem like the right path forward. It would require us to still use a new library which would likely change the api and still be a breaking change. We might as well migrate to a newer version of openapi-generator while we do that and acquire the advantages of a more up-to-date version.

Choosing a new library

@d-cs posted a nice summary of the different choices. We narrowed it down to Axios or Fetch. The other libraries didn't match how widely adopted and focused these libraries are.

Fetch is simply the standard JavaScript as a whole is moving to, which brings more longterm support aligning us better with our goal of migrating to a long-term solution. node-fetch has significantly more weekly downloads than axios. The package size of node-fetch is also noticeably smaller than axios.

Additionally, future improvements could likely be done to allow us to also support the browser #165 since Fetch is native to the browser.

Node-fetch

Fetch is a native browser API that is not implemented in Node.js by default. Node-fetch is the natural choice for the Node implementation because it is the most widely adopted.

The openapi-generator uses the browser implementation of Fetch so we must substitute node-fetch in manually.

Implementation steps

@davidgamero and I will work to implement the following changes.

Other repositories

• Update kubernetes-client/gen's typescript-fetch files to let us pass in the typescriptThreePlus config option ^{1 2}
• Update openapi-generator's typescript-fetch flavor to mark parameters as optional if all parameters are optional ³

Kubernetes-client repository

• Increment OPENAPI_GENERATOR_COMMIT to be version 5.3.0
• npm install node-fetch to install node-fetch
• Switch generate-client script to use typescript-fetch
• Import and inject node-fetch in src/api.ts with the following

import fetch from 'node-fetch';

// inject node-fetch
if (!globalThis.fetch) {
    // @ts-ignore
    globalThis.fetch = fetch;
    globalThis.Headers = Headers;
    globalThis.Request = Request;
    globalThis.Response = Response;
}

• Generate api with npm run generate
• Match src/gen/api.ts to new generated layout (it changes slightly)
• Fix errors in /src folder (due to new api)
• Fix errors in test (due to new api)
• Fix examples (due to new api)
• Document breaking changes for users
• Release new version

[brendandburns]

This approach is going to introduce breaking changes for consumers of this client library (there were 151k downloads last week alone).

We need to have a more careful plan for how we roll out this change.

I would like to support existing users for at least a few versions (perhaps 3?) while we make this transition.

Perhaps the right way to do this is to introduce a new major version to signify the new generator?

e.g.

0.17.x == old generator, Kubernetes 1.23 API
0.18.x == old generator, Kubernetes 1.24 API
0.19.x == old generator, Kubernetes 1.25 API
Support for old generator stops after 1.25

1.0.x == new generator, Kubernetes 1.23 API
1.1.x == new generator, Kubernetes 1.24 API
1.2.x == new generator, Kubernetes 1.25 API
Support for subsequent kubernetes versions continues with the new generator.

This will require introducing a new branch for the 1.x series of releases, that's probably going to be necessary anyway since we don't want to make this change in a single PR.

Please note that this is going to be a big change that will touch nearly the entire library, I think

[OliverMKing]

That phasing plan makes sense to me. It's in-line with the npm semantic versioning.

It might make sense to deprecate the 0.x releases with a warning telling them that support for the old client api will no longer be supported after a certain version and encouraging them to upgrade. That should alert existing users of the changes.

I'm prepared to take on the responsibility associated with this upgrade.

[dominykas]

Is using both generators and fetch/request in parallel not an option during the transition? I haven't thought through all implications or the workload of doing that, just thinking out loud, but it would be nice if you could do something like makeApiClient(CoreV1Api, { fetch: true }) for the next 3 releases or smth, and only do the breaking change then?

[davidgamero]

I agree it would be nice if we could keep the user experience that cohesive, but unfortunately these changes seem too broad and breaking for that to be doable. Given the extent of the proposed changes, it could introduce significant overhead and complexity to have two major versions bundled in a single release.

I've taken a shot at incrementing OPENAPI_GENERATOR_COMMIT as proposed, and it appears that several response types have changed, an example is the removals of the .body properties ex:

(await api.listNamespacedPod(namespace)).body -> (await api.listNamespacedPod({ namespace }))

Another predictable area of major changes is the error interfaces since we are changing the network library, and the proposal includes incrementing typescript by a major version.

This may be a config option that I've inadvertently changed, but IMO it does come across cleaner.

This could be a concern if we had { fetch: true } as an option, since the option wouldn't just change functionality under the hood, but also several parts of the api interfaces.

I'll keep checking it out. There could be an easier way tweaking the generator options that I'm not familiar with.

[brendandburns]

One other option would be to package the old generated code and the new generated code into different directories and effectively have:

import api_old from @kubernetes/node-client

or

import api_new from @kubernetes/node-client

e.g. keep everything in one place from a release perspective, but introduce a fork in the imports that you can use.

[davidgamero]

That could definitely work if we introduce a named export for api_old. I'm not sure how we could do that without potentially breaking existing default import compatibility unless we nest the api_new inside it.

Another possibility would be to have

import api_old from @kubernetes/node-client

or

import api_new from @kubernetes/node-client/fetch

to retain existing import compatibility for import k8s from @kubernetes/node-client

1. Current Top Level Exports

// ✅  `import { CoreV1Api } from @kubernetes/node-client`
// ✅ `import k8s from @kubernetes/node-client`
@kubernetes/node-client
├── CoreV1Api
...
└── KubeConfig

2. Named Level Exports

// ✅ `import { api_new } from @kubernetes/node-client`
// ✅ `import { api_old } from @kubernetes/node-client`
// ❌ `import k8s from @kubernetes/node-client` <- no longer works
@kubernetes/node-client
├── api_old
│    ├─ CoreV1Api
│    ...
│    └─ KubeConfig
│
└── api_new
     ├─ CoreV1Api
     ...
     └─ KubeConfig

3. Messy Nested Export

// ✅ `import { api_new } from @kubernetes/node-client`
// ✅ `import { api_old } from @kubernetes/node-client`
// ✅ `import k8s from @kubernetes/node-client` <- works still, but gross way to do it
@kubernetes/node-client
├── CoreV1Api
...
├── KubeConfig
└── api_new
     ├─ CoreV1Api
     ...
     └─ KubeConfig

[brendandburns]

I think there are different kinds of breaking changes. I feel like changing an import statement is a much lower cost breaking change vs changing the entire API surface area. So I would be ok to require

import { api_new as api } from @kubernetes/node-client

or

import { api_old as api } from @kubernetes/node-client

And I think that is feasible...

[OliverMKing]

I don't think putting both the new api and old api in the same package / directory makes sense to me.

The fact that it's a breaking change between them means that someone can't just swap out API's when making the change.

// switching from this
import { api_new as api } from @kubernetes/node-client
// to this would still require code changes in lots of places where the api is used
import { api_old as api } from @kubernetes/node-client

Someone migrating to the new api would have to change every single node-client import statement in their repo which would be time consuming and tedious. It's simpler from a user-standpoint to simply upgrade their version then fix any errors in the code.

By having both the old and new api in the same package we also wouldn't be able to take advantage of a npm deprecation warning which would help warn users of the upcoming drop of support for the old api.

Users wouldn't be upgraded to the breaking api without doing it manually (or changing their package.json to allow for major release updates) due to npm semantic versioning.

From a user-standpoint I don't think it's easier to change the import and I believe one of our goals should be gently pushing consumers to migrate when they get the chance (or at least be aware of upcoming drops in support). From our standpoint it's more difficult to work with a directory layout that has two apis rather than just different branches. Storing the new and old api in the in the same package would make more sense to me if there was some reason why a user would want to use both the old and new api but I can't think of one.

[davidgamero]

The major version suggestion seems be the cleanest way.

Putting the changes in a new major version would make upgrading a clear decision without interfering with existing code.

That way we don't mix code from two generators in a single package.

[ivanstanev]

Completely supporting the major version upgrade. Our team doesn't mind spending some time adjusting code to the new API, as long as we can stop using deprecated or regularly vulnerable packages! 😄

[brendandburns]

Ok, sounds like the consensus is for a major version upgrade. I have created a release-0.x branch, which we will use for the releases of the old generated client for the next ~9 months.

We can start sending PRs to the master branch now to start making these changes.

[brendandburns]

We should also probably turn this discussion into a markdown document that describes our approach and links off of the main README.md

[OliverMKing]

Quick update: @davidgamero and I have been working on this for the last two weeks. We're having to make some changes to open-api generator to support the way we need to do auth. @davidgamero has been working on identifying the exact improvements necessary.

[davidgamero]

Update 2: Two PRs have been completed against OpenAPITools/openapi-generator (the typescript generator template in specific), and a third is in progress.

Complete (PR)

• [typescript] Default auth method support and optional param object when all params optional
• useSingleRequestParameter should mark parameter optional if all properties are optional

In Progress (Issue) [REQ][typescript] Enable passing custom SecurityAuthentication to API

With this last PR, the ability to pass certificates to the fetch client should be exposed in the typescript generator, clearing the way for full auth migration

[davidgamero]

Update 3: Custom Authentication is now complete in the typescript generator. Currently migrating the request implementations outside the generated code (in /src)

OpenAPITools/openapi-generator#11400

[loynoir]

fetch

• node >=17.5.0 has --experimental-fetch, which is using undici
• node >= 16, has undici support
• node < 16, maybe fallback to node-fetch

https://nodejs.org/en/blog/release/v17.5.0/

Adds experimental support to the fetch API. This adds a --experimental-fetch flag that installs the fetch, Request, Response and Headers globals.

node 17's fetch is using undici

nodejs/node@76a229c4ff

[OliverMKing]

fetch

• node >=17.5.0 has --experimental-fetch, which is using undici
• node >= 16, has undici support
• node < 16, maybe fallback to node-fetch

https://nodejs.org/en/blog/release/v17.5.0/

Adds experimental support to the fetch API. This adds a --experimental-fetch flag that installs the fetch, Request, Response and Headers globals.

node 17's fetch is using undici

nodejs/node@76a229c4ff

Hopefully the TypeScript generator swaps over to this way.

[haoxins]

https://nodejs.org/en/blog/announcements/v18-release-announce/#fetch-experimental
Node 18 is here~

[davidgamero]

This will be great! It should be simpler to switch from node-fetch to the included fetch in the future (since their syntax is very similar)

[chadbr]

Hello,

If I understand correctly - Here:

javascript/FETCH_MIGRATION.md

Line 62 in b4ccd6e

- [ ] Fix errors in /src folder (due to new generated api)

The latest status is:
Fix errors in /src folder (due to new generated api)

Is this something I can help with? Is there a branch with source I can help to get compiling, I'm happy to try to help.

Thanks, Chad

[davidgamero]

Hi @chadbr , the current working branch is https://github.com/kubernetes-client/javascript/tree/release-1.x and the next step is migrating all the auth providers and their unit tests. It's mostly mapping headers and query params to use the new RequestContext ex PR: https://github.com/kubernetes-client/javascript/pull/790/files

Any help migrating those would be very appreciated! I'm finishing up the log.ts migration atm as it's one of the last places we still directly call the request api outside the generated code. As for auth providers, I only did the azure_auth so far, so the rest are all in need of a migration PR

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[joshperry]

Looks like this issue was already fixed and merged upstream, just waiting for a new release: OpenAPITools/openapi-generator#16386
local issue: #893

I guess the only other issue (for patch*) is the ergonomics of applying a custom header. With the upstream fix, at least patch routes and the middleware option will work, but replacing the whole request config (instead of merging, or separating params) when calling a client's method doesn't feel right, nor does reaching deep into the client's state to do the merge yourself.

Sorry @davidgamero, I totally missed your link.

[davidgamero]

@joshperry do you mean waiting for a release from openapi-generator or this project?
if you mean openapi we can just bump the hash we use- we don't build using their releases

[davidgamero]

i completely agree that the ergonomics of the custom header are cumbersome for this use case, and providing a way to merge new options into the config would make this much easier

[joshperry]

I was able to verify that regenerating this project with the latest tag of the openapi-generator project (v7.0.1) solves this issue (in conjunction with the middleware workaround). If you want to try, I published a version with js built to my fork, just npm i joshperry/k8s-node-client#dist-1.x.

[davidgamero]

the middleware workaround seems reasonable while we develop a cleaner long-term solution, so im in favor of a new rc with that code included. just made a pr for the changes @joshperry

[kberkos-public]

Bumping up the priority on this item, since the deprecated Request Package now has a security vulnerability:
https://nvd.nist.gov/vuln/detail/CVE-2023-28155

[efenner-cambia]

Any update on this? Very interested in closing out the CVE referenced above

[brendandburns]

@kberkos-public @efenner-cambia there is a pre-release of the library based on fetch that you can start consuming:
https://www.npmjs.com/package/@kubernetes/client-node/v/1.0.0-rc3

Note that the API has changed somewhat, so it is not a drop-in replacement.

[kberkos-public]

Thank you so much @brendandburns !

[RSAlderman]

I can see there's a later pre-release library https://www.npmjs.com/package/@kubernetes/client-node/v/1.0.0-rc4?activeTab=versions made available on 2023-12-23. @brendandburns do you know when v1.0.0 is going to be released and in which kubernetes version it will be included, to remediate CVE-2023-28155?

PSIRTRef_fup_3P_kubernetes PVR0429050

[ArvinB]

@brendandburns any update on this?

[brendandburns]

This client is not released as part of the Kubernetes release. There is no current eta for a non RC 1.0.0 release, we need more mileage and reports from people that they have migrated successfully to have confidence in the release.

[RSAlderman]

@brendandburns can you confirm that when kuberenetes/client-node/v/1.0.0 is available as well as remediating CVE-2023-28155, it will also remediate CVE-2023-26136; where it seems the required upgrade to tough-cookie-4.1.3 is dependent on the replacement of the request package?

PSIRTRef_fup_3P_kubernetes PVR0452658

[briandealwis]

I forgot to report that I migrated a private repository to rc2 with no issues. I didn’t realize that there was an rc3; I’ll add that to my todo list.

[mstruebing]

@briandealwis There is even an rc4 by now: https://www.npmjs.com/package/@kubernetes/client-node/v/1.0.0-rc4

[mdouglass]

Migrated 14 microservices to rc4 with no issues other than one TS type that needed to change (we had cast the watch result any in 0.20 to a request type and it needed to be AbortController now -- which makes more sense anyway)

[RSAlderman]

The latest pre-release library https://www.npmjs.com/package/@kubernetes/client-node/v/1.0.0-rc4?activeTab=versions remains rc4 made available on 2023-12-23, which appears to be receiving positive feedback for migrating microservices.

Is there any further thought when v1.0.0 maybe released and in which kubernetes version it will be included? I believe this is blocking the remediation of CVE-2023-28155 as well as CVE-2023-26136, where it seems the required upgrade to tough-cookie-4.1.3 is dependent on the replacement of the request package.

PSIRTRef_fup_3P_kubernetes PVR0429050 PVR0452658 PVR0473928 PVR0473931

[pauliusg]

I wouldn't say that rc4 feedback is positive. Yes, code works properly but it is a pain to use middleware. For example when you need to add a header when patching pod:

import * as k8s from '@kubernetes/client-node';

...

const kc = new k8s.KubeConfig();
kc.loadFromDefault();

const headerPatchMiddleware = new PromiseMiddlewareWrapper({
  pre: async (requestContext) => {
    requestContext.setHeaderParam('Content-type', 'application/json-patch+json');
    return requestContext;
  },
  post: async (responseContext) => responseContext,
});

const currentCluster = kc.getCurrentCluster();
if (!currentCluster) {
  throw new Error('Kube config does not have current cluster.');
}

const server = currentCluster.server;
if (!server) {
  throw new Error('Kube config cluster does not have server.');
}

const baseServerConfig: k8s.ServerConfiguration<Record<string, never>> =
  new k8s.ServerConfiguration<Record<string, never>>(server, {});
const k8sPatchPodConfiguration = k8s.createConfiguration({
  middleware: [headerPatchMiddleware],
  baseServer: baseServerConfig,
  authMethods: { default: kc },
});

const k8sApi = kc.makeApiClient(k8s.CoreV1Api);

...

const patch = [
  {
    op: 'replace',
    path: '/metadata/annotations',
    value: annotationsObject,
  },
];

await k8sApi.patchNamespacedPod(
  {
    name: podName,
    namespace: namespace,
    body: patch,
  },
  k8sPatchPodConfiguration,
);

I think you agree that we need quite much code to patch a pod, it should be simpler...

[iainsproat]

I'm not sure where the best place to return feedback on RC4 is, but I'll assume from the thread maybe here.

The errors being returned appear to have a body of a string or undefined, and not an object as they were in 0.2.0:

const config = new KubeConfig()
config.loadFromDefault()
try {
  config.makeApiClient(CoreV1Api).createNamespace({ body: { metadata: { name: 'k8s will 💩 itself' } } })
} catch (error) {
  if (!(error && typeof error === 'object' && error instanceof ApiException && 'body' in error)) {
    throw error
  }
  expect(error.body).toHaveProperty('reason') // fails in 1.0.0 but not 0.2.0
  //instead need to parse the body
  if (typeof error.body === 'string') { // it's also possible to get `undefined` error body
    const errorBody: unknown = JSON.parse(error.body)
    expect(error.body).toHaveProperty('reason') //passes
  }
}

[eplightning]

Also not sure where the 1.x feedback should go, but on top of the few already reported issues above here are few pain points from my migration to 1.x:

• Missing application/apply-patch+yaml from PatchStrategy enum. Working around this requires use of middleware
• PromiseMiddlewareWrapper not being exported, requiring user to deal with Promise -> Observable conversion (I might be completely wrong here tho)
• ObjectSerializer no longer being exported, which my custom cache/watch implementation was relying on

[sergi]

Just pointing out that fetch is implemented natively in Node.js, and it's been available since Node 18 (so it includes current and previous LTS, so strictly there's no need to use node-fetch. Of course, if the project wants to support earlier versions of Node.js, then node-fetch makes sense.

[lachieh]

Node 16 went EOL in September of 2023, so it's doesn't really feel valuable supporting it—especially since the standard k8s support window is only 14 months. Is there interest from the team in going straight to the node implementation of fetch or is the plan to stick with node-fetch?

Edit: It's worth noting that while native fetch was pulled out from behind the --experimental-fetch flag in v18, it wasn't marked as stable until v21 (v22 LTS). See Node API globals.

[mstruebing]

I would think we should start with finishing the node-fetch implementation.
Later on we can change that to the native fetch implementation. That shift should be significant easier than the move from request to node-fetch