feat: allow port forward to deployment and service #2749
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstruebing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
mstruebing
force-pushed
the
435-port-foward
branch
from
8d0ba93 to
3dffec5
Compare
| const namespace = process.argv[2] || 'default'; | ||
| const deploymentName = process.argv[3] || 'demo-deployment'; | ||
| const localPort = parseInt(process.argv[4] || '8080', 10); | ||
| const remotePort = parseInt(process.argv[5] || '8080', 10); |
There was a problem hiding this comment.
| const namespace = process.argv[2] || 'default'; | |
| const deploymentName = process.argv[3] || 'demo-deployment'; | |
| const localPort = parseInt(process.argv[4] || '8080', 10); | |
| const remotePort = parseInt(process.argv[5] || '8080', 10); | |
| const namespace = process.argv[2] ?? 'default'; | |
| const deploymentName = process.argv[3] ?? 'demo-deployment'; | |
| const localPort = parseInt(process.argv[4] ?? '8080', 10); | |
| const remotePort = parseInt(process.argv[5] ?? '8080', 10); |
|
|
||
| // This creates a local server that forwards traffic to a deployment in Kubernetes | ||
| // by resolving the deployment to its first ready pod and port-forwarding to that pod. | ||
| // Usage: npx ts-node port-forward-deployment.ts [namespace] [deploymentName] [localPort] [remotePort] |
There was a problem hiding this comment.
I don't think we should promote using ts-node. I believe it's unmaintained at this point. There is tsx, but also Node can run TypeScript directly now.
There was a problem hiding this comment.
Today I learned! Thank you I didn't knew NodeJS is capable of running typescript direclty.
In case anyone else stumbles upon this comment: https://nodejs.org/en/learn/typescript/run-natively
|
|
||
| const forward = new k8s.PortForward(kc); | ||
|
|
||
| const namespace = process.argv[2] || 'default'; |
There was a problem hiding this comment.
Same comment on these lines about using ?? instead of ||.
There was a problem hiding this comment.
I don't necessarily disagree, but why ?? instead of ||?
There was a problem hiding this comment.
I think in general we should prefer ?? unless we explicitly want to default on all falsy values. In this case, process.argv[x] will be undefined if not set, so they should behave identically. I haven't benchmarked it, but I'd also expect ?? to be faster since it does slightly less work under the hood.
|
|
||
| // This creates a local server that forwards traffic to a service in Kubernetes | ||
| // by resolving the service to its first ready pod and port-forwarding to that pod. | ||
| // Usage: npx ts-node port-forward-service.ts [namespace] [serviceName] [localPort] [remotePort] |
There was a problem hiding this comment.
Same comment about ts-node here.
examples/port-forward-deployment.js
Outdated
| const namespace = process.argv[2] || 'default'; | ||
| const deploymentName = process.argv[3] || 'demo-deployment'; | ||
| const localPort = parseInt(process.argv[4] || '8080', 10); | ||
| const remotePort = parseInt(process.argv[5] || '8080', 10); |
src/test/integration/portForward.ts
Outdated
| .catch((error) => { | ||
| console.error('Deployment port forward error:', error.message); | ||
| socket.destroy(); | ||
| }); |
There was a problem hiding this comment.
Can we use async-await and try...catch to be more consistent.
src/test/integration/portForward.ts
Outdated
| }); | ||
|
|
||
| // Give the server a moment to start | ||
| await setTimeout(500); |
There was a problem hiding this comment.
Is this necessary? Once the listen() Promise above resolves, the server should be ready to serve traffic.
src/test/integration/portForward.ts
Outdated
| const serviceServer = net.createServer((socket) => { | ||
| portForward | ||
| .portForwardService(namespace, serviceName, [containerPort], socket, null, socket) | ||
| .catch((error) => { |
There was a problem hiding this comment.
Same comment here about using async-await and try...catch.
src/test/integration/portForward.ts
Outdated
| }); | ||
|
|
||
| // Give the server a moment to start | ||
| await setTimeout(500); |
There was a problem hiding this comment.
Same question about this being necessary.
src/test/integration/portForward.ts
Outdated
| // Test connection to service via port-forward | ||
| for (let i = 0; i < 5; i++) { | ||
| try { | ||
| const response = await new Promise<string>((resolve, reject) => { |
There was a problem hiding this comment.
Is this logic all the same as above? If so, maybe it should be placed in its own function.
|
@cjihrig thanks for your review. I think I've addressed all your comments. I've made them in separate commits to easier keep track of what I already did and what I still need to do so it also be easy to review. |
fixes #2746