confidential-containers / operator

Operator to deploy confidential containers runtime
Apache License 2.0
102 stars 56 forks source link

pre-install-payload: avoid using `sed` to edit the containerd's configuration.toml #304

Open wainersm opened 7 months ago

wainersm commented 7 months ago

This is an idea that @stevenhorsman brought on our CI working group meeting today, inspired on recent changes of kata-deploy to adopt the tomlq tool (see https://github.com/kata-containers/kata-containers/commit/dd9f5b07b9243ca152c3d7fe9df472e2f31eb103).

In commit https://github.com/confidential-containers/operator/commit/603a5550f70a0f126285b2d63ac87edc992ab840 we introduced a handler for the disable_snapshot_annotations in containerd's configuration.toml (see https://github.com/confidential-containers/operator/blob/main/install/pre-install-payload/scripts/reqs-deploy.sh#L188). This handler is fairly complex and error prone because it uses grep/sed commands. A better alternative would be use a specific tool for editing toml like tomlq.

If we adopt tomlq then it will be needed to get it installed in pre-install-payload image (https://github.com/confidential-containers/operator/blob/main/install/pre-install-payload/Dockerfile).

Cc @fidencio @mkulke

wainersm commented 7 months ago

An alternative is to re-write https://github.com/confidential-containers/operator/blob/main/install/pre-install-payload/scripts/reqs-deploy.sh in either Go or Rust which should have libs for editing toml files.

mkulke commented 6 months ago

My suggestion wouldn't be to rewrite reqs-deploy.sh but have a small, dedicated CLI tool to adjust the the containerd configuration (instead of relying on sed or yq or other tools). The rust project uses toml-edit for the cargo command.

So, specifically the logic in: remove_nydus_snapshotter_from_containerd + configure_nydus_snapshotter_for_containerd. We can write unit-tests for it, have nice error messages, etc.

fidencio commented 6 months ago

So, specifically the logic in: remove_nydus_snapshotter_from_containerd + configure_nydus_snapshotter_for_containerd. We can write unit-tests for it, have nice error messages, etc.

I'm not against that, but we end up with yet another tomlq / toml editor.. I'd prefer, I think, improving one of the 7201 options that are not working well for our needs.

mkulke commented 6 months ago

I'm not against that, but we end up with yet another tomlq / toml editor.. I'd prefer, I think, improving one of the 7201 options that are not working well for our needs.

I don't think that would be case. the tools are largely ok, imo. the problem is how they are used. We could indeed, instead of using compiled code + a library, write a bash function invoking a cli tool comprehensively w/ tests, ci, fixtures, proper error handling, whatnot. In practice this is not a lot of fun to do in bash, so it's not done.

mkulke commented 6 months ago

note: there is containerd config dump which dumps the content of the current configuration, including the resolved imports.

I wrote a snippet that would illustrate how a dedicated tool for changing the containerd config could look like #313

if you look at the number of panics() and expects() it becomes obvious how many code paths there are for such a seemingly simple operation