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.

Sign up for GitHub

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

Jump to bottom

sans-io Client #406

Closed
clux opened this issue Feb 8, 2021 · 6 comments
Closed

sans-io Client #406

clux opened this issue Feb 8, 2021 · 6 comments
Labels
client kube Client related question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented Feb 8, 2021

Now that we use tower (#100 via #394), it's worth at least considering what remains to get the kube::Api to a complete sans-io state.

(Going to resume this from the closed discussions in #204 and #112, both of which are too tangential at this point.)

Currently, we have three main structs:

  • Config : kubernetes config from disk, environment, plus optional oauth mechanics
  • Service: a route through which every api request goes (our one embeds a hyper client)
  • Client: an object taking wire data, and routes them through a Service, then deals with the resulting result. However, it also deals with connection upgrading, service routing + error handling of that Service, and Codecs

At the moment we are pretty close to having Client be agnostic to the underlying hyper::Client, but there are some things that are tied together still:

  • Client error handling handles errors related to the Service's implementation (hyper::Error)
  • Client connection upgrading tied to Service's implementation (hyper::upgrade)
  • oauth (while optional) piggy-backs on the chosen tls stack
  • api::remote_command needs a hyper::upgrade for its WebSocketStream
  • Client is hard-coded to a static kube::Service rather than a dyn Tower::Service
  • kube::Error has explicit hyper::Error cases

Possibly there are more, but to me, it sounds like a lot of these could be confined to the service module or an equivalent feature, or by being more generic. I.e.:

  • abstracting Service requirement on Client to allow an arbitrary tower::Service
  • doing the same for WebSocketStream, or if that fails, moving their handling to service directly
  • make Service error handling something a Service should implement
  • putting our default Service implementation, and things tied to it behind a service-hyper feature
@clux clux added question Direction unclear; possibly a bug, possibly could be improved. client kube Client related labels Feb 8, 2021
This was referenced Feb 8, 2021
Closed
Closed
@kazk
Copy link
Member

kazk commented Feb 8, 2021

Hmm, I guess my definition of sans-io was off. I thought it meant abstracting Request -> Response (removing hyper::Client), so wasn't thinking of getting rid of the internal hyper usage.
I'm not sure what's the point of removing internal hyper either. hyper provides functions for working with http_body::Body trait that we'll need anyway.

it sounds like a lot of these could be confined to the service module

I agree.

abstracting Service requirement on Client to allow an arbitrary tower::Service

Can't be arbitrary, but Service<Request<B>> where B: http_body::Body with some other constraints like Clone should work. Methods from hyper used in Client should work with Response<B: http_body::Body> including hyper::upgrade::on.

But this requires Client<S, B> (and propagates to Api<K, S, B>), so I didn't try in #394.

Client is hard-coded to a static kube::Service rather than a dyn Tower::Service

It's currently just indirect. kube::Service can be created from tower::Service<Request<Body>, Response = Response<Body>, Error = BoxError>. This was mainly done to make sure the boundary is clear (extract stuff out of client/config), have certain wrappers types (Buffer and BoxedService), while avoiding type parameters on Client. We can get rid of kube::Service struct and provide middlewares under kube::service if we make Client take generic Service.

api::remote_command needs a hyper::upgrade for its WebSocketStream

hyper::upgrade::Upgraded type in api::remote_command can be generic. Another way might be to let Client return a generic Stream<Item = Result<Message, Error>> and use that.

oauth (while optional) piggy-backs on the chosen tls stack

kube::config should focus on loading the config. Any logic related to token refresh can be moved under kube::service.

@kazk
Copy link
Member

kazk commented Feb 8, 2021 •
edited
Loading

To clarify on the points raised:

  • Client error handling handles errors related to the Service's implementation (hyper::Error)

Client doesn't need to downcast and can just return kube::Error::Service(error).

  • Client connection upgrading tied to Service's implementation (hyper::upgrade)
  • api::remote_command needs a hyper::upgrade for its WebSocketStream

I don't think it's tied to hyper::Client. Just http::Response<B: http_body::Body>. If necessary, WebSocketStream<hyper::upgrade::Upgraded> can be replaced with WebSocketStream<S> where S: AsyncRead + AsyncWrite + Unpin + Sized + Send + 'static.

  • oauth (while optional) piggy-backs on the chosen tls stack

Logic to refresh token should be moved to a Layer and we can make the default implementation optional feature.

  • Client is hard-coded to a static kube::Service rather than a dyn Tower::Service

See the previous comment.

  • kube::Error has explicit hyper::Error cases

I think we'll need it for working with http_body::Body.

@clux
Copy link
Member Author

clux commented Feb 8, 2021

Hmm, I guess my definition of sans-io was off. I thought it meant abstracting Request -> Response (removing hyper::Client), so wasn't thinking of getting rid of the internal hyper usage.
I'm not sure what's the point of removing internal hyper either. hyper provides functions for working with http_body::Body trait that we'll need anyway.

I am being a bit absolutist in interpreting sans-io at the moment, partly for the sake clear module boundaries, but also for seeing if it's possible and worth doing. I'm not fully convinced it's a good idea or necessary to go all the way on it.

Whether or not it's worth going further depends largely on what kind of testing stories we can get from the it. I doubt actually replacing hyper is realistic at this point in time, but it could be helpful to move towards injecting a hyper picked by the user, so they get to choose features, rather than us having to re-export them (though from constraints it sounds more like we only pick a basic version of it, and they fill in the rest of the features..).

hyper::upgrade::Upgraded type in api::remote_command can be generic. Another way might be to let Client return a generic Stream<Item = Result<Message, Error>> and use that.

Interesting, sound like this would effectively encapsulate the protocol almost like a codec then?

(api::remote_command) I don't think it's tied to hyper::Client. Just http::Response<B: http_body::Body>. If necessary, WebSocketStream<hyper::upgrade::Upgraded> can be replaced with WebSocketStream<S> where S: AsyncRead + AsyncWrite + Unpin + Sized + Send + 'static.

hmm, that sounds nice. Maybe it's worth it 🤔

(Service) Can't be arbitrary, but Service<Request<B>> where B: http_body::Body with some other constraints like Clone should work. Methods from hyper used in Client should work with Response<B: http_body::Body> including hyper::upgrade::on. But this requires Client<S, B> (and propagates to Api<K, S, B>), so I didn't try in

Ah. Ok. Yeah, that's a little unwieldy for little benefit.

It's also interesting that http_body::Body has its associated errors in hyper. Feels a bit strange to me.

I wasn't sure how feasible a lot of this was, and so far it sounds like it's a bit painful, and it's not clear if we can get much by doing it yet. Thanks for your thoughts on all of these points.

@kazk
Copy link
Member

kazk commented Feb 8, 2021 •
edited
Loading

It's also interesting that http_body::Body has its associated errors in hyper.

It doesn't. But I think we should continue to use functions from hyper like hyper::body::to_bytes(body: T) where T: http_body::Body which returns hyper::Error.

@kazk
Copy link
Member

kazk commented Feb 9, 2021

Interesting, sound like this would effectively encapsulate the protocol almost like a codec then?

WebSocketStream is already framed, a stream of ws::Message. But that contains types we never use, so AttachedProcess works with a stream filter mapped to the following Message:

https://github.com/clux/kube-rs/blob/77e0e14dd94bbd5974752fa7f3b9aeed309ce254/kube/src/api/remote_command.rs#L259-L267

I was thinking Client can do this instead, but never mind. I forgot that it's bidirectional. Client returning something generic is not enough.

@clux clux mentioned this issue Feb 21, 2021
Closed
@clux
Copy link
Member Author

clux commented Feb 21, 2021

Closing in favour of of #429. A lot of this has been addressed and a lot of it is too nebulous without reason.

The most relevant parts herein is the consequences of potentially having Client taking more than one type of tower::Service: #406 (comment)

@clux clux closed this as completed Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
client kube Client related question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@clux @kazk