ROX-29500: Use stackrox protobufs instead of types.NetworkInfo #2189

JoukoVirtanen · 2025-06-23T01:14:04Z

Description

Currently the integration tests use types.NetworkInfo which largely duplicates sensor.NetworkConnection from the stackrox/stackrox protobuf definitions. types.NetworkInfo is unnecessary duplication that introduces complexity and overhead. There is currently a conversion function from sensor.NetworkConnection to types.NetworkInfo. There is loss of information in this step as types.NetworkInfo does not perfectly duplicate sensor.NetworkConnection. Another disadvantage of the current situation is that if a change is made to sensor.NetworkConnection, that change will need to be made manually to types.NetworkInfo. If that change is not made, that could lead to problems.

This PR gets rid of type.NetworInfo and replaces it with sensor.NetworkConnection.

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.

This PR just makes changes to the integration tests.

Testing Performed

CI is sufficient

codecov-commenter · 2025-06-23T01:22:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.83%. Comparing base (dd36a83) to head (66f1706).
Report is 17 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2189   +/-   ##
=======================================
  Coverage   28.83%   28.83%           
=======================================
  Files          96       96           
  Lines        5799     5799           
  Branches     2551     2551           
=======================================
  Hits         1672     1672           
  Misses       3408     3408           
  Partials      719      719

