-
Notifications
You must be signed in to change notification settings - Fork 93
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
feat: Added cron specific endpoint #2153
Changes from all commits
cbd1e4d
c78de46
4261abe
3cb68e7
1ea9281
ec22c0f
867f076
8ba655c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
use axum::extract::{DefaultBodyLimit, FromRequest, Path, Query}; | ||
use axum::response::IntoResponse; | ||
use axum::routing::{post, MethodRouter}; | ||
use relay_common::Uuid; | ||
use relay_config::Config; | ||
use relay_general::protocol::EventId; | ||
use relay_monitors::{CheckIn, CheckInStatus}; | ||
use serde::Deserialize; | ||
|
||
use crate::endpoints::common::{self, BadStoreRequest, TextResponse}; | ||
use crate::envelope::{ContentType, Envelope, Item, ItemType}; | ||
use crate::extractors::RequestMeta; | ||
use crate::service::ServiceState; | ||
|
||
#[derive(Debug, Deserialize)] | ||
struct CronPath { | ||
monitor_slug: String, | ||
} | ||
|
||
#[derive(Debug, Deserialize)] | ||
struct CronQuery { | ||
status: CheckInStatus, | ||
check_in_id: Option<Uuid>, | ||
environment: Option<String>, | ||
duration: Option<f64>, | ||
} | ||
|
||
#[derive(Debug, FromRequest)] | ||
#[from_request(state(ServiceState))] | ||
struct CronParams { | ||
meta: RequestMeta, | ||
#[from_request(via(Path))] | ||
path: CronPath, | ||
#[from_request(via(Query))] | ||
query: CronQuery, | ||
} | ||
|
||
impl CronParams { | ||
fn extract_envelope(self) -> Result<Box<Envelope>, BadStoreRequest> { | ||
let Self { meta, path, query } = self; | ||
|
||
let mut envelope = Envelope::from_request(Some(EventId::new()), meta); | ||
|
||
let mut item = Item::new(ItemType::CheckIn); | ||
item.set_payload( | ||
ContentType::Json, | ||
serde_json::to_vec(&CheckIn { | ||
check_in_id: query.check_in_id.unwrap_or_default(), | ||
monitor_slug: path.monitor_slug, | ||
status: query.status, | ||
environment: query.environment, | ||
duration: query.duration, | ||
monitor_config: None, | ||
}) | ||
.map_err(BadStoreRequest::InvalidJson)?, | ||
); | ||
envelope.add_item(item); | ||
|
||
Ok(envelope) | ||
} | ||
} | ||
|
||
async fn handle( | ||
state: ServiceState, | ||
params: CronParams, | ||
) -> Result<impl IntoResponse, BadStoreRequest> { | ||
let envelope = params.extract_envelope()?; | ||
|
||
// Never respond with a 429 | ||
match common::handle_envelope(&state, envelope).await { | ||
Ok(_) | Err(BadStoreRequest::RateLimited(_)) => (), | ||
Err(error) => return Err(error), | ||
}; | ||
// What do we want to return? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to have this comment here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @evanpurkhiser can we remove this comment? And return what you want to return? Or add a note here to followup and why the return value cannot be decided right now. |
||
Ok(TextResponse(None)) | ||
} | ||
|
||
pub fn route<B>(config: &Config) -> MethodRouter<ServiceState, B> | ||
where | ||
B: axum::body::HttpBody + Send + 'static, | ||
B::Data: Send, | ||
B::Error: Into<axum::BoxError>, | ||
{ | ||
post(handle).route_layer(DefaultBodyLimit::max(config.max_event_size())) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
mod attachments; | ||
mod common; | ||
mod cron; | ||
mod envelope; | ||
mod events; | ||
mod forward; | ||
|
@@ -53,6 +54,12 @@ where | |
let store_routes = Router::new() | ||
// Legacy store path that is missing the project parameter. | ||
.route("/api/store/", store::route(config)) | ||
// cron level routes. These are user facing APIs and as such avoid the project ID and | ||
// they support optional trailing slashes. | ||
.route("/api/cron/:monitor_slug/:sentry_key", cron::route(config)) | ||
.route("/api/cron/:monitor_slug/:sentry_key/", cron::route(config)) | ||
.route("/api/cron/:monitor_slug", cron::route(config)) | ||
.route("/api/cron/:monitor_slug/", cron::route(config)) | ||
Comment on lines
+59
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have to support trailing If not, IMO we have to keep our endpoints consistent, and steer the customers to use only one defined route. And currently we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I generally think, from a devex perspective, it's confusing to get errors for missing (or extra) slashes at the end of an URL. Although, I would have imagined we handle this in a cleaner way that adding both URL variants. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree would be nice to handle this in a more general way |
||
.route("/api/:project_id/store/", store::route(config)) | ||
.route("/api/:project_id/envelope/", envelope::route(config)) | ||
.route("/api/:project_id/security/", security_report::route(config)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ use axum::http::request::Parts; | |
use axum::http::StatusCode; | ||
use axum::response::{IntoResponse, Response}; | ||
use axum::RequestPartsExt; | ||
use data_encoding::BASE64; | ||
use relay_common::{ | ||
Auth, Dsn, ParseAuthError, ParseDsnError, ParseProjectKeyError, ProjectId, ProjectKey, Scheme, | ||
}; | ||
|
@@ -479,6 +480,30 @@ fn auth_from_parts(req: &Parts, path_key: Option<String>) -> Result<Auth, BadEve | |
auth = Some(header.parse::<Auth>()?); | ||
} | ||
|
||
// try to get authentication info from basic auth | ||
if let Some(basic_auth) = req | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What the reason even to add basic auth? Using basic auth is super confusion to me to be honest. Especially when you look at the format users should use to make it work. And I talk about the part when you have yo prove the key ( Much easier and more explicit way would be to use the header which we support or path param, which is also supported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're adding this as a bit of an experiment for crons to feel out the ergonomics and DX doing it this way. I don't think we're planning to immediately tell customers to use this. I agree with the points you make though @olksdr, I think it's not unlikely this will get removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, add the note here, which explains why this added and that this is just an experiment and will be removed in the follow-up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @olksdr fwiw I think basic auth should be generally supported, beyond this experiment, entirely independent of this. From the original cc @jan-auer |
||
.headers | ||
.get("authorization") | ||
.and_then(|value| value.to_str().ok()) | ||
.and_then(|x| { | ||
if x.len() >= 6 && x[..6].eq_ignore_ascii_case("basic ") { | ||
x.get(6..) | ||
} else { | ||
None | ||
} | ||
}) | ||
.and_then(|value| { | ||
let decoded = String::from_utf8(BASE64.decode(value.as_bytes()).ok()?).ok()?; | ||
evanpurkhiser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let (public_key, _) = decoded.split_once(':')?; | ||
Auth::from_pairs([("sentry_key", public_key)]).ok() | ||
}) | ||
{ | ||
if auth.is_some() { | ||
return Err(BadEventMeta::MultipleAuth); | ||
} | ||
auth = Some(basic_auth); | ||
} | ||
|
||
// try to extract authentication info from URL query_param .../?sentry_...=<key>... | ||
let query = req.uri.query().unwrap_or_default(); | ||
if query.contains("sentry_") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
import base64 | ||
|
||
|
||
def generate_check_in(slug): | ||
return { | ||
"check_in_id": "a460c25ff2554577b920fcfacae4e5eb", | ||
|
@@ -26,3 +29,57 @@ def test_monitors_with_processing( | |
"status": "in_progress", | ||
"duration": 21.0, | ||
} | ||
|
||
|
||
def test_crons_endpoint_with_processing( | ||
mini_sentry, relay_with_processing, monitors_consumer | ||
): | ||
project_id = 42 | ||
options = {"processing": {}} | ||
relay = relay_with_processing(options) | ||
monitors_consumer = monitors_consumer() | ||
|
||
mini_sentry.add_full_project_config(project_id) | ||
|
||
monitor_slug = "my-monitor" | ||
public_key = relay.get_dsn_public_key(project_id) | ||
basic_auth = base64.b64encode((public_key + ":").encode("utf-8")).decode("utf-8") | ||
relay.post( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just noticed, this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The endpoint supports either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah cool. I'll expand on POST in a future PR. |
||
"/api/cron/{}?status=ok".format(monitor_slug), | ||
headers={"Authorization": "Basic " + basic_auth}, | ||
) | ||
|
||
check_in, message = monitors_consumer.get_check_in() | ||
assert message["start_time"] is not None | ||
assert message["project_id"] == 42 | ||
assert check_in == { | ||
"check_in_id": "00000000000000000000000000000000", | ||
"monitor_slug": "my-monitor", | ||
"status": "ok", | ||
} | ||
|
||
|
||
def test_crons_endpoint_embedded_auth_with_processing( | ||
mini_sentry, relay_with_processing, monitors_consumer | ||
): | ||
project_id = 42 | ||
options = {"processing": {}} | ||
relay = relay_with_processing(options) | ||
monitors_consumer = monitors_consumer() | ||
|
||
mini_sentry.add_full_project_config(project_id) | ||
|
||
monitor_slug = "my-monitor" | ||
public_key = relay.get_dsn_public_key(project_id) | ||
relay.post( | ||
"/api/cron/{}/{}?status=ok".format(monitor_slug, public_key), | ||
) | ||
|
||
check_in, message = monitors_consumer.get_check_in() | ||
assert message["start_time"] is not None | ||
assert message["project_id"] == 42 | ||
assert check_in == { | ||
"check_in_id": "00000000000000000000000000000000", | ||
"monitor_slug": "my-monitor", | ||
"status": "ok", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is the unnecessary new line before this record