-
Notifications
You must be signed in to change notification settings - Fork 91
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(normalization): bypass host scrubbing via allow list #3813
feat(normalization): bypass host scrubbing via allow list #3813
Conversation
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.
Thank you for doing this! I left a few comments.
Co-authored-by: Joris Bayer <jjbayer@gmail.com>
/// ``` | ||
pub fn scrub_host(host: Host<&str>) -> Cow<'_, str> { | ||
pub fn scrub_host<'a>(host: Host<&'a str>, allow_list: &'a [String]) -> Cow<'a, str> { | ||
if allow_list.contains(&host.to_string()) { |
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.
scrub_ipv4/6
below still have a hard-coded check to allow localhost. Should we put localhost
as the default on sentry side instead, and remove the localhost special-case from the relay code?
@Dav1dde @jjbayer Do you guys know why this happened? Do I need to setup anything other than running
|
I haven't encountered this error before. |
@aldy505 any luck with your setup? If not, I'm happy to trigger your test runs manually. |
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.
@aldy505 I took the liberty of pushing some improvements:
- Also use the allow list in
span::processing
, which is used for standalone spans not embedded in any transaction. - Parse into
Host
directly to save on conversions.
…74195) Complementary relay config for getsentry/relay#3813, based on the discussion on getsentry/relay#3572 ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. --------- Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
Fixes compile error in getsentry#3813
As discussed on #3572. If this is okay, then someone would need to open a PR on sentry for additional relay config.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.