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

[Elastic Log Driver] Create a config shim between libbeat and the user #18605

Merged
merged 21 commits into from
May 29, 2020

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented May 17, 2020

What does this PR do?

This PR adds a "config shim" for the plugin. This is currently a draft that hard-codes a number of fundamental configs needed for the plugin to run, as opposed to letting the user specify unlimited dot-config options that we may or may not have tested.

Why is it important?

Right now, the config system for the plugin is one of the most user-hostile components of the plugin. We expose "raw" libbeat config options similar to filebeat command line options. In the long-term, a config system like this is untenable, as specifying complex config logic, nested options, and lists is incredibly awkward and verbose. This means that right now, we have none of the upsides of using libbeat's config, and the downside of relatively verbose config flags for basic options, i.e, output.elasticsearch.host.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

All supported config flags are documented and in config.go. Pull down the change, build the plugin on a docker host with mage BuildAndInstall, then test with a command like docker run -it --log-driver elastic/elastic-logging-plugin:8.0.0 --log-opt endpoint=griffon.nest:9200 debian:latest /bin/bash

@fearful-symmetry fearful-symmetry added enhancement Team:Integrations Label for the Integrations team labels May 17, 2020
@fearful-symmetry fearful-symmetry requested a review from a team May 17, 2020 20:40
@fearful-symmetry fearful-symmetry self-assigned this May 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels May 17, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 17, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 499
Skipped 127
Total 626

@urso
Copy link

urso commented May 18, 2020

+1 on only exposing a small set of settings via CLI.

Do we only want to support Elasticsearch output, or other outputs as well?

Having only a small number of settings, how about making them configurable via environment variables as well?

@fearful-symmetry
Copy link
Contributor Author

@urso I want to support additional inputs later, yah. Right now I'm hoping to have one small-ish PR to add support for the new CLI scheme, then later on add more inputs as needed.

Environment variable support could work, although it adds a great deal of ugliness to the config, as we need to hard-code any option that a user might want to fiddle with.

@fearful-symmetry fearful-symmetry requested review from urso, bpintea and a team and removed request for bpintea May 19, 2020 17:29
@fearful-symmetry
Copy link
Contributor Author

Just added support for the additional ES settings. Not sure if it's worth adding new outputs now, or just getting this through so we can move on to other components, since we're hoping this is just a shim until we can get this integrated with fleet.

@fearful-symmetry fearful-symmetry marked this pull request as ready for review May 26, 2020 14:41
@fearful-symmetry fearful-symmetry requested review from urso and a team May 26, 2020 15:51
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #18605 updated]

  • Start Time: 2020-05-26T16:15:53.703+0000

  • Duration: 35 min 11 sec

Test stats 🧪

Test Results
Failed 0
Passed 508
Skipped 127
Total 635

@fearful-symmetry fearful-symmetry requested review from urso, dedemorton and a team May 26, 2020 18:33
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 27, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #18605 updated]

  • Start Time: 2020-05-28T19:29:55.458+0000

  • Duration: 51 min 35 sec

Test stats 🧪

Test Results
Failed 0
Passed 507
Skipped 127
Total 634

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

You did a really nice job on the documentation updates! I like that the user doesn't need to think about as many options now. Made a few suggestions, but overall, definitely an improvement.

x-pack/dockerlogbeat/docs/configuration.asciidoc Outdated Show resolved Hide resolved
x-pack/dockerlogbeat/readme.md Outdated Show resolved Hide resolved
x-pack/dockerlogbeat/readme.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

Doc changes LGTM!

@fearful-symmetry fearful-symmetry merged commit 73150c2 into elastic:master May 29, 2020
fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request May 29, 2020
elastic#18605)

* init commit of config shim

* update docs

* make check

* add timeout

* move config system to use typeconv

* add rest of backoff settings.

* make fmt

* some cleanup

* use uint64 hash for structs

* make fmt

* create custom index manager, remove ILM support

* add support for multiple endpoints

* update tests

* update docs

* remove setup options

* remove old tests

* try to update asciidocm change 'endpoint' to 'hosts'

* trying to fix CI

* update docs

* fix backtics

(cherry picked from commit 73150c2)
fearful-symmetry added a commit that referenced this pull request Jun 1, 2020
#18605) (#18864)

* init commit of config shim

* update docs

* make check

* add timeout

* move config system to use typeconv

* add rest of backoff settings.

* make fmt

* some cleanup

* use uint64 hash for structs

* make fmt

* create custom index manager, remove ILM support

* add support for multiple endpoints

* update tests

* update docs

* remove setup options

* remove old tests

* try to update asciidocm change 'endpoint' to 'hosts'

* trying to fix CI

* update docs

* fix backtics

(cherry picked from commit 73150c2)
v1v added a commit to v1v/beats that referenced this pull request Jun 2, 2020
…-stage-level

* upstream/master: (30 commits)
  Add a GRPC listener service for Agent (elastic#18827)
  Disable host.* fields by default for iptables module (elastic#18756)
  [WIP] Clarify capabilities of the Filebeat auditd module (elastic#17068)
  fix: rename file and remove extra separator (elastic#18881)
  ci: enable JJBB (elastic#18812)
  Disable host.* fields by default for Checkpoint module (elastic#18754)
  Disable host.* fields by default for Cisco module (elastic#18753)
  Update latest.yml testing env to 7.7.0 (elastic#18535)
  Upgrade k8s.io/client-go and k8s keystore tests (elastic#18817)
  Add missing Jenkins stages for Auditbeat (elastic#18835)
  [Elastic Log Driver] Create a config shim between libbeat and the user (elastic#18605)
  Use indexers and matchers in config when defaults are enabled (elastic#18818)
  Fix panic on `metricbeat test modules` (elastic#18797)
  [CI] Fix permissions in MacOSX agents (elastic#18847)
  [Ingest Manager] When not port are specified and the https is used fallback to 443 (elastic#18844)
  [Ingest Manager] Fix install service script for windows (elastic#18814)
  [Metricbeat] Fix getting compute instance metadata with partial zone/region config (elastic#18757)
  Improve error messages in s3 input (elastic#18824)
  Add memory metrics into compute googlecloud (elastic#18802)
  include bucket name when logging error (elastic#18679)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Team:Integrations Label for the Integrations team v7.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants