ROX-26344: adds runtime configuration to collector #1866
Conversation
Includes support for enabling/disabling external IPs based on runtime config, deferring to existing feature flag if none is provided
There was a problem hiding this comment.
Using the CollectorConfig object makes sense, good idea !
I think that the next steps should be:
- on the Sensor side:
- Update the proto defs to introduce an enum type wrapping and replacing
NetworkFlowsControlMessage - introduce the runtime-config object in the new enum
- implement a switch-case in NetworkStatusNotifier in onRecvControlMessage or its successor
- when receiving a runtime-config in the new switch-case, update the value in CollectorConfig
- Update the proto defs to introduce an enum type wrapping and replacing
- on the ConnTracker side:
- everytime we start a scrape in either
NetworkStatusNotifier::RunSingleorNetworkStatusNotifier::RunSingleAfterglow, set the ExtIP attribute usingConnectionTracker::EnableExternalIPs
- everytime we start a scrape in either
There was a problem hiding this comment.
As discussed previously and in #11746 , we should remember to unify the names of the environment variables that enable the ExtIP feature.
I think that it would make sense to update CollectorConfig's env var name to match the one in Central (ROX_EXTERNAL_IPS) as part of this PR.
ovalenti
dismissed
their stale review
The proposed change has been made, thanks !
collector/lib/CollectorConfig.h
Outdated
| runtime_config_ = std::move(runtime_config); | ||
| } | ||
|
|
||
| std::optional<sensor::CollectorConfig> GetRuntimeConfig() const { |
There was a problem hiding this comment.
Lets not copy the object every time it's accessed.
| std::optional<sensor::CollectorConfig> GetRuntimeConfig() const { | |
| const std::optional<sensor::CollectorConfig>& GetRuntimeConfig() const { |
collector/lib/CollectorConfig.cpp
Outdated
| @@ -47,7 +47,7 @@ BoolEnvVar set_import_users("ROX_COLLECTOR_SET_IMPORT_USERS", false); | |||
|
|
|||
| BoolEnvVar collect_connection_status("ROX_COLLECT_CONNECTION_STATUS", true); | |||
|
|
|||
| BoolEnvVar enable_external_ips("ROX_ENABLE_EXTERNAL_IPS", false); | |||
| BoolEnvVar enable_external_ips("ROX_EXTERNAL_IPS", false); | |||
There was a problem hiding this comment.
I actually think the ENABLE makes it clearer what the env variable is for. But I would argue a variable in the form <PROGRAM>_<FEATURE>_<PARAM> is even more clear, so in this case we might want to name the variable ROX_EXTERNAL_IPS_ENABLE or ROX_COLLECTOR_EXTERNAL_IPS_ENABLE
There was a problem hiding this comment.
In general I think I agree. @ovalenti what do you think? should we update the central feature flag?
There was a problem hiding this comment.
In Central, it looks like the convention is to use short names and no verb... https://github.com/stackrox/stackrox/blob/0b83c7607cb58d28e0165e20936f30d0c11d12c0/pkg/features/list.go
Although unifying the flag names seemed like a good idea at first, the collector one is likely not going to be used directly by any customer. So I am starting to think that we could keep the names as they were, or as Mauro suggests.
There was a problem hiding this comment.
I don't think ENABLE is needed in the name. In stackrox/stackrox the function to check the value of feature flags is Enable(), so it would seem to be redundant.
There was a problem hiding this comment.
In stackrox/stackrox the function to check the value of feature flags is
Enable(), so it would seem to be redundant.
It wouldn't be redundant to a user that is trying to configure collector without any knowledge of what the code in the main repo looks like 🙂
There was a problem hiding this comment.
I will revert this for now and we can decide on naming conventions later on
| const auto& cfg = runtime_config_.value(); | ||
| const auto& network_cfg = cfg.network_connection_config(); | ||
| return network_cfg.enable_external_ips(); |
There was a problem hiding this comment.
We could've also done something like...
return runtime_config_.value()
.network_connection_config()
.enable_external_ips();But meh, this is good too.
Description
Includes support for enabling/disabling external IPs based on runtime config, deferring to existing feature flag if none is provided
In subsequent PRs, the runtime configuration will be populated.
Checklist
- [x] Updated documentation accordinglyAutomated testing
- [ ] Added integration tests- [ ] Added regression testsIf any of these don't apply, please comment below.
Testing Performed
Unit tests added to exercise the intended behaviour. It is not possible to integration-test this until Sensor->Collector communication has been built.