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

Filebeat coredns module #11200

Merged
merged 24 commits into from
Mar 26, 2019
Merged

Filebeat coredns module #11200

merged 24 commits into from
Mar 26, 2019

Conversation

alakahakai
Copy link

@alakahakai alakahakai commented Mar 11, 2019

Add filebeat coredns module

Still in progress, waiting on #11134 to be merged first.

11134 is now merged. Get this ready for review.

@alakahakai alakahakai added in progress Pull request is currently in progress. module Filebeat Filebeat SecOps labels Mar 11, 2019
@alakahakai alakahakai requested review from a team as code owners March 11, 2019 21:11
@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@alakahakai alakahakai requested a review from a team as a code owner March 11, 2019 21:11
@alakahakai alakahakai requested a review from a team March 11, 2019 21:20
@ruflin
Copy link
Member

ruflin commented Mar 12, 2019

For awareness, there is also a Metricbeat module for CoreDNS in the works. I wonder if we should sync up on some of the field names? #10585

@alakahakai
Copy link
Author

It seems that coredns metricbeat module focuses on stats and uses coredns.stats object, and there does not seem to be a conflict anywhere from a brief first glance.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Just a few minor issues on the pipeline.

I do wonder if this should just be one fileset like coredns.log that can be used with k8s+coredns or a standalone coredns deployment. The JSON decoding could be conditional based on the log file extension (like decode_json_fields.when.regexp.message: ‘^{‘). I’m just not sure how to handle selecting between k8s mode and regular log mode; maybe it could just be that both the k8s json path and regular log path are defaults at the same time.

var:
	  - name: paths
	    default:
	      - /var/lib/docker/containers/*/*-json.log
              - /var/log/coredns.log

@alakahakai
Copy link
Author

Just a few minor issues on the pipeline.

I do wonder if this should just be one fileset like coredns.log that can be used with k8s+coredns or a standalone coredns deployment. The JSON decoding could be conditional based on the log file extension (like decode_json_fields.when.regexp.message: ‘^{‘). I’m just not sure how to handle selecting between k8s mode and regular log mode; maybe it could just be that both the k8s json path and regular log path are defaults at the same time.

I actually think using a separate fileset will give more flexibility (allow different processing easily) and have better performance. The drawback is to maintain a separate fileset.

@ruflin
Copy link
Member

ruflin commented Mar 19, 2019

If the resulting events are the same for both filesets I would recommend to build one. @ycombinator did something similar for Elasticsearch where there are also JSON and plain logs. Perhaps he can comment here.

@alakahakai
Copy link
Author

alakahakai commented Mar 19, 2019

If the resulting events are the same for both filesets I would recommend to build one. @ycombinator did something similar for Elasticsearch where there are also JSON and plain logs. Perhaps he can comment here.

Note these two deployment modes are exclusive, meaning in one mode it should not ingest logs from the other deployment, and vice versa. Also, one uses auto-discovery and the other does not.

@ruflin
Copy link
Member

ruflin commented Mar 20, 2019

@alakahakai Not sure I can follow your above comment. I expect that per Filebeat instance, only 1 option is used. But let's assume we have 2 deployments, one with json logs and one without. I expect the events which come from the different filebeat instances to be almost identical.

@ycombinator
Copy link
Contributor

ycombinator commented Mar 20, 2019

@ycombinator did something similar for Elasticsearch where there are also JSON and plain logs. Perhaps he can comment here.

@alakahakai In case it's applicable and you're interested in seeing how we handle either JSON or plaintext logs in the elasticsearch filebeat module's server fileset (as an example), look here: https://github.com/elastic/beats/tree/master/filebeat/module/elasticsearch/server

Some specifics:

  1. In the defaults, we allow the fileset to read from either plaintext (*.log) or JSON files (*.json) over here: https://github.com/elastic/beats/blob/master/filebeat/module/elasticsearch/server/manifest.yml#L6-L7. This way the user doesn't have to actively choose one. Of course, they can override the paths themselves in the module's configuration.

  2. The fileset sets up 3 ingest pipelines in all:

    ingest_pipeline:
    - ingest/pipeline.json
    - ingest/pipeline-plaintext.json
    - ingest/pipeline-json.json

    The first pipeline in the list, ingest/pipeline.json, is the entrypoint pipeline to which all events are sent. That pipeline determines whether an event is using plaintext or JSON formatting, then delegates further processing conditionally to one of the other two pipelines in the list, ingest/pipeline-plaintext.json or ingest/pipeline-json.json, respectively.

    The ingest/pipeline.json pipeline also performs any common processing after either the ingest/pipeline-plaintext.json or ingest/pipeline-json.json pipelines have parsed and normalized the event:

    {
    "script": {
    "lang": "painless",
    "source": "if (ctx.elasticsearch.server.gc != null && ctx.elasticsearch.server.gc.observation_duration != null) { if (ctx.elasticsearch.server.gc.observation_duration.unit == params.seconds_unit) { ctx.elasticsearch.server.gc.observation_duration.ms = ctx.elasticsearch.server.gc.observation_duration.time * params.ms_in_one_s;}if (ctx.elasticsearch.server.gc.observation_duration.unit == params.milliseconds_unit) { ctx.elasticsearch.server.gc.observation_duration.ms = ctx.elasticsearch.server.gc.observation_duration.time; } if (ctx.elasticsearch.server.gc.observation_duration.unit == params.minutes_unit) { ctx.elasticsearch.server.gc.observation_duration.ms = ctx.elasticsearch.server.gc.observation_duration.time * params.ms_in_one_m; }} if (ctx.elasticsearch.server.gc != null && ctx.elasticsearch.server.gc.collection_duration != null) { if (ctx.elasticsearch.server.gc.collection_duration.unit == params.seconds_unit) { ctx.elasticsearch.server.gc.collection_duration.ms = ctx.elasticsearch.server.gc.collection_duration.time * params.ms_in_one_s;} if (ctx.elasticsearch.server.gc.collection_duration.unit == params.milliseconds_unit) {ctx.elasticsearch.server.gc.collection_duration.ms = ctx.elasticsearch.server.gc.collection_duration.time; } if (ctx.elasticsearch.server.gc.collection_duration.unit == params.minutes_unit) { ctx.elasticsearch.server.gc.collection_duration.ms = ctx.elasticsearch.server.gc.collection_duration.time * params.ms_in_one_m; }}",
    "params": {
    "minutes_unit": "m",
    "seconds_unit": "s",
    "milliseconds_unit": "ms",
    "ms_in_one_s": 1000,
    "ms_in_one_m": 60000
    }
    }
    },
    {
    "remove": {
    "field": [
    "elasticsearch.server.gc.collection_duration.time",
    "elasticsearch.server.gc.collection_duration.unit",
    "elasticsearch.server.gc.observation_duration.time",
    "elasticsearch.server.gc.observation_duration.unit"
    ],
    "ignore_missing": true
    }
    },
    {
    "date": {
    "field": "elasticsearch.server.timestamp",
    "target_field": "@timestamp",
    "formats": [
    "ISO8601"
    ],
    {< if .convert_timezone >}"timezone": "{{ event.timezone }}",{< end >}
    "ignore_failure": true
    }
    },
    {
    "remove": {
    "field": "elasticsearch.server.timestamp"
    }
    },
    .

The benefit of this approach is that the user doesn't need to perform any additional configuration . Filebeat automatically figures out the format of their events and handles each format appropriately.

Again, I'm not familiar with corednes and I'm not sure if any of the above is applicable in your case. But I just wanted to post it here since @ruflin mentioned it, in case it might give you some ideas.

@alakahakai
Copy link
Author

alakahakai commented Mar 20, 2019

@ycombinator Thanks for the information on how to process multiple types of logs files in one fileset. It is useful. However, I may not be able to use it here because of the reason that I mention earlier: These two deployment modes are exclusive, meaning in one mode it should not ingest logs from the other deployment, and vice versa. Also, one uses auto-discovery and the other does not.

To clarify further, for the Kubernetes *-json.log files (the asterisk here is an auto-generated 64-byte container ID), not all of them will be for coredns, it depends on auto-disocvery. For example, some will be for filebeat envoy module, some will be for filebeat coredns module, and some will be for something else. So we cannot enable ingestion of all the json files by default in the coredns module, meaning we cannot put *.log and *-json.log in the manifest.yml file, and enable them by default. Notice that currently kubernetes fileset is by default set to disabled in the _meta/config.yml, and it will be enabled by auto-discovery per pod/container when annotations are specified accordingly.

- module: coredns
  log: 
    enabled: true
  kubernetes:
    enabled: false

@alakahakai
Copy link
Author

alakahakai commented Mar 20, 2019

@alakahakai Not sure I can follow your above comment. I expect that per Filebeat instance, only 1 option is used. But let's assume we have 2 deployments, one with json logs and one without. I expect the events which come from the different filebeat instances to be almost identical.

@ruflin They are not really identical - the one from Kubernetes has its own meta fields for enrichment. But it is not the main problem. Please take a look at my comments above. Any suggestion is welcome.

@ruflin
Copy link
Member

ruflin commented Mar 21, 2019

Let's try to split up the issue here.

Data structure

Comparing https://github.com/elastic/beats/pull/11200/files#diff-fe233fd564fc292bbb395cdc55fc5280 with https://github.com/elastic/beats/pull/11200/files#diff-e2e15eb66ce61d13cdd689942e7bfbf2 from my perspective they are almost identical. It seem the non k8s is a subset of the k8s one? And the fields that are added in k8s are all k8s specific, not directly related to coredns itself but more to figure out where the event is coming from, like our k8s add metadata processor.

Config files

Different config files are needed based on the deployment. This is definitively a challenge. I wonder if we could something similar like we do for os here with var: https://github.com/elastic/beats/blob/master/filebeat/module/system/syslog/manifest.yml#L4

If we can't find an easy solution I'm also ok with having 2 metricsets for now but we should have to goal to merge them in my opinion.

A more general question about CoreDNS: Is there only 1 log file? Or can it be that CoreDNS has different logs?

@alakahakai
Copy link
Author

alakahakai commented Mar 21, 2019

Data structure

Comparing https://github.com/elastic/beats/pull/11200/files#diff-fe233fd564fc292bbb395cdc55fc5280 with https://github.com/elastic/beats/pull/11200/files#diff-e2e15eb66ce61d13cdd689942e7bfbf2 from my perspective they are almost identical. It seem the non k8s is a subset of the k8s one? And the fields that are added in k8s are all k8s specific, not directly related to coredns itself but more to figure out where the event is coming from, like our k8s add metadata processor.

Yes, I agree that the k8s log file is a superset of the native one.

Config files

Different config files are needed based on the deployment. This is definitively a challenge. I wonder if we could something similar like we do for os here with var: https://github.com/elastic/beats/blob/master/filebeat/module/system/syslog/manifest.yml#L4

I will take a look at that. If there is an easy way to get the runtime state of filebeat to see whether it is running in k8s mode (daemon-set), and add a var according to that, then this might work.

If we can't find an easy solution I'm also ok with having 2 metricsets for now but we should have to goal to merge them in my opinion.

Yes, using one to cover both dynamically will be pretty cool.

A more general question about CoreDNS: Is there only 1 log file? Or can it be that CoreDNS has different logs?

Native deployment will normally only have one log file, while k8s deployment will normally have multiple pods hence multiple log files.

@alakahakai
Copy link
Author

alakahakai commented Mar 21, 2019

Different config files are needed based on the deployment. This is definitively a challenge. I wonder if we could something similar like we do for os here with var: https://github.com/elastic/beats/blob/master/filebeat/module/system/syslog/manifest.yml#L4

I am branching into a separate topic. The manifest file that you quoted has the following variable setting:

- name: convert_timezone
  default: false
  # if ES < 6.1.0, this flag switches to false automatically when evaluating the
  # pipeline
  min_elasticsearch_version:
    version: 6.1.0
    value: false

It seems to me that the min_elasticsearch_version setting is not needed here since both the original value and the min_elasticsearch_version value are false. Did I get it right?

@alakahakai
Copy link
Author

alakahakai commented Mar 22, 2019

Thanks for all the help. I have merged two filesets into one. One trick here is to only put native log under path, while let autodiscovery apply to the JSON k8s log files.

It works except that filebeat will potentially ingest native coredns log files in k8s deployment mode if they exist on the host, which should not happen in real deployments. So, I guess we can live with this limitation for now. If we have to address this, then I think we might need to introduce some checking of runtime state about kubernetes autodiscovery into fileset.go.

@alakahakai
Copy link
Author

alakahakai commented Mar 22, 2019

jenkins, test this

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this forward and getting it into 1 fileset.

@odacremolbap Could you have a look at this as you potentially have more knowledge about CoreDNS and could also test this on your end?

@alakahakai
Copy link
Author

jenkins, test this

@alakahakai
Copy link
Author

@ruflin Should we move ahead with this?


processors:
- add_kubernetes_metadata:
in_cluster: true
Copy link
Member

@andrewkroh andrewkroh Mar 21, 2019

Choose a reason for hiding this comment

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

The indentation here looks like it's off by 2 spaces.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, good catch. It was manually edited for the README and a mistake was made.

@alakahakai alakahakai merged commit f98f2f4 into elastic:master Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants