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

Add signer trait #40

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

tdikland
Copy link
Contributor

Add an abstraction for creating signed URLs for the datafiles in (cloud) object storage.

@tdikland
Copy link
Contributor Author

@roeap

A couple of considerations:

  1. I've kept the signature of the signer as simple as possible. We could always add some other methods with default implementation later.
  2. For now the signer trait is designed in such that it lives for the duration of the server (for simplicity). A major alternative is to have to instantiate a new signer for every request that fetches file actions from storage.
  3. Due to (2) it is a little bit more cumbersome to implement this trait for structs wrapping object_store::signer::Signer (but -of course- it still can be done).

Let me know if you think this is reasonable.

pub trait Signer: Send + Sync {
/// Create a signed url from a (cloud) object store URI that is valid from
/// now and expires after the specified duration.
async fn sign(&self, uri: &str, expires_in: Duration) -> Result<SignedUrl, SignerError>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this take a Url? - not entirely sure, but since we should never be dealing with relative paths o.a. and kernel exposes urls as well i think it may be easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not aware that kernel would only exposed parsed URLs. In that case it would make sense to take a url::Url. A (future) consideration could be to take a impl IntoUrl (ref: https://docs.rs/reqwest/latest/reqwest/trait.IntoUrl.html). I'll make the change to url::Url for now


/// A signed URL.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct SignedUrl {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a dedicated type for signed urls? If I think about the flow, we would always get urls from the tables and sign them ad-hoc just before returning them to the client. So are there any situations where we would want to check the properties exposed by this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protocol prescribes the inclusion of the expiration of the signed URL in the table actions. This type contains both + some convenience methods.

Alternatives:

  1. return something like Result<(Url, u64), SignerError> which is harder to grasp and extend.
  2. keep track of the expiration time outside of the Signer which would be slightly incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants