Repository: kubernetes-client/javascript
Pull request #1880 (2024-09-20 00:07:23)

Fix cp promise returns by converting the exec callbacks into promises
Addresses issue https://github.com/kubernetes-client/javascript/issues/1767

Wraps the exec in a promise properly such that awaiting the promise will only return after the file is copied

linux-foundation-easycla[bot]:  <a href="https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/20677673/89748560/1880/#/?version=2"><img src="https://s3.amazonaws.com/cla-project-logo-prod/cla-not-signed.svg" alt="CLA Not Signed" align="left" height="28" width="328"></a><br/><br /><ul><li><a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/20677673/89748560/1880/#/?version=2' target='_blank'>:x:</a> - login: @joeferner / name: Joe Ferner . The commit (329cc209f3e3fd7c41ef1845eeed20ba34aa43db) is not authorized under a signed CLA. <a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/20677673/89748560/1880/#/?version=2' target='_blank'>Please click here to be authorized</a>. For further assistance with EasyCLA, <a href='https://jira.linuxfoundation.org/servicedesk/customer/portal/4' target='_blank'>please submit a support request ticket</a>.</li></ul><!-- Date Modified: 2024-09-20 00:07:28.515316 -->
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-client/javascript/pull/1880#" title="Author self-approved">joeferner</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign [mstruebing](https://github.com/mstruebing) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-client%2Fjavascript).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-client/javascript/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mstruebing"]} -->
k8s-ci-robot: Welcome @joeferner! <br><br>It looks like this is your first PR to <a href='https://github.com/kubernetes-client/javascript'>kubernetes-client/javascript</a> 🎉. Please refer to our [pull request process documentation](https://git.k8s.io/community/contributors/guide/pull-requests.md) to help your PR have a smooth ride to approval. <br><br>You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. [Here is the bot commands documentation](https://go.k8s.io/bot-commands). <br><br>You can also check if kubernetes-client/javascript has [its own contribution guidelines](https://github.com/kubernetes-client/javascript/tree/master/CONTRIBUTING.md). <br><br>You may want to refer to our [testing guide](https://git.k8s.io/community/contributors/devel/sig-testing/testing.md) if you run into trouble with your tests not passing. <br><br>If you are having difficulty getting your pull request seen, please follow the [recommended escalation practices](https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#why-is-my-pull-request-not-getting-reviewed). Also, for tips and tricks in the contribution process you may want to read the [Kubernetes contributor cheat sheet](https://git.k8s.io/community/contributors/guide/contributor-cheatsheet/README.md). We want to make sure your contribution gets all the attention it needs! <br><br>Thank you, and welcome to Kubernetes. :smiley:
brendandburns: Thanks for the PR! Can you add a unit test demonstrating the problem as well so that we can be sure this fix works (and that it stays fixed)

Patch:
diff --git a/package-lock.json b/package-lock.json
index f61dc892..eac10a79 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -2858,9 +2858,9 @@
       }
     },
     "node_modules/nyc": {
-      "version": "17.1.0",
-      "resolved": "https://registry.npmjs.org/nyc/-/nyc-17.1.0.tgz",
-      "integrity": "sha512-U42vQ4czpKa0QdI1hu950XuNhYqgoM+ZF1HT+VuUHL9hPfDPVvNQyltmMqdE9bUHMVa+8yNbc3QKTj8zQhlVxQ==",
+      "version": "17.0.0",
+      "resolved": "https://registry.npmjs.org/nyc/-/nyc-17.0.0.tgz",
+      "integrity": "sha512-ISp44nqNCaPugLLGGfknzQwSwt10SSS5IMoPR7GLoMAyS18Iw5js8U7ga2VF9lYuMZ42gOHr3UddZw4WZltxKg==",
       "dev": true,
       "dependencies": {
         "@istanbuljs/load-nyc-config": "^1.0.0",
@@ -2870,7 +2870,7 @@
         "decamelize": "^1.2.0",
         "find-cache-dir": "^3.2.0",
         "find-up": "^4.1.0",
-        "foreground-child": "^3.3.0",
+        "foreground-child": "^2.0.0",
         "get-package-type": "^0.1.0",
         "glob": "^7.1.6",
         "istanbul-lib-coverage": "^3.0.0",
@@ -2922,34 +2922,6 @@
         "node": ">=8"
       }
     },
-    "node_modules/nyc/node_modules/foreground-child": {
-      "version": "3.3.0",
-      "resolved": "https://registry.npmjs.org/foreground-child/-/foreground-child-3.3.0.tgz",
-      "integrity": "sha512-Ld2g8rrAyMYFXBhEqMz8ZAHBi4J4uS1i/CxGMDnjyFWddMXLVcDp051DZfu+t7+ab7Wv6SMqpWmyFIj5UbfFvg==",
-      "dev": true,
-      "dependencies": {
-        "cross-spawn": "^7.0.0",
-        "signal-exit": "^4.0.1"
-      },
-      "engines": {
-        "node": ">=14"
-      },
-      "funding": {
-        "url": "https://github.com/sponsors/isaacs"
-      }
-    },
-    "node_modules/nyc/node_modules/foreground-child/node_modules/signal-exit": {
-      "version": "4.1.0",
-      "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-4.1.0.tgz",
-      "integrity": "sha512-bzyZ1e88w9O1iNJbKnOlvYTrWPDl46O1bG0D3XInv+9tkPrxrN8jUUTiFlDkkmKWgn1M6CfIA13SuGqOa9Korw==",
-      "dev": true,
-      "engines": {
-        "node": ">=14"
-      },
-      "funding": {
-        "url": "https://github.com/sponsors/isaacs"
-      }
-    },
     "node_modules/nyc/node_modules/locate-path": {
       "version": "5.0.0",
       "resolved": "https://registry.npmjs.org/locate-path/-/locate-path-5.0.0.tgz",
@@ -6599,9 +6571,9 @@
       "dev": true
     },
     "nyc": {
-      "version": "17.1.0",
-      "resolved": "https://registry.npmjs.org/nyc/-/nyc-17.1.0.tgz",
-      "integrity": "sha512-U42vQ4czpKa0QdI1hu950XuNhYqgoM+ZF1HT+VuUHL9hPfDPVvNQyltmMqdE9bUHMVa+8yNbc3QKTj8zQhlVxQ==",
+      "version": "17.0.0",
+      "resolved": "https://registry.npmjs.org/nyc/-/nyc-17.0.0.tgz",
+      "integrity": "sha512-ISp44nqNCaPugLLGGfknzQwSwt10SSS5IMoPR7GLoMAyS18Iw5js8U7ga2VF9lYuMZ42gOHr3UddZw4WZltxKg==",
       "dev": true,
       "requires": {
         "@istanbuljs/load-nyc-config": "^1.0.0",
@@ -6611,7 +6583,7 @@
         "decamelize": "^1.2.0",
         "find-cache-dir": "^3.2.0",
         "find-up": "^4.1.0",
-        "foreground-child": "^3.3.0",
+        "foreground-child": "^2.0.0",
         "get-package-type": "^0.1.0",
         "glob": "^7.1.6",
         "istanbul-lib-coverage": "^3.0.0",
@@ -6654,24 +6626,6 @@
             "path-exists": "^4.0.0"
           }
         },
