ROX-24945: adds rate limiting of network events per container #1945

Stringy · 2024-11-07T09:20:19Z

Description

This adds new rate limiting of network connections per container, on top of the existing afterglow and batching methods in order to provide a safety mechanism (upper limit) to the number of reported network connections for a given workload. When External IPs is enabled, the volume of connections will naturally be much higher, so this limit is primarily in service to controlling that volume.

Some caveats and decisions:

The rate limiting is implemented at the last possible moment, as the batch message is being constructed. This is means we do not rate limit connections that would otherwise be dropped by afterglow. It also means that we're rate limiting the delta between the old state and the new state.
Close events (those with a close timestamp) are not rate limited. Otherwise we could end up with a situation where a connection exists in Sensor's model, but its close event is never sent so that connection will exist forever.
The time window for rate limiting is pegged to the scrape interval, to provide a logical and well-understood time frame for scale. This means we can also think of this rate limit as a constraint on the maximum size of batch we'll ever send to Sensor.
The rate limiting is enabled regardless of external IPs. I think this makes the overall behaviour of network event reporting much clearer and more predictable, but I am not married to this notion and should we want to retain old behaviour for the vast majority of cases, we can change this.

Depends on: stackrox/stackrox#13228

Checklist

Investigated and inspected CI test results
Updated documentation accordingly

Automated testing

Added unit tests
Added integration tests
Added regression tests

If any of these don't apply, please comment below.

Testing Performed

TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.

For more details, ref the Confluence page about this section.

Molter73 · 2024-11-07T09:40:36Z

collector/lib/CollectorConfig.h

@@ -107,6 +107,16 @@ class CollectorConfig {
    return enable_external_ips_;
  }

+  int64_t PerContainerRateLimit() const {
+    std::shared_lock<std::shared_mutex> lock(mutex_);


[nitpick] There's a helper method that should make it a bit clear this is a read lock.

Suggested change

std::shared_lock<std::shared_mutex> lock(mutex_);

auto lock = ReadLock();

Molter73 · 2024-11-07T09:41:59Z

collector/lib/CollectorConfig.h

+    if (runtime_config_.has_value()) {
+      return runtime_config_.value()
+          .networking()
+          .per_container_rate_limit();
+    }
+    return per_container_rate_limit_;


This handling is getting out of hand, we need to make it so all this configuration is always stored in the runtime_config_ object and, if we are not using it, just overwrite the value in there ourselves.

Molter73 · 2024-11-07T09:42:43Z

collector/lib/NetworkSignalHandler.cpp

@@ -1,10 +1,13 @@
 #include "NetworkSignalHandler.h"

+#include <chrono>


Why do we need chrono now?

We don't - removed

Molter73 · 2024-11-07T09:43:37Z

collector/lib/NetworkSignalHandler.cpp

@@ -147,6 +150,7 @@ SignalHandler::Result NetworkSignalHandler::HandleSignal(sinsp_evt* evt) {
    return SignalHandler::IGNORED;
  }

+  CLOG(DEBUG) << "Live event: " << result.value();


Last time I tried adding something like this it completely destroyed performance in debug mode, have you tried it?

Yeah this was just for manual verification - removed now.

Molter73 · 2024-11-07T09:44:53Z

collector/lib/NetworkStatusNotifier.cpp

@@ -1,10 +1,13 @@
 #include "NetworkStatusNotifier.h"

+#include <sstream>


Are we sure we need sstream? I would assume Logging.h is enough.

Molter73 · 2024-11-07T09:52:54Z

collector/lib/NetworkStatusNotifier.h

@@ -31,7 +32,8 @@ class NetworkStatusNotifier : protected ProtoAllocator<sensor::NetworkConnection
        config_(config),
        comm_(comm),
        connections_total_reporter_(connections_total_reporter),
-        connections_rate_reporter_(connections_rate_reporter) {
+        connections_rate_reporter_(connections_rate_reporter),
+        connections_rate_limiter_(RateLimitCache(config.PerContainerRateLimit(), config.PerContainerRateLimit(), config.ScrapeInterval())) {


Why are we constructing a RateLimitCache and then copy constructing another one? Couldn't we just...

Suggested change

connections_rate_limiter_(RateLimitCache(config.PerContainerRateLimit(), config.PerContainerRateLimit(), config.ScrapeInterval())) {

connections_rate_limiter_(config.PerContainerRateLimit(), config.PerContainerRateLimit(), config.ScrapeInterval()) {

This was due to my ignorance about how these constructors worked - my bad! I've removed the connections_rate_limiter_ member now anyway so this problem has also been removed.

Molter73 · 2024-11-07T09:54:18Z

integration-tests/suites/network_stress.go

+	s.StartContainerStats()
+	image_store := config.Images()
+
+	berserker_img := image_store.QaImageByKey("performance-berserker")


Molter73 · 2024-11-07T09:56:44Z

integration-tests/suites/network_stress.go

+	berserker_img := image_store.QaImageByKey("performance-berserker")
+
+	images := []string{
+		berserker_img,
+	}
+
+	for _, image := range images {
+		err := s.executor.PullImage(image)
+		s.Require().NoError(err)
+	}


I mean...

Suggested change

berserker_img := image_store.QaImageByKey("performance-berserker")

images := []string{

berserker_img,

}

for _, image := range images {

err := s.executor.PullImage(image)

s.Require().NoError(err)

}

berserker_img := image_store.QaImageByKey("performance-berserker")

err := s.Executor().PullImage(berserker_img)

s.Require().NoError(err)

Unless you are actively planning to add more images to the test.

I've (temporarily) removed this test suite now, in favour of unit tests. I'll work on it on the side and add it back in in another PR.

Molter73 · 2024-11-07T09:58:45Z

integration-tests/suites/network_stress.go

+	berserkerID, err := s.Executor().StartContainer(config.ContainerStartConfig{
+		Name:  "berserker-server",
+		Image: berserker_img,
+		Env: map[string]string{
+			"BERSERKER__WORKLOAD__NCONNECTIONS": "100",
+			"RUST_BACKTRACE":                    "full",
+		},
+		Command:     []string{"/etc/berserker/network/server.toml"},
+		NetworkMode: "host",
+		Privileged:  true,
+	})
+	s.Require().NoError(err)
+	s.serverID = berserkerID


[nitpick]

Suggested change

berserkerID, err := s.Executor().StartContainer(config.ContainerStartConfig{

Name: "berserker-server",

Image: berserker_img,

Env: map[string]string{

"BERSERKER__WORKLOAD__NCONNECTIONS": "100",

"RUST_BACKTRACE": "full",

},

Command: []string{"/etc/berserker/network/server.toml"},

NetworkMode: "host",

Privileged: true,

})

s.Require().NoError(err)

s.serverID = berserkerID

s.serverID, err = s.Executor().StartContainer(config.ContainerStartConfig{

Name: "berserker-server",

Image: berserker_img,

Env: map[string]string{

"BERSERKER__WORKLOAD__NCONNECTIONS": "100",

"RUST_BACKTRACE": "full",

},

Command: []string{"/etc/berserker/network/server.toml"},

NetworkMode: "host",

Privileged: true,

})

s.Require().NoError(err)

Molter73 · 2024-11-07T09:59:15Z

integration-tests/suites/network_stress.go

+	berserkerID, err = s.Executor().StartContainer(config.ContainerStartConfig{
+		Name:  "berserker-client",
+		Image: berserker_img,
+		Env: map[string]string{
+			"BERSERKER__WORKLOAD__NCONNECTIONS": "100",
+			"RUST_BACKTRACE":                    "full",
+		},
+		Command:     []string{"/etc/berserker/network/client.toml"},
+		NetworkMode: "host",
+		Privileged:  true,
+	})
+	s.Require().NoError(err)
+	s.clientID = berserkerID
+}


[nitpick]

Suggested change

berserkerID, err = s.Executor().StartContainer(config.ContainerStartConfig{

Name: "berserker-client",

Image: berserker_img,

Env: map[string]string{

"BERSERKER__WORKLOAD__NCONNECTIONS": "100",

"RUST_BACKTRACE": "full",

},

Command: []string{"/etc/berserker/network/client.toml"},

NetworkMode: "host",

Privileged: true,

})

s.Require().NoError(err)

s.clientID = berserkerID

}

s.clientID, err = s.Executor().StartContainer(config.ContainerStartConfig{

Name: "berserker-client",

Image: berserker_img,

Env: map[string]string{

"BERSERKER__WORKLOAD__NCONNECTIONS": "100",

"RUST_BACKTRACE": "full",

},

Command: []string{"/etc/berserker/network/client.toml"},

NetworkMode: "host",

Privileged: true,

})

s.Require().NoError(err)

}

Molter73

LGTM! Left a few comments but nothing major.

Molter73 · 2024-11-08T14:41:11Z

collector/lib/NetworkSignalHandler.cpp

@@ -5,6 +5,7 @@
 #include <libsinsp/sinsp.h>

 #include "EventMap.h"
+#include "Logging.h"


Think we can now remove this include as well.

Molter73 · 2024-11-08T14:42:16Z

collector/lib/NetworkStatusNotifier.h

@@ -73,6 +78,7 @@ class NetworkStatusNotifier : protected ProtoAllocator<sensor::NetworkConnection
  std::shared_ptr<CollectorConnectionStats<float>> connections_rate_reporter_;
  std::chrono::steady_clock::time_point connections_last_report_time_;     // time delta between the current reporting and the previous (rate computation)
  std::optional<ConnectionTracker::Stats> connections_rate_counter_last_;  // previous counter values (rate computation)
+                                                                           //


Suggested change

//

Molter73 · 2024-11-08T14:52:29Z

collector/test/NetworkStatusNotifierTest.cpp

+  Connection conn1("containerId", Endpoint(Address(10, 0, 1, 32), 1024), Endpoint(Address(192, 168, 0, 1), 1000), L4Proto::TCP, true);
+  Connection conn2("containerId", Endpoint(Address(10, 0, 1, 32), 1024), Endpoint(Address(192, 168, 0, 2), 1001), L4Proto::TCP, true);
+  Connection conn3("containerId", Endpoint(Address(10, 0, 1, 32), 1024), Endpoint(Address(192, 168, 0, 3), 1002), L4Proto::TCP, true);
+  Connection conn4("containerId", Endpoint(Address(10, 0, 1, 32), 1024), Endpoint(Address(192, 168, 0, 4), 1003), L4Proto::TCP, true);


Could we group these in a vector or array?

I've tried this out but I think I prefer the clarity of statically defining the connections and the deltas - manipulating vectors hides a bit of the intention behind each one. To make it all a bit clearer, though, I've statically define active and closed statuses so there's no more duplication in the deltas

Molter73 · 2024-11-08T14:53:02Z

collector/test/NetworkStatusNotifierTest.cpp

+  Connection otherConn1("containerIdOther", Endpoint(Address(10, 10, 1, 32), 1024), Endpoint(Address(192, 168, 0, 1), 1000), L4Proto::TCP, true);
+  Connection otherConn2("containerIdOther", Endpoint(Address(10, 10, 1, 32), 1024), Endpoint(Address(192, 168, 0, 2), 1001), L4Proto::TCP, true);


Same for these two to be grouped in a vector or array.

Molter73 · 2024-11-08T14:54:13Z

collector/test/NetworkStatusNotifierTest.cpp

+  ConnMap deltaSingle = {
+      {conn1, ConnStatus(1234, true)},
+      {conn2, ConnStatus(1234, true)},
+      {conn3, ConnStatus(1234, true)},
+      {conn4, ConnStatus(1234, true)},
+  };


With the connections in a container, this can be generated in a for loop:

Suggested change

ConnMap deltaSingle = {

{conn1, ConnStatus(1234, true)},

{conn2, ConnStatus(1234, true)},

{conn3, ConnStatus(1234, true)},

{conn4, ConnStatus(1234, true)},

};

ConnMap deltaSingle{};

for {const auto& conn : connections) {

deltaSingle.emplace(conn, ConnStatus(1234, true));

};

There's probably something similar you can do to deltaDuo and deltaClose.

Molter73 · 2024-11-08T15:01:55Z

collector/test/NetworkStatusNotifierTest.cpp

+  google::protobuf::RepeatedPtrField<sensor::NetworkConnection>
+      updatesSingle,
+      updatesDuo,
+      updatesClose;


[nitpick] I had a bit of problem understanding these are 3 separate variables of type google::protobuf::RepeatedPtrField<sensor::NetworkConnection>, probably a skill issue, so feel free to ignore this, but you could define a shorter name for the type and declare all three variables separately:

Suggested change

google::protobuf::RepeatedPtrField<sensor::NetworkConnection>

updatesSingle,

updatesDuo,

updatesClose;

using UpdatedConnections = google::protobuf::RepeatedPtrField<sensor::NetworkConnection>;

UpdatedConnections updatesSingle;

UpdatedConnections updatesDuo;

UpdatedConnections updatesClose;

Or you could use an array

Suggested change

google::protobuf::RepeatedPtrField<sensor::NetworkConnection>

updatesSingle,

updatesDuo,

updatesClose;

using UpdatedConnections = google::protobuf::RepeatedPtrField<sensor::NetworkConnection>;

std::array<UpdatedConnections, 3> updatedConnections;

collector/lib/CollectorConfig.h

collector/lib/RateLimit.cpp

collector/lib/NetworkStatusNotifier.cpp

ovalenti

Thanks for the stats counter. Overall, this looks good !

I tend to agree with @Molter73 on the fact that it would be easier to make sure that the runtime config always contains the value to use (even if it requires us to write the default values there).

collector/test/NetworkStatusNotifierTest.cpp

collector/proto/third_party/stackrox

Stringy · 2024-11-18T14:03:57Z

it would be easier to make sure that the runtime config always contains the value to use (even if it requires us to write the default values there).

Yes, I agree, though I don't think we can define the defaults in the proto itself (due to limitations with protobufs) so I think we'll need to enforce them somewhere. It might make sense to encapsulate that in Collector, but outside of the existing CollectorConfig class. I'll look into it in a follow up PR.

collector/lib/NetworkStatusNotifier.cpp

JoukoVirtanen

LGTM!

Stringy requested a review from a team as a code owner November 7, 2024 09:20

Molter73 reviewed Nov 7, 2024

View reviewed changes

Stringy added 6 commits November 8, 2024 10:29

ROX-24945: adds initial safety rate limit for connections

976758f

Adds runtime configuration support to rate limiting

49ee5e7

Dont forget to actually rate limit

91e9164

Adds unit test for network rate limiting

6aab57b

Removes network stress integration test

de37dd6

Use ReadLock

0493951

Stringy force-pushed the giles/ROX-24945-connections-release-valve branch from 8a18ea2 to 0493951 Compare November 8, 2024 10:36

Stringy requested a review from Molter73 November 8, 2024 10:40

Add CountLimiter for simpler rate limiting

1ddc8aa

Stringy requested a review from ovalenti November 8, 2024 12:49

Molter73 approved these changes Nov 8, 2024

View reviewed changes

Fixes following PR review

cb24a4d

Stringy commented Nov 11, 2024

View reviewed changes

collector/lib/CollectorConfig.h Show resolved Hide resolved

JoukoVirtanen reviewed Nov 12, 2024

View reviewed changes

collector/lib/RateLimit.cpp Outdated Show resolved Hide resolved

Use try_emplace from C++17 instead of emplace

30ffe3e

ovalenti reviewed Nov 12, 2024

View reviewed changes

collector/lib/NetworkStatusNotifier.cpp Outdated Show resolved Hide resolved

Adds connection rate limiting stats

c51915d

Stringy requested review from ovalenti and JoukoVirtanen November 14, 2024 08:58

ovalenti approved these changes Nov 18, 2024

View reviewed changes

collector/test/NetworkStatusNotifierTest.cpp Outdated Show resolved Hide resolved

ovalenti reviewed Nov 18, 2024

View reviewed changes

collector/proto/third_party/stackrox Outdated Show resolved Hide resolved

Stringy added 2 commits November 18, 2024 14:05

Remove left-over scrape interval helper

aa31b46

Update stackrox submodule to include finalised runtime config protos

6775a70

JoukoVirtanen reviewed Nov 18, 2024

View reviewed changes

collector/lib/NetworkStatusNotifier.cpp Outdated Show resolved Hide resolved

JoukoVirtanen approved these changes Nov 18, 2024

View reviewed changes

auto -> int64_t for clarity

0958c7e

Stringy merged commit db929ab into master Nov 19, 2024
50 of 53 checks passed

Stringy deleted the giles/ROX-24945-connections-release-valve branch November 19, 2024 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-24945: adds rate limiting of network events per container #1945

ROX-24945: adds rate limiting of network events per container #1945

Stringy commented Nov 7, 2024 •

edited

Loading

Molter73 Nov 7, 2024

Stringy Nov 8, 2024

Molter73 Nov 7, 2024

Molter73 Nov 7, 2024

Stringy Nov 8, 2024

Molter73 Nov 7, 2024

Stringy Nov 8, 2024

Molter73 Nov 7, 2024

Stringy Nov 8, 2024

Molter73 Nov 7, 2024

Stringy Nov 8, 2024

Molter73 Nov 7, 2024

Molter73 Nov 7, 2024

Stringy Nov 8, 2024

Molter73 Nov 7, 2024

Molter73 Nov 7, 2024

Molter73 left a comment

Molter73 Nov 8, 2024

Molter73 Nov 8, 2024

Molter73 Nov 8, 2024

Stringy Nov 11, 2024

Molter73 Nov 8, 2024

Molter73 Nov 8, 2024 •

edited

Loading

Molter73 Nov 8, 2024

Molter73 Nov 8, 2024

Molter73 Nov 8, 2024 •

edited

Loading

ovalenti left a comment

Stringy commented Nov 18, 2024

JoukoVirtanen left a comment

	std::shared_lock<std::shared_mutex> lock(mutex_);
	auto lock = ReadLock();

		@@ -1,10 +1,13 @@
		#include "NetworkSignalHandler.h"

		#include <chrono>

		@@ -1,10 +1,13 @@
		#include "NetworkStatusNotifier.h"

		#include <sstream>

	connections_rate_limiter_(RateLimitCache(config.PerContainerRateLimit(), config.PerContainerRateLimit(), config.ScrapeInterval())) {
	connections_rate_limiter_(config.PerContainerRateLimit(), config.PerContainerRateLimit(), config.ScrapeInterval()) {

		Connection otherConn1("containerIdOther", Endpoint(Address(10, 10, 1, 32), 1024), Endpoint(Address(192, 168, 0, 1), 1000), L4Proto::TCP, true);
		Connection otherConn2("containerIdOther", Endpoint(Address(10, 10, 1, 32), 1024), Endpoint(Address(192, 168, 0, 2), 1001), L4Proto::TCP, true);

ROX-24945: adds rate limiting of network events per container #1945

ROX-24945: adds rate limiting of network events per container #1945

Conversation

Stringy commented Nov 7, 2024 • edited Loading

Description

Checklist

Testing Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Molter73 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Molter73 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Molter73 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

ovalenti left a comment

Choose a reason for hiding this comment

Stringy commented Nov 18, 2024

JoukoVirtanen left a comment

Choose a reason for hiding this comment

Stringy commented Nov 7, 2024 •

edited

Loading

Molter73 Nov 8, 2024 •

edited

Loading

Molter73 Nov 8, 2024 •

edited

Loading