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

feat: Added cron specific endpoint #2153

Merged
merged 8 commits into from
Jun 6, 2023
Merged
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

- Use different error message for empty strings in schema processing. ([#2151](https://github.com/getsentry/relay/pull/2151))

- Relay now supports a simplified cron check-in API. ([#2153](https://github.com/getsentry/relay/pull/2153))
Copy link
Contributor

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


## 23.5.1

**Bug Fixes**:
Expand Down
18 changes: 9 additions & 9 deletions relay-monitors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub enum ProcessCheckInError {
///
#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)]
#[serde(rename_all = "snake_case")]
enum CheckInStatus {
pub enum CheckInStatus {
/// Check-in had no issues during execution.
Ok,
/// Check-in failed or otherwise had some issues.
Expand Down Expand Up @@ -81,7 +81,7 @@ enum IntervalName {

/// The monitor configuration playload for upserting monitors during check-in
#[derive(Debug, Deserialize, Serialize)]
struct MonitorConfig {
pub struct MonitorConfig {
/// The monitor schedule configuration
schedule: Schedule,

Expand All @@ -102,28 +102,28 @@ struct MonitorConfig {

/// The monitor check-in payload.
#[derive(Debug, Deserialize, Serialize)]
struct CheckIn {
pub struct CheckIn {
/// Unique identifier of this check-in.
#[serde(serialize_with = "uuid_simple")]
check_in_id: Uuid,
pub check_in_id: Uuid,

/// Identifier of the monitor for this check-in.
monitor_slug: String,
pub monitor_slug: String,

/// Status of this check-in. Defaults to `"unknown"`.
status: CheckInStatus,
pub status: CheckInStatus,

/// The environment to associate the check-in with
#[serde(default, skip_serializing_if = "Option::is_none")]
environment: Option<String>,
pub environment: Option<String>,

/// Duration of this check since it has started in seconds.
#[serde(default, skip_serializing_if = "Option::is_none")]
duration: Option<f64>,
pub duration: Option<f64>,

/// monitor configuration to support upserts.
#[serde(default, skip_serializing_if = "Option::is_none")]
monitor_config: Option<MonitorConfig>,
pub monitor_config: Option<MonitorConfig>,
}

/// Normalizes a monitor check-in payload.
Expand Down
3 changes: 1 addition & 2 deletions relay-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ publish = false
default = []
processing = [
"dep:minidump",
"dep:relay-monitors",
"dep:symbolic-common",
"dep:symbolic-unreal",
"dep:zstd",
Expand Down Expand Up @@ -54,7 +53,7 @@ relay-general = { path = "../relay-general" }
relay-kafka = { path = "../relay-kafka", optional = true }
relay-log = { path = "../relay-log", features = ["sentry"] }
relay-metrics = { path = "../relay-metrics" }
relay-monitors = { path = "../relay-monitors", optional = true }
relay-monitors = { path = "../relay-monitors" }
relay-profiling = { path = "../relay-profiling" }
relay-dynamic-config = { path = "../relay-dynamic-config"}
relay-quotas = { path = "../relay-quotas" }
Expand Down
85 changes: 85 additions & 0 deletions relay-server/src/endpoints/cron.rs
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?
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to have this comment here?

Copy link
Contributor

Choose a reason for hiding this comment

The 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()))
}
7 changes: 7 additions & 0 deletions relay-server/src/endpoints/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

mod attachments;
mod common;
mod cron;
mod envelope;
mod events;
mod forward;
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to support trailing / and no trailing / URLs? Have the routes without trailing slashes been already exposed to customers and customer got used to using them by now?

If not, IMO we have to keep our endpoints consistent, and steer the customers to use only one defined route. And currently we have / at the end, which I would also lean to.

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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))
Expand Down
25 changes: 25 additions & 0 deletions relay-server/src/extractors/request_meta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 (curl -u sentry_key: ) this colon after sentry_key is super easy to miss and mess up your curl command. IMO it makes this very error-prone to the customer.

Much easier and more explicit way would be to use the header which we support or path param, which is also supported.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 x-sentry-auth header basically nothing remains and we already support auth purely by ?sentry_key in the URL. At this point I would honestly love to see us supporting just Bearer and basic auth with just the public key.

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_") {
Expand Down
57 changes: 57 additions & 0 deletions tests/integration/test_crons.py
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",
Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed, this should be get right @mitsuhiko ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The endpoint supports either.

Copy link
Member

Choose a reason for hiding this comment

The 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",
}