Syslog sink #133

syedriko · 2023-03-22T03:16:43Z

Initial implementation of the syslog sink

JIRA: https://issues.redhat.com/browse/LOG-2288

openshift-ci · 2023-03-22T03:17:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: syedriko
Once this PR has been reviewed and has the lgtm label, please assign alanconway for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vimalk78 · 2023-03-22T07:24:01Z

src/sinks/util/mod.rs

 pub enum Encoding {
    Text,
    Json,
+    Syslog(SyslogConf),
+}


extending the enum and adding a new encoding scheme will perhaps break any code which does a match on this enum.
This enum is used in papertrail sink and it does a match on the enum, perhaps this sink should be changed to handle the new encoding type.

This will also force any new sink to consider syslog encoding, is this desired?

@vimalk78 do you mean 'consider' or 'require'. Does this enum represent the encodings which a sink "may" support or "must" support?

since it is a part of enum, and if a match is done on the enum the enum MUST be matched on all variants.

The compiler would've flagged a mismatch between an enum and a non-exhaustive match but it didn't because we don't compile the papertrail sink.
But please consider the larger context. This PR will not become an upstream submission. In the current version of the upstream encodings are handled differently (https://github.com/vectordotdev/vector/tree/master/lib/codecs/src/encoding/format) and the syslog code would live there.

if this implementation is specifically for us, just to be safe, we should put a feature flag to isolate these changes

#[cfg(feature = "sinks-syslog-rh")]

in more recent releases, (e.g. master which now is 0.27) , this enum does not exist. What is there is more sophisticated way of specifying EncodingWithFraming , and encoding can be chosen from a much larger set of serializers in SerilizerConfig

One of these is LogFmt , and this code seems to be suggesting that LogFmt can be used as serializer for syslog. So perhaps in future releases, we can reuse what already exists to realize a SyslogEncoder.

vimalk78 · 2023-03-22T07:34:29Z

Cargo.toml

@@ -372,7 +372,8 @@ ocp-logging = [
  "sinks-prometheus",
  "sinks-gcp",
  "sinks-splunk_hec",
-  "sinks-http"
+  "sinks-http",
+  "sinks-socket",


so, we are not introducing any new sink for syslog, we are using socket sink with syslog encoding.

just a thought, perhaps a dedicated sink will give us better control over having parity with fluentd syslog output.

Right, technically speaking I did not add a new sink, just a new encoding.

vimalk78 · 2023-03-22T07:39:29Z

src/sinks/util/mod.rs

+
+#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
+#[serde(rename_all = "snake_case")]
+pub struct SyslogConf {


there are some Syslog structures defined in tests/syslog.rs
It already seems to contain same structures as added here, perhaps chance of reuse.

probably should consider flipping it around so the tests depend upon code that does not live in "tests"

In our case the SyslogConf struct reflects the syslog output definition in the CLF and maps directly to the syslog encoding configuration in vector config file, with input validation tied to it. I don't think it's worth the effort trying to combine the two.

vimalk78 · 2023-03-22T07:59:42Z

src/sinks/util/mod.rs

+        Encoding::Syslog(config) => {
+            let mut buf = String::from("<");
+            let pri = get_num_facility(&config.facility, &log) * 8 + get_num_severity(&config.severity, &log);
+            buf.push_str(&pri.to_string());


doing push_str so many times does not seem very efficient. We can do some pre-allocation, or use concat_string macro.
https://github.com/hoodie/concatenation_benchmarks-rs#concat_string_macro

That's an interesting find, there's a benchmark for everything ;)
Agreed, this is naive and I hoped push_str() reallocates exponentially.

jcantrill

Are we missing some unit test?

jcantrill · 2023-03-22T14:59:59Z

src/sinks/util/mod.rs

+                    _ => 24,
+                };
+                if num == 24 {
+                    return Err(de::Error::invalid_value(de::Unexpected::Unsigned(num as u64), &"unknown facility"));


Isn't this captured by the earlier check?

The earlier check was for a facility specified as a number, facility = "12", this one is for a name, facility = "user". There's probably space for more elegance here.

jcantrill · 2023-03-22T15:01:56Z

src/sinks/util/mod.rs

 pub enum Encoding {
    Text,
    Json,
+    Syslog(SyslogConf),
+}


@vimalk78 do you mean 'consider' or 'require'. Does this enum represent the encodings which a sink "may" support or "must" support?

jcantrill · 2023-03-22T15:03:23Z

src/sinks/util/mod.rs

+    Syslog(SyslogConf),
+}
+
+fn add_log_source(log: &LogEvent, buf: &mut String) {


This looks kubernetes specific. That seems like a concern for a general syslog sink. Do we need to abstract it someplace else or at least we should add TODO to do something with it when we do an upstream pull

This PR is not going upstream as is, including for this ^^^ reason.

jcantrill · 2023-03-22T15:04:40Z

src/sinks/util/mod.rs

+                        }
+                    }
+                    Err(_) => {
+                            let num = match field_value_string.to_uppercase().as_str() {


is there some duplication here to deserialization that can be reused?

Yes, this and deserialization, for both severity and facility are asking for refactoring.

jcantrill · 2023-03-22T15:05:09Z

src/sinks/util/mod.rs

+}
+
+fn get_timestamp(log: &LogEvent) -> DateTime::<Local> {
+    match log.get("@timestamp") {


is this specific to our implementation needs for openshift?

vector defines a timestamp key as part of its log schema in src/config/log_schema.rs we can use that.

pub fn timestamp_key(&self) -> &str { &self.timestamp_key }

This PR is not going upstream as it is, there are things in it that don't make sense outside of our use case.

syedriko · 2023-03-31T14:07:56Z

/test unit

syedriko · 2023-03-31T16:01:57Z

/test cluster-logging-operator-e2e-5-6

syedriko · 2023-03-31T18:15:38Z

/test cluster-logging-operator-e2e-5-6

github-actions bot added the domain: sinks label Mar 22, 2023

openshift-ci bot requested review from pmoogi-redhat and vparfonov March 22, 2023 03:17

vimalk78 reviewed Mar 22, 2023

View reviewed changes

jcantrill reviewed Mar 22, 2023

View reviewed changes

syedriko added 5 commits March 30, 2023 10:46

Syslog sink

d07a3c9

added missing NOTICE severity

48be488

corrected handling of hostname

9da44c9

NILVALUE for structured data

d0c75b4

added spaces to the addLogSource fields

481ffae

added payload_key

9eb7fc3

syedriko force-pushed the syedriko-syslog-sink branch from 43b4fd5 to 9eb7fc3 Compare April 1, 2023 05:54

added defaults for RFC5424 fields

92c3fc7

syedriko merged commit 2929973 into ViaQ:v0.21-rh Apr 3, 2023
25 of 42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syslog sink #133

Syslog sink #133

syedriko commented Mar 22, 2023 •

edited

openshift-ci bot commented Mar 22, 2023

vimalk78 Mar 22, 2023

jcantrill Mar 22, 2023

vimalk78 Mar 22, 2023

syedriko Mar 23, 2023 •

edited

vimalk78 Mar 23, 2023 •

edited

vimalk78 Mar 23, 2023

vimalk78 Mar 22, 2023

vimalk78 Mar 22, 2023

syedriko Mar 23, 2023

vimalk78 Mar 22, 2023

jcantrill Mar 22, 2023

syedriko Mar 23, 2023

vimalk78 Mar 22, 2023

syedriko Mar 23, 2023

jcantrill left a comment

jcantrill Mar 22, 2023

syedriko Mar 23, 2023

jcantrill Mar 22, 2023

jcantrill Mar 22, 2023

syedriko Mar 23, 2023

jcantrill Mar 22, 2023

syedriko Mar 23, 2023

jcantrill Mar 22, 2023

vimalk78 Mar 22, 2023

syedriko Mar 23, 2023

syedriko commented Mar 31, 2023

syedriko commented Mar 31, 2023

syedriko commented Mar 31, 2023

Syslog sink #133

Syslog sink #133

Conversation

syedriko commented Mar 22, 2023 • edited

openshift-ci bot commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syedriko Mar 23, 2023 • edited

Choose a reason for hiding this comment

vimalk78 Mar 23, 2023 • edited

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

jcantrill 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

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

syedriko commented Mar 31, 2023

syedriko commented Mar 31, 2023

syedriko commented Mar 31, 2023

syedriko commented Mar 22, 2023 •

edited

syedriko Mar 23, 2023 •

edited

vimalk78 Mar 23, 2023 •

edited