ROX-26726: Collector integration tests should record all connections #1902
Conversation
| types.SortConnections(expected) | ||
| } | ||
| timer := time.NewTimer(timeout) | ||
| ticker := time.NewTicker(100 * time.Millisecond) |
There was a problem hiding this comment.
Instead of using a ticker get the connections when a new connection comes in, like the other functions.
JoukoVirtanen
force-pushed
the
jv-ROX-26726-collector-integration-tests-should-record-all-connections
branch
from
e14e9d3
to
0fe6716
Compare
…ifferent non nil timestamps are considered to be equivalent
JoukoVirtanen
force-pushed
the
jv-ROX-26726-collector-integration-tests-should-record-all-connections
branch
from
7590794
to
d1a1847
Compare
There was a problem hiding this comment.
I'm half way through the review and I've decided not to finish it. While I see value in having the mock sensor store all the connections seen, the added checks in expect_conn.go add complexity where it isn't needed (see my comments on that file for more details). Please revisit the implementation to see if we can get rid of most of the methods in there and reach out to me if you want to discuss potential alternatives.
| func (s *MockSensor) checkIfConnectionsMatchExpected(t *testing.T, connections []types.NetworkInfo, expected []types.NetworkInfo) bool { | ||
| if len(connections) > len(expected) { | ||
| return assert.ElementsMatch(t, expected, connections, "networking connections do not match") | ||
| } | ||
|
|
||
| if len(connections) == len(expected) { | ||
| types.SortConnections(connections) | ||
| for i := range expected { | ||
| if !expected[i].Equal(connections[i]) { | ||
| return assert.ElementsMatch(t, expected, connections, "networking connections do not match") | ||
| } | ||
| } | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Isn't this whole method the same as just calling assert.ElementsMatch(t, expected, connections, "networking connections do not match")? From the testify docs:
ElementsMatch asserts that the specified listA(array, slice...) is equal to specified listB(array, slice...) ignoring the order of the elements. If there are duplicate elements, the number of appearances of each of them in both lists should match.
https://pkg.go.dev/github.com/stretchr/testify/assert#ElementsMatch
| // getConnectionsAndCompare gets the connections for a container, sorts them if order is set to true, and compares with a set of expected | ||
| // connections. If asssertMismatch is true and the set of observed connections does not match the set of expected connections an assert is | ||
| // triggered. If assertMismatch is not set then just return if the observed and expected connections match. | ||
| func (s *MockSensor) getConnectionsAndCompare(t *testing.T, containerID string, order bool, assertMismatch bool, expected ...types.NetworkInfo) bool { |
There was a problem hiding this comment.
If checkIfConnectionsMatchExpected always sorts the container connections, what is the point of ordering them here based on the order parameter?
There was a problem hiding this comment.
Also, why do we need assertMismatch? Couldn't the caller just check if the returned value is false?
There was a problem hiding this comment.
On a separate note about order, the way collector handles connections is inherently asynchronous, so even with extreme care, if the implementation preserved the order, expecting a given order of connections to always be in the same order seems unrealistic and a great way to add flakes to our tests.
Description
Currently the mock server only maintains a map where the key is a networking connection, instead of a full list of all connections that it sees, this means that if a connection is opened and closed multiple times it will only save the opening event once. For some tests all of the opening events will need to be recorded. For the runtime configuration tests the same connection will be opened and closed multiple times, so the changes here are needed for it. See #1899
This PR is built on another PR that makes it so that the mock server can properly handle connections involving external IPs and CIDR blocks. #1901
Other changes are also made.
The addition of a function that requires that the elements of the list of expected connections matches the elements of the list of observed. This differs from the existing function, which only requires that each expected connection be seen. The old function would tolerate having extra observed connections that were not expected.
Connections with different last active timestamps are considered equivalent.
In the future similar changes will be made for networking endpoints and processes.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
The changes in this PR were used to add more integration tests here #1899