ROX-26516: Collector reads runtime config from configmap

[JoukoVirtanen]

Description

Makes it so that the runtime config can be read from a ConfigMap. The ConfigMap is added to collector here stackrox/stackrox#12910. The ConfigMap is optional and is not needed in order for collector to run. For now the runtime config only enables or disables the external IPs feature. If the ConfigMap is not used the external IPs feature will be controlled by an environment variable.

Json is used for the configuration. This is convenient, because there is a method for converting Json strings to protobuf objects. YAML was considered, but having the ConfigMap use Json makes it easy to copy the ConfigMap into the API, and use it there. Also using a library for YAML, proved to be a pain.

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.

Testing Performed

Testing was performed using stackrox/stackrox#12910 which adds the ConfigMap to collector.

Created the following yaml file for a ConfigMap

apiVersion: v1
kind: ConfigMap
metadata:
  name: collector-config
  namespace: stackrox
data:
  runtime_config.yaml: |
    networkConnectionConfig:
        enableExternalIps: true

Saved the contents in config-map.yml.

Ran

kubectl create -f config-map.yml

Deployed ACS on a GKE infra cluster using deploy/deploy-local.sh.

Set the collector image to one from this PR using ks edit ds collector.

The collector logs were checked and the following was found

[INFO    2024/10/08 20:46:11] Runtime configuration: network_connection_config {
  enable_external_ips: true
}

enableExternalIps false

The configmap was changed to

apiVersion: v1
kind: ConfigMap
metadata:
  name: collector-config
  namespace: stackrox
data:
  runtime_config.yaml: |
    networkConnectionConfig:
        enableExternalIps: false

Collector pods were deleted and allowed to come back up

ks delete pod -l app=collector

[INFO    2024/10/08 20:52:24] Runtime configuration: network_connection_config {
}

Invalid config

The configmap was changed to the following

apiVersion: v1
data:
  runtime_config.yaml: |
    networkConnectionConfig:
        enableExternalIps: asdf
kind: ConfigMap
metadata:
  creationTimestamp: "2024-10-08T20:51:09Z"
  name: collector-config
  namespace: stackrox
  resourceVersion: "1587598"
  uid: 7594e306-7334-42b5-b439-ed04ae88ffed

The collector log had the following

[INFO    2024/10/08 20:52:24] Runtime configuration: network_connection_config {
}

This is the expected result.

No config map

The configmap was deleted and collector was restarted. The following was found in the collector log.

There was nothing in the collector logs.

Different config path

The configmap was changed to the following

apiVersion: v1
data:
  collector.yaml: |
    networkConnectionConfig:
        enableExternalIps: true
kind: ConfigMap
metadata:
  creationTimestamp: "2024-10-08T21:00:49Z"
  name: collector-config
  namespace: stackrox
  resourceVersion: "1593991"
  uid: 829de9f9-0f19-4457-927d-b3ea33b8f38f

The following were added to the env section of collector

        - name: ROX_COLLECTOR_CONFIG_PATH
          value: /etc/stackrox/collector.yaml

The following was in the collector log

[INFO    2024/10/08 21:05:08] Runtime configuration: network_connection_config {
  enable_external_ips: true
}

[Molter73]

Overall looks good, but I still have some comments before I'm ready to approve.

[Molter73]

We can probably rewrite all of these as a single test with something like:

TEST(CollectorConfigTest, TestConfigurationFile) {
  std::vector<std::pair<std::string, bool>> tests = {
    { R"({
       "networkConnectionConfig": {
         "enableExternalIps": true
       }
     })", true },
    { R"({
           "networkConnectionConfig": {
             "enableExternalIps": false
           }
         })", false },
    // ...
  };
  
  for (const auto& [input, expected] : tests) {
    CollectorConfig config;
    config.HandleConfigMapString(input);
    EXPECT_EQUAL(config.EnableExternalIPs(), expected);
  }
}

Since the test is concise (literally 3 lines in a loop), the result should overall have less bloat and make it easier to understand what is being tested.

[Molter73]

We are also missing a test for an empty string as input.

[JoukoVirtanen]

Done

[Molter73]

I also now realize the only place I can actually check what the configuration is going to look like is these tests, which is not great, we need a way to have a formal specification of the configuration that we can properly review, but for now I guess it's fine 🤷🏻‍♂️ .

I do have some objections though:

• networkConnectionConfig is redundant, we know this is a configuration parameter, so it should be networkConnection.
• enableExternalIps is not extensible.

I would prefer the configuration to look something closer to this:

networkConnection:
  externalIPs:
    enable: true

[JoukoVirtanen]

If you want to change these, we need to change the protobuf definitions.

[Molter73]

Why not? Is not like the feature is GA.

[JoukoVirtanen]

I didn't say that we couldn't change the protobuf definitions.

[ovalenti]

The multi-threaded angle.

[Molter73]

LGTM!

[Molter73]

You don't need the if, you already expected runtime_config.has_value() to be true.

Suggested change

	EXPECT_TRUE(runtime_config.has_value());
	if (runtime_config.has_value()) {
	EXPECT_TRUE(runtime_config.has_value());

[JoukoVirtanen]

Done

[SimonBaeumer]

Nice to document it!