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(new source): New fluent source #7548

Merged
merged 18 commits into from
Jun 7, 2021
Merged

feat(new source): New fluent source #7548

merged 18 commits into from
Jun 7, 2021

Conversation

jszwedko
Copy link
Member

New source for consuming data from fluentd and fluent-bit via their native forward protocol.

This implementation supports the various formats that the specification allows for but does not implement the authentication protocol (#7532) or the acknowledgment (#7533) parts of the protocol. I left these for follow-up based on user demand given it is a significant divergence from the existing TCP source which just consumes data rather than request/response.

Aside from the source implementation, the other significant change here is extraction of some bits of the docker_logs source into a shared docker module to use it in the fluent source integration tests. The integration tests are more similar to the docker_logs integration tests than the other component integration tests in that they each run a separate docker container rather than just standing up one long running one before running the tests as these tests need to run fluentd and fluent-bit with different configurations.

Apologies, I didn't break up the early commits into logical changes, but hopefully the PR itself is reviewable.

Closes #419

@jszwedko jszwedko requested review from bruceg, pablosichert, spencergilbert and a team May 21, 2021 19:46
jszwedko added 13 commits May 21, 2021 15:47
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
@jszwedko jszwedko force-pushed the add-fluent-source branch from 6ce5e82 to d5e8aab Compare May 21, 2021 19:47
@jszwedko jszwedko removed request for a team May 21, 2021 19:47
@@ -343,6 +349,8 @@ wasm = ["lucet-runtime", "lucet-wasi", "lucetc", "vector-wasm"]
# transforms and sinks should depend on this feature.
kubernetes = ["k8s-openapi", "evmap"]

docker = ["bollard", "dirs-next"]
Copy link
Member Author

Choose a reason for hiding this comment

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

New feature to extract some of the bits from the docker_logs source to share with the fluent integration tests.

@@ -0,0 +1,105 @@
use bollard::{errors::Error as DockerError, Docker, API_DEFAULT_VERSION};
Copy link
Member Author

Choose a reason for hiding this comment

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

This file contains bits extracted from docker_logs.

@@ -1,6 +1,7 @@
use super::util::MultilineConfig;
use crate::{
config::{log_schema, DataType, SourceConfig, SourceContext, SourceDescription},
docker::{docker, DockerTlsConfig},
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes to this file are just removals from moving bits into src/docker.rs.

@@ -53,17 +53,17 @@ async fn make_listener(
}
}
pub trait IsErrorFatal {
fn is_error_fatal() -> bool;
fn is_error_fatal(&self) -> bool;
Copy link
Member Author

Choose a reason for hiding this comment

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

Started injecting self here as the fluent source decides which errors are fatal based on whether they are i/o or decode errors (only i/o errors are fatal).

@@ -760,11 +760,7 @@ async fn parses_sink_full_es_basic_auth() {
.unwrap();
}

#[cfg(all(
feature = "docker",
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this feature constraint was here as the feature didn't exist, but it flagged this test as failing to compile when I reintroduced it.

Copy link
Contributor

@spencergilbert spencergilbert left a comment

Choose a reason for hiding this comment

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

Walked through this with @jszwedko and seems good, but definitely should have someone more rust savvy give it a once-over :)

Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

Nothing obviously bad jumped out at me, just a couple of minor tweaks. The real test is integrating with fluentd and fluent-bit, which you are testing.

Can fluentd run in podman as well as docker? If so, is there any option to allow for testing under podman?

Comment on lines 92 to 94
if log.get(log_schema().host_key()).is_none() {
log.insert(log_schema().host_key(), host);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this better with the entry interface?

Suggested change
if log.get(log_schema().host_key()).is_none() {
log.insert(log_schema().host_key(), host);
}
log.entry(log_schema().host_key()).or_insert(host);

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I always forget about that interface.

Copy link
Member Author

@jszwedko jszwedko May 28, 2021

Choose a reason for hiding this comment

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

Actually, entry is a private function that takes a Lookup and returns a Result<Entry, _> which seemed harder to work with then what I have here. I ended up just swapping the get + is_none with contains().

Comment on lines 231 to 233
if !self.unread_frames.is_empty() {
return Ok(self.unread_frames.pop_front());
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to check is_empty first:

Suggested change
if !self.unread_frames.is_empty() {
return Ok(self.unread_frames.pop_front());
}
if let Some(frame) = self.unread_frames.pop_front() {
return Ok(frame);
}

jszwedko added 2 commits May 28, 2021 13:16
Signed-off-by: Jesse Szwedko <[email protected]>
@jszwedko
Copy link
Member Author

jszwedko commented May 28, 2021

Can fluentd run in podman as well as docker? If so, is there any option to allow for testing under podman?

I took a brief look at this and it seems like it could possibly work but I wasn't able to get podman running on my Linux system (I ran into this issue: https://stackoverflow.com/questions/65775075/problems-running-podman-in-ubuntu-20-04 but wasn't able to resolve it).

I was able to run podman system service -t 0 unix:///tmp/podman.sock to run a docker compatible API and then export DOCKER_HOST to point to this though and things appeared to be working (minus the issue I ran into with the uid/gids). I do think we'll need this unreleased change in podman: containers/podman#9972 to configure host.docker.internal to point to the host system (needed since we can't use 127.0.0.1 to indicate vector on OSX given that docker is running in a VM).

Would you be able to try it out?

@jszwedko
Copy link
Member Author

Kicked off integration test to make sure it passes in CI: https://github.com/timberio/vector/actions/runs/886440980

jszwedko added 3 commits May 28, 2021 18:23
Signed-off-by: Jesse Szwedko <[email protected]>
They need to bind to 0.0.0.0 so that they also bind to the docker bridge
so that fluentd / fluentbit, running in docker, can send traffic to it.

Signed-off-by: Jesse Szwedko <[email protected]>
@jszwedko jszwedko merged commit 1408491 into master Jun 7, 2021
@jszwedko jszwedko deleted the add-fluent-source branch June 7, 2021 22:35
jszwedko added a commit that referenced this pull request Jun 8, 2021
Missed this before I merged the #7548

Signed-off-by: Jesse Szwedko <[email protected]>
@jszwedko jszwedko mentioned this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New fluent source
3 participants