OliverMKing commented 2 years ago

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

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

Kubernetes-client repository

[x] Increment OPENAPI_GENERATOR_COMMIT to be version 5.3.0
[x] npm install node-fetch to install node-fetch
[x] Switch generate-client script to use typescript-fetch
[x] 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;
}

[x] Generate api with npm run generate
[x] Match src/gen/api.ts to new generated layout (it changes slightly)
[x] 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

joshperry commented 9 months ago

Looks like this issue was already fixed and merged upstream, just waiting for a new release: https://github.com/OpenAPITools/openapi-generator/pull/16386 local issue: https://github.com/kubernetes-client/javascript/issues/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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

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

brendandburns commented 7 months ago

@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 commented 7 months ago

Thank you so much @brendandburns !

RSAlderman commented 5 months ago

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_3Pkubernetes PVR0429050

ArvinB commented 5 months ago

@brendandburns any update on this?

brendandburns commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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

mdouglass commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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
  }
}

kubernetes-client / javascript