chore(plugins/container): Make sockets optional when disabled

[aditki]

The init_config JSON schema currently requires all 7 engine keys to be present in the engines block, and requires sockets for SocketsContainer types even when enabled=false. This forces users to define every engine (including irrelevant ones like bpm on Kubernetes) and provide socket paths for disabled engines.

The C++ from_json code already handles missing keys gracefully via j.value() defaults, so the schema is strictly more restrictive than the implementation requires.

Changes:

• Use oneOf conditional validation on SocketsContainer so sockets is only required when enabled=true. Disabled engines no longer need socket paths.
• Apply same oneOf pattern to StaticContainer so container_id, container_name, and container_image are only required when enabled=true.

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area plugins

/area registry

/area build

/area documentation

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

[poiana]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aditki
Once this PR has been reviewed and has the lgtm label, please assign deepskyblue86 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• plugins/container/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[poiana]

Welcome @aditki! It looks like this is your first PR to falcosecurity/plugins 🎉

[ekoops]

Hey thank you for this contribution. I looked at the parsing code and it seems like we are interested in this: https://github.com/falcosecurity/plugins/blob/main/plugins/container/src/plugin_config.cpp#L23-L34 . As you can see, the default value for each of those engine is SimpleEngine{}, SocketsEngine{} or StaticEngine{}. SimpleEngine and SocketsEngine constructor sets enabled to true by default: this means that omitting engines in the configuration will result in the engines being enabled, not disabled.

I agree with you that this improves ergonomic, but it is an API breaking change, and I guess we cannot afford it.
@leogr @irozzo-1A WDYT?

[deepskyblue86]

Hey thank you for this contribution. I looked at the parsing code and it seems like we are interested in this: main/plugins/container/src/plugin_config.cpp#L23-L34 . As you can see, the default value for each of those engine is SimpleEngine{}, SocketsEngine{} or StaticEngine{}. SimpleEngine and SocketsEngine constructor sets enabled to true by default: this means that omitting engines in the configuration will result in the engines being enabled, not disabled.

I agree with you that this improves ergonomic, but it is an API breaking change, and I guess we cannot afford it. @leogr @irozzo-1A WDYT?

There's another issue, if I got the change right: sockets will become mandatory when enabled is true. This will be quite inconvenient for rootless podman.

[leogr]

this means that omitting engines in the configuration will result in the engines being enabled, not disabled.