-        "foreground-child": {
-          "version": "3.3.0",
-          "resolved": "https://registry.npmjs.org/foreground-child/-/foreground-child-3.3.0.tgz",
-          "integrity": "sha512-Ld2g8rrAyMYFXBhEqMz8ZAHBi4J4uS1i/CxGMDnjyFWddMXLVcDp051DZfu+t7+ab7Wv6SMqpWmyFIj5UbfFvg==",
-          "dev": true,
-          "requires": {
-            "cross-spawn": "^7.0.0",
-            "signal-exit": "^4.0.1"
-          },
-          "dependencies": {
-            "signal-exit": {
-              "version": "4.1.0",
-              "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-4.1.0.tgz",
-              "integrity": "sha512-bzyZ1e88w9O1iNJbKnOlvYTrWPDl46O1bG0D3XInv+9tkPrxrN8jUUTiFlDkkmKWgn1M6CfIA13SuGqOa9Korw==",
-              "dev": true
-            }
-          }
-        },
         "locate-path": {
           "version": "5.0.0",
           "resolved": "https://registry.npmjs.org/locate-path/-/locate-path-5.0.0.tgz",
diff --git a/src/cp.ts b/src/cp.ts
index 3cb8117b..b6a4a6bc 100644
--- a/src/cp.ts
+++ b/src/cp.ts
@@ -35,26 +35,39 @@ export class Cp {
         command.push(srcPath);
         const writerStream = fs.createWriteStream(tmpFileName);
         const errStream = new WritableStreamBuffer();
-        this.execInstance.exec(
-            namespace,
-            podName,
-            containerName,
-            command,
-            writerStream,
-            errStream,
-            null,
-            false,
-            async ({ status }) => {
-                writerStream.close();
-                if (status === 'Failure' || errStream.size()) {
-                    throw new Error(`Error from cpFromPod - details: \n ${errStream.getContentsAsString()}`);
-                }
-                await tar.x({
-                    file: tmpFileName,
-                    cwd: tgtPath,
-                });
-            },
-        );
+        return new Promise<void>((resolve, reject) => {
+            this.execInstance
+                .exec(
+                    namespace,
+                    podName,
+                    containerName,
+                    command,
+                    writerStream,
+                    errStream,
+                    null,
+                    false,
+                    async ({ status }) => {
+                        try {
+                            writerStream.close();
+                            if (status === 'Failure' || errStream.size()) {
+                                return reject(
+                                    new Error(
+                                        `Error from cpFromPod - details: \n ${errStream.getContentsAsString()}`,
+                                    ),
+                                );
+                            }
+                            await tar.x({
+                                file: tmpFileName,
+                                cwd: tgtPath,
+                            });
+                            resolve();
+                        } catch (e) {
+                            reject(e);
+                        }
+                    },
+                )
+                .catch(reject);
+        });
     }
 
     /**
@@ -78,20 +91,31 @@ export class Cp {
         await tar.c({ file: tmpFileName, cwd }, [srcPath]);
         const readStream = fs.createReadStream(tmpFileName);
         const errStream = new WritableStreamBuffer();
-        this.execInstance.exec(
-            namespace,
-            podName,
-            containerName,
-            command,
-            null,
-            errStream,
-            readStream,
-            false,
-            async ({ status }) => {
-                if (status === 'Failure' || errStream.size()) {
-                    throw new Error(`Error from cpToPod - details: \n ${errStream.getContentsAsString()}`);
-                }
-            },
-        );
+        return new Promise<void>((resolve, reject) => {
+            this.execInstance
+                .exec(
+                    namespace,
+                    podName,
+                    containerName,
+                    command,
+                    null,
+                    errStream,
+                    readStream,
+                    false,
+                    async ({ status }) => {
+                        await fs.promises.unlink(tmpFileName);
+                        if (status === 'Failure' || errStream.size()) {
+                            reject(
+                                new Error(
+                                    `Error from cpToPod - details: \n ${errStream.getContentsAsString()}`,
+                                ),
+                            );
+                        } else {
+                            resolve();
+                        }
+                    },
+                )
+                .catch(reject);
+        });
     }
 }
diff --git a/src/util.ts b/src/util.ts
index 024b4416..3436c350 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -194,3 +194,7 @@ export const resolvablePromise = <T>(): ResolvablePromise<T> => {
     promise.reject = reject!;
     return promise;
 };
+
+export const sleep = (ms: number): Promise<void> => {
+    return new Promise<void>((resolve) => setTimeout(resolve));
+};
diff --git a/src/web-socket-handler.ts b/src/web-socket-handler.ts
index 628b1fb3..9b50d2ba 100644
--- a/src/web-socket-handler.ts
+++ b/src/web-socket-handler.ts
@@ -6,11 +6,14 @@ import { KubeConfig } from './config';
 
 const protocols = ['v4.channel.k8s.io', 'v3.channel.k8s.io', 'v2.channel.k8s.io', 'channel.k8s.io'];
 
+export type TextHandler = (text: string) => boolean;
+export type BinaryHandler = (stream: number, buff: Buffer) => boolean;
+
 export interface WebSocketInterface {
     connect(
         path: string,
-        textHandler: ((text: string) => boolean) | null,
-        binaryHandler: ((stream: number, buff: Buffer) => boolean) | null,
+        textHandler: TextHandler | null,
+        binaryHandler: BinaryHandler | null,
     ): Promise<WebSocket.WebSocket>;
 }
 


Test patch:
diff --git a/src/cp_test.ts b/src/cp_test.ts
index 7c1bb705..528ae5f5 100644
--- a/src/cp_test.ts
+++ b/src/cp_test.ts
@@ -1,28 +1,50 @@
 import { anything, anyFunction, instance, mock, verify, when } from 'ts-mockito';
 import * as querystring from 'querystring';
+import { expect } from 'chai';
 import WebSocket = require('isomorphic-ws');
-
+import * as fs from 'node:fs';
+import * as path from 'node:path';
+import { tmpdir } from 'os';
+import * as tar from 'tar';
 import { CallAwaiter } from '../test';
 import { KubeConfig } from './config';
 import { Exec } from './exec';
 import { Cp } from './cp';
-import { WebSocketHandler, WebSocketInterface } from './web-socket-handler';
+import { BinaryHandler, WebSocketHandler, WebSocketInterface } from './web-socket-handler';
+import { V1Status } from './api';
+import { randomUUID } from 'crypto';
+import { sleep } from './util';
 
 describe('Cp', () => {
+    let tmpDir = '';
+
+    beforeEach(() => {
+        tmpDir = `${tmpdir()}/${randomUUID()}`;
+        fs.mkdirSync(tmpDir);
+    });
+
+    afterEach(() => {
+        if (tmpDir) {
+            fs.rmSync(tmpDir, { recursive: true, force: true });
+        }
+    });
+
     describe('cpFromPod', () => {
         it('should run create tar command to a url', async () => {
             const kc = new KubeConfig();
-            const fakeWebSocket: WebSocketInterface = mock(WebSocketHandler);
-            const exec = new Exec(kc, instance(fakeWebSocket));
+            const fakeWebSocketInterface: WebSocketInterface = mock(WebSocketHandler);
+            const fakeWebSocket: WebSocket.WebSocket = mock(WebSocket);
+            const fakeConn: WebSocket.WebSocket = instance(fakeWebSocket);
+            const callAwaiter: CallAwaiter = new CallAwaiter();
+            const exec = new Exec(kc, instance(fakeWebSocketInterface));
             const cp = new Cp(kc, exec);
 
             const namespace = 'somenamespace';
             const pod = 'somepod';
             const container = 'container';
             const srcPath = '/';
-            const tgtPath = '/';
             const cmdArray = ['tar', 'zcf', '-', srcPath];
-            const path = `/api/v1/namespaces/${namespace}/pods/${pod}/exec`;
+            const queryPath = `/api/v1/namespaces/${namespace}/pods/${pod}/exec`;
 
             const query = {
                 stdout: true,
@@ -34,9 +56,46 @@ describe('Cp', () => {
             };
             const queryStr = querystring.stringify(query);
 
-            await cp.cpFromPod(namespace, pod, container, srcPath, tgtPath);
-            // tslint:disable-next-line:max-line-length
-            verify(fakeWebSocket.connect(`${path}?${queryStr}`, null, anyFunction())).called();
+            when(fakeWebSocketInterface.connect(`${queryPath}?${queryStr}`, null, anyFunction())).thenCall(
+                callAwaiter.resolveCall('connect', fakeConn),
+            );
+            when(fakeWebSocket.close()).thenCall(callAwaiter.resolveCall('close'));
+
+            let complete = false;
+            let lastErr = undefined;
+            const promise = cp
+                .cpFromPod(namespace, pod, container, srcPath, tmpDir)
+                .then(() => {
+                    complete = true;
+                })
+                .catch((err) => {
+                    lastErr = err;
+                });
+            expect(lastErr).to.be.undefined;
+            expect(complete).to.be.false;
+
+            const binaryHandler: BinaryHandler = (await callAwaiter.awaitCall('connect'))[2];
+
+            // simulate a network hope with a sleep
+            await sleep(1);
+            const contents = fs.readFileSync('testdata/archive.tgz');
+            binaryHandler(WebSocketHandler.StdoutStream, contents);
+
+            // simulate a network hope with a sleep
+            await sleep(1);
+            const status: V1Status = {
+                status: 'Success',
+            };
+            binaryHandler(WebSocketHandler.StatusStream, Buffer.from(JSON.stringify(status)));
+
+            await promise;
+
+            expect(lastErr).to.be.undefined;
+            expect(complete).to.be.true;
+
+            const found = fs.readFileSync(path.join(tmpDir, 'archive.txt')).toString('utf8');
+            const expected = fs.readFileSync('testdata/archive.txt').toString('utf8');
+            expect(found).to.eq(expected);
         });
     });
 
@@ -52,10 +111,11 @@ describe('Cp', () => {
             const namespace = 'somenamespace';
             const pod = 'somepod';
             const container = 'container';
-            const srcPath = 'testdata/archive.txt';
+            const srcPath = 'archive.txt';
             const tgtPath = '/';
             const cmdArray = ['tar', 'xf', '-', '-C', tgtPath];
-            const path = `/api/v1/namespaces/${namespace}/pods/${pod}/exec`;
+            const cwd = 'testdata/';
+            const queryPath = `/api/v1/namespaces/${namespace}/pods/${pod}/exec`;
 
             const query = {
                 stdout: false,
@@ -68,14 +128,56 @@ describe('Cp', () => {
             const queryStr = querystring.stringify(query);
 
             const fakeConn: WebSocket.WebSocket = instance(fakeWebSocket);
-            when(fakeWebSocketInterface.connect(`${path}?${queryStr}`, null, anyFunction())).thenResolve(
-                fakeConn,
+            when(fakeWebSocketInterface.connect(`${queryPath}?${queryStr}`, null, anyFunction())).thenCall(
+                callAwaiter.resolveCall('connect', fakeConn),
             );
-            when(fakeWebSocket.send(anything())).thenCall(callAwaiter.resolveCall('send'));
+
+            const outFilename = path.join(tmpDir, 'send-data.tar');
+            const out = fs.createWriteStream(outFilename);
+            when(fakeWebSocket.send(anything())).thenCall((data) => {
+                const streamNum = data.readInt8(0);
+                if (streamNum === WebSocketHandler.StdinStream) {
+                    out.write(data.subarray(1));
+                } else {
+                    console.log(streamNum);
+                }
+            });
+
             when(fakeWebSocket.close()).thenCall(callAwaiter.resolveCall('close'));
 
-            await cp.cpToPod(namespace, pod, container, srcPath, tgtPath);
-            verify(fakeWebSocketInterface.connect(`${path}?${queryStr}`, null, anyFunction())).called();
+            let complete = false;
+            let lastErr = undefined;
+            const promise = cp
+                .cpToPod(namespace, pod, container, srcPath, tgtPath, cwd)
+                .then(() => {
+                    complete = true;
+                })
+                .catch((err) => {
+                    lastErr = err;
+                });
+            expect(lastErr).to.be.undefined;
+            expect(complete).to.be.false;
+
+            const binaryHandler: BinaryHandler = (await callAwaiter.awaitCall('connect'))[2];
+
+            // wait for all data to be written and close called
+            await callAwaiter.awaitCall('close');
+            out.close();
+            await tar.x({ f: outFilename, cwd: tmpDir });
+
+            // simulate a network hope with a sleep
+            await sleep(1);
+            const status: V1Status = {
+                status: 'Success',
+            };
+            binaryHandler(WebSocketHandler.StatusStream, Buffer.from(JSON.stringify(status)));
+
+            await promise;
+
+            expect(lastErr).to.be.undefined;
+            expect(complete).to.be.true;
+
+            verify(fakeWebSocketInterface.connect(`${queryPath}?${queryStr}`, null, anyFunction())).called();
         });
     });
 });
diff --git a/test/call-awaiter.ts b/test/call-awaiter.ts
index e42048c7..3f8e3b84 100644
--- a/test/call-awaiter.ts
+++ b/test/call-awaiter.ts
@@ -3,11 +3,14 @@ import { EventEmitter } from 'events';
 export class CallAwaiter extends EventEmitter {
   public awaitCall(event: string) {
     return new Promise<any[]>((resolve) => {
-      this.once(event, resolve);
+      this.once(event, (...args: any[]) => resolve(args));
     });
   }
 
-  public resolveCall(event: string) {
-    return (...args: any[]) => this.emit(event, ...args);
+  public resolveCall(event: string, returnValue?: any) {
+    return (...args: any[]) => {
+      this.emit(event, ...args);
+      return returnValue;
+    }
   }
 }