enhancement(http source): URL path configuration flexibility, path also exposed as a new field #6626

prognant · 2021-03-04T17:29:30Z

Add three options to the http source:

url_path: the path where log event should be posted (default: /)
path_key: the name of the key used to export the path for each log even (default: path)
strict_path: whether strict path should be enforce, if true only event send to the exact url_path will be accepted, if false log sent to a path that start with the value of url_path will be accepted

For example:

url_path: "/events"
strict_path: false

Will accept events posted to "/events/1234" and "/events/5678".

Outside the http source there is no behaviour change.

Open question: should we accept all URLs for the heroku_logs source as it would allow a user to send logs from multiple app to the same source.

jszwedko

Nice work @prognant !

I left one comment about configuration, but otherwise I think this looks good. I appreciate the comprehensive tests as well as support for url_path and strict_path config options.

In my opinion, we should expand this support to heroku_logs and any other HTTP-based sources for consistency. I think this would include: prometheus_remote_write, splunk_hec, aws_kinesis_firehose. We can split that off into a separate issue though.

jszwedko · 2021-03-04T22:40:42Z

src/sources/http.rs

@@ -20,41 +17,47 @@ use tokio_util::codec::Decoder;
 use warp::http::{HeaderMap, HeaderValue, StatusCode};

 #[derive(Deserialize, Serialize, Debug, Clone)]
+#[serde(default)]


I think this (plus the Default trait implementation below) will have the undesired effect of not requiring the user to specify the address field in their config. We currently require this field.

On the other hand, I could see trying to choose a sensible default for it like you did here. I might suggest something other than :80 as that is likely to be blocked.

For now, I'd recommend just replacing the #[serde(default)]s below and removing it from the struct. You can provide a default value for the field with something like:

https://github.com/timberio/vector/blob/c79545c74bb981c66e1082c12d635240c777aefd/src/sources/syslog.rs#L40-L41

https://github.com/timberio/vector/blob/c79545c74bb981c66e1082c12d635240c777aefd/src/sources/syslog.rs#L64-L66

I think not changing the default behaviour should be favoured when possible, I followed your suggestion to keep the current mandatory behaviour for the address setting and rely on function to provided other default values.

jszwedko · 2021-03-04T22:57:13Z

docs/reference/components/sources/http.cue

@@ -107,8 +107,38 @@ components: sources: http: {
 				}
 			}
 		}
