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

Fix parsing of interface options with _ (#4334) #4378

Merged
merged 1 commit into from May 29, 2017

Conversation

Projects
None yet
5 participants
@zecke
Copy link
Contributor

commented May 23, 2017

In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by using a tag in the structure.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2017

Can one of the admins verify this patch?

@zecke

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2017

Sorry heading out for tonight but wanted to share my findings in regard to issue #4334.

  • There are more variables that might be used in the config file (Src_ip, Dst_ip, proc_prefix)
  • To test one could probably easily have a round-trip test? Parse the config and write what was parsed and check there is no difference?
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2017

@zecke Thanks a lot for bringing this up. If you are right, it seems that this could affect quite a few more variables.

@tsg

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2017

Hmm, yeah, looks like this broke with #2928. :(

@zecke Thanks a lot for finding & fixing this. Can you add config tags to the rest of the InterfaceConfig struct? All the structs in config.go are used in configurations, but only that struct is lacking tags.

Our system tests didn't catch these because they always work on reading from files, rather than opening an interface.

Fix parsing of interface options with _ (#4334)
In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by adding tags for all
documented variables.

@zecke zecke force-pushed the zecke:fix-4334 branch from 98b4e09 to 406cea1 May 24, 2017

@zecke

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2017

@tsg Shouldn't bpf_filter work for reading from files too? I have not looked at the test setup but setting a filter that should only match a subset of the packets in a file and verifying the number of imported packets should be doable?

I annotated the struct (and aligned the tags to match the longest one). I have left the last options out as they are not documented in the reference file and I didn't know what is being used (top_speed, topspeed, topSpeed)

@clennpillo

This comment has been minimized.

Copy link

commented May 28, 2017

Thanks for fixing this, this was preventing us from monitoring a busy interface, I guess this PR will be merged soon ?

@tsg

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2017

@zecke Currently it's not applied on reading from file. We could implement it, but then it would probably be a different setting, because this one applies to "interfaces"?

I'm going to merge it for now like this and improve on the system tests if something like this ever surfaces again. I'll add the Changelog while backporting, thanks a lot for the fix!

@tsg tsg merged commit 813466d into elastic:master May 29, 2017

5 checks passed

CLA Commit author has signed the CLA
Details
codecov/patch Coverage not affected when comparing e018722...406cea1
Details
codecov/project 63.17% (+0.01%) compared to e018722
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

tsg added a commit to tsg/beats that referenced this pull request May 29, 2017

Fix parsing of interface options with _ (elastic#4334) (elastic#4378)
In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by adding tags for all
documented variables.
(cherry picked from commit 813466d)

@tsg tsg removed the needs_backport label May 29, 2017

tsg added a commit to tsg/beats that referenced this pull request May 29, 2017

Fix parsing of interface options with _ (elastic#4334) (elastic#4378)
In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by adding tags for all
documented variables.
(cherry picked from commit 813466d)

exekias added a commit that referenced this pull request May 30, 2017

Cherry-pick #4378 to 5.x: Fix parsing of interface options with _ (#4334
) (#4411)

* Fix parsing of interface options with _ (#4334) (#4378)

In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by adding tags for all
documented variables.
(cherry picked from commit 813466d)

zecke added a commit to zecke/beats that referenced this pull request May 31, 2017

WIP/not merge: Introduce extra interface for introspection...
In elastic#4378 we briefly discussed how to test the parsing of config
parameters. The way ucfg works is that the yaml/json is unpacked
into the struct and  with the current approach at least two issues
can occur:

1.) One can have entries in the config file that are not consumed
2.) One can have members in the struct that are not filled

To allow testing proper parsing of a valid configuration file and
help users to show what has been passed I introduce a new config
option.

Instead of forcing this new method for the existing Beater interface,
add a new introspection interface...

TODO: Is this a sound approach? Shall a list of interfaces be returned?
Print them to a file? What about the b.Config? How to add this as a
system test?

TODO: Don't use go-yaml/yaml but the one used by ucfg?
@zecke

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2017

@tsg "interfaces": True, let's not do that then.

In zecke@73e275d I created a new interface (to allow easy porting of the existing beats?!) and played a bit with printing a yaml representation of what has been parsed. It prints something like this...

I think in general it is a nice addition:

  1. It might help users to find typos in their config file (in case something is not picked up?)
  2. We could somehow integrate it into the system tests and just compare text output?

What do you think?

interfaces:
  device: en0
  type: pcap
  file: ""
  withvlans: false
  bpffilter: ""
  snaplen: 65535
  buffersizemb: 0
  topspeed: false
  dumpfile: ""
  oneatatime: false
  loop: 1
flows:
  enabled: false
  timeout: 30s
  period: 10s
protocols:
  amqp: {}
  cassandra: {}
  dns: {}
  http: {}
  icmp: {}
...
runoptions:
  uid: null
  gid: null

@zecke zecke deleted the zecke:fix-4334 branch May 31, 2017

monicasarbu added a commit that referenced this pull request Jun 6, 2017

Automatic merge from 5.4 to 5.x branch (#4449)
* Bump version in the 5.x branch to 5.5.0 (#4007)

* Backport codecov file to 5.x (#4040)

This will make sure builds do not go red on 5.x because of some small diffs in coverage.

* Properly shut down crawler in case one prospector is misconfigured (#4037) (#4048)

If one prospector started to already send data and a second one was misconfigured, the beat paniced during shutdown. This is no prevented by properly shutting down the crawler also on error.

Closes #3917
(cherry picked from commit 95195cc)

* Fix link to the MacOSX SDK tarball (#4120) (#4122)

The original download was temporarily down and then it came back up
with a different sha1. Switching to what seems to be a link closer to the
source.

This will require backporting in all branches that need to be built.
(cherry picked from commit 7d15bf3)

* Deprecate the metrics endpoint in Winlogbeat (#4145)

The metrics endpoint is replaced by the http endpoint for all beats in 6.0. See #3717

* elasticsearch: set _type=doc (#3757) (#4191)

The `_type` field is deprecated per
elastic/elasticsearch#15613
(cherry picked from commit bec7603)

* Reduce the number of notifications from travis CI. (#4210) (#4214)

- Disable PR notifications.
- Send failed build notifications.
- Send an update when build transistions from red -> green.
(cherry picked from commit 1844718)

* Fix MongoDB dbstats fields mapping (#4258)

(cherry picked from commit 6b0b077)

* Deprecate document_type in filebeat 5.5 (#4225)

`_type` is removed in elasticsearch 6.0 and `document_type` is removed in filebeat 6.0. We recommend using `fields` instead.

* Ignore permission errors in Metricbeat’s TestFileSystemList (#3562)

The test can fail if some calls to statfs fail due to permission errors. For example:

`stat("/var/lib/docker/aufs/mnt/50d0d5f599f0f19450e7649f73a0e23da1f172048e555df2b1cb78b3fefa355b", 0x7ffd2e5b8ed0) = -1 EACCES (Permission denied)`

* Miscellaneous test fixes

- Fix and enable the python smoke test for heartbeat
- Remove fmt.Printf from metricbeat ceph module
- Fix Windows path issues in libbeat/paths tests
- Fix ioutil.TempDir usage in Packetbeat tests (it broke windows)

* Using single quotes around Windows paths

The thrift test config used double quotes around Windows path separators and this was interpreted incorrectly in YAML parsing.

* Rename TestBadCondition to TestConditions

This test doesn’t actually test any bad conditions. Plus there is another test in the same directory with the name TestBadCondition.

* Remove OS specific error message check from mockbeat

The error message “no such file or directory” is an OS specific error message. There is a different error message on Windows. Simply checking for “error loading config file” should be sufficient.

* Use shorter filename in Filebeat test for Windows

The test was failing on Windows when `os.rename` failed with `[Error 3] The system cannot find the path specified`. The root cause of the failure was that the path was ~260 characters on Jenkins which is greater than the `MAX_PATH` value in Windows. This PR shortens the test log’s name to resolve the issue.

The other changes to normalize the filepath are nice to have for Windows, but not strictly required.

* Add filesystem name to test error message

Errors that are logged by the system/filesystem test case don’t have enough context to debug them. This adds the filesystem that caused the error to the message.

* Less strict error matching in Winlogbeat config_test

Error string testing is brittle. The PR makes the test less stringent by not checking the full error message that includes the Golang stdlib error.

* Use logp.Beta or logp.Experimental in metricsets

And in system tests, centralize the logic for asserting that there are no ERR or WARN in logs.

Filter out errors about “The service process could not connect to the service controller” that occur when testing on Jenkins where Jenkins itself is running as a service. This confuses the Beat because it thinks that it is running as service, but it’s not.

* Fix Winlogbeat test by checking full hostname (#3942) (#4304)

The `computer_name` field in events is the full hostname, but the `win32api.GetComputerName` was returning the shortened netbios name. So the test fail on machines with longer hostnames.
(cherry picked from commit 5c6e623)

* Clean geoip.paths before using the path (#4306)

Use filepath.Clean on the configured paths to fix any invalid OS path separators.

Skip the geoip test with symlinks on Windows (`os.symlink` isn’t supported on Windows).

* Use .go-version to specify the Go version for all CI builds (#4303) (#4307)

Having a simple file that requires no parsing to retrieve the Go version
provides us a standard portable way to know what Go version to use for builds.
It's basically the least common denominator for builds accross CI systems
(Jenkins, AppVeyor, Travis) and operating systems.

Also changed AppVeyor to invalidate the cached Go version only when the
.go-version file changes instead of when the .appveyor.yml changes.

* Fix testing env in the 5.x branch. (#4412)

It was set on 5.4.0 BC, which got removed in the mean time.

* Cherry-pick #4378 to 5.x: Fix parsing of interface options with _ (#4334) (#4411)

* Fix parsing of interface options with _ (#4334) (#4378)

In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by adding tags for all
documented variables.
(cherry picked from commit 813466d)

* Allow string characters in browser patch version (#4418)

Both for NGINX and Apache logs

(cherry picked from commit a10c1b7)

* Fix type for HAProxy health.last field (#4410) (#4425)

Fixes #4407. Also adds docs for two fields where docs were missing.
(cherry picked from commit a2ea586)

ramon-garcia added a commit to ramon-garcia/beats that referenced this pull request Dec 5, 2017

Fix parsing of interface options with _ (elastic#4334) (elastic#4378)
In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by adding tags for all
documented variables.

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

Fix parsing of interface options with _ (elastic#4334) (elastic#4378)
In commit 5547060 linting issues
were addresses and variables containing _ were renamed. This broke
the config parsing.

In packetbeat this seems to effect the with_vlans, bpf_filter and
the buffer_size_mb options. Correct it by adding tags for all
documented variables.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.