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.

Sign up for GitHub

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

feat: Added cron specific endpoint #2153

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cbd1e4d
feat: Added cron specific endpoint
mitsuhiko May 24, 2023
c78de46
ref: remove unused var
mitsuhiko May 24, 2023
4261abe
fix: always compile in monitors
mitsuhiko May 24, 2023
3cb68e7
test: Added basic monitors test
mitsuhiko May 24, 2023
1ea9281
test: Added failing test for URL embedded auth
mitsuhiko May 24, 2023
ec22c0f
fix: path handling for crons
mitsuhiko May 24, 2023
867f076
doc: Added changelog entry
mitsuhiko May 24, 2023
8ba655c
Merge branch 'master' into feature/cron-endpoint
mitsuhiko May 24, 2023
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
Prev Previous commit
Next Next commit
test: Added basic monitors test
  • Loading branch information
@mitsuhiko
mitsuhiko committed May 24, 2023
commit 3cb68e7f1c566f94f5cc10a4201a1f8f35a6114e
25 changes: 25 additions & 0 deletions relay-server/src/extractors/request_meta.rs
View file
Open in desktop
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
31 changes: 31 additions & 0 deletions tests/integration/test_crons.py
View file
Open in desktop
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,31 @@ 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",
}