+		url_path: {


What would you think of just calling this path? I think, in the context it appears, it'll be pretty clear it refers to an HTTP URL path.

prognant · 2021-03-05T13:06:09Z

Thanks for the review, I addressed your comments and created a follow-up issue to leverage this change in other relevant sources.

Nice work @prognant !

I left one comment about configuration, but otherwise I think this looks good. I appreciate the comprehensive tests as well as support for url_path and strict_path config options.

In my opinion, we should expand this support to heroku_logs and any other HTTP-based sources for consistency. I think this would include: prometheus_remote_write, splunk_hec, aws_kinesis_firehose. We can split that off into a separate issue though.

jszwedko

Thanks @prognant ! I noticed one more thing, but otherwise this looks good to me.

jszwedko · 2021-03-05T15:26:15Z

src/sources/http.rs

@@ -30,6 +30,12 @@ pub struct SimpleHttpConfig {
    query_parameters: Vec<String>,
    tls: Option<TlsConfig>,
    auth: Option<HttpSourceAuthConfig>,
+    #[serde(default = "crate::serde::default_true")]


TIL we have these helpers

jszwedko · 2021-03-05T16:31:48Z

src/sources/http.rs

+        .await;
+
+        assert_eq!(
+            405,


I realized that 405, "Method not allowed", is a bit of an odd error for this condition. I would have expected a 404.

This looks like a bug in warp: seanmonstar/warp#77

I think you could work around this by, instead of relying on warp's error handling with path::end, just accepting all requests and then, in the handler itself, when strict_match is true, check that the path matches and return a 404 manually if not.

bruceg

The code looks good, except for the HTTP response code issue identified by @jszwedko. Otherwise, I just have a few wording suggestions for the docs.

docs/reference/components/sources/http.cue

bruceg · 2021-03-08T23:37:25Z

src/sources/http.rs

+    for event in events.iter_mut() {
+        event
+            .as_mut_log()
+            .insert(key, Value::from(path.to_string()));


FWIW this should be equivalent to:

Suggested change

.insert(key, Value::from(path.to_string()));

.insert(key, path.to_string().into());

(Not really a suggestion to change it, just a note)

…source Signed-off-by: Sam Giles <sam.giles@citymapper.com>

Allow the HTTP source to use a customised path (instead of / previously). The URL path is also extracted into the "path" key (can be changed by configuration). Signed-off-by: prognant <pierre.rognant@datadoghq.com>

Signed-off-by: prognant <pierre.rognant@datadoghq.com>

Co-authored-by: Bruce Guenter <bruce@timber.io> Signed-off-by: prognant <pierre.rognant@datadoghq.com>

Signed-off-by: prognant <pierre.rognant@datadoghq.com>

jszwedko

Nice work!

jszwedko · 2021-03-09T14:17:55Z

src/sources/util/http.rs

+                        debug!(message = "Path rejected.");
+                        Err(warp::reject::custom(
+                            ErrorMessage::new(StatusCode::NOT_FOUND,
+                            StatusCode::NOT_FOUND.canonical_reason().unwrap().to_string())


Could we use unwrap_or_default() here? I realize that this particular code always has a canonical reason, but unwrap()s tend to jump out.

That sounds safer. In this case I think we may directly go for "Not found".to_string() WDYT ?

I'm also fine with that 😄

It looks like there is also a warp::reject::not_found() you could use instead of warp::reject::custom():

https://docs.rs/warp/0.2.5/warp/reject/fn.not_found.html

I tried to used warp::reject::not_found() but it seems we hit the same issue you shared earlier (seanmonstar/warp#77) that why I moved forward reusing the ErrorMessage struct that already has proper handling code to propagate a http return code.

Aha, gotcha, thanks for the note 👍

Signed-off-by: prognant <pierre.rognant@datadoghq.com>

binarylogic · 2021-03-10T14:16:57Z

Nice work!

prognant requested review from a team and bruceg and removed request for a team March 4, 2021 17:29

binarylogic requested review from jszwedko and removed request for a team March 4, 2021 18:25

jszwedko reviewed Mar 4, 2021

View reviewed changes

prognant force-pushed the prognant/5112-path-handling-for-http-sources branch 2 times, most recently from e3c0538 to 7527840 Compare March 5, 2021 12:27

prognant mentioned this pull request Mar 5, 2021

Support HTTP path field in all relevant sources - follow-up of #6626 #6636

Open

prognant requested a review from jszwedko March 5, 2021 14:55

jszwedko reviewed Mar 5, 2021

View reviewed changes

bruceg reviewed Mar 8, 2021

View reviewed changes

samgiles and others added 6 commits March 9, 2021 12:23

enhancement(http source): Add vector_http_path event key to http …

77f77f4

…source Signed-off-by: Sam Giles <sam.giles@citymapper.com>

URL path flexibility for the HTTP source

81d7e81

Allow the HTTP source to use a customised path (instead of / previously). The URL path is also extracted into the "path" key (can be changed by configuration). Signed-off-by: prognant <pierre.rognant@datadoghq.com>

Asdress reviews

2dd13eb

Signed-off-by: prognant <pierre.rognant@datadoghq.com>

make check-all happy

23f5ef7

Signed-off-by: prognant <pierre.rognant@datadoghq.com>

Apply suggestions from code review

e7c1869

Co-authored-by: Bruce Guenter <bruce@timber.io> Signed-off-by: prognant <pierre.rognant@datadoghq.com>

Throw 404 instead of 405

9a8556e

Signed-off-by: prognant <pierre.rognant@datadoghq.com>

prognant force-pushed the prognant/5112-path-handling-for-http-sources branch from a9acf69 to 9a8556e Compare March 9, 2021 11:27

jszwedko approved these changes Mar 9, 2021

View reviewed changes

Minor simplification

a85ad64

Signed-off-by: prognant <pierre.rognant@datadoghq.com>

prognant requested a review from bruceg March 9, 2021 16:06

bruceg approved these changes Mar 9, 2021

View reviewed changes

prognant merged commit 76ef136 into master Mar 10, 2021

prognant deleted the prognant/5112-path-handling-for-http-sources branch March 10, 2021 13:34

prognant mentioned this pull request Mar 12, 2021

enhancement(http source): Add vector_http_path event key to http source #5191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement(http source): URL path configuration flexibility, path also exposed as a new field #6626

enhancement(http source): URL path configuration flexibility, path also exposed as a new field #6626

prognant commented Mar 4, 2021

jszwedko left a comment

jszwedko Mar 4, 2021

prognant Mar 5, 2021

jszwedko Mar 4, 2021

prognant commented Mar 5, 2021

jszwedko left a comment

jszwedko Mar 5, 2021

jszwedko Mar 5, 2021

bruceg left a comment

bruceg Mar 8, 2021

jszwedko left a comment

jszwedko Mar 9, 2021

prognant Mar 9, 2021

jszwedko Mar 9, 2021

jszwedko Mar 9, 2021

prognant Mar 9, 2021

jszwedko Mar 9, 2021

binarylogic commented Mar 10, 2021

	.insert(key, Value::from(path.to_string()));
	.insert(key, path.to_string().into());

enhancement(http source): URL path configuration flexibility, path also exposed as a new field #6626

enhancement(http source): URL path configuration flexibility, path also exposed as a new field #6626

Conversation

prognant commented Mar 4, 2021

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prognant commented Mar 5, 2021

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruceg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binarylogic commented Mar 10, 2021