Flag	Coverage Δ
collector-unit-tests	`28.83% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sourcery-ai

Hey @JoukoVirtanen - I've reviewed your changes - here's some feedback:

Consider replacing the custom CompareBytes implementation with bytes.Compare for simplicity and performance.
Your Equal and LessNetworkConnection functions ignore the new Protocol field; include Protocol in the equality and ordering logic to avoid silently dropping mismatches.
The stringToIPNetworkBytes helper hardcodes a /32 mask—if you intend to support arbitrary CIDRs, parse the ipNetwork string into a net.IPNet and extract its mask length instead of always appending 32.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- Consider replacing the custom CompareBytes implementation with bytes.Compare for simplicity and performance.
- Your Equal and LessNetworkConnection functions ignore the new Protocol field; include Protocol in the equality and ordering logic to avoid silently dropping mismatches.
- The stringToIPNetworkBytes helper hardcodes a /32 mask—if you intend to support arbitrary CIDRs, parse the ipNetwork string into a net.IPNet and extract its mask length instead of always appending 32.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

integration-tests/pkg/types/network.go

JoukoVirtanen · 2025-06-28T03:28:48Z

integration-tests/pkg/types/network.go

+	return addr1.GetPort() < addr2.GetPort()
+}
+
+func LessNetworkConnection(conn1 *sensorAPI.NetworkConnection, conn2 *sensorAPI.NetworkConnection) bool {


I could do something like convert the connections to a string using the generated String method and then compare them, but I think that might fail in some cases. Two connections that should be identical might result in two different strings. Two identical sets of connections could be ordered differently if they were converted to strings and compared.

sourcery-ai

Hey @JoukoVirtanen - I've reviewed your changes and they look great!

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

integration-tests/suites/base.go

erthalion · 2025-07-01T13:18:24Z

integration-tests/integration_test.go

-					Role:           "ROLE_SERVER",
-					SocketFamily:   "SOCKET_FAMILY_UNKNOWN",
-					CloseTimestamp: types.NilTimestamp,
+					LocalAddress:   types.CreateNetworkAddress("", "", 40),


I think it's worth it here and in other similar cases to introduce a separate variable keeping the client port. This will explain why there is 40 here and few lines above.

erthalion · 2025-07-01T13:20:58Z

integration-tests/integration_test.go

-					SocketFamily:   "SOCKET_FAMILY_UNKNOWN",
-					CloseTimestamp: types.NilTimestamp,
+					LocalAddress:   types.CreateNetworkAddress("", "", 0),
+					RemoteAddress:  types.CreateNetworkAddress("", "", 40),


From what I understand the original implementation was replacing SERVER_IP with the real IP address. I see that it was done purely for testing, but maybe it makes sense to set the RemoteAddress here correctly as well?

The server IP is unknown at this point, and SERVER_IP can't be passed to CreateNetworkAddress. Instead RemoteAddress is set in the test suite. The value for the port is set here, but the value for the AddressData is overwritten in the test suite.

I still find it could be better. Even if the RemoteAddress set somewhere later, it makes sense to have all the information about the test right here where it's defined.

erthalion · 2025-07-01T13:26:03Z

integration-tests/pkg/types/network.go

-		return peerId.String()
+// The EqualVT method for NetworkAddress returns false if both of them are nil. That is not what
+// we want, so replace nil addr with a default NetworkAddress.
+func adjustNetworkAddressForComparison(addr *sensorAPI.NetworkAddress) *sensorAPI.NetworkAddress {


This feels error prone, since the data we're testing is going to be modified. I would prefer to extend the comparison function instead.

I have removed this function. I switched from using sensor.NeteworkConnection.EqualVT to proto.Equal and the later handles comparison between nils better so that this function is no longer needed. I keep a simplified version of adjustNetworkConnectionForComparison, but it takes in a NetworkConnection by values and returns a NetworkConnection.

There is still similar adjustNetworkConnectionForComparison, which causes the same question.

When you wrote that comment I was passing connections by value to adjustNetworkConnectionForComparison and it was also returning NetworkConnection structs by value. @Stringy asked to change it to use pointers instead and I did so. I moved adjustNetworkConnectionForComparison to inside EqualNetworkConnection, to make it safer. The thinking there was that it would make it less likely to be used incorrectly in the future. EqualNetworkConnection takes in connections via value, so it should be safe.

I am now using CloneVT() before passing the connections to adjustNetworkConnectionForComparison.

erthalion · 2025-07-01T13:29:09Z

integration-tests/pkg/types/network.go

-func (n *NetworkInfo) IsActive() bool {
-	// no close timestamp means the connection is open, and active
-	return n.CloseTimestamp == NilTimestamp
+func CompareBytes(b1 []byte, b2 []byte) int {


How is this different from bytes.Compare in https://pkg.go.dev/bytes?

It's not. I now use bytes.Compare.

…reference

integration-tests/pkg/types/network.go

Stringy · 2025-07-14T13:39:07Z

integration-tests/pkg/types/network.go

+func adjustNetworkConnectionForComparison(conn sensorAPI.NetworkConnection) sensorAPI.NetworkConnection {
+	if conn.CloseTimestamp != nil {
+		conn.CloseTimestamp = NotNilTimestamp
 	}

-	ipNetwork := utils.IPNetworkFromCIDRBytes(ipNetworkData)
-	prefixLen := ipNetwork.PrefixLen()
-	// If this is IPv4 and the prefix length is 32 or this is IPv6 and the prefix length
-	// is 128 this is a regular IP address and not a CIDR block
-	if (ipNetwork.Family() == utils.IPv4 && prefixLen == byte(32)) ||
-		(ipNetwork.Family() == utils.IPv6 && prefixLen == byte(128)) {
-		peerId.Address = ipNetwork.IP()
-	} else {
-		peerId.IPNetwork = ipNetwork
-	}
-	return peerId.String()
+	return conn
 }


It might make sense to only make a copy of the connection if we need to adjust it, but otherwise return the original

e.g.

func adjustNetworkConnectionForComparison(conn *sensorAPI.NetworkConnection) *sensorAPI.NetworkConnection { if conn.CloseTimestamp != nil { conn := conn.CloneVT() // IIRC, not sure the exact copy function conn.CloseTimestamp = NotNilTimestamp } return conn }

Stringy · 2025-07-14T13:48:57Z

integration-tests/pkg/mock_sensor/server.go

-		return slices.ContainsFunc(conns, func(c types.NetworkInfo) bool {
-			return c.Equal(conn)
+		return slices.ContainsFunc(conns, func(c *sensorAPI.NetworkConnection) bool {
+			return types.EqualNetworkConnection(*c, *conn)


Why do we need to deref the pointers to do the comparison? can't we just make EqualNetworkConnection take the pointers and not pass by value?

I passed by values to prevent the connections from being modified. I guess I could pass by reference and then be careful. I don't think we need to extremely concerned about the performance of the integration tests. In collector we should be careful about passing by reference versus value, but I don't think it should matter here. It is probably best to be on the safe side and prevent mutation by passing in by value. I am definitely open to passing by reference.

See @erthalion's comment here about concerns how the values could be mutated #2189 (comment)

My suggestion is only partially to do with performance and mutation, and slightly more to do with API design.

When these functions are used we are never going to have a protobuf value; we're always going to have a pointer (due to the way the mock sensor works, and how protobufs work in general) so we're making the function needlessly more difficult/confusing to call by requiring the caller to first dereference and create a copy.

These copies are also shallow, which means that internal state could be shared between the copies anyway, which makes this even more error prone IMO. Pretty certain this will happen if we have nested proto definitions, which we do have for the vast majority of our definitions; NetworkConnection and ProcessSignal included.

If we take a copy-on-write approach (using CloneVT where appropriate), we can keep the API nice and clean, avoid these shallow copies in cases where we do have to do some modification.

@erthalion's concerns seem slightly different though, and slightly outside the scope of passing by value vs by reference.

I now pass in pointers and make deep copies with CloneVT().

integration-tests/suites/base.go

integration-tests/suites/runtime_config_file.go

…her than value

…or TestConnectionsAndEndpoints

Stringy · 2025-07-16T08:29:42Z

integration-tests/pkg/types/network.go

+	copyConn1 := conn1.CloneVT()
+	copyConn2 := conn2.CloneVT()


I don't think we need to always copy, and in fact we're copying up to four times in this function (if either copy has no close timestamp we copy it again)

We can drop both of these CloneVT calls, and just pass the conn1 and conn2 pointers to adjustNetworkConnectionForComparison which will do the clone only if we need to modify

JoukoVirtanen marked this pull request as ready for review June 25, 2025 18:45

JoukoVirtanen requested a review from a team as a code owner June 25, 2025 18:45

sourcery-ai bot reviewed Jun 25, 2025

View reviewed changes

JoukoVirtanen marked this pull request as draft June 26, 2025 02:35

JoukoVirtanen added 11 commits June 27, 2025 08:23

X-Smart-Branch-Parent: master

d3c4a64

Reduced usage of NetworkInfo

037aa91

Entirely removed types.NetworkInfo

d9cc8be

Changed NetworkInfoBatch to NetworkConnectionBatch

b325c41

Fixed TestProcessNetwork

7871e22

Fixed repeated_network_flow.go

3c27b20

Fixed listening_ports.go

67010b6

Fixed udp_networkflow.go

4bf0b9c

Check for equality between connections checks for protocol

340510f

Removed unused import

23de1bb

Using EqualVT from generated code rather than own function

b465bf6

JoukoVirtanen force-pushed the jv-use-stackrox-protobufs-instead-of-types branch from 92687d0 to b465bf6 Compare June 28, 2025 03:20

JoukoVirtanen commented Jun 28, 2025

View reviewed changes

integration-tests/pkg/types/network.go Outdated Show resolved Hide resolved

JoukoVirtanen commented Jun 28, 2025

View reviewed changes

JoukoVirtanen marked this pull request as ready for review June 28, 2025 16:26

sourcery-ai bot reviewed Jun 28, 2025

View reviewed changes

JoukoVirtanen changed the title ~~Jv use stackrox protobufs instead of types~~ ROX-29500: Use stackrox protobufs instead of types.NetworkInfo Jun 28, 2025

JoukoVirtanen commented Jun 28, 2025

View reviewed changes

integration-tests/suites/base.go Show resolved Hide resolved

erthalion reviewed Jul 1, 2025

View reviewed changes

JoukoVirtanen added 5 commits July 2, 2025 10:15

Setting port variables

b285896

Using EqualNetworkConnection for connections_and_endpoints.go

f04ed83

adjustNetworkConnectionForComparison passes conn by value instead of …

712c132

…reference

Using bytes.Compare instead of CompareBytes

56cae48

Using proto.Equal instead of EqualVT

1283ac1

JoukoVirtanen requested a review from erthalion July 3, 2025 04:05

JoukoVirtanen added 2 commits July 2, 2025 23:29

Removed NotNilTimestamp

f38392b

Fixed LessNetworkConnection

c0c605b

Stringy reviewed Jul 14, 2025

View reviewed changes

JoukoVirtanen added 3 commits July 14, 2025 13:46

Passing conn to adjustNetworkConnectionForComparison by reference rat…

9952557

…her than value

Moved some vars to const

2fdd62b

Moved adjustNetworkConnectionForComparison inside EqualNetworkConnection

1404d11

JoukoVirtanen requested a review from Stringy July 14, 2025 21:45

JoukoVirtanen added 3 commits July 14, 2025 16:06

connections_and_endpoints.go doesn't compare close timestamps

0e6a2eb

Use CloneVT to avoid mutation. Pass pointers to functions

453bfdc

Renamed NilTimestampStr back to NilTimestamp. Created placeholderIP f…

66f1706

…or TestConnectionsAndEndpoints

Stringy reviewed Jul 16, 2025

View reviewed changes

ROX-29500: Use stackrox protobufs instead of types.NetworkInfo #2189

Are you sure you want to change the base?

ROX-29500: Use stackrox protobufs instead of types.NetworkInfo #2189

Uh oh!

Conversation

JoukoVirtanen commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

codecov-commenter commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoukoVirtanen Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoukoVirtanen Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoukoVirtanen Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoukoVirtanen commented Jun 23, 2025 •

edited

Loading

codecov-commenter commented Jun 23, 2025 •

edited

Loading

JoukoVirtanen Jun 28, 2025 •

edited

Loading

JoukoVirtanen Jul 14, 2025 •

edited

Loading

JoukoVirtanen Jul 15, 2025 •

edited

Loading