Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

METRON-2357: Extends example 4 with a dynamic version #47

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vikgmdev
Copy link

@vikgmdev vikgmdev commented May 20, 2020

Contributor Comments

  • Extends the current Example 4 to include a dynamic version.

It will demonstrate how to automatically send each zeek log to a topic with the same name.

For instance the CONN::LOG log to be sent to the conn topic or Known::CERTS_LOG to the known-certs topic without defining a Log::Filter for each of those.

Pull Request Checklist

Thank you for submitting a contribution to Apache Metron's Bro kafka writer plugin.

In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:

For all changes:

  • Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
  • Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
  • Has your PR been rebased against the latest commit within the target branch (typically master)?

For code changes:

  • Have you included steps to reproduce the behavior or problem that is being changed or addressed?
  • Have you included steps or a guide to how the change may be verified and tested manually?
  • Have you ensured that the full suite of tests and checks have been executed via:
    bro-pkg test $GITHUB_USERNAME/metron-bro-plugin-kafka --version $BRANCH
    
  • Have you written or updated unit tests and or integration tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • Have you verified the basic functionality of the build by building and running locally with Apache Metron's Vagrant full-dev environment or the equivalent?

Copy link
Member

@JonZeolla JonZeolla left a comment

Choose a reason for hiding this comment

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

Hi @vikgmdev, thanks for the contribution! Please take a look at my feedback and we can work through the final few things there before getting this in.

I have also linked this PR in the JIRA you opened and put it in progress - if you email the metron dev mailing list someone should be able to set your permissions so I can reassign the issue to you.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
# replace `_` by `-` for compatibility with acceptable Kafka topic naes
const topic_name: string = sub(topic_name_under, /_/, "-");

if (|Kafka::logs_to_send| == 0 || stream_id in Kafka::logs_to_send)
Copy link
Member

Choose a reason for hiding this comment

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

What is the goal of the |Kafka::logs_to_send| == 0 || portion of this? My read of this is this could get confusing because this example ignores our logs_to_exclude and send_all_active_logs options, if they also get set in an environment following this example config. Is there a way we can refactor this, potentially leveraging send_to_kafka to make it more robust?

Copy link
Author

Choose a reason for hiding this comment

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

Hey, @JonZeolla I just improve this part of the example by removing the condition, it still works the same. I also had some confusions about that line |Kafka::logs_to_send| == 0 || but now is all clear so it isn't need at all.

@ottobackwards
Copy link
Contributor

I have added Victor to the jira contributors role and assigned the jira to him

@JonZeolla
Copy link
Member

Hi @vikgmdev any update on this? Thanks!

@JonZeolla
Copy link
Member

Hi @vikgmdev thanks again for the contribution. Do you have an update? If not we will unfortunately need to mark this as abandoned

@vikgmdev
Copy link
Author

Hi @JonZeolla hadn't the chance to push the updates. Let me retake the issue and I'll push the updates soon. Thanks a lot for your patience.

@vikgmdev
Copy link
Author

vikgmdev commented Oct 1, 2020

Hey, @JonZeolla just noticed I had the changes since a few months ago but haven't pushed them before. We'll wait for you re-review. Thanks.

@JonZeolla
Copy link
Member

Thanks @vikgmdev looking good. Have you tested with the end to end docker/ tests? I think after the initial spin-up you'll need to configure the environment using docker_execute_create_topic_in_kafka.sh (because the topics don't auto-create on publish yet), update the zeek scripts with what you have in the README, and then re-run to ensure it works.

@vikgmdev
Copy link
Author

vikgmdev commented Oct 10, 2020

Hey, @JonZeolla I encountered two issues when running the end to end docker/ tests...

The link to download the nitroba.pcap is no longer available:

--2020-10-10 11:26:40--  http://downloads.digitalcorpora.org/corpora/network-packet-dumps/2008-nitroba/nitroba.pcap
Resolving downloads.digitalcorpora.org (downloads.digitalcorpora.org)... 129.174.125.204
Connecting to downloads.digitalcorpora.org (downloads.digitalcorpora.org)|129.174.125.204|:80... failed: Connection timed out.
Retrying.

I commented that line to unblock me and keep with the test.

But after that, the script is crashing and throwing this error:

...
===================================================
Configuring kafka plugin
configured the kafka plugin
MADE /home/vick/Documents/Anubis/metron-bro-plugin-kafka/docker/test_output/sáb_10_oct_2020_11_32_09_CDT/exercise-traffic_pcap
Running docker_execute_process_data_dir with 
CONTAINER_NAME = metron-bro-plugin-kafka_zeek_1
PCAP_FILE_NAME = exercise-traffic.pcap
OUTPUT_DIRECTORY_NAME = exercise-traffic_pcap
===================================================
executing process_data_file.sh in the zeek docker container
 
PCAP_FILE_NAME = exercise-traffic.pcap
OUTPUT_DIRECTORY_NAME = exercise-traffic_pcap
================================
WARNING: No Site::local_nets have been defined.  It's usually a good idea to define your local networks.
1258563753.320650 fatal error: failed to read a packet from /root/data/example-traffic/exercise-traffic.pcap: truncated dump file; tried to read 1434 captured bytes, only got 415

so not sure if it's an actual bug on the test or if it's just a step I missed from my setup.

@JonZeolla
Copy link
Member

Hi @vikgmdev yeah unfortunately those are known issues. If you manually download nitroba and put it in the pcap directory, and change the zeek version to 3.1.5 it should be able to get through the tests until we can figure out a workaround for the change which is causing the tests to exit due to pcap issues. Sorry for the hassle with this... We plan to make this a non-issue as a part of future work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants