feat(sources): Add http source using Warp #1650

gedkins · 2020-01-31T00:08:37Z

This addresses #328.

This is based off the logplex source. Code that may be reused by other http source plugins has been separated out into a util/http.rs.

Supports text, json and ndjson body.

Signed-off-by: Giles Edkins gedkins@bluecatnetworks.com

This is based off the logplex source. Code that may be reused by other http source plugins has been separated out into a util/http.rs. Supports text, json and ndjson body. Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

leshow · 2020-01-31T02:53:56Z

Hey! I got a notification for this, I hope you don't mind if I nitpick a few random things.

src/sources/http.rs

leshow · 2020-01-31T02:59:01Z

src/sources/http.rs

+fn json_parse_array_of_object(value: JsonValue) -> Result<Vec<Event>, ErrorMessage> {
+    match value {
+        JsonValue::Array(v) => {
+            let mut out_vec = vec![];


JsonValue::Array(v) => Ok(v .into_iter() .map(json_parse_object) .collect::<Result<Vec<Event>, _>>()?),

There's a FromIterator<Result<A, E>> that makes this collect() call work. Kinda like traverse from Haskell. I didn't run any tests but I think this says the same thing.

binarylogic · 2020-01-31T05:40:10Z

Thanks! @lukesteensen is the best person to review this since he built the logplex source.

Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

Hoverbear · 2020-02-03T22:48:48Z

src/sources/http.rs

+}
+
+fn json_error() -> ErrorMessage {
+    ErrorMessage::new(StatusCode::BAD_REQUEST, "Bad JSON".to_string())


Maybe this should support some error context? Eg "Expected Array or Object, got Number"

Hoverbear · 2020-02-03T22:50:10Z

src/sources/http.rs

+}
+
+fn json_parse(json_str: String) -> Result<JsonValue, ErrorMessage> {
+    serde_json::from_str(&json_str).map_err(|_| json_error())


I don't know if this function's purpose warrants it's existence. If all it does is throw away error context from from_str maybe we can just remove it entirely?

src/sources/http.rs

Hoverbear · 2020-02-03T23:13:46Z

src/sources/util/http.rs

+                )
+                .and_then(|events| {
+                    out.send_all(futures::stream::iter_ok(events)).map_err(
+                        move |_: mpsc::SendError<Event>| {


Is there any context we can get from this error? It'd be sad to throw it away.

We sort of can get more context.
We can log the event it was trying to send when it failed (which is a good idea), but the only reason it would fail is if the receiver is dropped, and the error doesn't give any more insight than that.

Hoverbear · 2020-02-03T23:20:11Z

Once you're feeling this is ready to go code-wise we should also add documentation! You can see what that would look like here: https://github.com/timberio/vector/pull/1264/files#diff-d97eef340a70cc48bfd4c26e66a12780

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

bill-bateman · 2020-02-13T21:43:49Z

Just checking in if you are waiting on anything from us.

Is the documentation that we added enough?

Hoverbear · 2020-02-13T21:52:30Z

@bill-bateman Somehow this notification got lost, I'm sorry about the delay. :(

bill-bateman · 2020-02-13T21:54:04Z

@Hoverbear no worries!

Hoverbear · 2020-02-13T22:09:32Z

@LucioFranco I'm asking you to give a second set of eyes to this since I feel like it has some big picture ideas involved I might be yet familiar with.

.meta/sources/http.toml

Hoverbear · 2020-02-13T22:13:38Z

src/sources/util/http.rs

+
+fn string_to_static_str(s: String) -> &'static str {
+    //necessary because warp 0.1.18 needs a &'static str for the path
+    Box::leak(s.into_boxed_str())


I don't think this is safe, and it definitely looks like a memory leak. https://doc.rust-lang.org/std/boxed/struct.Box.html#method.leak

Yeah it's kinda gross. Couldn't see a way around this if we wanted the path to come from the config, since the version of warp needs &'static str.

If there's a way around this that I don't know then I'd love to know it.

Or if it's more important to not have the leak, we could just pass in a &'static str and have it be hard-coded.

I'd prefer to see it hard coded until we upgrade warp to 0.2.x, since this problem only exists for us in 0.1.x. :)

If we're listening on a socket, I suggest just listen to everything on that socket. The path could be part of the event maybe? @binarylogic 's spec in #328 didn't mention anything about being able to set paths.

OK! - I think I'll just make it hard-coded for now, since it doesn't seem to be needed.

You're right, there is to be a way to get the full-path after the fact (see here) but they say

should probably not be used for request matching/routing.

so not sure about that, better I think to have the hard-coded path and worry about this when someone wants a configurable path (by which point we'll hopefully be on warp 0.2.x!)

Yeah, for now I think removing path support is fine 👍

Hoverbear · 2020-02-13T22:49:07Z

@gedkins Can you ammend that commit with a DCO sign? git commit --amend -s

Co-Authored-By: Ana Hobden <operator@hoverbear.org> Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

LucioFranco · 2020-02-13T23:21:30Z

src/sources/util/http.rs

+                let trigger = trigger.clone();
+                info!("Handling http request: {:?}", headers);
+
+                futures::future::result(


btw if you bring in the IntoFuture trait then you can do Result::into_future.

oh nice didn't know about that

LucioFranco · 2020-02-13T23:44:52Z

src/sources/util/http.rs

+pub trait HttpSource: Clone + Send + Sync + 'static {
+    fn build_event(
+        &self,
+        body: impl Buf,


Is there a reason we don't just make this FullBody?

FullBody doesn't seem to exist in 0.2.x versions of warp so I just didn't use it.

0.1.20 (our version) vs 0.2.1

Right it should exist since we are using 0.1, right?

LucioFranco · 2020-02-13T23:49:40Z

src/sources/util/http.rs

+        }
+        let svc = filter
+            .and(warp::path::end())
+            .boxed()


why does this need to be boxed here?

doesn't need to be

LucioFranco · 2020-02-13T23:54:52Z

src/sources/util/http.rs

+
+fn string_to_static_str(s: String) -> &'static str {
+    //necessary because warp 0.1.18 needs a &'static str for the path
+    Box::leak(s.into_boxed_str())


Yeah, for now I think removing path support is fine 👍

LucioFranco · 2020-02-13T23:55:44Z

src/sources/util/http.rs

+                    Err(r)
+                }
+            };
+            futures::future::result(err)


should be able to use into_future here too

LucioFranco · 2020-02-14T00:19:07Z

src/sources/http.rs

+        .filter(|s| !s.is_empty())
+}
+
+fn decode_body(body: impl Buf, enc: Encoding) -> Result<Vec<Event>, ErrorMessage> {


So I think it may be a bit better here to do something like:

let body = buf.collect::<BytesMut>();

then for the encodings that need them in lines format we can take advantage of https://github.com/timberio/vector/blob/master/lib/codec/src/lib.rs#L18

then for the json encoding we can do something like:

serde_json::from_slice(&body[..])

This would avoid some additional copying

ok sounds good

new commit is mostly for this. lots of changes to the body_to_lines function, which is now not the prettiest... let me know what you think of it, if there are better ways to do what I'm doing.

one issue I had is that the codec won't give the last line if it doesn't end in a newline, so I have a special case for that.

Our codec that we wrote should handle that, you may want to call decode_eof though instead which should handle the last line.

oh perfect! Thanks :)

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear · 2020-03-02T23:34:41Z

@gedkins Thanks so much for this contribution. You rock!

thameezb · 2023-08-10T09:51:52Z

Silly question, looking at the code (and the underlying packages) this looks fully capable of listening on IPv6 addresses (note that I am not a rust dev). However during usage we get errors when trying to listen on both ::PORT and [::]:PORT

Is it possible to bind to the IPv6 global address?

jszwedko · 2023-08-10T15:53:22Z

Silly question, looking at the code (and the underlying packages) this looks fully capable of listening on IPv6 addresses (note that I am not a rust dev). However during usage we get errors when trying to listen on both ::PORT and [::]:PORT

Is it possible to bind to the IPv6 global address?

I responded to this over here: #2508 (comment) . It should be possible to bind to [::]:<port>.

feat(http source): add http source using Warp

46a7f97

This is based off the logplex source. Code that may be reused by other http source plugins has been separated out into a util/http.rs. Supports text, json and ndjson body. Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

leshow reviewed Jan 31, 2020

View reviewed changes

src/sources/http.rs Outdated Show resolved Hide resolved

leshow reviewed Jan 31, 2020

View reviewed changes

src/sources/http.rs Outdated Show resolved Hide resolved

leshow reviewed Jan 31, 2020

View reviewed changes

binarylogic requested a review from lukesteensen January 31, 2020 05:39

binarylogic assigned lukesteensen Jan 31, 2020

feat(http source): some tidy-ups to http source

4f208a7

Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

binarylogic requested review from Hoverbear and removed request for lukesteensen February 3, 2020 21:01

binarylogic assigned Hoverbear and unassigned lukesteensen Feb 3, 2020

Hoverbear reviewed Feb 3, 2020

View reviewed changes

src/sources/http.rs Outdated Show resolved Hide resolved

Hoverbear reviewed Feb 3, 2020

View reviewed changes

Bill Bateman added 2 commits February 4, 2020 16:02

feat(http source): more tidy-ups to http source

60e6900

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

feat(http source): adding documentation

956a902

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

gedkins requested a review from binarylogic as a code owner February 4, 2020 22:04

feat(http source): fixing format issues

ae38d93

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

gedkins changed the title ~~feat(http source): add http source using Warp~~ feat(http source): Add http source using Warp Feb 5, 2020

gedkins added 2 commits February 5, 2020 13:24

Merge branch 'master' into http_source_warp

7e80018

docs: Schema-related fixes for http source

c91c699

Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

gedkins changed the title ~~feat(http source): Add http source using Warp~~ feat(sources): Add http source using Warp Feb 5, 2020

binarylogic added the needs: docs Needs documentation updates label Feb 9, 2020

Hoverbear requested a review from LucioFranco February 13, 2020 22:09

Hoverbear reviewed Feb 13, 2020

View reviewed changes

.meta/sources/http.toml Outdated Show resolved Hide resolved

Hoverbear suggested changes Feb 13, 2020

View reviewed changes

gedkins and others added 2 commits February 13, 2020 18:04

Update .meta/sources/http.toml

e5245e3

Co-Authored-By: Ana Hobden <operator@hoverbear.org> Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

Run make generate

9384507

Signed-off-by: Giles Edkins <gedkins@bluecatnetworks.com>

gedkins force-pushed the http_source_warp branch from 876e9f3 to 9384507 Compare February 13, 2020 23:07

feat(sources): removing leak for http source

9eed009

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

LucioFranco reviewed Feb 14, 2020

View reviewed changes

Bill Bateman added 2 commits February 14, 2020 11:37

feat(http source): improvement to reading the body

0326b33

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

feat(http source): switch to using decoding_eof and FullBody

75eb7e7

Signed-off-by: Bill Bateman <bbateman@bluecatnetworks.com>

Hoverbear requested a review from LucioFranco February 23, 2020 16:26

binarylogic requested a review from Hoverbear March 2, 2020 15:31

Hoverbear added 4 commits March 2, 2020 13:07

Merge branch 'master' into http_source_warp

42d1ad2

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Fix tests

5ab261a

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Fix tests

74b3965

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Fix components

91f24da

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear approved these changes Mar 2, 2020

View reviewed changes

Hoverbear merged commit 1944ae9 into vectordotdev:master Mar 2, 2020

binarylogic mentioned this pull request Mar 9, 2020

New http source #328

Closed

3 tasks

feat(sources): Add http source using Warp #1650

feat(sources): Add http source using Warp #1650

Conversation

gedkins commented Jan 31, 2020

leshow commented Jan 31, 2020

Choose a reason for hiding this comment

binarylogic commented Jan 31, 2020

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

Hoverbear commented Feb 3, 2020

bill-bateman commented Feb 13, 2020

Hoverbear commented Feb 13, 2020

bill-bateman commented Feb 13, 2020

Hoverbear commented Feb 13, 2020

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

Hoverbear commented Feb 13, 2020

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

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

Hoverbear commented Mar 2, 2020

thameezb commented Aug 10, 2023

jszwedko commented Aug 10, 2023 • edited

jszwedko commented Aug 10, 2023 •

edited