[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[JonZeolla]

Github user JonZeolla commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r152850661
  
--- Diff: scripts/Bro/Kafka/logs-to-kafka.bro ---
@@ -14,32 +14,37 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 #
-##! load this script to enable log output to kafka
+
+##! Load this script to enable log output to kafka
 
 module Kafka;
 
 export {
+   ## Specify which :bro:type:`Log::ID` to exclude from being sent to 
kafka.
##
-   ## which log streams should be sent to kafka?
-   ## example:
-   ##  redef Kafka::logs_to_send = set(Conn::Log, HTTP::LOG, 
DNS::LOG);
+   ## Example:  redef Kafka::logs_to_exclude = set(SSH::LOG);
+   const logs_to_exclude: set[Log::ID] 
+
+   ## Specify which :bro:type:`Log::ID` to send to kafka.
##
+   ## Example:  redef Kafka::logs_to_send = set(Conn::Log, DNS::LOG);
const logs_to_send: set[Log::ID] 
 }
 
 event bro_init() =-5
 {
for (stream_id in Log::active_streams)
{
-   if (stream_id in Kafka::logs_to_send)
-   {
-   local filter: Log::Filter = [
-   $name = fmt("kafka-%s", stream_id),
-   $writer = Log::WRITER_KAFKAWRITER,
-   $config = table(["stream_id"] = fmt("%s", 
stream_id))
-   ];
+   if ( stream_id in Kafka::logs_to_exclude ||
+   (|Kafka::logs_to_send| > 0 && stream_id !in 
Kafka::logs_to_send) )
--- End diff --

Ok I'm convinced, I guess my posture on this is more aggressive than most.  
I will adjust


---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[nickwallen]

Github user nickwallen commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r152848070
  
--- Diff: scripts/Bro/Kafka/logs-to-kafka.bro ---
@@ -14,32 +14,37 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 #
-##! load this script to enable log output to kafka
+
+##! Load this script to enable log output to kafka
 
 module Kafka;
 
 export {
+   ## Specify which :bro:type:`Log::ID` to exclude from being sent to 
kafka.
##
-   ## which log streams should be sent to kafka?
-   ## example:
-   ##  redef Kafka::logs_to_send = set(Conn::Log, HTTP::LOG, 
DNS::LOG);
+   ## Example:  redef Kafka::logs_to_exclude = set(SSH::LOG);
+   const logs_to_exclude: set[Log::ID] 
+
+   ## Specify which :bro:type:`Log::ID` to send to kafka.
##
+   ## Example:  redef Kafka::logs_to_send = set(Conn::Log, DNS::LOG);
const logs_to_send: set[Log::ID] 
 }
 
 event bro_init() =-5
 {
for (stream_id in Log::active_streams)
{
-   if (stream_id in Kafka::logs_to_send)
-   {
-   local filter: Log::Filter = [
-   $name = fmt("kafka-%s", stream_id),
-   $writer = Log::WRITER_KAFKAWRITER,
-   $config = table(["stream_id"] = fmt("%s", 
stream_id))
-   ];
+   if ( stream_id in Kafka::logs_to_exclude ||
+   (|Kafka::logs_to_send| > 0 && stream_id !in 
Kafka::logs_to_send) )
--- End diff --

I too would rather an unset `logs_to_send` to send nothing.  I'd rather not 
'surprise' the user with more logs.  Especially with how difficult it can be to 
truly 'see' the variety of logs that are actually landing in a topic.



---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[JonZeolla]

Github user JonZeolla commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r152674806
  
--- Diff: scripts/Bro/Kafka/logs-to-kafka.bro ---
@@ -14,32 +14,37 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 #
-##! load this script to enable log output to kafka
+
+##! Load this script to enable log output to kafka
 
 module Kafka;
 
 export {
+   ## Specify which :bro:type:`Log::ID` to exclude from being sent to 
kafka.
##
-   ## which log streams should be sent to kafka?
-   ## example:
-   ##  redef Kafka::logs_to_send = set(Conn::Log, HTTP::LOG, 
DNS::LOG);
+   ## Example:  redef Kafka::logs_to_exclude = set(SSH::LOG);
+   const logs_to_exclude: set[Log::ID] 
+
+   ## Specify which :bro:type:`Log::ID` to send to kafka.
##
+   ## Example:  redef Kafka::logs_to_send = set(Conn::Log, DNS::LOG);
const logs_to_send: set[Log::ID] 
 }
 
 event bro_init() =-5
 {
for (stream_id in Log::active_streams)
{
-   if (stream_id in Kafka::logs_to_send)
-   {
-   local filter: Log::Filter = [
-   $name = fmt("kafka-%s", stream_id),
-   $writer = Log::WRITER_KAFKAWRITER,
-   $config = table(["stream_id"] = fmt("%s", 
stream_id))
-   ];
+   if ( stream_id in Kafka::logs_to_exclude ||
+   (|Kafka::logs_to_send| > 0 && stream_id !in 
Kafka::logs_to_send) )
--- End diff --

I was talking to a few people in the bro community about this and I'm 
hearing that people mostly prefer an unset send_logs to send nothing.  What are 
your thoughts on that?  I would prefer to send all by default, but it's not a 
huge deal to go either way for me.


---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[JonZeolla]

Github user JonZeolla commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r152085762
  
--- Diff: scripts/Bro/Kafka/logs-to-kafka.bro ---
@@ -14,32 +14,37 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 #
-##! load this script to enable log output to kafka
+
+##! Load this script to enable log output to kafka
 
 module Kafka;
 
 export {
+   ## Specify which :bro:type:`Log::ID` to exclude from being sent to 
kafka.
##
-   ## which log streams should be sent to kafka?
-   ## example:
-   ##  redef Kafka::logs_to_send = set(Conn::Log, HTTP::LOG, 
DNS::LOG);
+   ## Example:  redef Kafka::logs_to_exclude = set(SSH::LOG);
+   const logs_to_exclude: set[Log::ID] 
+
+   ## Specify which :bro:type:`Log::ID` to send to kafka.
##
+   ## Example:  redef Kafka::logs_to_send = set(Conn::Log, DNS::LOG);
const logs_to_send: set[Log::ID] 
 }
 
 event bro_init() =-5
 {
for (stream_id in Log::active_streams)
{
-   if (stream_id in Kafka::logs_to_send)
-   {
-   local filter: Log::Filter = [
-   $name = fmt("kafka-%s", stream_id),
-   $writer = Log::WRITER_KAFKAWRITER,
-   $config = table(["stream_id"] = fmt("%s", 
stream_id))
-   ];
+   if ( stream_id in Kafka::logs_to_exclude ||
+   (|Kafka::logs_to_send| > 0 && stream_id !in 
Kafka::logs_to_send) )
--- End diff --

Actually, wait, sorry.  If `|Kafka::logs_to_send| > 0` is removed, this 
doesn't send when `logs_to_send` is unset.  Re-adding this.


---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[JonZeolla]

Github user JonZeolla commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r152075056
  
--- Diff: scripts/Bro/Kafka/logs-to-kafka.bro ---
@@ -14,32 +14,37 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 #
-##! load this script to enable log output to kafka
+
+##! Load this script to enable log output to kafka
 
 module Kafka;
 
 export {
+   ## Specify which :bro:type:`Log::ID` to exclude from being sent to 
kafka.
##
-   ## which log streams should be sent to kafka?
-   ## example:
-   ##  redef Kafka::logs_to_send = set(Conn::Log, HTTP::LOG, 
DNS::LOG);
+   ## Example:  redef Kafka::logs_to_exclude = set(SSH::LOG);
+   const logs_to_exclude: set[Log::ID] 
+
+   ## Specify which :bro:type:`Log::ID` to send to kafka.
##
+   ## Example:  redef Kafka::logs_to_send = set(Conn::Log, DNS::LOG);
const logs_to_send: set[Log::ID] 
 }
 
 event bro_init() =-5
 {
for (stream_id in Log::active_streams)
{
-   if (stream_id in Kafka::logs_to_send)
-   {
-   local filter: Log::Filter = [
-   $name = fmt("kafka-%s", stream_id),
-   $writer = Log::WRITER_KAFKAWRITER,
-   $config = table(["stream_id"] = fmt("%s", 
stream_id))
-   ];
+   if ( stream_id in Kafka::logs_to_exclude ||
+   (|Kafka::logs_to_send| > 0 && stream_id !in 
Kafka::logs_to_send) )
--- End diff --

Yeah, that's valid, I have removed the check and simplify.

Yeah, I would prefer a default 'send everything' policy when someone loads 
the package, as long as it's otherwise configured.  That said, it will require 
a bit of Metron testing to make sure that it can handle that.  We don't 
currently handle some of the less interesting logs that are on by default, like 
packet filter or loaded scripts.


---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[nickwallen]

Github user nickwallen commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r151794206
  
--- Diff: scripts/init.bro ---
@@ -18,11 +18,20 @@
 module Kafka;
 
 export {
-  const topic_name: string = "bro" 
-  const max_wait_on_shutdown: count = 3000 
-  const tag_json: bool = F 
-  const kafka_conf: table[string] of string = table(
-["metadata.broker.list"] = "localhost:9092"
-  ) 
-  const debug: string = "" 
+   ## Destination kafka topic name
+   const topic_name: string = "bro" 
+
+   ## Maximum wait on shutdown in milliseconds
+   const max_wait_on_shutdown: count = 3000 
+
+   ## Boolean to JSON with a log stream identifier
--- End diff --

Thanks for adding all the comments.But this one doesn't read so well.  
Or maybe I just don't read so well. :)  Read this one over once more.  If it 
still makes sense to you, then I'm good with it.


---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[nickwallen]

Github user nickwallen commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r151793309
  
--- Diff: scripts/Bro/Kafka/logs-to-kafka.bro ---
@@ -14,32 +14,37 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 #
-##! load this script to enable log output to kafka
+
+##! Load this script to enable log output to kafka
 
 module Kafka;
 
 export {
+   ## Specify which :bro:type:`Log::ID` to exclude from being sent to 
kafka.
##
-   ## which log streams should be sent to kafka?
-   ## example:
-   ##  redef Kafka::logs_to_send = set(Conn::Log, HTTP::LOG, 
DNS::LOG);
+   ## Example:  redef Kafka::logs_to_exclude = set(SSH::LOG);
+   const logs_to_exclude: set[Log::ID] 
+
+   ## Specify which :bro:type:`Log::ID` to send to kafka.
##
+   ## Example:  redef Kafka::logs_to_send = set(Conn::Log, DNS::LOG);
const logs_to_send: set[Log::ID] 
 }
 
 event bro_init() =-5
 {
for (stream_id in Log::active_streams)
{
-   if (stream_id in Kafka::logs_to_send)
-   {
-   local filter: Log::Filter = [
-   $name = fmt("kafka-%s", stream_id),
-   $writer = Log::WRITER_KAFKAWRITER,
-   $config = table(["stream_id"] = fmt("%s", 
stream_id))
-   ];
+   if ( stream_id in Kafka::logs_to_exclude ||
+   (|Kafka::logs_to_send| > 0 && stream_id !in 
Kafka::logs_to_send) )
--- End diff --

If a user configures nothing, neither defines `logs_to_send` nor 
`logs_to_exclude`, then we expect ALL logs to go to Kafka?


---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[nickwallen]

Github user nickwallen commented on a diff in the pull request:


https://github.com/apache/metron-bro-plugin-kafka/pull/2#discussion_r151791732
  
--- Diff: scripts/Bro/Kafka/logs-to-kafka.bro ---
@@ -14,32 +14,37 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 #
-##! load this script to enable log output to kafka
+
+##! Load this script to enable log output to kafka
 
 module Kafka;
 
 export {
+   ## Specify which :bro:type:`Log::ID` to exclude from being sent to 
kafka.
##
-   ## which log streams should be sent to kafka?
-   ## example:
-   ##  redef Kafka::logs_to_send = set(Conn::Log, HTTP::LOG, 
DNS::LOG);
+   ## Example:  redef Kafka::logs_to_exclude = set(SSH::LOG);
+   const logs_to_exclude: set[Log::ID] 
+
+   ## Specify which :bro:type:`Log::ID` to send to kafka.
##
+   ## Example:  redef Kafka::logs_to_send = set(Conn::Log, DNS::LOG);
const logs_to_send: set[Log::ID] 
 }
 
 event bro_init() =-5
 {
for (stream_id in Log::active_streams)
{
-   if (stream_id in Kafka::logs_to_send)
-   {
-   local filter: Log::Filter = [
-   $name = fmt("kafka-%s", stream_id),
-   $writer = Log::WRITER_KAFKAWRITER,
-   $config = table(["stream_id"] = fmt("%s", 
stream_id))
-   ];
+   if ( stream_id in Kafka::logs_to_exclude ||
+   (|Kafka::logs_to_send| > 0 && stream_id !in 
Kafka::logs_to_send) )
--- End diff --

Why do we have to check that `logs_to_send` > 0 ? Is this necessary before 
doing a 'contains' (`in`)?

If it is necessary then we should do the same for `logs_to_exclude`.  If it 
is NOT necessary, then let's just get rid of it to simplify the logic.


---

[GitHub] metron-bro-plugin-kafka pull request #2: DO NOT MERGE METRON-1304: Allow met...

[JonZeolla]

GitHub user JonZeolla opened a pull request:

https://github.com/apache/metron-bro-plugin-kafka/pull/2

DO NOT MERGE METRON-1304: Allow metron-bro-plugin-kafka to include or 
exclude logs

This PR assumes 
[METRON-1303](https://issues.apache.org/jira/browse/METRON-1303).

This allows us the option to specify the inclusion and exclusion of logs to 
send to kafka.  If you were to unset `Kafka::logs_to_send` and specify logs in 
`Kafka::logs_to_exclude`, all currently active logs, except for the ones you've 
excluded, should be sent to kafka.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/JonZeolla/metron-bro-plugin-kafka METRON-1304

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/metron-bro-plugin-kafka/pull/2.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2


commit f21e51f4f91452d66b644b1c041e9a3ae3b39bd7
Author: Jon Zeolla 
Date:   2017-11-07T12:12:53Z

METRON-1303:  Reorganize the metron-bro-plugin-kafka

commit c2f8b2c347f647076c1d0ba17dad5b3794d7957d
Author: Jon Zeolla 
Date:   2017-11-07T12:22:39Z

Fix broken link

commit 3dcecec4faf2119174123ea8a934a83e469080e2
Author: Jon Zeolla 
Date:   2017-11-07T13:12:37Z

METRON-1304: Allow metron-bro-plugin-kafka to include or exclude logs




---