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.

Sign up for GitHub

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

Jump to bottom

Jv ebpf deprecation cleanup #1509

Merged
merged 8 commits into from
Feb 7, 2024
Merged

Jv ebpf deprecation cleanup #1509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bfb44dd
X-Smart-Branch-Parent: master
JoukoVirtanen Jan 19, 2024
4b7c4c0
Changed default collection methods
JoukoVirtanen Jan 19, 2024
e8716d6
Fixed unit tests
JoukoVirtanen Jan 25, 2024
2614602
Removed the hardfail config. It behaves as it is true
JoukoVirtanen Jan 25, 2024
6c10210
Working on updating docs
JoukoVirtanen Feb 3, 2024
c244e8b
Made more updates to documentation
JoukoVirtanen Feb 3, 2024
ef66e06
Moved most of CollectorConfig constructor to InitCollectorConfig. Usi…
JoukoVirtanen Feb 6, 2024
0b7f5d5
Making it so that invalid collection methods are switched to core bpf
JoukoVirtanen Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion collector/collector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ int main(int argc, char** argv) {
CLOG(FATAL) << "Error parsing arguments";
}

CollectorConfig config(args);
CollectorConfig config;
config.InitCollectorConfig(args);

setCoreDumpLimit(config.IsCoreDumpEnabled());

Expand Down
12 changes: 6 additions & 6 deletions collector/lib/CollectorConfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ BoolEnvVar set_enable_core_dump("ENABLE_CORE_DUMP", false);
// If true, add originator process information in NetworkEndpoint
BoolEnvVar set_processes_listening_on_ports("ROX_PROCESSES_LISTENING_ON_PORT", CollectorConfig::kEnableProcessesListeningOnPorts);

BoolEnvVar core_bpf_hardfail("ROX_COLLECTOR_CORE_BPF_HARDFAIL", false);

BoolEnvVar set_import_users("ROX_COLLECTOR_SET_IMPORT_USERS", false);

BoolEnvVar collect_connection_status("ROX_COLLECT_CONNECTION_STATUS", true);
Expand All @@ -53,13 +51,15 @@ constexpr bool CollectorConfig::kEnableProcessesListeningOnPorts;
const UnorderedSet<L4ProtoPortPair> CollectorConfig::kIgnoredL4ProtoPortPairs = {{L4Proto::UDP, 9}};
;

CollectorConfig::CollectorConfig(CollectorArgs* args) {
CollectorConfig::CollectorConfig() {
// Set default configuration values
scrape_interval_ = kScrapeInterval;
turn_off_scrape_ = kTurnOffScrape;
collection_method_ = kCollectionMethod;
}

void CollectorConfig::InitCollectorConfig(CollectorArgs* args) {
enable_processes_listening_on_ports_ = set_processes_listening_on_ports.value();
core_bpf_hardfail_ = core_bpf_hardfail.value();
import_users_ = set_import_users.value();
collect_connection_status_ = collect_connection_status.value();
enable_external_ips_ = enable_external_ips.value();
Expand Down Expand Up @@ -130,8 +130,8 @@ CollectorConfig::CollectorConfig(CollectorArgs* args) {
} else if (cm == "core_bpf") {
collection_method_ = CollectionMethod::CORE_BPF;
} else {
CLOG(WARNING) << "Invalid collection-method (" << cm << "), using eBPF";
collection_method_ = CollectionMethod::EBPF;
CLOG(WARNING) << "Invalid collection-method (" << cm << "), using CO-RE BPF";
collection_method_ = CollectionMethod::CORE_BPF;
}
}

Expand Down
8 changes: 3 additions & 5 deletions collector/lib/CollectorConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class CollectorConfig {
public:
static constexpr bool kTurnOffScrape = false;
static constexpr int kScrapeInterval = 30;
static constexpr CollectionMethod kCollectionMethod = CollectionMethod::EBPF;
static constexpr CollectionMethod kCollectionMethod = CollectionMethod::CORE_BPF;
static constexpr const char* kSyscalls[] = {
"accept",
"accept4",
Expand Down Expand Up @@ -48,8 +48,8 @@ class CollectorConfig {
static const UnorderedSet<L4ProtoPortPair> kIgnoredL4ProtoPortPairs;
static constexpr bool kEnableProcessesListeningOnPorts = true;

CollectorConfig() = delete;
CollectorConfig(CollectorArgs* collectorArgs);
CollectorConfig();
void InitCollectorConfig(CollectorArgs* collectorArgs);

std::string asString() const;

Expand All @@ -69,7 +69,6 @@ class CollectorConfig {
bool IsCoreDumpEnabled() const;
Json::Value TLSConfiguration() const { return tls_config_; }
bool IsProcessesListeningOnPortsEnabled() const { return enable_processes_listening_on_ports_; }
bool CoReBPFHardfail() const { return core_bpf_hardfail_; }
bool ImportUsers() const { return import_users_; }
bool CollectConnectionStatus() const { return collect_connection_status_; }
bool EnableExternalIPs() const { return enable_external_ips_; }
Expand Down Expand Up @@ -99,7 +98,6 @@ class CollectorConfig {
bool enable_afterglow_ = true;
bool enable_core_dump_ = false;
bool enable_processes_listening_on_ports_;
bool core_bpf_hardfail_;
bool import_users_;
bool collect_connection_status_;
bool enable_external_ips_;
Expand Down
7 changes: 1 addition & 6 deletions collector/lib/KernelDriver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,7 @@ class KernelDriverCOREEBPF : public IKernelDriver {
config.GetSinspCpuPerBuffer(),
true, ppm_sc);
} catch (const sinsp_exception& ex) {
if (config.CoReBPFHardfail()) {
throw ex;
} else {
CLOG(WARNING) << ex.what();
return false;
}
throw ex;
}

return true;
Expand Down
15 changes: 4 additions & 11 deletions collector/test/HostHeuristicsTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,10 @@ class MockHostInfoHeuristics : public HostInfo {
MOCK_METHOD0(GetDistro, std::string&());
};

// Note that in every test below, ProcessHostHeuristics will be called first
// with creation of a config mock, which is somewhat annoying, but doesn't
// cause any serious issues.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This did create a serious issue when the default collection method was changed to CORE_BPF. This is because ProcessHostHeuristics calls Process, which runs the following when the collection method is CORE_BPF

      if (!host.HasBPFRingBufferSupport()) {
        CLOG(FATAL) << "Missing RingBuffer support, core_bpf is not available. "
                    << "HINT: Change collection method to eBPF with collector.collectionMethod=EBPF.";
      }

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not possible to mock out Process as well? I'm not sure it's the best solution to go through config setup directly, because it makes unit tests less "unit", in the sense that they now less isolated and a bit dependent on the outside world (in the form of env vars).

Copy link
Contributor Author

@JoukoVirtanen JoukoVirtanen Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I ended up solving this by moving most of the CollectorConfig constructor into an InitCollectorConfig function, including ProcessHostHeuristics, which called Process and caused the fatal error. I feel like the constructor for CollectorConfig was doing too much and splitting it into two is an improvement.

I considered alternatives such as moving ProcessHostHeuristics into either the HostHeuristics or CollectorConfig classes. Neither option seemed very good. I also considered creating a new class for ProcessHostHeuristics.

class MockCollectorConfig : public CollectorConfig {
public:
MockCollectorConfig(CollectorArgs* collectorArgs)
: CollectorConfig(collectorArgs){};

MOCK_CONST_METHOD0(UseEbpf, bool());
MockCollectorConfig()
: CollectorConfig(){};

void SetCollectionMethod(CollectionMethod cm) {
if (host_config_.HasCollectionMethod()) {
Expand All @@ -74,8 +69,7 @@ TEST(HostHeuristicsTest, TestS390XRHEL84) {
MockS390xHeuristics s390xHeuristics;
MockHostInfoHeuristics host;
KernelVersion version = KernelVersion("4.18.0-305.88.1.el8_4.s390x", "", "s390x");
CollectorArgs* args = CollectorArgs::getInstance();
MockCollectorConfig config(args);
JoukoVirtanen marked this conversation as resolved.
Show resolved Hide resolved
MockCollectorConfig config;
HostConfig hconfig;

config.SetCollectionMethod(CollectionMethod::EBPF);
Expand All @@ -91,8 +85,7 @@ TEST(HostHeuristicsTest, TestARM64Heuristic) {
MockARM64Heuristics heuristic;
MockHostInfoHeuristics host;
KernelVersion version = KernelVersion("6.5.5-200.fc38.aarch64", "", "aarch64");
CollectorArgs* args = CollectorArgs::getInstance();
MockCollectorConfig config(args);
MockCollectorConfig config;
HostConfig hconfig;

config.SetCollectionMethod(CollectionMethod::EBPF);
Expand Down
10 changes: 5 additions & 5 deletions collector/test/NetworkStatusNotifierTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "internalapi/sensor/network_connection_iservice.grpc.pb.h"

#include "CollectorArgs.h"
#include "CollectorConfig.h"
#include "DuplexGRPC.h"
#include "NetworkStatusNotifier.h"
Expand Down Expand Up @@ -66,7 +67,7 @@ class Semaphore {

class MockCollectorConfig : public collector::CollectorConfig {
public:
MockCollectorConfig() : collector::CollectorConfig(0) {}
MockCollectorConfig() : collector::CollectorConfig() {}

void DisableAfterglow() {
enable_afterglow_ = false;
Expand Down Expand Up @@ -181,7 +182,7 @@ class NetworkConnectionInfoMessageParser {
/* Simple validation that the service starts and sends at least one event */
TEST(NetworkStatusNotifier, SimpleStartStop) {
bool running = true;
CollectorConfig config_(0);
CollectorConfig config_;
std::shared_ptr<MockConnScraper> conn_scraper = std::make_shared<MockConnScraper>();
auto conn_tracker = std::make_shared<ConnectionTracker>();
auto comm = std::make_shared<MockNetworkConnectionInfoServiceComm>();
Expand Down Expand Up @@ -242,6 +243,7 @@ TEST(NetworkStatusNotifier, SimpleStartStop) {
TEST(NetworkStatusNotifier, UpdateIPnoAfterglow) {
bool running = true;
MockCollectorConfig config;
config.DisableAfterglow();
std::shared_ptr<MockConnScraper> conn_scraper = std::make_shared<MockConnScraper>();
auto conn_tracker = std::make_shared<ConnectionTracker>();
auto comm = std::make_shared<MockNetworkConnectionInfoServiceComm>();
Expand All @@ -255,8 +257,6 @@ TEST(NetworkStatusNotifier, UpdateIPnoAfterglow) {
// the same server connection normalized and grouped in a known subnet
Connection conn3("containerId", Endpoint(Address(), 1024), Endpoint(IPNet(Address(139, 45, 0, 0), 16), 0), L4Proto::TCP, true);

config.DisableAfterglow();

// the connection is always ready
EXPECT_CALL(*comm, WaitForConnectionReady).WillRepeatedly(Return(true));
// gRPC shuts down the loop, so we will want writer->Sleep to return with false
Expand Down Expand Up @@ -336,4 +336,4 @@ TEST(NetworkStatusNotifier, UpdateIPnoAfterglow) {

} // namespace

} // namespace collector
} // namespace collector
14 changes: 7 additions & 7 deletions docs/design-overview.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@

## Data gathering methods
### Kernel drivers
Collector leverages eBPF probes and kernel modules for gathering data directly
Collector leverages a CO-RE eBPF probe and eBPF probes for gathering data directly
from the kernel.

#### Driver candidates
When collector starts up, it will automatically try to download a driver
compatible with the running kernel. For most distributions this will be
determined with the `uname -r` command, but some special cases exist. In such
cases, collector will create a set of potential candidates and try to find them
sequentially, first looking for them in the filesystem and then trying to
download them.
When collector starts up, if the COLLECTION_METHOD is eBPF, it will
automatically try to download a driver compatible with the running kernel.
For most distributions this will be determined with the `uname -r` command,
but some special cases exist. In such cases, collector will create a set of
potential candidates and try to find them sequentially, first looking for
them in the filesystem and then trying to download them.

In cases where the auto-generated list of candidates are failing to create a
working option, the `KERNEL_CANDIDATES` environment variable can be used to
Expand Down
2 changes: 1 addition & 1 deletion docs/how-to-start.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ services:
environment:
- GRPC_SERVER=localhost:9999
- COLLECTOR_CONFIG={"logLevel":"debug","turnOffScrape":true,"scrapeInterval":2}
- COLLECTION_METHOD=ebpf
- COLLECTION_METHOD=core-bpf
- MODULE_DOWNLOAD_BASE_URL=https://collector-modules.stackrox.io/612dd2ee06b660e728292de9393e18c81a88f347ec52a39207c5166b5302b656
- MODULE_VERSION=b6745d795b8497aaf387843dc8aa07463c944d3ad67288389b754daaebea4b62
- COLLECTOR_HOST_ROOT=/host
Expand Down
2 changes: 1 addition & 1 deletion docs/references.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ seconds. The default value is 30 seconds.
### Other arguments

* `--collection-method`: Which technology to use for data gathering. Either
"ebpf" or "kernel_module".
"ebpf" or "core-bpf".

* `--grpc-server`: GRPC server endpoint for Collector to communicate, in the
form "host:port".
Expand Down
9 changes: 4 additions & 5 deletions integration-tests/suites/common/collector_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,10 @@ func NewCollectorManager(e Executor, name string) *CollectorManager {
}

env := map[string]string{
"GRPC_SERVER": "localhost:9999",
"COLLECTION_METHOD": collectionMethod,
"COLLECTOR_PRE_ARGUMENTS": collectorOptions.PreArguments,
"ENABLE_CORE_DUMP": "false",
"ROX_COLLECTOR_CORE_BPF_HARDFAIL": "true",
"GRPC_SERVER": "localhost:9999",
"COLLECTION_METHOD": collectionMethod,
"COLLECTOR_PRE_ARGUMENTS": collectorOptions.PreArguments,
"ENABLE_CORE_DUMP": "false",
}

if !collectorOptions.Offline {
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/suites/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const (

var (
qa_tag = ReadEnvVar(envQATag)
collection_method = ReadEnvVarWithDefault(envCollectionMethod, CollectionMethodEBPF)
collection_method = ReadEnvVarWithDefault(envCollectionMethod, CollectionMethodCoreBPF)
stop_timeout = ReadEnvVarWithDefault(envStopTimeout, defaultStopTimeoutSeconds)

image_store *ImageStore
Expand Down
Loading