standard-ai / ya-gcp

Apache License 2.0
7 stars 8 forks source link

Update yup-oauth and add support for service account impersonation #21

Closed xlambein closed 1 year ago

xlambein commented 1 year ago

This PR's aim was to add service account impersonation as an authentication method, but in order to do this I had to update the version of yup-oauth to 8.1, which required quite a lot of changes.

The main one is that I removed the concept of TokenSource from this repository, for two reasons:

Since version 6, the library has added many new authenticators, covering (I think) the use cases that TokenSource was covering. Instead of a token source, AuthGrpcService now carries an authenticator and a list of scopes.

Updating yup-oauth also required replacing the C: crate::Connect + Clone + Send + Sync + 'static bounds with the following mouthful:

C: tower::Service<http::Uri> + Clone + Send + Sync + 'static,
C::Response: hyper::client::connect::Connection
    + tokio::io::AsyncRead
    + tokio::io::AsyncWrite
    + Send
    + Unpin
    + 'static,
C::Future: Send + Unpin + 'static,
C::Error: Into<Box<dyn std::error::Error + Send + Sync>>,

the reason being that hyper::Connect is actually a private trait that cannot be implemented, and so yup-oauth changed at some point to the bounds above, which are equivalent, albeit annoying.

rnarubin commented 1 year ago

there are some failing tests because tokio is technically an optional dependency. Optionality isn't useful in this context i don't think, you could move it to the required deps

matthew-healy commented 1 year ago

Closing this in favour of #25