Allow sections of Plugins to be merged, and not overwritten as entire sections.

[rayburgemeestre]

Hi all, This is a spin-off from the following PR #7347 (which I suggest we close, since I'm afraid the long history there, and non-essential improvements that I've included obfuscate the PR.). My hope is, that this more concise PR is easier to review.

This PR focuses on a very small change targetting 2.0.0, and I've included a unit test to demonstrate the difference it makes. Without the code change, the unit test would fail with: make-test-fail.txt

Summary
The change proposed in this PR allows various drop-in configuration toml files to configure parts of a Plugin.

For example, starting with the following containerd config.toml, it includes a bunch of potential drop-in configs:

imports = ["/etc/containerd/conf.d/*.toml"]

Let's say we have two drop-ins in that directory:

$ cat /etc/containerd/conf.d/cni.toml
[plugins."io.containerd.grpc.v1.cri".cni]
    bin_dir = "/cm/local/apps/kubernetes/current/bin/cni"

$ cat /etc/containerd/conf.d/registry.toml
[plugins."io.containerd.grpc.v1.cri".registry]
    config_path = "/cm/local/apps/containerd/var/etc/certs.d"

Current behavior

The current code in main/2.0.0 would overwrite in this case the whole CRI plugin with the path from registry.toml, and the cni.toml file would not have any effect. Since lexicographically registry.toml comes after cni.toml. The configuration effectively in containerd would be:

[registry]
  config_path = '/cm/local/apps/containerd/var/etc/certs.d'

Note the entire [cni] block missing.

A workaround for the drop-in to have the desired effect, we would have to merge / combine both configurations into a single drop-in file, as follows:

$ cat /etc/containerd/conf.d/combined.toml

[plugins]
  [plugins."io.containerd.grpc.v1.cri"]
    [plugins."io.containerd.grpc.v1.cri".cni]
      bin_dir = '/cm/local/apps/kubernetes/current/bin/cni'
    [plugins."io.containerd.grpc.v1.cri".registry]
      config_path = '/cm/local/apps/containerd/var/etc/certs.d'

Then containerd would indeed have both configuration changes. But the two files had to be merged into one.

Proposed behavior

We think it would be more convenient if every component can just focus on writing their own drop-in configuration for containerd. The change in this PR should fix just that, and the two files cni.toml and registry.toml actually do result in the following.

[cni]
  bin_dir = '/cm/local/apps/kubernetes/current/bin/cni'
[registry]
  config_path = '/cm/local/apps/containerd/var/etc/certs.d'

This example is already a real example, we have certain conditions that could result in either file to be written cni.toml and/or registry.toml, or none. But we agree it's a trivial one, but it makes a nice example to start with.

Motivation

The NVIDIA GPU operator and Kata Containers write their own drop-in configuration file for containerd.
These also cannot co-exist currently in 2.0.0 without this suggested change. We'd have to again merge these toml files together, which gets into a complicated mess. In our case each Kubernetes operator typically configure their hosts with a drop-in configuration file for containerd using an init container.

Conclusion

We're currently at containerd version 1.7.x, but at some point we'll start using 2.0.x, and then it would be really great if we could let go of our patch(es) and just use the upstream containerd from that point on.

Please let me know if this PR is an improvement compared to the old one. And whether or not we can get this merged for 2.0.x.?

In other PRs, such as my old PR, there seems to be agreement on the usefulness of this change, and at the same time some understandably hesitation, but now that there is a 2.0 release on the horizon, it is perhaps 'safer' to make this change?

[k8s-ci-robot]

Hi @rayburgemeestre. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dmcgowan]

Can you add to the test the cases for maps (such as runtimes) and arrays (such as pod_annotations). Those are the areas I think we should have the merging behavior specifically documented and tests to show it is consistent.

[samuelkarp]

/ok-to-test

[rayburgemeestre]

Certainly, I've added a commit, hopefully it comes close to what you had in mind @dmcgowan. If not please let me know 😅

[ktock]

Could you fix linter errors?