This is correct and expected. The plugin is made to have most of the engine enabled by default because the plugin is implemented to be fault-tolerant when an engine is not present (basically, it will ignore an enabled engine if the socket isn't available)

That being said, I would be ok with improving the schema only if the original behavior is preserved (so engines enabled and paths configured with defaults even when the corresponding config keys are omitted). However, considering the @ekoops point and @deepskyblue86 concern, I don't believe this change would be a UX improvement. So, is it worth implementing it? Probably not.

[ekoops]

this means that omitting engines in the configuration will result in the engines being enabled, not disabled.

This is correct and expected. The plugin is made to have most of the engine enabled by default because the plugin is implemented to be fault-tolerant when an engine is not present (basically, it will ignore an enabled engine if the socket isn't available)

That being said, I would be ok with improving the schema only if the original behavior is preserved (so engines enabled and paths configured with defaults even when the corresponding config keys are omitted). However, considering the @ekoops point and @deepskyblue86 concern, I don't believe this change would be a UX improvement. So, is it worth implementing it? Probably not.

I tend to agree. @deepskyblue86 WDYT?

[irozzo-1A]

There's another issue, if I got the change right: sockets will become mandatory when enabled is true. This will be quite inconvenient for rootless podman.

@deepskyblue86 If you look carefully, the "socket" field was required for SocketContainer type, and "podman" is of SocketContainer type, so this change is actually making it required only if enabled set to true.

I agree with you that this improves ergonomic, but it is an API breaking change, and I guess we cannot afford it.
@leogr @irozzo-1A WDYT?

Personally, I don't dislike the idea. It's a breaking change in the sense that it makes validation less strict, but the only risk is that new configuration would not work with old plugins, I don't know which is the policy we have in that regard, but I don't think it would be a big problem.

IMO It is counterintuitive that unspecified engines would be enabled by default, in that respect probably it's better to keep explicit config in order to avoid misunderstandings. Those are my 2cts.

[deepskyblue86]

There's another issue, if I got the change right: sockets will become mandatory when enabled is true. This will be quite inconvenient for rootless podman.

@deepskyblue86 If you look carefully, the "socket" field was required for SocketContainer type, and "podman" is of SocketContainer type, so this change is actually making it required only if enabled set to true.

@irozzo-1A You're right, I didn't check it carefully. It has weird ergonomics indeed. Now that I checked, I'm explicitly setting podman sockets to an empty array...

[aditki]

I'm explicitly setting podman sockets to an empty array

@deepskyblue Having empty array for sockets is also an issue. Then helm renders it as null iirc and the schema validation fails. I had to set the sockets of podman and docker to defaults even after disabling it.

[aditki]

There's another issue, if I got the change right: sockets will become mandatory when enabled is true. This will be quite inconvenient for rootless podman.

As @irozzo-1A pointed out, sockets is already required for all SocketsContainer types in the current schema regardless of enabled state. This PR actually makes it less restrictive by only requiring sockets when enabled: true. For the rootless podman case specifically, you mentioned you're setting sockets to an empty array but that's also problematic because YAML [] gets serialized as null through Go/Helm YAML processing, which then fails the type: array schema check. This is the exact issue that led to this PR. The current schema forces users into providing dummy socket paths for disabled engines just to pass validation paths that are never used.

[aditki]

I would be ok with improving the schema only if the original behavior is preserved... I don't believe this change would be a UX improvement. So, is it worth implementing it? Probably not.

@leogr I respect that the plugin is fault-tolerant, but the schema itself is the UX problem not the runtime behavior. The issue is that the Falco Helm chart's containerPlugin helper constructs the init_config programmatically, and when engines is only partially defined (ex: only containerd from values), the schema rejects it before the fault tolerant C++ code ever runs. This affects every Helm-based deployment that tries to configure engines. The Falco chart's own helper template doesn't produce schema valid output (falcosecurity/falco#3721 is related). Users are forced to override the entire engines block with all 7 keys and provide socket paths for disabled engines and can't even use empty arrays because of the YAML null serialization issue.

A scoped fix: Keep engines required, but make sockets optional when enabled: false. This preserves original behavior and only fixes the actual pain point.

[irozzo-1A]

A scoped fix: Keep engines required, but make sockets optional when enabled: false. This preserves original behavior and only fixes the actual pain point.

+1 I'm onboard with that

[ekoops]

I would be ok with improving the schema only if the original behavior is preserved... I don't believe this change would be a UX improvement. So, is it worth implementing it? Probably not.

@leogr I respect that the plugin is fault-tolerant, but the schema itself is the UX problem not the runtime behavior. The issue is that the Falco Helm chart's containerPlugin helper constructs the init_config programmatically, and when engines is only partially defined (ex: only containerd from values), the schema rejects it before the fault tolerant C++ code ever runs. This affects every Helm-based deployment that tries to configure engines. The Falco chart's own helper template doesn't produce schema valid output (falcosecurity/falco#3721 is related). Users are forced to override the entire engines block with all 7 keys and provide socket paths for disabled engines and can't even use empty arrays because of the YAML null serialization issue.

A scoped fix: Keep engines required, but make sockets optional when enabled: false. This preserves original behavior and only fixes the actual pain point.

Hey @aditki, I like your scoped fix. Engines should be required, as this avoid the surprise given by the C++ enabled-by-default logic, but there is no reason to specify socket paths for disabled SocketsContainers,
I'm onboard with your proposal 💪

[leogr]

I like the proposal as well. If my understanding is correct, it shouldn't introduce any breaking change, right?

[ekoops]

Change looks good. Could you please squash it? 🙏

[aditki]

Done!