Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions clients/python/src/objectstore_client/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ def __init__(
base_url: str,
metrics_backend: MetricsBackend | None = None,
propagate_traces: bool = False,
auth_token: str | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

This requires the user to know too much about our auth mechanism and do too much manually. The follow-up PR shows this very well: https://github.com/getsentry/objectstore/pull/231/files#diff-2901f12558868d8df8503eb8c709ebe697eaf471057eabeadede4c292f869fbeR29-R59

Compare with the implementation we had initially for this, which just required to configure a secret, and the client did all the rest.

We should change it like so:

  • The client is configured with options it needs to create tokens itself
    • (mandatory) The private key. If missing, no tokens will be issued.
    • (optional) A set of permissions. Builder defaults this to all permissions.
    • (optional) An expiry duration (not timestamp!). Defaults to something reasonably short, like 60s.
  • The session is responsible to create the token
    • References above options from the client or gets them injected
    • Together with the session's usecase and scopes it builds the token
    • Applies it to all outgoing requests on the session.
  • Caching:
    • The token can be reused until it is close to expiry, session then regenerates it on-the-fly
    • In practice, this means the session initializes w/o a token and the first request creates it

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 still disagree for all the reasons i've argued previously, but i will take your direction

retries: int | None = None,
timeout_ms: float | None = None,
connection_kwargs: Mapping[str, Any] | None = None,
Expand Down Expand Up @@ -125,6 +126,7 @@ def __init__(
self._base_path = urlparse(base_url).path
self._metrics_backend = metrics_backend or NoOpMetricsBackend()
self._propagate_traces = propagate_traces
self._auth_token = auth_token

def session(self, usecase: Usecase, **scopes: str | int | bool) -> Session:
"""
Expand Down Expand Up @@ -177,6 +179,7 @@ def session(self, usecase: Usecase, **scopes: str | int | bool) -> Session:
self._base_path,
self._metrics_backend,
self._propagate_traces,
self._auth_token,
usecase,
scope_str,
)
Expand All @@ -195,13 +198,15 @@ def __init__(
base_path: str,
metrics_backend: MetricsBackend,
propagate_traces: bool,
auth_token: str | None,
usecase: Usecase,
scope: str,
):
self._pool = pool
self._base_path = base_path
self._metrics_backend = metrics_backend
self._propagate_traces = propagate_traces
self._auth_token = auth_token
self._usecase = usecase
self._scope = scope

Expand All @@ -211,6 +216,8 @@ def _make_headers(self) -> dict[str, str]:
headers.update(
dict(sentry_sdk.get_current_scope().iter_trace_propagation_headers())
)
if self._auth_token:
headers["Authorization"] = f"Bearer {self._auth_token}"
return headers

def _make_url(self, key: str | None, full: bool = False) -> str:
Expand Down
16 changes: 16 additions & 0 deletions clients/rust/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const USER_AGENT: &str = concat!("objectstore-client/", env!("CARGO_PKG_VERSION"
struct ClientBuilderInner {
service_url: Url,
propagate_traces: bool,
auth_token: Option<String>,
reqwest_builder: reqwest::ClientBuilder,
}

Expand Down Expand Up @@ -67,10 +68,19 @@ impl ClientBuilder {
Self(Ok(ClientBuilderInner {
service_url,
propagate_traces: false,
auth_token: None,
reqwest_builder,
}))
}

/// Sets the authorization token that will be used on each request to Objectstore.
pub fn auth_token(mut self, auth_token: String) -> Self {
if let Ok(ref mut inner) = self.0 {
inner.auth_token = Some(auth_token);
}
self
}

/// Changes whether the `sentry-trace` header will be sent to Objectstore
/// to take advantage of Sentry's distributed tracing.
///
Expand Down Expand Up @@ -123,6 +133,7 @@ impl ClientBuilder {
reqwest: inner.reqwest_builder.build()?,
service_url: inner.service_url,
propagate_traces: inner.propagate_traces,
auth_token: inner.auth_token,
}),
})
}
Expand Down Expand Up @@ -348,6 +359,7 @@ pub(crate) struct ClientInner {
reqwest: reqwest::Client,
service_url: Url,
propagate_traces: bool,
auth_token: Option<String>,
}

/// A client for Objectstore. Use [`Client::builder`] to configure and construct a Client.
Expand Down Expand Up @@ -455,6 +467,10 @@ impl Session {

let mut builder = self.client.reqwest.request(method, url);

if let Some(auth_token) = &self.client.auth_token {
builder = builder.bearer_auth(auth_token);
}

if self.client.propagate_traces {
let trace_headers =
sentry_core::configure_scope(|scope| Some(scope.iter_trace_propagation_headers()));
Expand Down