-
Notifications
You must be signed in to change notification settings - Fork 24
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
Jv integration tests for runtime config #1899
Jv integration tests for runtime config #1899
Conversation
316fecb
to
ae8bf37
Compare
4b6cf76
to
4b227bd
Compare
6d06416
to
075a4df
Compare
collector/lib/ConfigLoader.cpp
Outdated
} catch (const YAML::Exception& e) { | ||
CLOG(ERROR) << "Failed to parse the configuration file: " << config_file << ". Error: " << e.what(); | ||
} catch (const std::exception& e) { | ||
CLOG(ERROR) << "An unknown error occured while loading the configuration file: " << config_file << ". Error: " << e.what(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm less than half way through, I'll finish the review tomorrow.
collector/test/ConfigLoaderTest.cpp
Outdated
@@ -67,12 +67,20 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigInvalid) { | |||
} | |||
|
|||
TEST(CollectorConfigTest, TestYamlConfigToConfigEmpty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this test name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -41,6 +41,7 @@ func NewDockerCollectorManager(e executor.Executor, name string) *DockerCollecto | |||
"/host/etc:ro": "/etc", | |||
"/host/usr/lib:ro": "/usr/lib", | |||
"/host/sys/kernel/debug:ro": "/sys/kernel/debug", | |||
"/etc/stackrox:ro": "/tmp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider creating a separate directory for this like /tmp/collector-test
and use that instead of the full /tmp directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
func AssertExternalIps(t *testing.T, enable bool, collectorIP string) { | ||
AssertRepeated(t, func() bool { | ||
body := QueryConfig(t, collectorIP) | ||
var response types.RuntimeConfig | ||
err := json.Unmarshal(body, &response) | ||
assert.NoError(t, err) | ||
|
||
return response.Networking.ExternalIps.Enable == enable | ||
}) | ||
} | ||
|
||
func AssertNoRuntimeConfig(t *testing.T, collectorIP string) { | ||
AssertRepeated(t, func() bool { | ||
body := QueryConfig(t, collectorIP) | ||
return strings.TrimSpace(string(body)) == "{}" | ||
}) | ||
} | ||
|
||
// TODO: This should be in its own package | ||
func AssertRepeated(t *testing.T, condition func() bool) { | ||
tick := time.NewTicker(1 * time.Second) | ||
timer := time.After(3 * time.Minute) | ||
|
||
for { | ||
select { | ||
case <-tick.C: | ||
if condition() { | ||
// Condition has been met | ||
return | ||
} | ||
|
||
case <-timer: | ||
// TODO: This message should be passed in rather than hard coded here | ||
t.Log("Timeout reached: Runtime configuration was not updated") | ||
t.FailNow() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to move all assertions to a separate pkg/assert
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
func QueryConfig(t *testing.T, collectorIP string) []byte { | ||
log.Info("Querying: /state/config") | ||
body, err := IntrospectionQuery(collectorIP, "/state/config") | ||
assert.NoError(t, err) | ||
log.Info("Response: %q", body) | ||
return body | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sold this is something we need, from a test we could just do:
body, err := IntrospectionQuery(s.Collector().IP(), "/state/config")
s.Require().NoError(err)
This is simple enough and you can add the logging if you deem it necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
b446ed0
to
347114d
Compare
@@ -33,7 +33,7 @@ loop: | |||
case <-timer: | |||
// we know they don't match at this point, but by using | |||
// ElementsMatch we get much better logging about the differences | |||
return assert.ElementsMatch(t, expected, s.Connections(containerID), "timed out waiting for networks") | |||
return assert.ElementsMatch(t, expected, s.Connections(containerID), "timed out waiting for network connections") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file were made here https://github.com/stackrox/collector/pull/1902/files
@@ -32,7 +32,6 @@ const ( | |||
// us to use any comparable type as the key) | |||
type ProcessMap map[types.ProcessInfo]interface{} | |||
type LineageMap map[types.ProcessLineage]interface{} | |||
type ConnMap map[types.NetworkInfo]interface{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file were made here https://github.com/stackrox/collector/pull/1902/files
func AssertExternalIps(t *testing.T, enable bool, collectorIP string) { | ||
AssertRepeated(t, func() bool { | ||
body, err := collector.IntrospectionQuery(collectorIP, "/state/config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to do this here, but the IntrospectionQuery
method needs to be part of a collector manager object, it would change this to be a lot simpler/clearer:
func AssertExternalIps(t *testing.T, enable bool, collector *CollectorManager) {
AssertRepeated(t, func() bool {
body, err := collector.IntrospectionQuery("/state/config")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be done in another PR. Maybe we should have an Introspection
struct that could be a member of a Collector
interface. The introspection object would get the IP address from the collector object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting the tests up, as silly as it may sound, it's a lot easier to understand what is going on now.
The comments I left now are mostly on moving some comments around to leave the code blocks on their own. This should help reduce clutter a bit more, since the description of what will be done is provided up front and the code is left as clean as possible.
// The runtime config file was deleted before starting collector so there should not be any config | ||
assert.AssertNoRuntimeConfig(s.T(), collectorIP) | ||
// Since there is no config the default is used, which means external IPs is disabled and we should | ||
// expect a normalized connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The runtime config file was deleted before starting collector so there should not be any config | |
assert.AssertNoRuntimeConfig(s.T(), collectorIP) | |
// Since there is no config the default is used, which means external IPs is disabled and we should | |
// expect a normalized connection | |
// The runtime config file was deleted before starting collector. | |
// Default configuration is external IPs disabled. | |
// We expect normalized connections. | |
assert.AssertNoRuntimeConfig(s.T(), collectorIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This same change can be applied to the other 2 tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not applied to all tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has now been applied to all tests.
CI results Run 1
The other konflux integration tests failed with either problems pulling the image. Both konflux and non-konflux cos integration tests failed with verifier errors. Run 2: Run 3:
I did not see a previous failure that caused this error. There were other failures unrelated to the changes in the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look ok, but #1902 still has comments on it that haven't been addressed, if your intent is to merge the changes from that PR with this one, please close it so I can move my comments to this PR.
tick := time.NewTicker(1 * time.Second) | ||
timer := time.After(3 * time.Minute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these timeouts be configurable via some parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
8420864
to
9b4f72e
Compare
I have addressed the PR review comments in the other PR. |
d734a7c
to
f092de4
Compare
4a03f7b
to
9241f09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but holding approval until #1902 is merged, so this can be rebased on top of that.
ba6babc
to
72fb150
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just a small comment on a TODO that I think we could solve quickly before merging, but we can also address it on the next PR that needs to use the helper function.
// TODO: This message should be passed in rather than hard coded here | ||
log.Error("Timeout reached: Runtime configuration was not updated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO
should be relatively simple to solve, add a format and args parameter to the function like this:
func AssertRepeated(t *testing.T, tickTime time.Duration, timeout time.Duration, condition func() bool, msg string, args ...interface{}) {
Then here you can just call log.Error
like this:
log.Error("Timeout reached: " + msg, args...)
If this ends up being too verbose on the call sites with the function being defined inline, you can always define the function and assign it to a variable, then pass that in.
Also, there are some tests on the multiarch for konflux, but those are due to the integration test image not being rebuilt for those archs, this is not a blocker for this PR and I'm trying to address it here: #1974 |
) | ||
|
||
func AssertExternalIps(t *testing.T, enable bool, collectorIP string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] should probably be enabled
rather than enable
(the latter implies we're turning it on)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
ExternalIps struct { | ||
Enable bool | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a note that we'll need to add PerContainerRateLimit
(probably in a follow up PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be defining a RuntimeConfig struct here. We should be importing the go code generated from the protobuf definitions. The same for EndpointInfo
, NetworkInfo
and other similar structs. That should be done in a different PR.
cmd := "mkdir " + dir | ||
s.execShellCommand(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do this via the standard library? same for deleteFile, setRuntimeConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we get the logging that comes with execShellCommand
. Also if there are changes to how commands need to be executed, using the standard library might not be the best method. E.g we might want to run the commands in a VM, locally, in a container, or somewhere else and changes to those things might result in the standard library not working. However, you did remind me that this is not the correct place for these functions and I have moved them to base.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've gone to great lengths to remove the various contexts in which things run, and today we're always running on the same host with the test containers so I don't think we should care about how commands run, or whether to restrict ourselves to using the shell instead of the stdlib. If this ever changes, then we'd have to overhaul the way the tests work again, so a few file system operations would be the least of our worries.
If these specific operations need to run in various contexts, then execShellCommand
is still inappropriate because it is intended for running a local shell command (via exec.Command
) and we'd have to change these functions anyway.
It's just that a shell command feels very expensive for an operation as simple as making a directory or deleting a file, and if we need logging then we can add logging here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we would have a better handle on the errors if we do it via the stdlib. We wouldn't need to worry about failures in exec'ing the shell or the command, for example.
we're also just suppressing all errors at the moment -- we should probably return them to the caller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
configStr := `networking: | ||
externalIps: | ||
enable: ` + strconv.FormatBool(enable) | ||
s.setRuntimeConfig(runtimeConfigFile, configStr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better for to construct the object and serialize it to a string, and in fact it may be best to have the caller pass the structure directly to setRuntimeConfig
func (s *RuntimeConfigFileTestSuite) setRuntimeConfig(path string, config types.RuntimeConfig) error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
e8bb38d Starting work 23ee1b0 Working with config introspection endpoint. No checks for connections yet 1b919ee Moved code for introspection endpoint from mock sensor to own directory 91ee35e Testing with an invalid config ae8bf37 Creates a container with an external connection and adds tests for it. Also made changes so that external connections can be checked for 34247e2 Mock server no longer maintains just a map of connections seen, but all connections edf7ca3 Should be able to handle cidr blocks and non-cidr blocks 4b227bd Able to handle the case when there is only port data dbed0a1 Brought in changes to server.go to handle external ips fd24fe8 Apply suggestions from code review 3eb5e48 Cleanup 13f8539 Able to control if connections should be ordered or not. Also added comments f9ea3fc Listening for new connection events instead of using a ticker 6d06416 Using ExpectSameElementsConnections instead of ExpectExactConnections 9c3f013 More error handling for reading the configuration. Using a volume mount a4fe63b Fixed lint error 8dce19c The runtime config file is only modified on the host machine 1c05a55 There is now only one source code file for runtime config introspection 7b77e23 Minor fixes for k8s e53509f Added comments and sleeps when config does not change so we get a scrape interval 2da34ae Increased the afterglow period ca6dd2c Update collector/lib/ConfigLoader.cpp 6aa60e0 Renamed unit test. Integration tests use a dedicated directory a6b0c57 Created a new assert package 949534d Apply suggestions from code review 09b13aa Split test into smaller tests 9346776 Apply suggestions from code review 3586964 Removed unneeded sort 5e95832 Using slices.ContainsFunc in HasConnection 76e244d Made timings in AssertRepeated configurable 5e94d5c Always sort the connections when comparing 12c04f3 Not using assertMismatch 129308e Updated comments 0b34466 Apply suggestions from code review e2fd23c Sorting connections inside Connections instead of SortedConnections a544a68 Created ElementsMatchFunc in assert pkg 6d1a737 Using spew for logging 2ae0101 Apply suggestions from code review 416b7f2 Importing logging package 9241f09 Simplified by removing checkIfConnectionsMatchExpected ba6babc Changed assert.True to assert.Fail
…eConfig object to a config string rather than writing the string
b836088
to
9a30908
Compare
s.execShellCommand(cmd) | ||
} | ||
|
||
func (s *RuntimeConfigFileTestSuite) setExternalIpsEnable(runtimeConfigFile string, enable bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might sound like a nit, but from a separation of concerns perspective, this function shouldn't do the marshalling - it should be handled by setRuntimeConfig
This function should just construct types.RuntimeConfig
object and pass it to setRuntimeConfig
. Otherwise, when we inevitably come to adding another field to these tests, we'll have to duplicate the marshaling logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cmd := "mkdir " + dir | ||
s.execShellCommand(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've gone to great lengths to remove the various contexts in which things run, and today we're always running on the same host with the test containers so I don't think we should care about how commands run, or whether to restrict ourselves to using the shell instead of the stdlib. If this ever changes, then we'd have to overhaul the way the tests work again, so a few file system operations would be the least of our worries.
If these specific operations need to run in various contexts, then execShellCommand
is still inappropriate because it is intended for running a local shell command (via exec.Command
) and we'd have to change these functions anyway.
It's just that a shell command feels very expensive for an operation as simple as making a directory or deleting a file, and if we need logging then we can add logging here.
cmd := "mkdir " + dir | ||
s.execShellCommand(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we would have a better handle on the errors if we do it via the stdlib. We wouldn't need to worry about failures in exec'ing the shell or the command, for example.
we're also just suppressing all errors at the moment -- we should probably return them to the caller
…ctions for createDir and removeFile
|
||
func (s *RuntimeConfigFileTestSuite) setRuntimeConfig(runtimeConfigFile string, configStr string) { | ||
cmd := "echo '" + configStr + "' > " + runtimeConfigFile | ||
s.execShellCommand(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use os.write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, there's a last couple of minor changes but otherwise LGTM!
Description
Integration tests for runtime configuration. The tests create various versions of the runtime configuration file and check that the configuration introspection endpoint returns the correct expected configuration.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Ran the integration tests locally 2,000 times without a failure using the following script