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

implement the last subresources #127

Open
9 of 11 tasks
clux opened this issue Feb 9, 2020 · 16 comments
Open
9 of 11 tasks

implement the last subresources #127

clux opened this issue Feb 9, 2020 · 16 comments
Labels
api Api abstraction related client-gold gold client requirements help wanted Not immediately prioritised, please help!

Comments

@clux
Copy link
Member

clux commented Feb 9, 2020

Subresources have a lot of special case logic, that is not easily derivable from k8s_openapi. So far we have implemented (see subresources.rs):

The general outline for the http based subresources:

We need a definition of what they are in subresources. The way to upgrade would be:

  1. Implement subresource queries straight on Resource without restrictions (for now):
impl Resource {
    /// Get a pod logs
    pub fn log(&self, name: &str, lp: &LogParams) -> Result<http::Request<Vec<u8>>> {
        ...
        // Check https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/ for urls
    }
}
  1. Make a marker trait for the subresource:
pub trait Loggable {}

impl<K> Api<K>
where
    K: Clone + DeserializeOwned + Meta + Loggable,
{
    pub async fn log(&self, name: &str, lp: &LogParams) -> Result<String> {
        let req = self.api.log(name, lp)?;
        Ok(self.client.request_text(req).await?)
    }

    pub async fn log_stream(&self, name: &str, lp: &LogParams) -> Result<impl Stream<Item = Result<Bytes>>> {
        let req = self.api.log(name, lp)?;
        Ok(self.client.request_text_stream(req).await?)
    }
}
  1. Designate what resources implement this subresource:
impl Loggable for k8s_openapi::api::core::v1::Pod {}

Write one test case for one resource in examples (e.g. examples/log_openapi.rs).

EDIT (0.46.0):

Now this is not as difficult for the specialized requests requiring websockets (ws feature).
This was discussed at length in #229, and finally implemented in #360.
Implementors of the last subresources ought to look at that PR for help.

@clux clux added enhancement good first issue Good for newcomers help wanted Not immediately prioritised, please help! labels Feb 9, 2020
@clux clux changed the title implement common subresources implement more common subresources Feb 9, 2020
@clux clux added api Api abstraction related and removed enhancement labels Feb 26, 2020
@koiuo

This comment has been minimized.

@clux

This comment has been minimized.

@clux clux removed the good first issue Good for newcomers label Jun 7, 2020
@clux clux added the client-gold gold client requirements label Aug 14, 2020
@clux
Copy link
Member Author

clux commented Jan 2, 2021

0.46.0 introduces the ws feature using async-tungstenite to pave the way for the last subresources listed above. Have updated the issue slightly. Implementations of the last ws-based subresources can look at #360 for an excellent PR that covers (among the websocket handling itself) examples for implementing pods/attach and pods/exec.

@clux clux changed the title implement more common subresources implement the last subresources Jan 2, 2021
@kazk
Copy link
Member

kazk commented Jan 5, 2021

I'll do pods/portforward. kube should probably just provide AsyncRead + AsyncWrite for each port and not bind to local ports. Users should be able to do that if they want to.

I have a rough sketch that can do something similar to Python client's example:

// Portforward nginx container
let mut pf = pods.portforward("example", &[80]).await?; // Portforwarder
let mut ports = pf.ports().unwrap(); // Vec<Port>
let mut port = ports[0].stream().unwrap(); // impl AsyncRead + AsyncWrite + Unpin
port.write_all(b"GET / HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\nAccept: */*\r\n\r\n")
    .await?;
let mut rstream = tokio_util::io::ReaderStream::new(port);
if let Some(res) = rstream.next().await {
    match res {
        Ok(bytes) => {
            let response = std::str::from_utf8(&bytes[..]).unwrap();
            println!("{}", response);
            assert!(response.contains("Welcome to nginx!"));
        }
        Err(err) => eprintln!("{:?}", err),
    }
}

I'll open a draft PR later (it's too messy at the moment to show...) to discuss the design.

