Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connectPostPodExec is not implemented #2

Closed
justinmchase opened this issue Jul 14, 2021 · 16 comments · Fixed by #6
Closed

connectPostPodExec is not implemented #2

justinmchase opened this issue Jul 14, 2021 · 16 comments · Fixed by #6
Labels
enhancement New feature or request

Comments

@justinmchase
Copy link

Here is my code:

core.connectPostPodExec(podName, {
  container: containerName,
  tty: false,
  stdin: true,
  stderr: true,
  stdout: true,
  command: `sh -c "kill -s TERM 1"`,
})

Using the official nodejs client this works... except there their API for the command argument is command: string[].

This is the generated code for this function:

  async connectPostPodExec(name: string, opts: {
    command?: string;
    container?: string;
    stderr?: boolean;
    stdin?: boolean;
    stdout?: boolean;
    tty?: boolean;
    abortSignal?: AbortSignal;
  } = {}) {
    const query = new URLSearchParams;
    if (opts["command"] != null) query.append("command", opts["command"]);
    if (opts["container"] != null) query.append("container", opts["container"]);
    if (opts["stderr"] != null) query.append("stderr", opts["stderr"] ? '1' : '0');
    if (opts["stdin"] != null) query.append("stdin", opts["stdin"] ? '1' : '0');
    if (opts["stdout"] != null) query.append("stdout", opts["stdout"] ? '1' : '0');
    if (opts["tty"] != null) query.append("tty", opts["tty"] ? '1' : '0');
    const resp = await this.#client.performRequest({
      method: "POST",
      path: `${this.#root}pods/${name}/exec`,
      expectJson: true,
      querystring: query,
      abortSignal: opts.abortSignal,
    });
  }

According to this blog:
https://www.openshift.com/blog/executing-commands-in-pods-using-k8s-api

To send the command sh -c "kill -s TERM 1" you need to be able to send multiple command parameters so:

command: ["sh", "-c", "kill -s TERM 1"]

Which then would then append each command into a query string such as:

?command=sh&command=-c&command=kill+-s+TERM+1

That means the generated code maybe should look like this:

  async connectPostPodExec(name: string, opts: {
    command?: string | string[];
    // ...
  } = {}) {
    const query = new URLSearchParams;
    if (opts["command"] != null) Array.isArray(opts["command"])
      ? opts["command"].forEach(command => query.append("command", command))
      : query.append("command", opts["command"]);
    // ...
  }
@justinmchase
Copy link
Author

The open API json for the command parameter for this function:

{
    "uniqueItems": true,
    "type": "string",
    "description": "Command is the remote command to execute. argv array. Not executed within a shell.",
    "name": "command",
    "in": "query"
}

It does say "argv array" but I'm not sure how you should know its an array rather than an individual string in this case just from that definition.

Here is the corresponding code in the nodejs Kube client:
https://github.com/kubernetes-client/javascript/blob/master/src/exec.ts#L36

@danopia
Copy link
Member

danopia commented Jul 14, 2021

Ok, so if I'm understanding the desync correctly, I'm only accepting a single string and it needs to also be valid to pass an array of strings.

Based on that JSON snippet it seems the only signal that an array is acceptable would be that uniqueItems key. (though I don't think that key is even correct because it would imply that arguments cannot be repeated, e.g. ?command=echo&command=hello&command=hello&command=hello would violate "uniqueItems": true according on my understanding)

So, this might be best as some sort of special-casing, the pod-specific streaming APIs tend to not fit well into the cookie cutter openAPI specs.

Just so I know, can you confirm if you've noticed other issues with this API function beyond the string array issue? I can put together my own repros but it'll take a little more time. You can also use the /x/kubernetes_client client directly, perhaps copy/pasting and fixing up the function from this module, to unblock yourself sooner.

Thanks for the report! I haven't done much with the raw exec API yet. 😄

@justinmchase
Copy link
Author

I tried hacking on it by extending some classes and getting my own version of the function to call the api but it still did not work so I need to do a little more work to prove absolutely the right fix. But I'm looking at it and will report back.

@justinmchase
Copy link
Author

You know what, I think this is why its not working:
https://github.com/kubernetes-client/javascript/blob/master/src/exec.ts#L57

I think that this particular API needs to be upgraded to a websocket to handle the stdin/out/err streams, etc. This is a good clue I should have an example tomorrow.

@danopia
Copy link
Member

danopia commented Jul 15, 2021

Ok, if it's one of the things that need websockets then this is actually a sizable piece of work I think, because /x/kubernetes_client first needs to form an opinion about how websockets are obtained.

That module also tries polyfilling the http requests on developer machines by shelling out to kubectl and that should also be extended to support these websockets if possible. This is done because kubectl handles a lot of annoying auth and networking stuff that can vary a lot between each person's laptop :P

I'm busy with real life things the next couple days, so feel free to try some things out! In any case I should be able to tackle exposing websockets etc sometime this weekend


I tried hacking on it by extending some classes and getting my own version of the function to call the api

To be clear, what I was referring to is that the connectPostPodExec function can be trivially pasted into your own code and all that needs replacing is this.#client (with an instance from /x/kubernetes_client) and this.#root (with /api/v1/namespaces/${namespace}/). Since API functions in /x/kubernetes_apis don't call each other, I normally find that approach cleaner than extending classes etc.

@justinmchase
Copy link
Author

Yeah I have a working way to patch it so I can adjust just the one function, I haven't gotten it to work yet but here is an example for future users who think they may need to patch a single function too:

import { CoreV1Api, CoreV1NamespacedApi, RestClient } from "../deps.ts";

// We need to patch the pod/exec function to pass multiple commands
// todo: Remove these classes once issue cloudydeno/deno-kubernetes_apis#2 is fixed
// https://github.com/cloudydeno/deno-kubernetes_apis/issues/2

export class PatchedCoreV1Api extends CoreV1Api {
  #client: RestClient
  constructor(client: RestClient) {
    super(client)
    this.#client = client;
  }
  
  patchedNamespace(name: string) {
    return new PatchedCoreV1NamespacedApi(this.#client, name);
  }
}

export class PatchedCoreV1NamespacedApi extends CoreV1NamespacedApi {

  #client: RestClient
  #root: string
  constructor(client: RestClient, namespace: string) {
    super(client, namespace)
    this.#client = client;
    this.#root = `/api/v1/namespaces/${namespace}/`;
  }

  public async patchedConnectPostPodExec(name: string, opts: {
    command?: string[]; // this is different
    container?: string;
    stderr?: boolean;
    stdin?: boolean;
    stdout?: boolean;
    tty?: boolean;
    abortSignal?: AbortSignal;
  } = {}) {
    const query = new URLSearchParams;
    if (opts["command"] != null) opts["command"].forEach(command => query.append("command", command)); // this is different
    if (opts["container"] != null) query.append("container", opts["container"]);
    if (opts["stderr"] != null) query.append("stderr", opts["stderr"] ? '1' : '0');
    if (opts["stdin"] != null) query.append("stdin", opts["stdin"] ? '1' : '0');
    if (opts["stdout"] != null) query.append("stdout", opts["stdout"] ? '1' : '0');
    if (opts["tty"] != null) query.append("tty", opts["tty"] ? '1' : '0');
    const _resp = await this.#client.performRequest({
      method: "POST",
      path: `${this.#root}pods/${name}/exec`,
      expectJson: true,
      querystring: query,
      abortSignal: opts.abortSignal,
    });
  }
}

@danopia
Copy link
Member

danopia commented Jul 27, 2021

Hey, I've started a WIP here for pod/exec and pod/portforward: https://github.com/cloudydeno/deno-kubernetes_client/pull/6/files This currently only works with kubectl proxy --reject-paths '^-$' as the transport because it'll authenticate the websockets.

I'm also still undecided on the API of this, because my initial thought was to split out stdout and stderr but when I was actually using the API I kept wanting to receive one stream with both types of packets preserving order. I think that performRequest will end up returning a TunneledResponse class that has functions to slice up the streams depending on usecase. Once the API is more settled I can rig up pod/exec emulation for KubectlRawRestClient.

Also, Deno WS has a bug where you won't receive the final output after closing stdin, so that's probably relevant if you'll be doing stdin stuff. There's an open PR to fix it.

Anyway; if you're still looking for this functionality in Deno then feel free to look at my WIP and see if it can function in your usecase.

@justinmchase
Copy link
Author

justinmchase commented Jul 27, 2021

Awesome, I can't think of a reason why you'd want to have two seperate outputs, of course you'd need to be able to differentiate between them but having one for loop would be better probably.

Ultimately I had to switch to node to solve my immediate issue but I keep coming up with these little ideas for apps which use the kubernetes api from within a pod, so I will probably try it again soon.

Also, I have a PR here for an unrelated topic, that I'd love to get some more eyes on it if you have a minute:
denoland/deno#11491

It doesn't quite help the websocket cert issue but its in a related code area, modifying the create_client_config would be where the code for adding the client certs to the websocket would go.

@lucsoft
Copy link

lucsoft commented Jul 30, 2023

patchedConnectPostPodExec

When i try to use this function, i get a Error: must specify one of -f and -k and the complete usage info about how to use kubectl

Also when will there a be fix directly landing here? its over two years :D

@danopia
Copy link
Member

danopia commented Jul 31, 2023

When i try to use this function, i get a Error: must specify one of -f and -k and the complete usage info about how to use kubectl

Yea, that's understandable, because of the KubectlRawRestClient (as opposed to KubeConfigRestClient) transport.

On developer setups, /x/kubernetes_client runs kubectl to actually talk to your cluster, by mapping the APIs into kubectl get --raw and similar. This was originally because Deno had several networking limitations, and also ensured compatibility with any cluster. This is the logic that is getting confused and giving you kubectl errors.

But KubeConfigRestClient can't do what you want either.

The blocker is that /x/kubernetes_client does not yet have an API for opening Kubernetes 'tunnels':


Also when will there a be fix directly landing here? its over two years :D

Well, as far as this issue, the 'today' fix would be removing connectPostPodExec :D I just haven't been in a rush to admit defeat like that.


Some things to try if you want to DIY:

  • use Deno.Command to run your own kubectl exec commands, without any of this library
  • I ported over /x/spdy_transport which can be used to connect directly to Kubernetes from Deno, but is still experimental. Exec example here.
  • if your cluster does not use client-certificate auth, you can try using WebSocketStream to connect directly to the PodExec endpoint, I have an example app here. You need to use the Kubernetes websocket protocol, which involves prepended bytes on messages, it's in that file.
  • If you try WebSocketStream but your kubeconfig doesn't work, there are some workarounds on Support for Channel-based APIs

@justinmchase
Copy link
Author

Amazing answer, I would have never figured all that out on my own.

@danopia
Copy link
Member

danopia commented Aug 5, 2023

Yea, honestly as a client implementor, the tunnel commands have taken more of my energy than the rest of the Kubernetes API :)

I want to finally close this feature out, so here's my plan:

  1. Define an API contract in /x/kubernetes_client for tunnels. I have the PR in draft already.
  2. Add a first implementation to /x/kubernetes_client using WebSocketStream. Despite the various restrictions, when the websocket stuff connects, it moves bytes quite well - e.g. working back-pressure. Will be good for testing the next steps.
  3. Get a minor release of /x/kubernetes_client tagged with the new API: v0.6.0
  4. Come back to /x/kubernetes_apis and redo the exec/attach/portforward methods using the new API. Will resolve this issue :) And tag a minor here as well.
  5. Add several more client implementations to /x/kubernetes_client. I have a rough SPDY client and I also want to try shelling out to kubectl exec, should be ok for basic usecases. I'll tag these as patches (at some point) since the tunnel clients are all experimental.

I'll report back after step 4.

@justinmchase
Copy link
Author

If you tag me on PR's I'll do my best to review.

@danopia
Copy link
Member

danopia commented Aug 8, 2023

I tagged /x/[email protected] with the WebSocketStream-based client impl.

I haven't done anything on /x/kubernetes_apis yet, but I did write up a minimal example of using the client impl directly to exec a process in a pod: https://deno.land/x/[email protected]/tunnel-beta/examples/ws-exec-poc.ts?source=

Feel free to try the approach out now and see if websockets works in your setup. (Customize the namespace, container name, etc)

The biggest weakness for Deno WebSockets is lack of TLS certificate setup (cluster CA, client-cert auth) which means that you must use token auth and you must trust the server CA with Deno's CLI flags if it is self-signed.
Tracking ticket for improving Deno's abilities here:

@danopia danopia changed the title connectPostPodExec doesn't work, has possibly incorrect command arg type connectPostPodExec is not implemented Aug 13, 2023
@danopia
Copy link
Member

danopia commented Aug 13, 2023

hi again, I have a draft at #6 which fixes up connectPostPodExec (now named tunnelPodExec) and returns a new utility class modelled after Deno.Command to make output easy to collect:

const exec = await coreApi.tunnelPodExec('my-pod', {
  command: ['uname', '-a'],
  stdout: true,
});
const output = await exec.output();
return new TextDecoder().decode(output.stdout).trimEnd();

I also added an emulation to the kubectl-based client so that basic execs (shown above) can work from outside the cluster. And when using a KubeConfig-based client, there's some extra methods to make ttys easier to work with, here's an example.

Any validation of the utility class is appreciated. I've been focused on a limited set of usecases so maybe haven't accounted for particular others. but in general I'll merge+tag this likely next weekend.

@danopia
Copy link
Member

danopia commented Aug 19, 2023

This is tagged at v0.5.0

When using tunnelPodExec for basic stdin/stdout, the default client should work on a developer's computer just fine. In other cases, and when running on a Kubernetes cluster, replace await autoDetectClient() with a WebSocket-ready client:

import { tunnelBeta, makeClientProviderChain } from 'https://deno.land/x/[email protected]/client.ts';

// Set up a Kubernetes API client which can use WebSockets:
const client = await makeClientProviderChain(tunnelBeta.WebsocketRestClient).getClient();

For in-cluster you'll also want to set DENO_CERT=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt or otherwise make Deno trust the cluster CA, due to denoland/deno#11846.

Anyway then you can use execs or port-fowards. There are several small examples here: https://github.com/cloudydeno/deno-kubernetes_apis/tree/main/lib/examples

Hopefully these new APIs serve people well, I welcome suggestions or bug reports

@danopia danopia added the enhancement New feature or request label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants