ROX-19898: refactor collector configuration in tests #1348
Conversation
integration-tests/suites/base.go
Outdated
| } | ||
|
|
||
| // isSelinuxPermissiveNeeded returns whether or not a given VM requires | ||
| // SELinux permissive mode. e.g. rhel, or fedora-coreos | ||
| func (s *IntegrationTestSuiteBase) isSelinuxPermissiveNeeded() bool { | ||
| vmType := config.VMInfo().Config | ||
| if strings.Contains(vmType, "coreos") || strings.Contains(vmType, "rhcos") { |
There was a problem hiding this comment.
Not sure how this change made it in, I think I hadn't pushed the base branch when I created this branch. I'll juggle commits at some point, or we can just merge this change as part of this PR (I don't think it strictly affects either of them)
There was a problem hiding this comment.
I think it's fine, we can merge it as part of this PR.
| s.Collector().Env["ROX_ENABLE_AFTERGLOW"] = strconv.FormatBool(s.EnableAfterglow) | ||
| collectorOptions := common.CollectorStartupOptions{ | ||
| Config: map[string]any{ | ||
| "scrapeInterval": s.ScrapeInterval, |
There was a problem hiding this comment.
@JoukoVirtanen is this correct? it seems in this test we're specifically setting a scrape interval, but always turn scraping off.
There was a problem hiding this comment.
I've tested this locally and the test still passes with turnScrapeOff=false. I think this probably better represents what is intended in this test, but could do with some confirmation in case there is some nuance here that I'm missing
There was a problem hiding this comment.
The scrapeInterval is also the reporting interval. Even if scrape is off the scrapeInterval will be used to determine when connections and endpoint are reported. It is a bit confusing and should be changed. In this case it shouldn't matter if scraping is done or not. In that case network connections will come from NetworkSignalHandler.
There was a problem hiding this comment.
Thanks! I'll turn off scraping again and fix the conflicts and hopefully we can merge this today
|
Stringy
changed the title
There was a problem hiding this comment.
This is great! I think the new way of organizing configuration for collector is a lot cleaner to read and I love we are using proper JSON serialization (although I'd prefer we didn't have that JSON env variable, but that will be dealt with at some other point in time).
LGTM! But I'd still like to hear from @JoukoVirtanen on your comment.
| configJson, err := json.Marshal(c.config) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I'd pay for go to implement something like the ? from rust so we could do configJson := json.Marshal(c.config)? and be done...
integration-tests/suites/base.go
Outdated
| } | ||
|
|
||
| // isSelinuxPermissiveNeeded returns whether or not a given VM requires | ||
| // SELinux permissive mode. e.g. rhel, or fedora-coreos | ||
| func (s *IntegrationTestSuiteBase) isSelinuxPermissiveNeeded() bool { | ||
| vmType := config.VMInfo().Config | ||
| if strings.Contains(vmType, "coreos") || strings.Contains(vmType, "rhcos") { |
There was a problem hiding this comment.
I think it's fine, we can merge it as part of this PR.
Stringy
force-pushed
the
giles/ROX-14542-collector-config-refactor
branch
from
cd2b715
to
d94fa77
Compare
Stringy
force-pushed
the
giles/ROX-14542-collector-config-refactor
branch
from
d94fa77
to
b8fc304
Compare
Description
To avoid having to provide (1) a hard-coded json string and (2) duplicated default options across all tests, this PR introduces
CollectorStartupOptionsstructure which allows tests to set specific options/environment variables/mount points before starting collector. The collector manager itself is responsible for serializing this information as necessary and tests only need to modify the options that they need to.Options provided by a test always supersede those defined by default in the collector manager.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Tested and built locally against Garden Linux (of all things) CI will catch everything else. This should have no impact on the outcome of the tests.