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

Add rpm packaging rebase #10429

Merged
merged 10 commits into from
Feb 1, 2019
Merged

Add rpm packaging rebase #10429

merged 10 commits into from
Feb 1, 2019

Conversation

tsg
Copy link
Contributor

@tsg tsg commented Jan 30, 2019

This takes over the code from #9092 and rebases it on top of master.

This adds support for programmatically reading the list of RPM packages. The previous version was using exec (called the rpm binary), but we'd like to keep Auditbeat exec free, because execs are currently blocked by seccomp, as a security feature.

Using the model from Journalbeat, the new code uses dlopen get the relevant C functions and calls them using CGo. This means that librpm is not a hard dependency, but only for when this functionality is needed.

@tsg tsg requested a review from a team as a code owner January 30, 2019 13:26
@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@tsg tsg requested a review from a team as a code owner January 30, 2019 22:49
@tsg
Copy link
Contributor Author

tsg commented Jan 31, 2019

jenkins, test this plz

// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

// +build linux
Copy link
Member

@andrewkroh andrewkroh Jan 31, 2019

Choose a reason for hiding this comment

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

linux,cgo better reflects the requirements of the file. Likewise anywhere that uses a tag that's meant to be the opposite should be updated (I think that would be // +build !linux !cgo).

You'll want to update anything that depends on this file directly to include the same label. This should allow for CGO_ENABLED=0 go build to still work on Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, should be done with the latest commit. I tested CGO_ENABLED=0 go build on linux.

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.

Code LGTM. There is a doc page that mentions this only works on dpkg type systems. That needs to be updated.

https://www.elastic.co/guide/en/beats/auditbeat/master/auditbeat-dataset-system-package.html#auditbeat-dataset-system-package

@tsg
Copy link
Contributor Author

tsg commented Jan 31, 2019

jenkins, test this plz

Copy link
Contributor

@cwurm cwurm 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 rebasing it, @tsg! Just one test failure on Windows related to build constraints (see comment), and the docs page @andrewkroh mentioned.

LGTM otherwise, if the relevant parts of CI go green.

// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

// +build !linux !cgo
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be // +build !linux,!windows !cgo,!windows, I think. The definition for Package is not available for Windows because package.go is // +build !windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, trying it as an extra // + build !windows line, I think that should work as well.

@cwurm
Copy link
Contributor

cwurm commented Feb 1, 2019

There is a system test in test_metricsets.py that will need its run condition updated:

@unittest.skipIf(sys.platform == "linux2" and platform.linux_distribution()[0] != "debian",

Unfortunately, it's already wrong at the moment (testing for "debian" which does not cover Ubuntu). I'd have fixed it but didn't want to cause a merge conflict here. It's tricky because Python does not have an exact out of the box equivalent to the Go code (which ultimately parses /etc/lsb-release).

Maybe we could do:

@unittest.skipIf(sys.platform == "linux2" and not (os.path.isdir("/var/lib/dpkg") or os.path.isdir("/var/lib/rpm")),
                     "Only implemented for dpkg and rpm")

@tsg
Copy link
Contributor Author

tsg commented Feb 1, 2019

jenkins, test this pls

@tsg
Copy link
Contributor Author

tsg commented Feb 1, 2019

jenkins, test this

@tsg
Copy link
Contributor Author

tsg commented Feb 1, 2019

jenkins, test this just one more time

@tsg tsg added the needs_backport PR is waiting to be backported to other branches. label Feb 1, 2019
@tsg
Copy link
Contributor Author

tsg commented Feb 1, 2019

All green except an unrelated Metricbeat test. I was hoping for an all-green, but I think this is good to merge.

@tsg tsg merged commit 99d09ea into elastic:master Feb 1, 2019
@tsg tsg added v6.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Feb 1, 2019
tsg added a commit to tsg/beats that referenced this pull request Feb 1, 2019
* Add RPM packages support to the package dataset

(cherry picked from commit 99d09ea)
@urso urso added needs_backport PR is waiting to be backported to other branches. and removed needs_backport PR is waiting to be backported to other branches. labels Feb 6, 2019
tsg added a commit to tsg/beats that referenced this pull request Feb 6, 2019
* Add RPM packages support to the package dataset

(cherry picked from commit 99d09ea)
tsg added a commit that referenced this pull request Feb 6, 2019
* Add RPM packages support to the package dataset

(cherry picked from commit 99d09ea)
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.

6 participants