That should make kube a gold client except for "Proto encoding".

Gold Requirements Client Capabilities

  • Support exec, attach, port-forward calls (these are not normally supported out of the box from swagger-codegen)
  • Proto encoding

I'm not sure what that means. Do we need to support Protobufs?

@clux
Copy link
Member Author

clux commented Jan 5, 2021

Yeah, that sounds sensible. Listening sounds like an application concern. In an example we can maybe use TcpListener or something and direct it to the writable stream to show how to do the kubectl port-forward equivalent.

protobufs

Ugh. Ah, yeah, I am not sure that's really possible atm. The go api has protobuf codegen hints (see api/types.go) like:

// +optional
// +patchMergeKey=name
// +patchStrategy=merge
EphemeralContainers []EphemeralContainer `json:"ephemeralContainers,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,34,rep,name=ephemeralContainers"`

whereas the (huge) swagger codegen has the equivalent json for that part of the PodSpec:

        "ephemeralContainers": {
          "description": "...",
          "items": {
            "$ref": "#/definitions/io.k8s.api.core.v1.EphemeralContainer"
          },
          "type": "array",
          "x-kubernetes-patch-merge-key": "name",
          "x-kubernetes-patch-strategy": "merge"
        },

think the 34 there would be required, but haven't looked at this much. Arnavion might be able to say for sure whether it's possible, I can ping him on k8s-openapi.

@kazk
Copy link
Member

kazk commented Jan 5, 2021

The official JavaScript client is Gold and just have something like this (only deserializing):

export class ProtoClient {
    public readonly 'config': KubeConfig;
    public async get(msgType: any, requestPath: string): Promise<any> {
        // ...
        const req = http.request(options);
        const result = await new Promise<any>((resolve, reject) => {
            let data = '';
            req.on('data', (chunk) => {
                data = data + chunk;
            });
            req.on('end', () => {
                const obj = msgType.deserializeBinary(data);
                resolve(obj);
            });
            req.on('error', (err) => {
                reject(err);
            });
        });
        req.end();
        return result;
    }
}

https://github.com/kubernetes-client/javascript/blob/master/src/proto-client.ts

(BTW, it can't portforward more than one port. https://github.com/kubernetes-client/javascript/blob/a8d4f1c7bea2b27403d380e22e492b6680f80b31/src/portforward.ts#L19)

I guess the requirement for Gold is not well-defined? Python one seems more capable, but that's Silver.


When I searched "Kubernetes Proto Encoding", https://kubernetes.io/docs/reference/using-api/api-concepts/#protobuf-encoding came up.

@clux
Copy link
Member Author

clux commented Jan 5, 2021

Yeah, I think the client grading document is a thing they they wrote and applied to the clients they generated themselves. It technically doesn't apply to us (because we don't have our client in that place they say it should be in), but it's a nice benchmark otherwise. I have opened an issue to continue protobuf discussions in #371 at any rate.

@chroche
Copy link

chroche commented Feb 6, 2021

Naive question about pods/eviction, this feature doesn't seem to get much love at all. Is there a possible workaround / a direct way to create ak8s_openapi Eviction? Is there a reason why there seems to be no interest in this feaure?

@clux
Copy link
Member Author

clux commented Feb 6, 2021

It's probably just that no one has asked for it yet. It actually looks very simple. I can have a go at adding it.

clux added a commit that referenced this issue Feb 6, 2021
@clux
Copy link
Member Author

clux commented Feb 6, 2021

Eviction handled in #393

@jmintb
Copy link
Contributor

jmintb commented Oct 27, 2022

Can I have a go at implementing ephemeral container support?

@clux
Copy link
Member Author

clux commented Oct 27, 2022

@jmintb Go for it!

@jmintb

This comment was marked as resolved.

@clux

This comment was marked as resolved.

@jmintb

This comment was marked as resolved.

@jmintb

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related client-gold gold client requirements help wanted Not immediately prioritised, please help!
Projects
Status: Backlog
Development

No branches or pull requests

5 participants